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

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

diff --git a/src/lib/font_id_allocator.cc b/src/lib/font_id_allocator.cc
index 5263e7f9087525c02e8485f26f5a8330d98a7784..112dd262bacfd98652c6ad0620808b4b17cf986f 100644 (file)
--- 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 7c93c6b782ff542e79794a2d011208a1bb283994..07110346b04441f2b7a7af166c04fd9d1e2fc502 100644 (file)
--- 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());
+}