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

✨ Add volume attribute to amp-video for use in amp-story-page #36693

Merged
merged 8 commits into from Nov 2, 2021

Conversation

apatronl
Copy link
Contributor

@apatronl apatronl commented Nov 1, 2021

Fixes #36381

Adds support for a new volume attribute in amp-video that is embedded in amp-story-page. Based on the outcome of the design review (#36262), volume has to be in the range [0.1, 1] to avoid loading extra audio bytes that will not be played.

  • Sets video volume when video is unmuted
  • Resets volume to default value of 1 when video is muted, this ensures all videos in the DOM play at the right volume even when re-used across pages

@amp-owners-bot
Copy link

amp-owners-bot bot commented Nov 1, 2021

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

extensions/amp-story/1.0/media-pool.js
extensions/amp-story/1.0/test/validator-amp-story-video-error.html
extensions/amp-story/1.0/test/validator-amp-story-video-error.out

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-story/1.0/test/validator-amp-story-video-error.html
extensions/amp-story/1.0/test/validator-amp-story-video-error.out
extensions/amp-video/validator-amp-video.protoascii

@alanorozco alanorozco requested review from alanorozco and removed request for rbeckthomas November 1, 2021 19:45
Copy link
Member

@alanorozco alanorozco left a comment

Choose a reason for hiding this comment

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

Approved for change in extension directory, though validator file still requires review from @ampproject/wg-caching

extensions/amp-story/1.0/media-pool.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/media-pool.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/media-pool.js Outdated Show resolved Hide resolved
@apatronl apatronl force-pushed the volume branch 2 times, most recently from a802a73 to 369bd39 Compare November 2, 2021 00:06
Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

LGTM for validation, thank you for adding the comment.

@gmajoulet gmajoulet merged commit 8aa4abe into ampproject:main Nov 2, 2021
@gmajoulet gmajoulet added this to In progress in wg-stories Sprint via automation Nov 2, 2021
@gmajoulet gmajoulet moved this from In progress to Done in wg-stories Sprint Nov 2, 2021
rileyajones pushed a commit to rileyajones/amphtml that referenced this pull request Nov 4, 2021
…ject#36693)

* Add volume support in amp-video for amp-story.

* Parse volume string to float.

* Update regex for volume.

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

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

* Add validator test cases.

Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
banaag pushed a commit to banaag/amphtml that referenced this pull request Nov 9, 2021
@banaag banaag mentioned this pull request Nov 9, 2021
banaag added a commit that referenced this pull request Nov 9, 2021
* cl/407238990 Two-way sync for PR #36693. No-op, or fixes merge conflicts, if any.

* Remove extra volume attr introduced in the merge.

* Update tests

* Remove incorrectly updated chromeextension files

Co-authored-by: Devin Mullins <twifkak@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add volume attribute to amp-video for use in amp-story-page
5 participants