-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[Story video] Video cache for amp-video[src]. #34570
Changes from 4 commits
e336494
9fc13de
3a3cb84
b3f653c
5c82d22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,12 @@ | |
|
||
import {Services} from '../../../src/services'; | ||
import {addParamsToUrl, resolveRelativeUrl} from '../../../src/url'; | ||
import {createElementWithAttributes, matches} from '../../../src/dom'; | ||
import { | ||
createElementWithAttributes, | ||
iterateCursor, | ||
matches, | ||
removeElement, | ||
} from '../../../src/dom'; | ||
import {extensionScriptInNode} from '../../../src/service/extension-script'; | ||
import {toArray} from '../../../src/core/types/array'; | ||
import {user} from '../../../src/log'; | ||
|
@@ -33,13 +38,17 @@ import {user} from '../../../src/log'; | |
export function fetchCachedSources(videoEl, win) { | ||
if ( | ||
!extensionScriptInNode(win, 'amp-cache-url', '0.1') || | ||
!videoEl.querySelector('source[src]').getAttribute('src') | ||
!( | ||
videoEl.getAttribute('src') || | ||
videoEl.querySelector('source[src]')?.getAttribute('src') | ||
) | ||
) { | ||
user().error('AMP-VIDEO', 'Video cache not properly configured'); | ||
return Promise.resolve(); | ||
} | ||
const {canonicalUrl, sourceUrl} = Services.documentInfoForDoc(win.document); | ||
const servicePromise = Services.cacheUrlServicePromiseForDoc(videoEl); | ||
replaceSrcWithSourceElement(videoEl, win); | ||
const videoUrl = resolveRelativeUrl(selectVideoSource(videoEl), sourceUrl); | ||
return servicePromise | ||
.then((service) => service.createCacheUrl(videoUrl)) | ||
|
@@ -95,3 +104,34 @@ function applySourcesToVideo(videoEl, sources) { | |
videoEl.insertBefore(sourceEl, videoEl.firstChild); | ||
}); | ||
} | ||
|
||
/** | ||
* Moves the src attribute to a source element to enable playing from multiple | ||
* sources: the cached ones and the fallback initial src. | ||
* @param {!Element} videoEl | ||
* @param {!Window} win | ||
* @return {!Promise} | ||
*/ | ||
function replaceSrcWithSourceElement(videoEl, win) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Genuinely asking: Is it a good idea to prepend There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, I added |
||
if (!videoEl.hasAttribute('src')) { | ||
return; | ||
} | ||
const sourceEl = win.document.createElement('source'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: We have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes it a bit harder to not set a |
||
const srcAttr = videoEl.getAttribute('src'); | ||
sourceEl.setAttribute('src', srcAttr); | ||
|
||
const typeAttr = videoEl.getAttribute('type'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Don't we also need to remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove it to keep the DOM clean and updated. Just for the record, since the video would not even get a Done |
||
if (typeAttr) { | ||
sourceEl.setAttribute('type', typeAttr); | ||
} | ||
|
||
// Remove the src attr so the source children can play. | ||
videoEl.removeAttribute('src'); | ||
|
||
// Remove all existing sources as they are never supposed to play for a video | ||
// that has a src, cf https://html.spec.whatwg.org/#concept-media-load-algorithm | ||
const sourceEls = videoEl.querySelectorAll('source'); | ||
iterateCursor(sourceEls, (el) => removeElement(el)); | ||
|
||
videoEl.insertBefore(sourceEl, videoEl.firstChild); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Does not return promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done