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] Fix videos from mediapool with noaudio don't have audio when reused. #38216

Merged
merged 27 commits into from
May 19, 2022

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented May 19, 2022

Part of #38206 to be cherrypicked (full fix in #38208).

Given that the gain nodes don't work well with badly annotated videos missing crossorigin, we don't want to use the gain node for muting videos (we only want to use gain nodes for volume=x attr)

@mszylkowski mszylkowski self-assigned this May 19, 2022
@amp-owners-bot
Copy link

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/media-pool.js

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.

Does this fix the case where an element is used once with volume="0.1" and then re-used with no volume attr?
For the reused element, expected volume would be 1, but it'd keep the previous volume

extensions/amp-story/1.0/media-pool.js Outdated Show resolved Hide resolved
Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
@mszylkowski
Copy link
Contributor Author

Does this fix the case where an element is used once with volume="0.1" and then re-used with no volume attr? For the reused element, expected volume would be 1, but it'd keep the previous volume

To me that's not P0 given the small number of people that use the attribute volume, was planning to fix on #38208

@mszylkowski mszylkowski merged commit 83c3690 into ampproject:main May 19, 2022
@mszylkowski mszylkowski deleted the unmute_noaudio_cp branch May 19, 2022 17:49
westonruter added a commit that referenced this pull request May 20, 2022
…a-menu-images-validator-spec

* 'main' of github.com:ampproject/amphtml: (90 commits)
  🔥 [Story mediapool] Fix videos from mediapool with `noaudio` don't have audio when reused. (#38216)
  Hide progress bar on the control group of auto advance experiment (#38215)
  ✨ Add Bento Autocomplete Component (#37837)
  🌐 [Story subscription] Subscription localization async (#38204)
  Dable: add new optional parameter "channel" (#38199)
  ✨ [AMP Story Paywall] Enable developers to specify a custom subscriptions page index (#38175)
  SwG Release 0.1.22.217 (#38187)
  amp-script: implements new size limits for sandboxed scripts (#38185)
  🖍  Hide the system layer and progress bar in preview mode (#38163)
  added minItems (#38177)
  Prevent expandTemplate from ReDOSing (#38178)
  Change amp-story-subscriptions attribute name to reflect its flexibility (#38176)
  🐛 [Story Preview] Enable amp-video to play in preview mode (#38149)
  Added the possibility to get page count to story messaging api (#38170)
  SwG Release 0.1.22.216 (#38168)
  Allow @newmuis to update OWNERS files (#38169)
  ✨ Add Richaudience to RTC callout vendors (#38160)
  🚀  SunMedia: Update amp-ad (#38128)
  Remove option to deploy PR artifacts to a static website (#38152)
  added some vars and requests in gfksensic.json (#37722)
  ...
ampprojectbot pushed a commit that referenced this pull request May 20, 2022
…ve audio when reused. (#38216)

* Added tasts

* Undo

* When noaudio, stop using gain nodes

* Update extensions/amp-story/1.0/media-pool.js

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
(cherry picked from commit 83c3690)
ampprojectbot pushed a commit that referenced this pull request May 20, 2022
…ve audio when reused. (#38216)

* Added tasts

* Undo

* When noaudio, stop using gain nodes

* Update extensions/amp-story/1.0/media-pool.js

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
(cherry picked from commit 83c3690)
ampprojectbot pushed a commit that referenced this pull request May 20, 2022
…ve audio when reused. (#38216)

* Added tasts

* Undo

* When noaudio, stop using gain nodes

* Update extensions/amp-story/1.0/media-pool.js

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
(cherry picked from commit 83c3690)
ampprojectbot pushed a commit that referenced this pull request Jun 10, 2022
…ve audio when reused. (#38216)

* Added tasts

* Undo

* When noaudio, stop using gain nodes

* Update extensions/amp-story/1.0/media-pool.js

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
(cherry picked from commit 83c3690)
ampprojectbot pushed a commit that referenced this pull request Jun 10, 2022
…ve audio when reused. (#38216)

* Added tasts

* Undo

* When noaudio, stop using gain nodes

* Update extensions/amp-story/1.0/media-pool.js

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
(cherry picked from commit 83c3690)
ampprojectbot pushed a commit that referenced this pull request Jun 10, 2022
…ve audio when reused. (#38216)

* Added tasts

* Undo

* When noaudio, stop using gain nodes

* Update extensions/amp-story/1.0/media-pool.js

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
(cherry picked from commit 83c3690)
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.

None yet

3 participants