Give ownership of info files to ReelWriters (#2912).

The motivation here is to stop a pattern where we create a file, close
it, and then re-open it (many times) as I think there are problems on
Windows when a virus scanner sees the new file, opens it for checking,
and then we can't re-open it.

This also makes things a fair bit simpler, as a lock is removed and we
don't try to differentiate read/write cases by opening the file in
different ways; it's now always writeable.

diff --git a/src/lib/film.cc b/src/lib/film.cc
index 1258cdb112fd860a5fccac4742038f7811cf569e..1ac519011ac970635232065b8b508bedce48c838 100644 (file)
--- a/src/lib/film.cc
+++ b/src/lib/film.cc
@@ -2235,21 +2235,6 @@ Film::marker (dcp::Marker type) const
        return i->second;
 }
 
-shared_ptr<InfoFileHandle>
-Film::info_file_handle (DCPTimePeriod period, bool read) const
-{
-       return std::make_shared<InfoFileHandle>(_info_file_mutex, info_file(period), read);
-}
-
-InfoFileHandle::InfoFileHandle (boost::mutex& mutex, boost::filesystem::path path, bool read)
-       : _lock (mutex)
-       , _file(path, read ? "rb" : (dcp::filesystem::exists(path) ? "r+b" : "wb"))
-{
-       if (!_file) {
-               throw OpenFileError(path, _file.open_error(), read ? OpenFileError::READ : (dcp::filesystem::exists(path) ? OpenFileError::READ_WRITE : OpenFileError::WRITE));
-       }
-}
-
 
 /** Add FFOC and LFOC markers to a list if they are not already there */
 void

diff --git a/src/lib/film.h b/src/lib/film.h
index 0c6a1943e44c69673a3502ab4bb3b8d4be18a16a..648d9828b79617b417b72f3b61950f24ce833645 100644 (file)
--- a/src/lib/film.h
+++ b/src/lib/film.h
@@ -84,28 +84,6 @@ struct ov_subs_in_vf_name;
 struct recover_test_2d_encrypted;
 
 
-class InfoFileHandle
-{
-public:
-       InfoFileHandle (boost::mutex& mutex, boost::filesystem::path file, bool read);
-
-       InfoFileHandle(InfoFileHandle const&) = delete;
-       InfoFileHandle& operator=(InfoFileHandle const&) = delete;
-       InfoFileHandle(InfoFileHandle&&) = delete;
-       InfoFileHandle& operator=(InfoFileHandle&&) = delete;
-
-       dcp::File& get () {
-               return _file;
-       }
-
-private:
-       friend class Film;
-
-       boost::mutex::scoped_lock _lock;
-       dcp::File _file;
-};
-
-
 /** @class Film
  *
  *  @brief A representation of some audio, video, subtitle and closed-caption content,
@@ -122,7 +100,6 @@ public:
        Film (Film const&) = delete;
        Film& operator= (Film const&) = delete;
 
-       std::shared_ptr<InfoFileHandle> info_file_handle (dcpomatic::DCPTimePeriod period, bool read) const;
        boost::filesystem::path j2c_path (int, Frame, Eyes, bool) const;
 
        boost::filesystem::path audio_analysis_path (std::shared_ptr<const Playlist>) const;
@@ -594,8 +571,6 @@ private:
 
        std::map<std::string, std::string> _ui_state;
 
-       mutable boost::mutex _info_file_mutex;
-
        boost::signals2::scoped_connection _playlist_change_connection;
        boost::signals2::scoped_connection _playlist_order_changed_connection;
        boost::signals2::scoped_connection _playlist_content_change_connection;

diff --git a/src/lib/frame_info.cc b/src/lib/frame_info.cc
index f348bca6a98a5e2347d8ce3c66a43c9e5ecf436a..e2382376cbc2b4886e59a625f0b4a912c44e3d82 100644 (file)
--- a/src/lib/frame_info.cc
+++ b/src/lib/frame_info.cc
@@ -41,14 +41,14 @@ J2KFrameInfo::J2KFrameInfo(dcp::J2KFrameInfo const& info)
 }
 
 
-J2KFrameInfo::J2KFrameInfo(shared_ptr<InfoFileHandle> info_file, Frame frame, Eyes eyes)
+J2KFrameInfo::J2KFrameInfo(dcp::File& info_file, Frame frame, Eyes eyes)
 {
-       info_file->get().seek(position(frame, eyes), SEEK_SET);
-       info_file->get().checked_read(&offset, sizeof(offset));
-       info_file->get().checked_read(&size, sizeof(size));
+       info_file.seek(position(frame, eyes), SEEK_SET);
+       info_file.checked_read(&offset, sizeof(offset));
+       info_file.checked_read(&size, sizeof(size));
 
        char hash_buffer[33];
-       info_file->get().checked_read(hash_buffer, 32);
+       info_file.checked_read(hash_buffer, 32);
        hash_buffer[32] = '\0';
        hash = hash_buffer;
 }
@@ -74,11 +74,11 @@ J2KFrameInfo::position(Frame frame, Eyes eyes) const
 
 /** @param frame reel-relative frame */
 void
