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

Test against VJS 5 and 6 #539

Closed
shawnbuso opened this issue Feb 28, 2018 · 15 comments
Closed

Test against VJS 5 and 6 #539

shawnbuso opened this issue Feb 28, 2018 · 15 comments
Assignees

Comments

@shawnbuso
Copy link
Contributor

contrib-ads 6 allows us to add support for VJS6. Legacy implementations will like still use VJS5 at least for some time. We should investigate adding support for testing against VJS 5 and 6 instead of just one.

@mysuf
Copy link

mysuf commented Mar 2, 2018

Hello, I just tested vjs6.6.3 with contrib-ads6.0.0 and vjs-ima1.2.0. I need to know that all(ads and video) completed (to display related videos) so I created custom event 'allcompleted' like this. It is ugly, but hadn't better idea how to do that.

player.on("adsready", () => {
     player.ima.addEventListener(google.ima.AdEvent.Type.ALL_ADS_COMPLETED, () => {
          player.trigger('allcompleted');
	  player.on('ended', () => player.trigger('allcompleted'));
    });
});

But player.on('ended') has now delay about 5s from time when ads and video really ended. Is this delay expected in new version? I will provide sample page if needed.

It also prints warning:

video.bundle.js:5 VIDEOJS: WARN: Unexpected endLinearAdMode invocation (State via r)

Thanks

Btw. I think that ads should be handled through beforeplay and beforeended events to provide place to play preroll and postroll. player's "ended" event should be considered as absolutely final event that tells "there is nothing more to play"... nor add final event to player like 'completed' or so..

With vjs6.6.3, contrib-ads5.16.x and vjs-ima1.1.x, it worked fine without delay (except short blink of original video before preroll started)..

@ypavlotsky
Copy link
Contributor

Thanks for the info!

@incompl
Copy link

incompl commented Mar 2, 2018

Unexpected endLinearAdMode invocation (State via r) isn't what I'd expect that error to look like: where it says r I would expect it to say something like ContentPlayback, Postroll, or AdsDone. Did it really say r or was there a copy/paste issue?

@mysuf
Copy link

mysuf commented Mar 2, 2018

Sorry. I use minified version. I will provide this error with better description;)

@incompl
Copy link

incompl commented Mar 2, 2018

Ohh, that makes sense. We'll fix this log warning in contrib-ads. This warning is really helpful for debugging these cases. I opened an issue: videojs/videojs-contrib-ads#338

@mysuf
Copy link

mysuf commented Mar 2, 2018

I don't use minified file from contrib-ads/dist. I bundle all together through commonJS, browserify and gulp so I'm not sure that it helps. This is non-minified debug warn msg:

video.bundle.js:9852 VIDEOJS: WARN: Unexpected endLinearAdMode invocation (State via AdsDone)

@incompl
Copy link

incompl commented Mar 2, 2018

Interesting, that suggests that endLinearAdMode is invoked after the postroll ad break has already ended. That may be something to investigate in the videojs-ima source to verify contrib-ads 6 support.

Another bit of useful information is if you're seeing an adtimeout event. In contrib-ads 6, we actually removed a delay before the ended event. If you're seeing it later, it may be because the postroll ad is timing out. If your timeout setting is 5 seconds, and the ended event is delayed 5 seconds, that is more evidence that could assist in debugging for sure.

@incompl
Copy link

incompl commented Mar 2, 2018

I looked at the source a bit and saw endLinearAdMode in reset. If reset can be invoked outside of an ad break (it seems like it can) you may want to consider wrapping the endLinearAdMode in if (player.ads.inAdBreak()) {...}.

@mysuf
Copy link

mysuf commented Mar 2, 2018

I will provide sample page with non-minified source within hour.. The issue is that it delays every 'ended' event even if all ads are completed (playing another source, repeat is ok).

Meanwhile I logged event order:
adsready
preroll playing(ok)
preroll ends(ok)
adend fires
video playing(ok)
video ends(ok)
delay few secs(nothing happens)
adtimeout fires
postroll playing(ok)
postroll ends(ok)
warn: unexpected endLinearAdMode invocation ..
allAdsCompleted(ima event) fires
adtimeout fires

When I play next, it keeps firing adtimeout (2per one playing) with delayed ended event.

@mysuf
Copy link

mysuf commented Mar 2, 2018

@incompl
Copy link

incompl commented Mar 2, 2018

Interesting, it looks like there is a pause while the player waits for a postroll but no postroll plays, then a postroll timeout, and then the postroll plays after the expected postroll break. This is probably the source of the strange behavior.

Does videojs-ima play postrolls on the ended event? For postroll timeouts and ended events to work correctly, postrolls should play on contentended. Here is a relevant except from the new documentation:

  • Contrib Ads triggers contentended (EVENT) -- This event means that it's time to play a postroll ad.
  • To play a Postroll ad, start and end an ad break with player.ads.startLinearAdMode() and player.ads.endLinearAdMode().
  • 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.

@shawnbuso
Copy link
Contributor Author

It looks like we are using ended instead of contentended - I'll create a PR to fix.

shawnbuso added a commit that referenced this issue Mar 14, 2018
Also changes reset() to verify that we're in an ad break before calling endLinearAdMode().

Addresses comments in #539.
@mysuf
Copy link

mysuf commented Mar 21, 2018

@incompl There is still uncatched endLinearAdMode() call in onAdBreakEnd. It prints warning when postroll ends. I switched if (player.ads.inAdBreak()) to onAdBreakEnd and deleted it in reset method (just oposite as its now) and it works fine (tried only preroll and postroll). The other thing is that I dont know why.)

@mysuf
Copy link

mysuf commented Mar 29, 2018

With @incompl help we probably found another issue using contrib-ads v6 with ima v1.2.1. videojs/videojs-contrib-ads#358. Can you look at it?

This part of code:

PlayerWrapper.prototype.onAdBreakStart = function (adEvent) {
  this.contentSource = this.vjsPlayer.currentSrc();
  this.vjsPlayer.off('contentended', this.boundContentEndedListener);
  if (adEvent.getAd().getAdPodInfo().getPodIndex() != -1) {
    // Skip this call for post-roll ads
    this.vjsPlayer.ads.startLinearAdMode();
  }
  this.vjsControls.hide();
  this.vjsPlayer.pause();
};

From what said incompl, couldnt be this an issue?

@Kiro705
Copy link
Member

Kiro705 commented Oct 15, 2019

This issue looks resolved. We test vjs 5 and 6 in our Travis tests

@Kiro705 Kiro705 closed this as completed Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants