From 20d5bce7bcedea93b5a2b1f8bf2a9abfdb3a3141 Mon Sep 17 00:00:00 2001 From: Greg Smith Date: Wed, 21 Sep 2016 16:00:49 -0400 Subject: [PATCH 1/6] Refactor redispatch WIP --- src/videojs.ads.js | 114 ++++++++++++++++++++++++++++++--------- test/videojs.ads.test.js | 6 +-- 2 files changed, 91 insertions(+), 29 deletions(-) diff --git a/src/videojs.ads.js b/src/videojs.ads.js index 0a1fb4d0..f9e27695 100644 --- a/src/videojs.ads.js +++ b/src/videojs.ads.js @@ -318,7 +318,8 @@ var (function() { var videoEvents = VIDEO_EVENTS.concat([ 'firstplay', - 'loadedalldata' + 'loadedalldata', + 'playing' ]); var returnTrue = function() { return true; }; @@ -337,43 +338,103 @@ var }; player.on(videoEvents, function redispatch(event) { - if (player.ads.state === 'ad-playback') { + + // We do a quick play/pause before we check for prerolls. This creates a "playing" + // event. This conditional block prefixes that event so it's "adplaying" if it + // happens while we're in the "preroll?" state. Not every browser is in the + // "preroll?" state for this event, so the following browsers come through here: + // * iPad + // * iPhone + // * Android + // * Safari + // This is too soon to check videoElementRecycled because there is no snapshot + // yet. We rely on the coincidence that all browsers for which + // videoElementRecycled would be true also happen to send their initial playing + // event during "preroll?" + if (event.type === 'playing' && player.ads.state === 'preroll?') { + triggerEvent('ad', event); + + // Here we send "adplaying" for browsers that send their initial "playing" event + // (caused the the initial play/pause) during the "ad-playback" state. + // The following browsers come through here: + // * Chrome + // * IE11 + // If the ad plays in the content tech (aka videoElementRecycled) there will be + // another playing event when the ad starts. We check videoElementRecycled to + // avoid a second adplaying event. Thankfully, at this point a snapshot exists + // so we can safely check videoElementRecycled. + } else if (event.type === 'playing' && + player.ads.state === 'ad-playback' && + !player.ads.videoElementRecycled()) { + triggerEvent('ad', event); + + // When ad is playing in content tech, we would normally prefix + // "playing" with "ad" to send "adplaying". However, when we did a play/pause + // before the preroll, we already sent "adplaying". This condition prevents us + // from sending another. + } else if (event.type === 'playing' && + player.ads.state === 'ad-playback' && + player.ads.videoElementRecycled()) { + + // Triggering an event prevents the unprefixed one from firing. + // "adcontentplaying" is only seen in this very specific condition. + triggerEvent('adcontent', event); + return; + + // When ad is playing in content tech, prefix everything with "ad". + // This block catches many events such as emptied, play, timeupdate, and ended. + } else if (player.ads.state === 'ad-playback') { if (player.ads.videoElementRecycled() || player.ads.stitchedAds()) { triggerEvent('ad', event); } + + // Send contentended if ended happens during content. Why do we do this? } else if (player.ads.state === 'content-playback' && event.type === 'ended') { triggerEvent('content', event); + + // Content resuming is complicated } else if (player.ads.state === 'content-resuming') { - if (player.ads.snapshot) { - // the video element was recycled for ad playback - if (player.currentSrc() !== player.ads.snapshot.currentSrc) { - if (event.type === 'loadstart') { - return; - } - return triggerEvent('content', event); - - // we ended playing postrolls and the video itself - // the content src is back in place - } else if (player.ads.snapshot.ended) { - if ((event.type === 'pause' || - event.type === 'ended')) { - // after loading a video, the natural state is to not be started - // in this case, it actually has, so, we do it manually - player.addClass('vjs-has-started'); - // let `pause` and `ended` events through, naturally - return; - } - // prefix all other events in content-resuming with `content` - return triggerEvent('content', event); + + // Content resuming after postroll + if (player.ads.snapshot && player.ads.snapshot.ended) { + + // Don't prefix `pause` and `ended` events + // They don't always happen during content-resuming, but they might. + // It seems to happen most often on iOS and Android. + if ((event.type === 'pause' || + event.type === 'ended')) { + return; } + + // All other events get "content" prefix + return triggerEvent('content', event); } - if (event.type !== 'playing') { - triggerEvent('content', event); + + // Content resuming after preroll or midroll + else { + + // Events besides "playing" get "content" prefix + if (event.type !== 'playing') { + triggerEvent('content', event); + } + } } }); + })(); + // "vjs-has-started" should be present at the end of a video. In this case we need + // to re-add it manually. + // Not sure why this happens on ended, I've never seen a case where that is needed. + player.on(['pause', 'ended'], function redispatch() { + if (player.ads.state === 'content-resuming' && + player.ads.snapshot && + player.ads.snapshot.ended) { + player.addClass('vjs-has-started'); + } + }); + // We now auto-play when an ad gets loaded if we're playing ads in the same video element as the content. // The problem is that in IE11, we cannot play in addurationchange but in iOS8, we cannot play from adcanplay. // This will allow ad-integrations from needing to do this themselves. @@ -445,7 +506,8 @@ var var currentSrcChanged; if (!this.snapshot) { - return false; + throw new Error( + 'You cannot use videoElementRecycled while there is no snapshot.'); } srcChanged = player.src() !== this.snapshot.src; diff --git a/test/videojs.ads.test.js b/test/videojs.ads.test.js index 21c6134c..382b7515 100644 --- a/test/videojs.ads.test.js +++ b/test/videojs.ads.test.js @@ -858,7 +858,7 @@ QUnit.test('adsready in content-playback triggers readyforpreroll', function(ass // Event prefixing during ad playback // ---------------------------------- -QUnit.test('player events during prerolls are prefixed if tech is reused for ad', function(assert) { +QUnit.skip('player events during prerolls are prefixed if tech is reused for ad', function(assert) { var prefixed, unprefixed; assert.expect(2); @@ -891,7 +891,7 @@ QUnit.test('player events during prerolls are prefixed if tech is reused for ad' assert.strictEqual(prefixed.callCount, 6, 'prefixed events fired'); }); -QUnit.test('player events during midrolls are prefixed if tech is reused for ad', function(assert) { +QUnit.skip('player events during midrolls are prefixed if tech is reused for ad', function(assert) { var prefixed, unprefixed; assert.expect(2); @@ -922,7 +922,7 @@ QUnit.test('player events during midrolls are prefixed if tech is reused for ad' assert.strictEqual(prefixed.callCount, 6, 'prefixed events fired'); }); -QUnit.test('player events during postrolls are prefixed if tech is reused for ad', function(assert) { +QUnit.skip('player events during postrolls are prefixed if tech is reused for ad', function(assert) { var prefixed, unprefixed; assert.expect(2); From a4522123251f1b96d0ffe8123e316303e43ac98c Mon Sep 17 00:00:00 2001 From: Greg Smith Date: Wed, 21 Sep 2016 16:17:39 -0400 Subject: [PATCH 2/6] improve some comments, unskip tests --- src/videojs.ads.js | 7 ++++--- test/videojs.ads.test.js | 6 +++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/videojs.ads.js b/src/videojs.ads.js index f9e27695..852e7fee 100644 --- a/src/videojs.ads.js +++ b/src/videojs.ads.js @@ -355,7 +355,7 @@ var triggerEvent('ad', event); // Here we send "adplaying" for browsers that send their initial "playing" event - // (caused the the initial play/pause) during the "ad-playback" state. + // (caused by the the initial play/pause) during the "ad-playback" state. // The following browsers come through here: // * Chrome // * IE11 @@ -368,7 +368,7 @@ var !player.ads.videoElementRecycled()) { triggerEvent('ad', event); - // When ad is playing in content tech, we would normally prefix + // When an ad is playing in content tech, we would normally prefix // "playing" with "ad" to send "adplaying". However, when we did a play/pause // before the preroll, we already sent "adplaying". This condition prevents us // from sending another. @@ -388,7 +388,8 @@ var triggerEvent('ad', event); } - // Send contentended if ended happens during content. Why do we do this? + // Send contentended if ended happens during content. + // We will make sure an ended event is sent after postrolls. } else if (player.ads.state === 'content-playback' && event.type === 'ended') { triggerEvent('content', event); diff --git a/test/videojs.ads.test.js b/test/videojs.ads.test.js index 382b7515..21c6134c 100644 --- a/test/videojs.ads.test.js +++ b/test/videojs.ads.test.js @@ -858,7 +858,7 @@ QUnit.test('adsready in content-playback triggers readyforpreroll', function(ass // Event prefixing during ad playback // ---------------------------------- -QUnit.skip('player events during prerolls are prefixed if tech is reused for ad', function(assert) { +QUnit.test('player events during prerolls are prefixed if tech is reused for ad', function(assert) { var prefixed, unprefixed; assert.expect(2); @@ -891,7 +891,7 @@ QUnit.skip('player events during prerolls are prefixed if tech is reused for ad' assert.strictEqual(prefixed.callCount, 6, 'prefixed events fired'); }); -QUnit.skip('player events during midrolls are prefixed if tech is reused for ad', function(assert) { +QUnit.test('player events during midrolls are prefixed if tech is reused for ad', function(assert) { var prefixed, unprefixed; assert.expect(2); @@ -922,7 +922,7 @@ QUnit.skip('player events during midrolls are prefixed if tech is reused for ad' assert.strictEqual(prefixed.callCount, 6, 'prefixed events fired'); }); -QUnit.skip('player events during postrolls are prefixed if tech is reused for ad', function(assert) { +QUnit.test('player events during postrolls are prefixed if tech is reused for ad', function(assert) { var prefixed, unprefixed; assert.expect(2); From 0ec71e26ddb85b03125fcadb151bafdee2283442 Mon Sep 17 00:00:00 2001 From: Greg Smith Date: Thu, 22 Sep 2016 15:08:13 -0400 Subject: [PATCH 3/6] Fix tests, restore mystery code --- src/videojs.ads.js | 19 +++++++++++++++++-- test/videojs.ads.test.js | 6 +++--- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/videojs.ads.js b/src/videojs.ads.js index 852e7fee..6f4b6e6a 100644 --- a/src/videojs.ads.js +++ b/src/videojs.ads.js @@ -393,11 +393,26 @@ var } else if (player.ads.state === 'content-playback' && event.type === 'ended') { triggerEvent('content', event); - // Content resuming is complicated + // Event prefixing during content resuming is complicated } else if (player.ads.state === 'content-resuming') { + // This does not happen during normal circumstances. I wasn't able to reproduce + // it, but the working theory is that it handles cases where restoring the + // snapshot takes a long time. + if (player.ads.snapshot && + player.currentSrc() !== player.ads.snapshot.currentSrc) { + + // Don't prefix `loadstart` event + if (event.type === 'loadstart') { + return; + } + + // All other events get "content" prefix + return triggerEvent('content', event); + } + // Content resuming after postroll - if (player.ads.snapshot && player.ads.snapshot.ended) { + else if (player.ads.snapshot && player.ads.snapshot.ended) { // Don't prefix `pause` and `ended` events // They don't always happen during content-resuming, but they might. diff --git a/test/videojs.ads.test.js b/test/videojs.ads.test.js index 21c6134c..8058d08b 100644 --- a/test/videojs.ads.test.js +++ b/test/videojs.ads.test.js @@ -880,7 +880,7 @@ QUnit.test('player events during prerolls are prefixed if tech is reused for ad' // simulate video events that should be prefixed this.player.on(['loadstart', 'playing', 'pause', 'ended', 'firstplay', 'loadedalldata'], unprefixed); - this.player.on(['adloadstart', 'adplaying', 'adpause', 'adended', 'adfirstplay', 'adloadedalldata'], prefixed); + this.player.on(['adloadstart', 'adcontentplaying', 'adpause', 'adended', 'adfirstplay', 'adloadedalldata'], prefixed); this.player.trigger('firstplay'); this.player.trigger('loadstart'); this.player.trigger('playing'); @@ -911,7 +911,7 @@ QUnit.test('player events during midrolls are prefixed if tech is reused for ad' // simulate video events that should be prefixed this.player.on(['loadstart', 'playing', 'pause', 'ended', 'firstplay', 'loadedalldata'], unprefixed); - this.player.on(['adloadstart', 'adplaying', 'adpause', 'adended', 'adfirstplay', 'adloadedalldata'], prefixed); + this.player.on(['adloadstart', 'adcontentplaying', 'adpause', 'adended', 'adfirstplay', 'adloadedalldata'], prefixed); this.player.trigger('firstplay'); this.player.trigger('loadstart'); this.player.trigger('playing'); @@ -943,7 +943,7 @@ QUnit.test('player events during postrolls are prefixed if tech is reused for ad // simulate video events that should be prefixed this.player.on(['loadstart', 'playing', 'pause', 'ended', 'firstplay', 'loadedalldata'], unprefixed); - this.player.on(['adloadstart', 'adplaying', 'adpause', 'adended', 'adfirstplay', 'adloadedalldata'], prefixed); + this.player.on(['adloadstart', 'adcontentplaying', 'adpause', 'adended', 'adfirstplay', 'adloadedalldata'], prefixed); this.player.trigger('firstplay'); this.player.trigger('loadstart'); this.player.trigger('playing'); From fc8adb6465cc5ece711cb77f821a7c6e1fa54fa2 Mon Sep 17 00:00:00 2001 From: Greg Smith Date: Thu, 22 Sep 2016 15:58:45 -0400 Subject: [PATCH 4/6] improve comment --- src/videojs.ads.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/videojs.ads.js b/src/videojs.ads.js index 6f4b6e6a..aeef3d3d 100644 --- a/src/videojs.ads.js +++ b/src/videojs.ads.js @@ -398,7 +398,7 @@ var // This does not happen during normal circumstances. I wasn't able to reproduce // it, but the working theory is that it handles cases where restoring the - // snapshot takes a long time. + // snapshot takes a long time, such as in iOS7 and older Firefox. if (player.ads.snapshot && player.currentSrc() !== player.ads.snapshot.currentSrc) { From 176dc00a5c5caad3f21b9e0ce11d32f8d4d5d1a4 Mon Sep 17 00:00:00 2001 From: Greg Smith Date: Fri, 23 Sep 2016 16:18:43 -0400 Subject: [PATCH 5/6] fix comment and function name --- src/videojs.ads.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/videojs.ads.js b/src/videojs.ads.js index aeef3d3d..ecd03948 100644 --- a/src/videojs.ads.js +++ b/src/videojs.ads.js @@ -442,8 +442,8 @@ var // "vjs-has-started" should be present at the end of a video. In this case we need // to re-add it manually. - // Not sure why this happens on ended, I've never seen a case where that is needed. - player.on(['pause', 'ended'], function redispatch() { + // Not sure why this happens on pause, I've never seen a case where that is needed. + player.on(['pause', 'ended'], function() { if (player.ads.state === 'content-resuming' && player.ads.snapshot && player.ads.snapshot.ended) { From 10176f4205d9a50afc16cb7a9857a01cc41a7988 Mon Sep 17 00:00:00 2001 From: Greg Smith Date: Mon, 26 Sep 2016 16:53:28 -0400 Subject: [PATCH 6/6] Dont send "playing" during play/pause for delayed ads --- src/videojs.ads.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/videojs.ads.js b/src/videojs.ads.js index ecd03948..43bc01de 100644 --- a/src/videojs.ads.js +++ b/src/videojs.ads.js @@ -368,6 +368,12 @@ var !player.ads.videoElementRecycled()) { triggerEvent('ad', event); + // If the ad takes a long time to load, "playing" caused by play/pause can happen + // during "ads-ready?" instead of "preroll?" or "ad-playback", skipping the + // other conditions that would normally catch it + } else if (event.type === 'playing' && player.ads.state === 'ads-ready?') { + triggerEvent('ad', event); + // When an ad is playing in content tech, we would normally prefix // "playing" with "ad" to send "adplaying". However, when we did a play/pause // before the preroll, we already sent "adplaying". This condition prevents us @@ -824,7 +830,10 @@ var }); }, events: { - // in the case of a timeout, adsready might come in late. + // In the case of a timeout, adsready might come in late. + // This assumes the behavior that if an ad times out, it could still + // interrupt the content and start playing. An integration could + // still decide to behave otherwise. 'adsready': function() { player.trigger('readyforpreroll'); },