Skip to content

Conversation

@kainino0x
Copy link
Contributor

@kainino0x kainino0x commented Nov 13, 2021

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

Copy link
Contributor

@Kangz Kangz left a 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?

@kainino0x
Copy link
Contributor Author

You're right, I was cranking out spec PRs and totally forgot about that! Will mark as draft until that's added.

@kainino0x kainino0x marked this pull request as draft November 16, 2021 02:44
@toji
Copy link
Member

toji commented Nov 17, 2021

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);
}

@kainino0x
Copy link
Contributor Author

kainino0x commented Nov 23, 2021

As for notifying developers that the texture is still alive, seems like a promise would be appropriate?

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:

  • Video element advances to frame 2
  • rAF callback, app renders frame 1 <- outdated render
  • Expire imported frame 1
  • Page renders
  • expired.then callback imports frame 2 <- doesn't run until now because "update the rendering" was running
  • Video element advances to frame 3
  • rAF callback renders frame 2 <- outdated render

rVFC:

  • Video element advances to frame 2
  • rVFC callback imports frame 2
  • rAF callback renders frame 2 <- up-to-date render
  • Expire imported frame 1
  • Page renders
  • Video element advances to frame 3
  • rVFC callback imports frame 3
  • rAF callback renders frame 3 <- up-to-date render

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:

  • Video element advances to frame 2
  • rAF callback, app checks if frame 1 is still valid; it is, so it renders frame 1 <- outdated render
  • Expire imported frame 1
  • Page renders
  • Video element advances to frame 3
  • rAF callback, app checks if frame 1 is still valid; it isn't, so imports and renders frame 3 <- up-to-date render
  • Expire imported frame 2
  • Page renders
  • Video element advances to frame 4
  • rAF callback, app checks if frame 3 is still valid; it is, so it renders frame 3 <- outdated render

In conclusion, I need to think about this some more.

@kainino0x
Copy link
Contributor Author

kainino0x commented Nov 23, 2021

TIL requestVideoFrameCallback callbacks (as drafted) are called in "update the rendering" immediately before rAF callbacks.

Note: The effective rate at which callbacks are run is the lesser rate between the video’s rate and the browser’s rate. When the video rate is lower than the browser rate, the callbacks' rate is limited by the frequency at which new frames are presented. When the video rate is greater than the browser rate, the callbacks' rate is limited by the frequency of the update the rendering steps. This means, a 25fps video playing in a browser that paints at 60Hz would fire callbacks at 25Hz; a 120fps video in that same 60Hz browser would fire callbacks at 60Hz.

https://wicg.github.io/video-rvfc/#video-rvfc-procedures

@kainino0x
Copy link
Contributor Author

kainino0x commented Nov 23, 2021

Imitating the lifetimes in rVFC would fix it:

externalTexture.isExpired where it changes BEFORE rAF (before or after rVFC?):

  • Video element advances to frame 2
  • Expire imported frame 1
  • rAF callback, app checks if frame 1 is still valid; it isn't, so imports and renders frame 2 <- up-to-date render
  • Page renders
  • Video element advances to frame 3
  • Expire imported frame 2
  • rAF callback, app checks if frame 2 is still valid; it isn't, so imports and renders frame 3 <- up-to-date render
  • Page renders

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.)

@kainino0x kainino0x force-pushed the longer-video-lifetime branch from fce719a to 9a4074b Compare November 24, 2021 03:00
@github-actions
Copy link
Contributor

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

@kainino0x kainino0x force-pushed the longer-video-lifetime branch from 9a4074b to 52baf82 Compare November 24, 2021 03:15
@kainino0x kainino0x requested a review from Kangz November 24, 2021 03:17
@kainino0x kainino0x marked this pull request as ready for review November 24, 2021 03:18
@kainino0x
Copy link
Contributor Author

Updated description with latest changes.

@github-actions
Copy link
Contributor

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

@Kangz
Copy link
Contributor

Kangz commented Nov 24, 2021

Just to make sure I completely understand #2302 (comment), see is what would happen? (it sounds good)

  • Video element advances to frame 2
  • Update the rendering
    • Expire imported frame 1
    • rAF callback, app checks if frame 1 is still valid; it isn't, so imports and renders frame 2 <- up-to-date render
    • Page renders
  • Video element advances to frame 3
  • Update the rendering
    • Expire imported frame 2
    • rAF callback, app checks if frame 2 is still valid; it isn't, so imports and renders frame 3 <- up-to-date render
    • Page renders

@kvark kvark requested a review from kdashg November 24, 2021 15:27
@kainino0x
Copy link
Contributor Author

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.)

@kainino0x kainino0x linked an issue Dec 2, 2021 that may be closed by this pull request
@kainino0x kainino0x added this to the V1.0 milestone Dec 4, 2021
@kainino0x
Copy link
Contributor Author

