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

Wrap all play calls in catch handler #35811

Merged
merged 8 commits into from
Aug 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion 3p/bodymovinanimation.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {setStyles} from '#core/dom/style';
import {tryPlay} from '#core/dom/video';
import {dict} from '#core/types/object';
import {parseJson} from '#core/types/object/json';

Expand Down Expand Up @@ -42,7 +43,7 @@ function parseMessage(event) {
const eventMessage = parseJson(getData(event));
const action = eventMessage['action'];
if (action == 'play') {
animationHandler.play();
tryPlay(animationHandler);
} else if (action == 'pause') {
animationHandler.pause();
} else if (action == 'stop') {
Expand Down
3 changes: 2 additions & 1 deletion 3p/viqeoplayer.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {tryPlay} from '#core/dom/video';
import {tryDecodeUriComponent} from '#core/types/string/url';

import {loadScript} from './3p';
Expand Down Expand Up @@ -76,7 +77,7 @@ function viqeoPlayerInitLoaded(global, VIQEO) {
return;
}
if (action === 'play') {
viqeoPlayerInstance.play();
tryPlay(viqeoPlayerInstance);
} else if (action === 'pause') {
viqeoPlayerInstance.pause();
} else if (action === 'stop') {
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-audio/0.1/amp-audio.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {Layout, applyFillContent, isLayoutSizeFixed} from '#core/dom/layout';
import {propagateAttributes} from '#core/dom/propagate-attributes';
import {realChildNodes} from '#core/dom/query';
import {tryPlay} from '#core/dom/video';

import {triggerAnalyticsEvent} from '../../../src/analytics';
import {listen} from '../../../src/event-helper';
Expand Down Expand Up @@ -235,7 +236,7 @@ export class AmpAudio extends AMP.BaseElement {
if (!this.isInvocationValid_()) {
return;
}
this.audio_.play();
tryPlay(this.audio_);
this.setPlayingStateForTesting_(true);
}

Expand All @@ -253,7 +254,7 @@ export class AmpAudio extends AMP.BaseElement {
/** @private */
audioPlaying_() {
const playHandler = () => {
this.audio_.play();
tryPlay(this.audio_);
this.setPlayingStateForTesting_(true);
};
const pauseHandler = () => {
Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-iframe/0.1/amp-iframe.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {playIgnoringError} from '#core/dom/video';
import {AMPDOC_SINGLETON_NAME} from '#core/constants/enums';
import {ActionTrust} from '#core/constants/action-constants';
import {IntersectionObserver3pHost} from '../../../src/utils/intersection-observer-3p-host';
Expand Down Expand Up @@ -835,7 +836,7 @@ export class AmpIframe extends AMP.BaseElement {
return false;
}
const audio = this.win.document.createElement('audio');
audio.play();
playIgnoringError(audio);
if (audio.paused) {
return false;
}
Expand Down
6 changes: 3 additions & 3 deletions extensions/amp-story/1.0/amp-story-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* </amp-story-page>
* </code>
*/
import {isAutoplaySupported, tryPlay} from '#core/dom/video';
import {
AFFILIATE_LINK_SELECTOR,
AmpStoryAffiliateLink,
Expand Down Expand Up @@ -53,7 +54,6 @@ import {getLogEntries} from './logging';
import {getMediaPerformanceMetricsService} from './media-performance-metrics-service';
import {getMode} from '../../../src/mode';
import {htmlFor} from '#core/dom/static-template';
import {isAutoplaySupported} from '#core/dom/video';
import {isExperimentOn} from '#experiments';
import {isPrerenderActivePage} from './prerender-active-page';
import {listen, listenOnce} from '../../../src/event-helper';
Expand Down Expand Up @@ -1028,7 +1028,7 @@ export class AmpStoryPage extends AMP.BaseElement {
*/
playMedia_(mediaPool, mediaEl) {
if (this.isBotUserAgent_) {
mediaEl.play();
tryPlay(mediaEl);
return Promise.resolve();
} else {
return this.loadPromise(mediaEl).then(
Expand Down Expand Up @@ -1151,7 +1151,7 @@ export class AmpStoryPage extends AMP.BaseElement {
mediaEl.muted = false;
mediaEl.removeAttribute('muted');
if (mediaEl.tagName === 'AUDIO' && mediaEl.paused) {
mediaEl.play();
tryPlay(mediaEl);
}
return Promise.resolve();
} else {
Expand Down
8 changes: 3 additions & 5 deletions extensions/amp-story/1.0/media-tasks.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {Deferred, tryResolve} from '#core/data-structures/promise';
import {Deferred} from '#core/data-structures/promise';
import {tryPlay} from '#core/dom/video';
import {Sources} from './sources';
import {isConnectedNode} from '#core/dom';

Expand Down Expand Up @@ -204,10 +205,7 @@ export class PlayTask extends MediaTask {
return Promise.resolve();
}

// The play() invocation is wrapped in a Promise.resolve(...) due to the
// fact that some browsers return a promise from media elements' play()
// function, while others return a boolean.
return tryResolve(() => mediaEl.play());
return tryPlay(mediaEl);
}
}

Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-video-docking/0.1/controls.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
toggle,
translate,
} from '#core/dom/style';
import {tryPlay} from '#core/dom/video';
import {once} from '#core/types/function';

import {Services} from '#service';
Expand Down Expand Up @@ -271,7 +272,7 @@ export class Controls {
}),

this.listenWhenEnabled_(this.playButton_, click, () => {
video.play(/* auto */ false);
tryPlay(video, /* auto */ false);
}),

this.listenWhenEnabled_(this.pauseButton_, click, () => {
Expand Down
13 changes: 2 additions & 11 deletions extensions/amp-video/0.1/amp-video.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {tryPlay} from '#core/dom/video';
import {EMPTY_METADATA} from '../../../src/mediasession-helper';
import {PauseHelper} from '#core/dom/video/pause-helper';
import {Services} from '#service';
Expand Down Expand Up @@ -792,17 +793,7 @@ export class AmpVideo extends AMP.BaseElement {
* @override
*/
play(unusedIsAutoplay) {
const ret = this.video_.play();

if (ret && ret.catch) {
ret.catch(() => {
// Empty catch to prevent useless unhandled promise rejection logging.
// Play can fail for many reasons such as video getting paused before
// play() is finished.
// We use events to know the state of the video and do not care about
// the success or failure of the play()'s returned promise.
});
}
tryPlay(this.video_);
}

/**
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-video/0.1/flexible-bitrate.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {tryPlay} from '#core/dom/video';
import {DomBasedWeakRef} from '#core/data-structures/dom-based-weakref';
import {Services} from '#service';
import {childElement, childElementsByTag} from '#core/dom/query';
Expand Down Expand Up @@ -239,14 +240,14 @@ export class BitrateManager {
video.pause();
const hasChanges = this.sortSources_(video);
if (!hasChanges) {
video.play();
tryPlay(video);
return;
}
video.load();
listenOnce(video, 'loadedmetadata', () => {
// Restore currentTime after loading new source.
video.currentTime = currentTime;
video.play();
tryPlay(video);
dev().fine(TAG, 'Playing at lower bitrate %s', video.currentSrc);
});
}
Expand Down
18 changes: 0 additions & 18 deletions extensions/amp-video/0.1/test/test-amp-video.js
Original file line number Diff line number Diff line change
Expand Up @@ -523,24 +523,6 @@ describes.realWin(
expect(impl.toggleFallback).to.have.been.calledWith(true);
});

it('play() should not log promise rejections', async () => {
const playPromise = Promise.reject('The play() request was interrupted');
const catchSpy = env.sandbox.spy(playPromise, 'catch');
await getVideo(
{
src: 'video.mp4',
width: 160,
height: 90,
},
null,
function (element, impl) {
env.sandbox.stub(impl.video_, 'play').returns(playPromise);
impl.play();
}
);
expect(catchSpy.called).to.be.true;
});

it('decode error retries the next source', async () => {
const s0 = doc.createElement('source');
s0.setAttribute('src', './0.mp4');
Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-video/1.0/component.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as Preact from '#preact';
import {ContainWrapper, useValueRef} from '#preact/component';
import {tryPlay} from '#core/dom/video';
import {Deferred} from '#core/data-structures/promise';
import {Loading} from '#core/constants/loading-instructions';
import {MIN_VISIBILITY_RATIO_FOR_AUTOPLAY} from '../../../src/video-interface';
Expand Down Expand Up @@ -141,7 +142,7 @@ function VideoWrapperWithRef(
}, [load, setPlayingState]);

const play = useCallback(() => {
return readyDeferred.promise.then(() => playerRef.current.play());
return readyDeferred.promise.then(() => tryPlay(playerRef.current));
}, [readyDeferred]);

const pause = useCallback(() => {
Expand Down
52 changes: 48 additions & 4 deletions src/core/dom/video/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import {devAssertElement} from '#core/assert';
import {tryResolve} from '#core/data-structures/promise';
import {setStyles} from '#core/dom/style';
import {devExpectedError} from '#core/error';

/**
* @param {!Window} win
Expand Down Expand Up @@ -37,10 +39,7 @@ export function detectIsAutoplaySupported(win) {

// Promise wrapped this way to catch both sync throws and async rejections.
// More info: https://github.com/tc39/proposal-promise-try
new Promise((resolve) => resolve(detectionElement.play())).catch(() => {
// Suppress any errors, useless to report as they are expected.
});

playIgnoringError(detectionElement);
return Promise.resolve(!detectionElement.paused);
}

Expand Down Expand Up @@ -78,3 +77,48 @@ export function resetIsAutoplaySupported(win) {
export function getInternalVideoElementFor(element) {
return devAssertElement(element.querySelector('video, iframe'));
}

/**
* @typedef {{
* play: (function(boolean): Promise<undefined>|undefined)
* }}
*/
let VideoOrBaseElementPlayableDef;

/**
* Tries to play the media element, marking any rejected error as an expected
* error for reproting.
*
* @param {!HTMLMediaElement|VideoOrBaseElementPlayableDef} element
* @param {boolean=} isAutoplay
* @return {Promise<undefined>}
*/
export function tryPlay(element, isAutoplay) {
// Some browsers return undefined, some a boolean, and some a real promise.
// Using tryResolve coerces all of those into a real promise.
const promise = tryResolve(() => element.play(!!isAutoplay));
// Fork the promise chain to report any rejected error as expected. We don't
// return the promise returned by `.catch()` so that we don't
// introduce any new microtasks.
promise.catch((err) => {
devExpectedError('TRYPLAY', err);
});
return promise;
}

/**
* Plays the media element, discarding any error without reporting it.
*
* @param {!HTMLMediaElement} element
*/
export function playIgnoringError(element) {
// Some browsers return undefined, some a boolean, and some a real promise.
// Using tryResolve coerces all of those into a real promise.
tryResolve(() => element.play()).catch(() => {
// Empty catch to prevent useless unhandled promise rejection logging.
// Play can fail for many reasons such as video getting paused before
// play() is finished.
// We use events to know the state of the video and do not care about
// the success or failure of the play()'s returned promise.
});
}
12 changes: 8 additions & 4 deletions src/service/video-manager-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ import {dispatchCustomEvent, removeElement} from '#core/dom';
import {measureIntersection} from '#core/dom/layout/intersection';
import {createViewportObserver} from '#core/dom/layout/viewport-observer';
import {toggle} from '#core/dom/style';
import {getInternalVideoElementFor, isAutoplaySupported} from '#core/dom/video';
import {
getInternalVideoElementFor,
isAutoplaySupported,
tryPlay,
} from '#core/dom/video';
import {clamp} from '#core/math';
import {isFiniteNumber} from '#core/types';
import {once} from '#core/types/function';
Expand Down Expand Up @@ -233,7 +237,7 @@ export class VideoManager {
// specific handling (e.g. user gesture requirement for unmuted playback).
const trust = ActionTrust.LOW;

registerAction('play', () => video.play(/* isAutoplay */ false));
registerAction('play', () => tryPlay(video, /* isAutoplay */ false));
registerAction('pause', () => video.pause());
registerAction('mute', () => video.mute());
registerAction('unmute', () => video.unmute());
Expand Down Expand Up @@ -479,7 +483,7 @@ class VideoEntry {

/** @private @const {function()} */
this.boundMediasessionPlay_ = () => {
this.video.play(/* isAutoplay */ false);
tryPlay(this.video, /* isAutoplay */ false);
};

/** @private @const {function()} */
Expand Down Expand Up @@ -901,7 +905,7 @@ class VideoEntry {
}
if (this.isVisible_) {
this.visibilitySessionManager_.beginSession();
this.video.play(/*autoplay*/ true);
tryPlay(this.video, /*autoplay*/ true);
this.playCalledByAutoplay_ = true;
} else {
if (this.isPlaying_) {
Expand Down