Implement MediaPool for amp-story #12156
Conversation
…tes progress bar through PAGE_PROGRESS events
…age updates progress bar through PAGE_PROGRESS events" This reverts commit 0500899.
| * The delay (in milliseconds) to wait between polling for loaded resources. | ||
| */ | ||
| const LOAD_TIMER_POLL_DELAY_MS = 250; | ||
| const PAGE_MEDIA_SELECTOR = 'amp-audio, amp-video, amp-img, anim'; |
| this.mediaPoolPromise_ = new Promise(resolve => { | ||
| this.setMediaPool = mediaPool => { | ||
| this.mediaLayoutPromise_ | ||
| .then(() => resolve(mediaPool)); |
|
|
||
| return Promise.all([ | ||
| this.beforeVisible(), | ||
| this.mediaLayoutPromise_, |
There was a problem hiding this comment.
mediaPoolPromise_ seems to cover mediaLayoutPromise_ anyway.
| this.markPageAsShown_(); | ||
| this.resolveLoadPromise_(); | ||
| waitForMediaLayout_() { | ||
| const mediaSet = scopedQuerySelectorAll(this.element, PAGE_MEDIA_SELECTOR); |
There was a problem hiding this comment.
nit: you can use toArray from dom.js and then call map directly instead of on prototype.
There was a problem hiding this comment.
Hmm, on another PR, a reviewer (don't remember who) advised that I do this instead to avoid allocating extra arrays.
There was a problem hiding this comment.
sgtm. In the grand scheme of things, neither of the option would make a real difference.
| if (!canPageBeShown) { | ||
| canPageBeShown = pageElement.canBeShown; | ||
| } | ||
| return mediaEl.mozHasAudio || |
There was a problem hiding this comment.
interesting! I hope muted which comes with autoplay does not mess this up.
There was a problem hiding this comment.
This is not the right definition! No one should look at this as a good example of how to detect whether a media element has audio, because it's not right! This causes #11857, but this was the previous definition; I'm just porting it over from page-elements.js as part of the refactor in this PR.
| this.copyAttributes_(domMediaEl, poolMediaEl); | ||
| sources.applyToElement(poolMediaEl); | ||
| poolMediaEl.setAttribute(REPLACED_MEDIA_ATTRIBUTE, domMediaEl.id); | ||
| domMediaEl.parentElement.replaceChild(poolMediaEl, domMediaEl); |
There was a problem hiding this comment.
without resetting the private this.video_ reference in amp-video, this will break some functionality. Most may be irrelevant to STAMP but one that probably matters is "SessionMedia" stuff (e.g. when you play a video and then lock your phone, the lock screen shows the poster and media controls for the video that was just playing).
Also not setting this.video_ will keep reference count > 0 which will keep the old dom node in memory for no good reason.
(I don't mind STAMP accessing .video_ for now, later however we may wanna to officially add swapVideo() to AMP core)
Let's also make sure there are enough tests in STAMPs to catch bug if amp-video does refactoring in a way that may break STAMP.
A good long-term solution could be STAMP having its own player amp-stamp-video. amp-video is not a lot of code anyway. Most things are handle by video-manager which STAMP version of video player can register to as well.
There was a problem hiding this comment.
MediaPool actually keeps the DOM node in memory anyway; if a video is evicted from the MediaPool, we put back the original video.
There was a problem hiding this comment.
I see. I assume simply unattaching it from the DOM frees up hardware decoding space on iOS though
| const currentTime = mediaEl.currentTime; | ||
|
|
||
| mediaEl.muted = false; | ||
| return mediaEl.play().then(() => { |
There was a problem hiding this comment.
play is not always a promise (MS Edge/IE)
There was a problem hiding this comment.
Wrapped in Promise.resolve, which (I think?) should handle both cases.
…ssary since we wait for layout to complete.
…toryPage directly.
* Change progress bar to not handle active page, since active page updates progress bar through PAGE_PROGRESS events * Revert "Change progress bar to not handle active page, since active page updates progress bar through PAGE_PROGRESS events" This reverts commit 0500899. * Add media pool to manage playback of media elements. * Update to work without PageElements * Allow preloading, muting/unmuting, and blessing videos * Rough commit to wait for media layout before registering/playing * Fix registration and cleanup some code * Revert changes to amp-audio and amp-video, as they are no longer necessary since we wait for layout to complete. * Fix lint and type errors. * Add default media and fix unblessing * Remove AudioManager, since audio is now managed by MediaPool and AmpStoryPage directly. * Add basic unit tests for MediaPool * Add extensions and AmpStoryHint * Remove amp-audio and amp-video changes * Fix review comments * Fix sync * Remove blacklisted outerHTML calls. * Use single quotes instead of backticks for string literals
This reverts commit b29ade9.
…ject#12334) This reverts commit b29ade9.
…ampproject#12334)" This reverts commit f8d6f38.
* Change progress bar to not handle active page, since active page updates progress bar through PAGE_PROGRESS events * Revert "Change progress bar to not handle active page, since active page updates progress bar through PAGE_PROGRESS events" This reverts commit 0500899. * Add media pool to manage playback of media elements. * Update to work without PageElements * Allow preloading, muting/unmuting, and blessing videos * Rough commit to wait for media layout before registering/playing * Fix registration and cleanup some code * Revert changes to amp-audio and amp-video, as they are no longer necessary since we wait for layout to complete. * Fix lint and type errors. * Add default media and fix unblessing * Remove AudioManager, since audio is now managed by MediaPool and AmpStoryPage directly. * Add basic unit tests for MediaPool * Add extensions and AmpStoryHint * Remove amp-audio and amp-video changes * Fix review comments * Fix sync * Remove blacklisted outerHTML calls. * Use single quotes instead of backticks for string literals
…ject#12334) This reverts commit b29ade9.
…ject#12334) This reverts commit b29ade9.
This PR changes the way we handle media within
amp-story. Instead of using the media elements (audio,video) that are directly in the document, we swap them in and out of the DOM so that the same few media elements are recycled, but the sources are simply changed to reflect the media on the currentamp-story-page.Since this is such a big change to how we play media within
amp-story, it ends up fixing (or partially fixing) a few of the media-related issues we have:MediaPoolto keep theSourcesin memory, rather than on the placeholder DOM elements)autoplayattribute, some videos still may play while inactive, but this can be fixed early on in theMediaPoollifecycle)It may also complicate some other issues:
MediaPoolwill swap out the<video>elements with ones from the pool. Hopefully the preloading will still be beneficial, and browser caching will allow us to still benefit from this change, but this is untested. Regardless, we can do extra work within theMediaPoolimplementation to make this work, long-term.Tests are still a work in progress.
/cc @alanorozco @prateekbh