-J2KFrameInfo::write(shared_ptr<InfoFileHandle> info_file, Frame frame, Eyes eyes) const
+J2KFrameInfo::write(dcp::File& info_file, Frame frame, Eyes eyes) const
 {
-       info_file->get().seek(position(frame, eyes), SEEK_SET);
-       info_file->get().checked_write(&offset, sizeof(offset));
-       info_file->get().checked_write(&size, sizeof(size));
-       info_file->get().checked_write(hash.c_str(), hash.size());
+       info_file.seek(position(frame, eyes), SEEK_SET);
+       info_file.checked_write(&offset, sizeof(offset));
+       info_file.checked_write(&size, sizeof(size));
+       info_file.checked_write(hash.c_str(), hash.size());
 }
 

diff --git a/src/lib/frame_info.h b/src/lib/frame_info.h
index a5a22bd9ef77cd54aa785d2b084ab931f94f0fea..da9503248ec1be284647addf2fd8cf9c2352fbb5 100644 (file)
--- a/src/lib/frame_info.h
+++ b/src/lib/frame_info.h
@@ -29,9 +29,15 @@ class J2KFrameInfo : public dcp::J2KFrameInfo
 public:
        J2KFrameInfo(dcp::J2KFrameInfo const& info);
        J2KFrameInfo(uint64_t offset_, uint64_t size_, std::string hash_);
-       J2KFrameInfo(std::shared_ptr<InfoFileHandle> info_file, Frame frame, Eyes eyes);
-
-       void write(std::shared_ptr<InfoFileHandle> info_file, Frame frame, Eyes eyes) const;
+       /** Seek to the required position in info_file and read the info for
+        *  the given frame and eyes.
+        */
+       J2KFrameInfo(dcp::File& info_file, Frame frame, Eyes eyes);
+
+       /** Seek to the required position in info_file and write the info for
+        *  the given frame and eyes.
+        */
+       void write(dcp::File& info_file, Frame frame, Eyes eyes) const;
 
        static int size_on_disk() {
                return _size_on_disk;

diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc
index 5210466b239fc56e493133bccea68f6e22fc126f..c0796e7149f801bfba52ff9ef609a0e3874903ff 100644 (file)
--- a/src/lib/reel_writer.cc
+++ b/src/lib/reel_writer.cc
@@ -115,8 +115,14 @@ ReelWriter::ReelWriter (
        , _content_summary (film()->content_summary(period))
        , _job (job)
        , _text_only (text_only)
+       , _info_file(film()->info_file(period), dcp::filesystem::exists(film()->info_file(period)) ? "r+b" : "wb")
        , _font_metrics(film()->frame_size().height)
 {
+       if (!_info_file) {
+               auto const info_file_path = film()->info_file(period);
+               throw OpenFileError(info_file_path, _info_file.open_error(), dcp::filesystem::exists(info_file_path) ? OpenFileError::READ_WRITE : OpenFileError::WRITE);
+       }
+
        _default_font = dcp::ArrayData(default_font_file());
 
        if (text_only) {
@@ -257,18 +263,9 @@ ReelWriter::check_existing_picture_asset (boost::filesystem::path asset)
                LOG_GENERAL ("Opened existing asset at %1", asset.string());
        }
 
-       shared_ptr<InfoFileHandle> info_file;
-
-       try {
-               info_file = film()->info_file_handle (_period, true);
-       } catch (OpenFileError &) {
-               LOG_GENERAL_NC ("Could not open film info file");
-               return 0;
-       }
-
        /* Offset of the last dcp::FrameInfo in the info file */
-       int const n = (dcp::filesystem::file_size(info_file->get().path()) / J2KFrameInfo::size_on_disk()) - 1;
-       LOG_GENERAL("The last FI is %1; info file is %2, info size %3", n, dcp::filesystem::file_size(info_file->get().path()), J2KFrameInfo::size_on_disk())
+       int const n = (dcp::filesystem::file_size(_info_file.path()) / J2KFrameInfo::size_on_disk()) - 1;
+       LOG_GENERAL("The last FI is %1; info file is %2, info size %3", n, dcp::filesystem::file_size(_info_file.path()), J2KFrameInfo::size_on_disk())
 
        Frame first_nonexistent_frame;
        if (film()->three_d()) {
@@ -278,7 +275,7 @@ ReelWriter::check_existing_picture_asset (boost::filesystem::path asset)
                first_nonexistent_frame = n;
        }
 
-       while (!existing_picture_frame_ok(asset_file, info_file, first_nonexistent_frame) && first_nonexistent_frame > 0) {
+       while (!existing_picture_frame_ok(asset_file, first_nonexistent_frame) && first_nonexistent_frame > 0) {
                --first_nonexistent_frame;
        }
 
@@ -304,7 +301,7 @@ ReelWriter::write (shared_ptr<const Data> encoded, Frame frame, Eyes eyes)
        }
 
        auto fin = J2KFrameInfo(_j2k_picture_asset_writer->write(encoded->data(), encoded->size()));
-       fin.write(film()->info_file_handle(_period, false), frame, eyes);
+       fin.write(_info_file, frame, eyes);
        _last_written[eyes] = encoded;
 }
 
@@ -333,14 +330,14 @@ ReelWriter::write(shared_ptr<dcp::MonoMPEG2PictureFrame> image)
 
 
 void
-ReelWriter::fake_write(dcp::J2KFrameInfo const& info)
+ReelWriter::fake_write(Frame frame, Eyes eyes)
 {
        if (!_j2k_picture_asset_writer) {
                /* We're not writing any data */
                return;
        }
 
-       _j2k_picture_asset_writer->fake_write(info);
+       _j2k_picture_asset_writer->fake_write(J2KFrameInfo(_info_file, frame, eyes));
 }
 
 
@@ -353,7 +350,7 @@ ReelWriter::repeat_write (Frame frame, Eyes eyes)
        }
 
        auto fin = J2KFrameInfo(_j2k_picture_asset_writer->write(_last_written[eyes]->data(), _last_written[eyes]->size()));
-       fin.write(film()->info_file_handle(_period, false), frame, eyes);
+       fin.write(_info_file, frame, eyes);
 }
 
 
