-
Notifications
You must be signed in to change notification settings - Fork 257
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
More complete support for stitched ad scenarios. #284
Conversation
misteroneill
commented
Sep 8, 2017
•
edited
Loading
edited
- Use player timeout methods to avoid even the slight risk of memory leaks (and obviating the need to clean them up manually on dispose).
- Do not cancel content play when using stitched ads.
- Do not use snapshots when playing stitched ads.
- Avoid loading states when playing stitched ads.
Looks awesome. Is player.setTimeout new in some version of videojs or did we just miss it? |
It's been in Video.js for a long time (supported by the minimum dependent version here). It's not a well-known feature, though. Also has |
feb5d1c
to
49a8d56
Compare
@@ -273,12 +275,18 @@ const contribAdsPlugin = function(options) { | |||
// Return true if content playback should mute and continue during ad breaks. | |||
// This is only done during live streams on platforms where it's supported. | |||
// This improves speed and accuracy when returning from an ad break. | |||
shouldPlayContentBehindAd(somePlayer) { | |||
shouldPlayContentBehindAd(somePlayer = player) { |
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.
Might as well default to the in-scope player.
Is this still a WIP / still incoming? |
e9d7c42
to
6b5e4c4
Compare
@incompl So far, I haven't seen anything to indicate this work is insufficient for our stitched ads use-case; so, I think it's fair to say it's no longer WIP. 👍 |
Great in that case I've reviewed and this LGTM. |
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.
LGTM!
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.
One thing I forgot about:
_stitchedAds
should go into the ads.reset()
method to make sure it's cleaned up on content source changes.
I believe ads.stitchedAds(val)
is the only method that allows users to directly change internal state, so if the user wants this behavior to continue for subsequent sources, the method should be called again. That said, I'm open to discussion!
6b5e4c4
to
c835699
Compare
c835699
to
7f499e0
Compare
Re-implementing this against contrib-ads 6 would be more than a rebase. It would be best to take an approach that reflects the recent architectural changes. I won't have time in the near-term to do that, though. Closing, for now. |