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(FEC-10275): Bumper incorrectly recognised as ad #352

Merged
merged 10 commits into from
Sep 2, 2020
Merged

Conversation

dan-ziv
Copy link
Contributor

@dan-ziv dan-ziv commented Aug 20, 2020

Description of the Changes

Player's getAd() API shouldn't return Ad object while ad is only loaded, this can cause issues when IMA loads its ads so early:

  1. AD_LOADED is triggered while media is playing - getAd() returns ad but no ad is currently playing.
  2. AD_LOADED is triggered while another ad/bumper is playing - getAd() returns the wrong ad.

Solution:

  • To access the loaded ad from the player's API, use getPendingAd() API. In such way, we are solving the above issues:
  1. AD_LOADED is triggered while media is playing - getAd() returns nothing, getPendingAd() returns the loaded ad.
  2. AD_LOADED is triggered while another ad/bumper is playing - getAd() returns the current ad, getPendingAd() returns the next ad.
  • change playAdsWithMSE implementation to listen to AD_LOADED event and only if the pending ad is linear, listen to ad break start to run the playAdsWithMSE logic.

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

@dan-ziv dan-ziv requested a review from a team August 20, 2020 15:06
@dan-ziv dan-ziv self-assigned this Aug 20, 2020
src/common/controllers/ads-controller.js Outdated Show resolved Hide resolved
Copy link
Contributor

@yairans yairans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dan-ziv What's about ad pod which has adloaded with no adbreakstart after (from the second ad in the pod). You can add check this is the first ad in pod using ad.index/position. otherwise dont listen to asbreakstart

@@ -280,11 +291,12 @@ class AdsController extends FakeEventTarget implements IAdsController {

_onAdLoaded(event: FakeEvent): void {
this._adIsLoading = false;
this._ad = event.payload.ad;
this._pendingAd = event.payload.ad;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dan-ziv same issue as I mentioned in playkit, postroll IMA+BUMPER postroll.
you'll get the incorrect pending ad since IMA post-roll loads before bumper, on bumper ad start playing getPendingAd will return null and needs to return IMA postroll.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because I added pendingAd this issue will reflect in an API call. But it's a separate issue that has been around all along: you'll trigger AD_LOADED event twice with a different ad payload.
So, anyway, I think we need to handle it in a different ticket and not overload it here. We can remove in the meanwhile the pendingAd API to avoid this issue. @OrenMe @yairans what do u think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, if you keep the pendingAd API it should be fixed since it won't return the correct value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove it as no one is using it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So agreed - let's remove from here and open ticket to handle the next ad issue.
I think it is ok to have a nextAd object with this exact same logic as you added here.
This way we have a flow where when ad is loaded you can get it in getnextAd and when it starts we clear nextAd and update the _ad object that can be retrieved in getAd.
And when another ad get's loaded it will be populated in the _nextAd - this can happen in several ways, if this is an ad pod then while current ad is playing the next will be loaded or for IMA specifically when last midroll finish and there is a postroll the postroll is already loaded.

yairans
yairans previously approved these changes Sep 1, 2020
OrenMe
OrenMe previously approved these changes Sep 2, 2020
@OrenMe OrenMe requested a review from Yuvalke September 2, 2020 11:32
OrenMe
OrenMe previously approved these changes Sep 2, 2020
@dan-ziv dan-ziv merged commit 716d01a into master Sep 2, 2020
@dan-ziv dan-ziv deleted the FEC-10275 branch September 2, 2020 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants