Skip to content

Commit

Permalink
Wrap all play calls in catch handler (#35811)
Browse files Browse the repository at this point in the history
* Wrap all play calls in catch handler

* Introduce playIgnoringError

* Fix type

* Fix undefined return case
  • Loading branch information
jridgewell committed Aug 26, 2021
1 parent b29db7c commit 8889d8c
Show file tree
Hide file tree
Showing 13 changed files with 80 additions and 54 deletions.
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

0 comments on commit 8889d8c

Please sign in to comment.