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

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.

8 files changed, 44 insertions, 85 deletions

diff --git a/src/lib/film.cc b/src/lib/film.cc
index 1258cdb11..1ac519011 100644
--- 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 0c6a1943e..648d9828b 100644
--- 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 f348bca6a..e2382376c 100644
--- 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 a5a22bd9e..da9503248 100644
--- 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 5210466b2..c0796e714 100644
--- 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 ac6531148..02ee5f972 100644
--- 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 971085046..138873cc3 100644
--- 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 3e93c9b7b..cfe3f97cf 100644
--- 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 */