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

Define importing VideoFrame into WebGPU #412

Closed
wants to merge 3 commits into from

Conversation

kainino0x
Copy link

@kainino0x kainino0x commented Nov 25, 2021

The WebGPU community group is so far unable to reach consensus on adding support for WebCodecs interop, because WebCodecs is not agreed upon by the community group members. It has been tentatively proposed to define WebGPU-WebCodecs interop in WebCodecs instead.

Draft for discussion.

Issue: gpuweb/gpuweb#2124


Preview | Diff

@kainino0x
Copy link
Author

Tangentially related, an open question came up a while ago when trying to decide how applications would actually use VideoFrame with WebGPU:
gpuweb/gpuweb#1380 (comment)

@kainino0x kainino0x force-pushed the webgpu-importExternalTexture branch from 763a1c2 to e9ba23b Compare November 25, 2021 05:09
@kainino0x
Copy link
Author

It's also likely we would want to later be able to import individual VideoFrame planes into WebGPU. This doesn't tackle that yet. See gpuweb/gpuweb#1380.

index.src.html Outdated Show resolved Hide resolved
Copy link
Contributor

@sandersdan sandersdan left a comment

Choose a reason for hiding this comment

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

I'm satisfied that this design is compatible with WebCodec's lifetime/ownership model.

@kainino0x kainino0x marked this pull request as ready for review December 2, 2021 02:07
@kainino0x
Copy link
Author

I've taken this PR out of Draft since the WebGPU group has agreed on hosting this interaction in WebCodecs. Members reiterated that trying to define these interactions in the WebGPU spec was unpalatable because (unlike the rest of WebGPU) there would be no alternate implementations available to validate the design. (Lifetime design in particular - note specific concerns about wired memory on Apple platform(s).)

@kainino0x
Copy link
Author

kainino0x commented Dec 2, 2021

(This is ready to review, and land whenever WebCodecs editors are happy with it!)

@chcunningham
Copy link
Collaborator

Sorry for the delay. I'll put it on the agenda for tomorrow's call.

@chcunningham chcunningham self-requested a review December 8, 2021 06:19
Copy link
Collaborator

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

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

Generally looks good to me.

(Lifetime design in particular - note specific concerns about wired memory on Apple platform(s).)

Do any concerns remain after considering Dale's points in gpuweb/gpuweb#2124 (comment)

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
@kainino0x
Copy link
Author

Any tips for building this locally? I'm using bikeshed 3.3.2.

LINK ERROR: Multiple possible 'hidden' idl refs for '['Document']'.
Arbitrarily chose https://html.spec.whatwg.org/multipage/interaction.html#dom-document-hidden
To auto-select one of the following refs, insert one of these lines into a <pre class=link-defaults> block:
spec:html; type:attribute; text:hidden
spec:page-visibility-2; type:attribute; text:hidden
{{Document/hidden}}
 ✘  Did not generate, due to fatal errors

@kainino0x
Copy link
Author

Never mind, of course as soon as I posted that I realized I could just manually invoke bikeshed (rather than using the makefile) without --die-on=warning.

@kainino0x
Copy link
Author

... hm, that didn't catch my videoframe-transfer-serialization error though. How did you catch it?

@kainino0x
Copy link
Author

FYI I verified the fixes above by building with #419 included and checking the generated HTML output. Marked comments as addressed (and I think this should be OK to land).

Do any concerns remain after considering Dale's points in gpuweb/gpuweb#2124 (comment)

I don't think so. WebGPU doesn't add any more ways to hold onto video frames than WebCodecs does by itself.

@chcunningham
Copy link
Collaborator

Fixes LGTM. @padenot, @aboba did you want additional time to review this?

... hm, that didn't catch my videoframe-transfer-serialization error though. How did you catch it?

I think just manually by looking through the generated preview.

@Kangz
Copy link

Kangz commented Jan 12, 2022

All, is there anything blocking forward progress on this PR? Are we waiting for a review from @padenot and @aboba?

@padenot
Copy link
Collaborator

padenot commented Jan 12, 2022

Conceptually this probably looks alright (from a Web Codecs perspective, after reading the various discussions in WebGPU issues), but it isn't clear to me why this change is in the Web Codecs specification. Monkey patching specifications is a bad practice, and we shouldn't do it. This PR changes the semantics of a dict member and a method, both defined in WebGPU, and doesn't change anything about Web Codecs, it merely uses an object that can be created using an API defined in the Web Codecs specification.

If the extension points needed for specifying the intended behavior of VideoFrames when used in conjunction with GPUDevice.importExternalTexture(...) are lacking, then we can add them here for sure, but it looks fine: [[Detached]] can be checked in WebGPU to know whether the VideoFrame is still valid.

@Kangz
Copy link

Kangz commented Jan 12, 2022

My understanding (and @kainino0x can expand) is that there are two reasons why this needs a change in the WebCodec specification:

  • It seems more natural that the destruction of the GPUExternalTexture is done immediately when the VideoFrame is closed, instead of polling in the WebGPU specification the status of [[Detached]] when the GPUExternalTexture is used.
  • There has been concerns by some browser vendors (Mozilla, Apple) about adding references to WebCodec's VideoFrame in the WebGPU specification because they want to implement WebGPU but are less clear about WebCodec. The interactions between the two APIs needs to be defined somewhere, so this PR suggests to define it in WebCodec since it is very minimal.

@padenot
Copy link
Collaborator

padenot commented Jan 12, 2022

* It seems more natural that the destruction of the `GPUExternalTexture` is done immediately when the `VideoFrame` is closed, instead of polling in the WebGPU specification the status of `[[Detached]]` when the `GPUExternalTexture` is used.

I understand this, but monkey patching is not ok. Would it be OK to state that GPUExternalTexture.[[destroyed]]'s getter returns the value of the underlying VideoFrame.[[Detached]] (plus any other bits of behaviour of WebGPU of course, and only when it's backed by a VideoFrame's underlying resource). Doing this, there's no polling, and WebGPU probably knows what to do with a [[destroyed]] GPUExternalTexture already.

The alternative is to have a way to signal to the GPUExternalTexture that the underlying resource has been destroyed, but it feels less elegant than the lazy approach described above.

* There has been concerns by some browser vendors (Mozilla, Apple) about adding references to WebCodec's `VideoFrame` in the WebGPU specification because they want to implement WebGPU but are less clear about WebCodec. The interactions between the two APIs needs to be defined somewhere, so this PR suggests to define it in WebCodec since it is very minimal.

It's fine not to implement a specification in its entirety, it's fairly common. That said, speaking for Mozilla, we're definitely implementing Web Codecs, so there's no concern from us here.

At the end of the day, this modifies the behaviour of the WebGPU API, it needs to go into the WebGPU spec.

@aboba
Copy link
Collaborator

aboba commented Jan 12, 2022

Agree with Paul that WebCodecs is probably not the best place to land this.

This seems related to the all-GPU pipeline discussion in Issue webmachinelearning/webnn#226
Note the discusson about whether existing ML implementations can make use of GPUExternalTexture

@kainino0x
Copy link
Author

Thank you all, I'll bring this feedback back to WebGPU and see where it goes. gpuweb/gpuweb#2124 (comment)

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.

6 participants