Bug 45101

Summary: [GStreamer] ::buffered() should return multiple ranges in some cases
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, slomo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
proposed patch
none
proposed patch
eric: review-
proposed patch gustavo: review+

Philippe Normand
Reported 2010-09-02 03:43:17 PDT
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
Attachments
proposed patch (2.82 KB, patch)
2010-09-05 23:48 PDT, Philippe Normand
no flags
proposed patch (383.22 KB, patch)
2010-11-04 09:54 PDT, Philippe Normand
eric: review-
proposed patch (19.65 KB, patch)
2010-11-08 02:31 PST, Philippe Normand
gustavo: review+
Philippe Normand
Comment 1 2010-09-05 23:48:52 PDT
Created attachment 66615 [details] proposed patch
Philippe Normand
Comment 2 2010-09-06 01:56:05 PDT
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
Comment 3 2010-11-04 09:54:39 PDT
Created attachment 72950 [details] proposed patch Now with a test!
Gustavo Noronha (kov)
Comment 4 2010-11-04 10:13:49 PDT
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 5 2010-11-05 01:52:41 PDT
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
Comment 6 2010-11-05 01:59:35 PDT
(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
Comment 7 2010-11-05 04:03:32 PDT
> > 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)
Comment 8 2010-11-05 04:43:11 PDT
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)
Comment 9 2010-11-05 11:16:22 PDT
(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
Comment 10 2010-11-08 02:31:21 PST
Created attachment 73229 [details] proposed patch
Philippe Normand
Comment 11 2010-12-02 07:43:59 PST
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
Comment 12 2010-12-07 06:08:59 PST
Ok the bots have been upgraded to latest GStreamer packages! :) Can we give this patch a shot Gustavo?
Gustavo Noronha (kov)
Comment 13 2010-12-07 12:37:40 PST
Comment on attachment 73229 [details] proposed patch Let's do it
Philippe Normand
Comment 14 2010-12-07 12:48:23 PST
Note You need to log in before you can comment on or make changes to this bug.