Neater and more correct XML subtitle parser.

11 files changed, 298 insertions, 678 deletions

diff --git a/src/font_node.cc b/src/font_node.cc
deleted file mode 100644
index 1aa4be6b..00000000
--- a/src/font_node.cc
+++ /dev/null
@@ -1,132 +0,0 @@
-/*
-    Copyright (C) 2012-2016 Carl Hetherington <cth@carlh.net>
-
-    This file is part of libdcp.
-
-    libdcp is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    libdcp is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with libdcp.  If not, see <http://www.gnu.org/licenses/>.
-
-    In addition, as a special exception, the copyright holders give
-    permission to link the code of portions of this program with the
-    OpenSSL library under certain conditions as described in each
-    individual source file, and distribute linked combinations
-    including the two.
-
-    You must obey the GNU General Public License in all respects
-    for all of the code used other than OpenSSL.  If you modify
-    file(s) with this exception, you may extend this exception to your
-    version of the file(s), but you are not obligated to do so.  If you
-    do not wish to do so, delete this exception statement from your
-    version.  If you delete this exception statement from all source
-    files in the program, then also delete it here.
-*/
-
-#include "types.h"
-#include "raw_convert.h"
-#include "font_node.h"
-#include "xml.h"
-#include "text_node.h"
-#include <libcxml/cxml.h>
-#include <boost/foreach.hpp>
-
-using std::string;
-using std::list;
-using boost::shared_ptr;
-using boost::optional;
-using namespace dcp;
-
-FontNode::FontNode (cxml::ConstNodePtr node, optional<int> tcr, Standard standard)
-{
-	text = node->content ();
-
-	if (standard == INTEROP) {
-		id = node->optional_string_attribute ("Id");
-	} else {
-		id = node->optional_string_attribute ("ID");
-	}
-	size = node->optional_number_attribute<int64_t> ("Size").get_value_or (0);
-	aspect_adjust = node->optional_number_attribute<float> ("AspectAdjust");
-	italic = node->optional_bool_attribute ("Italic");
-	bold = node->optional_string_attribute("Weight").get_value_or("normal") == "bold";
-	if (standard == INTEROP) {
-		underline = node->optional_bool_attribute ("Underlined");
-	} else {
-		underline = node->optional_bool_attribute ("Underline");
-	}
-	optional<string> c = node->optional_string_attribute ("Color");
-	if (c) {
-		colour = Colour (c.get ());
-	}
-	optional<string> const e = node->optional_string_attribute ("Effect");
-	if (e) {
-		effect = string_to_effect (e.get ());
-	}
-	c = node->optional_string_attribute ("EffectColor");
-	if (c) {
-		effect_colour = Colour (c.get ());
-	}
-
-	list<cxml::NodePtr> s = node->node_children ("Subtitle");
-	BOOST_FOREACH (cxml::NodePtr& i, s) {
-		subtitle_nodes.push_back (shared_ptr<SubtitleNode> (new SubtitleNode (i, tcr, standard)));
-	}
-
-	list<cxml::NodePtr> f = node->node_children ("Font");
-	BOOST_FOREACH (cxml::NodePtr& i, f) {
-		font_nodes.push_back (shared_ptr<FontNode> (new FontNode (i, tcr, standard)));
-	}
-
-	list<cxml::NodePtr> t = node->node_children ("Text");
-	BOOST_FOREACH (cxml::NodePtr& i, t) {
-		text_nodes.push_back (shared_ptr<TextNode> (new TextNode (i, tcr, standard)));
-	}
-}
-
-FontNode::FontNode (std::list<boost::shared_ptr<FontNode> > const & font_nodes)
-	: size (0)
-	, italic (false)
-	, bold (false)
-	, underline (false)
-	, colour ("FFFFFFFF")
-	, effect_colour ("FFFFFFFF")
-{
-	for (list<shared_ptr<FontNode> >::const_iterator i = font_nodes.begin(); i != font_nodes.end(); ++i) {
-		if ((*i)->id) {
-			id = (*i)->id;
-		}
-		if ((*i)->size != 0) {
-			size = (*i)->size;
-		}
-		if ((*i)->aspect_adjust) {
-			aspect_adjust = (*i)->aspect_adjust.get ();
-		}
-		if ((*i)->italic) {
-			italic = (*i)->italic.get ();
-		}
-		if ((*i)->bold) {
-			bold = (*i)->bold.get ();
-		}
-		if ((*i)->underline) {
-			underline = (*i)->underline.get ();
-		}
-		if ((*i)->colour) {
-			colour = (*i)->colour.get ();
-		}
-		if ((*i)->effect) {
-			effect = (*i)->effect.get ();
-		}
-		if ((*i)->effect_colour) {
-			effect_colour = (*i)->effect_colour.get ();
-		}
-	}
-}
diff --git a/src/font_node.h b/src/font_node.h
deleted file mode 100644
index 2108af9e..00000000
--- a/src/font_node.h
+++ /dev/null
@@ -1,76 +0,0 @@
-/*
-    Copyright (C) 2012-2016 Carl Hetherington <cth@carlh.net>
-
-    This file is part of libdcp.
-
-    libdcp is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    libdcp is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with libdcp.  If not, see <http://www.gnu.org/licenses/>.
-
-    In addition, as a special exception, the copyright holders give
-    permission to link the code of portions of this program with the
-    OpenSSL library under certain conditions as described in each
-    individual source file, and distribute linked combinations
-    including the two.
-
-    You must obey the GNU General Public License in all respects
-    for all of the code used other than OpenSSL.  If you modify
-    file(s) with this exception, you may extend this exception to your
-    version of the file(s), but you are not obligated to do so.  If you
-    do not wish to do so, delete this exception statement from your
-    version.  If you delete this exception statement from all source
-    files in the program, then also delete it here.
-*/
-
-/** @file  src/font_node.h
- *  @brief FontNode class
- */
-
-#include "types.h"
-#include "subtitle_node.h"
-#include <libcxml/cxml.h>
-#include <boost/shared_ptr.hpp>
-#include <boost/optional.hpp>
-#include <list>
-
-namespace dcp {
-
-/** @class FontNode
- *  @brief Helper class for parsing subtitle XML.
- */
-class FontNode
-{
-public:
-	FontNode ()
-		: size (0)
-	{}
-
-	FontNode (cxml::ConstNodePtr node, boost::optional<int> tcr, Standard standard);
-	explicit FontNode (std::list<boost::shared_ptr<FontNode> > const & font_nodes);
-
-	std::string text;
-	boost::optional<std::string> id;
-	int size;
-	boost::optional<float> aspect_adjust;
-	boost::optional<bool> italic;
-	boost::optional<bool> bold;
-	boost::optional<bool> underline;
-	boost::optional<Colour> colour;
-	boost::optional<Effect> effect;
-	boost::optional<Colour> effect_colour;
-
-	std::list<boost::shared_ptr<SubtitleNode> > subtitle_nodes;
-	std::list<boost::shared_ptr<FontNode> > font_nodes;
-	std::list<boost::shared_ptr<TextNode> > text_nodes;
-};
-
-}
diff --git a/src/interop_subtitle_asset.cc b/src/interop_subtitle_asset.cc
index 24b4cd00..99511222 100644
--- a/src/interop_subtitle_asset.cc
+++ b/src/interop_subtitle_asset.cc
@@ -35,7 +35,6 @@
 #include "interop_load_font_node.h"
 #include "xml.h"
 #include "raw_convert.h"
