-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[core] Per-bucket glyph and icon atlases #9213
Conversation
a8dce84
to
16acabd
Compare
16acabd
to
7ce6be3
Compare
7ce6be3
to
be01ae9
Compare
Memory usage is better than expected, but rendering performance is worse. Before:
After:
|
275f937
to
030912f
Compare
Profiling revealed that the textures were being re-created on every frame. 🙈 After fixing that, performance is the same as before:
|
src/mbgl/layout/symbol_layout.hpp
Outdated
@@ -94,6 +93,8 @@ class SymbolLayout { | |||
std::vector<SymbolInstance> symbolInstances; | |||
std::vector<SymbolFeature> features; | |||
|
|||
GlyphAtlas glyphAtlas; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the GlyphAtlas
more lightweight so that we only allocate significant memory/do significant work when we actually need the glyph atlas? I.e. some buckets may not produce any output at all, and in those cases it might make sense to delay initialization.
|
||
These are disparate responsibilities and should eventually be handled by different classes. When we implement | ||
data-driven support for `*-pattern`, we'll likely use per-bucket pattern atlases, and that would be a good time | ||
to refactor this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are patterns still handled globally rather than being the responsibility of a per-bucket atlas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The *-pattern
properties are all paint properties and not currently DDS enabled. So there's only one possible pattern per layer, and changing it via runtime styling takes effect immediately, without an async layout step. If/when we enable DDS, then we probably will want per-bucket atlases (at least in the data-driven case), but for now it makes more sense to keep it global.
Rather than having a single glyph atlas texture and a single image atlas texture, use:
This fixes a host of issues. Most prominently, it lifts limits on number and size of images and glyphs. All atlases are allowed to grow as large as they need to be (using shelf-pack-cpp's
autoResize
option). The effective limits are now a function of the maximum texture size (most important factor), unique glyphs/icons used per tile, and unique patterns used. Hopefully the result is high enough not to matter in practice.Secondly, it allows us to remove the restriction on changing the dimensions of an existing image, which is a prerequisite for full "smart setStyle" support (#7893). The restriction was required because symbol buffer attributes refer to coordinates within the atlas, such that buffer updates and atlas updates must be synchronized. With per-tile atlases, we can do that.
It does increase memory consumption somewhat, as glyphs and icons used in multiple tiles will appear in multiple textures. This increase is offset to some extent by the fact that the previous implementation added all images to the atlas (#9019) and never removed them (#2202).
I started out with per-bucket atlases, which would further increase the effective size limit at the expense of further increasing memory consumption. Per-tile atlases seem more natural and a better balance between segmentation and memory consumption.
This approach diverges from gl-js, which continues to use a single image atlas, and per-font-stack glyph atlases. The latter was implemented in mapbox/mapbox-gl-js#1923 but never ported to native. The approach in this PR has the advantage that it's amenable to making
text-font
data-driven, while gl-js's approach is not.TODO: