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

Register story page level background-audio. #27782

Merged
merged 1 commit into from Apr 16, 2020

Conversation

gmajoulet
Copy link
Contributor

#27366 has an optimization to run the registerAllMedia_() only once. But the background-audio is built and added to the DOM on layoutCallback, which happens after the first call to registerAllMedia_() for pages with distance > 2.

Fixes #27573, #27779

Copy link
Contributor

@Enriqe Enriqe left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

@newmuis
Copy link
Contributor

newmuis commented Apr 15, 2020

Should the background-audio be built on layout instead of build? Can we build an amp-audio instead of an audio so that it's privacy-preserving?

(edit: genuine question, I don't know the answer)

@gmajoulet
Copy link
Contributor Author

If I had to implement this feature today I think my first instinct would be to do it the way you're suggesting. It has many cons though: you'd still have race conditions with prerendering like the one Malte fixed today for videos, our media pool code might run in between the build and layoutCallback. You'd also need to load the amp-audio extension, adding more asynchronous things (and bytes) in the mix.

We can track this work into another ticket if you want us to investigate the pros/cons further! This one is bringing us back to where we were.

@brantgarvey
Copy link

Hi @gmajoulet & @Enriqe, sorry I'm new to Github, so forgive me if I ask silly questions, but I'm exploring with amp and amp stories.

I ran into the problem of having background audio not playing on page 3 of my story and I wanted to know how I can implement the change you have created here?

Here is the project I'm currently working on.

Cheers,
Brant

@gmajoulet
Copy link
Contributor Author

Hi,
This pull request is not yet available on production, as it takes several days to completely roll out. It should be available tomorrow though, you can expect things to start working automatically for your story!
Please let us know if you need anything else.

@brantgarvey
Copy link

Awesome @gmajoulet thanks heaps for your help with this bug!

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.

amp-story-page background audio not playing
6 participants