Fix numerous bugs in subtitle XML generation.

5 files changed, 97 insertions, 47 deletions

diff --git a/run/test/subs_in_out b/run/test/subs_in_out
new file mode 100755
index 00000000..cd464dac
--- /dev/null
+++ b/run/test/subs_in_out
@@ -0,0 +1,12 @@
+#!/bin/bash
+
+export LD_LIBRARY_PATH=build/src
+if [ "$1" == "--debug" ]; then
+    shift
+    gdb --args build/test/subs_in_out "$@"
+elif [ "$1" == "--valgrind" ]; then
+    shift
+    valgrind --tool="memcheck" --leak-check=full --show-reachable=yes build/test/subs_in_out "$@"
+else
+    build/test/subs_in_out "$@"
+fi
diff --git a/src/subtitle_asset.cc b/src/subtitle_asset.cc
index 15ddb2a8..25eebf95 100644
--- a/src/subtitle_asset.cc
+++ b/src/subtitle_asset.cc
@@ -34,7 +34,22 @@ using namespace libdcp;
 SubtitleAsset::SubtitleAsset (string directory, string xml_file)
 	: Asset (directory, xml_file)
 {
-	shared_ptr<XMLFile> xml (new XMLFile (path().string(), "DCSubtitle"));
+	read_xml (xml_file);
+}
+
+SubtitleAsset::SubtitleAsset (string directory, string movie_title, string language)
+	: Asset (directory)
+	, _movie_title (movie_title)
+	, _reel_number ("1")
+	, _language (language)
+{
+
+}
+
+void
+SubtitleAsset::read_xml (string xml_file)
+{
+	shared_ptr<XMLFile> xml (new XMLFile (xml_file, "DCSubtitle"));
 	
 	_uuid = xml->string_child ("SubtitleID");
 	_movie_title = xml->string_child ("MovieTitle");
@@ -54,15 +69,6 @@ SubtitleAsset::SubtitleAsset (string directory, string xml_file)
 	examine_font_nodes (xml, font_nodes, parse_state);
 }
 
