From 22aa0dd620b6db93a64e1e171fb5ddb18693e56e Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Wed, 19 Apr 2023 23:57:03 +0200 Subject: [PATCH] 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. --- src/lib/font.h | 4 +++ src/lib/font_comparator.h | 68 ++++++++++++++++++++++++++++++++++++ src/lib/font_config.cc | 7 ++-- src/lib/font_config.h | 3 +- test/font_comparator_test.cc | 27 ++++++++++++++ test/wscript | 1 + 6 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 src/lib/font_comparator.h create mode 100644 test/font_comparator_test.cc diff --git a/src/lib/font.h b/src/lib/font.h index b9e90f65e..0afd873e1 100644 --- a/src/lib/font.h +++ b/src/lib/font.h @@ -85,6 +85,10 @@ public: boost::optional file; }; + Content content() const { + return _content; + } + boost::signals2::signal Changed; private: diff --git a/src/lib/font_comparator.h b/src/lib/font_comparator.h new file mode 100644 index 000000000..07ad15b6a --- /dev/null +++ b/src/lib/font_comparator.h @@ -0,0 +1,68 @@ +/* + Copyright (C) 2023 Carl Hetherington + + 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 . + +*/ + + +#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(a.file); + auto const b_has_file = static_cast(b.file); + auto const a_has_data = static_cast(a.data); + auto const b_has_data = static_cast(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 d4a442fc1..8804bd6d9 100644 --- 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 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 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 edcb4be36..57f733b7b 100644 --- a/src/lib/font_config.h +++ b/src/lib/font_config.h @@ -19,6 +19,7 @@ */ +#include "font_comparator.h" #include #include #include @@ -41,7 +42,7 @@ private: ~FontConfig(); FcConfig* _config = nullptr; - std::map _available_fonts; + std::map _available_fonts; std::vector _temp_files; diff --git a/test/font_comparator_test.cc b/test/font_comparator_test.cc new file mode 100644 index 000000000..dad058a74 --- /dev/null +++ b/test/font_comparator_test.cc @@ -0,0 +1,27 @@ +#include "lib/font.h" +#include "lib/font_comparator.h" +#include +#include + + +using std::make_shared; +using std::map; +using std::shared_ptr; +using std::string; + + +BOOST_AUTO_TEST_CASE(font_comparator_test) +{ + map cache; + + auto font = make_shared("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 9917e7802..827a3feb8 100644 --- 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 -- 2.30.2