diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp index 867b82e285a..409128a6fd0 100644 --- a/src/mbgl/layout/symbol_layout.cpp +++ b/src/mbgl/layout/symbol_layout.cpp @@ -256,7 +256,7 @@ void SymbolLayout::prepare(uintptr_t tileUID, // Add the glyphs we need for this label to the glyph atlas. if (result) { - glyphAtlas.addGlyphs(tileUID, text, layout.get(), **glyphSet, face); + glyphAtlas.addGlyphs(tileUID, text, layout.get(), glyphSet, face); } return result; diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp index 88b319366d4..1b3f7518b5a 100644 --- a/src/mbgl/text/glyph_atlas.cpp +++ b/src/mbgl/text/glyph_atlas.cpp @@ -23,16 +23,17 @@ GlyphAtlas::GlyphAtlas(const Size size, FileSource& fileSource_) GlyphAtlas::~GlyphAtlas() = default; void GlyphAtlas::requestGlyphRange(const FontStack& fontStack, const GlyphRange& range) { - std::lock_guard lock(rangesMutex); - auto& rangeSets = ranges[fontStack]; + std::lock_guard lock(mutex); + auto& rangeSets = entries[fontStack].ranges; const auto& rangeSetsIt = rangeSets.find(range); if (rangeSetsIt != rangeSets.end()) { return; } - rangeSets.emplace(range, - std::make_unique(this, fontStack, range, observer, fileSource)); + rangeSets.emplace(std::piecewise_construct, + std::forward_as_tuple(range), + std::forward_as_tuple(this, fontStack, range, observer, fileSource)); } bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet& glyphRanges) { @@ -40,8 +41,8 @@ bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet& return true; } - std::lock_guard lock(rangesMutex); - const auto& rangeSets = ranges[fontStack]; + std::lock_guard lock(mutex); + const auto& rangeSets = entries[fontStack].ranges; bool hasRanges = true; for (const auto& range : glyphRanges) { @@ -55,7 +56,7 @@ bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet& continue; } - if (!rangeSetsIt->second->isParsed()) { + if (!rangeSetsIt->second.isParsed()) { hasRanges = false; } } @@ -64,16 +65,8 @@ bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet& } util::exclusive GlyphAtlas::getGlyphSet(const FontStack& fontStack) { - auto lock = std::make_unique>(glyphSetsMutex); - - auto it = glyphSets.find(fontStack); - if (it == glyphSets.end()) { - it = glyphSets.emplace(fontStack, std::make_unique()).first; - } - - // FIXME: We lock all GlyphSets, but what we should - // really do is lock only the one we are returning. - return { it->second.get(), std::move(lock) }; + auto lock = std::make_unique>(mutex); + return { &entries[fontStack].glyphSet, std::move(lock) }; } void GlyphAtlas::setObserver(GlyphAtlasObserver* observer_) { @@ -83,12 +76,10 @@ void GlyphAtlas::setObserver(GlyphAtlasObserver* observer_) { void GlyphAtlas::addGlyphs(uintptr_t tileUID, const std::u16string& text, const FontStack& fontStack, - const GlyphSet& glyphSet, + const util::exclusive& glyphSet, GlyphPositions& face) { - std::lock_guard lock(mtx); - - const std::map& sdfs = glyphSet.getSDFs(); + const std::map& sdfs = glyphSet->getSDFs(); for (char16_t chr : text) { @@ -107,7 +98,7 @@ Rect GlyphAtlas::addGlyph(uintptr_t tileUID, const FontStack& fontStack, const SDFGlyph& glyph) { - std::map& face = index[fontStack]; + std::map& face = entries[fontStack].glyphValues; auto it = face.find(glyph.id); // The glyph is already in this texture. @@ -151,10 +142,10 @@ Rect GlyphAtlas::addGlyph(uintptr_t tileUID, } void GlyphAtlas::removeGlyphs(uintptr_t tileUID) { - std::lock_guard lock(mtx); + std::lock_guard lock(mutex); - for (auto& faces : index) { - std::map& face = faces.second; + for (auto& entry : entries) { + std::map& face = entry.second.glyphValues; for (auto it = face.begin(); it != face.end(); /* we advance in the body */) { GlyphValue& value = it->second; value.ids.erase(tileUID); @@ -190,8 +181,6 @@ Size GlyphAtlas::getSize() const { } void GlyphAtlas::upload(gl::Context& context, gl::TextureUnit unit) { - std::lock_guard lock(mtx); - if (!texture) { texture = context.createTexture(image, unit); } else if (dirty) { diff --git a/src/mbgl/text/glyph_atlas.hpp b/src/mbgl/text/glyph_atlas.hpp index 84f29aeb737..8267630096f 100644 --- a/src/mbgl/text/glyph_atlas.hpp +++ b/src/mbgl/text/glyph_atlas.hpp @@ -17,8 +17,6 @@ #include #include #include -#include -#include namespace mbgl { @@ -57,7 +55,7 @@ class GlyphAtlas : public util::noncopyable { void addGlyphs(uintptr_t tileUID, const std::u16string& text, const FontStack&, - const GlyphSet&, + const util::exclusive&, GlyphPositions&); void removeGlyphs(uintptr_t tileUID); @@ -77,20 +75,9 @@ class GlyphAtlas : public util::noncopyable { const FontStack&, const SDFGlyph&); - FileSource& fileSource; std::string glyphURL; - std::unordered_map>, FontStackHash> ranges; - std::mutex rangesMutex; - - std::unordered_map, FontStackHash> glyphSets; - std::mutex glyphSetsMutex; - - util::WorkQueue workQueue; - - GlyphAtlasObserver* observer = nullptr; - struct GlyphValue { GlyphValue(Rect rect_, uintptr_t id) : rect(std::move(rect_)), ids({ id }) {} @@ -98,9 +85,19 @@ class GlyphAtlas : public util::noncopyable { std::unordered_set ids; }; - std::mutex mtx; + struct Entry { + std::map ranges; + GlyphSet glyphSet; + std::map glyphValues; + }; + + std::unordered_map entries; + std::mutex mutex; + + util::WorkQueue workQueue; + GlyphAtlasObserver* observer = nullptr; + BinPack bin; - std::unordered_map, FontStackHash> index; AlphaImage image; std::atomic dirty; mbgl::optional texture; diff --git a/test/text/glyph_atlas.test.cpp b/test/text/glyph_atlas.test.cpp index 1ad2af9d820..3679cabc1b5 100644 --- a/test/text/glyph_atlas.test.cpp +++ b/test/text/glyph_atlas.test.cpp @@ -83,8 +83,7 @@ TEST(GlyphAtlas, LoadingFail) { EXPECT_TRUE(error != nullptr); EXPECT_EQ(util::toString(error), "Failed by the test case"); - auto glyphSet = test.glyphAtlas.getGlyphSet({{"Test Stack"}}); - ASSERT_TRUE(glyphSet->getSDFs().empty()); + ASSERT_TRUE(test.glyphAtlas.getGlyphSet({{"Test Stack"}})->getSDFs().empty()); ASSERT_FALSE(test.glyphAtlas.hasGlyphRanges({{"Test Stack"}}, {{0, 255}})); test.end(); @@ -112,8 +111,7 @@ TEST(GlyphAtlas, LoadingCorrupted) { EXPECT_TRUE(error != nullptr); EXPECT_EQ(util::toString(error), "unknown pbf field type exception"); - auto glyphSet = test.glyphAtlas.getGlyphSet({{"Test Stack"}}); - ASSERT_TRUE(glyphSet->getSDFs().empty()); + ASSERT_TRUE(test.glyphAtlas.getGlyphSet({{"Test Stack"}})->getSDFs().empty()); ASSERT_FALSE(test.glyphAtlas.hasGlyphRanges({{"Test Stack"}}, {{0, 255}})); test.end(); @@ -144,21 +142,21 @@ TEST(GlyphAtlas, LoadingCancel) { } TEST(GlyphAtlas, InvalidSDFGlyph) { - GlyphSet glyphSet; - glyphSet.insert(66, SDFGlyph{ 66 /* ASCII 'B' */, + const FontStack fontStack{ "Mock Font" }; + + GlyphAtlasTest test; + GlyphPositions positions; + + auto glyphSet = test.glyphAtlas.getGlyphSet(fontStack); + glyphSet->insert(66, SDFGlyph{ 66 /* ASCII 'B' */, AlphaImage({7, 7}), /* correct */ { 1 /* width */, 1 /* height */, 0 /* left */, 0 /* top */, 0 /* advance */ } }); - glyphSet.insert(67, SDFGlyph{ 67 /* ASCII 'C' */, + glyphSet->insert(67, SDFGlyph{ 67 /* ASCII 'C' */, AlphaImage({518, 8}), /* correct */ { 512 /* width */, 2 /* height */, 0 /* left */, 0 /* top */, 0 /* advance */ } }); - - const FontStack fontStack{ "Mock Font" }; - - GlyphAtlasTest test; - GlyphPositions positions; test.glyphAtlas.addGlyphs(1, std::u16string{u"ABC"}, fontStack, glyphSet, positions); ASSERT_EQ(2u, positions.size());