Tidy up handling of language metadata for subtitles.

Tried to normalise the idea that subtitle languages are stored as
strings but set as LanguageTags.  This may be too defensive; perhaps
SMPTE language metadata is always right, or perhaps even if it isn't
we should throw errors.

8 files changed, 41 insertions, 25 deletions

diff --git a/src/cpl.cc b/src/cpl.cc
index 2768e797..e995581c 100644
--- a/src/cpl.cc
+++ b/src/cpl.cc
@@ -286,8 +286,8 @@ CPL::read_composition_metadata_asset (cxml::ConstNodePtr node)
 		if (!_reels.empty()) {
 			shared_ptr<dcp::ReelSubtitleAsset> sub = _reels.front()->main_subtitle();
 			if (sub) {
-				optional<dcp::LanguageTag> lang = sub->language();
-				if (lang && lang->to_string() == sll_split[0]) {
+				optional<string> lang = sub->language();
+				if (lang && lang == sll_split[0]) {
 					first = 1;
 				}
 			}
@@ -380,7 +380,7 @@ CPL::maybe_write_composition_metadata_asset (xmlpp::Element* node) const
 	active->add_child("Width", "meta")->add_child_text(raw_convert<string>(_main_picture_active_area->width));
 	active->add_child("Height", "meta")->add_child_text(raw_convert<string>(_main_picture_active_area->height));
 
-	optional<dcp::LanguageTag> first_subtitle_language;
+	optional<string> first_subtitle_language;
 	BOOST_FOREACH (shared_ptr<const Reel> i, _reels) {
 		if (i->main_subtitle()) {
 			first_subtitle_language = i->main_subtitle()->language();
@@ -393,13 +393,13 @@ CPL::maybe_write_composition_metadata_asset (xmlpp::Element* node) const
 	if (first_subtitle_language || !_additional_subtitle_languages.empty()) {
 		string lang;
 		if (first_subtitle_language) {
-			lang = first_subtitle_language->to_string();
+			lang = *first_subtitle_language;
 		}
-		BOOST_FOREACH (dcp::LanguageTag const& i, _additional_subtitle_languages) {
+		BOOST_FOREACH (string const& i, _additional_subtitle_languages) {
 			if (!lang.empty()) {
 				lang += " ";
 			}
-			lang += i.to_string();
+			lang += i;
 		}
 		meta->add_child("MainSubtitleLanguageList")->add_child_text(lang);
 	}
diff --git a/src/language_tag.h b/src/language_tag.h
index a419914c..a0beaad0 100644
--- a/src/language_tag.h
+++ b/src/language_tag.h
@@ -171,7 +171,7 @@ public:
 	};
 
 	LanguageTag () {}
-	LanguageTag (std::string tag);
+	explicit LanguageTag (std::string tag);
 
 	boost::optional<LanguageSubtag> language() const {
 		return _language;
diff --git a/src/reel_subtitle_asset.cc b/src/reel_subtitle_asset.cc
index f7df36f8..00c694a1 100644
--- a/src/reel_subtitle_asset.cc
+++ b/src/reel_subtitle_asset.cc
@@ -58,10 +58,7 @@ ReelSubtitleAsset::ReelSubtitleAsset (boost::shared_ptr<const cxml::Node> node)
 	: ReelAsset (node)
 	, ReelMXF (node)
 {
-	optional<string> const language = node->optional_string_child("Language");
-	if (language) {
-		_language = dcp::LanguageTag(*language);
-	}
+	_language = node->optional_string_child("Language");
 	node->done ();
 }
 
@@ -83,7 +80,7 @@ ReelSubtitleAsset::write_to_cpl (xmlpp::Node* node, Standard standard) const
 	xmlpp::Node* asset = write_to_cpl_asset (node, standard, hash());
 	write_to_cpl_mxf (asset);
 	if (_language) {
-		asset->add_child("Language")->add_child_text(_language->to_string());
+		asset->add_child("Language")->add_child_text(*_language);
 	}
 	return asset;
 }
@@ -100,3 +97,11 @@ ReelSubtitleAsset::equals (shared_ptr<const ReelSubtitleAsset> other, EqualityOp
 
 	return true;
 }
+
+
+void
+ReelSubtitleAsset::set_language (dcp::LanguageTag language)
+{
+	_language = language.to_string();
+}
+
diff --git a/src/reel_subtitle_asset.h b/src/reel_subtitle_asset.h
index 123478cf..a0cc405c 100644
--- a/src/reel_subtitle_asset.h
+++ b/src/reel_subtitle_asset.h
@@ -65,7 +65,7 @@ public:
 
 	void set_language (dcp::LanguageTag language);
 
-	boost::optional<dcp::LanguageTag> language () const {
+	boost::optional<std::string> language () const {
 		return _language;
 	}
 
@@ -73,7 +73,11 @@ private:
 	std::string key_type () const;
 	std::string cpl_node_name (Standard standard) const;
 
-	boost::optional<dcp::LanguageTag> _language;
+	/** As in other places, this is stored and returned as a string so that
+	 *  we can tolerate non-RFC-5646 strings, but must be set as a dcp::LanguageTag
+	 *  to try to ensure that we create compliant output.
+	 */
+	boost::optional<std::string> _language;
 };
 
 }
diff --git a/src/smpte_subtitle_asset.h b/src/smpte_subtitle_asset.h
index 79a0024b..09cd4634 100644
--- a/src/smpte_subtitle_asset.h
+++ b/src/smpte_subtitle_asset.h
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2012-2018 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2012-2020 Carl Hetherington <cth@carlh.net>
 
     This file is part of libdcp.
 
@@ -36,6 +36,7 @@
  */
 
 #include "subtitle_asset.h"
+#include "language_tag.h"
 #include "local_time.h"
 #include "mxf.h"
 #include "crypto_context.h"
@@ -81,8 +82,8 @@ public:
 		_content_title_text = t;
 	}
 
-	void set_language (std::string l) {
-		_language = l;
+	void set_language (dcp::LanguageTag l) {
+		_language = l.to_string();
 	}
 
 	void set_issue_date (LocalTime t) {
@@ -116,7 +117,9 @@ public:
 		return _content_title_text;
 	}
 
-	/** @return language as a xs:language, if one was specified */
+	/** @return Language, if one was set.  This should be a xs:language, but
+	 *  it might not be if a non-compliant DCP was read in.
+	 */
 	boost::optional<std::string> language () const {
 		return _language;
 	}
@@ -180,6 +183,9 @@ private:
 	int64_t _intrinsic_duration;
 	/** <ContentTitleText> from the asset */
 	std::string _content_title_text;
+	/** This is stored and returned as a string so that we can tolerate non-RFC-5646 strings,
+	 *  but must be set as a dcp::LanguageTag to try to ensure that we create compliant output.
+	 */
 	boost::optional<std::string> _language;
 	boost::optional<std::string> _annotation_text;
 	LocalTime _issue_date;
diff --git a/test/cpl_metadata_test.cc b/test/cpl_metadata_test.cc
index 5082b8f6..a97daaa9 100644
--- a/test/cpl_metadata_test.cc
+++ b/test/cpl_metadata_test.cc
@@ -233,7 +233,8 @@ BOOST_AUTO_TEST_CASE (cpl_metadata_read_test1)
 
 	list<shared_ptr<dcp::Reel> > reels = cpl.reels ();
 	BOOST_REQUIRE_EQUAL (reels.size(), 1);
-	BOOST_CHECK_EQUAL (reels.front()->main_subtitle()->language().get(), dcp::LanguageTag("de-DE"));
+	BOOST_REQUIRE (reels.front()->main_subtitle()->language());
+	BOOST_CHECK_EQUAL (reels.front()->main_subtitle()->language().get(), "de-DE");
 
 	vector<string> asl = cpl.additional_subtitle_languages();
 	BOOST_REQUIRE_EQUAL (asl.size(), 2);
diff --git a/test/reel_asset_test.cc b/test/reel_asset_test.cc
index 45736a7a..42d76525 100644
--- a/test/reel_asset_test.cc
+++ b/test/reel_asset_test.cc
@@ -103,5 +103,5 @@ BOOST_AUTO_TEST_CASE (reel_subtitle_asset_test)
 	BOOST_CHECK_EQUAL (ps.duration().get(), 525L);
 	BOOST_CHECK_EQUAL (ps.hash().get(), string("3EABjX9BB1CAWhLUtHhrGSyLgOY="));
 	BOOST_REQUIRE (ps.language());
-	BOOST_CHECK_EQUAL (ps.language()->to_string(), "de-DE");
+	BOOST_CHECK_EQUAL (ps.language().get(), "de-DE");
 }
diff --git a/test/write_subtitle_test.cc b/test/write_subtitle_test.cc
index 1f5ded21..c3217dbc 100644
--- a/test/write_subtitle_test.cc
+++ b/test/write_subtitle_test.cc
@@ -407,7 +407,7 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test)
 {
 	dcp::SMPTESubtitleAsset c;
 	c.set_reel_number (1);
-	c.set_language ("EN");
+	c.set_language (dcp::LanguageTag("en"));
 	c.set_content_title_text ("Test");
 	c.set_issue_date (dcp::LocalTime ("2016-04-01T03:52:00+00:00"));
 
@@ -472,7 +472,7 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test)
 		  "<dcst:ContentTitleText>Test</dcst:ContentTitleText>"
 		  "<dcst:IssueDate>2016-04-01T03:52:00.000+00:00</dcst:IssueDate>"
 		  "<dcst:ReelNumber>1</dcst:ReelNumber>"
-		  "<dcst:Language>EN</dcst:Language>"
+		  "<dcst:Language>en</dcst:Language>"
 		  "<dcst:EditRate>24 1</dcst:EditRate>"
 		  "<dcst:TimeCodeRate>24</dcst:TimeCodeRate>"
 		  "<dcst:SubtitleList>"
@@ -500,7 +500,7 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test2)
 {
 	dcp::SMPTESubtitleAsset c;
 	c.set_reel_number (1);
-	c.set_language ("EN");
+	c.set_language (dcp::LanguageTag("en"));
 	c.set_content_title_text ("Test");
 	c.set_issue_date (dcp::LocalTime ("2016-04-01T03:52:00+00:00"));
 
@@ -670,7 +670,7 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test2)
 		  "<dcst:ContentTitleText>Test</dcst:ContentTitleText>"
 		  "<dcst:IssueDate>2016-04-01T03:52:00.000+00:00</dcst:IssueDate>"
 		  "<dcst:ReelNumber>1</dcst:ReelNumber>"
-		  "<dcst:Language>EN</dcst:Language>"
+		  "<dcst:Language>en</dcst:Language>"
 		  "<dcst:EditRate>24 1</dcst:EditRate>"
 		  "<dcst:TimeCodeRate>24</dcst:TimeCodeRate>"
 		  "<dcst:SubtitleList>"
@@ -699,7 +699,7 @@ BOOST_AUTO_TEST_CASE (write_smpte_subtitle_test3)
 {
 	dcp::SMPTESubtitleAsset c;
 	c.set_reel_number (1);
-	c.set_language ("EN");
+	c.set_language (dcp::LanguageTag("en"));
 	c.set_content_title_text ("Test");
 
 	c.add (