Implemented Media Session API for video components by wassgha · Pull Request #10476 · ampproject/amphtml

wassgha · 2017-07-17T18:34:33Z

This implements the Media Session API inside the video manager as described in #7272

Changes

Added a new variable that players implementing the video-interface should expose : metaData which contains posterUrl (512x512 png image), title, album, artist.
Implemented guessing logic to fill in missing information from metaData (see to-do)
Update Media Session using gathered and guessed metaData when the video loads as well as when the video starts playing (so that we override the media session when switching between videos on the same page)
Added opt-out method preimplementsMediaSession for players already implementing the Media Session API (amp-youtube for example) so that we don't override their implementation
Implemented the preimplementsMediaSession on amp-youtube to opt-out
Added multiple parsers for album artwork in this order:

poster attribute

schema for NewsArticle usually contains a thumbnail

meta images (og:image in particular if available)

favicon

Deciding whether these other sources might be useful:

First image in the body of the document

first frame of the video

Excluded players list

List of video players that have a good implementation of the Media Session API already built-in.

amp-vimeo
amp-youtube

To-do

Determine which players may benefit from an opt-out like amp-youtube
Implement seek/next/previous track

Closes #7272 , Checks off an item on #4154

aghassemi · 2017-07-18T19:40:34Z

src/video-interface.js

+  /**
+   * Returns video's meta data (poster, artist, album, etc.)
+   * @return {!VideoMetaDef} metadata
+   */


getMetaData() for consistency

aghassemi · 2017-07-18T19:40:59Z

src/video-interface.js

 export let VideoAnalyticsDetailsDef;
+
+/**
+ * @typedef {{


define on top of the file

aghassemi · 2017-07-18T19:42:22Z

src/video-interface.js

   */
  hideControls() {}

+  /**


mention it will be used for session API

aghassemi · 2017-07-18T19:44:11Z

src/service/video-manager-impl.js

+    }
+
+  }
+


let's not do default. it doesn't really add value and just makes the JS files bigger

aghassemi · 2017-07-18T19:46:27Z

src/video-interface.js

+   * inferred meta-data to update the video's Media Session notification.
+   *
+   * @return {boolean}
+   */


preimplementsMediaSession sounded better

aghassemi · 2017-07-18T19:52:32Z

src/service/video-manager-impl.js

+        album: this.metaData_.album,
+        artwork: [
+          {
+            src: this.metaData_.posterUrl,


do they scale properly?

aghassemi · 2017-07-18T19:53:38Z

src/service/video-manager-impl.js

+        ],
+      });
+
+      navigator.mediaSession.setActionHandler('play', function() {


play(/*isAutoplay*/ false)

aghassemi · 2017-07-18T19:56:52Z

src/service/video-manager-impl.js

+    if (!metaData) {
+      metaData = {};
+    }
+    if (!metaData.artist) {


we should not mutate their object. Use map() from object.js to create a map using their object as initial data and then mutate (and cache) the map

aghassemi · 2017-07-18T19:58:40Z

src/service/video-manager-impl.js

+    }
+    if (!metaData.artist) {
+      metaData.artist = 'No artist';
+    }


poster is very amp-video specific. We should just expect poster to be in meta data, manager can't do anything reasonable without it (except for a default which I argue we should not have)

(oh unless do fallback to AMP specific thumbnail in the schema.org metadata and then to the favicon)

aghassemi · 2017-07-18T20:12:22Z

src/service/video-manager-impl.js

+                       || this.internalElement_.getAttribute('aria-label')
+                       || this.ampdoc_.win.document.title;
+    }
+    if (!metaData.album) {


what happens if we don't set it? we don't have good localization story for bultin text like "no album" yet.

wassgha · 2017-07-30T05:35:11Z

@aghassemi should be ready for a final review :) . If we want to implement next/previous from lightbox 2.0 in the same PR, we need to talk since I don't have much information about that.

aghassemi · 2017-07-31T18:27:29Z

extensions/amp-3q-player/0.1/amp-3q-player.js

  }

+  /** @override */
+  getMetaData() {


let's allow these to be just

getMetadata() { /// Not implemented }

and handle undefined/null in the video manager

aghassemi · 2017-07-31T18:29:29Z

src/mediasession-helper.js

+ * Updates the Media Session API's metadata
+ * @param {!./ampdoc-impl.AmpDoc} ampdoc
+ * @param {!./video-interface.VideoMetaDef} metaData
+ * @param {function} playHandler


function()=

aghassemi · 2017-07-31T18:33:58Z

src/mediasession-helper.js

+    // Clear mediaSession (required to fix a bug when switching between two
+    // videos)
+    navigator.mediaSession.metadata = new win.MediaMetadata({
+      title: '',


nit: as a constant variable defined outside and typed as video-interface.VideoMetaDef

aghassemi · 2017-07-31T18:35:01Z

src/mediasession-helper.js

+  const schema = doc.querySelector('script[type="application/ld+json"]');
+  if (!schema) {
+    // No schema element found
+    return undefined;


nit: just return;

aghassemi · 2017-07-31T18:36:54Z

src/mediasession-helper.js

+
+  // Image definition in schema could be one of :
+  if (schemaJson.image['@list']
+      && schemaJson.image['@list'][0]


undefined/null check is already covered by the typeof check

aghassemi · 2017-07-31T18:42:49Z

src/mediasession-helper.js

+      ]
+    });
+    // Add metaData
+    navigator.mediaSession.metadata = new win.MediaMetadata(metaData);


nit: do a sweep of variable naming for this PR so we always do (M/m)etadata instead of (M/m)etaData to match native naming.

aghassemi · 2017-07-31T18:46:56Z

extensions/amp-video/0.1/amp-video.js


+  /** @override */
+  getMetaData() {
+    return {


amp-video should do the lookups for poster instead of manager.

aghassemi · 2017-07-31T18:47:21Z

src/service/video-manager-impl.js

+    }
+
+    if (!this.metaData_.artwork || this.metaData_.artwork.length == 0) {
+      const posterUrl = this.video.element.getAttribute('poster')


ditto comment about poster being the responsibility of amp-video

aghassemi · 2017-07-31T18:49:34Z

src/service/video-manager-impl.js

+
+    if (!this.metaData_.title) {
+      const title = this.video.element.getAttribute('title')
+                    || this.video.element.getAttribute('aria-label')


probably should not look into internalElement (there shouldn't be any cases where inner element has has but not the custom element wrapper of it)

aghassemi · 2017-07-31T18:50:05Z

src/video-interface.js

 import {ActionTrust} from './action-trust'; /* eslint no-unused-vars: 0 */

+/**
+ * @typedef {{


artwork is array right?

aghassemi · 2017-08-01T21:17:31Z

@erwinmombay for the addEventListener extern, see google/closure-compiler#1959 and https://github.com/google/closure-compiler/blob/master/contrib/externs/w3c_eventsource.js it is supposed to be fixed already by closure compiler, an upgrade might be simpler than doing our own override?

aghassemi · 2017-08-01T22:23:46Z

src/mediasession-helper.js

+    // 1. "image": "http://..",
+    return schemaJson['image'];
+  } else if (schemaJson['image']['@list']
+      && schemaJson['image']['@list'][0]


ditto: unnecessary check here and 2 more places below

Didn't pass when I removed them :(

aghassemi · 2017-08-01T22:28:01Z

src/video-interface.js


+  /**
+   * Returns video's meta data (artwork, title, artist, album, etc.) for use
+   * with the Media Session API


array for the artwork

…ter default posters

wassgha added the Needs UX label Jul 17, 2017

googlebot added the cla: yes label Jul 17, 2017

wassgha force-pushed the media-session branch from f3adf5d to 32d8ffa Compare July 17, 2017 18:36

wassgha requested a review from aghassemi July 17, 2017 18:45

wassgha force-pushed the media-session branch 6 times, most recently from 6cb9cfc to e47397c Compare July 18, 2017 00:12

aghassemi suggested changes Jul 18, 2017

View reviewed changes

wassgha force-pushed the media-session branch 2 times, most recently from 5c90a36 to dbadc6b Compare July 24, 2017 19:46

wassgha force-pushed the media-session branch 5 times, most recently from a494d11 to 22cfa6c Compare July 30, 2017 05:31

wassgha force-pushed the media-session branch from 22cfa6c to 81741d0 Compare July 30, 2017 05:39

aghassemi suggested changes Jul 31, 2017

View reviewed changes

wassgha force-pushed the media-session branch 7 times, most recently from 5e95027 to 8221fd2 Compare August 1, 2017 16:42

wassgha force-pushed the media-session branch from a9ca2f6 to bd61f95 Compare August 1, 2017 21:32

aghassemi reviewed Aug 1, 2017

View reviewed changes

aghassemi approved these changes Aug 1, 2017

View reviewed changes

wassgha added 6 commits August 1, 2017 16:27

Implemented metaData and media session for amp-video

a7bb6ee

Started working on requested changes

429dee4

Implemented metadata and mediasession api for all players & added bet…

d25e085

…ter default posters

More requested changes

7e142ec

Created media session helper and implemented tests

acc0054

Requested changes for MediaSessionAPI implementation

ca3ff51

wassgha force-pushed the media-session branch from bd61f95 to 2f011f6 Compare August 1, 2017 23:29

Fixed test

588aeb1

wassgha force-pushed the media-session branch from 2f011f6 to 588aeb1 Compare August 1, 2017 23:30

aghassemi merged commit 3660192 into ampproject:master Aug 2, 2017

wassgha deleted the media-session branch August 2, 2017 00:28

wassgha mentioned this pull request Aug 2, 2017

Added album & artist attributes and MediaSession example to amp-video #10757

Merged

wassgha removed the Needs UX label Aug 4, 2017

ampprojectbot added the PR use: In Beta / Experimental label Aug 10, 2017

Conversation

wassgha commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Excluded players list

To-do

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wassgha commented Jul 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aghassemi Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wassgha commented Jul 17, 2017 •

edited

Loading

aghassemi commented Aug 1, 2017 •

edited

Loading

aghassemi Aug 1, 2017 •

edited

Loading