Fix equals() with image subtitles to not compare unique IDs.

Also add an option to save subtitle images to PNGs when they differ.

5 files changed, 96 insertions, 18 deletions

diff --git a/src/subtitle_asset.cc b/src/subtitle_asset.cc
index c6d2e790..96e5cba5 100644
--- a/src/subtitle_asset.cc
+++ b/src/subtitle_asset.cc
@@ -452,7 +452,7 @@ SubtitleAsset::equals (shared_ptr<const Asset> other_asset, EqualityOptions opti
 	}
 
 	if (_subtitles.size() != other->_subtitles.size()) {
-		note (DCP_ERROR, "subtitles differ");
+		note (DCP_ERROR, String::compose("different number of subtitles: %1 vs %2", _subtitles.size(), other->_subtitles.size()));
 		return false;
 	}
 
@@ -466,17 +466,16 @@ SubtitleAsset::equals (shared_ptr<const Asset> other_asset, EqualityOptions opti
 		shared_ptr<SubtitleImage> image_j = dynamic_pointer_cast<SubtitleImage> (*j);
 
 		if ((string_i && !string_j) || (image_i && !image_j)) {
-			note (DCP_ERROR, "subtitles differ");
+			note (DCP_ERROR, "subtitles differ: string vs. image");
 			return false;
 		}
 
 		if (string_i && *string_i != *string_j) {
-			note (DCP_ERROR, "subtitles differ");
+			note (DCP_ERROR, String::compose("subtitles differ in text or metadata: %1 vs %2", string_i->text(), string_j->text()));
 			return false;
 		}
 
-		if (image_i && *image_i != *image_j) {
-			note (DCP_ERROR, "subtitles differ");
+		if (image_i && !image_i->equals(image_j, options, note)) {
 			return false;
 		}
 
diff --git a/src/subtitle_image.cc b/src/subtitle_image.cc
index d8215850..e280e392 100644
--- a/src/subtitle_image.cc
+++ b/src/subtitle_image.cc
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2018 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2018-2020 Carl Hetherington <cth@carlh.net>
 
     This file is part of libdcp.
 
@@ -36,6 +36,7 @@
 
 using std::ostream;
 using std::string;
+using boost::shared_ptr;
 using namespace dcp;
 
 SubtitleImage::SubtitleImage (
@@ -112,6 +113,71 @@ dcp::operator!= (SubtitleImage const & a, SubtitleImage const & b)
 	return !(a == b);
 }
 
+bool
+SubtitleImage::equals (shared_ptr<SubtitleImage> other, EqualityOptions options, NoteHandler note)
+{
+	if (png_image() != other->png_image()) {
+		note (DCP_ERROR, "subtitle image PNG data differs");
+		if (options.export_differing_subtitles) {
+			string const base = "dcpdiff_subtitle_";
+			if (boost::filesystem::exists(base + "A.png")) {
+				note (DCP_ERROR, "could not export subtitle as " + base + "A.png already exists");
+			} else {
+				png_image().write(base + "A.png");
+			}
+			if (boost::filesystem::exists(base + "B.png")) {
+				note (DCP_ERROR, "could not export subtitle as " + base + "B.png already exists");
+			} else {
+				other->png_image().write(base + "B.png");
+			}
+			options.export_differing_subtitles = false;
+		}
+		return false;
+	}
+
+	if (in() != other->in()) {
+		note (DCP_ERROR, "subtitle in times differ");
+		return false;
+	}
+
+	if (out() != other->out()) {
+		note (DCP_ERROR, "subtitle out times differ");
+		return false;
+	}
+
+	if (h_position() != other->h_position()) {
+		note (DCP_ERROR, "subtitle horizontal positions differ");
+		return false;
+	}
+
+	if (h_align() != other->h_align()) {
+		note (DCP_ERROR, "subtitle horizontal alignments differ");
+		return false;
+	}
+
+	if (v_position() != other->v_position()) {
+		note (DCP_ERROR, "subtitle vertical positions differ");
+		return false;
+	}
+
+	if (v_align() != other->v_align()) {
+		note (DCP_ERROR, "subtitle vertical alignments differ");
+		return false;
+	}
+
+	if (fade_up_time() != other->fade_up_time()) {
+		note (DCP_ERROR, "subtitle fade-up times differ");
+		return false;
+	}
+
+	if (fade_down_time() != other->fade_down_time()) {
+		note (DCP_ERROR, "subtitle fade-down times differ");
+		return false;
+	}
+
+	return true;
+}
+
 ostream&
 dcp::operator<< (ostream& s, SubtitleImage const & sub)
 {
@@ -122,3 +188,4 @@ dcp::operator<< (ostream& s, SubtitleImage const & sub)
 
 	return s;
 }
+
diff --git a/src/subtitle_image.h b/src/subtitle_image.h
index 76c34765..e397fb2b 100644
--- a/src/subtitle_image.h
+++ b/src/subtitle_image.h
@@ -99,6 +99,8 @@ public:
 		return _file;
 	}
 
+	bool equals (boost::shared_ptr<dcp::SubtitleImage> other, EqualityOptions options, NoteHandler note);
+
 private:
 	ArrayData _png_image;
 	std::string _id;
diff --git a/src/types.h b/src/types.h
index 684145dd..1102f16b 100644
--- a/src/types.h
+++ b/src/types.h
@@ -215,6 +215,8 @@ extern std::ostream& operator<< (std::ostream& s, Fraction const & f);
  *
  *  When comparing things, we want to be able to ignore some differences;
  *  this class expresses those differences.
+ *
+ *  It also contains some settings for how the comparison should be done.
  */
 struct EqualityOptions
 {
@@ -229,6 +231,7 @@ struct EqualityOptions
 		, issue_dates_can_differ (false)
 		, load_font_nodes_can_differ (false)
 		, keep_going (false)
+		, export_differing_subtitles (false)
 	{}
 
 	/** The maximum allowable mean difference in pixel value between two images */
@@ -247,6 +250,8 @@ struct EqualityOptions
 	bool issue_dates_can_differ;
 	bool load_font_nodes_can_differ;
 	bool keep_going;
+	/** true to save the first pair of differeng image subtitles to the current working directory */
+	bool export_differing_subtitles;
 };
 
 /* I've been unable to make mingw happy with ERROR as a symbol, so
diff --git a/tools/dcpdiff.cc b/tools/dcpdiff.cc
index 3940c0d4..2bda4347 100644
--- a/tools/dcpdiff.cc
+++ b/tools/dcpdiff.cc
@@ -61,17 +61,18 @@ static void
 help (string n)
 {
 	cerr << "Syntax: " << n << " [OPTION] <DCP> <DCP>\n"
-	     << "  -V, --version                show libdcp version\n"
-	     << "  -h, --help                   show this help\n"
-	     << "  -v, --verbose                be verbose\n"
-	     << "      --cpl-annotation-texts   allow differing CPL annotation texts\n"
-	     << "      --reel-annotation-texts  allow differing reel annotation texts\n"
-	     << "  -a, --annotation-texts       allow different CPL and reel annotation texts\n"
-	     << "  -d, --issue-dates            allow different issue dates\n"
-	     << "  -m, --mean-pixel             maximum allowed mean pixel error (default 5)\n"
-	     << "  -s, --std-dev-pixel          maximum allowed standard deviation of pixel error (default 5)\n"
-	     << "      --key                    hexadecimal key to use to decrypt MXFs\n"
-	     << "      --ignore-missing-assets  ignore missing asset files\n"
+	     << "  -V, --version                     show libdcp version\n"
+	     << "  -h, --help                        show this help\n"
+	     << "  -v, --verbose                     be verbose\n"
+	     << "      --cpl-annotation-texts        allow differing CPL annotation texts\n"
+	     << "      --reel-annotation-texts       allow differing reel annotation texts\n"
+	     << "  -a, --annotation-texts            allow different CPL and reel annotation texts\n"
+	     << "  -d, --issue-dates                 allow different issue dates\n"
+	     << "  -m, --mean-pixel                  maximum allowed mean pixel error (default 5)\n"
+	     << "  -s, --std-dev-pixel               maximum allowed standard deviation of pixel error (default 5)\n"
+	     << "      --key                         hexadecimal key to use to decrypt MXFs\n"
+	     << "      --ignore-missing-assets       ignore missing asset files\n"
+	     << "      --export-differing-subtitles  export the first pair of differing image subtitles to the current working directory\n"
 	     << "\n"
 	     << "The <DCP>s are the DCP directories to compare.\n"
 	     << "Comparison is of metadata and content, ignoring timestamps\n"
@@ -147,10 +148,11 @@ main (int argc, char* argv[])
 			{ "cpl-annotation-texts", no_argument, 0, 'C'},
 			{ "key", required_argument, 0, 'D'},
 			{ "reel-annotation-texts", no_argument, 0, 'E'},
+			{ "export-differing-subtitles", no_argument, 0, 'F' },
 			{ 0, 0, 0, 0 }
 		};
 
-		int c = getopt_long (argc, argv, "Vhvm:s:adACD:E", long_options, &option_index);
+		int c = getopt_long (argc, argv, "Vhvm:s:adACD:EF", long_options, &option_index);
 
 		if (c == -1) {
 			break;
@@ -191,6 +193,9 @@ main (int argc, char* argv[])
 		case 'E':
 			options.reel_annotation_texts_can_differ = true;
 			break;
+		case 'F':
+			options.export_differing_subtitles = true;
+			break;
 		}
 	}