feat: More complete support for stitched ad scenarios. #415

misteroneill · 2018-08-01T21:11:04Z

This PR is a re-implementation of #284.

This is a WIP still in that tests need fleshing out for these scenarios.

misteroneill · 2018-08-01T21:17:19Z

CONTRIBUTING.md

-[karma]: http://karma-runner.github.io/
-[local]: http://localhost:9999/test/
-[conventions]: https://github.com/videojs/generator-videojs-plugin/blob/master/docs/conventions.md
+See: [docs/developer/index.md](docs/developer/index.md)


Some of the info here was incorrect. Seems better to send people to the correct documentation.

Awesome, I appreciate this

misteroneill · 2018-08-01T21:17:53Z

scripts/modules.rollup.config.js

-  moduleName: 'videojsContribAds',
-  entry: 'src/plugin.js',
+  name: 'videojsContribAds',
+  input: 'src/plugin.js',


Was getting tired of the deprecation warnings being logged.

misteroneill · 2018-08-01T21:18:31Z

src/plugin.js

@@ -172,11 +172,11 @@ const contribAdsPlugin = function(options) {
  player.ads = getAds(player);

  player.ads.settings = settings;
+  player.ads.stitchedAds(settings.stitchedAds);


This needs to come before we initialize the BeforePreroll state because that state relies on this being set correctly.

Makes sense, might be a good comment to add

misteroneill · 2018-08-01T21:19:52Z

test/index.html

-  <script src="integration/lib/shared-module-hooks.js"></script>
-  <script src="dist/bundle.js"></script>
-</body>
-</html>


This file doesn't do anything.

incompl · 2018-08-02T13:44:50Z

src/ads.js

+    // stream.
+    // With an argument, turns stitched ads mode on or off. This is best used
+    // immediately preceding a source change where the state would change to
+    // or from stitched ads.


Is it the same for the initial source, that it should be called before a source is set?

Yeah. There's a change in plugin.js that moves the setting of this to before the initial state is created. Otherwise, contrib-ads starts with stitchedAds being false.

incompl · 2018-08-02T13:45:47Z

src/ads.js

      return !videojs.browser.IS_IOS &&
             !videojs.browser.IS_ANDROID &&
             somePlayer.duration() === Infinity;
    },

