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

support direct use of gpu frames such as from webcodecs by library users #29528

Closed
wants to merge 1 commit into from

Conversation

maddanio
Copy link

Description

This introduces a user accessible isGPU flag that avoids pixel storage allocation and forces use of only the TexImage api. Previously this was only conceived for the VideoTexture class of three.js, but it is also desirable for example when using the WebCodecs api and feeding resulting frames to three.js

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 687.15
170.18
687.16
170.2
+10 B
+15 B
WebGPU 805.82
216.97
805.85
216.97
+28 B
+10 B
WebGPU Nodes 805.33
216.83
805.35
216.84
+28 B
+9 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 462.85
111.79
462.84
111.79
-4 B
+5 B
WebGPU 535.59
144.55
535.61
144.56
+14 B
+6 B
WebGPU Nodes 491.75
134.31
491.76
134.31
+14 B
+6 B

@mrdoob
Copy link
Owner

mrdoob commented Oct 11, 2024

Do you mind sharing more details of your use case and how you're using this?

@maddanio
Copy link
Author

I use the webcodecs api to decode video frames and use those as textures in a three.js scene.

@mrdoob
Copy link
Owner

mrdoob commented Oct 12, 2024

Can you share some example code?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 3, 2025

We want to move away from the texImage2D() API since it has conceptual flaws which cause performance and robustness issues. This has to do with the topic "Texture Completeness". There are some OpenGL/WebGL online resources you can read for more details.

The engine has kept the usage of texImage2D() in context of video texture because of a browser bug: https://issues.chromium.org/issues/40915685

However, it is not recommended to use texImage2D() for any other use cases than video textures right now.

@Mugen87 Mugen87 closed this Jan 3, 2025
@Mugen87 Mugen87 added this to the r173 milestone Jan 3, 2025
@maddanio
Copy link
Author

maddanio commented Jan 3, 2025

This would be extremely cumbersome tbh, we really want to just directly use the webcodec images

@maddanio
Copy link
Author

maddanio commented Jan 3, 2025

And these are effectively video textures, but with more control. If you block this you effectively make threejs incompatible with webcodecs (minus cumbersome workarounds). All this while the required change is very minimal

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 3, 2025

@maddanio for us to consider making changes to support webcodecs (whether these proposed changes, or others), I think we would need to have example code as requested in #29528 (comment). Otherwise we don't know how to test this and cannot safely maintain it. Keep in mind that while your PR is indeed minimal, you are also implicitly asking three.js maintainers to maintain support for this flag indefinitely. We will have respond when users ask why color management breaks when the isGPU flag is enabled, for example.

@maddanio
Copy link
Author

maddanio commented Jan 5, 2025

Ok, I just checked again, and there is a very significant performance boost in using images from the webcodecs api as texture with my patch, so it is extremely relevant for us. In general, is (good) webcodecs not something that is desired by three.js? I mean, you already have VideoTexture, and now there is a better api that allows way cooler applications (streaming, live stitching, inter-stream sync etc). I fully understand wanting an example and will make one today or tomorrow.
If there is woes about api do you have a better suggestion? maybe provide a contrete texture subclass for the webcodecs use case? whatever breaks on this direct path already breaks for video texture anyhow, so I am not sure that is a solid rejection reason?

@maddanio
Copy link
Author

maddanio commented Jan 5, 2025

ah, actually this would be for canvas texture I guess, because it always has this problem due to the bug.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 5, 2025

Could you make it somehow work with if you derive a class from VideoTexture?

@maddanio
Copy link
Author

maddanio commented Jan 5, 2025

ok, I added an example that showcases the performance. you can switch by setting isGpu to true/false. results depend on bowser. on my old machine there is maybe a 20% difference in chrome but a 200% difference in safari.

@maddanio
Copy link
Author

maddanio commented Jan 5, 2025

I will look into the subclassing idea

@maddanio
Copy link
Author

maddanio commented Jan 5, 2025

hmm, actually, yes, it feels rather hacky, but just using videotexture would actiually work. I still think it should be clearer/more documented

@maddanio
Copy link
Author

maddanio commented Jan 5, 2025

yes, using videotexture works, feels quite hacky, but works. so guess we can bury this pr. still think it would make sense to add something documenting the fact. should I make a pr for that?

@maddanio
Copy link
Author

maddanio commented Jan 5, 2025

I could also make a VideoFrameTexture that has a nicer interface and also effectively serves as documentation

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 5, 2025

I could also make a VideoFrameTexture that has a nicer interface and also effectively serves as documentation

I'm curious how the API would look like. Could you sketch a PR for that? That would help us to better understand your use case.

@maddanio
Copy link
Author

maddanio commented Jan 6, 2025

I just changed the commits on my branch, so you can see it directly here now. also added some documentation

@maddanio
Copy link
Author

maddanio commented Jan 6, 2025

hmm, this pr didnt update...will make a new one then :)

@maddanio
Copy link
Author

maddanio commented Jan 6, 2025

#30269

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.

4 participants