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

[Video] fallback on origin sources if request to video cache fails #36071

Closed
gmajoulet opened this issue Sep 14, 2021 · 3 comments
Closed

[Video] fallback on origin sources if request to video cache fails #36071

gmajoulet opened this issue Sep 14, 2021 · 3 comments

Comments

@gmajoulet
Copy link
Contributor

Description

When publishers are opted-in for a video cache (amp-video[cache=*]), a XHR is sent to the cache provider, and the XHR response is the buildCallback's response.
If that XHR fails (non 20x HTTP codes), the buildCallback fails, and the video playback fails. That is wrong, and we should use the origin sources for video playback.

Relevant code pointers:

return fetchCachedSources(

export function fetchCachedSources(
videoEl,
ampdoc,
maxBitrate = Number.POSITIVE_INFINITY
) {
const {win} = ampdoc;
// Keep non cached evergreen sources for crawlers.
if (Services.platformFor(win).isBot()) {
return Promise.resolve();
}
if (
!(
videoEl.getAttribute('src') ||
videoEl.querySelector('source[src]')?.getAttribute('src')
)
) {
user().error('AMP-VIDEO', 'Video cache not properly configured');
return Promise.resolve();
}
Services.performanceFor(ampdoc.win).addEnabledExperiment('video-cache');
const {canonicalUrl, sourceUrl} = Services.documentInfoForDoc(win.document);
maybeReplaceSrcWithSourceElement(videoEl, win);
const videoUrl = resolveRelativeUrl(selectVideoSource(videoEl), sourceUrl);
return getCacheUrlService(videoEl, ampdoc)
.then((service) => service.createCacheUrl(videoUrl))
.then((cacheUrl) => {
const requestUrl = addParamsToUrl(cacheUrl.replace(/\/[ic]\//, '/mbv/'), {
'amp_video_host_url':
/* document url that contains the video */ canonicalUrl,
});
return Services.xhrFor(win).fetch(requestUrl, {prerenderSafe: true});
})
.then((response) => response.json())
.then((jsonResponse) =>
applySourcesToVideo(videoEl, jsonResponse['sources'], maxBitrate)
)
.catch(() => {
// If cache fails, video should still load properly.
});
}

Ensuring that fetchCachedSources always resolve, either from amp-video or from video-cache, should work just fine.

Reproduction Steps

Relevant Logs

No response

Browser(s) Affected

No response

OS(s) Affected

No response

Device(s) Affected

No response

AMP Version Affected

No response

@gmajoulet
Copy link
Contributor Author

cc @coreymasanto no idea why I can't assign you

@gmajoulet
Copy link
Contributor Author

Update: this might be breaking up to 3% of video playbacks today. Videos impacted don't even build so I think they might be excluded from our dashboards. I'll run some tests and see if this should be raised to P0.

@gmajoulet
Copy link
Contributor Author

Update: red herring, everything is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

1 participant