From 93e89bd463bd51de6823a6796288f6283f885b06 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Sun, 29 Sep 2019 23:28:57 +0200 Subject: [PATCH] Improve OpenFileError so that it doesn't say "opening for read" in one case where it should say "opening for read/write". Also add some unit tests for ReelWriter. --- src/lib/exceptions.cc | 5 +- src/lib/exceptions.h | 10 +++- src/lib/ffmpeg.cc | 2 +- src/lib/ffmpeg_image_proxy.cc | 2 +- src/lib/file_group.cc | 2 +- src/lib/reel_writer.cc | 8 +++- src/lib/reel_writer.h | 3 ++ src/lib/util.cc | 4 +- src/lib/writer.cc | 2 +- src/wx/config_dialog.cc | 12 ++--- test/reel_writer_test.cc | 88 +++++++++++++++++++++++++++++++++++ test/wscript | 1 + 12 files changed, 121 insertions(+), 18 deletions(-) create mode 100644 test/reel_writer_test.cc diff --git a/src/lib/exceptions.cc b/src/lib/exceptions.cc index 481d2e89d..bf474e3ea 100644 --- a/src/lib/exceptions.cc +++ b/src/lib/exceptions.cc @@ -27,10 +27,11 @@ using std::string; using std::runtime_error; /** @param f File that we were trying to open */ -OpenFileError::OpenFileError (boost::filesystem::path f, int error, bool reading) +OpenFileError::OpenFileError (boost::filesystem::path f, int error, Mode mode) : FileError ( String::compose ( - reading ? _("could not open file %1 for reading (%2)") : _("could not open file %1 for writing (%2)"), + mode == READ_WRITE ? _("could not open file %1 for read/write (%2)") : + (mode == READ ? _("could not open file %1 for read (%2)") : _("could not open file %1 for write (%2)")), f.string(), error), f diff --git a/src/lib/exceptions.h b/src/lib/exceptions.h index fe87ababc..766d3d8d5 100644 --- a/src/lib/exceptions.h +++ b/src/lib/exceptions.h @@ -103,11 +103,17 @@ public: class OpenFileError : public FileError { public: + enum Mode { + READ, + WRITE, + READ_WRITE + }; + /** @param f File that we were trying to open. * @param error Code of error that occurred. - * @param reading true if we were opening to read, false if opening to write. + * @param mode Mode that we tried to open the file in. */ - OpenFileError (boost::filesystem::path f, int error, bool reading); + OpenFileError (boost::filesystem::path f, int error, Mode mode); }; /** @class ReadFileError. diff --git a/src/lib/ffmpeg.cc b/src/lib/ffmpeg.cc index fb3437910..f26d1d2cf 100644 --- a/src/lib/ffmpeg.cc +++ b/src/lib/ffmpeg.cc @@ -128,7 +128,7 @@ FFmpeg::setup_general () int e = avformat_open_input (&_format_context, 0, 0, &options); if (e < 0) { - throw OpenFileError (_ffmpeg_content->path(0).string(), e, true); + throw OpenFileError (_ffmpeg_content->path(0).string(), e, OpenFileError::READ); } if (avformat_find_stream_info (_format_context, 0) < 0) { diff --git a/src/lib/ffmpeg_image_proxy.cc b/src/lib/ffmpeg_image_proxy.cc index fa6cb288d..9c91d1d87 100644 --- a/src/lib/ffmpeg_image_proxy.cc +++ b/src/lib/ffmpeg_image_proxy.cc @@ -147,7 +147,7 @@ FFmpegImageProxy::image (optional) const } if (e < 0) { if (_path) { - throw OpenFileError (_path->string(), e, true); + throw OpenFileError (_path->string(), e, OpenFileError::READ); } else { boost::throw_exception(DecodeError(String::compose(_("Could not decode image (%1)"), e))); } diff --git a/src/lib/file_group.cc b/src/lib/file_group.cc index 942eb435d..3e8a7b79c 100644 --- a/src/lib/file_group.cc +++ b/src/lib/file_group.cc @@ -93,7 +93,7 @@ FileGroup::ensure_open_path (size_t p) const _current_path = p; _current_file = fopen_boost (_paths[_current_path], "rb"); if (_current_file == 0) { - throw OpenFileError (_paths[_current_path], errno, true); + throw OpenFileError (_paths[_current_path], errno, OpenFileError::READ); } } diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc index b7ccc07ce..355fe5c3b 100644 --- a/src/lib/reel_writer.cc +++ b/src/lib/reel_writer.cc @@ -62,6 +62,7 @@ using dcp::raw_convert; int const ReelWriter::_info_size = 48; +/** @param job Related job, or 0 */ ReelWriter::ReelWriter ( shared_ptr film, DCPTimePeriod period, shared_ptr job, int reel_index, int reel_count, optional content_summary ) @@ -97,7 +98,9 @@ ReelWriter::ReelWriter ( _film->internal_video_asset_dir() / _film->internal_video_asset_filename(_period) ); - job->sub (_("Checking existing image data")); + if (job) { + job->sub (_("Checking existing image data")); + } _first_nonexistant_frame = check_existing_picture_asset (); _picture_asset_writer = _picture_asset->start_write ( @@ -139,8 +142,9 @@ ReelWriter::write_frame_info (Frame frame, Eyes eyes, dcp::FrameInfo info) const } else { file = fopen_boost (info_file, "wb"); } + if (!file) { - throw OpenFileError (info_file, errno, read); + throw OpenFileError (info_file, errno, read ? OpenFileError::READ_WRITE : OpenFileError::WRITE); } dcpomatic_fseek (file, frame_info_position (frame, eyes), SEEK_SET); checked_fwrite (&info.offset, sizeof (info.offset), file, info_file); diff --git a/src/lib/reel_writer.h b/src/lib/reel_writer.h index ae64c3ac7..741b0914c 100644 --- a/src/lib/reel_writer.h +++ b/src/lib/reel_writer.h @@ -30,6 +30,7 @@ class Film; class Job; class Font; class AudioBuffers; +struct write_frame_info_test; namespace dcp { class MonoPictureAsset; @@ -89,6 +90,8 @@ public: private: + friend struct ::write_frame_info_test; + void write_frame_info (Frame frame, Eyes eyes, dcp::FrameInfo info) const; long frame_info_position (Frame frame, Eyes eyes) const; Frame check_existing_picture_asset (); diff --git a/src/lib/util.cc b/src/lib/util.cc index 340af1ea8..a17ac54f5 100644 --- a/src/lib/util.cc +++ b/src/lib/util.cc @@ -471,7 +471,7 @@ digest_head_tail (vector files, boost::uintmax_t size) while (i < int64_t (files.size()) && to_do > 0) { FILE* f = fopen_boost (files[i], "rb"); if (!f) { - throw OpenFileError (files[i].string(), errno, true); + throw OpenFileError (files[i].string(), errno, OpenFileError::READ); } boost::uintmax_t this_time = min (to_do, boost::filesystem::file_size (files[i])); @@ -491,7 +491,7 @@ digest_head_tail (vector files, boost::uintmax_t size) while (i >= 0 && to_do > 0) { FILE* f = fopen_boost (files[i], "rb"); if (!f) { - throw OpenFileError (files[i].string(), errno, true); + throw OpenFileError (files[i].string(), errno, OpenFileError::READ); } boost::uintmax_t this_time = min (to_do, boost::filesystem::file_size (files[i])); diff --git a/src/lib/writer.cc b/src/lib/writer.cc index d0f0825f1..8666d90c8 100644 --- a/src/lib/writer.cc +++ b/src/lib/writer.cc @@ -584,7 +584,7 @@ Writer::write_cover_sheet () boost::filesystem::path const cover = _film->file ("COVER_SHEET.txt"); FILE* f = fopen_boost (cover, "w"); if (!f) { - throw OpenFileError (cover, errno, false); + throw OpenFileError (cover, errno, OpenFileError::WRITE); } string text = Config::instance()->cover_sheet (); diff --git a/src/wx/config_dialog.cc b/src/wx/config_dialog.cc index 743953650..fecb55ea7 100644 --- a/src/wx/config_dialog.cc +++ b/src/wx/config_dialog.cc @@ -578,7 +578,7 @@ CertificateChainEditor::export_certificate () boost::filesystem::path path (wx_to_std(d->GetPath())); FILE* f = fopen_boost (path, "w"); if (!f) { - throw OpenFileError (path, errno, false); + throw OpenFileError (path, errno, OpenFileError::WRITE); } string const s = j->certificate (true); @@ -600,7 +600,7 @@ CertificateChainEditor::export_chain () boost::filesystem::path path (wx_to_std(d->GetPath())); FILE* f = fopen_boost (path, "w"); if (!f) { - throw OpenFileError (path, errno, false); + throw OpenFileError (path, errno, OpenFileError::WRITE); } string const s = _get()->chain(); @@ -774,7 +774,7 @@ CertificateChainEditor::export_private_key () boost::filesystem::path path (wx_to_std(d->GetPath())); FILE* f = fopen_boost (path, "w"); if (!f) { - throw OpenFileError (path, errno, false); + throw OpenFileError (path, errno, OpenFileError::WRITE); } string const s = _get()->key().get (); @@ -867,7 +867,7 @@ KeysPage::export_decryption_chain_and_key () boost::filesystem::path path (wx_to_std(d->GetPath())); FILE* f = fopen_boost (path, "w"); if (!f) { - throw OpenFileError (path, errno, false); + throw OpenFileError (path, errno, OpenFileError::WRITE); } string const chain = Config::instance()->decryption_chain()->chain(); @@ -902,7 +902,7 @@ KeysPage::import_decryption_chain_and_key () FILE* f = fopen_boost (wx_to_std (d->GetPath ()), "r"); if (!f) { - throw OpenFileError (wx_to_std (d->GetPath ()), errno, false); + throw OpenFileError (wx_to_std (d->GetPath ()), errno, OpenFileError::WRITE); } string current; @@ -954,7 +954,7 @@ KeysPage::export_decryption_certificate () boost::filesystem::path path (wx_to_std(d->GetPath())); FILE* f = fopen_boost (path, "w"); if (!f) { - throw OpenFileError (path, errno, false); + throw OpenFileError (path, errno, OpenFileError::WRITE); } string const s = Config::instance()->decryption_chain()->leaf().certificate (true); diff --git a/test/reel_writer_test.cc b/test/reel_writer_test.cc new file mode 100644 index 000000000..db63ca8bf --- /dev/null +++ b/test/reel_writer_test.cc @@ -0,0 +1,88 @@ +/* + Copyright (C) 2019 Carl Hetherington + + This file is part of DCP-o-matic. + + DCP-o-matic is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + DCP-o-matic is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with DCP-o-matic. If not, see . + +*/ + +/** @file test/reel_writer_test.cc + * @brief Test ReelWriter class. + * @ingroup selfcontained + */ + +#include "lib/reel_writer.h" +#include "lib/film.h" +#include "lib/cross.h" +#include "test.h" +#include + +using std::string; +using boost::shared_ptr; +using boost::optional; + +static bool equal (dcp::FrameInfo a, ReelWriter const & writer, boost::filesystem::path file, Frame frame, Eyes eyes) +{ + FILE* f = fopen_boost(file, "rb"); + BOOST_REQUIRE (f); + dcp::FrameInfo b = writer.read_frame_info(f, frame, eyes); + bool const r = a.offset == b.offset && a.size == b.size && a.hash == b.hash; + fclose (f); + return r; +} + +BOOST_AUTO_TEST_CASE (write_frame_info_test) +{ + shared_ptr film = new_test_film2 ("write_frame_info_test"); + dcpomatic::DCPTimePeriod const period (dcpomatic::DCPTime(0), dcpomatic::DCPTime(96000)); + ReelWriter writer (film, period, shared_ptr(), 0, 1, optional()); + + /* Write the first one */ + + boost::filesystem::path file = film->info_file (period); + BOOST_CHECK (!boost::filesystem::exists(file)); + dcp::FrameInfo info1(0, 123, "12345678901234567890123456789012"); + writer.write_frame_info (0, EYES_LEFT, info1); + BOOST_CHECK (boost::filesystem::exists(file)); + + BOOST_CHECK (equal(info1, writer, file, 0, EYES_LEFT)); + + /* Write some more */ + + dcp::FrameInfo info2(596, 14921, "123acb789f1234ae782012n456339522"); + writer.write_frame_info (5, EYES_RIGHT, info2); + BOOST_CHECK (boost::filesystem::exists(file)); + + BOOST_CHECK (equal(info1, writer, file, 0, EYES_LEFT)); + BOOST_CHECK (equal(info2, writer, file, 5, EYES_RIGHT)); + + dcp::FrameInfo info3(12494, 99157123, "xxxxyyyyabc12356ffsfdsf456339522"); + writer.write_frame_info (10, EYES_LEFT, info3); + BOOST_CHECK (boost::filesystem::exists(file)); + + BOOST_CHECK (equal(info1, writer, file, 0, EYES_LEFT)); + BOOST_CHECK (equal(info2, writer, file, 5, EYES_RIGHT)); + BOOST_CHECK (equal(info3, writer, file, 10, EYES_LEFT)); + + /* Overwrite one */ + + dcp::FrameInfo info4(55512494, 123599157123, "ABCDEFGyabc12356ffsfdsf4563395ZZ"); + writer.write_frame_info (5, EYES_RIGHT, info4); + BOOST_CHECK (boost::filesystem::exists(file)); + + BOOST_CHECK (equal(info1, writer, file, 0, EYES_LEFT)); + BOOST_CHECK (equal(info4, writer, file, 5, EYES_RIGHT)); + BOOST_CHECK (equal(info3, writer, file, 10, EYES_LEFT)); +} diff --git a/test/wscript b/test/wscript index 5e7634ea3..db773fb46 100644 --- a/test/wscript +++ b/test/wscript @@ -94,6 +94,7 @@ def build(bld): recover_test.cc rect_test.cc reels_test.cc + reel_writer_test.cc required_disk_space_test.cc remake_id_test.cc remake_with_subtitle_test.cc -- 2.30.2