Add audio support for amp-story #11509

newmuis · 2017-09-29T22:03:44Z

This is utility class to help manage playback of audio for the amp-story component.

dvoytenko

Bunch of stylistic comments. Deferring main logic review to @aghassemi . However, should there be tests here?

dvoytenko · 2017-10-02T20:28:07Z

extensions/amp-story/0.1/audio.js

+
+
+/** @typedef {{ source: AudioBufferSourceNode, gainNode: GainNode }} */
+let AudioSource;


Should be suffixed with Def

Removed entirely; it's actually unused now.

dvoytenko · 2017-10-02T20:28:25Z

extensions/amp-story/0.1/audio.js

+const VOLUME_CHANGE_DURATION_MS = 500;
+
+/**
+ * @const {!function(number): number}


dvoytenko · 2017-10-02T20:29:15Z

extensions/amp-story/0.1/audio.js

+ * @const {string}
+ */
+const PLAYABLE_ID_PREFIX = 'i-amphtml-playable-';
+


nit: remove extra line. Just one-line separator b/w constants.

dvoytenko · 2017-10-02T20:29:34Z

extensions/amp-story/0.1/audio.js

+
+
+export class AudioManager {
+  constructor(win, rootElement) {


jsdoc for args

dvoytenko · 2017-10-02T20:37:54Z

extensions/amp-story/0.1/audio.js

+   *     represented by the specified sourceElement.
+   */
+  createPlayable_(sourceElement) {
+    if (!(sourceElement instanceof Element)) {


User assertElement

dvoytenko · 2017-10-02T20:41:56Z

extensions/amp-story/0.1/audio.js

+  }
+
+
+  nowPlayingChanged_() {


dvoytenko · 2017-10-02T20:42:06Z

extensions/amp-story/0.1/audio.js

+
+
+class Playable {
+  constructor(win, sourceElement) {


dvoytenko · 2017-10-02T20:42:18Z

extensions/amp-story/0.1/audio.js

+     */
+    this.sourceElement_ = sourceElement;
+
+    this.depth_ = Playable.calculateDepthForElement(sourceElement);


dvoytenko · 2017-10-02T20:42:45Z

extensions/amp-story/0.1/audio.js

+ * An HTMLMediaElement that potentially has audio.
+ */
+class MediaElementPlayable extends Playable {
+  constructor(win, element) {


Deferred to super constructor

dvoytenko · 2017-10-02T20:42:51Z

extensions/amp-story/0.1/audio.js

+class MediaElementPlayable extends Playable {
+  constructor(win, element) {
+    super(win, element);
+    this.element_ = element;


Deferred to sourceElement in super class

aghassemi · 2017-10-02T21:24:29Z

extensions/amp-story/0.1/audio.js

+/**
+ * @const {string}
+ */
+const PLAYABLE_ID_PREFIX = 'i-amphtml-playable-';


nit: maybe i-amphtml-playable-audio- to account for other playable types later given nextId_ is scoped to audio.js only?

aghassemi · 2017-10-02T21:28:57Z

extensions/amp-story/0.1/audio.js

+export function upgradeBackgroundAudio(element) {
+  if (element.hasAttribute('background-audio')) {
+    const audioEl = element.ownerDocument.createElement('audio');
+    const audioSrc = element.getAttribute('background-audio');


do one of assertHttpsUrl or assertAbsoluteHttpOrHttpsUrl on audioSrc depending on your requirements.

aghassemi · 2017-10-02T21:29:45Z

extensions/amp-story/0.1/audio.js

+    audioEl.setAttribute('src', audioSrc);
+    audioEl.setAttribute('preload', 'auto');
+    audioEl.setAttribute('loop', '');
+    audioEl.setAttribute('autoplay', '');


is there a point in adding autoplay given mobile ignores it and desktop will soon ignore it too?

They only ignore it if it's unmuted, right? Added muted attribute.

not sure what they do for "audio", autoplay for muted audio is sort of strange although you may get some preloading advantages by using autoplay attr? regardless, this looks fine to me.

aghassemi · 2017-10-02T21:32:57Z

extensions/amp-story/0.1/audio.js

+   */
+  load(sourceElement) {
+    const playable = this.getPlayable_(sourceElement) ||
+        this.createPlayable_(sourceElement);


nit: alternative pattern that I slightly prefer : getOrCreatePlayable_

aghassemi · 2017-10-02T21:35:42Z

extensions/amp-story/0.1/audio.js

+
+  /**
+   * @param {!Element} sourceElement The element causing audio to be played.
+   * @return {!Playable} The {@link Playable} instance to play the audio


!Playable is not correct since this can return undefined. How about making it throw (by using dev().assert) if sourceElement is not Element or HTMLMediaElement?

Done. It actually has to be Element AND HTMLMediaElement, but since HTMLMediaElement implies Element, I only checked for HTMLMediaElement.

aghassemi · 2017-10-02T21:36:50Z

extensions/amp-story/0.1/audio.js

+  /**
+   * Loads the audio for the specified.
+   * @param {!Element} sourceElement The element whose audio should be loaded.
+   */


document return param

aghassemi · 2017-10-02T21:37:59Z

extensions/amp-story/0.1/audio.js

+  }
+
+  /** @private */
+  getMediaElementChildren_(element) {


doesn't seem to be used in this file (and is private) is used outside, make public

aghassemi · 2017-10-02T21:40:55Z

extensions/amp-story/0.1/audio.js

+   * @private
+   */
+  addToNowPlaying_(playable) {
+    const index = this.nowPlaying_.indexOf(playable);


nit: .includes

aghassemi · 2017-10-02T21:44:32Z

extensions/amp-story/0.1/audio.js

+      prioritiesByDepth[depth] = depthCount - iteration;
+    });
+
+    // Set volumes on each of the playables that is currently playing, based on


aghassemi · 2017-10-02T21:46:40Z

extensions/amp-story/0.1/audio.js

+  }
+
+  /** @override */
+  setVolume(volume, durationMs, unusedEasingFn) {


Let's document somewhere the iOS's restriction on setting volume and how that's handled.

newmuis · 2017-10-03T16:29:59Z

Filed #11546 to track the unit tests.

Comments addressed

newmuis added 2 commits September 29, 2017 17:58

Add audio support for amp-story.

6c0bb9d

Move imports below copyright

2d59539

newmuis requested review from dvoytenko and aghassemi September 29, 2017 22:03

googlebot added the cla: yes label Sep 29, 2017

This was referenced Sep 29, 2017

Send PRs to merge amphtml-story into the amphtml repo newmuis/amphtml-story#180

Closed

Add amp-story-page element #11510

Merged

Add the core amp-story component #11511

Merged

newmuis added this to Merge in Stories - By Type Oct 2, 2017

Update audio.js

c06c76a

newmuis requested a review from dreamofabear October 2, 2017 20:42

dvoytenko previously requested changes Oct 2, 2017

View reviewed changes

newmuis moved this from Awaiting Review to Needs Work in Stories - By Type Oct 2, 2017

aghassemi suggested changes Oct 2, 2017

View reviewed changes

Update audio.js

cd38bd7

ampprojectbot added this to the Pending Triage milestone Oct 2, 2017

newmuis added 4 commits October 3, 2017 08:36

Update audio.js

36ed110

Require HTTPS URLs for audio.js

1375eef

Update audio.js

f81d887

Fix lint errors and return statement

f923add

newmuis moved this from Needs Work to Awaiting Review in Stories - By Type Oct 3, 2017

aghassemi approved these changes Oct 3, 2017

View reviewed changes

newmuis moved this from Awaiting Review to Ready to Merge (approved + comments addressed) in Stories - By Type Oct 3, 2017

newmuis mentioned this pull request Oct 3, 2017

Add tests for amp-story AudioManager #11546

Closed

alanorozco merged commit c4bee20 into ampproject:master Oct 3, 2017

newmuis moved this from Ready to Merge (approved + comments addressed) to Merged in Stories - By Type Oct 3, 2017

newmuis moved this from Migration to amphtml repo to Done in Stories - By Type Jan 10, 2018



		/** @typedef {{ source: AudioBufferSourceNode, gainNode: GainNode }} */
		let AudioSource;

Add audio support for amp-story #11509

Add audio support for amp-story #11509

Conversation

newmuis commented Sep 29, 2017 • edited

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newmuis commented Oct 3, 2017

newmuis commented Sep 29, 2017 •

edited