Skip to content
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

fix: Resolve dangling endLinearAdMode call in onAdBreakEnd. #574

Merged
merged 1 commit into from
Mar 27, 2018
Merged

fix: Resolve dangling endLinearAdMode call in onAdBreakEnd. #574

merged 1 commit into from
Mar 27, 2018

Conversation

shawnbuso
Copy link
Contributor

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

Addresses #539

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

mysuf commented Mar 21, 2018

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
Copy link
Contributor Author

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
Copy link

mysuf commented Mar 21, 2018

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
Copy link
Contributor Author

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
Copy link

mysuf commented Mar 21, 2018

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
Copy link
Contributor Author

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
Copy link

mysuf commented Mar 21, 2018

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
Copy link

mysuf commented Mar 21, 2018

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
Copy link

mysuf commented Mar 22, 2018

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
Copy link
Contributor Author

No worries, thanks!

@shawnbuso shawnbuso merged commit 2158ba0 into googleads:master Mar 27, 2018
bustbr added a commit to bustbr/videojs-ima that referenced this pull request Apr 24, 2018
…oogleads#574)"

This reverts commit 2158ba0.

breaks compatibility to `videojs-contrib-ads` v4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants