Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add support for animated images #7999
Changes from all commits
473789b
2bc2eb6
92b19fa
740518f
719a245
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 multipleglTexSubImage2D
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.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.
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 thecanvas drawImage --> canvas getImageData --> texture update
was much slower and this approach avoids that. Implementing both approaches shouldn't add more complexity.