Fix: content can play too early after contentupdate #318

alex-barstow · 2018-01-19T21:58:37Z

Description

This addresses a couple of issues surrounding contentupdates:

Content can start playing immediately after a source change because we don't proactively pause the player if it is playing following a contentupdate
There is no contentupdate event handler in the 'preroll?' state, so if one occurs we remain in ‘preroll?’ through the source change. And because the ad timeout isn’t cleared, the eventual adtimeout event causes a transition directly into ‘content-playback’, bypassing 'content-set' / 'ads-ready?' / 'ads-ready'

EDIT: We are going to wait to address no.2 in the upcoming refactor

Specific Additions

If we are in 'content-playback' and a contentupdate occurs, pause the player if it is not already paused.
Add a contentupdate() handler in 'preroll?' that changes state to 'content-set'

As mentioned above: we'll hold off on no.2 until the refactor

…e handler in preroll?, reset _pausedOnContentupdate flag on playing

ldayananda · 2018-01-24T19:54:49Z

src/plugin.js

@@ -666,6 +670,10 @@ const contribAdsPlugin = function(options) {
          if (player.paused()) {
            this.state = 'content-set';
          } else {
+            // If we get here we missed the 'play' event, so pause the player so
+            // that content doesn't start playing immediately after the source change
+            player.pause();


I think we may want to pause the player if it's not paused yet and then go to the same state content-set.

If I understand correctly, the reason this else block will go to ads-ready? is that there is an assumption that the play event has been missed. So once the player is paused, we should be in the same state that causes the if clause to change state to content-set

Makes sense to me

From an offline conversation we came to the conclusion that the state transition to ads-ready? is needed still.

ldayananda

I think my main uncertainty is around postroll behavior. If the contentupdate event happens after a postroll, do we have the correct behavior?

Also, this should add some unit tests! 😄

Once that's out of the way I'll be happy to re-review

ldayananda · 2018-01-25T21:16:35Z

test/test.ads.js

@@ -729,6 +729,45 @@ QUnit.test('adsready in content-playback triggers readyforpreroll', function(ass
  assert.strictEqual(spy.getCall(0).args[0].type, 'readyforpreroll', 'readyforpreroll should have been triggered.');
 });

+QUnit.test('contentupdate in preroll? transitions to content-set', function(assert) {
+  assert.expect(2);
+


I believe all these tests should trigger loadstart before play as well, to make sure the player is setup correctly

ldayananda · 2018-01-25T21:22:19Z

test/test.ads.js

+  assert.expect(2);
+
+  this.player.trigger('play');
+  this.player.trigger('adtimeout');


I would just do this.players.ads.skipLinearAdMode() here, or explicitly do startLinearAdMode() and endLinearAdMode() right after.

ldayananda · 2018-01-25T21:25:04Z

test/test.ads.js

+  assert.expect(4);
+
+  this.player.trigger('play');
+  this.player.trigger('adtimeout');


same feedback for this line as the above test

incompl · 2018-01-26T15:00:36Z

src/plugin.js

@@ -435,6 +436,9 @@ const contribAdsPlugin = function(options) {
        },
        nopreroll() {
          this.state = 'content-playback';
+        },
+        contentupdate() {
+          this.state = 'content-set';


There is a loadstart/contentupdate for the initial source. Is there any guarantee that this isn't that event? Specifically, if the play / adsready came in before loadstart.

Good point. I'm not sure there is any such guarantee. We could add a safeguard, something like:

contentupdate() { // only change states if this is not the first loadstart/contentupdate if (player.ads._hasThereBeenALoadStartDuringPlayerLife) { this.state = 'content-set'; } }

You'll need to test it to make sure _hasThereBeenALoadStartDuringPlayerLife is set, it's also set on loadstart so the order of event handlers matters here.

In hindsight, this check won't have any value for subsequent videos in a playlist since _hasThereBeenALoadStartDuringPlayerLife will be true for the life of the player after the first loadstart, as the name suggests :P. Will need to find another way

incompl · 2018-01-26T15:08:31Z

test/test.ads.js

+  this.player.trigger('adsready');
+  assert.strictEqual(this.player.ads.state, 'preroll?');
+  this.player.trigger('contentupdate');
+  assert.strictEqual(this.player.ads.state, 'content-set');


You should avoid tests that assert the state. Firstly, they will break very shortly. Secondly, it is preferable to test things that indicate that the player is functioning correctly. For example, making sure it is paused if it should be paused.

If that's the case, I'll remove these first two tests in their entirety since they are just testing that certain state transitions occur under the proper conditions. Is that what you would recommend in this case?

Makes sense, it does seem like the third test covers the functionality you've implemented here.

incompl · 2018-01-26T15:10:54Z

test/test.ads.js

@@ -729,6 +729,46 @@ QUnit.test('adsready in content-playback triggers readyforpreroll', function(ass
  assert.strictEqual(spy.getCall(0).args[0].type, 'readyforpreroll', 'readyforpreroll should have been triggered.');
 });

+QUnit.test('contentupdate in preroll? transitions to content-set', function(assert) {
+  assert.expect(2);


I recommend avoiding using expect unless your code has assertions in a loop. For simple procedural tests, the expect doesn't add value and requires maintenance if the test is updated.

incompl · 2018-01-26T15:12:44Z

test/test.ads.js

+  sinon.spy(this.player, 'pause');
+  this.player.trigger('contentupdate');
+  assert.strictEqual(this.player.ads.state, 'ads-ready?');
+  assert.ok(this.player.pause.calledOnce, 'player was paused');


The assertions on pause.calledOnce and _pausedOnContentUpdate are good, you can remove the assertions on the state. The test remains just as valuable and would survive the refactor.

incompl · 2018-02-01T22:59:07Z

test/test.ads.js

@@ -729,6 +729,18 @@ QUnit.test('adsready in content-playback triggers readyforpreroll', function(ass
  assert.strictEqual(spy.getCall(0).args[0].type, 'readyforpreroll', 'readyforpreroll should have been triggered.');
 });

+QUnit.test('contentupdate in content-playback transitions to ads-ready? and pauses player if not already paused', function(assert) {


Minor: remove state reference from test name

incompl · 2018-02-01T22:59:36Z

test/test.ads.js

+  sinon.spy(this.player, 'pause');
+  this.player.trigger('contentupdate');
+  assert.ok(this.player.pause.calledOnce, 'player was paused');
+  assert.ok(this.player.ads._pausedOnContentupdate, '_pausedOnContentupdate is true');


Can we also test that the play happens?

incompl · 2018-02-01T23:01:17Z

src/plugin.js

@@ -666,6 +667,10 @@ const contribAdsPlugin = function(options) {
          if (player.paused()) {
            this.state = 'content-set';
          } else {
+            // If we get here we missed the 'play' event, so pause the player so
+            // that content doesn't start playing immediately after the source change
+            player.pause();


What about if contentupdate for the initial source happens during content-playback? This can happen with autoplay.

alex-barstow · 2018-02-05T18:55:38Z

This fix has been moved into the refactor. Closing.

call pause() on contentupdate if not already paused, add contentupdat…

90f5dba

…e handler in preroll?, reset _pausedOnContentupdate flag on playing

ldayananda reviewed Jan 24, 2018

View reviewed changes

alex-barstow added 3 commits January 25, 2018 12:45

added 2 tests

099f217

added one more test, modified existing test

1ee370b

fix confusing phrasing

45cc8f2

ldayananda reviewed Jan 25, 2018

View reviewed changes

make tests clearer, recommended changes

3923e3e

ldayananda approved these changes Jan 25, 2018

View reviewed changes

incompl reviewed Jan 26, 2018

View reviewed changes

alex-barstow added 2 commits January 31, 2018 11:37

code review changes

17ff5b1

fix test

839755a

incompl reviewed Feb 1, 2018

View reviewed changes

alex-barstow closed this Feb 5, 2018

alex-barstow deleted the fix-pause-on-contentupdate branch February 5, 2018 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: content can play too early after contentupdate #318

Fix: content can play too early after contentupdate #318

alex-barstow commented Jan 19, 2018 •

edited

Loading

ldayananda Jan 24, 2018

alex-barstow Jan 24, 2018

ldayananda Jan 24, 2018

ldayananda left a comment

ldayananda Jan 25, 2018

ldayananda Jan 25, 2018

ldayananda Jan 25, 2018

incompl Jan 26, 2018

alex-barstow Jan 26, 2018

incompl Jan 26, 2018

alex-barstow Jan 30, 2018 •

edited

Loading

incompl Jan 26, 2018

alex-barstow Jan 26, 2018 •

edited

Loading

incompl Jan 26, 2018

incompl Jan 26, 2018

incompl Jan 26, 2018

incompl Feb 1, 2018

incompl Feb 1, 2018

incompl Feb 1, 2018

alex-barstow commented Feb 5, 2018

Fix: content can play too early after contentupdate #318

Fix: content can play too early after contentupdate #318

Conversation

alex-barstow commented Jan 19, 2018 • edited Loading

Description

Specific Additions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldayananda 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

alex-barstow Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-barstow Jan 26, 2018 • edited Loading

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

alex-barstow commented Feb 5, 2018

alex-barstow commented Jan 19, 2018 •

edited

Loading

alex-barstow Jan 30, 2018 •

edited

Loading

alex-barstow Jan 26, 2018 •

edited

Loading