GPUExternalTexture: API for video; WGSL type and functions #1666

kainino0x · 2021-04-23T02:40:17Z

Dependent on #1689 and #1690, see last commit only

Canvas support to follow (it has more complicated semantics).

For video external texture lifetimes, I took the most restrictive option so we can easily change it later.

I also excluded the textureSampleLuminance/textureLoadLuminance functions as they're quite difficult to define: see #1681

Based on #1415 (comment).
Fixes #1415.

Preview | Diff

Kangz

LGTM!

wgsl/index.bs

spec/index.bs

Kangz · 2021-04-26T13:29:41Z

spec/index.bs

+            1. [=Queue a microtask=] to set |result|.{{GPUExternalTexture/[[destroyed]]}} to `true`,
+                releasing the underlying resource.
+
+                Issue: Is this too restrictive?


Maybe? We should probably discuss it in the group as a follow-up. An option would be to add a retained: boolean to the descriptor that would make that micro-task not enqueued but could have additional cost (like an extra copy to decouple the lifetime of the texture).

dneto0

I don't know enough about this subject, but looks good overall.
I have minor feedback.

wgsl/index.bs

github-actions · 2021-04-27T03:07:36Z

Previews, as seen when this build job started (69a4d3e):
WebGPU | IDL
WGSL
Explainer

github-actions · 2021-04-27T03:24:25Z

Previews, as seen when this build job started (a3400fb):
WebGPU | IDL
WGSL
Explainer

kainino0x · 2021-04-27T03:47:44Z

spec/index.bs

+The provided `sampler` is used to sample any underlying texture(s).
+If there is one RGBA underlying texture, this behaves as usual.
+If there are several underlying textures (Y+U+V or Y+UV), the sampler is used to sample each
+underlying texture separately, prior to conversion from YUV to RGBA or luminance.


We previously discussed leaving things open to allow implementations to still filter after conversion (if they wanted to), but the API surface in this PR doesn't allow it: the sampling parameters are baked into the sampler object, so they can't be passed in through the external texture's metadata UBO. Hence I've written the result down here concretely.

But if we DO want post-conversion filtering to be possible with 0-copy YUV sources, we should change the API to allow this somehow.

I think the post-conversion filtering can still be done with sampler with the same amount of work as when you don't have a sampler:

Take the fractional and integer parts of the texture coordinates (after multiplication by the texture size)

Sample the square starting at integer part with the sampler

Mix using the fractional part.

But how do you know whether to use linear or nearest filtering? I don't think the shader can introspect that info from a sampler object.

Ah yeah that makes sense. If we wanted to allow that we would need to add the sampler to extrenal_texture creation.

kainino0x · 2021-04-27T21:02:29Z

I have removed the luminance overloads in this PR, to be followed up on.

Items that need to be raised in the WGSL group (can be discussed in office hours and then written down in this proposal for review):

Should texture_external define itself in terms of SPIR-V as three textures and one UBO?
External textures are never mipmapped.
- Can we lift the restriction that they can only be used in fragment shaders?
- If so, should we use a different name than textureSample? Or simply have textureSample(texture_external) be special in that it can be used in any stage?

kainino0x · 2021-04-28T00:52:21Z

Apologies for the churn (force push), I wanted to detangle the various changes (and merge-from-main commit). After some work on the color spaces, this is ready again. There will be a followup to resolve some Issues about color conversions elsewhere in the spec.

kainino0x · 2021-04-28T18:07:45Z

From the office hour:

