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

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.

diff --git a/src/subtitle_asset.cc b/src/subtitle_asset.cc
index bb752441c1c2c3f0832fe67f211004e04303ec41..bf4b1c63e4c476a37748f818855d1eb63494073a 100644 (file)
--- a/src/subtitle_asset.cc
+++ b/src/subtitle_asset.cc
@@ -404,11 +404,31 @@ SubtitleAsset::maybe_add_subtitle (string text, vector<ParseState> const & parse
                        );
                break;
        case ParseState::Type::IMAGE:
+       {
+               switch (standard) {
+               case Standard::INTEROP:
+                       if (text.size() >= 4) {
+                               /* Remove file extension */
+                               text = text.substr(0, text.size() - 4);
+                       }
+                       break;
+               case Standard::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 (
                        make_shared<SubtitleImage>(
                                ArrayData(),
-                               standard == Standard::INTEROP ? text.substr(0, text.size() - 4) : text,
+                               text,
                                ps.in.get(),
                                ps.out.get(),
                                ps.h_position.get_value_or(0),
@@ -421,6 +441,7 @@ SubtitleAsset::maybe_add_subtitle (string text, vector<ParseState> const & parse
                        );
                break;
        }
+       }
 }
 
 

diff --git a/src/subtitle_asset_internal.cc b/src/subtitle_asset_internal.cc
index 80e861f117a9b55cae26299138197a8276bf038e..a9200618516dd48509776e6b274504fdde15587e 100644 (file)
--- a/src/subtitle_asset_internal.cc
+++ b/src/subtitle_asset_internal.cc
@@ -261,7 +261,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 == Standard::SMPTE) {
-               e->add_child_text (_id);
+               e->add_child_text ("urn:uuid:" + _id);
        } else {
                e->add_child_text (_id + ".png");
        }

diff --git a/src/warnings.h b/src/warnings.h
index e751d9c5938042b64eb1f3f53ac40df94123503b..88b4b7242c50f75846308cf6f3b3c263911dd91c 100644 (file)
--- a/src/warnings.h
+++ b/src/warnings.h
@@ -42,6 +42,7 @@
   _Pragma("GCC diagnostic ignored \"-Wparentheses\"") \
   _Pragma("GCC diagnostic ignored \"-Wdeprecated-copy\"") \
   _Pragma("GCC diagnostic ignored \"-Wsuggest-override\"")
+  _Pragma("GCC diagnostic ignored \"-Wunused-function\"")
 #else
 #define LIBDCP_DISABLE_WARNINGS \
   _Pragma("GCC diagnostic push") \

diff --git a/test/data/subs.mxf b/test/data/subs.mxf
index d321e5c620d09cfd02454a31d5709432bb8b8006..9e93251fbb8d0cda587eb26e0100b3aa991caaea 100644 (file)
Binary files a/test/data/subs.mxf and b/test/data/subs.mxf differ

diff --git a/test/smpte_subtitle_test.cc b/test/smpte_subtitle_test.cc
index 74b29951e9bb9c85490a8ae64739cc420337cfc7..cc6190c2a29571bc6e5b24c57a73ef0a4441ff90 100644 (file)
--- a/test/smpte_subtitle_test.cc
+++ b/test/smpte_subtitle_test.cc
@@ -161,18 +161,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()[0]);
-       BOOST_REQUIRE (si);
-       BOOST_CHECK (si->png_image() == dcp::ArrayData("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)
 {
@@ -456,10 +444,13 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test3)
        c.set_reel_number (1);
        c.set_language (dcp::LanguageTag("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 (
                make_shared<dcp::SubtitleImage>(
-                       dcp::ArrayData ("test/data/sub.png"),
+                       dcp::ArrayData(sub_image),
                        dcp::Time (0, 4,  9, 22, 24),
                        dcp::Time (0, 4, 11, 22, 24),
                        0,
@@ -473,8 +464,23 @@ 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[0]);
+       BOOST_REQUIRE (image);
+
+       BOOST_CHECK (image->png_image() == dcp::ArrayData(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));
 }

diff --git a/test/verify_test.cc b/test/verify_test.cc
index 4e26c66e5e225e860e52ea13b3a59e8d79fbae23..e1637911a9434ae1d24c2d7b1e5e0f70b3124ab7 100644 (file)
--- a/test/verify_test.cc
+++ b/test/verify_test.cc
@@ -193,7 +193,7 @@ private:
 };
 
 
-#if 0
+LIBDCP_DISABLE_WARNINGS
 static
 void
 dump_notes (vector<dcp::VerificationNote> const & notes)
@@ -202,7 +202,7 @@ dump_notes (vector<dcp::VerificationNote> const & notes)
                std::cout << dcp::note_to_string(i) << "\n";
        }
 }
-#endif
+LIBDCP_ENABLE_WARNINGS
 
 
 static
@@ -706,6 +706,7 @@ BOOST_AUTO_TEST_CASE (verify_invalid_smpte_subtitles)
 
        path const dir("build/test/verify_invalid_smpte_subtitles");
        prepare_directory (dir);
+       /* This broken_smpte.mxf does not use urn:uuid: for its subtitle ID, which we tolerate (rightly or wrongly) */
        copy_file ("test/data/broken_smpte.mxf", dir / "subs.mxf");
        auto asset = make_shared<dcp::SMPTESubtitleAsset>(dir / "subs.mxf");
        auto reel_asset = make_shared<dcp::ReelSMPTESubtitleAsset>(asset, dcp::Fraction(24, 1), 6046, 0);