@@ -967,14 +964,14 @@ ReelWriter::write(PlayerText subs, TextType type, optional<DCPTextTrack> track,
 
 
 bool
-ReelWriter::existing_picture_frame_ok (dcp::File& asset_file, shared_ptr<InfoFileHandle> info_file, Frame frame) const
+ReelWriter::existing_picture_frame_ok(dcp::File& asset_file, Frame frame)
 {
        LOG_GENERAL ("Checking existing picture frame %1", frame);
 
        /* Read the data from the info file; for 3D we just check the left
           frames until we find a good one.
        */
-       auto const info = J2KFrameInfo(info_file, frame, film()->three_d() ? Eyes::LEFT : Eyes::BOTH);
+       auto const info = J2KFrameInfo(_info_file, frame, film()->three_d() ? Eyes::LEFT : Eyes::BOTH);
 
        bool ok = true;
 

diff --git a/src/lib/reel_writer.h b/src/lib/reel_writer.h
index ac65311485d516195c3bbe090dacc3c28b81f0f5..02ee5f972ab2604ff020eb730866afe4e425c73f 100644 (file)
--- a/src/lib/reel_writer.h
+++ b/src/lib/reel_writer.h
@@ -75,7 +75,7 @@ public:
                );
 
        void write (std::shared_ptr<const dcp::Data> encoded, Frame frame, Eyes eyes);
-       void fake_write(dcp::J2KFrameInfo const& info);
+       void fake_write(Frame frame, Eyes eyes);
        void repeat_write (Frame frame, Eyes eyes);
        void write (std::shared_ptr<const AudioBuffers> audio);
        void write(PlayerText text, TextType type, boost::optional<DCPTextTrack> track, dcpomatic::DCPTimePeriod period, FontIdMap const& fonts, std::shared_ptr<dcpomatic::Font> chosen_interop_font);
@@ -106,7 +106,7 @@ private:
        friend struct ::write_frame_info_test;
 
        Frame check_existing_picture_asset (boost::filesystem::path asset);
-       bool existing_picture_frame_ok (dcp::File& asset_file, std::shared_ptr<InfoFileHandle> info_file, Frame frame) const;
+       bool existing_picture_frame_ok(dcp::File& asset_file, Frame frame);
        std::shared_ptr<dcp::TextAsset> empty_text_asset (TextType type, boost::optional<DCPTextTrack> track, bool with_dummy) const;
 
        std::shared_ptr<dcp::ReelPictureAsset> create_reel_picture (std::shared_ptr<dcp::Reel> reel, std::list<ReferencedReelAsset> const & refs) const;
@@ -138,6 +138,8 @@ private:
 
        dcp::ArrayData _default_font;
 
+       dcp::File _info_file;
+
        std::shared_ptr<dcp::J2KPictureAsset> _j2k_picture_asset;
        std::shared_ptr<dcp::MPEG2PictureAsset> _mpeg2_picture_asset;
        /** picture asset writer, or 0 if we are not writing any picture because we already have one */

diff --git a/src/lib/writer.cc b/src/lib/writer.cc
index 97108504657f66f44f9af4aa3eb74697721f8eca..138873cc36a854fe255c34944fc157b18eb67b5e 100644 (file)
--- a/src/lib/writer.cc
+++ b/src/lib/writer.cc
@@ -92,7 +92,7 @@ Writer::Writer(weak_ptr<const Film> weak_film, weak_ptr<Job> weak_job, boost::fi
        int reel_index = 0;
        auto const reels = film()->reels();
        for (auto p: reels) {
-               _reels.push_back(ReelWriter(weak_film, p, job, reel_index++, reels.size(), text_only, _output_dir));
+               _reels.emplace_back(weak_film, p, job, reel_index++, reels.size(), text_only, _output_dir);
        }
 
        _last_written.resize (reels.size());
@@ -237,17 +237,13 @@ Writer::fake_write (Frame frame, Eyes eyes)
                _full_condition.wait (lock);
        }
 
-       size_t const reel = video_reel (frame);
-       Frame const frame_in_reel = frame - _reels[reel].start ();
+       DCPOMATIC_ASSERT((film()->three_d() && eyes != Eyes::BOTH) || (!film()->three_d() && eyes == Eyes::BOTH));
 
        QueueItem qi;
        qi.type = QueueItem::Type::FAKE;
-       qi.info = J2KFrameInfo(film()->info_file_handle(_reels[reel].period(), true), frame_in_reel, eyes);
-
-       DCPOMATIC_ASSERT((film()->three_d() && eyes != Eyes::BOTH) || (!film()->three_d() && eyes == Eyes::BOTH));
-
+       auto const reel = video_reel(frame);
        qi.reel = reel;
-       qi.frame = frame_in_reel;
+       qi.frame = frame - _reels[reel].start();
        qi.eyes = eyes;
        _queue.push_back(qi);
 
@@ -416,7 +412,7 @@ try
                                        if (i.type == QueueItem::Type::FULL) {
                                                LOG_WARNING (N_("- type FULL, frame %1, eyes %2"), i.frame, (int) i.eyes);
                                        } else {
-                                               LOG_WARNING (N_("- type FAKE, size %1, frame %2, eyes %3"), i.info.size, i.frame, (int) i.eyes);
+                                               LOG_WARNING(N_("- type FAKE, frame %1, eyes %2"), i.frame, static_cast<int>(i.eyes));
                                        }
                                }
                        }
@@ -447,7 +443,7 @@ try
                                break;
                        case QueueItem::Type::FAKE:
                                LOG_DEBUG_ENCODE (N_("Writer FAKE-writes %1"), qi.frame);
-                               reel.fake_write(qi.info);
+                               reel.fake_write(qi.frame, qi.eyes);
                                ++_fake_written;
                                break;
                        case QueueItem::Type::REPEAT:

diff --git a/src/lib/writer.h b/src/lib/writer.h
index 3e93c9b7b56f4c5e9c8d293982dea52951cd2008..cfe3f97cfb1c520931ed9e841427e38844543b65 100644 (file)
--- a/src/lib/writer.h
+++ b/src/lib/writer.h
@@ -77,8 +77,6 @@ public:
 
        /** encoded data for FULL */
        std::shared_ptr<const dcp::Data> encoded;
-       /** info for FAKE */
-       dcp::J2KFrameInfo info;
        /** reel index */
        size_t reel = 0;
        /** frame index within the reel */

diff --git a/test/reel_writer_test.cc b/test/reel_writer_test.cc
index 411de9b1f17632afdf7ac19ab8a3f352f06bf3f3..cb5ad349ee9f02f0267ae27b350c59df71ee1ca8 100644 (file)
--- a/test/reel_writer_test.cc
+++ b/test/reel_writer_test.cc
@@ -47,7 +47,7 @@ using std::string;
 using boost::optional;
 
 
-static bool equal(J2KFrameInfo a, shared_ptr<InfoFileHandle> file, Frame frame, Eyes eyes)
+static bool equal(J2KFrameInfo a, dcp::File& file, Frame frame, Eyes eyes)
 {
        auto b = J2KFrameInfo(file, frame, eyes);
        return a.offset == b.offset && a.size == b.size && a.hash == b.hash;
@@ -58,49 +58,38 @@ BOOST_AUTO_TEST_CASE (write_frame_info_test)
 {
        auto film = new_test_film("write_frame_info_test");
        dcpomatic::DCPTimePeriod const period (dcpomatic::DCPTime(0), dcpomatic::DCPTime(96000));
-       ReelWriter writer(film, period, shared_ptr<Job>(), 0, 1, false, "foo");
-
-       /* Write the first one */
 
        J2KFrameInfo info1(0, 123, "12345678901234567890123456789012");
-       info1.write(film->info_file_handle(period, false), 0, Eyes::LEFT);
-       {
-               auto file = film->info_file_handle(period, true);
-               BOOST_CHECK(equal(info1, file, 0, Eyes::LEFT));
-       }
-
-       /* Write some more */
-
        J2KFrameInfo info2(596, 14921, "123acb789f1234ae782012n456339522");
-       info2.write(film->info_file_handle(period, false), 5, Eyes::RIGHT);
-
-       {
-               auto file = film->info_file_handle(period, true);
-               BOOST_CHECK(equal(info1, file, 0, Eyes::LEFT));
-               BOOST_CHECK(equal(info2, file, 5, Eyes::RIGHT));
-       }
-
        J2KFrameInfo info3(12494, 99157123, "xxxxyyyyabc12356ffsfdsf456339522");
-       info3.write(film->info_file_handle(period, false), 10, Eyes::LEFT);
+       J2KFrameInfo info4(55512494, 123599157123, "ABCDEFGyabc12356ffsfdsf4563395ZZ");
 
        {
-               auto file = film->info_file_handle(period, true);
-               BOOST_CHECK(equal(info1, file, 0, Eyes::LEFT));
-               BOOST_CHECK(equal(info2, file, 5, Eyes::RIGHT));
-               BOOST_CHECK(equal(info3, file, 10, Eyes::LEFT));
+               ReelWriter writer(film, period, shared_ptr<Job>(), 0, 1, false, "foo");
+               info1.write(writer._info_file, 0, Eyes::LEFT);
+               info2.write(writer._info_file, 5, Eyes::RIGHT);
+               info3.write(writer._info_file, 10, Eyes::LEFT);
        }
 
-       /* Overwrite one */
-
-       J2KFrameInfo info4(55512494, 123599157123, "ABCDEFGyabc12356ffsfdsf4563395ZZ");
-       info4.write(film->info_file_handle(period, false), 5, Eyes::RIGHT);
+       auto file1 = dcp::File(film->info_file(period), "rb");
+       BOOST_CHECK(equal(info1, file1, 0, Eyes::LEFT));
+       BOOST_CHECK(equal(info1, file1, 0, Eyes::LEFT));
+       BOOST_CHECK(equal(info2, file1, 5, Eyes::RIGHT));
+       BOOST_CHECK(equal(info1, file1, 0, Eyes::LEFT));
+       BOOST_CHECK(equal(info2, file1, 5, Eyes::RIGHT));
+       BOOST_CHECK(equal(info3, file1, 10, Eyes::LEFT));
 
        {
-               auto file = film->info_file_handle(period, true);
-               BOOST_CHECK(equal(info1, file, 0, Eyes::LEFT));
-               BOOST_CHECK(equal(info4, file, 5, Eyes::RIGHT));
-               BOOST_CHECK(equal(info3, file, 10, Eyes::LEFT));
+               ReelWriter writer(film, period, shared_ptr<Job>(), 0, 1, false, "foo");
+
+               /* Overwrite one */
+               info4.write(writer._info_file, 5, Eyes::RIGHT);
        }
+
+       auto file2 = dcp::File(film->info_file(period), "rb");
+       BOOST_CHECK(equal(info1, file2, 0, Eyes::LEFT));
+       BOOST_CHECK(equal(info4, file2, 5, Eyes::RIGHT));
+       BOOST_CHECK(equal(info3, file2, 10, Eyes::LEFT));
 }