Simplify the FontIDAllocator a lot (#2827).

This is at the expense of forward compatibility, and the need to
re-examine subtitle content (losing custom fonts as we do so).

But it does mean that the code is simpler, and there's not this weird
growth of IDs where a DCP gets imported with some font, and then the ID
becomes 0_font, and if you do it again it's 0_0_font, and so on.

9 files changed, 27 insertions, 41 deletions

diff --git a/src/lib/check_content_job.cc b/src/lib/check_content_job.cc
index f37890abf..2028b01ac 100644
--- a/src/lib/check_content_job.cc
+++ b/src/lib/check_content_job.cc
@@ -70,7 +70,7 @@ CheckContentJob::run ()
 	std::vector<shared_ptr<Content>> changed;
 	std::copy_if (content.begin(), content.end(), std::back_inserter(changed), [](shared_ptr<Content> c) { return c->changed(); });
 
-	if (_film->last_written_by_earlier_than(2, 16, 15)) {
+	if (_film->last_written_by_earlier_than(2, 17, 17)) {
 		for (auto c: content) {
 			if (auto stf = dynamic_pointer_cast<StringTextFileContent>(c)) {
 				stf->check_font_ids();
diff --git a/src/lib/dcp_content.cc b/src/lib/dcp_content.cc
index 0aeb1d041..b4e979481 100644
--- a/src/lib/dcp_content.cc
+++ b/src/lib/dcp_content.cc
@@ -851,9 +851,10 @@ DCPContent::check_font_ids()
 		return;
 	}
 
-	/* This might be called on a TextContent that already has the correct fonts
-	 * (e.g. if run from a build with a LastWrittenBy containing only a git hash)
-	 * so we'll get an error if we don't clear them out first.
+	/* This might be called on a TextContent that already has some fonts
+	 * (e.g. if run from a build with a LastWrittenBy containing only a git
+	 * hash, or from a version between 2.16.15 and 2.17.17) so we'll get an
+	 * error if we don't clear them out first.
 	 */
 	text[0]->clear_fonts();
 	DCPExaminer examiner(shared_from_this(), true);
diff --git a/src/lib/font_id_allocator.cc b/src/lib/font_id_allocator.cc
index 112dd262b..76b52e730 100644
--- a/src/lib/font_id_allocator.cc
+++ b/src/lib/font_id_allocator.cc
@@ -76,37 +76,23 @@ FontIDAllocator::add_font(int reel_index, string asset_id, string font_id)
 	if (!_default_font) {
 		_default_font = font;
 	}
-	_map[font] = 0;
+	_map[font] = {};
 }
 
 
 void
 FontIDAllocator::allocate()
 {
-	/* We'll first try adding <reel>_ to the start of the font ID, but if a reel has multiple
-	 * identical font IDs we will need to use some number that is not a reel ID.  Find the
-	 * first such number (1 higher than the highest reel index)
-	 */
-	auto next_unused = std::max_element(
-		_map.begin(),
-		_map.end(),
-		[] (std::pair<Font, int> const& a, std::pair<Font, int> const& b) {
-			return a.first.reel_index < b.first.reel_index;
-		})->first.reel_index + 1;
-
 	std::set<string> used_ids;
 
 	for (auto& font: _map) {
-		auto const proposed = String::compose("%1_%2", font.first.reel_index, font.first.font_id);
-		if (used_ids.find(proposed) != used_ids.end()) {
-			/* This ID was already used; we need to disambiguate it.  Do so by using
-			 * one of our unused prefixes.
-			 */
-			font.second = next_unused++;
-		} else {
-			/* This ID was not yet used */
-			font.second = font.first.reel_index;
+		auto proposed = font.first.font_id;
+		int prefix = 0;
+		while (used_ids.find(proposed) != used_ids.end()) {
+			proposed = String::compose("%1_%2", prefix++, font.first.font_id);
+			DCPOMATIC_ASSERT(prefix < 128);
 		}
+		font.second = proposed;
 		used_ids.insert(proposed);
 	}
 }
@@ -117,7 +103,7 @@ FontIDAllocator::font_id(int reel_index, string asset_id, string font_id) const
 {
 	auto iter = _map.find(Font(reel_index, asset_id, font_id));
 	DCPOMATIC_ASSERT(iter != _map.end());
-	return String::compose("%1_%2", iter->second, font_id);
+	return iter->second;
 }
 
 
diff --git a/src/lib/font_id_allocator.h b/src/lib/font_id_allocator.h
index fe4b9ef07..6737907c1 100644
--- a/src/lib/font_id_allocator.h
+++ b/src/lib/font_id_allocator.h
@@ -101,7 +101,7 @@ private:
 		std::string font_id;
 	};
 
-	std::map<Font, int> _map;
+	std::map<Font, std::string> _map;
 	boost::optional<Font> _default_font;
 };
 
diff --git a/test/data b/test/data
-Subproject 722dd8023aa1f4657328aa228ea146679cf7fab
+Subproject 7a5254c53354ea8aaa5c60ae44965e510dd9651
diff --git a/test/dcp_subtitle_test.cc b/test/dcp_subtitle_test.cc
index f7a9450f5..22e4fd9ff 100644
--- a/test/dcp_subtitle_test.cc
+++ b/test/dcp_subtitle_test.cc
@@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE (test_font_override)
 	content->only_text()->set_language(dcp::LanguageTag("de"));
 
 	BOOST_REQUIRE_EQUAL(content->text.size(), 1U);
-	auto font = content->text.front()->get_font("0_theFontId");
+	auto font = content->text.front()->get_font("theFontId");
 	BOOST_REQUIRE(font);
 	font->set_file("test/data/Inconsolata-VF.ttf");
 
diff --git a/test/font_id_allocator_test.cc b/test/font_id_allocator_test.cc
index 07110346b..19c4a2154 100644
--- a/test/font_id_allocator_test.cc
+++ b/test/font_id_allocator_test.cc
@@ -43,12 +43,12 @@ BOOST_AUTO_TEST_CASE(font_id_allocator_test_without_disambiguation)
 
 	allocator.allocate();
 
-	BOOST_CHECK(allocator.font_id(0, "asset1", "font") == "0_font");
-	BOOST_CHECK(allocator.font_id(0, "asset1", "font2") == "0_font2");
-	BOOST_CHECK(allocator.font_id(1, "asset2", "font") == "1_font");
-	BOOST_CHECK(allocator.font_id(1, "asset2", "font2") == "1_font2");
-	BOOST_CHECK(allocator.font_id(1, "asset3", "font3") == "1_font3");
-	BOOST_CHECK(allocator.font_id(1, "asset3", "font4") == "1_font4");
+	BOOST_CHECK_EQUAL(allocator.font_id(0, "asset1", "font"), "font");
+	BOOST_CHECK_EQUAL(allocator.font_id(0, "asset1", "font2"), "font2");
+	BOOST_CHECK_EQUAL(allocator.font_id(1, "asset2", "font"), "0_font");
+	BOOST_CHECK_EQUAL(allocator.font_id(1, "asset2", "font2"), "0_font2");
+	BOOST_CHECK_EQUAL(allocator.font_id(1, "asset3", "font3"), "font3");
+	BOOST_CHECK_EQUAL(allocator.font_id(1, "asset3", "font4"), "font4");
 }
 
 
@@ -68,10 +68,9 @@ BOOST_AUTO_TEST_CASE(font_id_allocator_test_with_disambiguation)
 
 	allocator.allocate();
 
-	BOOST_CHECK(allocator.font_id(0, "asset1", "font") == "0_font");
-	/* This should get a prefix that is higher than any reel index */
-	BOOST_CHECK(allocator.font_id(0, "asset2", "font") == "2_font");
-	BOOST_CHECK(allocator.font_id(1, "asset3", "font1") == "1_font1");
+	BOOST_CHECK(allocator.font_id(0, "asset1", "font") == "font");
+	BOOST_CHECK(allocator.font_id(0, "asset2", "font") == "0_font");
+	BOOST_CHECK(allocator.font_id(1, "asset3", "font1") == "font1");
 }
 
 