-SubtitleAsset::SubtitleAsset (string directory, string movie_title, string language)
-	: Asset (directory)
-	, _movie_title (movie_title)
-	, _reel_number ("1")
-	, _language (language)
-{
-
-}
-
 void
 SubtitleAsset::examine_font_nodes (
 	shared_ptr<XMLFile> xml,
@@ -134,7 +140,7 @@ SubtitleAsset::maybe_add_subtitle (string text, ParseState const & parse_state)
 				effective_text.v_position,
 				effective_text.v_align,
 				text,
-				effective_font.effect.get(),
+				effective_font.effect ? effective_font.effect.get() : NONE,
 				effective_font.effect_color.get(),
 				effective_subtitle.fade_up_time,
 				effective_subtitle.fade_down_time
@@ -378,7 +384,10 @@ SubtitleAsset::write_to_cpl (ostream& s) const
 
 struct SubtitleSorter {
 	bool operator() (shared_ptr<Subtitle> a, shared_ptr<Subtitle> b) {
-		return a->in() < b->in();
+		if (a->in() != b->in()) {
+			return a->in() < b->in();
+		}
+		return a->v_position() < b->v_position();
 	}
 };
 
@@ -386,14 +395,19 @@ void
 SubtitleAsset::write_xml ()
 {
 	ofstream f (path().string().c_str());
+	write_xml (f);
+}
 
-	f << "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+void
+SubtitleAsset::write_xml (ostream& s)
+{
+	s << "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
 	  << "<DCSubtitle Version=\"1.0\">\n"
 	  << "  <SubtitleID>" << _uuid << "</SubtitleID>\n"
 	  << "  <MovieTitle>" << _movie_title << "</MovieTitle>\n"
 	  << "  <ReelNumber>" << _reel_number << "</ReelNumber>\n"
 	  << "  <Language>" << _language << "</Language>\n"
-	  << "  <LoadFont Id=\"theFontId\" URI=\"arial.ttf\"/>";
+	  << "  <LoadFont Id=\"theFontId\" URI=\"arial.ttf\"/>\n";
 
 	_subtitles.sort (SubtitleSorter ());
 
@@ -414,42 +428,31 @@ SubtitleAsset::write_xml ()
 
 	for (list<shared_ptr<Subtitle> >::iterator i = _subtitles.begin(); i != _subtitles.end(); ++i) {
 
+		/* We will start a new <Font>...</Font> whenever some font property changes.
+		   I suppose should really make an optimal hierarchy of <Font> tags, but
+		   that seems hard.
+		*/
+
+		bool const font_changed = first              ||
+			italic       != (*i)->italic()       ||
+			color        != (*i)->color()        ||
+			size         != (*i)->size()         ||
+			effect       != (*i)->effect()       ||
+			effect_color != (*i)->effect_color();
+
 		stringstream a;
-		if (first || italic != (*i)->italic()) {
+		if (font_changed) {
 			italic = (*i)->italic ();
 			a << "Italic=\"" << (italic ? "yes" : "no") << "\" ";
-		}
-
-		if (first || color != (*i)->color()) {
 			color = (*i)->color ();
 			a << "Color=\"" << color.to_argb_string() << "\" ";
-		}
-
-		if (size || size != (*i)->size()) {
 			size = (*i)->size ();
 			a << "Size=\"" << size << "\" ";
-		}
-
-		if (first || effect != (*i)->effect()) {
 			effect = (*i)->effect ();
 			a << "Effect=\"" << effect_to_string(effect) << "\" ";
-		}
-
-		if (first || effect_color != (*i)->effect_color()) {
 			effect_color = (*i)->effect_color ();
 			a << "EffectColor=\"" << effect_color.to_argb_string() << "\" ";
-		}
-
-		if (first) {
-			a << "Script=\"normal\" Underlined=\"no\" Weight=\"normal\">";
-		}
-
-		if (!a.str().empty()) {
-			if (!first) {
-				f << "  </Font>\n";
-			} else {
-				f << "  <Font Id=\"theFontId\" " << a << ">\n";
-			}
+			a << "Script=\"normal\" Underlined=\"no\" Weight=\"normal\"";
 		}
 
 		if (first ||
@@ -460,15 +463,22 @@ SubtitleAsset::write_xml ()
 			    )) {
 
 			if (!first) {
-				f << "  </Subtitle>\n";
+				s << "  </Subtitle>\n";
+			}
+
+			if (font_changed) {
+				if (!first) {
+					s << "  </Font>\n";
+				}
+				s << "  <Font Id=\"theFontId\" " << a.str() << ">\n";
 			}
 
-			f << "  <Subtitle "
+			s << "  <Subtitle "
 			  << "SpotNumber=\"" << spot_number++ << "\" "
-			  << "TimeIn=" << (*i)->in().to_string() << "\" "
+			  << "TimeIn=\"" << (*i)->in().to_string() << "\" "
 			  << "TimeOut=\"" << (*i)->out().to_string() << "\" "
 			  << "FadeUpTime=\"" << (*i)->fade_up_time().to_ticks() << "\" "
-			  << "FadeDownTime=\"" << (*i)->fade_down_time().to_ticks() << "\" "
+			  << "FadeDownTime=\"" << (*i)->fade_down_time().to_ticks() << "\""
 			  << ">\n";
 
 			last_in = (*i)->in ();
@@ -477,14 +487,15 @@ SubtitleAsset::write_xml ()
 			last_fade_down_time = (*i)->fade_down_time ();
 		}
 
-		f << "      <Text "
+		s << "      <Text "
 		  << "VAlign=\"" << valign_to_string ((*i)->v_align()) << "\" "
-		  << "VPosition=\"" << (*i)->v_position() << "\" "
+		  << "VPosition=\"" << (*i)->v_position() << "\""
 		  << ">" << (*i)->text() << "</Text>\n";
 
 		first = false;
 	}
 
-	f << "  </Subtitle>\n";
-	f << "</Font>\n";
+	s << "  </Subtitle>\n";
+	s << "  </Font>\n";
+	s << "</DCSubtitle>\n";
 }
diff --git a/src/subtitle_asset.h b/src/subtitle_asset.h
index 1b834522..5d996c7a 100644
--- a/src/subtitle_asset.h
+++ b/src/subtitle_asset.h
@@ -201,7 +201,9 @@ public:
 
 	void add (boost::shared_ptr<Subtitle>);
 
+	void read_xml (std::string);
 	void write_xml ();
+	void write_xml (std::ostream& s);
 
 private:
 	std::string font_id_to_name (std::string id) const;
diff --git a/test/subs_in_out.cc b/test/subs_in_out.cc
new file mode 100644
index 00000000..209602e5
--- /dev/null
+++ b/test/subs_in_out.cc
@@ -0,0 +1,17 @@
+#include <iostream>
+#include "subtitle_asset.h"
+
+using namespace std;
+
+int main (int argc, char* argv[])
+{
+	if (argc < 2) {
+		cerr << "Syntax: " << argv[0] << " <subtitle file>\n";
+		exit (EXIT_FAILURE);
+	}
+	
+	libdcp::SubtitleAsset s ("foo", "bar", "baz");
+	s.read_xml (argv[1]);
+	s.write_xml (cout);
+	return 0;
+}
diff --git a/test/wscript b/test/wscript
index 14534bd9..f24683b7 100644
--- a/test/wscript
+++ b/test/wscript
@@ -23,3 +23,11 @@ def build(bld):
     obj.source = 'tests.cc'
     obj.target = 'tests'
     obj.install_path = ''
+
+    obj = bld(features = 'cxx cxxprogram')
+    obj.name   = 'subs_in_out'
+    obj.uselib = 'BOOST_TEST OPENJPEG'
+    obj.use    = 'libdcp'
+    obj.source = 'subs_in_out.cc'
+    obj.target = 'subs_in_out'
+    obj.install_path = ''