+    // Return true if the ads plugin should save and restore snapshots of the
+    // player state when moving into and out of ad mode.
+    shouldTakeSnapshots(somePlayer = player) {


incompl · 2018-08-02T13:49:07Z

src/states/BeforePreroll.js

+    // not be blocked and we should move immediately into the Preroll state.
+    if (player.ads.stitchedAds()) {
+      player.ads._shouldBlockPlay = false;
+      return this.transitionTo(Preroll, this.adsReady, this.shouldResumeToContent);


The Preroll state is defined as being after the play event: I think the onPlay handler already covers it, so I believe you need only turn off _shouldBlockPlay here.

Sounds good, I'll try it.

incompl · 2018-08-02T13:58:24Z

src/states/Preroll.js

+    // With stitched ads, we won't need to wait for a preroll and we can
+    // allow the integration to call startLinearAdMode whenever it's ready.
+    if (!player.ads.stitchedAds()) {
+      this.waitingForAdBreak = true;


Technically we're still waiting for ad break until startLinearAdMode, even though with stitched ads this will be a short time

I can see skipping the loading spinner, I wonder if nopreroll should be handled?

In our SSAI plugin, at least, we do fire it if there's no preroll stitched in. Makes sense to skip the spinner and preroll timeout only probably. I was thinking we might want the resumeAfterNoPreroll below.

misteroneill · 2018-08-03T16:31:46Z

Note to self, tests to add:

Prerolls do not block play.
No vjs-ad-loading class is added for any roll.
No timeout timers are queued for any roll.

incompl · 2018-08-07T17:34:19Z

docs/integrator/options.md

@@ -40,6 +40,8 @@ Default Value: `false`

 Set this to true if you are using ads stitched into the content video. This is necessary for ad events to be sent correctly.



We should update this based on the decision to deprecate this usage.

incompl · 2018-08-07T17:40:06Z

src/ads.js

+
+        // Keep the private property and the settings in sync. When this
+        // setter is removed, we can probably stop using the private property.
+        this._stitchedAds = this.settings.stitchedAds = !!arg;


Do we still need _stitchedAds as things stand?

We don't, good point. Removing.

incompl · 2018-08-07T19:22:42Z

examples/stitched-ad-plugin/plugin.js

+
+  var simulateStartOfAd = function(time) {
+    return waitForCurrentTime(time, function() {
+      player.on(['timeupdate', 'adtimeupdate'], simulateEndOfAd(time + 5));


Why both, by the way? Seems like only adtimeupdate should be needed?

That's probably correct. Copy/paste is the explanation, I'm sure.

incompl · 2018-08-07T19:23:27Z

examples/stitched-ad-plugin/plugin.js

+  });
+
+  // Simulate a mid-roll at 15 seconds.
+  player.on(['timeupdate', 'adtimeupdate'], simulateStartOfAd(15));


Same here, would expect only timeupdate needed.

misteroneill · 2018-08-08T13:55:05Z

In attempting to adopt this work in our SSAI plugin, I realized that the best thing we were doing was to fire nopreroll and nopostroll when we load a new source. Doing this means contrib-ads treats everything as a midroll, which, from a UX perspective, is irrelevant.

So, I wonder if that behavior ought to just be implicit in the stitchedAds: true scenario. It might make things simpler.

incompl · 2018-08-08T13:59:37Z

@misteroneill What changes would that involve?

misteroneill · 2018-08-08T14:00:45Z

I guess handling stitched ads as if nopreroll and nopostroll had always been fired.

misteroneill · 2018-08-08T19:20:31Z

I have looked at redispatch.handleEnded and it seems the best path forward was for the StitchedAdRoll state to handle it and re-trigger ended. The fewer stitched-ads conditionals, the better.

Changing content during SSAI ads should work. I'm not sure contrib-ads can/should handle that, but in our plugin we should mostly handle it via middleware.

incompl · 2018-08-08T19:43:13Z

I have been investigating changing sources during ads in contrib-ads and basically the issue is that there needs to be a way to differentiate "contentupdate due to ad loading" and "contentupdate due to content source change during ads". I'm not sure on what path to to take with that but input welcome. In the meantime, it's certainly not a blocker.

misteroneill · 2018-08-08T23:12:34Z

Yeah, I took a quick look at our SSAI plugin and we do plenty of setup/teardown to handle changing in and out of SSAI sources, but it doesn't seem that we handle the case of changing during ad playback. Something to consider for the future.

misteroneill · 2018-08-09T17:22:29Z

I'll be working on new tests for the re-thinking of this PR and then will submit for re-approval.

misteroneill · 2018-08-16T18:35:56Z

test/karma.conf.js

+      {pattern: 'dist/videojs-contrib-ads.js', nocache: true},
+      {pattern: 'dist/videojs-contrib-ads.css', nocache: true},
+      {pattern: 'test/integration/lib/shared-module-hooks.js', nocache: true},
+      {pattern: 'test/dist/bundle.js', nocache: true},


This allows you to refresh the Karma Chrome instance and get the latest built files.

incompl · 2018-08-16T18:35:55Z

examples/stitched-ad-plugin/plugin.js

+  // Initialize contrib-ads.
+  player.ads(options);
+
+  player.on('canplay', function() {


Out of curiosity, why canplay?

It's mainly so that you can replay the example video.

incompl · 2018-08-16T18:36:10Z

examples/stitched-ad-plugin/plugin.js

+      // Simulate a mid-roll at 15 seconds and a post-roll at 5 seconds from
+      // the end. Need to listen to both timeupdate and adtimeupdate because
+      // redispatch will prefix during ads.
+      player.on(['timeupdate', 'adtimeupdate'], function(e) {


This is so straightforward now 💯

incompl · 2018-08-16T18:44:08Z

src/states/StitchedContentPlayback.js

+
+    // Tell redispatch not to intercept ended events. This should prevent
+    // the firing of readyforpostroll.
+    this.player.ads._contentHasEnded = true;


Content hasn't ended so this is confusing. I agree with the goal of not having redispatch know about stitchedAds, but I think we haven't done that quite yet: _contentHasEnded basically got overloaded for special stitchedAds handling in a less transparent way. Having handleEnded return early if stitchedAds is true is the same in practice, but clearer. I've thought about it for awhile and can't come up with a better solution. Checking a top-level setting in redispatch, while not ideal, does benefit from being simple and clear.

Sounds good to me.

Seems like we can add a check in redispatch for when we see ended with stitched ads, but we do still want to get an adended event so the states can handle that and get us out of ad mode before re-triggering ended. Updating soon.

If we return early in redispatch, it will allow the original ended event to not be cancelled or redispatched and you won't need to retrigger it at all.

incompl · 2018-08-16T20:01:05Z

src/redispatch.js

@@ -86,7 +86,7 @@ const handleEnded = (player, event) => {
    }

  // Prefix ended due to content ending before postroll check
-  } else if (!player.ads._contentHasEnded) {
+  } else if (!player.ads._contentHasEnded && !player.ads.stitchedAds()) {


Do you need it to go into the if (player.ads.isInAdMode()) { block above when using stitched ads? If not, could do a

if (player.ads.stitchedAds()) { return }

below const handleEnded = (player, event) => {

Yeah, it does need to go into that block because StitchedAdRoll needs to see adended to call endLinearAdMode and then re-trigger ended. Otherwise, you can end in an ad state, which is probably not desirable.

It seems strange to cancel ended so that you can retrigger it. What if you did the above, and then handle ended event by leaving ad mode?

I thought it might be weird to see an ended event during ad mode. If I were writing an integration, I would expect that ended did not happen during ad mode.

Ah so you want the sequence to be adended -> leave ad mode -> ended rather than ended -> leave ad mode. That makes sense.

Yep, exactly.

incompl · 2018-08-16T20:16:59Z

LGTM

misteroneill commented Aug 1, 2018

View reviewed changes

incompl reviewed Aug 2, 2018

View reviewed changes

misteroneill force-pushed the stitched-ads branch from 3bed7ef to de09f8a Compare August 2, 2018 16:54

incompl reviewed Aug 7, 2018

View reviewed changes

incompl approved these changes Aug 7, 2018

View reviewed changes

misteroneill added 12 commits August 15, 2018 13:22

feat: More complete support for stitched ad scenarios.

1aaa3ee

Roll back changes to stitchedAds method

b5fc9cd

CR

4bcf774

Deprecate stitchedAds setter

3c5488d

Tweak comment

c059a9b

Add tests for ads.js changes

e098380

Add basic stitched ads example implementation

25aff2c

Fix tests

c73880d

Add state unit tests

bfbeef0

Update cancelContentPlay tests

14697c9

Fix tests

2f89ad0

CR

118f0aa

misteroneill added 4 commits August 15, 2018 13:24

Listen only to necessary events.

06c9f61

Rethink everything

2248689

Rework example

c13cc0a

Fixed example using new paradigm

ec3e88c

misteroneill force-pushed the stitched-ads branch from 3ca4caa to ec3e88c Compare August 15, 2018 17:26

misteroneill added 3 commits August 15, 2018 15:59

Add stitchedAds integration test

b80e262

State tests

d970cc2

Fix test

ad620ec

misteroneill commented Aug 16, 2018

View reviewed changes

incompl reviewed Aug 16, 2018

View reviewed changes

CR

da3b1dc

incompl reviewed Aug 16, 2018

View reviewed changes

misteroneill mentioned this pull request Aug 20, 2018

fix: Avoid multiple-registration warning messages by accepting only the first contrib-ads per context. #421

Merged

misteroneill merged commit a533bbb into master Aug 23, 2018

misteroneill deleted the stitched-ads branch August 23, 2018 19:24

		@@ -40,6 +40,8 @@ Default Value: `false`

		Set this to true if you are using ads stitched into the content video. This is necessary for ad events to be sent correctly.

feat: More complete support for stitched ad scenarios. #415

feat: More complete support for stitched ad scenarios. #415

Conversation

misteroneill commented Aug 1, 2018 • edited by gkatsev 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

misteroneill commented Aug 3, 2018

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

misteroneill commented Aug 8, 2018 • edited Loading

incompl commented Aug 8, 2018

misteroneill commented Aug 8, 2018

misteroneill commented Aug 8, 2018

incompl commented Aug 8, 2018

misteroneill commented Aug 8, 2018

misteroneill commented Aug 9, 2018

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 Aug 16, 2018

misteroneill commented Aug 1, 2018 •

edited by gkatsev

Loading

misteroneill commented Aug 8, 2018 •

edited

Loading