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

Fix Player.buffered() to more correct behavior #643

Closed
wants to merge 2 commits into from

Conversation

Mikhus
Copy link
Contributor

@Mikhus Mikhus commented Jul 15, 2013

The problem is that Player.buffered() always return the first time range. This means that while do seeking through the timeline, progress loader is not updated as far as browser sends new buffering info at the tail of buffered time-ranges, so, looks like it would be more correctly to grab this info from the last time-range object rather than from the first one.

The problem is that Player.buffered() always return the first time range. This means that while do seeking threough the timeline, progress loader is not updated as far as browser send new buffering info at the tail of buffered time-ranges, so, looks like it would be more correctly to grab this info from the last time-range object rather than from the first one.
@heff
Copy link
Member

heff commented Jul 29, 2013

Interesting, I didn't realize the browsers were finally using more than one range. That means we could build a loader bar with multiple ranges now.

This looks like a good update to me. Could you remove the extra space before buflen in the parenthesis ( buflen - 1) ? You could also probably change buflen to buflast since you're doing the -1 calculation twice, but up to you.

@Mikhus
Copy link
Contributor Author

Mikhus commented Jul 29, 2013

I'm not actually sure that all browsers do that in the same way (but at least Chrome, Opera and FF looks like do due to my investigations). But at least those which do that in current implementation will behave more correctly with the buffer bar. BTW, to make multiple ranges on a bar it will also require implement support for passing correct ranges in flash fallback. That's just to not forget about this :)

@heff
Copy link
Member

heff commented Jul 29, 2013

Yeah, good thought. The flash fallback doesn't support pseudo streaming yet, so we'll only have one range in Flash for the time being anyway.

@Mikhus
Copy link
Contributor Author

Mikhus commented Jul 29, 2013

I'd be glad to help add pseudo-streaming to flash, but it requires some effort I can't provide at present time. For my mutual project based on videojs I'd added this support but it was done not in generic way and for MP4 only. BTW, if someone will decide to do that in nearest future, that's what I could mention. Do not forget to add NetworkStream and NetworkConnection close() calls to adapters' die() methods. Without that there would be really funny effects like interfered sound effects from old piece of video :)

If after some time there would be no one who work on this task, I'll probably take a part, but right now I can't do any promise on that.

@heff
Copy link
Member

heff commented Jul 29, 2013

No problem. Thanks for the tip!

@Mikhus
Copy link
Contributor Author

Mikhus commented Jul 29, 2013

You'r welcome, thank you for the great project, it helps a lot! BTW, seems like Tom-Johnson already fixed 2 weeks ago a problem stated by me above (just looked to Flash source code) :) So my tip is not so useful as I expected it could be :)

@heff
Copy link
Member

heff commented Jul 29, 2013

Great, even better. :)

@heff heff closed this in 75ff273 Jul 29, 2013
@Mikhus
Copy link
Contributor Author

Mikhus commented Jul 30, 2013

So, looks like you are not going to merge this, or you just forgot to do this? Seems like I did all the changes you requested for this fix

@heff
Copy link
Member

heff commented Jul 30, 2013

No, it's merged. I use Pulley for pull requests, which merges locally instead of through Github, so can look a little weird. But check the commit log. You're in there.

@Mikhus
Copy link
Contributor Author

Mikhus commented Jul 30, 2013

Ok, thanks a lot, I just wait for it to update my code with new videoj src
:)
30.07.2013 18:34 ÐÏÌØÚÏ×ÁÔÅÌØ "Steve Heffernan" notifications@github.com
ÎÁÐÉÓÁÌ:

No, it's merged. I use Pulley https://github.com/jeresig/pulley for
pull requests, which merges locally instead of through Github, so can look
a little weird. But check the commit log. You're in there.

Reply to this email directly or view it on GitHubhttps://github.com//pull/643#issuecomment-21799050
.

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

Successfully merging this pull request may close these issues.

2 participants