Fix handling of empty font IDs and default DCP fonts (#2721) (part of #2722).

Previously we used an empty font ID as the default for when a subtitle
has no Font, but in #2721 we saw a DCP with an empty font ID which
raised an assertion (because we'd already added our default font with
the empty ID).

Here we try to fix this (and also make the default font correctly be
that from the first <LoadFont>).

13 files changed, 128 insertions, 27 deletions

diff --git a/src/lib/dcp_decoder.cc b/src/lib/dcp_decoder.cc
index 3a1871863..24ff507e6 100644
--- a/src/lib/dcp_decoder.cc
+++ b/src/lib/dcp_decoder.cc
@@ -314,7 +314,11 @@ DCPDecoder::pass_texts (
 				}
 
 				dcp::SubtitleString is_copy = *is;
-				is_copy.set_font(_font_id_allocator.font_id(_reel - _reels.begin(), asset->id(), is_copy.font().get_value_or("")));
+				if (is_copy.font()) {
+					is_copy.set_font(_font_id_allocator.font_id(_reel - _reels.begin(), asset->id(), is_copy.font().get()));
+				} else {
+					is_copy.set_font(_font_id_allocator.default_font_id());
+				}
 				strings.push_back(is_copy);
 			}
 
diff --git a/src/lib/dcp_examiner.cc b/src/lib/dcp_examiner.cc
index 400c7d26b..d9c904720 100644
--- a/src/lib/dcp_examiner.cc
+++ b/src/lib/dcp_examiner.cc
@@ -25,6 +25,7 @@
 #include "dcp_examiner.h"
 #include "dcpomatic_log.h"
 #include "exceptions.h"
+#include "font_id_allocator.h"
 #include "image.h"
 #include "text_content.h"
 #include "util.h"
@@ -210,7 +211,6 @@ DCPExaminer::DCPExaminer (shared_ptr<const DCPContent> content, bool tolerant)
 				for (auto const& font: asset->font_data()) {
 					_fonts.push_back({reel_index, asset->id(), make_shared<dcpomatic::Font>(font.first, font.second)});
 				}
-				_fonts.push_back({reel_index, asset->id(), make_shared<dcpomatic::Font>("")});
 			}
 		}
 
@@ -367,16 +367,22 @@ DCPExaminer::DCPExaminer (shared_ptr<const DCPContent> content, bool tolerant)
 void
 DCPExaminer::add_fonts(shared_ptr<TextContent> content)
 {
+	FontIDAllocator font_id_allocator;
+
 	for (auto const& font: _fonts) {
-		_font_id_allocator.add_font(font.reel_index, font.asset_id, font.font->id());
+		font_id_allocator.add_font(font.reel_index, font.asset_id, font.font->id());
 	}
 
-	_font_id_allocator.allocate();
+	font_id_allocator.allocate();
 
 	for (auto const& font: _fonts) {
 		auto font_copy = make_shared<dcpomatic::Font>(*font.font);
-		font_copy->set_id(_font_id_allocator.font_id(font.reel_index, font.asset_id, font.font->id()));
+		font_copy->set_id(font_id_allocator.font_id(font.reel_index, font.asset_id, font.font->id()));
 		content->add_font(font_copy);
 	}
+
+	if (!font_id_allocator.has_default_font()) {
+		content->add_font(make_shared<dcpomatic::Font>(font_id_allocator.default_font_id(), default_font_file()));
+	}
 }
 
diff --git a/src/lib/dcp_examiner.h b/src/lib/dcp_examiner.h
index 54e283548..444fb7567 100644
--- a/src/lib/dcp_examiner.h
+++ b/src/lib/dcp_examiner.h
@@ -27,7 +27,6 @@
 #include "audio_examiner.h"
 #include "dcp_text_track.h"
 #include "dcpomatic_assert.h"
-#include "font_id_allocator.h"
 #include "video_examiner.h"
 #include <dcp/dcp_time.h>
 #include <dcp/rating.h>
@@ -224,5 +223,4 @@ private:
 	};
 
 	std::vector<Font> _fonts;
-	FontIDAllocator _font_id_allocator;
 };
diff --git a/src/lib/dcp_subtitle_content.cc b/src/lib/dcp_subtitle_content.cc
index b3e24d5e2..8de5967ef 100644
--- a/src/lib/dcp_subtitle_content.cc
+++ b/src/lib/dcp_subtitle_content.cc
@@ -21,6 +21,7 @@
 #include "font.h"
 #include "dcp_subtitle_content.h"
 #include "film.h"