keep the definition of texture_external handwavy at least for now, adding the spir-v doesn't really help.
make sure it's possible for implementations to pretend a texture_external takes only one binding point even though it's actually up to 3 (or 4?).
yes, it can be used outside fragment shaders; use textureSampleLevel for sampling texture_external, but omit the level argument (it's implicitly 0).
note from @jdashg: May need to reserve one more texture slot for a 3d texture LUT (possibly faster than exponents and matrices).

kainino0x · 2021-04-28T18:11:51Z

It occurs to me that one extra global sampler would be needed to sample a LUT with appropriate filtering.

kainino0x · 2021-04-28T23:29:26Z

I've split out some other changes. This PR is now just for the last commit.

spec/index.bs

kainino0x · 2021-05-03T19:50:20Z

Resolution:

We do not need to have an allowance in the API for color-correct (post-conversion) filtering. Evidence points toward very few people caring about this. (Browser compositors, for example, do this "wrong" today.)
- Users can get correct results by using textureLoad or non-filtering samplers, and do filtering themselves (though possibly not quite as efficiently as the implementation can?)
- Can add this in a future WebCodecs import path if there is demand, because the users who need this probably also need WebCodecs.

kvark · 2021-05-04T19:32:30Z

spec/index.bs

-        for sampling 3D LUTs.
-    - for *each* external texture, one sampled texture binding for each plane,
-        one sampled texture binding for a 3D LUT, and one uniform buffer binding for metadata.
+    - three sampled texture bindings (for up to 3 planes),


I wonder if it would be easier to just give it the whole bind group

It doesn't help for the limit because they are for the whole pipeline layout.

Yes, but I wonder how the discussion goes about bind group re-creation. If we force users to re-create bind groups, we might as well just accept GPUExternalTexture instead of a bind group.

re-create bind groups when, for different external textures? I was imagining we would always use an exact number of bindings in both the bind group layout and the bind group regardless of the underlying representation, and bind dummy objects in any unused slots.

re-create every frame, according to #1666 (comment)

I don't think there's a good reason to use up, by design, an entire bind group slot for one external texture. I think there are reasonable applications that would need to bind two or more external textures (e.g. compositing a foreground canvas over a background webcam feed) but not want to use up two of their four bind groups.

spec/index.bs

wgsl/index.bs

kainino0x · 2021-05-06T18:08:28Z

@dneto0 @kvark I think this is ready to land as soon as #1690 lands - unless we want to block it on #1380 (which I don't). Could you please re-review?

kvark

This PR looks good to me, although the whole approach still feels temporary if we get WebCodecs later on.

github-actions · 2021-05-11T01:08:23Z

Previews, as seen when this build job started (05e5fb9):
WebGPU | IDL
WGSL
Explainer

github-actions · 2021-05-11T01:09:34Z

Previews, as seen when this build job started (53df64b):
WebGPU | IDL
WGSL
Explainer

dneto0

Approving the WGSL change.
The question in my mind is on the API side: if an external texture uses up multiple bindings, then how do I know what binding numbers are occupied? I presume they are in the same bindgroup.

kainino0x · 2021-05-11T01:57:36Z

I'm not precisely sure how to address this, but I think that in implementations we can hide the fact that multiple binding indices are used. IIRC in Dawn at least, we re-pack the bindings into a small range near 0 anyway, so this problem would disappear in that. However for the sake of alternate implementations we might want to reserve half of the binding index space for internal bindings, or something?

kainino0x requested review from RafaelCintron, ben-clayton, bjjones, dneto0 and kdashg April 23, 2021 02:40

Kangz approved these changes Apr 26, 2021

View reviewed changes

dneto0 requested changes Apr 26, 2021

View reviewed changes

wgsl/index.bs Outdated Show resolved Hide resolved

wgsl/index.bs Outdated Show resolved Hide resolved

kainino0x commented Apr 27, 2021

View reviewed changes

kainino0x force-pushed the external-texture branch 2 times, most recently from 502b1cc to 318f11d Compare April 27, 2021 21:23

kainino0x marked this pull request as draft April 27, 2021 21:48

kainino0x force-pushed the external-texture branch from 318f11d to b36cafb Compare April 27, 2021 22:08

kainino0x mentioned this pull request Apr 28, 2021

Add external_texture luminance functions #1681

Closed

kainino0x force-pushed the external-texture branch 2 times, most recently from 13adfee to a2cee13 Compare April 28, 2021 00:38

kainino0x marked this pull request as ready for review April 28, 2021 00:39

kainino0x force-pushed the external-texture branch from a2cee13 to d0dd2e4 Compare April 28, 2021 00:49

kainino0x mentioned this pull request Apr 28, 2021

copyToTexture for color-managed image sources #1483

Closed

kainino0x marked this pull request as draft April 28, 2021 18:37

kainino0x force-pushed the external-texture branch 4 times, most recently from 403d86d to b1e33ac Compare April 28, 2021 23:25

kainino0x marked this pull request as ready for review April 28, 2021 23:29

kainino0x force-pushed the external-texture branch from b1e33ac to 4858a00 Compare April 28, 2021 23:29

Kangz approved these changes Apr 29, 2021

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kvark reviewed May 4, 2021

View reviewed changes

spec/index.bs Show resolved Hide resolved

shaoboyan reviewed May 5, 2021

View reviewed changes

wgsl/index.bs Outdated Show resolved Hide resolved

kainino0x added 3 commits May 6, 2021 10:54

Add initial color spaces

c3097f2

GPUExternalTexture for video

e0e97ac

remove textureSampleLevel with offset

762d1e0

kainino0x force-pushed the external-texture branch from e1c6271 to 762d1e0 Compare May 6, 2021 18:03

kvark approved these changes May 6, 2021

View reviewed changes

kvark mentioned this pull request May 6, 2021

Investigation: Import VideoFrame from WebCodec to WebGPU #1380

Closed

Merge branch 'main' into external-texture

53df64b

kainino0x force-pushed the external-texture branch from 05e5fb9 to 53df64b Compare May 11, 2021 01:07

kainino0x enabled auto-merge (squash) May 11, 2021 01:09

kainino0x merged commit 118db57 into gpuweb:main May 11, 2021

dneto0 approved these changes May 11, 2021

View reviewed changes

kainino0x deleted the external-texture branch May 11, 2021 01:53

kainino0x mentioned this pull request May 11, 2021

What binding numbers are used by an external texture binding? #1721

Closed

kainino0x mentioned this pull request Nov 13, 2021

Extend lifetime of GPUExternalTexture imported from video element #2302

Merged

kainino0x mentioned this pull request Jun 8, 2022

Specify that texture_external sampling functions clamp coords to [0, 1] #2766

Closed

GPUExternalTexture: API for video; WGSL type and functions #1666

GPUExternalTexture: API for video; WGSL type and functions #1666

Uh oh!

Conversation

kainino0x commented Apr 23, 2021 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependent on #1689 and #1690, see last commit only

Uh oh!

Kangz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dneto0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 27, 2021

Uh oh!

github-actions bot commented Apr 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Apr 27, 2021

Uh oh!

kainino0x commented Apr 28, 2021

Uh oh!

kainino0x commented Apr 28, 2021

Uh oh!

kainino0x commented Apr 28, 2021

Uh oh!

kainino0x commented Apr 28, 2021

Uh oh!

Uh oh!

kainino0x commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kainino0x commented May 6, 2021

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 11, 2021

Uh oh!

github-actions bot commented May 11, 2021

Uh oh!

dneto0 left a comment

Choose a reason for hiding this comment

Uh oh!

kainino0x commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kainino0x commented Apr 23, 2021 •

edited by pr-preview bot

Loading

kainino0x commented May 3, 2021 •

edited

Loading