Do not use texSubImage2D if not necessary to improve performance #10383
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 754f735:
|
There was a problem hiding this comment.
There should be a flag in this resource, and default value for it.
@GoodBoyDigital
Regardless, later, we'll need to make immutable textures too - texStorage2D makes things faster for sure
|
Cool. Just for my understanding - what’s the use case or benefit of having
the existing implementation? I didn’t understand why it was that way in the
first place so clearly am missing something.
Also can you advise on the name of the flag, default, etc.
…On Sun, Mar 31, 2024 at 7:04 AM Ivan Popelyshev ***@***.***> wrote:
***@***.**** requested changes on this pull request.
There should be a flag.
Regardless, later, we'll need to make immutable textures too -
texStorage2D makes things faster for sure
—
Reply to this email directly, view it on GitHub
<#10383 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABVW5FNCFQFLD35L2MN6KDY3AJYBAVCNFSM6AAAAABFLZ5AXOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZQGI3DQNBUG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Actually, I dont know :) the change was introduced by @GoodBoyDigital long time ago. |
|
this is an interesting one. texImage2D: This function is used to define a two-dimensional texture image. It can initialize a texture from an array of data or from an HTML element (like img, canvas, video). When you call texImage2D, you're essentially creating a new texture or reinitializing an existing one, which includes allocating memory for the texture. If you are repeatedly calling texImage2D to update a texture with new images or data, it can be less efficient because it involves reallocating texture memory each time it's called. texSubImage2D: This function updates a subset of a two-dimensional texture image, and it can be more efficient for updating textures because it does not require the texture's memory to be reallocated. Instead, it reuses the existing texture's memory allocation. This makes texSubImage2D particularly useful for scenarios where you need to frequently update textures with new data (like streaming video frames into a texture or updating a texture atlas), as it can offer better performance by avoiding unnecessary memory allocations. on paper we should always strive for texSubImage2D.. but seems that might not be entirely true now? |
|
What are those descriptions from? It’s definitely the case, if the list
serv is to be believed, that in chrome at least the fastest path for video
is hit with texImage2D only
…On Tue, Apr 2, 2024 at 11:52 AM Mat Groves ***@***.***> wrote:
this is an interesting one.
*texImage2D:* This function is used to define a two-dimensional texture
image. It can initialize a texture from an array of data or from an HTML
element (like , , or
*texSubImage2D:* This function updates a subset of a two-dimensional
texture image, and it can be more efficient for updating textures because
it does not require the texture's memory to be reallocated. Instead, it
reuses the existing texture's memory allocation. This makes texSubImage2D
particularly useful for scenarios where you need to frequently update
textures with new data (like streaming video frames into a texture or
updating a texture atlas), as it can offer better performance by avoiding
unnecessary memory allocations.
on paper we should always strive for texSubImage2D.. but seems that might
not be entirely true now?
—
Reply to this email directly, view it on GitHub
<#10383 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABVW5C3T2FORWNK3GP54HDY3L47HAVCNFSM6AAAAABFLZ5AXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZSHAYTSNBWGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
i think @ivanpopelyshev has the best idea - lets add a flag to texture source. False by default, but make it true for video textures? @gfodor would you mind updating the PR pls? Thanks man!! |
|
Sure, I can take a spin at that.
…On Wed, Apr 10, 2024 at 12:41 PM Mat Groves ***@***.***> wrote:
i think @ivanpopelyshev <https://github.com/ivanpopelyshev> has the best
idea - lets add a flag to texture source. False by default, but make it
true for video textures?
that way users have the options.
@gfodor <https://github.com/gfodor> would you mind updating the PR pls?
Thanks man!!
—
Reply to this email directly, view it on GitHub
<#10383 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABVW5EIN4CO63YXFUVU4QTY4WIWTAVCNFSM6AAAAABFLZ5AXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBYGMYDONRYGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
following up on this one @gfodor - would be great to have this when you get some time! Thanks in advance |
Description of change
This PR updates the image uploader to use texImage2D instead of texSubImage2D when possible. In testing on my phone I see a significant performance improvement with this patch.
As per this discussion at least some time in the past it was acknowledged this was a problem: https://issues.chromium.org/issues/40469618#comment85
It's unclear if the fast path has been updated appropriately for texSubImage2D after that comment but in any case the performance improvement on my test device is noticable. (CPU utilization of my app went from 120-150% to 100-120%)
Pre-Merge Checklist
npm run lint)npm run test)