Simplify startPlayingAndWaitForVideo with modern async/await. #3319

kdashg · 2021-08-18T01:22:40Z

No description provided.

kenrussell · 2021-08-18T17:00:59Z

@dalecurtis do you think this mechanism will work reliably in Chromium? We've iterated on this primitive over and over in order to eliminate flakiness.

dalecurtis

I don't think this is safe.

dalecurtis · 2021-08-18T17:09:01Z

sdk/tests/js/webgl-test-utils.js

-  video.preload = 'auto';
-  video.play();
+  (async () => {
+    video.load(); // reset it


This will trigger a significant amount of unnecessary work since a load is already in flight.

dalecurtis · 2021-08-18T17:09:46Z

sdk/tests/js/webgl-test-utils.js

-    timeWatcher();
-  } else {
-    // Calls video.requestVideoFrameCallback(_ => { callback(video) })
-    rvfc.call(video, _ => { callback(video) });


This needs to be preserved since the play promise resolving doesn't mean that a frame can be drawn from it yet.

I think it probably does mean this? (see comments in main thread)

dalecurtis · 2021-08-18T17:10:41Z

sdk/tests/js/webgl-test-utils.js

+    // See whether setting the preload flag de-flakes video-related tests.
+    video.preload = 'auto';
+    try {
+      await video.play();


I don't think this is safe, see #3263 where I reverted this previously.

kenrussell

Thanks @dalecurtis for your review.

At the specification level:
https://html.spec.whatwg.org/multipage/media.html#dom-media-play

is one issue that the play() Promise can be resolved when the network state is still HAVE_FUTURE_DATA? In other words, per spec, it's not guaranteed that data's available for the first frame when this Promise resolves?

kdashg · 2021-08-18T23:12:39Z

HAVE_FUTURE_DATA seems to mean that there's data for the next frame:

Data for the immediate current playback position is available, as well as enough data for the user agent to advance the current playback position in the direction of playback at least a little without immediately reverting to the HAVE_METADATA state, and the text tracks are ready. For example, in video this corresponds to the user agent having data for at least the current frame and the next frame when the current playback position is at the instant in time between the two frames, or to the user agent having the video data for the current frame and audio data to keep playing at least a little when the current playback position is in the middle of a frame. The user agent cannot be in this state if playback has ended, as the current playback position can never advance in this case.

Do we need to await on timeupdate then? resize? There needs to be a way to do this, otherwise we can't expect websites to get it right either. :)

kdashg · 2021-08-18T23:16:30Z

I think HAVE_FUTURE_DATA ought to be enough to ensure we can texImage(video). At worst this might causes a wait-for-decode, but the data is ready at least.

kdashg · 2021-08-18T23:19:24Z

image-bitmap-from-video failures are likely #3322.

kenrussell · 2021-08-18T23:45:25Z

Big thanks for finding #3322 . The failures that were seen previously were more widespread, and included widespread conformance/textures/video failures:

https://chromium-review.googlesource.com/c/chromium/src/+/2775887/5

e.g.

https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/644735/overview
https://ci.chromium.org/ui/p/chromium/builders/try/mac_optional_gpu_tests_rel/56041/overview
https://ci.chromium.org/ui/p/chromium/builders/try/win10_chromium_x64_rel_ng/816557/overview

so unfortunately this change is incompatible with Chromium's implementation of the HTMLVideoElement.play() spec, at least.

kdashg · 2021-08-18T23:50:25Z

I do think we sort of fall short here of defining what should happen and-then-therefore testing that. We need to define at what point calling texImage(video) should succeed, and what should happen otherwise.

My intuition is that we should try to define it such that await video.play(); texImage(video) should reliably succeed. How implementable does that sound?

dalecurtis · 2021-08-19T00:16:44Z

We've generally interpreted readyState to indicate that the encoded data for a given playback position is available, not that we have rendered the frame yet. While it generally does mean that we have rendered a frame, it's not 100% accurate -- especially in rapid load/test cycles like seen in web platform and conformance tests.

Media elements have a ready state, which describes to what degree they are ready to be rendered at the current playback position.

Emphasis on "ready to be" mine. If we interpret that to mean the decoded frame must have been rendered, then significant load deferral optimizations for offscreen and/or autoplay-rejected videos are not possible. Which turns into a chicken or the egg problem since some sites won't try to play an element until HAVE_FUTURE_DATA.

requestVideoFrameCallback() is one way to ensure the frame has actually been presented. I think we could defer play promise resolution until the first frame is rendered, but it would need a deeper analysis and real world testing.

kdashg · 2021-08-19T00:20:44Z

I'm not sure we actually need the first frame to have been rendered in the general sense, though we should test that for sure. (gotta make sure the fastpaths work)

For instance, we need to support videos that are never rendered to the screen, and only exist floating loose from the DOM, yet still play and can be uploaded into textures.

dalecurtis · 2021-08-19T00:24:49Z

By rendered I just meant decoded and available to script. rVFC() works for offscreen elements. In Chromium's implementation we let texImage(VideoElement) drive the clock for offscreen elements, but its done asynchronously since we'd otherwise need to block on a background thread from the main thread.

kdashg · 2021-08-19T00:31:02Z

I think that await video.play(); texImage(video) should succeed. Is this implementable?

dalecurtis · 2021-08-19T00:34:09Z

Probably, I don't see any immediate perils, but as noted in #3319 (comment) it'll need some real world testing. Certainly simpler sounding things have ended up as very hairy yaks in the past :|

kenrussell · 2021-08-19T03:58:11Z

@grorg @kimmok what do you think about the feasibility of making the Promise returned from HTMLVideoElement.play obey these guarantees in Safari?

kainino0x · 2021-08-19T05:25:59Z

FYI I recently ported startPlayingAndWaitForVideo into the WebGPU CTS. If any changes end up landing here I'd like to port them over too.
/~https://github.com/gpuweb/cts/blob/main/src/webgpu/web_platform/util.ts

…ted. Chrome needs this currently. Also make function async. TODO: Update callsites to use async instead of callback.

kdashg · 2021-08-20T00:02:40Z

@dalecurtis PTAL
I sort of combined the two, and use rvfc where supported.
await video.play() seems to fix some flakiness in Firefox's CI.

kenrussell · 2021-08-20T00:11:10Z

sdk/tests/js/webgl-test-utils.js

+  }
+
+  if (video.requestVideoFrameCallback) {
+    await new Promise(go => video.requestVideoFrameCallback(go));


@dalecurtis do you think this will have the same basic semantics as the earlier code in Chromium?

It will have different semantics as I note in the comment thread below, but it should be okay % potential for minor slowness.

sdk/tests/js/webgl-test-utils.js

kdashg · 2021-08-20T00:34:06Z

Thanks all!

kdashg requested a review from kenrussell August 18, 2021 01:28

dalecurtis reviewed Aug 18, 2021

View reviewed changes

kenrussell reviewed Aug 18, 2021

View reviewed changes

kdashg added 2 commits August 19, 2021 10:32

Simplify startPlayingAndWaitForVideo with modern async/await.

dbc420c

[startPlayingAndWaitForVideo] Use requestVideoFrameCallback if suppor…

b0914ba

…ted. Chrome needs this currently. Also make function async. TODO: Update callsites to use async instead of callback.

kdashg force-pushed the simple-play-video branch from 0de4f23 to b0914ba Compare August 19, 2021 20:47

kenrussell reviewed Aug 20, 2021

View reviewed changes

dalecurtis approved these changes Aug 20, 2021

View reviewed changes

sdk/tests/js/webgl-test-utils.js Show resolved Hide resolved

sdk/tests/js/webgl-test-utils.js Show resolved Hide resolved

kdashg merged commit afcae42 into KhronosGroup:main Aug 20, 2021

hujiajie mentioned this pull request Aug 9, 2022

Strengthen frame readiness while testing fromPixels with video element tensorflow/tfjs#6735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify startPlayingAndWaitForVideo with modern async/await. #3319

Simplify startPlayingAndWaitForVideo with modern async/await. #3319

kdashg commented Aug 18, 2021

kenrussell commented Aug 18, 2021

dalecurtis left a comment

dalecurtis Aug 18, 2021

dalecurtis Aug 18, 2021

kdashg Aug 18, 2021

dalecurtis Aug 18, 2021

kenrussell left a comment

kdashg commented Aug 18, 2021

kdashg commented Aug 18, 2021

kdashg commented Aug 18, 2021

kenrussell commented Aug 18, 2021

kdashg commented Aug 18, 2021

dalecurtis commented Aug 19, 2021

kdashg commented Aug 19, 2021

dalecurtis commented Aug 19, 2021 •

edited

Loading

kdashg commented Aug 19, 2021

dalecurtis commented Aug 19, 2021

kenrussell commented Aug 19, 2021

kainino0x commented Aug 19, 2021

kdashg commented Aug 20, 2021

kenrussell Aug 20, 2021

dalecurtis Aug 20, 2021

kdashg commented Aug 20, 2021

Simplify startPlayingAndWaitForVideo with modern async/await. #3319

Simplify startPlayingAndWaitForVideo with modern async/await. #3319

Conversation

kdashg commented Aug 18, 2021

kenrussell commented Aug 18, 2021

dalecurtis left a comment

Choose a reason for hiding this comment

dalecurtis Aug 18, 2021

Choose a reason for hiding this comment

dalecurtis Aug 18, 2021

Choose a reason for hiding this comment

kdashg Aug 18, 2021

Choose a reason for hiding this comment

dalecurtis Aug 18, 2021

Choose a reason for hiding this comment

kenrussell left a comment

Choose a reason for hiding this comment

kdashg commented Aug 18, 2021

kdashg commented Aug 18, 2021

kdashg commented Aug 18, 2021

kenrussell commented Aug 18, 2021

kdashg commented Aug 18, 2021

dalecurtis commented Aug 19, 2021

kdashg commented Aug 19, 2021

dalecurtis commented Aug 19, 2021 • edited Loading

kdashg commented Aug 19, 2021

dalecurtis commented Aug 19, 2021

kenrussell commented Aug 19, 2021

kainino0x commented Aug 19, 2021

kdashg commented Aug 20, 2021

kenrussell Aug 20, 2021

Choose a reason for hiding this comment

dalecurtis Aug 20, 2021

Choose a reason for hiding this comment

kdashg commented Aug 20, 2021

dalecurtis commented Aug 19, 2021 •

edited

Loading