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.

Sign up for GitHub

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

Simplify startPlayingAndWaitForVideo with modern async/await. #3319

Merged
merged 2 commits into from
Aug 20, 2021
Merged

Simplify startPlayingAndWaitForVideo with modern async/await. #3319

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbc420c
Simplify startPlayingAndWaitForVideo with modern async/await.
kdashg Aug 11, 2021
b0914ba
[startPlayingAndWaitForVideo] Use requestVideoFrameCallback if suppor…
kdashg Aug 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 15 additions & 31 deletions sdk/tests/js/webgl-test-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2927,14 +2927,6 @@ var requestAnimFrame = function(callback) {
_requestAnimFrame.call(window, callback);
};

/**
* Provides video.requestVideoFrameCallback in a cross browser way.
* Returns a property, or undefined if unsuported.
*/
var getRequestVidFrameCallback = function() {
return HTMLVideoElement.prototype["requestVideoFrameCallback"];
};

var _cancelAnimFrame;

/**
Expand Down Expand Up @@ -3136,38 +3128,30 @@ var runSteps = function(steps) {
* @param {!function(!HTMLVideoElement): void} callback Function to call when
* video is ready.
*/
function startPlayingAndWaitForVideo(video, callback) {
async function startPlayingAndWaitForVideo(video, callback) {
if (video.error) {
testFailed('Video failed to load: ' + video.error);
return;
}

video.addEventListener(
kdashg marked this conversation as resolved.
Show resolved Hide resolved
'error', e => { testFailed('Video playback failed: ' + e.message); },
true);

var rvfc = getRequestVidFrameCallback();
if (rvfc === undefined) {
var timeWatcher = function() {
if (video.currentTime > 0) {
callback(video);
} else {
requestAnimFrame.call(window, timeWatcher);
}
};

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

video.loop = true;
video.muted = true;
// See whether setting the preload flag de-flakes video-related tests.
video.preload = 'auto';
video.play();
};

try {
await video.play();
} catch (e) {
testFailed('video.play failed: ' + e);
return;
}

if (video.requestVideoFrameCallback) {
kdashg marked this conversation as resolved.
Show resolved Hide resolved
await new Promise(go => video.requestVideoFrameCallback(go));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

callback(video);
}

var getHost = function(url) {
url = url.replace("\\", "/");
Expand Down