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.

diff --git a/src/lib/check_content_job.cc b/src/lib/check_content_job.cc
index f37890abfe39c783af4635012276fdb5be2c5ccb..2028b01ac0f2672b1367967539361a8636b9e877 100644 (file)
--- 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 0aeb1d04146276615e9faffe66f176bb372ade73..b4e9794813a0dbe3f50c52778d54b851582175a0 100644 (file)
--- 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 112dd262bacfd98652c6ad0620808b4b17cf986f..76b52e730c15afafd7d5ffee3ce277b4afa81217 100644 (file)
--- 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 fe4b9ef077e7b3d4bfab6c296a48d0644f5cfd39..6737907c118c966bb1f1750986e26efac342de7f 100644 (file)
--- 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
index 722dd8023aa1f4657328aa228ea146679cf7fab0..7a5254c53354ea8aaa5c60ae44965e510dd96511 160000 (submodule)
--- a/test/data
+++ b/test/data
@@ -1 +1 @@
-Subproject commit 722dd8023aa1f4657328aa228ea146679cf7fab0
+Subproject commit 7a5254c53354ea8aaa5c60ae44965e510dd96511

diff --git a/test/dcp_subtitle_test.cc b/test/dcp_subtitle_test.cc
index f7a9450f5827c09693c227483549a78e0e8bfba9..22e4fd9ffe9b6c1638c1d18d63773bc0c47f2e0b 100644 (file)
--- 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 07110346b04441f2b7a7af166c04fd9d1e2fc502..19c4a2154b2dd44a46faebda0ef2abdd70c9d3be 100644 (file)
--- 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 8768815ad97e16bf6f33145e161fb72bae587f46..56a0964c4c49f3803c87f2b71ba3d320aa4e1828 100644 (file)
--- 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 a931451630b769d72184a808728081275432eab9..49ad4ea05ddf3a5c6550c8e4c609c1f19db64636 100644 (file)
--- 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);
 }