From 1580bdc52a257870c908f32d2abe6fed84d83c50 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Wed, 9 Oct 2019 00:43:22 +0200 Subject: [PATCH] Fix cross-thread access to info files. May help with #1618. --- src/lib/film.cc | 34 +++++++++++++++++++++++ src/lib/film.h | 32 +++++++++++++++++++++- src/lib/reel_writer.cc | 58 ++++++++++++++++------------------------ src/lib/reel_writer.h | 5 ++-- src/lib/writer.cc | 14 +++++----- test/reel_writer_test.cc | 17 +++--------- 6 files changed, 101 insertions(+), 59 deletions(-) diff --git a/src/lib/film.cc b/src/lib/film.cc index dcaa73754..e85543b80 100644 --- a/src/lib/film.cc +++ b/src/lib/film.cc @@ -1740,3 +1740,37 @@ Film::marker (dcp::Marker type) const } return i->second; } + +shared_ptr +Film::info_file_handle (DCPTimePeriod period, bool read) const +{ + return shared_ptr (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 6f1294b29..0c1959056 100644 --- a/src/lib/film.h +++ b/src/lib/film.h @@ -36,7 +36,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -59,8 +61,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 @@ -74,7 +100,7 @@ public: explicit Film (boost::optional dir); ~Film (); - boost::filesystem::path info_file (dcpomatic::DCPTimePeriod p) const; + boost::shared_ptr info_file_handle (dcpomatic::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 (dcpomatic::DCPTimePeriod p) const; @@ -369,6 +395,8 @@ private: friend struct ::isdcf_name_test; template friend class ChangeSignaller; + boost::filesystem::path info_file (dcpomatic::DCPTimePeriod p) const; + void signal_change (ChangeType, Property); void signal_change (ChangeType, int); std::string video_identifier () const; @@ -445,6 +473,8 @@ private: */ bool _tolerant; + 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 263148902..f6313773e 100644 --- a/src/lib/reel_writer.cc +++ b/src/lib/reel_writer.cc @@ -1,5 +1,5 @@ /* - Copyright (C) 2012-2018 Carl Hetherington + Copyright (C) 2012-2019 Carl Hetherington This file is part of DCP-o-matic. @@ -134,41 +134,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 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 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 @@ -213,17 +199,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 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 */ @@ -246,7 +235,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; } @@ -655,7 +643,7 @@ ReelWriter::write (PlayerText subs, TextType type, optional 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 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 b96bcfc68..8649ea37f 100644 --- a/src/lib/reel_writer.h +++ b/src/lib/reel_writer.h @@ -33,6 +33,7 @@ namespace dcpomatic { class Film; class Job; class AudioBuffers; +class InfoFileHandle; struct write_frame_info_test; namespace dcp { @@ -89,7 +90,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 info, Frame frame, Eyes eyes) const; private: @@ -98,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 (); - 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 info_file, Frame frame) const; boost::shared_ptr _film; diff --git a/src/lib/writer.cc b/src/lib/writer.cc index 5b24d7491..9a0f83a22 100644 --- a/src/lib/writer.cc +++ b/src/lib/writer.cc @@ -217,16 +217,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 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 db63ca8bf..1774e8836 100644 --- 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 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 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)); -- 2.30.2