-
Notifications
You must be signed in to change notification settings - Fork 350
Extend lifetime of GPUExternalTexture imported from video element #2302
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
Conversation
Kangz
left a comment
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.
LGTM but don't we need something on the GPUExternalTexture to know if it is still alive?
|
You're right, I was cranking out spec PRs and totally forgot about that! Will mark as draft until that's added. |
|
LGTM as well! (Minor added bonus to this approach I haven't seen anybody comment on: It could reduce BindGroup churn.) As for notifying developers that the texture is still alive, seems like a promise would be appropriate? function updateVideoTexture() {
videoTexture = gpuDevice.importExternalTexture({ source: video });
videoTexture.expired.then(updateVideoTexture);
} |
This would be very similar to requestVideoFrameCallback (rVFC) but phrased as a promise instead. However since it's tied to the end of the frame lifetime instead of the beginning, it could lead to developers to re-importing the video when the previous frame expires rather than when they need it, which could result in <1 animation frame of latency when the video advances between the beginning and end of the animation frame, because video frame advancement is totally desynchronized with the animation loop AFAIU. E.g. with both video and animation updating at the same rate, say 60Hz: externalTexture.expired.then where expiry happens AFTER rAF callbacks:
rVFC:
That said, working through this exercise made me realize that a synchronous isExpired that changes after rAF callbacks is even worse: externalTexture.isExpired where it changes AFTER rAF callbacks:
In conclusion, I need to think about this some more. |
|
TIL requestVideoFrameCallback callbacks (as drafted) are called in "update the rendering" immediately before rAF callbacks.
|
|
Imitating the lifetimes in rVFC would fix it: externalTexture.isExpired where it changes BEFORE rAF (before or after rVFC?):
Though we might still want to guarantee that an imported frame lasts at least until the next end-of-rAF in case it was imported in some other async code. (This doesn't affect this case study.) |
Original PR: gpuweb#1666 Discussion: gpuweb#2124
fce719a to
9a4074b
Compare
9a4074b to
52baf82
Compare
|
Updated description with latest changes. |
|
Just to make sure I completely understand #2302 (comment), see is what would happen? (it sounds good)
|
|
Yup that looks correct. Technically the video element can advance at any time (I think). I tried to diagram this out yesterday but the resulting diagram was kind of incomprehensible. The rules as written in the spec make more sense. (Just before rVFC/rAF, textures expire if they're now outdated.) |
|
Editors to decide exactly what input we need from the group, or specific individuals. |
|
I'm putting this on the tacit resolution queue for next meeting (although I'll be OOO). Here's a summary for the meeting: The current spec says imported video textures are closed very quickly (in a microtask after the current task). This leads to developers needing to reimport frequently, which makes them feel they should copy out the data instead of reimporting. Additionally, browsers may need a cache for reimport performance. This PR extends the lifetime until the frame is not current on the source video anymore. For predictability, expiry always happens at a very specific point - within a frame after the video advances, the lifetime ends immediately before requestVideoFrameCallback callbacks, which are just before requestAnimationFrame callbacks. It also adds a boolean attribute |
9c78cf1 to
a2d998f
Compare
) SHA: 3b3e617 Reason: push, by @kainino0x Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
) SHA: 3b3e617 Reason: push, by @kainino0x Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
) SHA: 3b3e617 Reason: push, by @kainino0x Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
WebGPU meeting minutes 2022-02-16
|
|
Removing 'needs-cts-issue' for now because this is obsolete and will be updated in a future pull request to fix #3324. |
Original PR: #1666
Discussion: #2124
The change proposed here destroys "stale" textures (those which no longer represent the video's current frame) just before rAF callbacks (and before rVFC callbacks, which occur before rAF callbacks, if applicable, because once rVFC fires there's no need for the old texture). It exposes a readonly attribute "expired" which the application can check to know if it should re-import.
Notably, expiry occurs even if the texture was imported just prior to rVFC/rAF. This means it's possible to import on an event (say, "ontimeupdate") and have the texture become invalid before getting a chance to use it. I think this is OK because (1) applications should be importing later, inside their rendering task (rAF or rVFC callback), and (2) the strict timing requirements should mean developers find any such errors immediately.
Alternatively there could be a provision that keeps such textures alive a bit longer, but this could result in applications accidentally introducing an extra (video) frame of latency unnecessarily.
Preview | Diff
Preview | Diff