Skip to content

✨ [video cache] Append captions track from cache response#37893

Merged
processprocess merged 15 commits intoampproject:mainfrom
processprocess:track
Mar 23, 2022
Merged

✨ [video cache] Append captions track from cache response#37893
processprocess merged 15 commits intoampproject:mainfrom
processprocess:track

Conversation

@processprocess
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants