fix: Resolve dangling endLinearAdMode call in onAdBreakEnd.

[shawnbuso]

We don't want this call to happen after post-rolls. This fixes that.

Addresses #539

[mysuf]

I tried this solution, but it does not stop contrib-ads from firing after all ads completed. When I change source (to related video) after postroll and this next video ends, there is again postroll timeout without ad. Something is still wrong.

I did it like this:

PlayerWrapper.prototype.onAdBreakEnd = function () {
  this.vjsPlayer.on('contentended', this.boundContentEndedListener);
  if (this.vjsPlayer.ads.inAdBreak()) {
    this.vjsPlayer.ads.endLinearAdMode();
  }
  this.vjsControls.show();
};

PlayerWrapper.prototype.reset = function () {
  this.vjsPlayer.on('contentended', this.boundContentEndedListener);
  this.vjsControls.show();
  this.vjsPlayer.ads.endLinearAdMode();
  // Reset the content time we give the SDK. Fixes an issue where requesting
  // VMAP followed by VMAP would play the second mid-rolls as pre-rolls if
  // the first playthrough of the video passed the second response's
  // mid-roll time.
  this.contentPlayheadTracker.currentTime = 0;
};

And from some reason, it works as expected (without warnings and without unexpected timeouts on playlist). Thats why I mentioned incompl Maybe he can explain that behaviour.

[shawnbuso]

Is the difference in your code snippet just that you're always calling endLinearAdMode in reset(), regardless of whether or not you're currently in an ad break? I think it still makes sense to check, since we shouldn't be ending linear ad mode if we're not actually in linear ad mode.

[mysuf]

Yes. What you say make sense and your current fix was also first thing I did when trying to make it work. It works with one video (repeating, seeking backwards..), but it triggers postroll timeout on next one. Dunno why.. contrib-ads doc says:

Contrib Ads triggers ended (EVENT) -- This standard media event happens when all ads and content have completed. After this, no additional ads are expected, even if the user seeks backwards.

So I would expect that after ended event, ads goes off. There must be some unfinished bussiness or nvm.

Btw. I serve next video via player.src() and source interceptor.

[shawnbuso]

Hmm if you're accessing player.src directly that could be an issue - it can mess with videojs-ima's internal state machine. Have you tried using player.ima.setContentWithAdTag instead? You can provide null or an empty string for the adTag if you want to use the same one again.

[mysuf]

Do you think that handling player via ima plugin is good idea? Because related videos are displayed at the end of video playback when all ads are finished. Currently we dont need ads on every item in playlist. Also there are cases where ima plugin may not be registered to the player so it chains dependency on inactive ima plugin -> sdk -> contrib-ads -> which leads to if-else handling.

The point is that it doesnt trigger on other videos with contrib-ads v5 and it does with v6. I will look into contrib-ads, but I have feeling that it is related to startLinearAdMode/endLinearAdMode and that endLinearAdMode is not called after postroll. Maybe there can be another case than ads.inAdBreak().

[shawnbuso]

We maintain our own state machine that depends on knowing when the source of the player changes, so we added a method to pass through the source to the player. We could update it to instead rely on events from the player, but that has the potential to be much more buggy and wouldn't tie together the process of setting the content source and the ad tag.

[mysuf]

For that purpose, I still use this as part of source interceptor: https://github.com/videojs/videojs-contrib-ads#contentsrc. For now, feel free to merge and close this. I will look into the code and open new issue if I find something interesting. Thanks.

[mysuf]

When I think about it, it could be also my fault because now I am not sure that videojs.use('*', sourceInterceptor); is triggered before changesource event which you and contrib-ads are probably listening. And wrong order of these events could cause my issue. I will check it tommorow. Thank you once again.

[mysuf]

Ok. So your pull is absolutely correct. player.currentSrc() is actually not current h5player src, but value from internal videojs cache. And I forgot to update this value during manupulation with source. Sorry for such false alarm.

[shawnbuso]

No worries, thanks!