add onStyleImageMissing to allow dynamically loaded or generated images

[ansis]

ports mapbox/mapbox-gl-js#7987

This adds a onStyleImageMissing method to MapObserver that gets called with the id of each missing icon. Missing images can be provided by calling Style#addImage from within the listener. If an image is not added it is skipped and tile parsing continues.

virtual void onStyleImageMissing(const std::string&)

how the listener is called:

• ImageManager calls Renderer::Impl#onStyleImageMissing with id and callback
• Renderer::Impl#onStyleImageMissing calls the observer set by RendererFrontend with id and callback
• the platform's RendererFrontend calls the Map::Impl#onStyleImageMissing on the map thread with id and callback
• Map::Impl#onStyleImageMissing calls MapObserver#onStyleImageMissing with just the id and then calls the callback after the listener has been called

RendererFrontend is responsible for invoking the callback on the render thread.

---

TODO

• fix build
• Android bindings: Add style image missing binding integration #14320
• iOS bindings: [ios] Add iOS bindings and example for missing icons event. #14302
• avoid unnecessary tile reparsing when images are added (-js doesn't do this, I need to look into why this is necessary and how we can work around it)

[1ec5]

First of all, sorry for not taking the time to review and respond to mapbox/mapbox-gl-js#7587 in greater detail before the design for this event was implemented in GL JS and now ported here. Hopefully some of this feedback is still actionable, either here or in related work. For now, this feedback is specifically about the API proposed here for lazily loading images, not about its intended use for animation (#14125).

I’m a big fan of the idea that developers can supply programmatically generated content on demand. Computed shape sources already facilitate extremely dynamic or repetitive content, and computed raster sources would allow for tighter integration with platform 2D drawing libraries. (#7471 proposes the iOS equivalent to canvas sources.) The ability to set a paint or layout property to executable code in lambda functions (#7860) would free developers from having to learn a new language just to manipulate the style.

It seems like onStyleImageMissing is similar in spirit to these computed features, except that it’s an event rather than a callback, more like faulting than pulling. As an iOS/macOS developer, I’d think a method named -[MGLMapViewDelegate mapView:lacksStyleImageNamed:] would be where I’d be expected to log an error; I’d think the opportunity has already passed to provide that image. In fact, the iOS precedent cited in mapbox/mapbox-gl-js#7587, -[MGLMapViewDelegate mapView:imageForAnnotation:], requires a return value. I suppose the SDK could make the delegate method look like it’s pulling, by adding the returned image to the style for the application, but it sounds like you’d like to enable asynchronous image loading as part of this feature.

The nice thing about the lambda expressions proposed in #7860 is that they would be embedded in the context in which they’re evaluated. By analogy, developers really like the completion block in -[MGLMapView setCamera:withDuration:animationTimingFunction:completionHandler:] over -[MGLMapViewDelegate mapView:regionDidChange:] because they know which camera operation triggered the change when writing the block. The map-level API proposed here needs to provide more context in order for the application to make an informed decision on what to add to the style, especially the layer ID that triggered the event. Additionally, because the API traffics in image IDs rather than image data, the application has to craft appropriate image IDs upfront when styling the layer.

There was concern in #7860 about having to block worker threads on expressions that can only be evaluated on the main thread. But the event proposed here would also have to be called on the main thread, as with any MGLMapViewDelegate method, because map view delegates are typically UI classes.

[eklipse2k8]

1. Is there support image eviction if the cache gets too full?
2. I think a better API would be more notification style, and then it would be up the caller to setImage() when its ready. I think use cases here would allow developers to fetch network images, or generate the image in a background thread.

For 1 maybe there could be a parallel notification to ask if canRemoveImage?

[jaegs]

I think a better API would be more notification style, and then it would be up the caller to setImage() when its ready. I think use cases here would allow developers to fetch network images, or generate the image in a background thread.

@eklipse2k8, @ansis - one thing to consider: what would the map display when it's waiting for the image to be generated asynchronously? And when the image is finally added, will the placement of the features be stable or will they be reshuffled to fit the image?

[asheemmamoowala]

In fact, the iOS precedent cited in mapbox/mapbox-gl-js#7587, -[MGLMapViewDelegate mapView:imageForAnnotation:], requires a return value. I suppose the SDK could make the delegate method look like it’s pulling, by adding the returned image to the style for the application, but it sounds like you’d like to enable asynchronous image loading as part of this feature.

@1ec5 Are you suggesting that the optionality to provide an image in a callback synchronously would be preferable? For the MGLComputedShapeSource the options exists to provide a response synchronously in the callback, but it is not used synchronously when received. The data still has to be processed in a tile worker.

The ImageManager is in a similar situation, where it may not be on the main thread. A synchronous response to the callback would still not appear immediately in the next frame.

[1ec5]

Are you suggesting that the optionality to provide an image in a callback synchronously would be preferable?

The profile picture use case described in mapbox/mapbox-gl-js#7587 implies that the images are not only lazily loaded but also lazily fetched from the network. On the other hand, an asynchronous callback model is error-prone. What happens if the corresponding mbgl method never gets called? This PR currently relies on the developer to call addImage, which doesn’t block any rendering on the callback, but also isn’t particularly discoverable.

Lazy network requests are also poorly suited to animation, which is apparently an intended use case. Imagine the user having to sit through each frame loading on demand in series, only animating smoothly after looping. In practice, I think we can expect that an application is either generating each frame completely programmatically (which can be synchronous) or is receiving the animated image wholesale. An asynchronous, frame-by-frame API for animation begins to sound like a streaming video API, which seems like overkill for icons, at least from the perspective of a developer wanting support for animated image formats (#10595).

For the MGLComputedShapeSource the options exists to provide a response synchronously in the callback, but it is not used synchronously when received. The data still has to be processed in a tile worker.

At least on iOS, the map is guaranteed to try to rerender 60 times a second (unless another frame rate is specified), so that may not be a problem visually. Regardless, if the developer is required to add the image to the style themselves, they’ll expect the model (getImage) to be updated immediately, if not the rendered map, and won’t expect to get another fault for the same image on the next frame. So that would be the main thing to ensure, I guess.

[ansis]

@1ec5 thanks for all the feedback!

The ability to set a paint or layout property to executable code in lambda functions (#7860) would free developers from having to learn a new language just to manipulate the style.

This would be a pretty deep change that has a bunch of potential issues. As you mentioned, threading presents some challenges (also in -js). Crossing the jni boundary could be slow. Deduplication could be tricky. These apply here as well, but on a different scale than supporting lambdas everywhere. I'd say this idea is worth researching though!

. In fact, the iOS precedent cited in mapbox/mapbox-gl-js#7587, -[MGLMapViewDelegate mapView:imageForAnnotation:], requires a return value. I suppose the SDK could make the delegate method look like it’s pulling, by adding the returned image to the style for the application

Yep! the iOS SDK could implement it this way if that is following the precedent. I opted for not doing this in core because of the lack of precedent.

The profile picture use case described in mapbox/mapbox-gl-js#7587 implies that the images are not only lazily loaded but also lazily fetched from the network. On the other hand, an asynchronous callback model is error-prone. What happens if the corresponding mbgl method never gets called? This PR currently relies on the developer to call addImage, which doesn’t block any rendering on the callback, but also isn’t particularly discoverable.

Yep! Those exact tradeoffs were discussed for the -js implementation. It isn't a clear choice which is better but I chose the current approach because lack of discoverability seemed like less of a trap than making your map hang. Your idea about returning an image in the delegate sounds like it could improve the discoverability!

Lazy network requests are also poorly suited to animation, which is apparently an intended use case. Imagine the user having to sit through each frame loading on demand in series, only animating smoothly after looping. In practice, I think we can expect that an application is either generating each frame completely programmatically (which can be synchronous) or is receiving the animated image wholesale.

This pr isn't really aimed at animation. The main use case is generating static icons. But this pr also optimizes updating images with addImage. Previously this triggered tile reparsing and now it just updates the atlas. This could be used to push images before each frame. I'm still planning on porting some other changes from -js that provide a (synchronous) pull interface for frames.

[ansis]

@alexshalamov @asheemmamoowala Thanks for your reviews. I've added another commit 6089031 which fixed some of your suggested changes and also implemented patching for image atlases.

Previously addImage would trigger tile reparsing. Now, if the icons are the same size it just patches the atlas texture which should be much faster.

If you could review it, that would be great!

cc @kkaefer in case you have interest in reviewing this port.

[ansis]

This PR doesn't support asynchronous images but if you know the size of the image you can provide a blank image and then update it later with addImage once you've loaded the real one. The general idea looks something like this:

// in your MapObserver implementation
onStyleImageMissing(const std::string& id) {
    uint32_t width = 10;
    uint32_t height = 10;
    mbgl::PremultipliedImage image({ width, height });
    auto image = std::make_unique<mbgl::style::Image>(id, std::move(image), 1);
    map->getStyle().addImage(std::move(image));

    // after adding the image generate the real one
    loadAndGenerateImage(id).
}

loadAndGenerateImage(const std::string& id) {
     // load all your resources and then call generateImage(...)
     ...
}

generateImage(const std::string& id) {
    uint32_t width = 10;
    uint32_t height = 10;
    mbgl::PremultipliedImage image({ width, height });
    ... // modify `image` with your contents
    // update the image
    map->getStyle().addImage(std::move(image));
}

[asheemmamoowala]

This PR doesn't support asynchronous images but if you know the size of the image you can provide a blank image and then update it later with addImage once you've loaded the real one.

@ansais is this the only way to provide images asynchronously or a performance recommendation?

[kkaefer]

Does this have to be public?

[ansis]

ImageAtlas accesses this directly. Is there a better way?

[kkaefer]

Why is there a done function? What does it do that we can't do just through the mere fact of returning from this function?

[ansis]

I might be wrong here, but: on Android the map and renderer are on different threads. The missing image event originates on the render thread but needs to be called on the ui thread. The observer for RendererFrontend needs to be implemented in a way to calls onto the main thread and then calls the callback back on the render thread. I haven't implemented the changes to the Android platform yet.

[ansis]

Wait, I think you're right. This isn't necessary here

[ansis]

So, I think the RendererFrontend's observer needs to have this callback and since Map::Impl implements the same interface it also needs it

[kkaefer]

As we're accumulating more objects that work together to form an ImageAtlas, maybe it's time to bundle them up together in a follow up PR?

[ansis]

Sounds like a good idea

[pozdnyakov]

Few non-blocking nits