Closed mimse closed 5 years ago
Hi there! This is a really important feature so I spent some time testing it and exploring how things work.
I work on an ad integration for which source changes during ads work without this change, so I was really curious why it worked there but didn't work in the example integration. I found that, in short, source change during ads works as long as ad mode is ended when the source changes. This works because ad mode ending puts you in the ContentPlayback state, and then the contentchanged
handling that already exists takes care of the rest.
According to the videojs-contrib-ads contract and documentation, the ad integration is responsible for calling endLinearAdMode. So I started looking for a way to address this in example-plugin.js
. I thought it was probably possible with videojs middleware but didn't quite get it working.
Then I realized your PR does indeed call endLinearAdMode in app.js, and that should be enough. So I copied your PR with just the change to app.js and it seems to work.
So, in short: it looks like your change to app.js
is sufficient and the change to Preroll.js
is not necessary.
I'd be willing to accept that change, but since endLinearAdMode should be invoked by the ad plugin, a solution that is constrained within example-plugin.js
may be a more useful example to anyone who wants to use this project and get the benefit of source changing during ads.
Hi @incompl,
Thanks for having a look at my PR.
I have been testing a lot the last week and haven't been able to get it working as defined in the documentation.
This is the events I´m seeing running the example with my modifications,
And here without my changes to Preroll state
As you can see, it plays an ad after the source is changed. Which is correct. But it plays as content and not as an Ad.
Also getting VIDEOJS: WARN: Unexpected startLinearAdMode invocation (Preroll) in console.
I do think the trick to this is going to be calling endLinearAdMode at the right time on source change, not adding a state transition. If you're getting "Unexpected startLinearAdMode" it's a sign that the change to the state transition logic is creating an unexpected flow. I don't believe that adding a state transition is going to be likely to address this.
I created a PR to try to address it by ending linear ad mode in middleware. Could you try it out? https://github.com/videojs/videojs-contrib-ads/pull/455
It solves preroll, but what about other adbreak types?
Hi @mysuf
That is the thing. I do not have the full overview of this codebase to know if I'm breaking stuff in other places or this needs to be added to other types of adbreaks to.
This is why I created this PR :-)
Hi. I only pointed that out. If transition properly resets current state (e.g. clearing timeouts, pending events etc), then I'm voting for transition. If not, then @incompl 's middleware PR is probably better way.
I think this PR has potential to be a better solution, but it's not there at this point.
Requiring endLinearAdMode
to be called by the external code that is changing the source (in this example, app.js
is not tenable in practice: we don't have control over the code that changes sources and we can't ask that every 3rd party piece of code that changes sources contain videojs-contrib-ads support. Speaking as the developer of production ad plugins that uses videojs-contrib-ads, this would not be a criteria I'd be able to accept and deploy to our users. This was the main goal I set out to resolve in my PR. Maybe there is a better way than using middleware, like calling endLinearAdMode in the state transition code, but I haven't been able to find one yet.
A different issue is that when I test this code, there is a delay of a couple seconds before the new ad/content plays. I'm not sure why, but this suggests to me that something isn't cleaned up and it doesn't work until a timeout fires. In order to have confidence that the code functions as intended, I'd need to see that delay explained or fixed.
I wish I had time to dig into this more, but it's a doozy of an issue. For now, that's where I'm at.
I think (I dont know this plugin like you do @incompl) that the best solution would be contrib-ads event handler on source middleware since plugin registration. Then you never miss any event and you'll get more options. Before contrib-ads getAds init, it could be basic event handler watching for load events. With getAds, it could be replaced with full event handler. Functions are passed by reference so you can reuse listener (to preserve dispatch order) and only to replace function inside. Another step would be reset() method that will properly reset contrib-ads to default state (like when you initialize plugin first time) on content change when contrib-ads will be in other than default state.
Requiring endLinearAdMode to be called by the external code that is changing the source (in this example, app.js is not tenable in practice: we don't have control over the code that changes sources and we can't ask that every 3rd party piece of code that changes sources contain videojs-contrib-ads support.
This is what videojs-ima is doing.
Not saying it is the right way of doing it. But that is where I got the idea from :-)
The main issue is whether the videojs-contrib-ads should support player zapping during an ad? If it is on the roadmap, I agree @mysuf the videojs-contrib-ads need be updagted. Otherwise #455 workaround is good to make sure contrib-ads in correct state.
This PR fixes this videojs/videojs-contrib-ads#450 issue I created.