fix: Fix bug that could cause double ended events

README.md

@@ -106,17 +106,19 @@ This is the basic flow for a simple use case, but there are other things the int
 * `skipLinearAdMode` (METHOD) -- At a time when `startLinearAdMode` is expected, calling `skipLinearAdMode` will immediately resume content playback instead.
 * `nopreroll` (EVENT) -- You can trigger this event even before `readyforpreroll` to indicate that no preroll will play. The ad plugin will not check for prerolls and will instead begin content playback after the `play` event (or immediately, if playback was already requested).
 * `nopostroll` (EVENT) -- Similar to `nopreroll`, you can trigger this event even before `contentended` to indicate that no postroll will play.  The ad plugin will not wait for a postroll to play and will instead immediately trigger the `ended` event.
-* `adserror` (EVENT) -- This event skips prerolls when seen before a preroll ad break. It skips postrolls if called after contentended and before a postroll ad break. It ends linear ad mode if seen during an ad break.
 * `contentresumed` (EVENT) - If your integration does not result in a "playing" event when resuming content after an ad, send this event to signal that content can resume. This was added to support stitched ads and is not normally necessary.
 
 There are some other useful events that videojs-contrib-ads may trigger:
 
  * `contentchanged` (EVENT) -- Fires when a new content video has been loaded in the player (specifically, at the same time as the `loadstart` media event for the new source). This means the ad workflow has restarted from the beginning. Your integration will need to trigger `adsready` again, for example. Note that when changing sources, the playback state of the player is retained: if the previous source was playing, the new source will also be playing and the ad workflow will not wait for a new `play` event.
 
-Deprecated events:
+### Deprecated events
 
-* `contentupdate` (EVENT) -- Replaced by `contentchanged`, which is more reliable.
-* `adscanceled` (EVENT) -- Intended to cancel all ads, it was never fully implemented. Instead, use `nopreroll` and `nopostroll`.
+The following events are slated for removal from contrib-ads and will have no special behavior once removed. These events should no longer be used in integrating ad plugins. Replacements are provided for matching functionality that will continue to be supported.
+
+* `contentupdate` (EVENT) -- In the future, contrib-ads will no longer trigger this event. Listen to the new `contentchanged` event instead; it is is more reliable.
+* `adscanceled` (EVENT) -- In the future, this event will no longer result in special behavior in contrib-ads. It was intended to cancel all ads, but it was never fully implemented. Instead, trigger `nopreroll` and `nopostroll`.
+* `adserror` (EVENT) -- In the future, this event will no longer result in special behavior in contrib-ads. Today, this event skips prerolls when seen before a preroll ad break. It skips postrolls if seen after contentended and before a postroll ad break. It ends linear ad mode if seen during an ad break. These behaviors should be replaced using `skipLinearAdMode` and `endLinearAdMode` in the ad integration.
 
 ### Public Methods
 
@@ -540,10 +542,10 @@ A short list of features, fixes and changes for each release is available in [CH
 
 ## Roadmap
 
-### Unplanned Major Version Update
+### Version 7
 
 * Pause content video if there is a programmatic call to play (prefixed as adplay) while an ad is playing in an ad container (rather than content video element). Prefixing doesn't prevent the videojs behavior, so this would prevent the content from playing behind the ad. Right now, ad integrations I am aware of are doing this on their own, so this would require a migration to move the behavior into this project.
-* `contentended` has a confusing name: real `ended` events are later sent, and that is when content should be considered ended. The `content` prefix is used for events when content is resuming after an ad. A better name would be `readyforpostroll`. That would make it clearer to implementations that the correct response would be to either play a postroll or send the `nopostroll` event.
+* `contentended` has a confusing name: real `ended` events are later sent, and that is when content should be considered ended. The `content` prefix is used for events when content is resuming after an ad. A better name would be `readyforpostroll`. That would make it clearer to implementations that the correct response would be to either play a postroll or send the `nopostroll` event. `resumeended`, which is a workaround for this issue, could then become `contentended`, which would mirror all the other redispatched events.
 
 ## License
 

examples/basic-ad-plugin/example-integration.js

@@ -104,6 +104,8 @@
       if (!state.postrollPlayed && playPostroll) {
         state.postrollPlayed = true;
         playAd();
+      } else {
+        player.trigger('nopostroll');
       }
     });
 

migration-guides/migrating-to-6.0.md

@@ -25,6 +25,7 @@ Deprecated interfaces will be removed in a future major version update.
 
 * `contentupdate` is now deprecated. It has been replaced by `contentchanged`. `contentupdate` was never intended to fire for the initial source, but over time its behavior eroded. To make migration easier for anyone who depends on the current behavior, we're providing a deprecation period and a new event with correct behavior.
 * `adscanceled` is now deprecated. Instead, use `nopreroll` and `nopostroll`. `adscanceled` was initially intended to function similarly to calling both `nopreroll` and `nopostroll` but it was never fully implemented.
