Player.play() doesn't always cause transition from ads-ready to preroll?

[shawnbuso]

I'm the owner of the videojs-ima plugin. I'm doing a small re-write to get it up to date with the latest contrib-ads plugin. I'm running into an issue with my playlist sample - every other time I call play(), the content just plays instead of causing a transition from ads-ready to preroll?. As a result I only see ads for every other video.

Steps to repro:

1. Load my sample.
2. Open the dev console.
3. Click the little Android dude in the video 1 thumbnail (don't click the video to play it).
4. After the pre-roll, click the doubleclick logo in the video 2 thumbnail.

In my testing I see the following: Page loads I click on the Android thumbnail to request ads for that video Ads plugin transitions content-set -> ads-ready After triggering ads-ready, the implementation calls player.play() Ads plugin transitions ads-ready -> preroll? -> ad-playback -> content-resuming -> content-playback I click on the doubleclick thumbnail to request new ads for that video Ads plugin transitions content-playback -> content-set -> ads-ready After triggering ads-ready, the implementation calls player.play() Ads plugin does not transition to preroll?, the content just plays

If you repeat this process you continue to see ads for every other video, no matter which video you start with (e.g. if you click the doubleclick ad first instead of the Android ad, then that will always have ads and the android ad will not). Any tips on what may be going wrong here?

[incompl]

Howdy, I'm happy to help figure this out. What are the versions of contrib-ads you're migration from and to? That'll help me narrow down the cause.

[shawnbuso]

Awesome, thanks so much! We're upgrading from VJS 5.3.0 and contrib-ads 5.0.3 to VJS 5.20.4 and contrib-ads 5.1.0.

I also just noticed we're getting an error now - when I switch videos, I'm seeing "VIDEOJS: ERROR: TypeError: Cannot read property 'trackChangeHandler' of null". Oddly enough I still see ads for every other video, even though I'm seeing this error every time I switch.

[marguinbc]

Hi @shawnbuso, that error seems to be a result of code introduced to address an issue with iOS changing the state of textTracks in videojs-contrib-ad. In theory, it should only be called when playing on iOS but clearly there's an issue where we should probably be checking to make sure the trackChangeHandler exists.

As for your original issue, I'll defer to incompl since he already volunteered to assist.

[marguinbc]

Found the line of code, here, it looks like we should also be checking to make sure we have a snapshot before trying to check a parameter on it.

[alex-barstow]

Incoming PR that will at least fix the TypeError: https://github.com/videojs/videojs-contrib-ads/pull/292

[incompl]

It looks like trackChangeHandler is the original issue, so looks like we're already on track to resolve this bug. I'll continue discussion in the PR. We'll ship an update for you as soon as this is merged.

[mimse]

reset: function reset() {
      player.ads.disableNextSnapshotRestore = false;
      player.ads._contentEnding = false;
      player.ads._contentHasEnded = false;
      player.ads.snapshot = null;
      player.ads.adType = null;
      player.ads._hasThereBeenALoadedData = false;
      player.ads._hasThereBeenALoadedMetaData = false;
      player.ads._cancelledPlay = false;
    }

I fixed by resetting _cancelledPlay back to false when the content is being updated``

I will make a PR for it

[incompl]

@mimse Interesting, I'll definitely look into that. Last night we tested the other PR we thought would fix this issue, so I'm going to do a release with that, and regardless of whether or not that fixes this particular issue, I'll look at your PR soon after.

[mimse]

@incompl If "cancelledplay" is not set to false when updating content, the play event is dispatched as contentplay event, and content-set -> play is never run.

Hope this makes sense

[incompl]

@misme Yup it makes sense, I just want to consider if this is the best place for the code, if we should remove the old logic that sets cancelledplay to false, test it out on some players, etc.

[incompl]

I released videojs-contrib-ads@5.1.1 with the first fix from this thread, I'll look at the new PR once I've finished some Proprietary Business™ that I need to attend to.

[incompl]

I released videojs-contrib-ads@5.1.2 with the other fix from this thread. @shawnbuso can you confirm if one of these resolves your issue?

[shawnbuso]

That release does fix the trackChangeHandler issue, but I'm still seeing the issue where every other video has no ads. I'm still not getting a transition from adsready to preroll?. I've updated the sample page in my initial report to use the new contrib-ads 5.1.2.

[mimse]

@shawnbuso in all browsers?

[incompl]

I'll take another look at this as soon as I'm able.

[shawnbuso]

So far I've only tested on desktop - Chrome on Ubuntu and Mac, and Safari on Mac. Let me know if there are any other specific browsers you'd like me to take a look at.

[incompl]

I took a deep dive and debugged your example in detail. Your issue is caused by this code:

https://github.com/videojs/videojs-contrib-ads/blob/master/src/plugin.js#L666-L675

I need to further investigate the purpose of this code is before removing or changing it, but I made a build of contrib-ads without this code and made a copy of your example using this build. In this example you can see that the ad plays for the second video:

http://solutions.brightcove.com/gsmith/contrib-ads/

[marguinbc]

To add to what @incompl mentioned above, it appears as though that code, when minified, translates to

contentupdate: function() {
  s.ads.shouldPlayContentBehindAd(s) || o(s), s.paused() ? this.state = "content-set" : this.state = "ads-ready?"
  }

where o(s) is evaluating to false as there is no return value, causing the state to never be reset to ads-ready?. Not sure why that is happening but if I det a breakpoint on that line and manually set player.ads.state="ads-ready?" in the console, the next ad plays fine.

Do with this information what you will. :)

[incompl]

I am planning to fix this and release an update this week. I just need to take the time to grab some Android phones and make sure I don't cause a regression.

[mimse]

@incompl are you doing 5.1.3, so we can test? :-)

[incompl]

I tested this on some Android phones on Friday and found no issues. Published 5.1.3.

[shawnbuso]

Just tested my sample with 5.1.3 and it looks like this is fixed :) Thanks!

[incompl]

🎉

[incompl]

We just found a bug in a different ad integration that is fixed by this. Thanks to your bug report, we already have a fix. Appreciate it! 🕺