Glyph Atlas improvements #1923

bhousel · 2016-01-12T21:33:25Z

Improves but does not completely eliminate issues with glyph atlas overflow (#141)

GlyphSource now manages multiple GlyphAtlases, 1 per fontstack.
GlyphAtlas GL textures start out 128x128 and grow by power of 2 up to 1024x1024
BinPack now uses shelf best height fit algorithm, more efficient packing for things with similar heights
Doesn't attempt to remove glyphs when removing tiles (performance boost)

bhousel · 2016-01-12T21:37:45Z

cc @mourner @ansis @jfirebaugh

ansis · 2016-01-12T21:46:28Z

Doesn't attempt to remove glyphs when removing tiles (performance boost)

Do glyphs ever get removed? How big is the performance benefit?

bhousel · 2016-01-12T22:04:45Z

Do glyphs ever get removed? How big is the performance benefit?

They do not get removed - for most languages with only a few dozen glyphs this is probably desired anyway as the overhead of constantly removing and then adding them back is not worth it. I would like to benchmark this rather than just trusting my own perceived performance.

For Asian character sets, I could see a potential rationale for refcounting the glyphs and removing the ones that aren't visible anymore.

I have been testing around this location in Southern China: # 10.01/22.9452/113.1932 - set tilt to maximum to load as many tiles as possible, and scroll around. It does take a LOT of scrolling to get to 2048x2048, and even textures larger than that are considered safe for WebGL.

To exceed the 1024x1024 limit, we would need to change how texture coordinates are packed in SymbolBucket shaderAttributes.

mikemorris · 2016-01-12T22:13:52Z

For Asian character sets, I could see a potential rationale for refcounting the glyphs and removing the ones that aren't visible anymore.

Would it be possible to refcount all glyphs added to the atlas and only cull unreferenced glyphs if the atlas would be in danger of overflowing?

mourner · 2016-01-12T22:35:34Z

Yes, I think MRU-cache-like behavior would be optimal — remove the least used glyphs when atlas gets near overflowing. Might need some algorithmic effort though since you would have to use "holes" of different size in the atlas to place new glyphs.

mourner · 2016-01-12T22:38:46Z

As for the PR — the code changes look great. Rebase this into a few clean commits with passing builds and we can merge — even with the overflowing problem, it's better than the current approach.

bhousel · 2016-01-13T03:26:29Z

As for the PR — the code changes look great. Rebase this into a few clean commits with passing builds and we can merge — even with the overflowing problem, it's better than the current approach.

Great! I squashed away the middle commits and rebased to master.. I'll open a separate issue to track follow on improvements to this.

mourner · 2016-01-13T20:38:06Z

Looks good to me, @ansis want to give a final look?
Also, do we need a similar native PR before we can merge?

bhousel · 2016-01-13T20:40:38Z

I understand there is a need for resizing of sprite atlas also - want me to open a PR for that? It would be pretty easy to do (at least up to 1024x1024 dimensions).

ansis · 2016-01-13T22:01:39Z

@lucaswoj do you think have a separate glyphAtlas for each fonstack will cause problems with making text-font a data-driven property?

lucaswoj · 2016-01-13T23:38:56Z

do you think have a separate glyphAtlas for each fonstack will cause problems with making text-font a data-driven property?

Yes, it probably would. But I don't want to delay landing this bug fix for that reason.

mikemorris · 2016-01-14T05:41:26Z

@lucaswoj do you think have a separate glyphAtlas for each fonstack will cause problems with making text-font a data-driven property?

This will probably need to change to per-font glyph atlases with mapbox/DEPRECATED-mapbox-gl#4 if that makes a difference re data-driven styling.

lucaswoj · 2016-01-14T16:28:47Z

Upon further thought, I don't think this will affect data-driven styling. We render each symbol in a separate draw call. There will be a performance cost to switching glyph atlases between each draw call but we can put off digging into that.

mourner · 2016-01-14T16:44:53Z

But this will change after we do #1898, right?

lucaswoj · 2016-01-14T17:09:37Z

If/when we use ANGLE_instanced_arrays to render symbols, there'd still be one draw call per font

ansis · 2016-01-14T20:46:07Z

@bhousel looks good to me!

The only thing I'm wondering about is whether "not removing glyphs when tiles are removed" is worth it.(cc @kkaefer ) It would be useful to run a quick benchmark on master and see how much tile removeGlyphs takes up.

Other than that, this should be ready to merge!

bhousel · 2016-01-15T22:01:15Z

The only thing I'm wondering about is whether "not removing glyphs when tiles are removed" is worth it.(cc @kkaefer ) It would be useful to run a quick benchmark on master and see how much tile removeGlyphs takes up.

I did a quick benchmark of this... It's admittedly not great, but I cycled through a 5 glyph-heavy spots and let the map "quiesce" each time.

tl;dr:

GlyphAtlas.removeGlyphs takes up 3.5% of the time spent. (I consider this significant).
On this branch GlyphAtlas.addGlyphs jumps up a bit (I think a lot of that is the console warnings, which would go away once we grow the texture as much as I'd like).
DrawSymbols is slower too, probably from redundant glyphAtlas.updateTexture(gl); calls - these can be minimized.
I do agree that a better solution involves refcounting and replacing unused glyphs in the texture, so I want to do that as a next priority.
merge if you like it - it's at least not worse than what we have now.

Master:

This Branch:

jfirebaugh · 2016-01-15T22:05:00Z

test/js/symbol/binpack.test.js

+    t.test('not enough room', function(t) {
+        var bp = new BinPack(10, 10);
+        t.deepEqual(bp.allocate(10, 10), {x: 0, y: 0, w: 10, h: 10 }, 'first 10x10 bin');
+        t.deepEqual(bp.allocate(10, 10), {x: -1, y: -1 }, 'not enough room');


This can be a followup task, but allocate should return null or undefined rather than (-1, -1) to indicate failure.

jfirebaugh · 2016-01-15T22:07:26Z

merge if you like it

I like it, let's merge and ticket followups.

ansis · 2016-01-15T22:42:53Z

yeah, looks good! thanks for profiling

bhousel · 2016-01-16T00:12:24Z

Thanks! I'll cleanup & merge tonight..

bhousel · 2016-01-16T01:28:49Z

merged as
9d216c1 Limit glyph atlas texture growth to 1024x1024, remove comment
e55c8c5 Use Shelf Best Height Fit algorithm in BinPack, support texture resize
24b5eea Now GlyphSource will manage multiple GlyphAtlases (1 per fontstack)

ansis · 2016-01-16T01:57:07Z

\o/

jfirebaugh added the in progress label Jan 12, 2016

bhousel added 2 commits January 12, 2016 21:52

Now GlyphSource will manage multiple GlyphAtlases (1 per fontstack)

4b953da

Use Shelf Best Height Fit algorithm in BinPack, support texture resize

9d9aec7

bhousel force-pushed the glyph_overflow branch from 1c3f5c6 to d0c6b7b Compare January 13, 2016 03:08

jfirebaugh reviewed Jan 15, 2016
View reviewed changes

Limit glyph atlas texture growth to 1024x1024, remove comment

4a28715

bhousel force-pushed the glyph_overflow branch from d0c6b7b to 4a28715 Compare January 16, 2016 01:24

bhousel closed this Jan 16, 2016

jfirebaugh removed the in progress label Jan 16, 2016

bhousel deleted the glyph_overflow branch January 16, 2016 01:28

ansis mentioned this pull request Feb 1, 2016

Increase glyph atlas capacity mapbox/mapbox-gl-native#3781

Closed

bhousel mentioned this pull request Jul 22, 2016

Testing glyph rendering capacity for CJK fonts mapbox/mapbox-gl-test-suite#126

Closed

2 tasks

jfirebaugh mentioned this pull request Jun 9, 2017

[core] Per-bucket glyph and icon atlases mapbox/mapbox-gl-native#9213

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Glyph Atlas improvements #1923

Glyph Atlas improvements #1923

bhousel commented Jan 12, 2016

bhousel commented Jan 12, 2016

ansis commented Jan 12, 2016

bhousel commented Jan 12, 2016

mikemorris commented Jan 12, 2016

mourner commented Jan 12, 2016

mourner commented Jan 12, 2016

bhousel commented Jan 13, 2016

mourner commented Jan 13, 2016

bhousel commented Jan 13, 2016

ansis commented Jan 13, 2016

lucaswoj commented Jan 13, 2016

mikemorris commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

mourner commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

ansis commented Jan 14, 2016

bhousel commented Jan 15, 2016

jfirebaugh Jan 15, 2016

jfirebaugh commented Jan 15, 2016

ansis commented Jan 15, 2016

bhousel commented Jan 16, 2016

bhousel commented Jan 16, 2016

ansis commented Jan 16, 2016

Glyph Atlas improvements #1923

Glyph Atlas improvements #1923

Conversation

bhousel commented Jan 12, 2016

bhousel commented Jan 12, 2016

ansis commented Jan 12, 2016

bhousel commented Jan 12, 2016

mikemorris commented Jan 12, 2016

mourner commented Jan 12, 2016

mourner commented Jan 12, 2016

bhousel commented Jan 13, 2016

mourner commented Jan 13, 2016

bhousel commented Jan 13, 2016

ansis commented Jan 13, 2016

lucaswoj commented Jan 13, 2016

mikemorris commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

mourner commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

ansis commented Jan 14, 2016

bhousel commented Jan 15, 2016

tl;dr:

Master:

This Branch:

jfirebaugh Jan 15, 2016

Choose a reason for hiding this comment

jfirebaugh commented Jan 15, 2016

ansis commented Jan 15, 2016

bhousel commented Jan 16, 2016

bhousel commented Jan 16, 2016

ansis commented Jan 16, 2016