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.

Sign up for GitHub

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(Ads): Limit interstitial duration to actual duration if available #7480

Merged
merged 2 commits into from
Oct 23, 2024
Merged

fix(Ads): Limit interstitial duration to actual duration if available #7480

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4431ad
fix(Ads): Limit interstitial duration to actual duration if available
avelad Oct 23, 2024
923c2eb
Update comment
avelad Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion lib/ads/interstitial_ad_manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,11 +675,26 @@ shaka.ads.InterstitialAdManager = class {
});
try {
this.updatePlayerConfig_();
// playRangeEnd in src= causes the ended event not to be fired when that
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment in context of the next paragraph of code. Could you improve the comments in this whole try/catch? Happy to have a conversation about it if it helps you figure out what is missing from the comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

For src= we use https://www.w3.org/TR/media-frags/ whose implementation in Safari implies that when the playRangeEnd time is reached, if the stream has not finished, what it does is pause the content, but does not emit the ended event.

Copy link
Member

Choose a reason for hiding this comment

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

IIUC, you are saying that this trick of using play range to limit the interstitial will not trigger an inappropriate 'ended' event, so we can then go back to the main content. Is that correct?

If that's right, I would add to this comment:

"So we can still return to the main content after that."

// position is reached. So we don't use it because we would never go back
// to the main stream.
const loadMode = this.basePlayer_.getLoadMode();
if (loadMode == shaka.Player.LoadMode.MEDIA_SOURCE &&
interstitial.startTime && interstitial.endTime &&
interstitial.endTime != Infinity &&
interstitial.startTime != interstitial.endTime) {
const duration = interstitial.endTime - interstitial.startTime;
if (duration > 0) {
this.player_.configure('playRangeEnd', duration);
}
}
if (interstitial.playoutLimit) {
playoutLimitTimer = new shaka.util.Timer(() => {
ad.skip();
}).tickAfter(interstitial.playoutLimit);
this.player_.configure('playRangeEnd', interstitial.playoutLimit);
if (loadMode == shaka.Player.LoadMode.MEDIA_SOURCE) {
this.player_.configure('playRangeEnd', interstitial.playoutLimit);
}
}
await this.player_.attach(this.video_);
if (this.preloadManagerInterstitials_.has(interstitial)) {
Expand Down
Loading