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

improvement: PlayProgressBar Smooth Transition #4591

Merged
merged 2 commits into from Nov 7, 2017

Conversation

@vhmth
Copy link
Contributor

@vhmth vhmth commented Aug 28, 2017

Instead of tying the play progress update lifecycle to the timeupdate
event, the update happens every 15ms. This makes it so the play progress
transitions smoothly.

Fixes #4526

Description

Here's a video!

https://www.useloom.com/share/d578023adf3545e8b97a065d22d2ea92

Specific Changes proposed

Instead of relying on the timeupdate event exclusively to update the play progress bar, we set an interval on play and clear this interval on pause and ended.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors
Copy link
Member

@misteroneill misteroneill left a comment

Thanks for the PR!

I'd like other core contributors to weigh in, but I have some concerns.

Intervals tend not to be a great way to do animation because they aren't very precise (anything that blocks the UI thread blocks/offsets execution of timers). If we were to stop using timeupdate to trigger progress bar updates, I think it'd be better to make use of requestAnimationFrame.

Also, the loss of throttling concerns me. The update() function is not fast and causes reflow/repaint. Calling it on a 15ms interval might cause performance problems on slower/mobile devices.

src/js/control-bar/progress-control/seek-bar.js Outdated
}, 15);
});

this.on(player, ['ended', 'pause'], () => {

This comment has been minimized.

@misteroneill

misteroneill Aug 28, 2017
Member

This list might not be sufficient. I could potentially see including things like abort, emptied, error, and maybe others. Additionally, I'd be curious to know what happens when including contrib-ads and its event redispatch behavior (such as, would it need to be updated to clear this interval in its own cases).

This comment has been minimized.

@vhmth

vhmth Aug 28, 2017
Author Contributor

Agreed here. Probably more cases I can cover. I'd like to test these plus others that can be found in Html5.events (or something like that - I was grep-happy last night and it came up on my radar).

This comment has been minimized.

@heff

heff Aug 29, 2017
Member

I think the video element is pretty good about firing a pause whenever it's 'not playing', even on ended, but that's worth verifying. Contrib-ads is an interesting question where I have no idea, though this seems like a pretty basic use of play/pause, so if there's a problem with this then contrib-ads probably breaks other things too.

This comment has been minimized.

@vhmth

vhmth Aug 29, 2017
Author Contributor

Not sure what contrib-ads is, but should be an easy search against the codebase. Will update y'all here.

This comment has been minimized.

@vhmth

vhmth Sep 18, 2017
Author Contributor

Seems like switching to playing and adding waiting and stalled is the way to go. I've updated the PR to include those events.

This comment has been minimized.

This comment has been minimized.

@vhmth

vhmth Sep 18, 2017
Author Contributor

@misteroneill @heff do either of y'all know of an easy way to test the redispatch behavior with contrib-ads? It seems like the only events that get special treatment are playing, ended, loadstart, loadedata and loadedmetadata. Even though the 2 of that list that we're using is playing and ended, they seem to be pretty straightforward. Fairly confident it will continue to work since it just ferries those events through. It would be nice to do a quick sanity check on my end though.

This comment has been minimized.

@gkatsev

gkatsev Sep 18, 2017
Member

It shouldn't be an issue because the player controls bind the event listeners before contrib-ads's redispatch runs.

src/js/control-bar/progress-control/seek-bar.js Outdated
// via an interval
this.updateInterval = null;

this.on(player, ['play'], () => {

This comment has been minimized.

@misteroneill

misteroneill Aug 28, 2017
Member

I think this would probably need to be playing as opposed to play.

This comment has been minimized.

@vhmth

vhmth Aug 28, 2017
Author Contributor

Tying to playing will result in the same issue we saw before. We can't tie the update flow to these HTML5 video events since they don't trigger enough to allow for smooth play progress.

This comment has been minimized.

@heff

heff Aug 29, 2017
Member

playing is only fired once when the video actually starts playing after a play and shouldn't make any difference in the update speed, so I agree with @misteroneill that it seems more right to use playing. Maybe try it out and if you do see a difference I'd be really interested to know why.

This comment has been minimized.

@vhmth

vhmth Aug 29, 2017
Author Contributor

Ah actually you guys are completely correct. It seems likeplaying fires only once. No idea why I saw it fire multiple times. I was probably mistaking this for another event.

https://stackoverflow.com/a/14270041/696130

Seems like I should definitely check for waiting too when clearing the interval.

@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Aug 28, 2017

@misteroneill agreed on throttling. I can add back throttling the update function to ensure it only gets fired at most once every 15ms. However, we can't throttle it more than that if we want the play progress bar to appear as though it's progressing smoothly (at least if we do it in JS).

Another option here (that I'm not opposed to) is to make use of CSS transitions. It would require a bit more work, and I noticed janky issues when clicking around (we have to do logic where we remove the transition when someone clicks around the ProgressBar). I'd like to hear from a couple core contributors on this matter. I can write the code no problem, but there seem to be a few gotchas.

@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Aug 28, 2017

Also, tell the Brightcove peeps Loom says hi @misteroneill! 👋 :-)

@mawenmaywin
Copy link

@mawenmaywin mawenmaywin commented Aug 28, 2017

My version of VideoJS doesn't seem to have seperate .js files. How can I implement this with video.dev.js file? Are you sure we are talking about the same plugin here?

@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Aug 28, 2017

@mawenmaywin this PR modifies the source code of video.js, which is only present when you clone the repo, not when you install via npm. You can clone my repo here, checkout the vinay/smooth_play_progress branch, and then build the dist files. Then copy the dist files to your node_modules directory in the application you're testing out locally to see this work.

Alternatively, these changes are pretty small. You can also just modify the dist files directly to add this interval and check it yourself.

@mawenmaywin
Copy link

@mawenmaywin mawenmaywin commented Aug 28, 2017

I'm sorry I am sort of a newb when it comes to plugins and Javascript. I don't understand what you're talking about but I think I will read up on node_modules and npm's.

Copy link
Member

@heff heff left a comment

Nice use of Video.js (via Loom) to help build Video.js! Pretty cool. :)

src/js/control-bar/progress-control/seek-bar.js Outdated
// via an interval
this.updateInterval = null;

this.on(player, ['play'], () => {

This comment has been minimized.

@heff

heff Aug 29, 2017
Member

playing is only fired once when the video actually starts playing after a play and shouldn't make any difference in the update speed, so I agree with @misteroneill that it seems more right to use playing. Maybe try it out and if you do see a difference I'd be really interested to know why.

src/js/control-bar/progress-control/seek-bar.js Outdated
}, 15);
});

this.on(player, ['ended', 'pause'], () => {

This comment has been minimized.

@heff

heff Aug 29, 2017
Member

I think the video element is pretty good about firing a pause whenever it's 'not playing', even on ended, but that's worth verifying. Contrib-ads is an interesting question where I have no idea, though this seems like a pretty basic use of play/pause, so if there's a problem with this then contrib-ads probably breaks other things too.

@vhmth vhmth force-pushed the loomhq:vinay/smooth_play_progress branch 2 times, most recently Sep 18, 2017
@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Sep 18, 2017

Given @gkatsev's update on not needing to test contrib-ads and my updates to the events that this timer binds to, what am I missing here? Looking at the unit tests, it doesn't seem like this really fits into any of them. Happy to add any unit tests y'all think are appropriate though.

On another note, doesn't seem like this should result in any updates to the docs.

I'll be here and dedicated to getting this over the finish line as soon as I get replies. :-)

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Sep 19, 2017

I'm still a bit worried about having a setInterval that runs every 15ms. Though, I do agree with you it'll be nice to get this fixed. Maybe a larger interval would be ok? like 50ms or 100ms?
I know some projects use linear interpolation to calculate where the line should be but that's tricky too.

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Sep 19, 2017

Yup, I don't think we need any docs updated for this change, thanks for thinking of that :)

@gkatsev gkatsev added the patch label Sep 19, 2017
@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Sep 23, 2017

@gkatsev just lmk what you wanna go with. I'm cool with bumping it down to 50.

@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Oct 5, 2017

Hey y'all bump here?

@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Oct 8, 2017

It seems like the main concern might be that the interval is left sitting around when it shouldn't be. Are there any tests you'd like me to write to put everyone's mind at ease? I'd be happy to do that if it helps get this landed faster.

@paulius005
Copy link

@paulius005 paulius005 commented Oct 19, 2017

Any updates here?

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Oct 24, 2017

Hey, thanks for your patience, we went to demuxed (and I even spoke there) and FOMS in early october and that took a lot of energy.

@vhmth thanks for the looms, they were great. Maybe 30ms would be a good compromise? it being 2x of 15 and 30fps is a decent goal :). Thoughts @misteroneill and @heff?

Also, I think bring the throttling back would be good, and of course, it should match the internal we're using. Another thought is to wrap the update call in the interval a requestAnimationFrame. Do you think that makes sense?

@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Oct 25, 2017

@gkatsev no worries. Dope! Good to put a face to the avatar.

Just to be clear, action items:

  1. 30ms timeout
  2. Add a throttle back to the update func that's at 30ms as well
  3. Call requestAnimationFrame within the timeout (seems reasonable)
@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Oct 25, 2017

I can do that very quickly, will wait to hear from others before I update though

@misteroneill
Copy link
Member

@misteroneill misteroneill commented Oct 25, 2017

Yeah, that sounds good.

I'm not 100% sure stalled is needed here - I'm not sure it means playback has stopped.

@heff
Copy link
Member

@heff heff commented Oct 25, 2017

Yeah, stalled here is the network, not playback. Otherwise 👍

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Oct 31, 2017

@vhmth any updates? :) looks like there's some conflicts now due to this pr (#4648).

@vhmth vhmth force-pushed the loomhq:vinay/smooth_play_progress branch Nov 2, 2017
Instead of tying the play progress update lifecycle to the timeupdate
event, the update happens every 15ms. This makes it so the play progress
transitions smoothly.

Fixes #4526
@vhmth vhmth force-pushed the loomhq:vinay/smooth_play_progress branch to 80500a0 Nov 2, 2017
@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Nov 2, 2017

@gkatsev updated and rebased! Still looking good locally and tests are passing still. :-)

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 2, 2017

Awesome, thanks @vhmth!

@gkatsev
gkatsev approved these changes Nov 7, 2017
Copy link
Member

@gkatsev gkatsev left a comment

I removed stalled from the events list. We can always add it back in if we find that we do in-fact want it.

Thanks for your work @vhmth just tried it out locally and it looks great!

@gkatsev gkatsev merged commit acc641a into videojs:master Nov 7, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@vhmth
Copy link
Contributor Author

@vhmth vhmth commented Nov 7, 2017

Sweet!

@apurvgandhwani

This comment has been minimized.

Copy link

@apurvgandhwani apurvgandhwani commented on 80500a0 Aug 24, 2018

What is Fn?

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants