In 1c73379ed8483dcf71c5ccfc459c2c22516a9aef I changed

FontConfig::_available_fonts to use the font ID as a key, but that's
totally wrong because the same Font object with the same ID can have
its font filename/data changed, and in that case we don't want to
use the cached font.

Here we use the actual TTF/OTF font data as the key.  We could have
just hashed the data (whether it comes from a disk file or is held
in memory) but this is slower in the case where we have the filename,
as then the file must be loaded from disk for each comparison.

This fixes #2518.

diff --git a/src/lib/font.h b/src/lib/font.h
index b9e90f65e07e0052df4d5f4afa7da5f52c1c7e03..0afd873e1ded8fdd2a02d5a0c69c5534a682531c 100644 (file)
--- a/src/lib/font.h
+++ b/src/lib/font.h
@@ -85,6 +85,10 @@ public:
                boost::optional<boost::filesystem::path> file;
        };
 
+       Content content() const {
+               return _content;
+       }
+
        boost::signals2::signal<void()> Changed;
 
 private:

diff --git a/src/lib/font_comparator.h b/src/lib/font_comparator.h
new file mode 100644 (file)
index 0000000..07ad15b
--- /dev/null
+++ b/src/lib/font_comparator.h
@@ -0,0 +1,68 @@
+/*
+    Copyright (C) 2023 Carl Hetherington <cth@carlh.net>
+
+    This file is part of DCP-o-matic.
+
+    DCP-o-matic is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    DCP-o-matic is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with DCP-o-matic.  If not, see <http://www.gnu.org/licenses/>.
+
+*/
+
+
+#include "dcpomatic_assert.h"
+
+
+/** Comparator to allow dcpomatic::Font::Content to be compared for use in a map */
+struct FontComparator
+{
+       bool operator()(dcpomatic::Font::Content const& a, dcpomatic::Font::Content const& b) const {
+               auto const a_has_file = static_cast<bool>(a.file);
+               auto const b_has_file = static_cast<bool>(b.file);
+               auto const a_has_data = static_cast<bool>(a.data);
+               auto const b_has_data = static_cast<bool>(b.data);
+
+               if (!a_has_file && !a_has_data && !b_has_file && !b_has_data) {
+                       /* Neither font has any font data: a == b */
+                       return false;
+               } else if (!a_has_file && !a_has_data) {
+                       /* Fonts with no data are the "lowest": a < b */
+                       return true;
+               } else if (!b_has_file && !b_has_data) {
+                       /* ... so here b < a */
+                       return false;
+               } else if (a_has_file && !b_has_file) {
+                       /* Fonts with file are lower than fonts with data: a < b */
+                       return true;
+               } else if (!a_has_file && b_has_file) {
+                       /* ... so here b < a */
+                       return false;
+               } else if (a_has_file && b_has_file) {
+                       /* Compared as "equals" */
+                       return a.file->string() < b.file->string();
+               } else if (a_has_data && b_has_data) {
+                       /* Compared as "equals" */
+                       auto const a_size = a.data->size();
+                       auto const b_size = b.data->size();
+                       if (a_size != b_size) {
+                               return a_size < b_size;
+                       }
+                       return memcmp(a.data->data(), b.data->data(), a_size) < 0;
+               }
+
+               /* Should never get here */
+               DCPOMATIC_ASSERT(false);
+               return false;
+       };
+};
+
+

diff --git a/src/lib/font_config.cc b/src/lib/font_config.cc
index d4a442fc1f84434978b0ec02eaabce5fd6f89bd6..8804bd6d96522b8aa2baf0a8b9e341405393766e 100644 (file)
--- a/src/lib/font_config.cc
+++ b/src/lib/font_config.cc
@@ -56,7 +56,7 @@ FontConfig::~FontConfig()
 string
 FontConfig::make_font_available(shared_ptr<dcpomatic::Font> font)
 {
-       auto existing = _available_fonts.find(font->id());
+       auto existing = _available_fonts.find(font->content());
        if (existing != _available_fonts.end()) {
                return existing->second;
        }
@@ -107,7 +107,10 @@ FontConfig::make_font_available(shared_ptr<dcpomatic::Font> font)
 
        DCPOMATIC_ASSERT(font_name);
 
-       _available_fonts[font->id()] = *font_name;
+       /* We need to use the font object as the key, as we may be passed the same shared_ptr to a modified
+        * Font object in the future and in that case we need to load the new font.
+        */
+       _available_fonts[font->content()] = *font_name;
 
        FcConfigBuildFonts(_config);
        return *font_name;

diff --git a/src/lib/font_config.h b/src/lib/font_config.h
index edcb4be36a8a0d1b89c48ead0960ffa4e3547081..57f733b7b07ee854a8e2c8ece48879b30bd07f33 100644 (file)
--- a/src/lib/font_config.h
+++ b/src/lib/font_config.h
@@ -19,6 +19,7 @@
 */
 
 
+#include "font_comparator.h"
 #include <fontconfig/fontconfig.h>
 #include <boost/filesystem.hpp>
 #include <map>
@@ -41,7 +42,7 @@ private:
        ~FontConfig();
 
        FcConfig* _config = nullptr;
-       std::map<std::string, std::string> _available_fonts;
+       std::map<dcpomatic::Font::Content, std::string, FontComparator> _available_fonts;
 
        std::vector<boost::filesystem::path> _temp_files;
 

diff --git a/test/font_comparator_test.cc b/test/font_comparator_test.cc
new file mode 100644 (file)
index 0000000..dad058a
--- /dev/null
+++ b/test/font_comparator_test.cc
@@ -0,0 +1,27 @@
+#include "lib/font.h"
+#include "lib/font_comparator.h"
+#include <boost/test/unit_test.hpp>
+#include <iostream>
+
+
+using std::make_shared;
+using std::map;
+using std::shared_ptr;
+using std::string;
+
+
+BOOST_AUTO_TEST_CASE(font_comparator_test)
+{
+       map<dcpomatic::Font::Content, string, FontComparator> cache;
+
+       auto font = make_shared<dcpomatic::Font>("foo");
+
+       BOOST_CHECK(cache.find(font->content()) == cache.end());
+       cache[font->content()] = "foo";
+       BOOST_CHECK(cache.find(font->content()) != cache.end());
+
+       font->set_file("test/data/Inconsolata-VF.ttf");
+       BOOST_CHECK(cache.find(font->content()) == cache.end());
+}
+
+

diff --git a/test/wscript b/test/wscript
index 9917e780286744f431ed9346b9c725a4cc751d14..827a3feb854d4644bc481013f8c032c5eca4bcb9 100644 (file)
--- a/test/wscript
+++ b/test/wscript
@@ -94,6 +94,7 @@ def build(bld):
                  file_naming_test.cc
                  film_metadata_test.cc
                  find_missing_test.cc
+                 font_comparator_test.cc
                  frame_interval_checker_test.cc
                  frame_rate_test.cc
                  guess_crop_test.cc