-#include "font_node.h"
 #include "util.h"
 #include "font_asset.h"
 #include "dcp_assert.h"
@@ -66,17 +65,16 @@ InteropSubtitleAsset::InteropSubtitleAsset (boost::filesystem::path file)
 	_movie_title = xml->string_child ("MovieTitle");
 	_load_font_nodes = type_children<dcp::InteropLoadFontNode> (xml, "LoadFont");
 
-	list<shared_ptr<dcp::FontNode> > font_nodes;
-	BOOST_FOREACH (cxml::NodePtr const & i, xml->node_children ("Font")) {
-		font_nodes.push_back (shared_ptr<FontNode> (new FontNode (i, optional<int>(), INTEROP)));
-	}
+	/* Now we need to drop down to xmlpp */
 
-	list<shared_ptr<dcp::SubtitleNode> > subtitle_nodes;
-	BOOST_FOREACH (cxml::NodePtr const & i, xml->node_children ("Subtitle")) {
-		subtitle_nodes.push_back (shared_ptr<SubtitleNode> (new SubtitleNode (i, optional<int>(), INTEROP)));
+	list<ParseState> ps;
+	xmlpp::Node::NodeList c = xml->node()->get_children ();
+	for (xmlpp::Node::NodeList::const_iterator i = c.begin(); i != c.end(); ++i) {
+		xmlpp::Element const * e = dynamic_cast<xmlpp::Element const *> (*i);
+		if (e && (e->get_name() == "Font" || e->get_name() == "Subtitle")) {
+			parse_subtitles (e, ps, optional<int>(), INTEROP);
+		}
 	}
-
-	parse_subtitles (xml, font_nodes, subtitle_nodes);
 }
 
 InteropSubtitleAsset::InteropSubtitleAsset ()
diff --git a/src/smpte_subtitle_asset.cc b/src/smpte_subtitle_asset.cc
index c58162b3..128729d8 100644
--- a/src/smpte_subtitle_asset.cc
+++ b/src/smpte_subtitle_asset.cc
@@ -37,7 +37,6 @@
 
 #include "smpte_subtitle_asset.h"
 #include "smpte_load_font_node.h"
-#include "font_node.h"
 #include "exceptions.h"
 #include "xml.h"
 #include "raw_convert.h"
@@ -139,20 +138,17 @@ SMPTESubtitleAsset::parse_xml (shared_ptr<cxml::Document> xml)
 		_start_time = Time (xml->string_child ("StartTime"), _time_code_rate);
 	}
 
-	shared_ptr<cxml::Node> subtitle_list = xml->optional_node_child ("SubtitleList");
+	/* Now we need to drop down to xmlpp */
 