+#include "font_id_allocator.h"
 #include "text_content.h"
 #include <dcp/raw_convert.h>
 #include <dcp/interop_subtitle_asset.h>
@@ -73,20 +74,43 @@ DCPSubtitleContent::examine (shared_ptr<const Film> film, shared_ptr<Job> job)
 	_length = ContentTime::from_seconds(subtitle_asset->latest_subtitle_out().as_seconds());
 
 	subtitle_asset->fix_empty_font_ids();
+	add_fonts(only_text(), subtitle_asset);
+}
+
+
+void
+DCPSubtitleContent::add_fonts(shared_ptr<TextContent> content, shared_ptr<dcp::SubtitleAsset> subtitle_asset)
+{
+	FontIDAllocator font_id_allocator;
+
+	for (auto node: subtitle_asset->load_font_nodes()) {
+		font_id_allocator.add_font(0, subtitle_asset->id(), node->id);
+	}
+	font_id_allocator.allocate();
 
 	auto font_data = subtitle_asset->font_data();
 	for (auto node: subtitle_asset->load_font_nodes()) {
 		auto data = font_data.find(node->id);
+		shared_ptr<dcpomatic::Font> font;
 		if (data != font_data.end()) {
-			only_text()->add_font(make_shared<Font>(node->id, data->second));
+			font = make_shared<Font>(
+				font_id_allocator.font_id(0, subtitle_asset->id(), node->id),
+				data->second
+				);
 		} else {
-			only_text()->add_font(make_shared<Font>(node->id));
+			font = make_shared<Font>(
+				font_id_allocator.font_id(0, subtitle_asset->id(), node->id)
+				);
 		}
+		content->add_font(font);
 	}
 
-	only_text()->add_font(make_shared<Font>(""));
+	if (!font_id_allocator.has_default_font()) {
+		content->add_font(make_shared<dcpomatic::Font>(font_id_allocator.default_font_id(), default_font_file()));
+	}
 }
 
+
 DCPTime
 DCPSubtitleContent::full_length (shared_ptr<const Film> film) const
 {
diff --git a/src/lib/dcp_subtitle_content.h b/src/lib/dcp_subtitle_content.h
index 5949f8b0b..89a6f26a2 100644
--- a/src/lib/dcp_subtitle_content.h
+++ b/src/lib/dcp_subtitle_content.h
@@ -35,5 +35,7 @@ public:
 	dcpomatic::DCPTime approximate_length () const override;
 
 private:
+	void add_fonts(std::shared_ptr<TextContent> content, std::shared_ptr<dcp::SubtitleAsset> subtitle_asset);
+
 	dcpomatic::ContentTime _length;
 };