diff --git a/test/hints_test.cc b/test/hints_test.cc
index 8768815ad..56a0964c4 100644
--- a/test/hints_test.cc
+++ b/test/hints_test.cc
@@ -198,7 +198,7 @@ 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("0_font_%1", i));
+		auto const font = content->text[0]->get_font(String::compose("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 a93145163..49ad4ea05 100644
--- a/test/subtitle_font_id_test.cc
+++ b/test/subtitle_font_id_test.cc
@@ -51,7 +51,7 @@ BOOST_AUTO_TEST_CASE(full_dcp_subtitle_font_id_test)
 
 	BOOST_REQUIRE_EQUAL(text->fonts().size(), 1U);
 	auto font = text->fonts().front();
-	BOOST_CHECK_EQUAL(font->id(), "0_theFontId");
+	BOOST_CHECK_EQUAL(font->id(), "theFontId");
 	BOOST_REQUIRE(font->data());
 	BOOST_CHECK_EQUAL(font->data()->size(), 367112);
 }
@@ -69,7 +69,7 @@ BOOST_AUTO_TEST_CASE(dcp_subtitle_font_id_test)
 
 	BOOST_REQUIRE_EQUAL(text->fonts().size(), 1U);
 	auto font = text->fonts().front();
-	BOOST_CHECK_EQUAL(font->id(), "0_theFontId");
+	BOOST_CHECK_EQUAL(font->id(), "theFontId");
 	BOOST_REQUIRE(font->data());
 	BOOST_CHECK_EQUAL(font->data()->size(), 367112);
 }