-	list<shared_ptr<dcp::FontNode> > font_nodes;
-	BOOST_FOREACH (cxml::NodePtr const & i, subtitle_list->node_children ("Font")) {
-		font_nodes.push_back (shared_ptr<FontNode> (new FontNode (i, _time_code_rate, SMPTE)));
-	}
-
-	list<shared_ptr<dcp::SubtitleNode> > subtitle_nodes;
-	BOOST_FOREACH (cxml::NodePtr const & i, subtitle_list->node_children ("Subtitle")) {
-		subtitle_nodes.push_back (shared_ptr<SubtitleNode> (new SubtitleNode (i, _time_code_rate, SMPTE)));
+	list<ParseState> ps;
+	xmlpp::Node::NodeList c = xml->node()->get_children ();
+	for (xmlpp::Node::NodeList::const_iterator i = c.begin(); i != c.end(); ++i) {
+		xmlpp::Element const * e = dynamic_cast<xmlpp::Element const *> (*i);
+		if (e && e->get_name() == "SubtitleList") {
+			parse_subtitles (e, ps, _time_code_rate, SMPTE);
+		}
 	}
 
-	parse_subtitles (xml, font_nodes, subtitle_nodes);
-
 	/* Guess intrinsic duration */
 	_intrinsic_duration = latest_subtitle_out().as_editable_units (_edit_rate.numerator / _edit_rate.denominator);
 }
diff --git a/src/subtitle_asset.cc b/src/subtitle_asset.cc
index b9f6336c..02dfd80f 100644
--- a/src/subtitle_asset.cc
+++ b/src/subtitle_asset.cc
@@ -32,17 +32,17 @@
 */
 
 #include "raw_convert.h"
+#include "compose.hpp"
 #include "subtitle_asset.h"
 #include "util.h"
 #include "xml.h"
-#include "font_node.h"
-#include "text_node.h"
 #include "subtitle_string.h"
 #include "dcp_assert.h"
 #include <asdcp/AS_DCP.h>
 #include <asdcp/KM_util.h>
 #include <libxml++/nodes/element.h>
 #include <boost/algorithm/string.hpp>
+#include <boost/lexical_cast.hpp>
 #include <boost/shared_array.hpp>
 #include <boost/foreach.hpp>
 
@@ -55,6 +55,7 @@ using boost::shared_ptr;
 using boost::shared_array;
 using boost::optional;
 using boost::dynamic_pointer_cast;
+using boost::lexical_cast;
 using namespace dcp;
 
 SubtitleAsset::SubtitleAsset ()
@@ -68,108 +69,289 @@ SubtitleAsset::SubtitleAsset (boost::filesystem::path file)
 
 }
 
-void
-SubtitleAsset::parse_subtitles (
-	shared_ptr<cxml::Document> xml,
-	list<shared_ptr<dcp::FontNode> > font_nodes,
-	list<shared_ptr<dcp::SubtitleNode> > subtitle_nodes
-	)
+string
+string_attribute (xmlpp::Element const * node, string name)
 {
-	/* Make Subtitle objects to represent the raw XML nodes in a sane way */
-	ParseState parse_state;
-	examine_nodes (xml, font_nodes, parse_state);
-	examine_nodes (xml, subtitle_nodes, parse_state);
+	xmlpp::Attribute* a = node->get_attribute (name);
+	if (!a) {
+		throw XMLError (String::compose ("missing attribute %1", name));
+	}
+	return string (a->get_value ());
 }
 
