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

Player triggers ended twice #358

Closed
mysuf opened this issue Mar 28, 2018 · 17 comments
Closed

Player triggers ended twice #358

mysuf opened this issue Mar 28, 2018 · 17 comments
Labels

Comments

@mysuf
Copy link
Contributor

mysuf commented Mar 28, 2018

Using contrib-ads v6, player triggers ended event with contentended and then second time when postroll ends. It could break components that depends on ended event. Is this behaviour expected? I would expect that contrib-ads will throttle default ended event to end of postroll. Thanks.

@incompl
Copy link
Contributor

incompl commented Mar 28, 2018

Which version of contrib-ads are you using? There was an issue with double ended in 6.0.0 that was fixed in 6.0.1.

@incompl incompl added the bug label Mar 28, 2018
@mysuf
Copy link
Contributor Author

mysuf commented Mar 28, 2018

Thats my case (using 6.0.0). I will check it with 6.0.1 and close this if it resolves that. Thank you.

@mysuf
Copy link
Contributor Author

mysuf commented Mar 28, 2018

With 6.0.1 it seems even worse, because second ended event triggers earlier (together with adtimeout). I am using videojs-ima so it could be also related to videojs-ima issue.

@incompl
Copy link
Contributor

incompl commented Mar 28, 2018

Do you have an example I could look at to help diagnose the issue? It would also be useful to see if you can reproduce it in the contrib-ads example integration, because if you can, that shows it is a contrib-ads issue.

@mysuf
Copy link
Contributor Author

mysuf commented Mar 28, 2018

This is part of event log from player instance when content video ends:

15.305s: timeupdate // playing content video
15.307s: timeupdate // playing content video
15.772s: timeupdate // playing content video
15.859s: timeupdate // playing content video
15.862s: pause // ima onAdBreakStart handles pause when contentended event 
15.867s: contentended
15.867s: ended
20.870s: ended
20.870s: adtimeout

Here is a sample page: https://games.tiscali.cz/player/vjs-testpage.html

Insert this into console before play:

var player = videojs.getAllPlayers()[0]
var defaultTrigger = player.trigger;
player.trigger = function(e, t) {
	console.log(e.type||e);
	return defaultTrigger.call(player, e, t);
}

Btw. sample page is v6.0.0 but first ended triggers with contentended in both versions so it is probably contrib-ads issue. Second ended can be possibly wrong handled by videojs-ima.

@incompl
Copy link
Contributor

incompl commented Mar 28, 2018

I could not reproduce the issue with 6.0.1 in the sample page, which would be needed for me to expect ended to work correctly, did you try it that way?

@mysuf
Copy link
Contributor Author

mysuf commented Mar 28, 2018

I will. I have many components including ima plugin that change source so ofc issue can be also there. I tried to find "trigger('ended" in whole project folder without any result. So I am thinking what can cause such behavior.. :/

@incompl
Copy link
Contributor

incompl commented Mar 28, 2018

Ended is a standard media event, it happens without any code explicitly triggering it: https://developer.mozilla.org/en-US/docs/Web/Events/ended

videojs-contrib-ads has a feature that tries to make sure that media events such as ended happen as expected, even with the presence of ads: https://github.com/videojs/videojs-contrib-ads/#redispatch

@mysuf
Copy link
Contributor Author

mysuf commented Mar 28, 2018

Yea. I understand. The only reason why I reported it here is because it happens at exactly same time as contrib-ads contentended and adtimeout events.

@incompl
Copy link
Contributor

incompl commented Mar 28, 2018

OK cool, just wanted to provide the info in case it was helpful.

@mysuf
Copy link
Contributor Author

mysuf commented Mar 28, 2018

This does not explain ended when contentended, but maybe explains second one. In sample above, there is missing adstart followed by adend when postroll plays. Is that why adtimeout triggers ended? If true, I will report it to videojs-ima.

@incompl
Copy link
Contributor

incompl commented Mar 28, 2018

The missing adstart / adend observation led me to understanding the issue. Adstart happens when the integration (in this case, videojs-ima) calls startLinearAdMode. This is when the ad beak begins. I noticed this in the debugger in your example page:

screen shot 2018-03-28 at 3 41 08 pm

It seems videojs-ima does not call startLinearAdMode for the postroll, so contrib-ads does not realize that an ad is playing. Notice the result of player.ads.inAdBreak() in the preroll vs. postroll. This would definitely explain strange behavior.

@mysuf
Copy link
Contributor Author

mysuf commented Mar 28, 2018

Ok. I've got it. That makes sense. But does this bug explain ended together with contentended?

@incompl
Copy link
Contributor

incompl commented Mar 28, 2018

I'm not 100% sure, but the event redispatching relies on the player state matching what it expects, so it seems like a likely cause for this issue. handleEnded in redispatch.js checks both isInAdMode and isContentResuming, both of which could be wrong if an ad is playing without startLinearAdMode and endLinearAdMode being called.

@mysuf
Copy link
Contributor Author

mysuf commented Mar 28, 2018

Ok. There is still chance that its my mistake, because I take care of ads.contentSrc everytime player.src() is changed (there is quality switcher and related videos). I still dont properly understand how IMA SDK handle player's video element and changing source and when create its own. On desktop its always new video elements created by SDK so I thought it wouldnt be a problem. I will check also this scenario and report it after proved. Thanks!

@mysuf
Copy link
Contributor Author

mysuf commented Mar 29, 2018

Removed ads.contentSrc handling and still same so definetely ima issue.

@mysuf
Copy link
Contributor Author

mysuf commented Apr 4, 2018

I tried your example and it looks correct and clear. So I am closing this issue as its probably all around ima implementation..

@mysuf mysuf closed this as completed Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants