Fix cross-thread access to info files. May help with #1618.

diff --git a/src/lib/film.cc b/src/lib/film.cc
index aa16fdad8b485b24125ff7dd0cd9da688493a99d..cc60ab30361649a0acda470ef73389d2b8bd1802 100644 (file)
--- a/src/lib/film.cc
+++ b/src/lib/film.cc
@@ -1685,3 +1685,37 @@ Film::closed_caption_tracks () const
 
        return tt;
 }
+
+shared_ptr<InfoFileHandle>
+Film::info_file_handle (DCPTimePeriod period, bool read) const
+{
+       return shared_ptr<InfoFileHandle> (new InfoFileHandle(_info_file_mutex, info_file(period), read));
+}
+
+InfoFileHandle::InfoFileHandle (boost::mutex& mutex, boost::filesystem::path file, bool read)
+       : _lock (mutex)
+       , _file (file)
+{
+       if (read) {
+               _handle = fopen_boost (file, "rb");
+               if (!_handle) {
+                       throw OpenFileError (file, errno, OpenFileError::READ);
+               }
+       } else {
+               bool const exists = boost::filesystem::exists (file);
+               if (exists) {
+                       _handle = fopen_boost (file, "r+b");
+               } else {
+                       _handle = fopen_boost (file, "wb");
+               }
+
+               if (!_handle) {
+                       throw OpenFileError (file, errno, exists ? OpenFileError::READ_WRITE : OpenFileError::WRITE);
+               }
+       }
+}
+
+InfoFileHandle::~InfoFileHandle ()
+{
+       fclose (_handle);
+}

diff --git a/src/lib/film.h b/src/lib/film.h
index b77ce7a7651099dfd4334381002613b824207af4..e1260f26aae5083a6378830083f579a458e8162f 100644 (file)
--- a/src/lib/film.h
+++ b/src/lib/film.h
@@ -36,7 +36,9 @@
 #include <dcp/encrypted_kdm.h>
 #include <boost/signals2.hpp>
 #include <boost/enable_shared_from_this.hpp>
+#include <boost/thread.hpp>
 #include <boost/filesystem.hpp>
+#include <boost/thread/mutex.hpp>
 #include <string>
 #include <vector>
 #include <inttypes.h>
@@ -56,8 +58,32 @@ class AudioMapping;
 class Ratio;
 class Job;
 class ScreenKDM;
+class Film;
 struct isdcf_name_test;
 
+class InfoFileHandle
+{
+public:
+       ~InfoFileHandle ();
+
+       FILE* get () const {
+               return _handle;
+       }
+
+       boost::filesystem::path file () const {
+               return _file;
+       }
+
+private:
+       friend class Film;
+
+       InfoFileHandle (boost::mutex& mutex, boost::filesystem::path file, bool read);
+
+       boost::mutex::scoped_lock _lock;
+       FILE* _handle;
+       boost::filesystem::path _file;
+};
+
 /** @class Film
  *
  *  @brief A representation of some audio and video content, and details of
@@ -71,7 +97,7 @@ public:
        explicit Film (boost::optional<boost::filesystem::path> dir);
        ~Film ();
 
-       boost::filesystem::path info_file (DCPTimePeriod p) const;
+       boost::shared_ptr<InfoFileHandle> info_file_handle (DCPTimePeriod period, bool read) const;
        boost::filesystem::path j2c_path (int, Frame, Eyes, bool) const;
        boost::filesystem::path internal_video_asset_dir () const;
        boost::filesystem::path internal_video_asset_filename (DCPTimePeriod p) const;
@@ -345,6 +371,8 @@ private:
        friend struct ::isdcf_name_test;
        template <typename> friend class ChangeSignaller;
 
+       boost::filesystem::path info_file (DCPTimePeriod p) const;
+
        void signal_change (ChangeType, Property);
        void signal_change (ChangeType, int);
        std::string video_identifier () const;
@@ -415,6 +443,8 @@ private:
        boost::shared_ptr<Film> _template_film;
 
 
+       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/reel_writer.cc b/src/lib/reel_writer.cc
index 355fe5c3bfac1e1982f2135aa992245e125de50e..c680d3a4e722ed17f0839ea27cbf88c41d471661 100644 (file)
--- a/src/lib/reel_writer.cc
+++ b/src/lib/reel_writer.cc
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2012-2018 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2012-2019 Carl Hetherington <cth@carlh.net>
 
     This file is part of DCP-o-matic.
 
@@ -132,41 +132,27 @@ ReelWriter::ReelWriter (
 void
 ReelWriter::write_frame_info (Frame frame, Eyes eyes, dcp::FrameInfo info) const
 {
-       FILE* file = 0;
-       boost::filesystem::path info_file = _film->info_file (_period);
-
-       bool const read = boost::filesystem::exists (info_file);
-
-       if (read) {
-               file = fopen_boost (info_file, "r+b");
-       } else {
-               file = fopen_boost (info_file, "wb");
-       }
-
-       if (!file) {
-               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);
-       checked_fwrite (&info.size, sizeof (info.size), file, info_file);
-       checked_fwrite (info.hash.c_str(), info.hash.size(), file, info_file);
-       fclose (file);
+       shared_ptr<InfoFileHandle> handle = _film->info_file_handle(_period, false);
+       dcpomatic_fseek (handle->get(), frame_info_position(frame, eyes), SEEK_SET);
+       checked_fwrite (&info.offset, sizeof(info.offset), handle->get(), handle->file());
+       checked_fwrite (&info.size, sizeof (info.size), handle->get(), handle->file());
+       checked_fwrite (info.hash.c_str(), info.hash.size(), handle->get(), handle->file());
 }
 
 dcp::FrameInfo
-ReelWriter::read_frame_info (FILE* file, Frame frame, Eyes eyes) const
+ReelWriter::read_frame_info (shared_ptr<InfoFileHandle> info, Frame frame, Eyes eyes) const
 {
-       dcp::FrameInfo info;
-       dcpomatic_fseek (file, frame_info_position (frame, eyes), SEEK_SET);
-       checked_fread (&info.offset, sizeof(info.offset), file, _film->info_file(_period));
-       checked_fread (&info.size, sizeof(info.size), file, _film->info_file(_period));
+       dcp::FrameInfo frame_info;
+       dcpomatic_fseek (info->get(), frame_info_position(frame, eyes), SEEK_SET);
+       checked_fread (&frame_info.offset, sizeof(frame_info.offset), info->get(), info->file());
+       checked_fread (&frame_info.size, sizeof(frame_info.size), info->get(), info->file());
 
        char hash_buffer[33];
-       checked_fread (hash_buffer, 32, file, _film->info_file(_period));
+       checked_fread (hash_buffer, 32, info->get(), info->file());
        hash_buffer[32] = '\0';
-       info.hash = hash_buffer;
+       frame_info.hash = hash_buffer;
 
-       return info;
+       return frame_info;
 }
 
 long
@@ -211,17 +197,20 @@ ReelWriter::check_existing_picture_asset ()
                LOG_GENERAL ("Opened existing asset at %1", asset.string());
        }
 
-       /* Offset of the last dcp::FrameInfo in the info file */
-       int const n = (boost::filesystem::file_size (_film->info_file(_period)) / _info_size) - 1;
-       LOG_GENERAL ("The last FI is %1; info file is %2, info size %3", n, boost::filesystem::file_size (_film->info_file(_period)), _info_size);
+       shared_ptr<InfoFileHandle> info_file;
 
-       FILE* info_file = fopen_boost (_film->info_file(_period), "rb");
-       if (!info_file) {
+       try {
+               info_file = _film->info_file_handle (_period, true);
+       } catch (OpenFileError) {
                LOG_GENERAL_NC ("Could not open film info file");
                fclose (asset_file);
                return 0;
        }
 
+       /* Offset of the last dcp::FrameInfo in the info file */
+       int const n = (boost::filesystem::file_size(info_file->file()) / _info_size) - 1;
+       LOG_GENERAL ("The last FI is %1; info file is %2, info size %3", n, boost::filesystem::file_size(info_file->file()), _info_size);
+
        Frame first_nonexistant_frame;
        if (_film->three_d ()) {
                /* Start looking at the last left frame */
@@ -244,7 +233,6 @@ ReelWriter::check_existing_picture_asset ()
        LOG_GENERAL ("Proceeding with first nonexistant frame %1", first_nonexistant_frame);
 
        fclose (asset_file);
-       fclose (info_file);
 
        return first_nonexistant_frame;
 }
@@ -634,7 +622,7 @@ ReelWriter::write (PlayerText subs, TextType type, optional<DCPTextTrack> track,
 }
 
 bool
-ReelWriter::existing_picture_frame_ok (FILE* asset_file, FILE* info_file, Frame frame) const
+ReelWriter::existing_picture_frame_ok (FILE* asset_file, shared_ptr<InfoFileHandle> info_file, Frame frame) const
 {
        LOG_GENERAL ("Checking existing picture frame %1", frame);
 

diff --git a/src/lib/reel_writer.h b/src/lib/reel_writer.h
index 741b0914cc9dc2f028844a71797399a164b0f215..f13f05dde00bd06f19c515248dd9320d47eb12de 100644 (file)
--- a/src/lib/reel_writer.h
+++ b/src/lib/reel_writer.h
@@ -30,6 +30,7 @@ class Film;
 class Job;
 class Font;
 class AudioBuffers;
+class InfoFileHandle;
 struct write_frame_info_test;
 
 namespace dcp {
@@ -86,7 +87,7 @@ public:
                return _first_nonexistant_frame;
        }
 
-       dcp::FrameInfo read_frame_info (FILE* file, Frame frame, Eyes eyes) const;
+       dcp::FrameInfo read_frame_info (boost::shared_ptr<InfoFileHandle> info, Frame frame, Eyes eyes) const;
 
 private:
 
@@ -95,7 +96,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 ();
-       bool existing_picture_frame_ok (FILE* asset_file, FILE* info_file, Frame frame) const;
+       bool existing_picture_frame_ok (FILE* asset_file, boost::shared_ptr<InfoFileHandle> info_file, Frame frame) const;
 
        boost::shared_ptr<const Film> _film;
 

diff --git a/src/lib/writer.cc b/src/lib/writer.cc
index 8666d90c871fc7cee962b90f5b7064942db9b2e1..ca412dd3f5ba7f5f21db2566f9228c73cf497139 100644 (file)
--- a/src/lib/writer.cc
+++ b/src/lib/writer.cc
@@ -216,16 +216,14 @@ Writer::fake_write (Frame frame, Eyes eyes)
        size_t const reel = video_reel (frame);
        Frame const reel_frame = frame - _reels[reel].start ();
 
-       FILE* file = fopen_boost (_film->info_file(_reels[reel].period()), "rb");
-       if (!file) {
-               throw ReadFileError (_film->info_file(_reels[reel].period()));
-       }
-       dcp::FrameInfo info = _reels[reel].read_frame_info (file, reel_frame, eyes);
-       fclose (file);
-
        QueueItem qi;
        qi.type = QueueItem::FAKE;
-       qi.size = info.size;
+
+       {
+               shared_ptr<InfoFileHandle> info_file = _film->info_file_handle(_reels[reel].period(), true);
+               qi.size = _reels[reel].read_frame_info(info_file, reel_frame, eyes).size;
+       }
+
        qi.reel = reel;
        qi.frame = reel_frame;
        if (_film->three_d() && eyes == EYES_BOTH) {

diff --git a/test/reel_writer_test.cc b/test/reel_writer_test.cc
index 02b6fc8498322b20bef75dd748c78ad486cbd7ef..4943962ed16b29905d65f3c5afe1d445b02cbcac 100644 (file)
--- a/test/reel_writer_test.cc
+++ b/test/reel_writer_test.cc
@@ -33,14 +33,10 @@ 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)
+static bool equal (dcp::FrameInfo a, ReelWriter const & writer, shared_ptr<InfoFileHandle> 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;
+       dcp::FrameInfo b = writer.read_frame_info(file, frame, eyes);
+       return a.offset == b.offset && a.size == b.size && a.hash == b.hash;
 }
 
 BOOST_AUTO_TEST_CASE (write_frame_info_test)
@@ -51,11 +47,9 @@ BOOST_AUTO_TEST_CASE (write_frame_info_test)
 
        /* 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));
+       shared_ptr<InfoFileHandle> file = film->info_file_handle(period, true);
 
        BOOST_CHECK (equal(info1, writer, file, 0, EYES_LEFT));
 
@@ -63,14 +57,12 @@ BOOST_AUTO_TEST_CASE (write_frame_info_test)
 
        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));
@@ -80,7 +72,6 @@ BOOST_AUTO_TEST_CASE (write_frame_info_test)
 
        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));