From 3fe2ae4108e8ce4355c90640458d13cf81af160c Mon Sep 17 00:00:00 2001 From: Corey Masanto Date: Mon, 9 May 2022 17:36:43 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20[Story=20Preview]=20Enable=20amp?= =?UTF-8?q?-video=20to=20play=20in=20preview=20mode=20(#38149)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Enable amp-video to play in preview mode * First draft of video re-registration * Add reregistration logic * Add storyIsBeingPreviewed_() method to amp-story-page * Refactor and comment reregister logic in amp-story-page.js * Add missing comments/descriptions * Lint * Remove unnecessary indentation * Move the reregistration-specific logic in registration() to its correct location * Update incomplete method description * Ensure that an ID is used instead of 'undefined' * Lint * Allow for custom sources when resetting media pool elements * Lint * Update Sources.removeFrom() to use video sources instead of amp-video sources * Toggle the video error message off before attempting to replay the video * Rename reregisterUnplayedVideos_() to the more accurate reregisterAndPlayUnplayedVideos_() --- extensions/amp-story/1.0/amp-story-page.js | 154 +++++++++++++++++---- extensions/amp-story/1.0/media-pool.js | 45 ++++-- extensions/amp-story/1.0/sources.js | 11 +- 3 files changed, 172 insertions(+), 38 deletions(-) diff --git a/extensions/amp-story/1.0/amp-story-page.js b/extensions/amp-story/1.0/amp-story-page.js index d520ac08bdc4..1f32c7ac40fe 100644 --- a/extensions/amp-story/1.0/amp-story-page.js +++ b/extensions/amp-story/1.0/amp-story-page.js @@ -9,6 +9,7 @@ * */ import {CommonSignals_Enum} from '#core/constants/common-signals'; +import {VisibilityState_Enum} from '#core/constants/visibility-state'; import {Deferred} from '#core/data-structures/promise'; import {removeElement} from '#core/dom'; import {whenUpgradedToCustomElement} from '#core/dom/amp-element-helpers'; @@ -517,37 +518,51 @@ export class AmpStoryPage extends AMP.BaseElement { const registerAllPromise = this.registerAllMedia_(); if (this.isActive()) { - registerAllPromise.then(() => { - if (this.state_ === PageState.NOT_ACTIVE) { - return; - } - this.signals() - .whenSignal(CommonSignals_Enum.LOAD_END) - .then(() => { - if (this.state_ == PageState.PLAYING) { - this.advancement_.start(); - } - }); - this.preloadAllMedia_().then(() => { + registerAllPromise + .then(() => { if (this.state_ === PageState.NOT_ACTIVE) { return; } - this.startMeasuringAllVideoPerformance_(); - this.startListeningToVideoEvents_(); - // iOS 14.2 and 14.3 requires play to be called before unmute - this.playAllMedia_().then(() => { - if ( - !this.storeService_.get(StateProperty.MUTED_STATE) && - this.state_ !== PageState.NOT_ACTIVE - ) { - this.unmuteAllMedia(); + this.signals() + .whenSignal(CommonSignals_Enum.LOAD_END) + .then(() => { + if (this.state_ == PageState.PLAYING) { + this.advancement_.start(); + } + }); + this.preloadAllMedia_().then(() => { + if (this.state_ === PageState.NOT_ACTIVE) { + return; } + this.startMeasuringAllVideoPerformance_(); + this.startListeningToVideoEvents_(); + // iOS 14.2 and 14.3 requires play to be called before unmute + this.playAllMedia_().then(() => { + if ( + !this.storeService_.get(StateProperty.MUTED_STATE) && + this.state_ !== PageState.NOT_ACTIVE + ) { + this.unmuteAllMedia(); + } + }); + this.toggleCaptions_( + this.storeService_.get(StateProperty.CAPTIONS_STATE) + ); }); - this.toggleCaptions_( - this.storeService_.get(StateProperty.CAPTIONS_STATE) - ); + }) + .then(() => { + // In the PREVIEW state, a video can only use cached sources. If it + // fails to play due to any issue with the cached sources, we + // reregister the video once it has obtained its origin sources. + if (this.storyIsBeingPreviewed_()) { + // We first block the reregistration on video layout end because + // that is the point at which the story has entered the VISIBLE + // state and its origin sources have been added. + return this.waitForPlaybackMediaLayoutEnd_().then(() => { + return this.reregisterAndPlayUnplayedVideos_(); + }); + } }); - }); this.maybeStartAnimations_(); this.checkPageHasAudio_(); this.checkPageHasCaptions_(); @@ -556,6 +571,27 @@ export class AmpStoryPage extends AMP.BaseElement { } } + /** + * @return {!Promise} A promise that resolves when all videos that failed to + * play have been reregistered and played. + * @private + */ + reregisterAndPlayUnplayedVideos_() { + const videos = this.getAllVideos_(); + const unplayedVideos = videos.filter( + (video) => video.readyState < /* HAVE_CURRENT_DATA */ 2 + ); + return this.mediaPoolPromise_.then((pool) => { + const playPromises = unplayedVideos.map((video) => { + return this.reregisterMedia_(pool, video).then(() => { + this.toggleErrorMessage_(false); + return this.playMedia_(pool, video); + }); + }); + return Promise.all(playPromises); + }); + } + /** @override */ layoutCallback() { // Do not loop if the audio is used to auto-advance. @@ -649,10 +685,33 @@ export class AmpStoryPage extends AMP.BaseElement { } /** - * @return {!Promise} + * @return {!Promise} A promise that blocks until all playback media on the + * page have begun their layouts. + * @private + */ + waitForPlaybackMediaLayoutStart_() { + return this.waitForPlaybackMediaLayout_(true /* waitForLayoutStart */); + } + + /** + * @return {!Promise} A promise that blocks until all playback media on the + * page have completed their layouts. + * @private + */ + waitForPlaybackMediaLayoutEnd_() { + return this.waitForPlaybackMediaLayout_(false /* waitForLayoutStart */); + } + + /** + * @param {boolean} waitForLayoutStart Whether this method should only block + * until all playback media have begun their layouts, as opposed to + * having completed them. + * @return {!Promise} A promise that blocks until all playback media on the + * page have begun or completed their layouts, depending on the value of + * `waitForLayoutStart`. * @private */ - waitForPlaybackMediaLayout_() { + waitForPlaybackMediaLayout_(waitForLayoutStart) { const mediaSet = toArray( this.getMediaBySelector_(Selectors.ALL_PLAYBACK_AMP_MEDIA) ); @@ -662,10 +721,13 @@ export class AmpStoryPage extends AMP.BaseElement { switch (mediaEl.tagName.toLowerCase()) { case 'amp-audio': case 'amp-video': + const loadSignal = waitForLayoutStart + ? CommonSignals_Enum.LOAD_START + : CommonSignals_Enum.LOAD_END; const signal = mediaEl.getAttribute('layout') === Layout_Enum.NODISPLAY ? CommonSignals_Enum.BUILT - : CommonSignals_Enum.LOAD_END; + : loadSignal; whenUpgradedToCustomElement(mediaEl) .then((el) => el.signals().whenSignal(signal)) @@ -1047,7 +1109,14 @@ export class AmpStoryPage extends AMP.BaseElement { */ registerAllMedia_() { if (!this.registerAllMediaPromise_) { - this.registerAllMediaPromise_ = this.waitForPlaybackMediaLayout_().then( + // In preview mode, the `amp-video` layout callback does not resolve + // because it is blocked on requests for origin sources that cannot be + // made in the SERP due to privacy concerns. So, instead of indefinitely + // blocking registration, we register media elements at layout start. + const waitForPlaybackMediaLayoutPromise = this.storyIsBeingPreviewed_() + ? this.waitForPlaybackMediaLayoutStart_() + : this.waitForPlaybackMediaLayoutEnd_(); + this.registerAllMediaPromise_ = waitForPlaybackMediaLayoutPromise.then( () => this.whenAllMediaElements_((p, e) => this.registerMedia_(p, e)) ); } @@ -1073,6 +1142,33 @@ export class AmpStoryPage extends AMP.BaseElement { } } + /** + * Reregisters the given media. + * @param {!./media-pool.MediaPool} mediaPool + * @param {!Element} mediaEl + * @return {!Promise} Promise that resolves after the media is reregistered. + * @private + */ + reregisterMedia_(mediaPool, mediaEl) { + if (this.isBotUserAgent_) { + // No-op. + return Promise.resolve(); + } else { + return mediaPool.reregister( + /** @type {!./media-pool.DomElementDef} */ (mediaEl) + ); + } + } + + /** + * @return {boolean} Whether this page's story is currently being previewed. + * @private + */ + storyIsBeingPreviewed_() { + const visibilityState = this.getAmpDoc().getVisibilityState(); + return visibilityState === VisibilityState_Enum.PREVIEW; + } + /** * Starts playing animations, if the animation manager is available. * @private diff --git a/extensions/amp-story/1.0/media-pool.js b/extensions/amp-story/1.0/media-pool.js index 281093b1927f..a9e55b786994 100644 --- a/extensions/amp-story/1.0/media-pool.js +++ b/extensions/amp-story/1.0/media-pool.js @@ -554,15 +554,17 @@ export class MediaPool { /** * @param {!PoolBoundElementDef} poolMediaEl The element whose source should * be reset. + * @param {!Sources=} sources Optional sources for the media element. * @return {!Promise} A promise that is resolved when the pool media element * has been reset. */ - resetPoolMediaElementSource_(poolMediaEl) { - const defaultSources = this.getDefaultSource_(); - + resetPoolMediaElementSource_( + poolMediaEl, + sources = this.getDefaultSource_() + ) { return this.enqueueMediaElementTask_( poolMediaEl, - new UpdateSourcesTask(this.win_, defaultSources) + new UpdateSourcesTask(this.win_, sources) ).then(() => this.enqueueMediaElementTask_(poolMediaEl, new LoadTask())); } @@ -663,24 +665,51 @@ export class MediaPool { return this.enqueueMediaElementTask_(poolMediaEl, new BlessTask()); } + /** + * Reregisters the specified element to be usable by the media pool. This + * is useful in cases where the element's sources have updated since the + * previous registration and a reload of the element using these new sources + * is desired. + * @param {!DomElementDef} domMediaEl The media element to be reregistered. + * @return {!Promise} A promise that is resolved when the element has been + * successfully reregistered, or rejected otherwise. + */ + reregister(domMediaEl) { + return this.register(domMediaEl, true /** isReregistration */); + } + /** * Registers the specified element to be usable by the media pool. Elements * should be registered as early as possible, in order to prevent them from - * being played while not managed by the media pool. If the media element is - * already registered, this is a no-op. Registering elements from within the - * pool is not allowed, and will also be a no-op. + * being played while not managed by the media pool. Registering elements + * from within the pool is not allowed, and will also be a no-op. + * + * If the media element is already registered and `isReregistration` is true, + * then the media element will be loaded. However, if the element is + * registered and `isReregistration` is false, then this is a no-op. * @param {!DomElementDef} domMediaEl The media element to be * registered. + * @param {boolean=} isReregistration Whether the given element has already + * been registered. * @return {!Promise} A promise that is resolved when the element has been * successfully registered, or rejected otherwise. */ - register(domMediaEl) { + register(domMediaEl, isReregistration = false) { const parent = domMediaEl.parentNode; if (parent && parent.signals) { this.trackAmpElementToBless_(/** @type {!AmpElement} */ (parent)); } if (this.isPoolMediaElement_(domMediaEl)) { + // In the case of a reregistration, `UpdateSourcesTask` and `LoadTask` + // are used to load the element using its sources (which may have changed + // since the previous registration). + if (isReregistration) { + const sources = Sources.removeFrom(this.win_, domMediaEl); + this.sources_[domMediaEl.id] = sources; + return this.resetPoolMediaElementSource_(domMediaEl, sources); + } + // This media element originated from the media pool. return Promise.resolve(); } diff --git a/extensions/amp-story/1.0/sources.js b/extensions/amp-story/1.0/sources.js index a798b5a7c92b..059487aa0c92 100644 --- a/extensions/amp-story/1.0/sources.js +++ b/extensions/amp-story/1.0/sources.js @@ -96,7 +96,16 @@ export class Sources { * element. */ static removeFrom(win, element) { - const elementToUse = ampMediaElementFor(element) || element; + let elementToUse; + if (element.tagName === 'VIDEO') { + // A video element and its amp-video parent can each have different + // sources. We prefer to remove and return the video's sources because + // amp-video's sources are primarily those provided by the publisher's + // whereas the video's sources are added and modified via amp-video JS. + elementToUse = element; + } else { + elementToUse = ampMediaElementFor(element) || element; + } let srcEl = null; // If the src attribute is specified, create a source element from it as it