Add an advanced option to check data against the frame info file when making hashes (#2758).2758-safe-write-take2

7 files changed, 146 insertions, 3 deletions

diff --git a/src/lib/config.cc b/src/lib/config.cc
index af97c7af7..731373ad9 100644
--- a/src/lib/config.cc
+++ b/src/lib/config.cc
@@ -219,6 +219,7 @@ Config::set_defaults()
 	_auto_crop_threshold = 0.1;
 	_last_release_notes_version = boost::none;
 	_allow_smpte_bv20 = false;
+	_check_disk_writes = false;
 	_isdcf_name_part_length = 14;
 	_enable_player_http_server = false;
 	_player_http_server_port = 8080;
@@ -670,6 +671,7 @@ try
 	}
 
 	_allow_smpte_bv20 = f.optional_bool_child("AllowSMPTEBv20").get_value_or(false);
+	_check_disk_writes = f.optional_bool_child("CheckDiskWrites").get_value_or(false);
 	_isdcf_name_part_length = f.optional_number_child<int>("ISDCFNamePartLength").get_value_or(14);
 	_enable_player_http_server = f.optional_bool_child("EnablePlayerHTTPServer").get_value_or(false);
 	_player_http_server_port = f.optional_number_child<int>("PlayerHTTPServerPort").get_value_or(8080);
@@ -1154,6 +1156,8 @@ Config::write_config() const
 
 	/* [XML] AllowSMPTEBv20 1 to allow the user to choose SMPTE (Bv2.0 only) as a standard, otherwise 0 */
 	cxml::add_text_child(root, "AllowSMPTEBv20", _allow_smpte_bv20 ? "1" : "0");
+	/* [XML] CheckDiskWrites 1 to check for disk corruption when creating MXF hashes, otherwise 0 */
+	cxml::add_text_child(root, "CheckDiskWrites", _check_disk_writes ? "1" : "0");
 	/* [XML] ISDCFNamePartLength Maximum length of the "name" part of an ISDCF name, which should be 14 according to the standard */
 	cxml::add_text_child(root, "ISDCFNamePartLength", fmt::to_string(_isdcf_name_part_length));
 	/* [XML] EnablePlayerHTTPServer 1 to enable a HTTP server to control the player, otherwise 0 */
diff --git a/src/lib/config.h b/src/lib/config.h
index d8ff70db8..e774712d6 100644
--- a/src/lib/config.h
+++ b/src/lib/config.h
@@ -116,6 +116,7 @@ public:
 		AUDIO_MAPPING,
 		AUTO_CROP_THRESHOLD,
 		ALLOW_SMPTE_BV20,
+		CHECK_DISK_WRITES,
 		ISDCF_NAME_PART_LENGTH,
 		ALLOW_ANY_CONTAINER,
 #ifdef DCPOMATIC_GROK
@@ -656,6 +657,10 @@ public:
 		return _allow_smpte_bv20;
 	}
 
+	bool check_disk_writes() const {
+		return _check_disk_writes;
+	}
+
 #ifdef DCPOMATIC_GROK
 	class Grok
 	{
@@ -1249,6 +1254,10 @@ public:
 		maybe_set(_allow_smpte_bv20, allow, ALLOW_SMPTE_BV20);
 	}
 
+	void set_check_disk_writes(bool check) {
+		maybe_set(_check_disk_writes, check, CHECK_DISK_WRITES);
+	}
+
 #ifdef DCPOMATIC_GROK
 	void set_grok(Grok const& grok);
 #endif
@@ -1517,6 +1526,7 @@ private:
 	boost::optional<int> _main_content_divider_sash_position;
 	DefaultAddFileLocation _default_add_file_location;
 	bool _allow_smpte_bv20;
+	bool _check_disk_writes;
 	int _isdcf_name_part_length;
 	bool _enable_player_http_server;
 	int _player_http_server_port;
diff --git a/src/lib/info_file_checker.cc b/src/lib/info_file_checker.cc
new file mode 100644
index 000000000..1e277a5ac
--- /dev/null
+++ b/src/lib/info_file_checker.cc
@@ -0,0 +1,62 @@
+/*
+    Copyright (C) 2024 Carl Hetherington <cth@carlh.net>
+
+    This file is part of DCP-o-matic.
+
+    DCP-o-matic is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    DCP-o-matic is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with DCP-o-matic.  If not, see <http://www.gnu.org/licenses/>.
+
+*/
+
+
+#include "info_file_checker.h"
+#include <dcp/filesystem.h>
+
+#include "i18n.h"
+
+
+using std::shared_ptr;
+
+
+InfoFileChecker::InfoFileChecker(dcp::File& file)
+	: _file(file)
+	, _next(_file)
+{
+
+}
+
+
+void
+InfoFileChecker::check(int64_t offset, uint8_t const* data, int length)
+{
+	while (_next && offset <= static_cast<int64_t>(_next->offset) && _next->offset < static_cast<uint64_t>(offset + length)) {
+		auto hash_start_offset = _next->offset - offset;
+		auto hash_length = std::min(_next->size, length - hash_start_offset);
+		_digester.add(data + hash_start_offset, hash_length);
+		_next->offset += hash_length;
+		_next->size -= hash_length;
+		if (_next->size == 0) {
+			if (_digester.get_vector() != _next->hash) {
+				throw EncodeError(_("File on disk is corrupted"));
+			}
+
+			_next = boost::none;
+			try {
+				_next = J2KFrameInfo(_file);
+			} catch (...) {}
+
+			_digester = Digester();
+		}
+	}
+}
+
diff --git a/src/lib/info_file_checker.h b/src/lib/info_file_checker.h
new file mode 100644
index 000000000..9b761edff
--- /dev/null
+++ b/src/lib/info_file_checker.h
@@ -0,0 +1,40 @@
+/*
+    Copyright (C) 2024 Carl Hetherington <cth@carlh.net>
+
+    This file is part of DCP-o-matic.
+
+    DCP-o-matic is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    DCP-o-matic is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with DCP-o-matic.  If not, see <http://www.gnu.org/licenses/>.
+
+*/
+
+
+#include "digester.h"
+#include "j2k_frame_info.h"
+#include <dcp/file.h>
+#include <boost/filesystem.hpp>
+
+
+class InfoFileChecker
+{
+public:
+	InfoFileChecker(dcp::File& file);
+
+	void check(int64_t offset, uint8_t const* data, int length);
+
+private:
+	dcp::File& _file;
+	boost::optional<J2KFrameInfo> _next;
+	Digester _digester;
+};
+
diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc
index a27d7f63e..3a8a95ab1 100644
--- a/src/lib/reel_writer.cc
+++ b/src/lib/reel_writer.cc
@@ -29,6 +29,7 @@
 #include "film_util.h"
 #include "image.h"
 #include "image_png.h"
+#include "info_file_checker.h"
 #include "job.h"
 #include "j2k_frame_info.h"
 #include "log.h"
@@ -757,10 +758,23 @@ try
 	}
 
 	int64_t total_done = 0;
+
+	auto progress = [&total_done, total_size, set_progress](int64_t done, int64_t) {
+		set_progress(total_done + done, total_size);
+	};
+
 	for (auto asset: assets) {
-		asset->hash([&total_done, total_size, set_progress](int64_t done, int64_t) {
-			set_progress(total_done + done, total_size);
-		});
+		if (asset == _j2k_picture_asset && Config::instance()->check_disk_writes()) {
+			try  {
+				InfoFileChecker checker(_info_file);
+				asset->hash(progress, boost::bind(&InfoFileChecker::check, &checker, _1, _2, _3));
+			} catch (OpenFileError&) {
+				LOG_ERROR_NC("Could not open info file to check disk writes");
+				asset->hash(progress);
+			}
+		} else {
+			asset->hash(progress);
+		}
 		total_done += asset->file() ? boost::filesystem::file_size(*asset->file()) : 0;
 	}
 
diff --git a/src/lib/wscript b/src/lib/wscript
index 990d11547..4913115e0 100644
--- a/src/lib/wscript
+++ b/src/lib/wscript
@@ -133,6 +133,7 @@ sources = """
           hints.cc
           http_server.cc
           id.cc
+          info_file_checker.cc
           internet.cc
           image.cc
           image_content.cc
diff --git a/src/wx/full_config_dialog.cc b/src/wx/full_config_dialog.cc
index eff3fdee2..361e534f8 100644
--- a/src/wx/full_config_dialog.cc
+++ b/src/wx/full_config_dialog.cc
@@ -1218,6 +1218,10 @@ private:
 		table->Add(_only_servers_encode, 1, wxEXPAND | wxLEFT, DCPOMATIC_SIZER_GAP);
 		table->AddSpacer(0);
 
+		_check_disk_writes = new CheckBox(_panel, _("Check disk writes"));
+		table->Add(_check_disk_writes, 1, wxEXPAND | wxLEFT, DCPOMATIC_SIZER_GAP);
+		table->AddSpacer(0);
+
 		_layout_for_short_screen = new CheckBox(_panel, _("Layout for short screen"));
 		table->Add(_layout_for_short_screen, 1, wxEXPAND | wxLEFT, DCPOMATIC_SIZER_GAP);
 		table->AddSpacer(0);
@@ -1314,6 +1318,7 @@ private:
 		_video_display_mode->Bind(wxEVT_CHOICE, boost::bind(&AdvancedPage::video_display_mode_changed, this));
 		_show_experimental_audio_processors->bind(&AdvancedPage::show_experimental_audio_processors_changed, this);
 		_only_servers_encode->bind(&AdvancedPage::only_servers_encode_changed, this);
+		_check_disk_writes->bind(&AdvancedPage::check_disk_writes_changed, this);
 		_layout_for_short_screen->bind(&AdvancedPage::layout_for_short_screen_changed, this);
 		_frames_in_memory_multiplier->Bind(wxEVT_SPINCTRL, boost::bind(&AdvancedPage::frames_in_memory_multiplier_changed, this));
 		_dcp_metadata_filename_format->Changed.connect(boost::bind(&AdvancedPage::dcp_metadata_filename_format_changed, this));
@@ -1347,6 +1352,7 @@ private:
 		}
 		checked_set(_show_experimental_audio_processors, config->show_experimental_audio_processors());
 		checked_set(_only_servers_encode, config->only_servers_encode());
+		checked_set(_check_disk_writes, config->check_disk_writes());
 		checked_set(_layout_for_short_screen, config->layout_for_short_screen());
 		checked_set(_log_general, config->log_types() & LogEntry::TYPE_GENERAL);
 		checked_set(_log_warning, config->log_types() & LogEntry::TYPE_WARNING);
@@ -1398,6 +1404,11 @@ private:
 		Config::instance()->set_dcp_metadata_filename_format(_dcp_metadata_filename_format->get());
 	}
 
+	void check_disk_writes_changed()
+	{
+		Config::instance()->set_check_disk_writes(_check_disk_writes->GetValue());
+	}
+
 	void dcp_asset_filename_format_changed()
 	{
 		Config::instance()->set_dcp_asset_filename_format(_dcp_asset_filename_format->get());
@@ -1450,6 +1461,7 @@ private:
 	wxSpinCtrl* _frames_in_memory_multiplier = nullptr;
 	CheckBox* _show_experimental_audio_processors = nullptr;
 	CheckBox* _only_servers_encode = nullptr;
+	CheckBox* _check_disk_writes = nullptr;
 	CheckBox* _layout_for_short_screen = nullptr;
 	NameFormatEditor* _dcp_metadata_filename_format = nullptr;
 	NameFormatEditor* _dcp_asset_filename_format = nullptr;