Implemented video-interface and autoplay for amp-brid-player #8632

DarXector · 2017-04-07T08:50:45Z

Continuing from #8611

Implemented video-interface and autoplay for amp-brid-player.
Added brid-player to dep check whitelist

aghassemi · 2017-04-11T22:19:58Z

@DarXector Thanks for the PR, I will do a detailed review soon, meanwhile could you please add amp-bird-player to the video-interface integration tests and make sure they pass?

To do so you need to:
1- Add a test to test-video-players.js (similar to amp-youtube that's already there)
2- Include the script in video-players.html

These will run an extensive suite of tests to ensure all the postMessages between the component and the iframe work properly. You can run the tests locally by running gulp test --integration

If you run into test failures, best way to debug manually is to add autoplay attribute on one of the examples that uses your component.

Adding autoplay attribute will instruct AMP runtime to use most of the postMessage video-interface integrations. Your video should start muted auto playing when it comes to view and pause when it goes out of view. Tapping the video should also unmute it.

DarXector · 2017-04-24T08:39:15Z

@aghassemi This has been completed.

aghassemi · 2017-04-25T00:09:25Z

@DarXector could you please rebase your PR (presubmit checks are failing and require a rebase to be fixed due to a backward-incompatible change we made couple of weeks ago)

googlebot · 2017-04-25T14:34:18Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2017-04-25T14:46:54Z

CLAs look good, thanks!

jridgewell · 2017-04-25T18:09:29Z

extensions/amp-brid-player/0.1/amp-brid-player.js

+
+    let feedType = '';
+
+    if (this.element.getAttribute('data-video')) {


#hasAttribute

jridgewell · 2017-04-25T18:09:38Z

extensions/amp-brid-player/0.1/amp-brid-player.js

+
+    if (this.element.getAttribute('data-video')) {
+      feedType = 'video';
+    } else if (this.element.getAttribute('data-playlist')) {


#hasAttribute

jridgewell · 2017-04-25T18:12:14Z

extensions/amp-brid-player/0.1/amp-brid-player.js

+
+    this.playerReadyPromise_.then(() => {
+      if (this.iframe_ && this.iframe_.contentWindow) {
+        const args = typeof opt_arg !== 'undefined' ? '|' + opt_arg : '';


Nit: const args = opt_arg === undefined ? '' : '|' + opt_arg;

jridgewell · 2017-04-25T18:13:27Z

extensions/amp-brid-player/0.1/amp-brid-player.js

+  /** @private */
+  handleBridMessages_(event) {
+
+    if (event.origin.indexOf('services.brid.tv') == -1 ||


Are there subdomains? Could this just be an equality check?

jridgewell · 2017-04-25T18:14:15Z

extensions/amp-brid-player/0.1/amp-brid-player.js

+
+    if (event.origin.indexOf('services.brid.tv') == -1 ||
+        event.source != this.iframe_.contentWindow ||
+        typeof event.data !== 'string' || event.data.indexOf('Brid') == -1) {


The data should start with Brid, right?

DarXector · 2017-04-26T11:01:10Z

@aghassemi @jridgewell I rebased the PR and adopted your observations.

aghassemi

Thanks for adding video-interface support! Few requests and then we can merge after tests pass.

aghassemi · 2017-04-26T15:49:43Z

extensions/amp-brid-player/0.1/test/validator-amp-brid-player.out

-amp-brid-player/0.1/test/validator-amp-brid-player.html:47:2 The mandatory attribute 'data-player' is missing in tag 'amp-brid-player'. (see https://www.ampproject.org/docs/reference/components/media/amp-brid-player) [AMP_TAG_PROBLEM]
-amp-brid-player/0.1/test/validator-amp-brid-player.html:53:2 The tag 'amp-brid-player' is missing a mandatory attribute - pick one of ['data-playlist', 'data-video']. (see https://www.ampproject.org/docs/reference/components/media/amp-brid-player) [AMP_TAG_PROBLEM]
+amp-brid-player/0.1/test/validator-amp-brid-player.html:48:2 The mandatory attribute 'data-player' is missing in tag 'amp-brid-player'. (see https://www.ampproject.org/docs/reference/components/amp-brid-player) [AMP_TAG_PROBLEM]
+amp-brid-player/0.1/test/validator-amp-brid-player.html:55:2 The tag 'amp-brid-player' is missing a mandatory attribute - pick one of ['data-playlist', 'data-video']. (see https://www.ampproject.org/docs/reference/components/amp-brid-player) [AMP_TAG_PROBLEM]


@DarXector Tests seems to be failing due to /media/ being removed here

aghassemi · 2017-04-26T15:54:14Z

extensions/amp-brid-player/0.1/amp-brid-player.js

    this.applyFillContent(iframe);
    this.element.appendChild(iframe);
    this.iframe_ = iframe;
-    return this.loadPromise(iframe);
+
+    this.win.addEventListener(


Please use

this.unlistenMessage_ = listen( this.win, 'message', this. handleBridMessages_.bind(this) );

and define this.unlistenMessage_ in the contractor.

We also need a new unlayoutCallback override that does:

/** @override */ unlayoutCallback() { if (this.iframe_) { removeElement(this.iframe_); this.iframe_ = null; } if (this.unlistenMessage_) { this.unlistenMessage_(); } this.playerReadyPromise_ = new Promise(resolve => { this.playerReadyResolver_ = resolve; }); return true; // Call layoutCallback again. }

aghassemi · 2017-04-26T15:56:12Z

extensions/amp-brid-player/0.1/amp-brid-player.js

-        'Brid|pause',
-        'https://services.brid.tv'
-      );
+      this.pause();


There shouldn't be a need to have the if checks above as sendCommand_ and therefore pause handles that.

aghassemi · 2017-04-26T16:00:57Z

extensions/amp-brid-player/0.1/amp-brid-player.js


+/**
+ * @implements {../../../src/video-interface.VideoInterface}


Since you now implement the video-interface, you will get autoplay for free. Please

Update the validation file to allow for autoplay attribute (attrs: { name: "autoplay" })

Update the documentation MD file with:

**autoplay** If this attribute is present, and the browser supports autoplay: * the video is automatically muted before autoplay starts * when the video is scrolled out of view, the video is paused * when the video is scrolled into view, the video resumes playback * when the user taps the video, the video is unmuted * if the user has interacted with the video (e.g., mutes/unmutes, pauses/resumes, etc.), and the video is scrolled in or out of view, the state of the video remains as how the user left it. For example, if the user pauses the video, then scrolls the video out of view and returns to the video, the video is still paused.

Update the example file to include autoplay and buttons for play, pause, mute, unmute actions. Please see amp-youtube.amp.html in examples for reference.

After doing this I get:

AssertionError: expected 'DISALLOWED_ATTR autoplay amp-brid-player' to equal '' at C:/Users/Reesta/AppData/Local/Temp/e150422a9df00df6e8740f9f1e38f9a5.browserify:190016:38 <- D:/Ampps/www/amphtml/test/integration/test-example-validation.js:118:37

when running test.

@honeybadgerdontcare Do you know what the best approach here is? We are adding a new attribute and using it in an example but looks like test-example-validation uses the deployed validator rather than local. Should we not use the new attribute in an example until validator is deployed?

According to test-example-validation.js there's a todo of using the local validator. Perhaps we should file an issue to use the local validator now that it is open-sourced.

Seems similar to #8859 where a new attribute was added and the integration test had to be skipped after the fact. Honestly I hadn't seen these integration tests till now (cc @Gregable as an fyi).

It may be best to add the integration test after the attribute is in the released validator and that can be tracked in a separate issue (as opposed to what happened with amp-pixel where there are several issues, one to skip then unskip).

Thanks. @DarXector For now let's comment out brid-player.amp.html in test-example-validation and I will add it back when validator is in production.

@aghassemi Done

aghassemi · 2017-04-28T17:57:35Z

@DarXector I tested this manually and only found one issue. unmute does not seem to work. It does send back the unmuted event correctly (hence tests passing as there is no way for tests to verify actual sound) but it does not actually unmute the video. Could you please look into that? (mute works perfectly fine).

Other than that, it looks great!

aghassemi · 2017-04-28T17:59:12Z

extensions/amp-brid-player/0.1/amp-brid-player.js

+    this.win.addEventListener(
+      'message', event => this.handleBridMessages_(event)
+    );
+


please move this.element.appendChild(iframe); here (so it is after the listener)

aghassemi · 2017-04-28T17:59:28Z

extensions/amp-brid-player/0.1/amp-brid-player.js

+      this. handleBridMessages_.bind(this)
+    );
+
+    this.win.addEventListener(


remove this, already listening above.

aghassemi · 2017-04-28T18:19:31Z

@DarXector Find another issue related to a race condition around mute, testing manually I noticed ~30% of the time the first mute call fails to actually mute the video. (verified that it is sent for sure). To reproduce, only keep the autoplay video in the example and keep refreshing, it is supposed to always autoplay muted (otherwise it can't autoplay on mobile) but notice it sometimes autoplays with sound.

mute command is sent as soon as ready is received. Maybe player is firing ready too early before it is actually ready to execute message commands?

DarXector · 2017-04-28T19:55:00Z

@aghassemi Concerning the unmute issue, I've identified the problem with value parsing on the player side. This has been reported to the dev team, and the fix will be released next week.

As for the race issue, I was not able to reproduce this while testing on mobile or desktop environments. What devices/OS/browsers did you use?

aghassemi · 2017-04-28T23:15:50Z

@DarXector I can't repro the race anymore either. Will let you know if I see it again.

aghassemi · 2017-04-28T23:17:00Z

extensions/amp-brid-player/0.1/test/test-amp-brid-player.js

+      timer.promise(50).then(() => {
+        const bridTimerIframe = bc.querySelector('iframe');
+
+        //console.log('getBridPlayer', bridTimerIframe.contentWindow);


please also rebase, there seems to be a git conflict.

Rebased and removed the above code. Also we have pushed a new version with unmute fix.

aghassemi · 2017-04-28T23:18:09Z

@DarXector we can merge this while your team works on fixing unmute. autoplay attribute won't be usable for a few weeks anyway (until our validator goes to prod with the new attribute)

aghassemi · 2017-05-08T23:25:34Z

@DarXector looks good. validator test is failing (related to /media/ in the Urls) otherwise good to go after updating the validator-amp-brid-player.out

amp-brid-player validator fix amp-brid-player added to test-video-players integration test amp-brid-player added to test-video-players integration test amp-brid-player incorporating suggestions from PR ampproject#8632 amp-brid-player validation fixes from PR ampproject#8632 amp-brid-player commented out brid-player.amp.html until validator is in production amp-brid-player removed unnecessary comment amp-brid-player removed extra message event listener amp-brid-player fixed urls in the validator file

DarXector · 2017-05-09T10:47:37Z

@aghassemi Done. But not sure what happened to the amp-brid-player.md, I fixed it on my branch and it's now passing. I also did an interactive rebase.

aghassemi · 2017-05-09T15:42:58Z

@DarXector .md file was okay as it was, please revert back to < from < in the .md and rebase. Thanks.

rsimha · 2017-05-09T15:43:36Z

@DarXector, I've landed a fix for the failing test that complained about amp-brid-player.md, so it should no longer be an issue. The link checker tool was mis-parsing links within script tags, so I've made it warning-only for now. You can ignore the warning and proceed. Sorry!

aghassemi · 2017-05-10T16:01:42Z

@DarXector merged!

aghassemi · 2017-05-10T16:19:41Z

@DarXector I was just playing around with the player, kudos! such a good video player! excellent UI and performance. video-interface implementation is working perfectly too.

I also had a question, does brid player support captions? if so, we are thinking about implementing a feature where captions autoplay as well during muted autoplay ( #6376 ). If implementing this interests you please let me know and I will ping you when the internals are ready (on your end it would most likely be just implementing two new showCaptions, hideCaptions methods)

DarXector · 2017-05-11T12:42:31Z

@aghassemi Thank you for the kind words, we are intent on improving it further.

The player supports captions through a plugin and if the user supplied it through our CMS. So the feature should not be difficult to implement.

On another note, when do you expect for the release with amp-brid-player autoplay/video-interface to go through? We already have partners interested in this feature.

aghassemi · 2017-05-11T16:11:43Z

@DarXector it is currently in Dev Channel (to enable Dev Channel visit https://cdn.ampproject.org/experiments.html) and with Dev Channel enabled, autoplay works ( e.g. https://output.jsbin.com/daxuxewime/quiet ).

For full production, probably ~3 weeks before autoplay attribute validates in AMP Validator.

ampwafflebot added the in progress label Apr 7, 2017

aghassemi self-requested a review April 7, 2017 15:52

aghassemi self-assigned this Apr 7, 2017

DarXector force-pushed the master branch from fc296f2 to 9fb3973 Compare April 25, 2017 14:46

jridgewell reviewed Apr 25, 2017

View reviewed changes

DarXector force-pushed the master branch from 9fb3973 to 6fa580f Compare April 26, 2017 07:55

DarXector added a commit to DarXector/amphtml that referenced this pull request Apr 26, 2017

amp-brid-player incorporating suggestions from PR ampproject#8632

9b5f4d8

aghassemi suggested changes Apr 26, 2017

View reviewed changes

DarXector added a commit to DarXector/amphtml that referenced this pull request Apr 27, 2017

amp-brid-player validation fixes from PR ampproject#8632

769e31b

aghassemi mentioned this pull request Apr 27, 2017

Use local validator for test-example-validation.js #9002

Closed

DarXector force-pushed the master branch from b35156b to 0d8ac4e Compare April 28, 2017 10:01

DarXector added a commit to DarXector/amphtml that referenced this pull request Apr 28, 2017

amp-brid-player incorporating suggestions from PR ampproject#8632

2e8f983

DarXector added a commit to DarXector/amphtml that referenced this pull request Apr 28, 2017

amp-brid-player validation fixes from PR ampproject#8632

cc216a5

aghassemi suggested changes Apr 28, 2017

View reviewed changes

aghassemi reviewed Apr 28, 2017

View reviewed changes

DarXector force-pushed the master branch from 6f099c0 to 3ff3956 Compare May 3, 2017 10:28

DarXector added a commit to DarXector/amphtml that referenced this pull request May 3, 2017

amp-brid-player incorporating suggestions from PR ampproject#8632

a081eb7

DarXector added a commit to DarXector/amphtml that referenced this pull request May 3, 2017

amp-brid-player validation fixes from PR ampproject#8632

761007a

googlebot added the cla: yes label May 3, 2017

DarXector force-pushed the master branch from bf950df to 13784f0 Compare May 8, 2017 13:29

DarXector added a commit to DarXector/amphtml that referenced this pull request May 8, 2017

amp-brid-player incorporating suggestions from PR ampproject#8632

f4b1a9f

DarXector added a commit to DarXector/amphtml that referenced this pull request May 8, 2017

amp-brid-player validation fixes from PR ampproject#8632

1c76a40

DarXector force-pushed the master branch from 13784f0 to 4a09a25 Compare May 9, 2017 09:53

amp-brid-player README file fix

b9cede1

amp-brid-player undo README file fix

e270b3e

aghassemi approved these changes May 10, 2017

View reviewed changes

aghassemi merged commit b22cd57 into ampproject:master May 10, 2017

ampprojectbot added the PR use: In Beta / Experimental label May 11, 2017

ampprojectbot added the PR use: In Stable label May 23, 2017

aghassemi mentioned this pull request Jun 30, 2017

Intent to implement: video-interface and autoplay for amp-brid-player. #8611

Closed


		let feedType = '';

		if (this.element.getAttribute('data-video')) {


		/**
		* @implements {../../../src/video-interface.VideoInterface}

Implemented video-interface and autoplay for amp-brid-player #8632

Implemented video-interface and autoplay for amp-brid-player #8632

Conversation

DarXector commented Apr 7, 2017

aghassemi commented Apr 11, 2017

DarXector commented Apr 24, 2017

aghassemi commented Apr 25, 2017

googlebot commented Apr 25, 2017

googlebot commented Apr 25, 2017

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

DarXector commented Apr 26, 2017

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi Apr 26, 2017 • edited

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

aghassemi commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented Apr 28, 2017 • edited

DarXector commented Apr 28, 2017

aghassemi commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented Apr 28, 2017 • edited

aghassemi commented May 8, 2017

DarXector commented May 9, 2017 • edited

aghassemi commented May 9, 2017

rsimha commented May 9, 2017

aghassemi commented May 10, 2017

aghassemi commented May 10, 2017

DarXector commented May 11, 2017

aghassemi commented May 11, 2017

aghassemi Apr 26, 2017 •

edited

aghassemi commented Apr 28, 2017 •

edited

aghassemi commented Apr 28, 2017 •

edited

DarXector commented May 9, 2017 •

edited