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

[Story mediapool] Reused videos with gain node stay muted after reuse #38206

Closed
mszylkowski opened this issue May 17, 2022 · 2 comments · Fixed by #38208
Closed

[Story mediapool] Reused videos with gain node stay muted after reuse #38206

mszylkowski opened this issue May 17, 2022 · 2 comments · Fixed by #38208
Assignees

Comments

@mszylkowski
Copy link
Contributor

mszylkowski commented May 17, 2022

Description

When the mediapool changes the volume of a video, it connects a gainNode to createMediaElementSource to distort the audio coming from the video. However, the gainNode and MediaElementSource need to be removed from the video before it's reused by another amp-video. Currently, reusing the mediapool video element will keep it silent.

Eg: a story with 8 pages that have noaudio videos (that are muted) will initialize 8 mediapool videos that are all muted. All videos on pages 9+ will be muted because they reuse the same sources, and they were never unmuted.

Reproduction Steps

Go to https://stories.nws.ai/1288496525/what-makes-content-good-copy-only-mp4-2/ and unmute the story, then advance until the 3rd to last video shows up (page with Andrew Robertson) and it will not be audible. The error MediaElementAudioSource outputs zeroes due to CORS access restrictions for <video URL> is thrown on the console.

Relevant Logs

MediaElementAudioSource outputs zeroes due to CORS access restrictions for <video URL>

I don't believe CORS is the cause of muting because the unmuted videos shouldn't have the createMediaElementSource at all. This error could make videos with volume=x mute when in cross origin. Also when reloading the page, those videos play properly. However, MediaElementAudioSource should not process the audio from videos with audio (and no volume attr)

Browser(s) Affected

Chrome

OS(s) Affected

Mac, others?

Device(s) Affected

Macbook, others?

AMP Version Affected

latest

@newmuis
Copy link
Contributor

newmuis commented May 17, 2022

Should be able to reset the gain node in amp-video's resetOnDomChange

@mszylkowski
Copy link
Contributor Author

mszylkowski commented May 17, 2022

Should be able to reset the gain node in amp-video's resetOnDomChange

I would not implement it in AmpVideo.resetOnDomChange since the volume feature is implemented in the media-pool, but it would be fine in MediaPool.maybeResetAmpMedia. However, the real issue is that the gain nodes that were set up on a given video can't be reused on another CORS video without the proper attributes and headers, so the video on page 10 that doesn't have the crossorigin attribute or headers will fail to play audio (due to the log on this bug). I'll note that disconnecting the gain node and making the video element output audio normally is impossible as per the API: WebAudio/web-audio-api#1202 once you create a MediaElementSource you can't remove it from the HTMLMediaElement.

My suggestion:

Prevent noaudio attribute from using gain nodes, which gets rid this bug on Google video cached videos and other stories that use the noaudio indicator on videos that don't have audio. As a best effort we can reset the gain nodes when resetting the video sources (only if they have a gain node attached), but creators can also use volume=1 to reset the volume to 1.

noaudio can be implemented by not unmuting the video at all.

This won't fix stories that have the volume=x attribute on amp-videos, but on those stories they can explicitly add crossorigin attribute on all amp-videos and fix this (for the videos that are served on a different origin).

Alternative:

Force all Google cached videos to have the crossorigin attribute, and reset the gain node to 1. This won't fix stories that set noaudio manually on videos, which is why I don't want to do this.

Alternative:

Keep separate video pools for videos attached to gain nodes and videos that play the audio directly. It's clear we shouldn't do this due to complexity and headaches.

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

Successfully merging a pull request may close this issue.

2 participants