feat: Add Play middleware to avoid calling play on tech #337

ldayananda · 2018-02-28T21:53:50Z

Limitations: can't be used on mobile platforms and is only available for use on later versions of videojs.

Implements a terminating play middleware.
Removes special adplaying handling
Reworks the "fast track" cases where there is no preroll ad to first advance to Preroll state and contentResuming mode
Simplify logic to determine whether a manual play is required after resuming to content
Further modularizes cancelContentPlay and removes 1ms timeout

Todos:

write integration tests for play middleware
finalize mocking for unit tests for play middleware
more negative test cases in unit tests

incompl · 2018-03-05T22:49:34Z

src/plugin.js

@@ -315,6 +325,7 @@ const contribAdsPlugin = function(options) {
  };

  player.ads._state = new BeforePreroll(player);
+  player.ads._state.init(player);


Ah interesting, was this causing an issue or was it just something you noticed?

Mostly something I noticed, I wanted to set terminate at the earliest point possible in BeforePreroll but felt putting it in the constructor wasn't right

Yup makes total sense.

incompl · 2018-03-05T22:51:52Z

src/states/BeforePreroll.js

@@ -30,7 +33,10 @@ export default class BeforePreroll extends ContentState {
    player.ads.debug('Received play event (BeforePreroll)');

    // Don't start content playback yet
-    cancelContentPlay(player);
+    if (!isMiddlewareMediatorSupported()) {


Maybe you already planned on doing this, but the next step is to move this logic into cancelContentPlay.js. That is, always cancelContentPlay on play if !isMiddlewareMediatorSupported. Then all the states have to do is set _shouldBlockPlay.

incompl · 2018-03-21T21:55:37Z

migration-guides/migrating-to-6.0.md

@@ -18,6 +18,7 @@ by integrations.
 * adTimeoutTimeout has been removed. It was not part of the documented interface, but make note if your integration inspected it.
 * There is no longer a snapshot object while checking for postrolls. Now a snapshot is only taken when a postroll ad break actually begins.
 * The `contentplayback` event (removed in [4.0.0](https://github.com/videojs/videojs-contrib-ads/blob/cc664517aa0d07398decc0aa5d41974330efc4e4/CHANGELOG.md#400), re-added as deprecated in [4.1.1](https://github.com/videojs/videojs-contrib-ads/blob/cc664517aa0d07398decc0aa5d41974330efc4e4/CHANGELOG.md#411)), has been removed. Use the `playing` event instead.
+* The `adplaying` behavior is an implementation detail and has changed in this update. The `adplaying` event is no longer guaranteed to happen once per ad break. It is not intended to be used to detect the beginning of an ad break. The `ads-pod-started` event should be used instead. The `ads-ad-started` event can be used to detect the start of an individual ad in an ad break. There will be multiple `ads-ad-started` events corresponding to each ad in the ad break.


incompl · 2018-03-21T21:59:08Z

src/cancelContentPlay.js

+    // Don't use cancelContentPlay while playMiddleware is in use
+    return;
+  } else if (player.ads._shouldBlockPlay === false) {
+    // Don't block play if explictly set to not block


To make this comment more specific, maybe "Only block play if the ad plugin is in a state where play should be blocked. This currently means BeforePreroll and Preroll."

incompl · 2018-03-21T21:59:22Z

src/cancelContentPlay.js

    // another cancellation is already in flight, so do nothing
    return;
  }

+  player.ads.debug('Using cancelContentPlay to block content playback');


incompl · 2018-03-21T22:04:56Z

src/playMiddleware.js

+    },
+    callPlay() {
+      // Block play calls while waiting for an ad
+      if (obj.isMiddlewareMediatorSupported() &&


Is the check to isMiddlewareMediatorSupported needed?

I ask because it's checked before setting up the middleware in plugin.js

This is mostly defensive. Although it is checked before registering the the middleware, if in case that changes we want to make sure we don't use both the playMiddleware and cancelContentPlay at the same time.

I think in this case it doesn't have much defensive value but could lead to confusion about how the code works. That's subjective though so not a big deal.

Looking at this again, it looks like it's impossible for this to return false because the middleware is only set up on the same condition. Sorry to waffle but I think we should remove it unless there is a way for it to return true because a reader will assume it does something.

So here's my thinking behind the "defensive code" reasoning:

once a middleware is registered, it will always run(since we're using the MIME type agnostic '*')

there's a number of video.js versions that don't have Middleware Mediator support (since it was added relatively recently)

since the middleware is registered in plugin.js, I'm worried that the reason why we check isMiddlewareMediatorSupported would be lost over time and we'll wind up doing something that causes a runtime exception.

Maybe I'm just worrying about nothing though.

Once middleware is registered it will always run, and conditions for it being registered will never change, so it won't need to be checked again.

The video.js versions that don't have mediator support are handled within the check before registering.

The last concern is definitely a valid one, but I think it is best addressed by commenting the code to add the reason. Basically it sounds like you're writing a workaround for a hypothetical bug, but really, you're making it harder to identify the hypothetical bug when if and when it happens.

Maybe the last concern can be addressed by doing two things:

adding more comments to explain why we check for support first in plugin.js

check if videojs.middleware exists before trying to return the middleware.TERMINATOR here, to avoid a runtime exception breaking the whole player.

Also, this check is also done in initCancelContentPlay, do you feel the same about that?

"adding more comments to explain why we check for support first in plugin.js" 👍

"check if videojs.middleware exists before trying to return the middleware.TERMINATOR" That doesn't seem much different. If middleware isn't supported, we would not have gotten that far.

"Also, this check is also done in initCancelContentPlay, do you feel the same about that?" I do, yeah. It's performing a check for an unrelated feature. The logic should be hoisted to the first place that can handle it and form a bottleneck rather than spreading checks throughout various modules.

incompl · 2018-03-21T22:06:14Z

src/playMiddleware.js

+    play(terminated, value) {
+      if (terminated) {
+        player.ads.debug('Play event was terminated.');
+        player.trigger('play');


I would suggest a comment explaining why we trigger play after play was terminated.

incompl · 2018-03-21T22:07:17Z

src/plugin.js

@@ -9,6 +9,7 @@ import redispatch from './redispatch.js';
 import initializeContentupdate from './contentupdate.js';
 import adMacroReplacement from './macros.js';
 import cueTextTracks from './cueTextTracks.js';
+import pm from './playMiddleware.js';


I suggest spelling it out as import playMiddleware from './playMiddleware.js'; to follow the project convention and make the code more readable.

incompl · 2018-03-21T22:09:04Z

src/plugin.js

@@ -120,6 +126,14 @@ const contribAdsPlugin = function(options) {
    player.ads._pausedOnContentupdate = false;
  });

+  // Keep track of whether a play event has happened
+  player.on('play', () => {
+    videojs.log('*$*', '_playRequested about to be set');


Some debug messages to remove still

incompl · 2018-03-21T22:10:57Z

src/redispatch.js


  if (player.ads.inAdBreak()) {
    prefixEvent(player, 'ad', event);
-  } else if (player.ads.isContentResuming() || resumingAfterNoPreroll) {
+  } else if (player.ads.isContentResuming() ||


I suggest a comment for this line

incompl · 2018-03-21T22:16:15Z

src/states/ContentPlayback.js

+   * Reset resumingAfterNoPreroll once the content actually starts playing
+   */
+  onPlaying(player) {
+    this.resumingAfterNoPreroll = false;


I believe we transition to Content Mode after a playing event, so would this be the second playing event? Not sure that resumingAfterNoPreroll needs to be part of ContentPlayback and that the ad states could handle it.

In that case, would it work to set this.resumingAfterNoPreroll = false; in ContentPlayback.init?

No. If there is nopreroll, adtimeout e.c.t in either BeforePreroll or Preroll states, we immediately transition to ContentPlayback. This is why the resumingAfterNoPreroll flag needs to be passed into this state. Also, this is why playing will happen after the transition to ContentPlayback.

Ah, I think this further indicates an issue with how we're handling nopreroll. Normally we transition to ContentPlayback after playing, but in this case we don't. As a result, preroll logic is creeping into ContentPlayback.

What do you think of setting resumingAfterNoPreroll within Preroll, and transitioning to Preroll on nopreroll (heh) with resumingAfterNoPreroll and contentResuming both set to true?

so Preroll -> Preroll?

Honestly I think this would be a big change and wouldn't fit in the context of this PR. I think making resumingAfterNoPreroll "first-class" should be done separately, and really we shouldn't have the special resumingAfterNoPreroll flag and instead go into contentResuming until playing.

I agree with avoiding the special resumingAfterNoPreroll flag and staying in Preroll until playing. The state transition would be BeforePreroll -> Preroll if nopreroll happened earlier. If nopreroll happened during Preroll it would stay in that state and set contentResuming to true.

The reason I think it fits into this PR is that you're creating technical debt (the preroll handling in ContentPlayback) to work around the problem. Rather than introduce new technical debt with the intent to remove it later, it's preferable to not introduce it at all.

I'm 100% down to assist in making this change.

incompl · 2018-03-21T22:25:32Z

src/states/Preroll.js

@@ -26,8 +26,13 @@ export default class Preroll extends AdState {
      player.trigger('adtimeout');
    }, timeout);

+    if (!this.inAdBreak() && !this.isContentResuming()) {
+      player.ads._shouldBlockPlay = true;


In init you would never be in ad break or content resuming, so this check isn't needed.

This is mostly defensive since the flag is used for both cancelContentPlay and playMiddleware and the timing of when it is set is important.

I would also like to avoid retesting it currently

Todo: remove conditional check

incompl · 2018-03-21T22:26:25Z

src/states/abstract/State.js

@@ -86,6 +86,13 @@ export default class State {
    return false;
  }

+  /*
+   * Overriden by BeforePrerollState, PrerollState and ContentPlaybackState.


Don't need State in the names

Ah, I was following the convention elsewhere in this file

It's probably my bad, I renamed them at one point during development. I think we should use the current file names.

ldayananda

Review comments below:

ldayananda · 2018-03-22T19:08:52Z

src/playMiddleware.js

+      if (terminated) {
+        player.ads.debug('Play call to Tech was terminated.');
+        // Trigger play event to ensure that event order remains the same
+        // as with cancelContentPlay.


Change this to refer to the "intent to play" rather than refer to cancelContentPlay

ldayananda · 2018-03-22T19:10:45Z

src/plugin.js

@@ -9,6 +9,8 @@ import redispatch from './redispatch.js';
 import initializeContentupdate from './contentupdate.js';
 import adMacroReplacement from './macros.js';
 import cueTextTracks from './cueTextTracks.js';
+import playMiddlewareFeature from './playMiddleware.js';
+const { playMiddleware, isMiddlewareMediatorSupported } = playMiddlewareFeature;


put this below all the imports

ldayananda · 2018-03-22T19:15:22Z

src/states/Preroll.js

@@ -26,8 +26,13 @@ export default class Preroll extends AdState {
      player.trigger('adtimeout');
    }, timeout);

+    if (!this.inAdBreak() && !this.isContentResuming()) {
+      player.ads._shouldBlockPlay = true;


Todo: remove conditional check

ldayananda · 2018-03-22T19:44:23Z

src/playMiddleware.js

+ */
+obj.isMiddlewareMediatorSupported = function() {
+
+  if (videojs.browser.IS_IOS || videojs.browser.IS_ANDROID) {


Maybe we could just call load() on mobile devices to fulfill the user gesture requirement?

ldayananda · 2018-03-22T19:48:15Z

src/cancelContentPlay.js

+  if (pm.isMiddlewareMediatorSupported()) {
+    // Don't use cancelContentPlay while playMiddleware is in use
+    return;
+  } else if (player.ads._shouldBlockPlay === false) {


Make cancelContentPlay self-sufficient. Handle play events in this file.

Alternatives:

add another flag for "between init and starting preroll ad break"

have a 3 value flag "cancelContentPlay" "middleware" "don't block"

ldayananda · 2018-03-22T19:57:03Z

src/states/ContentPlayback.js

@@ -7,16 +8,36 @@ import {ContentState, Midroll, Postroll} from '../states.js';
 */
 export default class ContentPlayback extends ContentState {

-  init(player) {
-    // Play the content if cancelContentPlay happened or we paused on 'contentupdate'
+  init(player, resumingAfterNoPreroll) {


Future issue: change the 2nd arguments to an options object

Created #355 for this

ldayananda · 2018-03-22T20:15:45Z

src/states/ContentPlayback.js

@@ -1,4 +1,5 @@
 import {ContentState, Midroll, Postroll} from '../states.js';
+// import pm from '../playMiddleware.js';


… return the terminator if it is supported

…thod for testing/debugging purposes. More play middleware unit tests

…ported. Update unit tests. Lock down integration tests

ldayananda · 2018-04-12T19:38:54Z

src/states/Preroll.js

-    this.afterLoadStart(() => {
-      this.transitionTo(ContentPlayback);
-    });
+    } else {


Does this change seem right @incompl? This way both endLinearAdMode and resumeAfterNoPreroll will wait for playing to transitionTo ContentPlayback

adserror is deprecated and I think we should focus on removing reliance on it rather than fixing it.

I'm not sure I really follow what you mean. I don't want to add any more fundamental changes to this PR, so I don't plan on removing the behavior of adserror here, just making it more clear what is going on.

Might as well change it to match everything else, there is a risk in changing it but it's probably not high.

Cool, I think we agree that this change makes sense then?

ldayananda · 2018-04-12T19:42:04Z

test/integration/test.ads.js

@@ -214,7 +154,7 @@ QUnit.test('changing the src triggers "contentupdate"', function(assert) {
  this.player.on('contentupdate', spy);

  // set src and trigger synthetic 'loadstart'
-  this.player.src('http://media.w3.org/2010/05/sintel/trailer.mp4');
+  this.player.currentSrc = () => 'AH HA!!! I AM NOT A REAL SOURCE';


@incompl do we want to change this back?

It's kind of nice that this demonstrates that a source isn't really being loaded, but we could also make the string less annoying.

Hehe, I'm fine with leaving it

ldayananda · 2018-04-12T19:47:15Z

test/integration/test.cancelContentPlay.js

+  afterEach: _.flow(restoreVideojs, sharedHooks.afterEach)
+});
+
+QUnit.test('pauses to wait for prerolls when the plugin loads BEFORE play', function(assert) {


These tests were moved from test.ads.js

incompl · 2018-04-19T13:29:44Z

src/playMiddleware.js

+import videojs from 'video.js';
+
+const obj = {};
+let videojsReference = videojs;


A comment for this line would be helpful. It's not obvious why this is necessary.

incompl · 2018-04-19T13:33:58Z

src/plugin.js

+  // and on desktop currently(as the user-gesture requirement on mobile
+  // will disallow calling play once play blocking is lifted)
+  // The middleware must also be registered outside of the plugin,
+  // to avoid a middleware factory being created for each player


Good comment

incompl · 2018-04-19T13:35:43Z

src/plugin.js

+  // The middleware must also be registered outside of the plugin,
+  // to avoid a middleware factory being created for each player
+  if (isMiddlewareMediatorSupported() && settings.debug) {
+    videojs.log('ADS:', 'Play middleware has been registered with videojs');


It would be a little tidier to move this to where middleware is actually registered.

incompl · 2018-04-19T13:41:10Z

src/states/Preroll.js

    // Loading spinner from now until ad start or end of ad break.
    player.addClass('vjs-ad-loading');

+    // If adserror, adscanceled, nopreroll or skipLinearAdMode already
+    // ocurred, resume to content immediately
+    if (shouldResumeToContent || player.ads.nopreroll_) {


I realize it may be late in the game to mess with this, but is the player.ads.nopreroll_ part still necessary? The shouldResumeToContent stuff seems pretty airtight.

The main reason for it is that we set player.ads.nopreroll_ globally after listening for the nopreroll event in plugin.js. My logic was that it's still possible for there to be a nopreroll event roughly at the same time as play/one of the other negative scenarios and so we should check just in case. If we go down the route of removing nopreroll_ from the global state and making it part of the BeforePreroll & Preroll states only, I think we would be safe to remove it then

Yeah that sounds like the right direction to go in. Don't need to do it now.

incompl · 2018-04-19T13:47:24Z

LGTM, just some minor comments

bcsdasneogi · 2018-04-19T16:58:32Z

QA : LGTM

incompl reviewed Mar 5, 2018

View reviewed changes

incompl mentioned this pull request Mar 9, 2018

how to change content source correctly #346

Closed

incompl reviewed Mar 21, 2018

View reviewed changes

ldayananda commented Mar 22, 2018

View reviewed changes

ldayananda added 15 commits April 3, 2018 19:22

add middleware stub

a38d96e

basic support method

9e86c48

first try at play middleware

bc9b5b3

reworked changes for non-autoplay

5a5123f

wip: set a plugin-level var to block ads. Play middleware should only…

200aa32

… return the terminator if it is supported

update tests

dff6d47

test restructure

3a96a5f

more playMiddleware supported unit tests

01ddbfa

add state-specific unit tests for _shouldBlockPlay

b14e2db

convenience isMiddlewareMediatorSupported method instead of module me…

e004b55

…thod for testing/debugging purposes. More play middleware unit tests

fixed bug where content would not start playing after adtimeout

71610e9

couple playMiddleware and cancelContentPlay together

a1f5de6

use different export style for playMiddleware.isMiddlewareMediatorSup…

b45d11c

…ported. Update unit tests. Lock down integration tests

fix/reset most integration tests

3a00c09

fix snapshot test

eac4e08

ldayananda added 10 commits April 9, 2018 13:49

more test logging

7232d66

try cleaning up after redispatch

03facd3

try to narrow down

e675d76

make play middleware unit tests real nested qunit modules

d8ba76b

just use global Qunit in tests

5d07c47

try just commenting out new test

237dbd6

try with a fixed test

fda69db

add another test

7fe5036

one more test

5adfc85

adding another test

ebdd033

ldayananda commented Apr 12, 2018

View reviewed changes

cleaning up

8c35bd4

ldayananda changed the title ~~[WIP] feat: Add Play middleware to avoid calling play on tech~~ feat: Add Play middleware to avoid calling play on tech Apr 12, 2018

incompl reviewed Apr 19, 2018

View reviewed changes

ldayananda added 4 commits April 19, 2018 11:23

CR feedback

2a045c9

CR comments

c6e9cc3

Run tests on chrome without autoplay policies

85d3013

fixing travisChrome launch settings

cbf2f0e

ldayananda mentioned this pull request Apr 19, 2018

Play middleware: only trigger play event when our play middleware terminates #365

Merged

ldayananda added 2 commits April 19, 2018 13:32

Possibly more stable test

0cb68eb

revert one test change

c6a102b

ldayananda merged commit 1482511 into master Apr 19, 2018

misteroneill deleted the play-middleware branch January 24, 2019 20:02

		@@ -1,4 +1,5 @@
		import {ContentState, Midroll, Postroll} from '../states.js';
		// import pm from '../playMiddleware.js';

feat: Add Play middleware to avoid calling play on tech #337

feat: Add Play middleware to avoid calling play on tech #337

Conversation

ldayananda commented Feb 28, 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

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

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

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

incompl commented Apr 19, 2018

bcsdasneogi commented Apr 19, 2018

ldayananda commented Feb 28, 2018 •

edited

Loading