Fix font ID allocation from DCP when there are subs and ccaps using the same IDs (#2822).

Previously we would always use <last-reel+1>_id as a disambiguated ID
for every case.

2 files changed, 39 insertions, 11 deletions

diff --git a/src/lib/font_id_allocator.cc b/src/lib/font_id_allocator.cc
index 5263e7f90..112dd262b 100644
--- a/src/lib/font_id_allocator.cc
+++ b/src/lib/font_id_allocator.cc
@@ -83,32 +83,31 @@ FontIDAllocator::add_font(int reel_index, string asset_id, string font_id)
 void
 FontIDAllocator::allocate()
 {
-	/* Last reel index that we have; i.e. the last prefix number that would be used by "old"
-	 * font IDs (i.e. ones before this FontIDAllocator was added and used)
+	/* 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 const last_reel = std::max_element(
+	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;
+		})->first.reel_index + 1;
 
-	/* Number of times each ID has been used */
-	std::map<string, int> used_count;
+	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_count.find(proposed) != used_count.end()) {
+		if (used_ids.find(proposed) != used_ids.end()) {
 			/* This ID was already used; we need to disambiguate it.  Do so by using
-			 * an ID above last_reel
+			 * one of our unused prefixes.
 			 */
-			font.second = last_reel + used_count[proposed];
-			++used_count[proposed];
+			font.second = next_unused++;
 		} else {
 			/* This ID was not yet used */
-			used_count[proposed] = 1;
 			font.second = font.first.reel_index;
 		}
+		used_ids.insert(proposed);
 	}
 }
 
diff --git a/test/font_id_allocator_test.cc b/test/font_id_allocator_test.cc
index 7c93c6b78..07110346b 100644
--- a/test/font_id_allocator_test.cc
+++ b/test/font_id_allocator_test.cc
@@ -23,6 +23,10 @@
 #include <boost/test/unit_test.hpp>
 
 
+using std::string;
+using std::vector;
+
+
 BOOST_AUTO_TEST_CASE(font_id_allocator_test_without_disambiguation)
 {
 	FontIDAllocator allocator;
@@ -70,3 +74,28 @@ BOOST_AUTO_TEST_CASE(font_id_allocator_test_with_disambiguation)
 	BOOST_CHECK(allocator.font_id(1, "asset3", "font1") == "1_font1");
 }
 
+
+/* Bug #2822: multiple reels, each with subs + closed captions, and each using the same
+ * basic font ID.
+ */
+BOOST_AUTO_TEST_CASE(font_id_allocator_test_with_disambiguation2)
+{
+	FontIDAllocator allocator;
+
+	allocator.add_font(0, "asset1", "font");
+	allocator.add_font(0, "asset2", "font");
+
+	allocator.add_font(1, "asset1", "font");
+	allocator.add_font(1, "asset2", "font");
+
+	allocator.allocate();
+	vector<string> ids = {
+		allocator.font_id(0, "asset1", "font"),
+		allocator.font_id(0, "asset2", "font"),
+		allocator.font_id(1, "asset1", "font"),
+		allocator.font_id(1, "asset2", "font")
+	};
+
+	std::sort(ids.begin(), ids.end());
+	BOOST_CHECK(std::adjacent_find(ids.begin(), ids.end()) == ids.end());
+}