diff --git a/src/lib/dcp_subtitle_decoder.cc b/src/lib/dcp_subtitle_decoder.cc
index b3e6d7553..711dc77f2 100644
--- a/src/lib/dcp_subtitle_decoder.cc
+++ b/src/lib/dcp_subtitle_decoder.cc
@@ -43,15 +43,22 @@ DCPSubtitleDecoder::DCPSubtitleDecoder (shared_ptr<const Film> film, shared_ptr<
 	: Decoder (film)
 {
 	/* Load the XML or MXF file */
-	auto const asset = load (content->path(0));
-	asset->fix_empty_font_ids ();
-	_subtitles = asset->subtitles ();
+	_asset = load(content->path(0));
+	_asset->fix_empty_font_ids();
+	_subtitles = _asset->subtitles();
 	_next = _subtitles.begin ();
 
-	_subtitle_standard = asset->subtitle_standard();
+	_subtitle_standard = _asset->subtitle_standard();
 
 	text.push_back (make_shared<TextDecoder>(this, content->only_text()));
 	update_position();
+
+	FontIDAllocator font_id_allocator;
+
+	for (auto node: _asset->load_font_nodes()) {
+		_font_id_allocator.add_font(0, _asset->id(), node->id);
+	}
+	_font_id_allocator.allocate();
 }
 
 
@@ -91,7 +98,13 @@ DCPSubtitleDecoder::pass ()
 	while (_next != _subtitles.end () && content_time_period (*_next) == p) {
 		auto ns = dynamic_pointer_cast<const dcp::SubtitleString>(*_next);
 		if (ns) {
-			s.push_back (*ns);
+			dcp::SubtitleString ns_copy = *ns;
+			if (ns_copy.font()) {
+				ns_copy.set_font(_font_id_allocator.font_id(0, _asset->id(), ns_copy.font().get()));
+			} else {
+				ns_copy.set_font(_font_id_allocator.default_font_id());
+			}
+			s.push_back(ns_copy);
 			++_next;
 		} else {
 			/* XXX: perhaps these image subs should also be collected together like the string ones are;
diff --git a/src/lib/dcp_subtitle_decoder.h b/src/lib/dcp_subtitle_decoder.h
index 45a4999dd..9d0851253 100644
--- a/src/lib/dcp_subtitle_decoder.h
+++ b/src/lib/dcp_subtitle_decoder.h
@@ -19,8 +19,9 @@
 */
 
 
-#include "text_decoder.h"
 #include "dcp_subtitle.h"
+#include "font_id_allocator.h"
+#include "text_decoder.h"
 
 
 class DCPSubtitleContent;
@@ -44,4 +45,7 @@ private:
 	std::vector<std::shared_ptr<const dcp::Subtitle>>::const_iterator _next;
 
 	dcp::SubtitleStandard _subtitle_standard;
+
+	std::shared_ptr<dcp::SubtitleAsset> _asset;
+	FontIDAllocator _font_id_allocator;
 };
diff --git a/src/lib/font_id_allocator.cc b/src/lib/font_id_allocator.cc
index c566c3676..5263e7f90 100644
--- a/src/lib/font_id_allocator.cc
+++ b/src/lib/font_id_allocator.cc
@@ -64,17 +64,19 @@ void
 FontIDAllocator::add_fonts_from_asset(int reel_index, shared_ptr<const dcp::SubtitleAsset> asset)
 {
 	for (auto const& font: asset->font_data()) {
-		_map[Font(reel_index, asset->id(), font.first)] = 0;
+		add_font(reel_index, asset->id(), font.first);
 	}
-
-	_map[Font(reel_index, asset->id(), "")] = 0;
 }
 
 
 void
 FontIDAllocator::add_font(int reel_index, string asset_id, string font_id)
 {
-	_map[Font(reel_index, asset_id, font_id)] = 0;
+	auto font = Font(reel_index, asset_id, font_id);
+	if (!_default_font) {
+		_default_font = font;
+	}
+	_map[font] = 0;
 }
 
 
@@ -119,3 +121,13 @@ FontIDAllocator::font_id(int reel_index, string asset_id, string font_id) const
 	return String::compose("%1_%2", iter->second, font_id);
 }
 
+
+string
+FontIDAllocator::default_font_id() const
+{
+	if (_default_font) {
+		return font_id(_default_font->reel_index, _default_font->asset_id, _default_font->font_id);
+	}
+
+	return "default";
+}
diff --git a/src/lib/font_id_allocator.h b/src/lib/font_id_allocator.h
index bd99cad63..fe4b9ef07 100644
--- a/src/lib/font_id_allocator.h
+++ b/src/lib/font_id_allocator.h
@@ -27,6 +27,7 @@
 #include <memory>
 #include <string>
 #include <vector>
+#include <boost/optional.hpp>
 
 
 namespace dcp {
@@ -66,6 +67,11 @@ public:
 	void allocate();
 
 	std::string font_id(int reel_index, std::string asset_id, std::string font_id) const;
+	std::string default_font_id() const;
+
+	bool has_default_font() const {
+		return static_cast<bool>(_default_font);
+	}
 
 private:
 	void add_fonts_from_asset(int reel_index, std::shared_ptr<const dcp::SubtitleAsset> asset);
@@ -96,6 +102,7 @@ private:
 	};
 
 	std::map<Font, int> _map;
+	boost::optional<Font> _default_font;
 };
 
 
diff --git a/test/data b/test/data
-Subproject a4ad4c1a4880d02aabf2790e11c4e5c2c28034d
+Subproject 314e09cbac2e023a7acb61e1b32db76fe6dd775
diff --git a/test/dcp_subtitle_test.cc b/test/dcp_subtitle_test.cc
index 9b7b77279..4928d92c1 100644
--- a/test/dcp_subtitle_test.cc
+++ b/test/dcp_subtitle_test.cc
@@ -246,7 +246,9 @@ BOOST_AUTO_TEST_CASE (test_font_override)
 	film->set_interop(true);
 
 	BOOST_REQUIRE_EQUAL(content->text.size(), 1U);
-	content->text.front()->get_font("theFontId")->set_file("test/data/Inconsolata-VF.ttf");
+	auto font = content->text.front()->get_font("0_theFontId");
+	BOOST_REQUIRE(font);
+	font->set_file("test/data/Inconsolata-VF.ttf");
 
 	make_and_verify_dcp (film, { dcp::VerificationNote::Code::INVALID_STANDARD });
 	check_file (subtitle_file(film).parent_path() / "font_0.ttf", "test/data/Inconsolata-VF.ttf");
