[core] Don't use a separate SpriteAtlas for annotation images #9003

jfirebaugh · 2017-05-15T18:11:58Z

Instead, just add them to the Style as needed. Includes changes from #8905 and takes care to avoid regressing #3817.

In order for this to not reduce the total combined capacity for spite and annotation images, while simultaneously not increasing the base memory consumption and failing the related test, it includes a switch to auto-growing the SpriteAtlas texture using shelf-pack-cpp. This also fixes #8247 ("Measured vectorFootprint = 31104204 Bytes").

mb12 · 2017-05-16T05:42:30Z

@jfirebaugh The size/capacity of Style::spriteAtlas should also be changed to 2048x2048 since it now needs to have capacity for style sprite as well as user added annotations.

kkaefer · 2017-05-16T08:42:58Z

src/mbgl/sprite/sprite_atlas.cpp

@@ -42,13 +42,8 @@ SpriteAtlas::SpriteAtlas(Size size_, float pixelRatio_)

 SpriteAtlas::~SpriteAtlas() = default;

-void SpriteAtlas::onSpriteLoaded(Images&& result) {
+void SpriteAtlas::onSpriteLoaded() {
 markAsLoaded();


We can now remove this public method and set loaded = true directly.

kkaefer · 2017-05-16T08:53:04Z

src/mbgl/annotation/annotation_manager.cpp

+ if (style.getImage(image)) {
+ style.removeImage(image);
+ }
+ }


I'm worried about how this will interact with image names from the sprite sheet: Removing annotation images that have the same name as images in the sprite sheet means that they will be dangling.

I went back and forth about this. We could prefix the ids when adding to the style. But using the same namespace would actually fix #4750.

Could we use some sort of reference counting system?

What do you have in mind?

For glyphs, we store the GlyphRequestor in each Entry/GlyphValue, and only remove it from the atlas once all requestors are gone. We could use a similar model for requesting sprite images that were implicitly added to the sprite atlas.

I elected to use a prefix, as that solves both the removal issue and other issues with ID collisions.

kkaefer · 2017-05-16T08:56:21Z

src/mbgl/annotation/annotation_manager.cpp

@@ -167,13 +159,26 @@ void AnnotationManager::updateStyle(Style& style) {
 shape.second->updateStyle(style);
 }

+ for (const auto& image : images) {
+ if (!style.getImage(image.first)) {
+ style.addImage(image.first, std::make_unique<style::Image>(image.second));


Can we move images instead of copying them? This will leave the Image object in the unordered_map empty, but we'll never access it again (there's no getImage method on the AnnotationManager). We can treat the mere presence of the key as the availability of this image, and only call style.addImage when we still own the image.

For clarity, I'd rather copy; it's cheap, because all that's being copied is an Immutable, which is a shared pointer. (Edit: we actually have to copy because annotation images must persist across style replacement.)

kkaefer · 2017-05-16T08:56:55Z

src/mbgl/annotation/annotation_manager.cpp

- addSpriteImage(spriteImages, id, std::move(image), [&](style::Image& added) {
- spriteAtlas.addImage(id, added.impl);
- });
+ images.emplace(id, *image);


Why are we making a copy here? We already own the style::Image object.

jfirebaugh · 2017-05-16T17:46:13Z

In order for this to not reduce the total combined capacity for spite and annotation images, while simultaneously not increasing the base memory consumption and failing the related test, we'll need to implement auto-growing atlas textures.

ivovandongen · 2017-05-19T16:24:22Z

src/mbgl/sprite/sprite_atlas.cpp

- if (entry.iconRect) {
- bin.release(*entry.iconRect);
+ if (entry.iconBin) {
+// shelfPack.release(*iconBin);


Does shelfpack currently support removing bins?

Not currently, we're waiting on v2 / mapbox/shelf-pack-cpp#5 which @bhousel is working on.

jfirebaugh · 2017-05-26T05:38:53Z

This is ready for re-review.

kkaefer · 2017-05-26T10:10:29Z

src/mbgl/annotation/annotation_manager.cpp

@@ -155,7 +148,7 @@ void AnnotationManager::updateStyle(Style& style) {
 std::unique_ptr<SymbolLayer> layer = std::make_unique<SymbolLayer>(PointLayerID, SourceID);

 layer->setSourceLayer(PointLayerID);
- layer->setIconImage({"{sprite}"});
+ layer->setIconImage({SourceID + ".{sprite}"});


kkaefer · 2017-05-26T10:12:53Z

src/mbgl/annotation/annotation_manager.cpp

@@ -166,13 +159,26 @@ void AnnotationManager::updateStyle(Style& style) {
 shape.second->updateStyle(style);
 }

+ for (const auto& image : images) {
+ if (!style.getImage(image.first)) {
+ style.addImage(std::make_unique<style::Image>(image.second));


Could we move them instead of copying them? What is the benefit of retaining the image?

See previous discussion.

kkaefer · 2017-05-26T10:14:16Z

src/mbgl/annotation/annotation_manager.cpp

+ // create a new image with the input ID prefixed by "com.mapbox.annotations".
+ std::string id = SourceID + "." + image->getID();
+ images.emplace(id,
+ style::Image(id, image->getImage().clone(), image->getPixelRatio(), image->isSdf()));


This is a no-op when there is already an image with the same ID. Adding a different image with the same name will thus not have any affect. Is that the intended API behavior?

Maybe we could have a list of IDs we currently have, a list of IDs to remove, and a map of new sprite images to add, then replace existing entries in the map when calling addImage.

Good point; I think the existing behavior is to replace the image (if it's the same size). I'll confirm and preserve that if so.

Keeping a map of new sprite images to add isn't sufficient, because we also need to add all images in the case where there's a fresh style.

Eventually I think we should refactor AnnotationManager so that it updates the style immediately in each operation, and is notified when there's a fresh style, but that's a separate refactor. For now I made it add all images every updateStyle.

We gained a lot of overhead by reducing the initial SpriteAtlas size.

Instead, just add them to the Style as needed. Includes changes from #8905 and takes care to avoid regressing #3817.

jfirebaugh requested a review from kkaefer May 15, 2017 18:11

jfirebaugh force-pushed the render-sprite-atlas branch 2 times, most recently from d990b2c to 75fecf2 Compare May 15, 2017 21:16

kkaefer suggested changes May 16, 2017

View reviewed changes

jfirebaugh force-pushed the render-sprite-atlas branch 2 times, most recently from 417d118 to 6c6cb2e Compare May 16, 2017 16:51

jfirebaugh force-pushed the render-sprite-atlas branch from 6c6cb2e to 7b61b8a Compare May 16, 2017 19:37

ivovandongen mentioned this pull request May 17, 2017

Asynchronous rendering #8820

Closed

28 tasks

jfirebaugh force-pushed the render-sprite-atlas branch from 7b61b8a to 957e6e0 Compare May 18, 2017 23:40

ivovandongen reviewed May 19, 2017

View reviewed changes

jfirebaugh force-pushed the render-sprite-atlas branch from 957e6e0 to f3dbb82 Compare May 22, 2017 20:33

jfirebaugh changed the base branch from master to sprite-atlas-coordinates May 22, 2017 20:34

jfirebaugh force-pushed the sprite-atlas-coordinates branch from 67d5254 to f7cd879 Compare May 23, 2017 19:29

jfirebaugh changed the base branch from sprite-atlas-coordinates to master May 23, 2017 19:59

jfirebaugh force-pushed the render-sprite-atlas branch 7 times, most recently from 25018c1 to 5dc6a13 Compare May 26, 2017 04:30

jfirebaugh force-pushed the render-sprite-atlas branch 2 times, most recently from e9cd79d to 4faedf6 Compare May 26, 2017 05:45

kkaefer approved these changes May 26, 2017

View reviewed changes

jfirebaugh added 3 commits May 26, 2017 10:23

[tests] Add test for calling addAnnotationImage with an existing ID

ddecdd6

[core] Auto-growable SpriteAtlas using shelf-pack

6ab995b

[test] Lower memory ceiling

bef533c

We gained a lot of overhead by reducing the initial SpriteAtlas size.

jfirebaugh added 2 commits May 26, 2017 10:43

[core] Don't use a separate SpriteAtlas for annotation images

e04c90b

Instead, just add them to the Style as needed. Includes changes from #8905 and takes care to avoid regressing #3817.

[core] Don't need unique_ptr for AnnotationManager

ac2f1f3

jfirebaugh force-pushed the render-sprite-atlas branch from 4faedf6 to ac2f1f3 Compare May 26, 2017 17:44

jfirebaugh merged commit 2e0ceeb into master May 26, 2017

kkaefer deleted the render-sprite-atlas branch May 29, 2017 09:28

kkaefer added Core The cross-platform C++ core, aka mbgl refactor labels Jun 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Don't use a separate SpriteAtlas for annotation images #9003

[core] Don't use a separate SpriteAtlas for annotation images #9003

jfirebaugh commented May 15, 2017 •

edited

Loading

mb12 commented May 16, 2017

kkaefer May 16, 2017

kkaefer May 16, 2017

jfirebaugh May 16, 2017

kkaefer May 16, 2017

jfirebaugh May 16, 2017

kkaefer May 17, 2017

jfirebaugh May 24, 2017

kkaefer May 16, 2017

jfirebaugh May 16, 2017 •

edited

Loading

kkaefer May 16, 2017

jfirebaugh commented May 16, 2017

ivovandongen May 19, 2017

jfirebaugh May 19, 2017

jfirebaugh commented May 26, 2017

kkaefer May 26, 2017

kkaefer May 26, 2017

jfirebaugh May 26, 2017

kkaefer May 26, 2017

kkaefer May 26, 2017

kkaefer May 26, 2017

jfirebaugh May 26, 2017

jfirebaugh May 26, 2017

[core] Don't use a separate SpriteAtlas for annotation images #9003

[core] Don't use a separate SpriteAtlas for annotation images #9003

Conversation

jfirebaugh commented May 15, 2017 • edited Loading

mb12 commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh commented May 15, 2017 •

edited

Loading

jfirebaugh May 16, 2017 •

edited

Loading