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>).

diff --git a/src/lib/dcp_decoder.cc b/src/lib/dcp_decoder.cc
index 3a187186390dcaa76330494f1e94410d51b87476..24ff507e687ba43f5918e3bb4986e9fc979dbbf8 100644 (file)
--- 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 400c7d26ba86eeb35799276af4cdce24a3b87517..d9c904720efd09b229ec336f91c37c2f43a533ef 100644 (file)
--- 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 54e283548a49f031a030df4b8b4e823f31bab33b..444fb7567f6947098645591802da349f54aaacac 100644 (file)
--- 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 b3e24d5e2dceef959cfb74d67fecbb64f62b9928..8de5967efb51421d64240fec484a43da786cbbc8 100644 (file)
--- 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 5949f8b0ba5ef866b68a418379ff98c182b0053f..89a6f26a2b28dd34e7df1cc58e42669b3db44687 100644 (file)
--- 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 b3e6d75531bcbf4fd86ffd0d812aae9558048f19..711dc77f263e6cf7d2d64ba40bc877f33a6a761d 100644 (file)
--- 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 45a4999ddcf769781bd87c8a577264b6511a6e06..9d085125382e82f55d237e52414585f3e3930cf4 100644 (file)
--- 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 c566c3676c956f9685f92d455e5261cfabfc2ce9..5263e7f9087525c02e8485f26f5a8330d98a7784 100644 (file)
--- 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 bd99cad6313fa336ddddef6df85d08d0a161d2ec..fe4b9ef077e7b3d4bfab6c296a48d0644f5cfd39 100644 (file)
--- 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
index a4ad4c1a4880d02aabf2790e11c4e5c2c28034dc..314e09cbac2e023a7acb61e1b32db76fe6dd775e 160000 (submodule)
--- a/test/data
+++ b/test/data
@@ -1 +1 @@
-Subproject commit a4ad4c1a4880d02aabf2790e11c4e5c2c28034dc
+Subproject commit 314e09cbac2e023a7acb61e1b32db76fe6dd775e

diff --git a/test/dcp_subtitle_test.cc b/test/dcp_subtitle_test.cc
index 9b7b77279968f6afffa977c448a685ebb7989ed4..4928d92c1883c634c150258995fdd42e3a944d93 100644 (file)
--- 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 a989d3aeb5b0e6c74c23340f1341af6e5c07c1aa..949ba18c0c5abbecba258e68976f03805f1b515a 100644 (file)
--- 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 f6bd48c51d0859260b5e36e42c63fd57f3db9f47..56207bfcbc28e1818570fe38f2fa0e1580db7965 100644 (file)
--- 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"));
+}