-void
-SubtitleAsset::examine_nodes (
-	shared_ptr<const cxml::Node> xml,
-	list<shared_ptr<dcp::SubtitleNode> > const & subtitle_nodes,
-	ParseState& parse_state
-	)
+optional<string>
+optional_string_attribute (xmlpp::Element const * node, string name)
 {
-	BOOST_FOREACH (shared_ptr<dcp::SubtitleNode> i, subtitle_nodes) {
-		parse_state.subtitle_nodes.push_back (i);
-		examine_nodes (xml, i->text_nodes, parse_state);
-		examine_nodes (xml, i->font_nodes, parse_state);
-		parse_state.subtitle_nodes.pop_back ();
+	xmlpp::Attribute* a = node->get_attribute (name);
+	if (!a) {
+		return optional<string>();
 	}
+	return string (a->get_value ());
 }
 
-void
-SubtitleAsset::examine_nodes (
-	shared_ptr<const cxml::Node> xml,
-	list<shared_ptr<dcp::FontNode> > const & font_nodes,
-	ParseState& parse_state
-	)
+optional<bool>
+optional_bool_attribute (xmlpp::Element const * node, string name)
+{
+	optional<string> s = optional_string_attribute (node, name);
+	if (!s) {
+		return optional<bool> ();
+	}
+
+	return (s.get() == "1" || s.get() == "yes");
+}
+
+template <class T>
+optional<T>
+optional_number_attribute (xmlpp::Element const * node, string name)
+{
+	boost::optional<std::string> s = optional_string_attribute (node, name);
+	if (!s) {
+		return boost::optional<T> ();
+	}
+
+	std::string t = s.get ();
+	boost::erase_all (t, " ");
+	locked_stringstream u;
+	u.imbue (std::locale::classic ());
+	u << t;
+	T n;
+	u >> n;
+	return n;
+}
+
+SubtitleAsset::ParseState
+SubtitleAsset::font_node_state (xmlpp::Element const * node, Standard standard) const
+{
+	ParseState ps;
+
+	if (standard == INTEROP) {
+		ps.font_id = optional_string_attribute (node, "Id");
+	} else {
+		ps.font_id = optional_string_attribute (node, "ID");
+	}
+	ps.size = optional_number_attribute<int64_t> (node, "Size");
+	ps.aspect_adjust = optional_number_attribute<float> (node, "AspectAdjust");
+	ps.italic = optional_bool_attribute (node, "Italic");
+	ps.bold = optional_string_attribute(node, "Weight").get_value_or("normal") == "bold";
+	if (standard == INTEROP) {
+		ps.underline = optional_bool_attribute (node, "Underlined");
+	} else {
+		ps.underline = optional_bool_attribute (node, "Underline");
+	}
+	optional<string> c = optional_string_attribute (node, "Color");
+	if (c) {
+		ps.colour = Colour (c.get ());
+	}
+	optional<string> const e = optional_string_attribute (node, "Effect");
+	if (e) {
+		ps.effect = string_to_effect (e.get ());
+	}
+	c = optional_string_attribute (node, "EffectColor");
+	if (c) {
+		ps.effect_colour = Colour (c.get ());
+	}
+
+	return ps;
+}
+
+SubtitleAsset::ParseState
+SubtitleAsset::text_node_state (xmlpp::Element const * node) const
+{
+	ParseState ps;
+
+	optional<float> hp = optional_number_attribute<float> (node, "HPosition");
+	if (!hp) {
+		hp = optional_number_attribute<float> (node, "Hposition");
+	}
+	if (hp) {
+		ps.h_position = hp.get () / 100;
+	}
+
+	optional<string> ha = optional_string_attribute (node, "HAlign");
+	if (!ha) {
+		ha = optional_string_attribute (node, "Halign");
+	}
+	if (ha) {
+		ps.h_align = string_to_halign (ha.get ());
+	}
+
+	optional<float> vp = optional_number_attribute<float> (node, "VPosition");
+	if (!vp) {
+		vp = optional_number_attribute<float> (node, "Vposition");
+	}
+	if (vp) {
+		ps.v_position = vp.get () / 100;
+	}
+
+	optional<string> va = optional_string_attribute (node, "VAlign");
+	if (!va) {
+		va = optional_string_attribute (node, "Valign");
+	}
+	if (va) {
+		ps.v_align = string_to_valign (va.get ());
+	}
+
+	optional<string> d = optional_string_attribute (node, "Direction");
+	if (d) {
+		ps.direction = string_to_direction (d.get ());
+	}
+
+	return ps;
+}
+
+SubtitleAsset::ParseState
+SubtitleAsset::subtitle_node_state (xmlpp::Element const * node, optional<int> tcr) const
 {
-	BOOST_FOREACH (shared_ptr<dcp::FontNode> i, font_nodes) {
+	ParseState ps;
+	ps.in = Time (string_attribute(node, "TimeIn"), tcr);
+	ps.out = Time (string_attribute(node, "TimeOut"), tcr);
+	ps.fade_up_time = fade_time (node, "FadeUpTime", tcr);
+	ps.fade_down_time = fade_time (node, "FadeDownTime", tcr);
+	return ps;
+}
 
-		parse_state.font_nodes.push_back (i);
-		maybe_add_subtitle (i->text, parse_state);
+Time
+SubtitleAsset::fade_time (xmlpp::Element const * node, string name, optional<int> tcr) const
+{
+	string const u = optional_string_attribute(node, name).get_value_or ("");
+	Time t;
 
-		examine_nodes (xml, i->subtitle_nodes, parse_state);
-		examine_nodes (xml, i->font_nodes, parse_state);
-		examine_nodes (xml, i->text_nodes, parse_state);
+	if (u.empty ()) {
+		t = Time (0, 0, 0, 20, 250);
+	} else if (u.find (":") != string::npos) {
+		t = Time (u, tcr);
+	} else {
+		t = Time (0, 0, 0, lexical_cast<int> (u), tcr.get_value_or(250));
+	}
 
-		parse_state.font_nodes.pop_back ();
+	if (t > Time (0, 0, 8, 0, 250)) {
+		t = Time (0, 0, 8, 0, 250);
 	}
+
+	return t;
 }
 
 void
-SubtitleAsset::examine_nodes (
-	shared_ptr<const cxml::Node> xml,
-	list<shared_ptr<dcp::TextNode> > const & text_nodes,
-	ParseState& parse_state
-	)
+SubtitleAsset::parse_subtitles (xmlpp::Element const * node, list<ParseState>& state, optional<int> tcr, Standard standard)
 {
-	BOOST_FOREACH (shared_ptr<dcp::TextNode> i, text_nodes) {
-		parse_state.text_nodes.push_back (i);
-		maybe_add_subtitle (i->text, parse_state);
-		examine_nodes (xml, i->font_nodes, parse_state);
-		parse_state.text_nodes.pop_back ();
+	if (node->get_name() == "Font") {
+		state.push_back (font_node_state (node, standard));
+	} else if (node->get_name() == "Subtitle") {
+		state.push_back (subtitle_node_state (node, tcr));
+	} else if (node->get_name() == "Text") {
+		state.push_back (text_node_state (node));
+	} else if (node->get_name() == "SubtitleList") {
+		state.push_back (ParseState ());
+	} else {
+		throw XMLError ("unexpected node " + node->get_name());
+	}
+
+	xmlpp::Node::NodeList c = node->get_children ();
+	for (xmlpp::Node::NodeList::const_iterator i = c.begin(); i != c.end(); ++i) {
+		xmlpp::ContentNode const * v = dynamic_cast<xmlpp::ContentNode const *> (*i);
+		if (v) {
+			maybe_add_subtitle (v->get_content(), state);
+		}
+		xmlpp::Element const * e = dynamic_cast<xmlpp::Element const *> (*i);
+		if (e) {
+			parse_subtitles (e, state, tcr, standard);
+		}
 	}
+
+	state.pop_back ();
 }
 
 void
-SubtitleAsset::maybe_add_subtitle (string text, ParseState const & parse_state)
+SubtitleAsset::maybe_add_subtitle (string text, list<ParseState> const & parse_state)
 {
 	if (empty_or_white_space (text)) {
 		return;
 	}
 
-	if (parse_state.text_nodes.empty() || parse_state.subtitle_nodes.empty ()) {
-		return;
+	ParseState ps;
+	BOOST_FOREACH (ParseState const & i, parse_state) {
+		if (i.font_id) {
+			ps.font_id = i.font_id.get();
+		}
+		if (i.size) {
+			ps.size = i.size.get();
+		}
+		if (i.aspect_adjust) {
+			ps.aspect_adjust = i.aspect_adjust.get();
+		}
+		if (i.italic) {
+			ps.italic = i.italic.get();
+		}
+		if (i.bold) {
+			ps.bold = i.bold.get();
+		}
+		if (i.underline) {
+			ps.underline = i.underline.get();
+		}
+		if (i.colour) {
+			ps.colour = i.colour.get();
+		}
+		if (i.effect) {
+			ps.effect = i.effect.get();
+		}
+		if (i.effect_colour) {
+			ps.effect_colour = i.effect_colour.get();
+		}
+		if (i.h_position) {
+			ps.h_position = i.h_position.get();
+		}
+		if (i.h_align) {
+			ps.h_align = i.h_align.get();
+		}
+		if (i.v_position) {
+			ps.v_position = i.v_position.get();
+		}
+		if (i.v_align) {
+			ps.v_align = i.v_align.get();
+		}
+		if (i.direction) {
+			ps.direction = i.direction.get();
+		}
+		if (i.in) {
+			ps.in = i.in.get();
+		}
+		if (i.out) {
+			ps.out = i.out.get();
+		}
+		if (i.fade_up_time) {
+			ps.fade_up_time = i.fade_up_time.get();
+		}
+		if (i.fade_down_time) {
+			ps.fade_down_time = i.fade_down_time.get();
+		}
 	}
 
-	DCP_ASSERT (!parse_state.text_nodes.empty ());
-	DCP_ASSERT (!parse_state.subtitle_nodes.empty ());
-
-	dcp::FontNode effective_font (parse_state.font_nodes);
-	dcp::TextNode effective_text (*parse_state.text_nodes.back ());
-	dcp::SubtitleNode effective_subtitle (*parse_state.subtitle_nodes.back ());
+	if (!ps.in || !ps.out) {
+		/* We're not in a <Text> node; just ignore this content */
+		return;
+	}
 
 	_subtitles.push_back (
 		SubtitleString (
-			effective_font.id,
-			effective_font.italic.get_value_or (false),
-			effective_font.bold.get_value_or (false),
-			effective_font.underline.get_value_or (false),
-			effective_font.colour.get_value_or (dcp::Colour (255, 255, 255)),
-			effective_font.size,
-			effective_font.aspect_adjust.get_value_or (1.0),
-			effective_subtitle.in,
-			effective_subtitle.out,
-			effective_text.h_position,
-			effective_text.h_align,
-			effective_text.v_position,
-			effective_text.v_align,
-			effective_text.direction,
+			ps.font_id,
+			ps.italic.get_value_or (false),
+			ps.bold.get_value_or (false),
+			ps.underline.get_value_or (false),
+			ps.colour.get_value_or (dcp::Colour (255, 255, 255)),
+			ps.size.get_value_or (42),
+			ps.aspect_adjust.get_value_or (1.0),
+			ps.in.get(),
+			ps.out.get(),
+			ps.h_position.get_value_or(0),
+			ps.h_align.get_value_or(HALIGN_CENTER),
+			ps.v_position.get_value_or(0),
+			ps.v_align.get_value_or(VALIGN_CENTER),
+			ps.direction.get_value_or (DIRECTION_LTR),
 			text,
-			effective_font.effect.get_value_or (NONE),
-			effective_font.effect_colour.get_value_or (dcp::Colour (0, 0, 0)),
-			effective_subtitle.fade_up_time,
-			effective_subtitle.fade_down_time
+			ps.effect.get_value_or (NONE),
+			ps.effect_colour.get_value_or (dcp::Colour (255, 255, 255)),
+			ps.fade_up_time.get_value_or(Time()),
+			ps.fade_down_time.get_value_or(Time())
 			)
 		);
 }
diff --git a/src/subtitle_asset.h b/src/subtitle_asset.h
index d212579f..62314263 100644
--- a/src/subtitle_asset.h
+++ b/src/subtitle_asset.h
@@ -98,11 +98,32 @@ protected:
 	friend struct ::interop_dcp_font_test;
 	friend struct ::smpte_dcp_font_test;
 
-	void parse_subtitles (
-		boost::shared_ptr<cxml::Document> xml,
-		std::list<boost::shared_ptr<FontNode> > font_nodes,
-		std::list<boost::shared_ptr<SubtitleNode> > subtitle_nodes
-		);
+	struct ParseState {
+		boost::optional<std::string> font_id;
+		boost::optional<int64_t> size;
+		boost::optional<float> aspect_adjust;
+		boost::optional<bool> italic;
+		boost::optional<bool> bold;
+		boost::optional<bool> underline;
+		boost::optional<Colour> colour;
+		boost::optional<Effect> effect;
+		boost::optional<Colour> effect_colour;
+		boost::optional<float> h_position;
+		boost::optional<HAlign> h_align;
+		boost::optional<float> v_position;
+		boost::optional<VAlign> v_align;
+		boost::optional<Direction> direction;
+		boost::optional<Time> in;
+		boost::optional<Time> out;
+		boost::optional<Time> fade_up_time;
+		boost::optional<Time> fade_down_time;
+	};
+
+	void parse_subtitles (xmlpp::Element const * node, std::list<ParseState>& state, boost::optional<int> tcr, Standard standard);
+	ParseState font_node_state (xmlpp::Element const * node, Standard standard) const;
+	ParseState text_node_state (xmlpp::Element const * node) const;
+	ParseState subtitle_node_state (xmlpp::Element const * node, boost::optional<int> tcr) const;
+	Time fade_time (xmlpp::Element const * node, std::string name, boost::optional<int> tcr) const;
 
 	void subtitles_as_xml (xmlpp::Element* root, int time_code_rate, Standard standard) const;
 
@@ -135,34 +156,7 @@ protected:
 	std::list<Font> _fonts;
 
 private:
-	/** @struct ParseState
-	 *  @brief  A struct to hold state when parsing a subtitle XML file.
-	 */
-	struct ParseState {
-		std::list<boost::shared_ptr<FontNode> > font_nodes;
-		std::list<boost::shared_ptr<TextNode> > text_nodes;
-		std::list<boost::shared_ptr<SubtitleNode> > subtitle_nodes;
-	};
-
-	void maybe_add_subtitle (std::string text, ParseState const & parse_state);
-
-	void examine_nodes (
-		boost::shared_ptr<const cxml::Node> xml,
-		std::list<boost::shared_ptr<FontNode> > const & font_nodes,
-		ParseState& parse_state
-		);
-
-	void examine_nodes (
-		boost::shared_ptr<const cxml::Node> xml,
-		std::list<boost::shared_ptr<TextNode> > const & text_nodes,
-		ParseState& parse_state
-		);
-
-	void examine_nodes (
-		boost::shared_ptr<const cxml::Node> xml,
-		std::list<boost::shared_ptr<SubtitleNode> > const & subtitle_nodes,
-		ParseState& parse_state
-		);
+	void maybe_add_subtitle (std::string text, std::list<ParseState> const & parse_state);
 };
 
 }
diff --git a/src/subtitle_node.cc b/src/subtitle_node.cc
deleted file mode 100644
index 82c11401..00000000
--- a/src/subtitle_node.cc
+++ /dev/null
@@ -1,87 +0,0 @@
-/*
-    Copyright (C) 2012-2016 Carl Hetherington <cth@carlh.net>
-
-    This file is part of libdcp.
-
-    libdcp is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    libdcp is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with libdcp.  If not, see <http://www.gnu.org/licenses/>.
-
-    In addition, as a special exception, the copyright holders give
-    permission to link the code of portions of this program with the
-    OpenSSL library under certain conditions as described in each
-    individual source file, and distribute linked combinations
-    including the two.
-
-    You must obey the GNU General Public License in all respects
-    for all of the code used other than OpenSSL.  If you modify
-    file(s) with this exception, you may extend this exception to your
-    version of the file(s), but you are not obligated to do so.  If you
-    do not wish to do so, delete this exception statement from your
-    version.  If you delete this exception statement from all source
-    files in the program, then also delete it here.
-*/
-
-#include "subtitle_node.h"
-#include "xml.h"
-#include "font_node.h"
-#include "text_node.h"
-#include <libcxml/cxml.h>
-#include <boost/lexical_cast.hpp>
-
-using std::string;
-using std::list;
-using boost::optional;
-using boost::shared_ptr;
-using boost::lexical_cast;
-using namespace dcp;
-
-/** @param tcr Timecode rate for SMPTE, or empty for Interop */
-SubtitleNode::SubtitleNode (boost::shared_ptr<const cxml::Node> node, optional<int> tcr, Standard standard)
-{
-	in = Time (node->string_attribute ("TimeIn"), tcr);
-	out = Time (node->string_attribute ("TimeOut"), tcr);
-
-	list<cxml::NodePtr> f = node->node_children ("Font");
-	for (list<cxml::NodePtr>::iterator i = f.begin(); i != f.end(); ++i) {
-		font_nodes.push_back (shared_ptr<FontNode> (new FontNode (*i, tcr, standard)));
-	}
-
-	list<cxml::NodePtr> t = node->node_children ("Text");
-	for (list<cxml::NodePtr>::iterator i = t.begin(); i != t.end(); ++i) {
-		text_nodes.push_back (shared_ptr<TextNode> (new TextNode (*i, tcr, standard)));
-	}
-
-	fade_up_time = fade_time (node, "FadeUpTime", tcr);
-	fade_down_time = fade_time (node, "FadeDownTime", tcr);
-}
-
-Time
-SubtitleNode::fade_time (shared_ptr<const cxml::Node> node, string name, optional<int> tcr)
-{
-	string const u = node->optional_string_attribute (name).get_value_or ("");
-	Time t;
-
-	if (u.empty ()) {
-		t = Time (0, 0, 0, 20, 250);
-	} else if (u.find (":") != string::npos) {
-		t = Time (u, tcr);
-	} else {
-		t = Time (0, 0, 0, lexical_cast<int> (u), tcr.get_value_or(250));
-	}
-
-	if (t > Time (0, 0, 8, 0, 250)) {
-		t = Time (0, 0, 8, 0, 250);
-	}
-
-	return t;
-}
diff --git a/src/subtitle_node.h b/src/subtitle_node.h
deleted file mode 100644
index e08ecfd4..00000000
--- a/src/subtitle_node.h
+++ /dev/null
@@ -1,70 +0,0 @@
-/*
-    Copyright (C) 2012-2016 Carl Hetherington <cth@carlh.net>
-
-    This file is part of libdcp.
-
-    libdcp is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    libdcp is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with libdcp.  If not, see <http://www.gnu.org/licenses/>.
-
-    In addition, as a special exception, the copyright holders give
-    permission to link the code of portions of this program with the
-    OpenSSL library under certain conditions as described in each
-    individual source file, and distribute linked combinations
-    including the two.
-
-    You must obey the GNU General Public License in all respects
-    for all of the code used other than OpenSSL.  If you modify
-    file(s) with this exception, you may extend this exception to your
-    version of the file(s), but you are not obligated to do so.  If you
-    do not wish to do so, delete this exception statement from your
-    version.  If you delete this exception statement from all source
-    files in the program, then also delete it here.
-*/
-
-#ifndef LIBDCP_SUBTITLE_NODE_H
-#define LIBDCP_SUBTITLE_NODE_H
-
-#include "dcp_time.h"
-#include <boost/shared_ptr.hpp>
-#include <boost/optional.hpp>
-#include <list>
-
-namespace cxml {
-	class Node;
-}
-
-namespace dcp {
-
-class FontNode;
-class TextNode;
-
-class SubtitleNode
-{
-public:
-	SubtitleNode () {}
-	SubtitleNode (boost::shared_ptr<const cxml::Node> node, boost::optional<int> tcr, Standard standard);
-
-	Time in;
-	Time out;
-	Time fade_up_time;
-	Time fade_down_time;
-	std::list<boost::shared_ptr<FontNode> > font_nodes;
-	std::list<boost::shared_ptr<TextNode> > text_nodes;
-
-private:
-	Time fade_time (boost::shared_ptr<const cxml::Node>, std::string name, boost::optional<int> tcr);
-};
-
-}
-
-#endif
diff --git a/src/text_node.cc b/src/text_node.cc
deleted file mode 100644
index 6a48470d..00000000
--- a/src/text_node.cc
+++ /dev/null
@@ -1,104 +0,0 @@
-/*
-    Copyright (C) 2012-2016 Carl Hetherington <cth@carlh.net>
-
-    This file is part of libdcp.
-
-    libdcp is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    libdcp is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with libdcp.  If not, see <http://www.gnu.org/licenses/>.
-
-    In addition, as a special exception, the copyright holders give
-    permission to link the code of portions of this program with the
-    OpenSSL library under certain conditions as described in each
-    individual source file, and distribute linked combinations
-    including the two.
-
-    You must obey the GNU General Public License in all respects
-    for all of the code used other than OpenSSL.  If you modify
-    file(s) with this exception, you may extend this exception to your
-    version of the file(s), but you are not obligated to do so.  If you
-    do not wish to do so, delete this exception statement from your
-    version.  If you delete this exception statement from all source
-    files in the program, then also delete it here.
-*/
-
-/** @file  src/text.cc
- *  @brief TextNode class for parsing subtitle XML.
- */
-
-#include "text_node.h"
-#include "xml.h"
-#include "font_node.h"
-#include <libcxml/cxml.h>
-#include <boost/foreach.hpp>
-
-using std::string;
-using std::list;
-using boost::shared_ptr;
-using boost::optional;
-using namespace dcp;
-
-/** Read a &lt;Text&gt; node from a subtitle XML file, noting its contents
- *  in this object's member variables.
- *  @param node Node to read.
- */
-TextNode::TextNode (boost::shared_ptr<const cxml::Node> node, optional<int> tcr, Standard standard)
-	: h_position (0)
-	, h_align (HALIGN_CENTER)
-	, v_position (0)
-	, v_align (VALIGN_CENTER)
-	, direction (DIRECTION_LTR)
-{
-	text = node->content ();
-
-	optional<float> hp = node->optional_number_attribute<float> ("HPosition");
-	if (!hp) {
-		hp = node->optional_number_attribute<float> ("Hposition");
-	}
-	if (hp) {
-		h_position = hp.get () / 100;
-	}
-
-	optional<string> ha = node->optional_string_attribute ("HAlign");
-	if (!ha) {
-		ha = node->optional_string_attribute ("Halign");
-	}
-	if (ha) {
-		h_align = string_to_halign (ha.get ());
-	}
-
-	optional<float> vp = node->optional_number_attribute<float> ("VPosition");
-	if (!vp) {
-		vp = node->optional_number_attribute<float> ("Vposition");
-	}
-	if (vp) {
-		v_position = vp.get () / 100;
-	}
-
-	optional<string> va = node->optional_string_attribute ("VAlign");
-	if (!va) {
-		va = node->optional_string_attribute ("Valign");
-	}
-	if (va) {
-		v_align = string_to_valign (va.get ());
-	}
-
-	optional<string> d = node->optional_string_attribute ("Direction");
-	if (d) {
-		direction = string_to_direction (d.get ());
-	}
-
-	list<cxml::NodePtr> f = node->node_children ("Font");
-	BOOST_FOREACH (cxml::NodePtr& i, f) {
-		font_nodes.push_back (shared_ptr<FontNode> (new FontNode (i, tcr, standard)));
-	}
-}
diff --git a/src/text_node.h b/src/text_node.h
deleted file mode 100644
index 21246cb2..00000000
--- a/src/text_node.h
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
-    Copyright (C) 2012-2016 Carl Hetherington <cth@carlh.net>
-
-    This file is part of libdcp.
-
-    libdcp is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    libdcp is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with libdcp.  If not, see <http://www.gnu.org/licenses/>.
-
-    In addition, as a special exception, the copyright holders give
-    permission to link the code of portions of this program with the
-    OpenSSL library under certain conditions as described in each
-    individual source file, and distribute linked combinations
-    including the two.
-
-    You must obey the GNU General Public License in all respects
-    for all of the code used other than OpenSSL.  If you modify
-    file(s) with this exception, you may extend this exception to your
-    version of the file(s), but you are not obligated to do so.  If you
-    do not wish to do so, delete this exception statement from your
-    version.  If you delete this exception statement from all source
-    files in the program, then also delete it here.
-*/
-
-/** @file  src/text.h
- *  @brief TextNode class for parsing subtitle XML.
- */
-
-#include "types.h"
-#include <boost/shared_ptr.hpp>
-#include <boost/optional.hpp>
-#include <list>
-
-namespace cxml {
-	class Node;
-}
-
-namespace dcp {
-
-class FontNode;
-
-/** @class TextNode
- *  @brief Parser for Text nodes from subtitle XML.
- */
-class TextNode
-{
-public:
-	/** Construct a default text node */
-	TextNode ()
-		: h_position (0)
-		, h_align (HALIGN_LEFT)
-		, v_position (0)
-		, v_align (VALIGN_TOP)
-		, direction (DIRECTION_LTR)
-	{}
-
-	TextNode (boost::shared_ptr<const cxml::Node> node, boost::optional<int> tcr, Standard standard);
-
-	float h_position;
-	HAlign h_align;
-	float v_position;
-	VAlign v_align;
-	Direction direction;
-	std::string text;
-	std::list<boost::shared_ptr<FontNode> > font_nodes;
-};
-
-}
diff --git a/src/wscript b/src/wscript
index be7d4621..489d669e 100644
--- a/src/wscript
+++ b/src/wscript
@@ -54,7 +54,6 @@ def build(bld):
              exceptions.cc
              file.cc
              font_asset.cc
-             font_node.cc
              gamma_transfer_function.cc
              interop_load_font_node.cc
              interop_subtitle_asset.cc
@@ -96,10 +95,8 @@ def build(bld):
              stereo_picture_asset_reader.cc
              stereo_picture_asset_writer.cc
              stereo_picture_frame.cc
-             subtitle_node.cc
              subtitle_asset.cc
              subtitle_string.cc
-             text_node.cc
              transfer_function.cc
              types.cc
              util.cc
@@ -167,7 +164,6 @@ def build(bld):
               stereo_picture_asset_reader.h
               stereo_picture_asset_writer.h
               stereo_picture_frame.h
-              subtitle_node.h
               subtitle_asset.h
               subtitle_string.h
               transfer_function.h