Bug 45101 – [GStreamer] ::buffered() should return multiple ranges in some cases

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Philippe Normand]

If on-disk buffering is enabled and if the user seeks to a non-downloaded region, the ::buffered() result should reflect this by returning more than one timerange.

This bug highly depends on https://bugzilla.gnome.org/show_bug.cgi?id=623121

[Philippe Normand]

Created attachment 66615 [details]
proposed patch

[Philippe Normand]

Not marked for review yet, I should write a layout test and probably wait the GStreamer features needed by this patch are released ;)

[Philippe Normand]

Created attachment 72950 [details]
proposed patch

Now with a test!

[Gustavo Noronha (kov)]

Comment on attachment 72950 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72950&action=review

Looks good overall. I'd just recommend doing the check for having exactly 2 buffered ranges, like we discussed on IRC.

> WebCore/platform/gtk/RenderThemeGtk.cpp:934
> +            rangeRect.setLocation(IntPoint(trackRect.x() + (start / mediaDuration)* totalWidth , trackRect.y()));

I believe you should not need the parens here, or else put the multiplication inside it, any of them would make the code more obvious to me. Also, there's a rogue space before the ',' =)

[Eric Seidel (no email)]

Comment on attachment 72950 [details]
proposed patch

This is a bad idea.  Please don't do this.

Unless mod_bw comes installed in a normal Apache distribution, you're asking that *every* webkit developer install mod_bw in order to run the layout tests.

This is not acceptable.

[Philippe Normand]

(In reply to comment #5)
> (From update of attachment 72950 [details])
> This is a bad idea.  Please don't do this.
> 
> Unless mod_bw comes installed in a normal Apache distribution, you're asking that *every* webkit developer install mod_bw in order to run the layout tests.
> 
> This is not acceptable.

I don't think mod_php comes with normal Apache distribution and still we require it.

[Philippe Normand]

> 
> I don't think mod_php comes with normal Apache distribution and still we require it.

Oh well at least on Debian apache2.2-common depends on libapache2-mod-php5 ... So, I'll have a look at automagically download mod_bw with the auto-install stuff like you suggested..

[Mark Rowe (bdash)]

Why is mod_bw necessary for this?  Couldn’t a similar result be achieved by having a script feed up the file in the desired manner?

[Gustavo Noronha (kov)]

(In reply to comment #8)
> Why is mod_bw necessary for this?  Couldn’t a similar result be achieved by having a script feed up the file in the desired manner?

A discussion on the mailing list led us in this direction - Philippe is working on getting this done with a cgi that is already in use today as a base instead.

[Philippe Normand]

Created attachment 73229 [details]
proposed patch

[Philippe Normand]

Note: Gstreamer core and plugins-base 0.10.31 have been released. Bots might need an update of these packages when this patch lands or the test will fail.

[Philippe Normand]

Ok the bots have been upgraded to latest GStreamer packages! :) Can we give this patch a shot Gustavo?

[Gustavo Noronha (kov)]

Comment on attachment 73229 [details]
proposed patch

Let's do it

[Philippe Normand]

Thanks, landed in http://trac.webkit.org/changeset/73454 !