[core] Don't overwrite locally renderable glyphs with remote glyphs #15407

asheemmamoowala · 2019-08-19T07:16:31Z

TODO

Unit tests
Manual test to verify the fix

src/mbgl/text/glyph_manager.cpp

pozdnyakov · 2019-08-20T13:59:11Z

src/mbgl/text/glyph_manager.cpp

@@ -99,8 +99,10 @@ void GlyphManager::processResponse(const Response& res, const FontStack& fontSta

        for (auto& glyph : glyphs) {
            auto id = glyph.id;
-            entry.glyphs.erase(id);
-            entry.glyphs.emplace(id, makeMutable<Glyph>(std::move(glyph)));
+            if (!localGlyphRasterizer->canRasterizeGlyph(fontStack, id)) {


We have similar check inside GlyphManager::getGlyphs(), so that we request only the ranges that cannot be rasterized locally. How would it be possible to have locally rasterizable glyphs in the response?

There are a few cases where the 256 glyph range includes glyphs that are locally rasterizable and remotely fetched.

In such a range, when a glyph is fetched from the remote request, processResponse will currently add it to the entries table.

A subsequent call to getGlyphs will correctly pass the canRasterizeGlyph check, but will return the glyph that was previously written into entries by processResponse.

mapbox-gl-native/src/mbgl/text/glyph_manager.cpp

Lines 35 to 38 in 5d1ed5d

if (localGlyphRasterizer->canRasterizeGlyph(fontStack, glyphID)) {

if (entry.glyphs.find(glyphID) == entry.glyphs.end()) {

entry.glyphs.emplace(glyphID, makeMutable<Glyph>(generateLocalSDF(fontStack, glyphID)));

}

asheemmamoowala · 2019-08-21T03:03:11Z

test/text/glyph_manager.test.cpp

@@ -253,6 +253,57 @@ TEST(GlyphManager, LoadLocalCJKGlyph) {
        });
 }

+TEST(GlyphManager, LoadLocalCJKGlyphAfterLoadingRangeFromURL) {


@pozdnyakov I've added a unit test that reproduces the issue. The fixture is pretty large - but I haven't got an idea of how to create a more synthetic fixture for this test.

It requests 々(0x3005), which is in the range 12244-12543. This range overlaps with Katakana letters which are expected to be locally generated. Without the changes in GlyphManager, this test will use the glyph fetched for the pbf for the katakana letter - and fails.

asheemmamoowala · 2019-08-21T04:13:00Z

Ive as yet been unable to reproduce this issue on the macosapp, mbgl-glfw and on Android. The streets-v11 style doesn't show the behavior shown in #15405, so the fix here is speculative, though the unit test does show there is an underlying issue that is worth fixing.

asheemmamoowala · 2019-08-21T04:31:54Z

CI Failures:

ci/circleci: ios-debug-xcode11 : is also failing on master. Will need to rebase after a fix is merged there.
ci/circleci: android-debug-arm-v8 : Sailfish OS error, but I'm unable to access the logs of that device
~~ci/circleci: linux-render-tests : Failing the regressions/mapbox-gl-js#3107 test which is entirely unrelated to the changes in this PR.~~ (resolved by rebuild)

cc @pozdnyakov @julianrex @LukasPaczos

asheemmamoowala · 2019-08-21T05:12:05Z

Changelog:

Fixed a bug where glyphs generated through the LocalGlyphRasterizer interface were changing fonts during some zoom changes. ([core] Locally generated glyphs shift on over-zoomed tiles #15405)

chloekraw · 2019-08-21T05:20:11Z

Thanks @asheemmamoowala! I have a preference for the following changelog, any objections?

Fixed a bug where glyphs generated through localIdeographFontFamily: true were changing fonts during some zoom changes. ([core] Locally generated glyphs shift on over-zoomed tiles #15405)

Reasons:

tying this to localIdeographFontFamily increases clarity for the customer about how they could encounter the bug
shifting baseline is a side effect of shifting fonts (and not capturing glyph metrics in our renderer); the main problem is the font changes
changelogs on native (are supposed to) use the past tense

asheemmamoowala · 2019-08-21T05:29:57Z

@chloekraw The platform teams will need to change the final message to reflect the platform version of localIdeographFontFamily, and that is why I was more generic in the description.

GL-Native doesn't use localIdeographFontFamily: true that flag is from GL-JS, so I've edited my original proposal (#15407 (comment)) to reflect your suggestions and the correct option

chloekraw · 2019-08-21T05:33:22Z

@asheemmamoowala yeah, we always take that step to translate property names to the proper ones on each platform when doing every release. It doesn’t stop us from referencing specific properties in our changelogs.

Also, localIdeographFontFamily: true happens to be accurate for Android.

pozdnyakov

@asheemmamoowala thanks for the explanation and the unit test!

alexshalamov · 2019-08-21T07:50:02Z

@pozdnyakov testTileSetFromTileURLTemplates failure is unrelated to this PR, should we add changelog and land it?

asheemmamoowala added the Core The cross-platform C++ core, aka mbgl label Aug 19, 2019

asheemmamoowala added this to the release-queso milestone Aug 19, 2019

asheemmamoowala requested review from tmpsantos and pozdnyakov August 19, 2019 07:16

pozdnyakov reviewed Aug 19, 2019

View reviewed changes

src/mbgl/text/glyph_manager.cpp Outdated Show resolved Hide resolved

chloekraw mentioned this pull request Aug 19, 2019

[core] Locally generated glyphs shift on over-zoomed tiles #15405

Closed

alexshalamov changed the title ~~Don't overwrite locally renderable glyphs with remote glyphs~~ [core] Don't overwrite locally renderable glyphs with remote glyphs Aug 20, 2019

alexshalamov force-pushed the fix-local-glyphs branch from 90ec5d3 to 258468d Compare August 20, 2019 13:29

pozdnyakov reviewed Aug 20, 2019

View reviewed changes

friedbunny added the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 20, 2019

asheemmamoowala commented Aug 21, 2019

View reviewed changes

asheemmamoowala requested a review from pozdnyakov August 21, 2019 04:42

pozdnyakov approved these changes Aug 21, 2019

View reviewed changes

alexshalamov approved these changes Aug 21, 2019

View reviewed changes

Asheem Mamoowala and others added 4 commits August 21, 2019 11:17

[core] Don't overwrite locally renderable glyphs with remote glyphs

269da81

Correclty implement canRasterizeGlyph in unit test stub

a69867e

Add unit test and fixture

1edbd89

[android][ios] Add change log entries

5347783

pozdnyakov force-pushed the fix-local-glyphs branch from 3103104 to 5347783 Compare August 21, 2019 08:28

asheemmamoowala requested a review from a team August 21, 2019 08:28

pozdnyakov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 21, 2019

pozdnyakov merged commit 4767b56 into master Aug 21, 2019

pozdnyakov deleted the fix-local-glyphs branch August 21, 2019 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Don't overwrite locally renderable glyphs with remote glyphs #15407

[core] Don't overwrite locally renderable glyphs with remote glyphs #15407

asheemmamoowala commented Aug 19, 2019 •

edited

Loading

pozdnyakov Aug 20, 2019

asheemmamoowala Aug 20, 2019

asheemmamoowala Aug 21, 2019

asheemmamoowala commented Aug 21, 2019

asheemmamoowala commented Aug 21, 2019 •

edited

Loading

asheemmamoowala commented Aug 21, 2019 •

edited

Loading

chloekraw commented Aug 21, 2019

asheemmamoowala commented Aug 21, 2019 •

edited

Loading

chloekraw commented Aug 21, 2019

pozdnyakov left a comment

alexshalamov commented Aug 21, 2019

	if (localGlyphRasterizer->canRasterizeGlyph(fontStack, glyphID)) {
	if (entry.glyphs.find(glyphID) == entry.glyphs.end()) {
	entry.glyphs.emplace(glyphID, makeMutable<Glyph>(generateLocalSDF(fontStack, glyphID)));
	}

[core] Don't overwrite locally renderable glyphs with remote glyphs #15407

[core] Don't overwrite locally renderable glyphs with remote glyphs #15407

Conversation

asheemmamoowala commented Aug 19, 2019 • edited Loading

TODO

pozdnyakov Aug 20, 2019

Choose a reason for hiding this comment

asheemmamoowala Aug 20, 2019

Choose a reason for hiding this comment

asheemmamoowala Aug 21, 2019

Choose a reason for hiding this comment

asheemmamoowala commented Aug 21, 2019

asheemmamoowala commented Aug 21, 2019 • edited Loading

asheemmamoowala commented Aug 21, 2019 • edited Loading

Changelog:

chloekraw commented Aug 21, 2019

asheemmamoowala commented Aug 21, 2019 • edited Loading

chloekraw commented Aug 21, 2019

pozdnyakov left a comment

Choose a reason for hiding this comment

alexshalamov commented Aug 21, 2019

asheemmamoowala commented Aug 19, 2019 •

edited

Loading

asheemmamoowala commented Aug 21, 2019 •

edited

Loading

asheemmamoowala commented Aug 21, 2019 •

edited

Loading

asheemmamoowala commented Aug 21, 2019 •

edited

Loading