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

[Story video] Video cache for amp-video[src]. #34570

Merged
merged 5 commits into from
May 27, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
65 changes: 65 additions & 0 deletions extensions/amp-video/0.1/test/test-video-cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,21 @@ describes.realWin('amp-video cached-sources', {amp: true}, (env) => {
'https://example-com.cdn.ampproject.org/mbv/s/example.com/video2.mp4?amp_video_host_url=https%3A%2F%2Fcanonical.com'
);
});

it('should select the video[src] and never the sources children', async () => {
const videoEl = createVideo([
{src: 'video2.mp4'},
{src: 'video3.mp4', type: 'video/mp4'},
]);
videoEl.setAttribute('src', 'video1.mp4');
const xhrSpy = env.sandbox.spy(xhrService, 'fetch');

await fetchCachedSources(videoEl, env.win);

expect(xhrSpy).to.have.been.calledWith(
'https://example-com.cdn.ampproject.org/mbv/s/example.com/video1.mp4?amp_video_host_url=https%3A%2F%2Fcanonical.com'
);
});
});

describe('url forming', () => {
Expand Down Expand Up @@ -145,6 +160,56 @@ describes.realWin('amp-video cached-sources', {amp: true}, (env) => {
expect(addedSources[1].getAttribute('data-bitrate')).to.equal('1500');
expect(addedSources[2].getAttribute('data-bitrate')).to.equal('700');
});

it('should add video[src] as the last fallback source', async () => {
env.sandbox.stub(xhrService, 'fetch').resolves({
json: () =>
Promise.resolve({
sources: [
{'url': 'video1.mp4', 'bitrate_kbps': 700, type: 'video/mp4'},
{'url': 'video2.mp4', 'bitrate_kbps': 2000, type: 'video/mp4'},
{'url': 'video3.mp4', 'bitrate_kbps': 1500, type: 'video/mp4'},
],
}),
});

const videoEl = createVideo([{src: 'video.mp4'}]);
videoEl.setAttribute('src', 'video1.mp4');
videoEl.setAttribute('type', 'video/mp4');

await fetchCachedSources(videoEl, env.win);

const lastSource = videoEl.querySelector('source:last-of-type');
expect(lastSource.getAttribute('src')).to.equal('video1.mp4');
expect(lastSource.getAttribute('type')).to.equal('video/mp4');
});

it('should clear the unused sources when video[src]', async () => {
env.sandbox.stub(xhrService, 'fetch').resolves({
json: () =>
Promise.resolve({
sources: [
{'url': 'video1.mp4', 'bitrate_kbps': 700, type: 'video/mp4'},
{'url': 'video2.mp4', 'bitrate_kbps': 2000, type: 'video/mp4'},
{'url': 'video3.mp4', 'bitrate_kbps': 1500, type: 'video/mp4'},
],
}),
});

const videoEl = createVideo([
{src: 'video.mp4'},
{src: 'video.mp4'},
{src: 'video.mp4'},
{src: 'video.mp4'},
{src: 'video.mp4'},
]);
videoEl.setAttribute('src', 'video1.mp4');

await fetchCachedSources(videoEl, env.win);

const addedSources = videoEl.querySelectorAll('source');
expect(addedSources).to.have.lengthOf(4); // 3 from cache + 1 fallback.
});
});

describe('end to end', () => {
Expand Down
43 changes: 41 additions & 2 deletions extensions/amp-video/0.1/video-cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@

import {Services} from '../../../src/services';
import {addParamsToUrl, resolveRelativeUrl} from '../../../src/url';
import {createElementWithAttributes, matches} from '../../../src/dom';
import {
createElementWithAttributes,
matches,
removeElement,
} from '../../../src/dom';
import {extensionScriptInNode} from '../../../src/service/extension-script';
import {toArray} from '../../../src/core/types/array';
import {user} from '../../../src/log';
Expand All @@ -33,13 +37,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')
gmajoulet marked this conversation as resolved.
Show resolved Hide resolved
gmajoulet marked this conversation as resolved.
Show resolved Hide resolved
)
) {
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))
Expand Down Expand Up @@ -95,3 +103,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}
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

*/
function replaceSrcWithSourceElement(videoEl, win) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genuinely asking: Is it a good idea to prepend maybe on method names if it doesn't necessarily do what it says in the method name? Eg: maybeReplaceSrcWithSourceElement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I added maybe* in a lot of our Stories methods in the past so why not! Not a big deal.

if (!videoEl.hasAttribute('src')) {
return;
}
const sourceEl = win.document.createElement('source');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: We have the createElementWithAttributes if you want to use it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes it a bit harder to not set a type (vs setting it to null or "") if it was not on the amp-video in the first place, I'll keep the current implementation

const srcAttr = videoEl.getAttribute('src');
sourceEl.setAttribute('src', srcAttr);

const typeAttr = videoEl.getAttribute('type');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Don't we also need to remove the type attribute from the videoEl? Let's say amp-video[type] is mp4 but the video cache returns wmv, then the type on amp-video could be wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 src the browser would not consider it for playback, so it shouldn't have any impact.

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 = toArray(videoEl.querySelectorAll('source'));
sourceEls.forEach((el) => removeElement(el));
gmajoulet marked this conversation as resolved.
Show resolved Hide resolved

videoEl.insertBefore(sourceEl, videoEl.firstChild);
}