From 4fc40b5615124fbe6369d3ee58adf4a1a0e9241a Mon Sep 17 00:00:00 2001 From: Ale Patron Date: Mon, 25 Oct 2021 21:44:43 -0400 Subject: [PATCH 1/5] Add volume support in amp-video for amp-story. --- extensions/amp-story/1.0/media-pool.js | 22 +++++++++++++++++++ .../amp-video/validator-amp-video.protoascii | 4 ++++ 2 files changed, 26 insertions(+) diff --git a/extensions/amp-story/1.0/media-pool.js b/extensions/amp-story/1.0/media-pool.js index eb3b556d554a..f3e4a8a1f33f 100644 --- a/extensions/amp-story/1.0/media-pool.js +++ b/extensions/amp-story/1.0/media-pool.js @@ -850,6 +850,14 @@ export class MediaPool { return Promise.resolve(); } + // When a video is muted, reset its volume to the default value of 1. + if (mediaType == MediaType.VIDEO) { + let parent = domMediaEl.parentElement; + if (parent) { + domMediaEl.volume = 1; + } + } + return this.enqueueMediaElementTask_(poolMediaEl, new MuteTask()); } @@ -870,6 +878,20 @@ export class MediaPool { return Promise.resolve(); } + if (mediaType == MediaType.VIDEO) { + let parent = domMediaEl.parentElement; + if (parent) { + let volume = parent.getAttribute('volume'); + if (volume) { + domMediaEl.volume = volume; + } + } + + if (parent.getAttribute('noaudio')) { + domMediaEl.volume = 0; + } + } + return this.enqueueMediaElementTask_(poolMediaEl, new UnmuteTask()); } diff --git a/extensions/amp-video/validator-amp-video.protoascii b/extensions/amp-video/validator-amp-video.protoascii index 2327ea1be7f8..e9b960f9d20c 100644 --- a/extensions/amp-video/validator-amp-video.protoascii +++ b/extensions/amp-video/validator-amp-video.protoascii @@ -183,6 +183,10 @@ tags: { # in amp-story name: "captions-id" requires_extension: "amp-story-captions" } + attrs: { + name: "volume" + value_regex: "^(?:0*(?:\.\d+)?|1(\.0*)?)$" + } attr_lists: "extended-amp-global" attr_lists: "amp-video-common" spec_url: "https://amp.dev/documentation/components/amp-video/" From 2bd8317022f06b4a8bf1150a0db3bfb013e6d05a Mon Sep 17 00:00:00 2001 From: Ale Patron Date: Tue, 26 Oct 2021 16:17:16 -0400 Subject: [PATCH 2/5] Parse volume string to float. --- extensions/amp-story/1.0/media-pool.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/extensions/amp-story/1.0/media-pool.js b/extensions/amp-story/1.0/media-pool.js index f3e4a8a1f33f..0d7015a84018 100644 --- a/extensions/amp-story/1.0/media-pool.js +++ b/extensions/amp-story/1.0/media-pool.js @@ -883,13 +883,9 @@ export class MediaPool { if (parent) { let volume = parent.getAttribute('volume'); if (volume) { - domMediaEl.volume = volume; + domMediaEl.volume = Number.parseFloat(volume); } } - - if (parent.getAttribute('noaudio')) { - domMediaEl.volume = 0; - } } return this.enqueueMediaElementTask_(poolMediaEl, new UnmuteTask()); From 6d46cb1b651b4d42598b668de1945222f42962ef Mon Sep 17 00:00:00 2001 From: Ale Patron Date: Mon, 1 Nov 2021 12:36:10 -0400 Subject: [PATCH 3/5] Update regex for volume. --- extensions/amp-video/validator-amp-video.protoascii | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-video/validator-amp-video.protoascii b/extensions/amp-video/validator-amp-video.protoascii index e9b960f9d20c..f4c47706d624 100644 --- a/extensions/amp-video/validator-amp-video.protoascii +++ b/extensions/amp-video/validator-amp-video.protoascii @@ -185,7 +185,7 @@ tags: { # in amp-story } attrs: { name: "volume" - value_regex: "^(?:0*(?:\.\d+)?|1(\.0*)?)$" + value_regex: "^((0?\.[1-9]+)?|1(\.0*)?)$" } attr_lists: "extended-amp-global" attr_lists: "amp-video-common" From 03bc0adc6f78adb01219132f40491a01ff85a318 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandrina=20Patr=C3=B3n?= Date: Mon, 1 Nov 2021 20:01:31 -0400 Subject: [PATCH 4/5] Update extensions/amp-story/1.0/media-pool.js Co-authored-by: Gabriel Majoulet --- extensions/amp-story/1.0/media-pool.js | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/extensions/amp-story/1.0/media-pool.js b/extensions/amp-story/1.0/media-pool.js index 0d7015a84018..f5c35ab1b845 100644 --- a/extensions/amp-story/1.0/media-pool.js +++ b/extensions/amp-story/1.0/media-pool.js @@ -852,12 +852,9 @@ export class MediaPool { // When a video is muted, reset its volume to the default value of 1. if (mediaType == MediaType.VIDEO) { - let parent = domMediaEl.parentElement; - if (parent) { - domMediaEl.volume = 1; - } + domMediaEl.volume = 1; } - + return this.enqueueMediaElementTask_(poolMediaEl, new MuteTask()); } @@ -879,11 +876,11 @@ export class MediaPool { } if (mediaType == MediaType.VIDEO) { - let parent = domMediaEl.parentElement; - if (parent) { - let volume = parent.getAttribute('volume'); + const ampVideoEl = domMediaEl.parentElement; + if (ampVideoEl) { + const volume = ampVideoEl.getAttribute('volume'); if (volume) { - domMediaEl.volume = Number.parseFloat(volume); + domMediaEl.volume = parseFloat(volume); } } } From 4dc16c6fc9b1c78ba8613ca06bdb3e8253b60742 Mon Sep 17 00:00:00 2001 From: Ale Patron Date: Tue, 2 Nov 2021 14:05:09 -0400 Subject: [PATCH 5/5] Add validator test cases. --- .../test/validator-amp-story-video-error.html | 34 ++++++++++++++++ .../test/validator-amp-story-video-error.out | 40 ++++++++++++++++++- .../amp-video/validator-amp-video.protoascii | 2 + 3 files changed, 75 insertions(+), 1 deletion(-) diff --git a/extensions/amp-story/1.0/test/validator-amp-story-video-error.html b/extensions/amp-story/1.0/test/validator-amp-story-video-error.html index 1feb36b34405..a13b0cf6980c 100644 --- a/extensions/amp-story/1.0/test/validator-amp-story-video-error.html +++ b/extensions/amp-story/1.0/test/validator-amp-story-video-error.html @@ -32,6 +32,40 @@ + + + + +
+

