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

Progress bar #1253

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@brainopia
Contributor

brainopia commented May 29, 2014

Hello, I've done a few things in this pull-request.

  1. player.buffered now returns a true timerange with all regions. I'm going to make a video.js plugin which will pick a fastest mirror based on tracking buffered information.

    Previous approach to player.buffered leads to a following situation. If you seek to the later part of the video and then to the earlier one then player.buffered would return result for the former region and therefore no progress could be tracked until both regions would merge.

  2. Change load-progress-bar to display correct information about all buffered regions.

    Our users would complain that we display incorrect buffered information. When seeking back and forward across video. Also there is a bug in Opera http://www.sitepoint.com/essential-audio-and-video-events-for-html5/ which can lead to preloading the region with the small part at the end of the video which would be displayed in previous approach as a fully buffered video.

  3. Support for player.played (with multiple regions).

  4. Change play-progress-bar to display player.played regions (otherwise information of buffered regions in earlier parts of the video won't be visible).

@mmcc mmcc added bug labels May 29, 2014

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc May 29, 2014

Member

Awesome, thanks for the PR! I believe this work would address #1073 as well. It's going to be a few days before someone can dig into this one for review, but in the meantime would you mind taking a look at the tests? Looks like there are quite a few failing at the moment.

Member

mmcc commented May 29, 2014

Awesome, thanks for the PR! I believe this work would address #1073 as well. It's going to be a few days before someone can dig into this one for review, but in the meantime would you mind taking a look at the tests? Looks like there are quite a few failing at the moment.

@brainopia

This comment has been minimized.

Show comment
Hide comment
@brainopia

brainopia May 30, 2014

Contributor

Tests has been fixed (I didn't know that there is a separate Tech for tests).

#1073 — looks like related to stale cache, the easiest fix is to get a time directly from player. The more difficult but correct fix would take a bit of effort to trace why is cache getting stale after seeking. I can take a look if you want.

Also I think I haven't made a big enough emphasis on changes in this pull-request. Besides technical fixes to player.buffered and player.played, I've took a bold step and changed an UI logic for progress-control. The best way to understand these changes is to see them in action:

Take a look — and try to seek back-and-forward and play different parts of the video (if you won't seek back-and-forward then almost nothing changes).

Contributor

brainopia commented May 30, 2014

Tests has been fixed (I didn't know that there is a separate Tech for tests).

#1073 — looks like related to stale cache, the easiest fix is to get a time directly from player. The more difficult but correct fix would take a bit of effort to trace why is cache getting stale after seeking. I can take a look if you want.

Also I think I haven't made a big enough emphasis on changes in this pull-request. Besides technical fixes to player.buffered and player.played, I've took a bold step and changed an UI logic for progress-control. The best way to understand these changes is to see them in action:

Take a look — and try to seek back-and-forward and play different parts of the video (if you won't seek back-and-forward then almost nothing changes).

@brainopia

This comment has been minimized.

Show comment
Hide comment
@brainopia

brainopia May 30, 2014

Contributor

Also we can turn the UI-change for progress-bar into config setting 😄

Contributor

brainopia commented May 30, 2014

Also we can turn the UI-change for progress-bar into config setting 😄

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Jun 3, 2014

Member

This is really cool! This seems like the kind of thing that would definitely need to be configurable (from the UI side at least) since I think it would confuse some users. Things get a little busy when you click around:

busy

Member

mmcc commented Jun 3, 2014

This is really cool! This seems like the kind of thing that would definitely need to be configurable (from the UI side at least) since I think it would confuse some users. Things get a little busy when you click around:

busy

Show outdated Hide outdated src/js/control-bar/progress-control.js
};
// remove unloaded buffered ranges
for (var i=0; i < (children.length - buffered.length); i++) {

This comment has been minimized.

@heff

heff Jun 11, 2014

Member

children here is a live NodeList, so children.length is going to change as you remove elements from it, and not work as this expects. e.g. with 4 children and 2 buffered

@heff

heff Jun 11, 2014

Member

children here is a live NodeList, so children.length is going to change as you remove elements from it, and not work as this expects. e.g. with 4 children and 2 buffered

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 11, 2014

Member

Great stuff, thanks! I've been wanting to update the load progress to actually use the multiple time ranges for a long time now. This is a nice implementation of that.

Using played data for the progress bar is a very cool demo, but people really expect the progress bar to represent the current time. So I think that part would be better fit for a plugin, that shows the played ranges in addition to the existing bars. We've got to be really careful about what we pull into core video.js, and having it as a plugin will allow us to validate that people want to use it.

otherwise information of buffered regions in earlier parts of the video won't be visible

That is true. Have you seen any other players handle that issue? I wonder if users care too much about that, and if they do I'd be interested if there's other UI changes that we could make so the loading is visible without changing how the play progress bar works.

We should open another issue for that part, but in the mean time if you can remove the play progress bar changes (moving them to a plugin), we should be able to accept this.

Thanks again!

Member

heff commented Jun 11, 2014

Great stuff, thanks! I've been wanting to update the load progress to actually use the multiple time ranges for a long time now. This is a nice implementation of that.

Using played data for the progress bar is a very cool demo, but people really expect the progress bar to represent the current time. So I think that part would be better fit for a plugin, that shows the played ranges in addition to the existing bars. We've got to be really careful about what we pull into core video.js, and having it as a plugin will allow us to validate that people want to use it.

otherwise information of buffered regions in earlier parts of the video won't be visible

That is true. Have you seen any other players handle that issue? I wonder if users care too much about that, and if they do I'd be interested if there's other UI changes that we could make so the loading is visible without changing how the play progress bar works.

We should open another issue for that part, but in the mean time if you can remove the play progress bar changes (moving them to a plugin), we should be able to accept this.

Thanks again!

Show outdated Hide outdated src/js/media/flash.js
@@ -309,6 +309,10 @@ vjs.Flash.prototype.buffered = function(){
return vjs.createTimeRange(0, this.el_.vjs_getProperty('buffered'));
};
vjs.Flash.prototype.played = function(){
return vjs.createTimeRange(0, this.el_.vjs_getProperty('currentTime'));
};

This comment has been minimized.

@heff

heff Jun 11, 2014

Member

This is a decent fallback. Long term I don't think it would be too hard to store the played ranges manually. It would be similar to the manual progress events we fire for flash.

@heff

heff Jun 11, 2014

Member

This is a decent fallback. Long term I don't think it would be too hard to store the played ranges manually. It would be similar to the manual progress events we fire for flash.

@brainopia

This comment has been minimized.

Show comment
Hide comment
@brainopia

brainopia Jun 16, 2014

Contributor

@heff @mmcc I've removed multiple play-progress regions, but have decided to try another strategy to display information about buffered regions prior to current time — adding a bit of opacity to play-progress control. You can try out the result.

@heff I've fixed the issue with children.length

I can squash the commits if you wish 😉

Contributor

brainopia commented Jun 16, 2014

@heff @mmcc I've removed multiple play-progress regions, but have decided to try another strategy to display information about buffered regions prior to current time — adding a bit of opacity to play-progress control. You can try out the result.

@heff I've fixed the issue with children.length

I can squash the commits if you wish 😉

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Jun 17, 2014

Member

@heff and I just discussed the play progress stuff offline, and while it's definitely an improvement, we agree that it's probably not a great fit for core. We can always show people how to bump down the opacity in docs / guides, or even extract that out to a plugin.

Ultimately, we can revisit the play progress styling later, but for now let's try to keep this PR simple and focus on the loading progress aspect. You're welcome to squash your commits, but that happens automatically during our acceptance process, so it's not a big deal either way.

Member

mmcc commented Jun 17, 2014

@heff and I just discussed the play progress stuff offline, and while it's definitely an improvement, we agree that it's probably not a great fit for core. We can always show people how to bump down the opacity in docs / guides, or even extract that out to a plugin.

Ultimately, we can revisit the play progress styling later, but for now let's try to keep this PR simple and focus on the loading progress aspect. You're welcome to squash your commits, but that happens automatically during our acceptance process, so it's not a big deal either way.

@brainopia

This comment has been minimized.

Show comment
Hide comment
@brainopia

brainopia Jun 17, 2014

Contributor

Gotcha. Removed opacity. Therefore on UI side only multiple buffer-regions are left.

Contributor

brainopia commented Jun 17, 2014

Gotcha. Removed opacity. Therefore on UI side only multiple buffer-regions are left.

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Jun 17, 2014

Member

Awesome, thanks so much. We don't want to surprise folks too much in a minor version update :)

Member

mmcc commented Jun 17, 2014

Awesome, thanks so much. We don't want to surprise folks too much in a minor version update :)

@brainopia

This comment has been minimized.

Show comment
Hide comment
@brainopia

brainopia Jun 25, 2014

Contributor

@mmcc Is there anything else I can do to help to move this pull-request along?

Contributor

brainopia commented Jun 25, 2014

@mmcc Is there anything else I can do to help to move this pull-request along?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 25, 2014

Member

Honestly, if we could split the played related changes out into their own pull request, this would be a lot cleaner to pull in. Do you think you'd be able to do that?

Member

heff commented Jun 25, 2014

Honestly, if we could split the played related changes out into their own pull request, this would be a lot cleaner to pull in. Do you think you'd be able to do that?

@heff heff added enhancement and removed bug labels Jun 25, 2014

@brainopia

This comment has been minimized.

Show comment
Hide comment
@brainopia

brainopia Jul 8, 2014

Contributor

Sure, I've reverted played property.

Contributor

brainopia commented Jul 8, 2014

Sure, I've reverted played property.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 28, 2014

Member

This is great work, thanks! Sorry it's taken a bit to get through this.

I ran into an issue where percentify was throwing an error in IE8 because it was returning NaN when time and duration were both zero. I'll send over an update in a second for you to check out.

Member

heff commented Jul 28, 2014

This is great work, thanks! Sorry it's taken a bit to get through this.

I ran into an issue where percentify was throwing an error in IE8 because it was returning NaN when time and duration were both zero. I'll send over an update in a second for you to check out.

@@ -117,7 +117,9 @@ vjs.Slider.prototype.update = function(){
}
// Set the new bar width
bar.el().style.width = vjs.round(barProgress * 100, 2) + '%';
if (bar) {

This comment has been minimized.

@heff

heff Jul 28, 2014

Member

Just wondering, why was this check needed?

@heff

heff Jul 28, 2014

Member

Just wondering, why was this check needed?

This comment has been minimized.

@brainopia

brainopia Jul 28, 2014

Contributor

If I remember correctly, it was needed to implement play-progress regions based on player.played instead of current time. But since it was dropped from this pull-request, then this condition can safely be removed.

@brainopia

brainopia Jul 28, 2014

Contributor

If I remember correctly, it was needed to implement play-progress regions based on player.played instead of current time. But since it was dropped from this pull-request, then this condition can safely be removed.

This comment has been minimized.

@heff

heff Jul 29, 2014

Member

Ok well, it doesn't hurt for now. Check out the pull request I made against your branch and let me know if it works for you. Then we can get this merged in.

@heff

heff Jul 29, 2014

Member

Ok well, it doesn't hurt for now. Check out the pull request I made against your branch and let me know if it works for you. Then we can get this merged in.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 31, 2014

Member

Updated my pull request so that the progress bar now encapsulates all the individual time ranges. Just having broken up time ranges made the bar look a little broken. Now it's a little more subtle.

If anyone has any thoughts let me know.
screen shot 2014-07-31 at 3 18 25 pm

Member

heff commented Jul 31, 2014

Updated my pull request so that the progress bar now encapsulates all the individual time ranges. Just having broken up time ranges made the bar look a little broken. Now it's a little more subtle.

If anyone has any thoughts let me know.
screen shot 2014-07-31 at 3 18 25 pm

@heff heff closed this in b2fbc80 Aug 1, 2014

heff added a commit that referenced this pull request Aug 1, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Aug 1, 2014

Member

@brainopia Thanks for your help on this! Just merged it in and it'll go out with the next minor release.

Would you still be interested in putting together something for the played property?

If you're on IRC check out the #videojs channel.

Member

heff commented Aug 1, 2014

@brainopia Thanks for your help on this! Just merged it in and it'll go out with the next minor release.

Would you still be interested in putting together something for the played property?

If you're on IRC check out the #videojs channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment