From 066e30f30f851396bd6f10d50a448507e1c0b392 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Tue, 15 Mar 2022 15:32:49 +0100 Subject: Nearly pretty-print subtitle XML (though not in nodes). This is an attempt to fix DoM bug #2205. --- src/interop_subtitle_asset.cc | 2 +- src/smpte_subtitle_asset.cc | 4 +- src/subtitle_asset.cc | 124 ++++++++++++++++- src/subtitle_asset.h | 6 + test/interop_subtitle_test.cc | 60 ++++---- test/ref/write_interop_subtitle_test3/ASSETMAP | 2 +- .../pkl_e94b8a0d-27f7-408a-af16-78d3df419a91.xml | 6 +- test/ref/write_interop_subtitle_test3/subs.xml | 10 +- test/shared_subtitle_test.cc | 50 +++++++ test/smpte_subtitle_test.cc | 152 ++++++++++----------- test/verify_test.cc | 4 +- 11 files changed, 294 insertions(+), 126 deletions(-) diff --git a/src/interop_subtitle_asset.cc b/src/interop_subtitle_asset.cc index d5147284..bb0cad70 100644 --- a/src/interop_subtitle_asset.cc +++ b/src/interop_subtitle_asset.cc @@ -125,7 +125,7 @@ InteropSubtitleAsset::xml_as_string () const subtitles_as_xml (root, 250, Standard::INTEROP); - return doc.write_to_string ("UTF-8"); + return format_xml(doc, {}); } diff --git a/src/smpte_subtitle_asset.cc b/src/smpte_subtitle_asset.cc index f09d99f3..0693323c 100644 --- a/src/smpte_subtitle_asset.cc +++ b/src/smpte_subtitle_asset.cc @@ -344,8 +344,6 @@ SMPTESubtitleAsset::xml_as_string () const { xmlpp::Document doc; auto root = doc.create_root_node ("SubtitleReel"); - root->set_namespace_declaration (subtitle_smpte_ns); - root->set_namespace_declaration ("http://www.w3.org/2001/XMLSchema", "xs"); DCP_ASSERT (_xml_id); root->add_child("Id")->add_child_text("urn:uuid:" + *_xml_id); @@ -374,7 +372,7 @@ SMPTESubtitleAsset::xml_as_string () const subtitles_as_xml (root->add_child("SubtitleList"), _time_code_rate, Standard::SMPTE); - return doc.write_to_string ("UTF-8"); + return format_xml(doc, { {"", subtitle_smpte_ns}, {"xs", "http://www.w3.org/2001/XMLSchema"} }); } diff --git a/src/subtitle_asset.cc b/src/subtitle_asset.cc index 22781196..bd8bb993 100644 --- a/src/subtitle_asset.cc +++ b/src/subtitle_asset.cc @@ -54,18 +54,20 @@ #include #include #include +#include -using std::dynamic_pointer_cast; -using std::string; -using std::cout; using std::cerr; +using std::cout; +using std::dynamic_pointer_cast; +using std::make_shared; using std::map; +using std::pair; using std::shared_ptr; +using std::string; using std::vector; -using std::make_shared; -using boost::optional; using boost::lexical_cast; +using boost::optional; using namespace dcp; @@ -799,3 +801,115 @@ SubtitleAsset::fix_empty_font_ids () } } } + + +namespace { + +struct State +{ + int indent; + string xml; + int disable_formatting; +}; + +} + + +static +void +format_xml_node (xmlpp::Node const* node, State& state) +{ + if (auto text_node = dynamic_cast(node)) { + string content = text_node->get_content(); + boost::replace_all(content, "&", "&"); + boost::replace_all(content, "<", "<"); + boost::replace_all(content, ">", ">"); + state.xml += content; + } else if (auto element = dynamic_cast(node)) { + ++state.indent; + + auto children = element->get_children(); + auto const should_disable_formatting = + std::any_of( + children.begin(), children.end(), + [](xmlpp::Node const* node) { return static_cast(dynamic_cast(node)); } + ) || element->get_name() == "Text"; + + if (!state.disable_formatting) { + state.xml += "\n" + string(state.indent * 2, ' '); + } + + state.xml += "<" + element->get_name(); + + for (auto attribute: element->get_attributes()) { + state.xml += String::compose(" %1=\"%2\"", attribute->get_name().raw(), attribute->get_value().raw()); + } + + if (children.empty()) { + state.xml += "/>"; + } else { + state.xml += ">"; + + if (should_disable_formatting) { + ++state.disable_formatting; + } + + for (auto child: children) { + format_xml_node(child, state); + } + + if (!state.disable_formatting) { + state.xml += "\n" + string(state.indent * 2, ' '); + } + + state.xml += String::compose("", element->get_name().raw()); + + if (should_disable_formatting) { + --state.disable_formatting; + } + } + + --state.indent; + } +} + + +/** Format XML much as write_to_string_formatted() would do, except without adding any white space + * to nodes. This is an attempt to avoid changing what is actually displayed as subtitles + * while also formatting the XML in such a way as to avoid DoM bug 2205. + * + * namespace is a list of namespaces for the root node; it would be nicer to set these up with + * set_namespace_declaration in the caller and then to extract them here but I couldn't find a way + * to get all namespaces with the libxml++ API. + */ +string +SubtitleAsset::format_xml (xmlpp::Document const& document, vector> const& namespaces) +{ + auto root = document.get_root_node(); + + State state = {}; + state.xml = "\n<" + root->get_name(); + + for (auto const& ns: namespaces) { + if (ns.first.empty()) { + state.xml += String::compose(" xmlns=\"%1\"", ns.second); + } else { + state.xml += String::compose(" xmlns:%1=\"%2\"", ns.first, ns.second); + } + } + + for (auto attribute: root->get_attributes()) { + state.xml += String::compose(" %1=\"%2\"", attribute->get_name().raw(), attribute->get_value().raw()); + } + + state.xml += ">"; + + for (auto child: document.get_root_node()->get_children()) { + format_xml_node(child, state); + } + + state.xml += String::compose("\n\n", root->get_name().raw()); + + return state.xml; +} + diff --git a/src/subtitle_asset.h b/src/subtitle_asset.h index f51906e2..88b5378c 100644 --- a/src/subtitle_asset.h +++ b/src/subtitle_asset.h @@ -48,9 +48,13 @@ #include #include #include +#include +#include +#include namespace xmlpp { + class Document; class Element; } @@ -128,6 +132,8 @@ public: return _raw_xml; } + static std::string format_xml (xmlpp::Document const& document, std::vector> const& namespaces); + protected: friend struct ::interop_dcp_font_test; friend struct ::smpte_dcp_font_test; diff --git a/test/interop_subtitle_test.cc b/test/interop_subtitle_test.cc index ff0940b0..3369fc24 100644 --- a/test/interop_subtitle_test.cc +++ b/test/interop_subtitle_test.cc @@ -762,21 +762,21 @@ BOOST_AUTO_TEST_CASE (write_interop_subtitle_test) c._id = "a6c58cff-3e1e-4b38-acec-a42224475ef6"; check_xml ( - "" - "a6c58cff-3e1e-4b38-acec-a42224475ef6" - "Test" - "1" - "EN" - "" - "" - "Hello world" - "" - "" - "" - "" - "What's going on" - "" - "" + "\n" + " a6c58cff-3e1e-4b38-acec-a42224475ef6\n" + " Test\n" + " 1\n" + " EN\n" + " \n" + " \n" + " Hello world\n" + " \n" + " \n" + " \n" + " \n" + " What's going on\n" + " \n" + " \n" "", c.xml_as_string (), vector() @@ -846,21 +846,21 @@ BOOST_AUTO_TEST_CASE (write_interop_subtitle_test2) c._id = "a6c58cff-3e1e-4b38-acec-a42224475ef6"; check_xml ( - "" - "a6c58cff-3e1e-4b38-acec-a42224475ef6" - "Test" - "1" - "EN" - "" - "" - "Hello world" - "" - "" - "" - "" - "What's going on" - "" - "" + "\n" + " a6c58cff-3e1e-4b38-acec-a42224475ef6\n" + " Test\n" + " 1\n" + " EN\n" + " \n" + " \n" + " Hello world\n" + " \n" + " \n" + " \n" + " \n" + " What's going on\n" + " \n" + " \n" "", c.xml_as_string (), vector() diff --git a/test/ref/write_interop_subtitle_test3/ASSETMAP b/test/ref/write_interop_subtitle_test3/ASSETMAP index 7c901513..570b0d5b 100644 --- a/test/ref/write_interop_subtitle_test3/ASSETMAP +++ b/test/ref/write_interop_subtitle_test3/ASSETMAP @@ -37,7 +37,7 @@ subs.xml 1 0 - 414 + 438 diff --git a/test/ref/write_interop_subtitle_test3/pkl_e94b8a0d-27f7-408a-af16-78d3df419a91.xml b/test/ref/write_interop_subtitle_test3/pkl_e94b8a0d-27f7-408a-af16-78d3df419a91.xml index 2da8e910..6f192259 100644 --- a/test/ref/write_interop_subtitle_test3/pkl_e94b8a0d-27f7-408a-af16-78d3df419a91.xml +++ b/test/ref/write_interop_subtitle_test3/pkl_e94b8a0d-27f7-408a-af16-78d3df419a91.xml @@ -9,15 +9,15 @@ urn:uuid:46c3eb45-15e5-47d6-8684-d8641e4dc516 46c3eb45-15e5-47d6-8684-d8641e4dc516 - 614bJ4VLsNZ6mLbdXbZXjGuoSsY= + Y8rZTMi93JjmkgwFK7Fu3W5rz/Y= 1061 text/xml;asdcpKind=CPL urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6 a6c58cff-3e1e-4b38-acec-a42224475ef6 - cVnFjMLTQnSIAlIzJpNB/p7B230= - 414 + ilLkOSjxgHZxkAdOPVJBoDFiUh8= + 438 text/xml;asdcpKind=Subtitle diff --git a/test/ref/write_interop_subtitle_test3/subs.xml b/test/ref/write_interop_subtitle_test3/subs.xml index d1c8fa7e..050ee930 100644 --- a/test/ref/write_interop_subtitle_test3/subs.xml +++ b/test/ref/write_interop_subtitle_test3/subs.xml @@ -1,2 +1,10 @@ -a6c58cff-3e1e-4b38-acec-a42224475ef6Test1ENd36f4bb3-c4fa-4a95-9915-6fec3110cd71.png + + a6c58cff-3e1e-4b38-acec-a42224475ef6 + Test + 1 + EN + + d36f4bb3-c4fa-4a95-9915-6fec3110cd71.png + + diff --git a/test/shared_subtitle_test.cc b/test/shared_subtitle_test.cc index f18b03db..ae05642a 100644 --- a/test/shared_subtitle_test.cc +++ b/test/shared_subtitle_test.cc @@ -167,3 +167,53 @@ BOOST_AUTO_TEST_CASE (pull_fonts_test3) BOOST_CHECK_EQUAL (sub1->font._values["size"], "42"); } + +/* Check that subtitle XML is prettily formatted without inserting any white space into + * node, which I think has the potential to alter appearance. + */ +BOOST_AUTO_TEST_CASE (format_xml_test1) +{ + xmlpp::Document doc; + auto root = doc.create_root_node("Foo"); + root->add_child("Empty"); + root->add_child("Text")->add_child_text("Hello world"); + root->add_child("Font")->add_child("Text")->add_child_text("Say what"); + auto fred = root->add_child("Text")->add_child("Font"); + fred->set_attribute("bob", "job"); + fred->add_child_text("Fred"); + fred->add_child("Text")->add_child_text("Jim"); + fred->add_child_text("Sheila"); + BOOST_REQUIRE_EQUAL (dcp::SubtitleAsset::format_xml(doc, { {"", "fred"}, {"jim", "sheila"} }), +"\n" +"\n" +" \n" +" Hello world\n" +" \n" +" Say what\n" +" \n" +" FredJimSheila\n" +"\n"); +} + + +BOOST_AUTO_TEST_CASE (format_xml_test2) +{ + xmlpp::DomParser parser; + auto path = private_test / "DKH_UT_EN20160601def.xml"; + parser.parse_file(path.string().c_str()); + auto document = parser.get_document(); + check_xml (dcp::file_to_string(private_test / "DKH_UT_EN20160601def.reformatted.xml"), dcp::SubtitleAsset::format_xml(*document, {}), {}); +} + + +BOOST_AUTO_TEST_CASE (format_xml_entities_test) +{ + xmlpp::Document doc; + auto root = doc.create_root_node("Foo"); + root->add_child("Bar")->add_child_text("Don't panic & xml \"is\" 'great' & < > —"); + BOOST_REQUIRE_EQUAL(dcp::SubtitleAsset::format_xml(doc, {}), +"\n" +"\n" +" Don't panic &amp; xml \"is\" 'great' & < > —\n" +"\n"); +} diff --git a/test/smpte_subtitle_test.cc b/test/smpte_subtitle_test.cc index 2b8491a3..f1f66bfb 100644 --- a/test/smpte_subtitle_test.cc +++ b/test/smpte_subtitle_test.cc @@ -257,27 +257,27 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test) c._xml_id = "a6c58cff-3e1e-4b38-acec-a42224475ef6"; check_xml ( - "" - "" - "urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6" - "Test" - "2016-04-01T03:52:00.000+00:00" - "1" - "en" - "24 1" - "24" - "" - "" - "" - "Hello world" - "" - "" - "" - "" - "What's going on" - "" - "" - "" + "\n" + "\n" + " urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6\n" + " Test\n" + " 2016-04-01T03:52:00.000+00:00\n" + " 1\n" + " en\n" + " 24 1\n" + " 24\n" + " \n" + " \n" + " \n" + " Hello world\n" + " \n" + " \n" + " \n" + " \n" + " What's going on\n" + " \n" + " \n" + " \n" "", c.xml_as_string (), vector() @@ -449,31 +449,23 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test2) check_xml ( c.xml_as_string(), - "" - "" - "urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6" - "Test" - "2016-04-01T03:52:00.000+00:00" - "1" - "en" - "24 1" - "24" - "" - "" - "" - "" - "Testing is " - "really" - " fun" - "" - "" - "This is the " - "second" - " line" - "" - "" - "" - "" + "\n" + "\n" + " urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6\n" + " Test\n" + " 2016-04-01T03:52:00.000+00:00\n" + " 1\n" + " en\n" + " 24 1\n" + " 24\n" + " \n" + " \n" + " \n" + " Testing is really fun\n" + " This is the second line\n" + " \n" + " \n" + " \n" "", vector() ); @@ -593,23 +585,23 @@ BOOST_AUTO_TEST_CASE (write_subtitles_in_vertical_order_with_top_alignment) check_xml ( c.xml_as_string(), - "" - "" - "urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6" - "Test" - "2016-04-01T03:52:00.000+00:00" - "1" - "en" - "24 1" - "24" - "" - "" - "" - "Top line" - "Bottom line" - "" - "" - "" + "\n" + "\n" + " urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6\n" + " Test\n" + " 2016-04-01T03:52:00.000+00:00\n" + " 1\n" + " en\n" + " 24 1\n" + " 24\n" + " \n" + " \n" + " \n" + " Top line\n" + " Bottom line\n" + " \n" + " \n" + " \n" "", {} ); @@ -679,23 +671,23 @@ BOOST_AUTO_TEST_CASE (write_subtitles_in_vertical_order_with_bottom_alignment) check_xml ( c.xml_as_string(), - "" - "" - "urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6" - "Test" - "2016-04-01T03:52:00.000+00:00" - "1" - "en" - "24 1" - "24" - "" - "" - "" - "Top line" - "Bottom line" - "" - "" - "" + "\n" + "\n" + " urn:uuid:a6c58cff-3e1e-4b38-acec-a42224475ef6\n" + " Test\n" + " 2016-04-01T03:52:00.000+00:00\n" + " 1\n" + " en\n" + " 24 1\n" + " 24\n" + " \n" + " \n" + " \n" + " Top line\n" + " Bottom line\n" + " \n" + " \n" + " \n" "", {} ); diff --git a/test/verify_test.cc b/test/verify_test.cc index b2e98981..6a3dacc4 100644 --- a/test/verify_test.cc +++ b/test/verify_test.cc @@ -1292,7 +1292,7 @@ BOOST_AUTO_TEST_CASE (verify_invalid_closed_caption_xml_size_in_bytes) { dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::INVALID_CLOSED_CAPTION_XML_SIZE_IN_BYTES, - string("372207"), + string("419346"), canonical(dir / "subs.mxf") }, { dcp::VerificationNote::Type::WARNING, dcp::VerificationNote::Code::INVALID_SUBTITLE_FIRST_TEXT_TIME }, @@ -1332,7 +1332,7 @@ verify_timed_text_asset_too_large (string name) check_verify_result ( { dir }, { - { dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::INVALID_TIMED_TEXT_SIZE_IN_BYTES, string("121695136"), canonical(dir / "subs.mxf") }, + { dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::INVALID_TIMED_TEXT_SIZE_IN_BYTES, string("121695542"), canonical(dir / "subs.mxf") }, { dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::INVALID_TIMED_TEXT_FONT_SIZE_IN_BYTES, string("121634816"), canonical(dir / "subs.mxf") }, { dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::MISSING_SUBTITLE_START_TIME, canonical(dir / "subs.mxf") }, { dcp::VerificationNote::Type::WARNING, dcp::VerificationNote::Code::INVALID_SUBTITLE_FIRST_TEXT_TIME }, -- cgit v1.2.3