-
Notifications
You must be signed in to change notification settings - Fork 2
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
Ad Holiday Feature #171
Ad Holiday Feature #171
Conversation
037ad18
to
f09e25a
Compare
d1b81ef
to
0fc1dc7
Compare
this.suppressedEventsController.add( | ||
this.player.exports.PlayerEvent.Paused, | ||
this.player.exports.PlayerEvent.Seek, | ||
this.player.exports.PlayerEvent.Seeked | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these event suppressings are re-added everytime a performBreakSkip
happens, but I don't see that these events get removed again from the break, or maybe I've missed it.
That would mean that after the first time performBreakSkip
was called, the player would not emit any paused/seek/seeked events anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I thought these were one-off suppression events, I based it off of the logic I found here, which also doesn't seem to remove the suppressed events:
bitmovin-player-web-integrations-yospace/src/ts/InternalBitmovinYospacePlayer.ts
Lines 1137 to 1159 in 2de540a
skip: () => { | |
if (this.isAdActive()) { | |
if (this.playerPolicy.canSkip() === 0) { | |
const ad = this.getCurrentAd(); | |
const adBreak = this.getCurrentAdBreak(); | |
const seekTarget = this.getAdStartTime(ad) + this.getAdDuration(ad); | |
if (seekTarget >= this.player.getDuration()) { | |
this.isPlaybackFinished = true; | |
this.suppressedEventsController.add( | |
this.player.exports.PlayerEvent.Paused, | |
this.player.exports.PlayerEvent.Seek, | |
this.player.exports.PlayerEvent.Seeked | |
); | |
this.player.pause(); | |
this.player.seek(toSeconds(adBreak.getStart()) - 1); // -1 to be sure to don't have a frame of the ad visible | |
this.fireEvent({ | |
timestamp: Date.now(), | |
type: this.player.exports.PlayerEvent.PlaybackFinished, | |
}); | |
} else { | |
this.player.seek(seekTarget, 'ad-skip'); | |
} |
but I see now that this logic is for the post-roll, it finishes playback right away, and presumably clears suppressed events the next time a video is loaded.
In theory I could use this same logic to enable post-roll ad immunity, e.g. instead of trying to seek past the post-roll, just end playback when the postroll is detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed suppression, it seems to have caused a bug with the holiday logic as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to be clear, now when skipping an ad break, the player would trigger Pause
, Seek
, Seeked
, Play
, Playing
events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If this is not desired I'll probably need some guidance on how to correctly suppress and unsuppress events for the skip, or help finding the relevant documentation.
With suppression on during the skip I can see edge cases like the user interacting with the player controls while the suppression is still active, before the skip seek has had time to finish. Since the break never activates, the player controls are still usable during the duration of the skip, unlike regular ad skipping.
I think the skip would need to do something like disable UI interaction -> suppress events -> perform skip -> unsuppress events -> enable UI interaction
for it to avoid weird corner case states.
The holiday logic currently relies on the seeked
event, should we want to suppress it I'll need to look at an alternate solution
0c028d6
to
b53de5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good 👍
Would be good to get @saravanans-github's feedback on the open question before approving/merging.
(cherry picked from commit 5fc19b3)
This fixes an issue with ads not being properly deactivated when seeked past
0a070f8
to
f008469
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good from my side, waiting for @saravanans-github and the customer to confirm test results
# Conflicts: # CHANGELOG.md # package-lock.json # package.json
Description
By providing an ad immunity configuration after initialization, a user that has watched an ad break will become immune to ad breaks for a period. Any ad break position passed during the immunity period is permanently disabled, so that it is not possible to enter the break at a later point after the immunity has expired. See changelog for more detailed description of new API methods.
Checklist (for PR submitter and reviewers)
CHANGELOG
entry