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

Simplify placeholders for amp-story MediaPool #12865

Closed
newmuis opened this issue Jan 17, 2018 · 5 comments
Closed

Simplify placeholders for amp-story MediaPool #12865

newmuis opened this issue Jan 17, 2018 · 5 comments

Comments

@newmuis
Copy link
Contributor

newmuis commented Jan 17, 2018

Currently, we use the original video elements in the DOM as placeholders that are then swapped out when the page containing the media is (pre)loaded. We could, instead, use a simpler element (e.g. an image for videos, or a div for audio) to save resources and have fewer media elements in the page.

This would also prevent the browser from trying to keep placeholder media in memory, as only media allocated by MediaPool should consume substantial resources.

@newmuis
Copy link
Contributor Author

newmuis commented Jan 17, 2018

We can do this swap when we register the element; all information needed to play the media should be present at that time.

@newmuis
Copy link
Contributor Author

newmuis commented Jan 22, 2018

I think this may be able to be used to solve #12728 and #12878, so I'll begin investigating.

@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @newmuis Do you have any updates?

@newmuis
Copy link
Contributor Author

newmuis commented Jun 5, 2018

This is another bug that is the same root cause as #12728, so I am going to merge it in and make that the master issue.

Duplicate of #12728

@rustycourses
Copy link

rustycourses commented Nov 14, 2018

I am still getting an error form this with stories that have more than 15/16 video clips.

The error appears in the console when you get to the 15/16 video clip: Cannot read property 'classList' of undefined_

screenshot 2018-11-14 at 19 15 09

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

No branches or pull requests

3 participants