Improve handling of image subtitle IDs in XML (DoM bug #1965)backport

When reading/writing the XML for image subtitles, we assumed that
the content of the <Image> tag is just the ID of the PNG in the MXF,
without any prefix.

DoM bug #1965 mentions a DCP where this is not the case, and SMPTE
429-5-2009 has an example where there is urn:uuid: in the XML.

This change makes DoM write this urn:uuid: prefix, and accept it if
it's present (but not complain if it's not).

If the urn:uuid: _is_ required in the field, it's a bit surprising
that nobody has complained up to this point.  Maybe nobody noticed,
or nobody reported it.

Cherry-picked from 098007a1ee6a46b6ff11398f94faff5c85951da4 in master.

4 files changed, 48 insertions, 21 deletions

diff --git a/src/subtitle_asset.cc b/src/subtitle_asset.cc
index 083a8040..aeec99d7 100644
--- a/src/subtitle_asset.cc
+++ b/src/subtitle_asset.cc
@@ -385,13 +385,33 @@ SubtitleAsset::maybe_add_subtitle (string text, list<ParseState> const & parse_s
 				)
 			);
 		break;
-	case ParseState::IMAGE:
+	case ParseState::Type::IMAGE:
+	{
+		switch (standard) {
+		case INTEROP:
+			if (text.size() >= 4) {
+				/* Remove file extension */
+				text = text.substr(0, text.size() - 4);
+			}
+			break;
+		case SMPTE:
+			/* It looks like this urn:uuid: is required, but DoM wasn't expecting it (and not writing it)
+			 * until around 2.15.140 so I guess either:
+			 *   a) it is not (always) used in the field, or
+			 *   b) nobody noticed / complained.
+			 */
+			if (text.substr(0, 9) == "urn:uuid:") {
+				text = text.substr(9);
+			}
+			break;
+		}
+
 		/* Add a subtitle with no image data and we'll fill that in later */
 		_subtitles.push_back (
 			shared_ptr<Subtitle> (
 				new SubtitleImage (
-					Data (),
-					standard == INTEROP ? text.substr(0, text.size() - 4) : text,
+					Data(),
+					text,
 					ps.in.get(),
 					ps.out.get(),
 					ps.h_position.get_value_or(0),
@@ -405,6 +425,7 @@ SubtitleAsset::maybe_add_subtitle (string text, list<ParseState> const & parse_s
 			);
 		break;
 	}
+	}
 }
 
 list<shared_ptr<Subtitle> >
diff --git a/src/subtitle_asset_internal.cc b/src/subtitle_asset_internal.cc
index 7b9e18e9..25e03f0b 100644
--- a/src/subtitle_asset_internal.cc
+++ b/src/subtitle_asset_internal.cc
@@ -239,7 +239,7 @@ order::Image::as_xml (xmlpp::Element* parent, Context& context) const
 
 	position_align (e, context, _h_align, _h_position, _v_align, _v_position);
 	if (context.standard == SMPTE) {
-		e->add_child_text (_id);
+		e->add_child_text ("urn:uuid:" + _id);
 	} else {
 		e->add_child_text (_id + ".png");
 	}
diff --git a/test/data/subs.mxf b/test/data/subs.mxf
index ec208dce..9e93251f 100644
--- a/test/data/subs.mxf
+++ b/test/data/subs.mxf
diff --git a/test/smpte_subtitle_test.cc b/test/smpte_subtitle_test.cc
index b8232e9f..c75b205b 100644
--- a/test/smpte_subtitle_test.cc
+++ b/test/smpte_subtitle_test.cc
@@ -162,18 +162,6 @@ BOOST_AUTO_TEST_CASE (read_smpte_subtitle_test2)
 }
 
 
-/** And another one featuring image subtitles */
-BOOST_AUTO_TEST_CASE (read_smpte_subtitle_test3)
-{
-	dcp::SMPTESubtitleAsset subs ("test/data/subs.mxf");
-
-	BOOST_REQUIRE_EQUAL (subs.subtitles().size(), 1);
-	auto si = dynamic_pointer_cast<const dcp::SubtitleImage>(subs.subtitles().front());
-	BOOST_REQUIRE (si);
-	BOOST_CHECK (si->png_image() == dcp::Data("test/data/sub.png"));
-}
-
-
 /* Write some subtitle content as SMPTE XML and check that it is right */
 BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test)
 {
@@ -473,11 +461,14 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test3)
 	c.set_reel_number (1);
 	c.set_language ("en");
 	c.set_content_title_text ("Test");
+	c.set_start_time (dcp::Time());
+
+	boost::filesystem::path const sub_image = "test/data/sub.png";
 
 	c.add (
 		shared_ptr<dcp::SubtitleImage>(
 			new dcp::SubtitleImage(
-				dcp::Data ("test/data/sub.png"),
+				dcp::Data (sub_image),
 				dcp::Time (0, 4,  9, 22, 24),
 				dcp::Time (0, 4, 11, 22, 24),
 				0,
@@ -492,9 +483,24 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test3)
 
 	c._id = "a6c58cff-3e1e-4b38-acec-a42224475ef6";
 
-	boost::filesystem::create_directories ("build/test/write_smpte_subtitle_test3");
-	c.write ("build/test/write_smpte_subtitle_test3/subs.mxf");
-
-	/* XXX: check this result when we can read them back in again */
+	boost::filesystem::path path = "build/test/write_smpte_subtitle_test3";
+	boost::filesystem::create_directories (path);
+	c.write (path / "subs.mxf");
+
+	dcp::SMPTESubtitleAsset read_back (path / "subs.mxf");
+	auto subs = read_back.subtitles ();
+	BOOST_REQUIRE_EQUAL (subs.size(), 1U);
+	auto image = dynamic_pointer_cast<const dcp::SubtitleImage>(subs.front());
+	BOOST_REQUIRE (image);
+
+	BOOST_CHECK (image->png_image() == dcp::Data(sub_image));
+	BOOST_CHECK (image->in() == dcp::Time(0, 4, 9, 22, 24));
+	BOOST_CHECK (image->out() == dcp::Time(0, 4, 11, 22, 24));
+	BOOST_CHECK_CLOSE (image->h_position(), 0.0, 1);
+	BOOST_CHECK (image->h_align() == dcp::HALIGN_CENTER);
+	BOOST_CHECK_CLOSE (image->v_position(), 0.8, 1);
+	BOOST_CHECK (image->v_align() == dcp::VALIGN_TOP);
+	BOOST_CHECK (image->fade_up_time() == dcp::Time(0, 0, 0, 0, 24));
+	BOOST_CHECK (image->fade_down_time() == dcp::Time(0, 0, 0, 0, 24));
 }