From 895b386b6751afd3d068155f4ea71e762297672a Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Thu, 24 Oct 2019 00:24:24 +0200 Subject: [PATCH] Avoid unnecessary re-writes of video assets if they are staying the same (#1638). This is particularly useful as it avoids the hard-link-breaking copy step which is necessary if you're going to re-write the video asset with new IDs. --- src/lib/reel_writer.cc | 111 ++++++++++++++++++++++++--------------- src/lib/reel_writer.h | 3 +- test/reel_writer_test.cc | 66 +++++++++++++++++++++++ 3 files changed, 136 insertions(+), 44 deletions(-) diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc index eeea48e42..116761dab 100644 --- a/src/lib/reel_writer.cc +++ b/src/lib/reel_writer.cc @@ -78,36 +78,60 @@ ReelWriter::ReelWriter ( , _content_summary (content_summary) , _job (job) { - /* Create our picture asset in a subdirectory, named according to those - film's parameters which affect the video output. We will hard-link - it into the DCP later. + /* Create or find our picture asset in a subdirectory, named + according to those film's parameters which affect the video + output. We will hard-link it into the DCP later. */ dcp::Standard const standard = _film->interop() ? dcp::INTEROP : dcp::SMPTE; - if (_film->three_d ()) { - _picture_asset.reset (new dcp::StereoPictureAsset (dcp::Fraction (_film->video_frame_rate(), 1), standard)); - } else { - _picture_asset.reset (new dcp::MonoPictureAsset (dcp::Fraction (_film->video_frame_rate(), 1), standard)); - } + boost::filesystem::path const asset = + _film->internal_video_asset_dir() / _film->internal_video_asset_filename(_period); - _picture_asset->set_size (_film->frame_size ()); + _first_nonexistant_frame = check_existing_picture_asset (asset); - if (_film->encrypted ()) { - _picture_asset->set_key (_film->key ()); - _picture_asset->set_context_id (_film->context_id ()); - } + if (_first_nonexistant_frame < period.duration().frames_round(_film->video_frame_rate())) { + /* We do not have a complete picture asset. If there is an + existing asset, break any hard links to it as we are about + to change its contents (if only by changing the IDs); see + #1126. + */ + if (boost::filesystem::exists(asset) && boost::filesystem::hard_link_count(asset) > 1) { + if (job) { + job->sub (_("Copying old video file")); + copy_in_bits (asset, asset.string() + ".tmp", bind(&Job::set_progress, job.get(), _1, false)); + } else { + boost::filesystem::copy_file (asset, asset.string() + ".tmp"); + } + boost::filesystem::remove (asset); + boost::filesystem::rename (asset.string() + ".tmp", asset); + } - _picture_asset->set_file ( - _film->internal_video_asset_dir() / _film->internal_video_asset_filename(_period) - ); - _first_nonexistant_frame = check_existing_picture_asset (); + if (_film->three_d ()) { + _picture_asset.reset (new dcp::StereoPictureAsset(dcp::Fraction(_film->video_frame_rate(), 1), standard)); + } else { + _picture_asset.reset (new dcp::MonoPictureAsset(dcp::Fraction(_film->video_frame_rate(), 1), standard)); + } - _picture_asset_writer = _picture_asset->start_write ( - _film->internal_video_asset_dir() / _film->internal_video_asset_filename(_period), - _first_nonexistant_frame > 0 - ); + _picture_asset->set_size (_film->frame_size()); + + if (_film->encrypted ()) { + _picture_asset->set_key (_film->key()); + _picture_asset->set_context_id (_film->context_id()); + } + + _picture_asset->set_file (asset); + _picture_asset_writer = _picture_asset->start_write (asset, _first_nonexistant_frame > 0); + } else { + /* We already have a complete picture asset that we can just re-use */ + /* XXX: what about if the encryption key changes? */ + if (_film->three_d ()) { + _picture_asset.reset (new dcp::StereoPictureAsset(asset)); + } else { + _picture_asset.reset (new dcp::MonoPictureAsset(asset)); + } + } if (_film->audio_channels ()) { _sound_asset.reset ( @@ -174,28 +198,10 @@ ReelWriter::frame_info_position (Frame frame, Eyes eyes) const } Frame -ReelWriter::check_existing_picture_asset () +ReelWriter::check_existing_picture_asset (boost::filesystem::path asset) { - DCPOMATIC_ASSERT (_picture_asset->file()); - boost::filesystem::path asset = _picture_asset->file().get(); - shared_ptr job = _job.lock (); - /* If there is an existing asset, break any hard links to it as we are about to change its contents - (if only by changing the IDs); see #1126. - */ - - if (boost::filesystem::exists(asset) && boost::filesystem::hard_link_count(asset) > 1) { - if (job) { - job->sub (_("Copying old video file")); - copy_in_bits (asset, asset.string() + ".tmp", bind(&Job::set_progress, job.get(), _1, false)); - } else { - boost::filesystem::copy_file (asset, asset.string() + ".tmp"); - } - boost::filesystem::remove (asset); - boost::filesystem::rename (asset.string() + ".tmp", asset); - } - if (job) { job->sub (_("Checking existing image data")); } @@ -252,6 +258,11 @@ ReelWriter::check_existing_picture_asset () void ReelWriter::write (optional encoded, Frame frame, Eyes eyes) { + if (!_picture_asset_writer) { + /* We're not writing any data */ + return; + } + dcp::FrameInfo fin = _picture_asset_writer->write (encoded->data().get (), encoded->size()); write_frame_info (frame, eyes, fin); _last_written[eyes] = encoded; @@ -262,6 +273,11 @@ ReelWriter::write (optional encoded, Frame frame, Eyes eyes) void ReelWriter::fake_write (Frame frame, Eyes eyes, int size) { + if (!_picture_asset_writer) { + /* We're not writing any data */ + return; + } + _picture_asset_writer->fake_write (size); _last_written_video_frame = frame; _last_written_eyes = eyes; @@ -270,6 +286,11 @@ ReelWriter::fake_write (Frame frame, Eyes eyes, int size) void ReelWriter::repeat_write (Frame frame, Eyes eyes) { + if (!_picture_asset_writer) { + /* We're not writing any data */ + return; + } + dcp::FrameInfo fin = _picture_asset_writer->write ( _last_written[eyes]->data().get(), _last_written[eyes]->size() @@ -282,7 +303,7 @@ ReelWriter::repeat_write (Frame frame, Eyes eyes) void ReelWriter::finish () { - if (!_picture_asset_writer->finalize ()) { + if (_picture_asset_writer && !_picture_asset_writer->finalize ()) { /* Nothing was written to the picture asset */ LOG_GENERAL ("Nothing was written to reel %1 of %2", _reel_index, _reel_count); _picture_asset.reset (); @@ -300,8 +321,12 @@ ReelWriter::finish () boost::filesystem::path video_to; video_to /= _film->dir (_film->dcp_name()); video_to /= video_asset_filename (_picture_asset, _reel_index, _reel_count, _content_summary); - + /* There may be an existing "to" file if we are recreating a DCP in the same place without + changing any video. + */ boost::system::error_code ec; + boost::filesystem::remove (video_to, ec); + boost::filesystem::create_hard_link (video_from, video_to, ec); if (ec) { LOG_WARNING_NC ("Hard-link failed; copying instead"); @@ -317,7 +342,7 @@ ReelWriter::finish () } else { boost::filesystem::copy_file (video_from, video_to, ec); if (ec) { - LOG_ERROR ("Failed to copy video file from %1 to %2 (%3)", video_from.string(), video_to.string(), ec.message ()); + LOG_ERROR ("Failed to copy video file from %1 to %2 (%3)", video_from.string(), video_to.string(), ec.message()); throw FileError (ec.message(), video_from); } } diff --git a/src/lib/reel_writer.h b/src/lib/reel_writer.h index 46f477616..d241c0fac 100644 --- a/src/lib/reel_writer.h +++ b/src/lib/reel_writer.h @@ -99,7 +99,7 @@ private: 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 (); + Frame check_existing_picture_asset (boost::filesystem::path asset); bool existing_picture_frame_ok (FILE* asset_file, boost::shared_ptr info_file, Frame frame) const; boost::shared_ptr _film; @@ -120,6 +120,7 @@ private: boost::weak_ptr _job; boost::shared_ptr _picture_asset; + /** picture asset writer, or 0 if we are not writing any picture because we already have one */ boost::shared_ptr _picture_asset_writer; boost::shared_ptr _sound_asset; boost::shared_ptr _sound_asset_writer; diff --git a/test/reel_writer_test.cc b/test/reel_writer_test.cc index 1b4b62522..5177a5c2f 100644 --- a/test/reel_writer_test.cc +++ b/test/reel_writer_test.cc @@ -26,7 +26,16 @@ #include "lib/reel_writer.h" #include "lib/film.h" #include "lib/cross.h" +#include "lib/content_factory.h" +#include "lib/content.h" +#include "lib/audio_content.h" +#include "lib/video_content.h" #include "test.h" +#include +#include +#include +#include +#include #include using std::string; @@ -87,3 +96,60 @@ BOOST_AUTO_TEST_CASE (write_frame_info_test) BOOST_CHECK (equal(info3, writer, file, 10, EYES_LEFT)); } } + +/** Check that the reel writer correctly re-uses a video asset changed if we remake + * a DCP with no video changes. + */ +BOOST_AUTO_TEST_CASE (reel_reuse_video_test) +{ + /* Make a DCP */ + shared_ptr film = new_test_film2 ("reel_reuse_video_test"); + shared_ptr video = content_factory("test/data/flat_red.png").front(); + film->examine_and_add_content (video); + BOOST_REQUIRE (!wait_for_jobs()); + shared_ptr audio = content_factory("test/data/white.wav").front(); + film->examine_and_add_content (audio); + BOOST_REQUIRE (!wait_for_jobs()); + film->make_dcp (); + BOOST_REQUIRE (!wait_for_jobs()); + + /* Find main picture and sound asset IDs */ + dcp::DCP dcp1 (film->dir(film->dcp_name())); + dcp1.read (); + BOOST_REQUIRE_EQUAL (dcp1.cpls().size(), 1); + BOOST_REQUIRE_EQUAL (dcp1.cpls().front()->reels().size(), 1); + BOOST_REQUIRE (dcp1.cpls().front()->reels().front()->main_picture()); + BOOST_REQUIRE (dcp1.cpls().front()->reels().front()->main_sound()); + string const picture_id = dcp1.cpls().front()->reels().front()->main_picture()->asset()->id(); + string const sound_id = dcp1.cpls().front()->reels().front()->main_sound()->asset()->id(); + + /* Change the audio and re-make */ + audio->audio->set_gain (-3); + film->make_dcp (); + BOOST_REQUIRE (!wait_for_jobs()); + + /* Video ID should be the same, sound different */ + dcp::DCP dcp2 (film->dir(film->dcp_name())); + dcp2.read (); + BOOST_REQUIRE_EQUAL (dcp2.cpls().size(), 1); + BOOST_REQUIRE_EQUAL (dcp2.cpls().front()->reels().size(), 1); + BOOST_REQUIRE (dcp2.cpls().front()->reels().front()->main_picture()); + BOOST_REQUIRE (dcp2.cpls().front()->reels().front()->main_sound()); + BOOST_CHECK_EQUAL (picture_id, dcp2.cpls().front()->reels().front()->main_picture()->asset()->id()); + BOOST_CHECK (sound_id != dcp2.cpls().front()->reels().front()->main_sound()->asset()->id()); + + /* Crop video and re-make */ + video->video->set_left_crop (5); + film->make_dcp (); + BOOST_REQUIRE (!wait_for_jobs()); + + /* Video and sound IDs should be different */ + dcp::DCP dcp3 (film->dir(film->dcp_name())); + dcp3.read (); + BOOST_REQUIRE_EQUAL (dcp3.cpls().size(), 1); + BOOST_REQUIRE_EQUAL (dcp3.cpls().front()->reels().size(), 1); + BOOST_REQUIRE (dcp3.cpls().front()->reels().front()->main_picture()); + BOOST_REQUIRE (dcp3.cpls().front()->reels().front()->main_sound()); + BOOST_CHECK (picture_id != dcp3.cpls().front()->reels().front()->main_picture()->asset()->id()); + BOOST_CHECK (sound_id != dcp3.cpls().front()->reels().front()->main_sound()->asset()->id()); +} -- 2.30.2