diff --git a/test/hints_test.cc b/test/hints_test.cc
index a989d3aeb..949ba18c0 100644
--- a/test/hints_test.cc
+++ b/test/hints_test.cc
@@ -198,7 +198,8 @@ BOOST_AUTO_TEST_CASE (hint_subtitle_mxf_too_big)
 		content->text[0]->set_language(dcp::LanguageTag("en-US"));
 		film->examine_and_add_content(content);
 		BOOST_REQUIRE (!wait_for_jobs());
-		auto const font = content->text[0]->get_font(String::compose("font_%1", i));
+		auto const font = content->text[0]->get_font(String::compose("0_font_%1", i));
+		BOOST_REQUIRE(font);
 		font->set_file("build/test/hint_subtitle_mxf_too_big.ttf");
 	}
 
diff --git a/test/subtitle_font_id_test.cc b/test/subtitle_font_id_test.cc
index f6bd48c51..56207bfcb 100644
--- a/test/subtitle_font_id_test.cc
+++ b/test/subtitle_font_id_test.cc
@@ -47,8 +47,7 @@ BOOST_AUTO_TEST_CASE(full_dcp_subtitle_font_id_test)
 	auto text = content[0]->only_text();
 	BOOST_REQUIRE(text);
 
-	/* There's the font from the DCP and also a dummy one with an empty ID */
-	BOOST_REQUIRE_EQUAL(text->fonts().size(), 2U);
+	BOOST_REQUIRE_EQUAL(text->fonts().size(), 1U);
 	auto font = text->fonts().front();
 	BOOST_CHECK_EQUAL(font->id(), "0_theFontId");
 	BOOST_REQUIRE(font->data());
@@ -66,10 +65,9 @@ BOOST_AUTO_TEST_CASE(dcp_subtitle_font_id_test)
 	auto text = content[0]->only_text();
 	BOOST_REQUIRE(text);
 
-	/* There's the font from the DCP and also a dummy one with an empty ID */
-	BOOST_REQUIRE_EQUAL(text->fonts().size(), 2U);
+	BOOST_REQUIRE_EQUAL(text->fonts().size(), 1U);
 	auto font = text->fonts().front();
-	BOOST_CHECK_EQUAL(font->id(), "theFontId");
+	BOOST_CHECK_EQUAL(font->id(), "0_theFontId");
 	BOOST_REQUIRE(font->data());
 	BOOST_CHECK_EQUAL(font->data()->size(), 367112);
 }
@@ -261,3 +259,33 @@ BOOST_AUTO_TEST_CASE(subtitle_with_no_font_test)
 	make_and_verify_dcp(check_film);
 }
 
+
+BOOST_AUTO_TEST_CASE(load_dcp_with_empty_font_id_test)
+{
+	auto dcp = std::make_shared<DCPContent>(TestPaths::private_data() / "kr_vf");
+	auto film = new_test_film2("load_dcp_with_empty_font_id_test", { dcp });
+}
+
+
+BOOST_AUTO_TEST_CASE(use_first_loadfont_as_default)
+{
+	auto dcp = std::make_shared<DCPContent>("test/data/use_default_font");
+	auto film = new_test_film2("use_first_loadfont_as_default", { dcp });
+	dcp->only_text()->set_use(true);
+	dcp->only_text()->set_language(dcp::LanguageTag("de"));
+	make_and_verify_dcp(
+		film,
+		{ dcp::VerificationNote::Code::INVALID_SUBTITLE_FIRST_TEXT_TIME }
+		);
+
+	dcp::DCP test(film->dir(film->dcp_name()));
+	test.read();
+	BOOST_REQUIRE(!test.cpls().empty());
+	auto cpl = test.cpls()[0];
+	BOOST_REQUIRE(!cpl->reels().empty());
+	auto reel = cpl->reels()[0];
+	BOOST_REQUIRE(reel->main_subtitle()->asset());
+	auto subtitle = std::dynamic_pointer_cast<dcp::SMPTESubtitleAsset>(reel->main_subtitle()->asset());
+	BOOST_REQUIRE_EQUAL(subtitle->font_data().size(), 1U);
+	BOOST_CHECK(subtitle->font_data().begin()->second == dcp::ArrayData("test/data/Inconsolata-VF.ttf"));
+}