Editors to decide exactly what input we need from the group, or specific individuals.

@kainino0x
Copy link
Contributor Author

kainino0x commented Jan 29, 2022

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 GPUExternalTexture.expired which tells the app to reimport to get a new frame. Apps use this if WebGPU rendering isn't driven by requestVideoFrameCallback already. (Could be approximated by observing requestVideoFrameCallback, but that's a bit indirect, slightly imprecise, and not in all browsers. This is direct and easy to use.)

@kainino0x kainino0x added tacit resolution queue Editors have agreed and intend to land if no feedback is given and removed for webgpu editors meeting labels Jan 29, 2022
@kainino0x kainino0x self-assigned this Feb 16, 2022
@kainino0x kainino0x force-pushed the longer-video-lifetime branch from 9c78cf1 to a2d998f Compare February 18, 2022 00:09
@kainino0x kainino0x enabled auto-merge (squash) February 18, 2022 00:10
@kainino0x kainino0x removed the tacit resolution queue Editors have agreed and intend to land if no feedback is given label Feb 18, 2022
@kainino0x kainino0x merged commit 3b3e617 into gpuweb:main Feb 18, 2022
@kainino0x kainino0x deleted the longer-video-lifetime branch February 18, 2022 00:15
github-actions bot added a commit that referenced this pull request Feb 18, 2022
)

SHA: 3b3e617
Reason: push, by @kainino0x

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions
Copy link
Contributor

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

github-actions bot added a commit that referenced this pull request Feb 18, 2022
)

SHA: 3b3e617
Reason: push, by @kainino0x

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Feb 18, 2022
)

SHA: 3b3e617
Reason: push, by @kainino0x

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@kdashg
Copy link
Contributor

kdashg commented Feb 23, 2022

WebGPU meeting minutes 2022-02-16
  • Extend lifetime of GPUExternalTexture imported from video element #2302
    • KN: Importing external textures from video elements
    • Conservatively spec'ed initially. Textures would expire quickly.
    • Devs need to re-import the same frame multiple times. Looks expensive, even though we'd cache inside the browser.
    • Extends the lifetime in a well-defined way - until frame's no longer current.
    • Expires texture at very specific time in frame's lifetime.
    • Think this is a fairly safe change, and better constraint than what we had previously. Should work better for programming against.
    • Does add a boolean attribute to GPUExternalTexture saying when it's expired. Transitions at a well-defined point.
    • KG: this is at some microtask boundary?
    • KN: yes. Right before requestAnimationFrame, or if you have requestVideoFrameCallback - it's right before that.
    • MM: if I import a video frame, and next line does the same thing - guaranteed to get the same frame?
    • KN: I don't think so. Not 100% sure. Videos behave asynchronously, change behind the scenes.
    • MM: what happens in WebGL if you texImage2D twice in a row?
    • KG / KN: not guaranteed to get the same frame. Same guarantees here.
    • KG: if you had 1000 fps video for example you could theoretically get two different frames.
    • BJ: "expires" flag and requestVideoFrame loop aren't necessarily ticking for each frame in this case.
    • KN: if you imported twice you could get 2 different objects that expire at 2 different times.
    • MM: memory use would be proportional to frame rate of video if you imported a bunch of times.
    • KG: potentially.
    • BJ: could you observe in a lightweight way that you did get a different object back? Guaranteed to get same JS object back if frame didn't change?
    • KN: pretty sure you get a new JS object.
    • BJ: imagining blog post where someone tries to determine frame rate of video by repeatedly re-fetching frames.
    • Discussion about frame rates, time stamps of frames.
    • KG: texture uploads from video are naive in WebGL - tried to provide additional information like width, height, timestamp - didn't end up finishing those proposals.
    • KN: and WebCodecs came along while implementing this, and VideoFrame handles this case.
    • KN: concerns with landing this?
    • MM: no - just think some more design work may be needed to ensure the tutorial Brandon hypothesized doesn't get written.
    • MM: one more question - there's requestVideoFrame - will these handles expire at the next 60 Hz rAF or the next frame of the video they came from?
    • KN: the latter. Doesn't expire until after the video frame isn't current any more. At next boundary when video advances to next frame - the GPUExternalTexture will expire. Last and current external textures' lifetimes may overlap slightly.
    • KG: think we're good to land this.
    • MM: this is good, you can write a naive requestVideoFrame loop and it'll work.
    • KN: yes. Can drive rendering both by video frame rate and animation frame rate. Works well in both cases.

@kainino0x kainino0x added the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Jun 29, 2022
juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 17, 2022
@lokokung
Copy link
Contributor

lokokung commented Dec 6, 2022

Removing 'needs-cts-issue' for now because this is obsolete and will be updated in a future pull request to fix #3324.

@lokokung lokokung removed the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Dec 6, 2022
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.

Determine GPUExternalTexture lifetime

5 participants