Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for animated images #7999

Merged
merged 5 commits into from
Mar 12, 2019
Merged

add support for animated images #7999

merged 5 commits into from
Mar 12, 2019

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Mar 6, 2019

This adds support for updating and animating images. fixes #7588 and fixes #4804

There are two parts to this:

StyleImageInterface

export type StyleImageInterface = {
    width: number,
    height: number,
    data: Uint8Array | Uint8ClampedArray,
    render?: () => void,
    onAdd?: (map: Map, id: string) => void,
    onRemove?: () => void
};

map.addImage(...) now accepts images that implement the StyleImageInterface. This interface adds three new and optional methods:

  • render gets called once for every map frame that the icon is used in the visible map. The user can update the icon in this method to animate the icon.
  • onAdd gets called when the icon gets added to the map to allow for resource initialization, etc.
  • onRemove mirrors onAdd and will be useful if/when we add image eviction

This pull-based approach to image updating allows the user to only rerender the image when it is necessary. The image is not rerendered for cached tiles. Image renders are synchronized with map renders.

map.updateImage(id, image)

This updates and image and immediately updates map. This is a lighter alternative to the above render approach and should be useful for instant, infrequent updates. For example, changing an icon on click. Or changing an icon when a resource has loaded.

Both approaches use the same underlying implementation.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

Add support for a `StyleImageInterface` that allows images to be
rerendered with a `render` method.

Add a `map.updateImage(...)` method that can be used to do
one-off updates to images.
@ansis ansis requested review from mourner and ryanhamley March 6, 2019 20:49
src/render/image_atlas.js Outdated Show resolved Hide resolved
src/render/image_atlas.js Outdated Show resolved Hide resolved
@ansis ansis requested a review from kkaefer March 7, 2019 16:08
src/render/image_manager.js Show resolved Hide resolved
src/style/style_image.js Show resolved Hide resolved
src/render/image_manager.js Show resolved Hide resolved
} else {
gl.texSubImage2D(gl.TEXTURE_2D, 0, 0, 0, width, height, gl.RGBA, gl.UNSIGNED_BYTE, image.data);
gl.texSubImage2D(gl.TEXTURE_2D, 0, x, y, width, height, gl.RGBA, gl.UNSIGNED_BYTE, image.data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this behave when we have a lot of icons in the atlas? Is there a threshold where it's more efficient to update the entire texture rather than patching it? I remember reading somewhere (can't find the reference now) that a glTexSubImage2D causes a copy of the underlying texture object (to avoid race conditions with render calls that are in flight), which then gets patched. This means that multiple glTexSubImage2D calls would repeatedly copy and patch rather than have one swap operation.

We already coalesce all icon updates before issuing GL calls, but then trigger upload calls for every single icon rather than coalescing them. Not saying that we need to change this now, but it could be a potential performance bottleneck.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to know!

I'm going to merge this as is but we should keep an eye on the performance this. Profiling this with a low number of images seemed to work ok. It's likely that for larger numbers of images your approach could work better. I initially implemented this by copying the images into the image and then calling texSubImage2D once per texture. The cost of the manual copies was measurable. It's very possible that by switching to the current approach I just made the cost more hidden.

Hopefully if this API turns out to be useful we can optimize based on users' use cases.

One of my motivations for patching images individually is that it lets you supply dom elements as the input. We aren't using this right now but I'm hoping to extend image support to include HTMLVideoElement. In a prototype, updating a image by going the canvas drawImage --> canvas getImageData --> texture update was much slower and this approach avoids that. Implementing both approaches shouldn't add more complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add updateImage API, or allow addImage to update updatable and animatable images
4 participants