-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: More complete support for stitched ad scenarios. #415
Conversation
[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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the info here was incorrect. Seems better to send people to the correct documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I appreciate this
moduleName: 'videojsContribAds', | ||
entry: 'src/plugin.js', | ||
name: 'videojsContribAds', | ||
input: 'src/plugin.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was getting tired of the deprecation warnings being logged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
src/plugin.js
Outdated
@@ -172,11 +172,11 @@ const contribAdsPlugin = function(options) { | |||
player.ads = getAds(player); | |||
|
|||
player.ads.settings = settings; | |||
player.ads.stitchedAds(settings.stitchedAds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to come before we initialize the BeforePreroll
state because that state relies on this being set correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, might be a good comment to add
<script src="integration/lib/shared-module-hooks.js"></script> | ||
<script src="dist/bundle.js"></script> | ||
</body> | ||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file doesn't do anything.
src/ads.js
Outdated
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the same for the initial source, that it should be called before a source is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
src/states/BeforePreroll.js
Outdated
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll try it.
src/states/Preroll.js
Outdated
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically we're still waiting for ad break until startLinearAdMode, even though with stitched ads this will be a short time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see skipping the loading spinner, I wonder if nopreroll should be handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
3bed7ef
to
de09f8a
Compare
Note to self, tests to add:
|
docs/integrator/options.md
Outdated
@@ -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. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update this based on the decision to deprecate this usage.
src/ads.js
Outdated
|
||
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need _stitchedAds
as things stand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't, good point. Removing.
|
||
var simulateStartOfAd = function(time) { | ||
return waitForCurrentTime(time, function() { | ||
player.on(['timeupdate', 'adtimeupdate'], simulateEndOfAd(time + 5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why both, by the way? Seems like only adtimeupdate
should be needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably correct. Copy/paste is the explanation, I'm sure.
}); | ||
|
||
// Simulate a mid-roll at 15 seconds. | ||
player.on(['timeupdate', 'adtimeupdate'], simulateStartOfAd(15)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, would expect only timeupdate
needed.
In attempting to adopt this work in our SSAI plugin, I realized that the best thing we were doing was to fire So, I wonder if that behavior ought to just be implicit in the |
@misteroneill What changes would that involve? |
I guess handling stitched ads as if |
I have looked at 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. |
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. |
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. |
I'll be working on new tests for the re-thinking of this PR and then will submit for re-approval. |
3ca4caa
to
ec3e88c
Compare
{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}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows you to refresh the Karma Chrome instance and get the latest built files.
// Initialize contrib-ads. | ||
player.ads(options); | ||
|
||
player.on('canplay', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why canplay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mainly so that you can replay the example video.
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so straightforward now 💯
|
||
// Tell redispatch not to intercept ended events. This should prevent | ||
// the firing of readyforpostroll. | ||
this.player.ads._contentHasEnded = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so you want the sequence to be adended -> leave ad mode -> ended rather than ended -> leave ad mode. That makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly.
LGTM |
This PR is a re-implementation of #284.
This is a WIP still in that tests need fleshing out for these scenarios.