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 cache] Append captions track from cache response #37893

Merged
merged 15 commits into from
Mar 23, 2022

Conversation

processprocess
Copy link
Contributor

The video cache will respond with captions_url (not implemented at the time of this PR).
If the response contains captions_url and the video does not already have a track child, a track element will be appended to the video with src and type.

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

This will fail to load on publisher's origins: the captions will load from the cdn.ampproject.org URL, while being on the origin URL, resulting in a CORS error.

I believe you will need to set a crossorigin attribute on the <video tag for it to work. You can test this very easily by setting the <track src= pointing to a different domain.

This however needs to happen much earlier in the code process, as the attribute needs to be present BEFORE we send the video cache request, as we use it to determine whether the video URLs need the ACAO header:

https://github.com/ampproject/amphtml/blob/main/extensions/amp-video/0.1/video-cache.js#L233

@processprocess
Copy link
Contributor Author

I believe you will need to set a crossorigin attribute on the <video tag for it to work.

This however needs to happen much earlier in the code process, as the attribute needs to be present BEFORE we send the video cache request, as we use it to determine whether the video URLs need the ACAO header

Because of this, wouldn't we need to set 'amp_video_require_acao_header': 1 always?

@processprocess processprocess merged commit 9bf7cbd into ampproject:main Mar 23, 2022
wg-stories Sprint automation moved this from In progress to Done Mar 23, 2022
@processprocess processprocess deleted the track branch March 23, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants