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.

diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc
index eeea48e42979330322bb600793370bc147cdf122..116761dab857b269528fb1b7f558ec1abc587964 100644 (file)
--- 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 = _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<Data> 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<Data> 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 46f47761622a0afae5424374af8b9e45bc97d8f5..d241c0fac16c067336179170050b99a427d66d4b 100644 (file)
--- 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<InfoFileHandle> info_file, Frame frame) const;
 
        boost::shared_ptr<const Film> _film;
@@ -120,6 +120,7 @@ private:
        boost::weak_ptr<Job> _job;
 
        boost::shared_ptr<dcp::PictureAsset> _picture_asset;
+       /** picture asset writer, or 0 if we are not writing any picture because we already have one */
        boost::shared_ptr<dcp::PictureAssetWriter> _picture_asset_writer;
        boost::shared_ptr<dcp::SoundAsset> _sound_asset;
        boost::shared_ptr<dcp::SoundAssetWriter> _sound_asset_writer;

diff --git a/test/reel_writer_test.cc b/test/reel_writer_test.cc
index 1b4b62522a4c5ca40c4dee24ef87f4bd2a6a26a7..5177a5c2f6eacb174706911ce256122af988c54c 100644 (file)
--- 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 <dcp/dcp.h>
+#include <dcp/cpl.h>
+#include <dcp/reel.h>
+#include <dcp/reel_picture_asset.h>
+#include <dcp/reel_sound_asset.h>
 #include <boost/test/unit_test.hpp>
 
 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> film = new_test_film2 ("reel_reuse_video_test");
+       shared_ptr<Content> video = content_factory("test/data/flat_red.png").front();
+       film->examine_and_add_content (video);
+       BOOST_REQUIRE (!wait_for_jobs());
+       shared_ptr<Content> 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());
+}