+* `adserror` is now deprecated. Currently this event will skip prerolls when seen before a preroll ad break, skip postrolls if called after contentended and before a postroll ad break, and end linear ad mode if seen during an ad break. It is more declarative for the ad integration to do these things explicitly with `skipLinearAdMode` and `endLinearAdMode`. In the future, this event will not have any special behavior. The event isn't being "removed" in any sense because contrib-ads does not trigger this event, so integrations may continue to use it for other purposes.
 
 ## Timeout behavior changes
 

src/adBreak.js

@@ -7,7 +7,6 @@
 import * as snapshot from './snapshot.js';
 
 function start(player) {
-
   player.ads.debug('Starting ad break');
 
   player.ads._inLinearAdMode = true;
@@ -42,9 +41,13 @@ function start(player) {
   player.ads.removeNativePoster();
 }
 
-function end(player) {
+function end(player, callback) {
   player.ads.debug('Ending ad break');
 
+  if (callback === undefined) {
+    callback = () => {};
+  }
+
   player.ads.adType = null;
 
   player.ads._inLinearAdMode = false;
@@ -60,13 +63,15 @@ function end(player) {
   if (player.ads.isLive(player)) {
     player.addClass('vjs-live');
   }
+
+  // Restore snapshot
   if (!player.ads.shouldPlayContentBehindAd(player)) {
-    snapshot.restorePlayerSnapshot(player, player.ads.snapshot);
-  }
+    snapshot.restorePlayerSnapshot(player, player.ads.snapshot, callback);
 
   // Reset the volume to pre-ad levels
-  if (player.ads.shouldPlayContentBehindAd(player)) {
+  } else {
     player.volume(player.ads.preAdVolume_);
+    callback();
   }
 
 }

src/redispatch.js

@@ -69,16 +69,24 @@ const handlePlaying = (player, event) => {
 const handleEnded = (player, event) => {
   if (player.ads.isInAdMode()) {
 
-    // The true ended event fired either after the postroll
-    // or because there was no postroll.
+    // Cancel ended events during content resuming. Normally we would
+    // prefix them, but `contentended` has a special meaning. In the
+    // future we'd like to rename the existing `contentended` to
+    // `readyforpostroll`, then we could remove the special `resumeended`
+    // and do a conventional content prefix here.
     if (player.ads.isContentResuming()) {
-      return;
-    }
+      cancelEvent(player, event);
 
-    // Prefix ended due to ad ending.
-    prefixEvent(player, 'ad', event);
+      // Important: do not use this event outside of videojs-contrib-ads.
+      // It will be removed and your code will break.
+      player.trigger('resumeended');
+
+    // Ad prefix in ad mode
+    } else {
+      prefixEvent(player, 'ad', event);
+    }
 
-  // Prefix ended due to content ending before preroll check
+  // Prefix ended due to content ending before postroll check
   } else if (!player.ads._contentHasEnded) {
     prefixEvent(player, 'content', event);
   }

src/snapshot.js

@@ -62,9 +62,14 @@ export function getPlayerSnapshot(player) {
  * @param {Object} player - the videojs player object
  * @param {Object} snapshotObject - the player state to apply
  */
-export function restorePlayerSnapshot(player, snapshotObject) {
+export function restorePlayerSnapshot(player, snapshotObject, callback) {
+  if (callback === undefined) {
+    callback = () => {};
+  }
+
   if (player.ads.disableNextSnapshotRestore === true) {
     player.ads.disableNextSnapshotRestore = false;
+    callback();
     return;
   }
 
@@ -188,6 +193,9 @@ export function restorePlayerSnapshot(player, snapshotObject) {
   // restoration is required
 
   if (player.ads.videoElementRecycled()) {
+    // Snapshot restore is done, so now we're really finished.
+    player.one('resumeended', callback);
+
     // on ios7, fiddling with textTracks too early will cause safari to crash
     player.one('contentloadedmetadata', restoreTracks);
 
@@ -220,5 +228,8 @@ export function restorePlayerSnapshot(player, snapshotObject) {
       // just resume playback at the current time.
       player.play();
     }
+
+    // snapshot restore is complete
+    callback();
   }
 }

src/states/AdsDone.js

@@ -18,6 +18,7 @@ export default class AdsDone extends ContentState {
   init(player) {
     // From now on, `ended` events won't be redispatched
     player.ads._contentHasEnded = true;
+    player.trigger('ended');
   }
 
   /*

src/states/Postroll.js

@@ -37,11 +37,8 @@ export default class Postroll extends AdState {
 
     // No postroll, ads are done
     } else {
-      player.setTimeout(() => {
-        player.ads.debug('Triggered ended event (no postroll)');
-        this.contentResuming = true;
-        player.trigger('ended');
-      }, 1);
+      this.contentResuming = true;
+      this.transitionTo(AdsDone);
     }
   }
 
@@ -76,12 +73,10 @@ export default class Postroll extends AdState {
 
     if (this.inAdBreak()) {
       player.removeClass('vjs-ad-loading');
-      adBreak.end(player);
-
       this.contentResuming = true;
-
-      player.ads.debug('Triggered ended event (endLinearAdMode)');
-      player.trigger('ended');
+      adBreak.end(player, () => {
+        this.transitionTo(AdsDone);
+      });
     }
   }
 
@@ -119,19 +114,8 @@ export default class Postroll extends AdState {
     // if there was an error.
     if (player.ads.inAdBreak()) {
       player.ads.endLinearAdMode();
-    }
-
-    this.abort();
-  }
-
-  /*
-   * On ended, transition to AdsDone state.
-   */
-  onEnded() {
-    if (this.isContentResuming()) {
-      this.transitionTo(AdsDone);
     } else {
-      videojs.log.warn('Unexpected ended event during postroll');
+      this.abort();
     }
   }
 
@@ -171,9 +155,7 @@ export default class Postroll extends AdState {
 
     this.contentResuming = true;
     player.removeClass('vjs-ad-loading');
-
-    player.ads.debug('Triggered ended event (postroll abort)');
-    player.trigger('ended');
+    this.transitionTo(AdsDone);
   }
 
   /*

test/karma.conf.js

@@ -55,7 +55,7 @@ module.exports = function(config) {
     browserNoActivityTimeout: 300000,
     client: {
       qunit: {
-        testTimeout: 10000
+        testTimeout: 15000
       }
     }
   });

test/states/test.AdsDone.js

@@ -8,7 +8,11 @@ import {AdsDone} from '../../src/states.js';
  */
 QUnit.module('AdsDone', {
   beforeEach: function() {
+    this.events = [];
     this.player = {
+      trigger: (event) => {
+        this.events.push(event);
+      },
       ads: {}
     };
 
@@ -21,6 +25,11 @@ QUnit.test('sets _contentHasEnded on init', function(assert) {
   assert.equal(this.player.ads._contentHasEnded, true, 'content has ended');
 });
 
+QUnit.test('ended event on init', function(assert) {
+  this.adsDone.init(this.player);
+  assert.equal(this.events[0], 'ended', 'content has ended');
+});
+
 QUnit.test('does not play midrolls', function(assert) {
   this.adsDone.transitionTo = sinon.spy();
 

test/states/test.Postroll.js

@@ -68,7 +68,6 @@ QUnit.test('ends linear ad mode & ended event on ads error', function(assert) {
   this.player.ads.inAdBreak = () => true;
   this.postroll.onAdsError(this.player);
   assert.equal(this.player.ads.endLinearAdMode.callCount, 1, 'linear ad mode ended');
-  assert.equal(this.events[0], 'ended', 'saw ended event');
 });
 
 QUnit.test('no endLinearAdMode on adserror if not in ad break', function(assert) {
@@ -78,7 +77,6 @@ QUnit.test('no endLinearAdMode on adserror if not in ad break', function(assert)
   this.player.ads.inAdBreak = () => false;
   this.postroll.onAdsError(this.player);
   assert.equal(this.player.ads.endLinearAdMode.callCount, 0, 'linear ad mode ended');
-  assert.equal(this.events[0], 'ended', 'saw ended event');
 });
 
 QUnit.test('does not transition to AdsDone unless content resuming', function(assert) {
@@ -87,13 +85,6 @@ QUnit.test('does not transition to AdsDone unless content resuming', function(as
   assert.equal(this.newState, undefined, 'no transition');
 });
 
-QUnit.test('transitions to AdsDone on ended', function(assert) {
-  this.postroll.isContentResuming = () => true;
-  this.postroll.init(this.player);
-  this.postroll.onEnded(this.player);
-  assert.equal(this.newState, 'AdsDone');
-});
-
 QUnit.test('transitions to BeforePreroll on content changed after ad break', function(assert) {
   this.postroll.isContentResuming = () => true;
   this.postroll.init(this.player);
@@ -141,7 +132,6 @@ QUnit.test('can abort', function(assert) {
   this.postroll.abort();
   assert.equal(this.postroll.contentResuming, true, 'contentResuming');
   assert.ok(removeClassSpy.calledWith('vjs-ad-loading'), 'loading class removed');
-  assert.equal(this.events[0], 'ended', 'saw ended event');
 });
 
 QUnit.test('can clean up', function(assert) {

test/test.ads.js

@@ -385,6 +385,7 @@ QUnit.test('an "ended" event is fired after postroll if not fired naturally', fu
 
   this.player.ads.startLinearAdMode(); // start postroll
   this.player.ads.endLinearAdMode();
+  this.player.trigger('resumeended');
   assert.strictEqual(endedSpy.callCount, 1, 'ended event happened');
 });
 
@@ -400,6 +401,7 @@ QUnit.test('ended events when content ends first and second time', function(asse
 
   this.player.ads.startLinearAdMode(); // Postroll starts
   this.player.ads.endLinearAdMode();
+  this.player.trigger('resumeended');
 
   assert.strictEqual(endedSpy.callCount, 1, 'ended event after postroll');
 

test/test.events-postroll.js

@@ -75,12 +75,8 @@ QUnit.test('ended event and postrolls: 0 before postroll, 1 after', function(ass
     }, 1000);
   });
 
-  // Seek to end once we're ready so postroll can play quickly
-  this.player.one('playing', () => {
-    this.player.currentTime(46);
-  });
-
   this.player.play();
+  this.player.currentTime(46);
 
 });