Your browser doesn't support HTML5 video.

+
+ + + + + + + + + +
+

Your browser doesn't support HTML5 video.

+
+ + + + + diff --git a/extensions/amp-story/1.0/test/validator-amp-story-video-error.out b/extensions/amp-story/1.0/test/validator-amp-story-video-error.out index ff075aac023f..473a3a32d02e 100644 --- a/extensions/amp-story/1.0/test/validator-amp-story-video-error.out +++ b/extensions/amp-story/1.0/test/validator-amp-story-video-error.out @@ -37,6 +37,44 @@ amp-story/1.0/test/validator-amp-story-video-error.html:23:8 The mandatory attri | | | +| +| +| +| > ^~~~~~~~~ +amp-story/1.0/test/validator-amp-story-video-error.html:38:8 The attribute 'volume' in tag 'amp-video' is set to the invalid value '0'. (see https://amp.dev/documentation/components/amp-video/) +| height="270" +| poster="/foo.jpg" +| src="/video/tokyo.mp4" +| layout="responsive" +| volume="0"> +|
+|

Your browser doesn't support HTML5 video.

+|
+| +| +| +| +| +| +| +| +| > ^~~~~~~~~ +amp-story/1.0/test/validator-amp-story-video-error.html:55:8 The attribute 'volume' in tag 'amp-video' is set to the invalid value '1.01'. (see https://amp.dev/documentation/components/amp-video/) +| height="270" +| poster="/foo.jpg" +| src="/video/tokyo.mp4" +| layout="responsive" +| volume="1.01"> +|
+|

Your browser doesn't support HTML5 video.

+|
+| +| +| +| +| | | -| +| \ No newline at end of file diff --git a/extensions/amp-video/validator-amp-video.protoascii b/extensions/amp-video/validator-amp-video.protoascii index f4c47706d624..601ebd0c61a8 100644 --- a/extensions/amp-video/validator-amp-video.protoascii +++ b/extensions/amp-video/validator-amp-video.protoascii @@ -185,6 +185,8 @@ tags: { # in amp-story } attrs: { name: "volume" + # Disallow 0 to prevent shipping unnecessary audio track when the video is muted. + # Volume must be in the range [0.1, 1]. value_regex: "^((0?\.[1-9]+)?|1(\.0*)?)$" } attr_lists: "extended-amp-global"