GNOME Bugzilla – Bug 623121
[queue2] downloaded areas of the media are not exposed
Last modified: 2010-09-03 18:09:13 UTC
With a buffering query you currently only get informations about the current buffered range. However if the user seeks beyond the current stop value of the range, queue2 will start a new range. It would be nice if application side could make use of that and display all the buffered ranges when they represent separate parts of the downloaded media. So this means we'd need an array representing those ranges in the buffering query result.
What exactly do you need? An array of structures containing a) the start byte position, b) the stop byte position and c) if it's the currently buffering range? That could be easily added I guess
I would keep the current fields (start/stop of the current range) and add an array with all the values (even position = start, odd position = stop or somethign similar) The result of this query should be generated rather quickly too.
Yes I'd be fine with what Wim is suggesting. Values would be in byte I guess, then I can do the same math I do with the stats of the current range.
Would this API be acceptable? /** * gst_query_set_buffering_ranges: * @query: a GST_QUERY_BUFFERING type query #GstQuery * @ranges: array of media buffered ranges. * * Set the buffering-ranges result field in @query. The @ranges array * should contain #gint64 items. Odd indexes store start positions and * even indexes store stop range positions. * * Since: 0.10.31 */ void gst_query_set_buffering_ranges (GstQuery * query, GValue* ranges); /** * gst_query_parse_buffering_ranges: * @query: a GST_QUERY_BUFFERING type query #GstQuery * * Parse an available query and get the buffered ranges array. It * should contain #gint64 items. Odd indexes store start positions and * even indexes store stop range positions. * * Returns: an array of media buffered ranges. * * Since: 0.10.31 */ const GValue* gst_query_parse_buffering_ranges (GstQuery * query);
Maybe use a GValueArray here instead... or say in the docs that this is a NULL terminated array. And let set_buffering_ranges() copy the array instead of taking ownership of it (and mark it const). Other than that this looks good but I still don't like the even/odd index interface here :)
Created attachment 168629 [details] [review] gstvalue: new GstInt64Range type
Created attachment 169148 [details] [review] gstquery: new buffering_ranges API Added a new query type to retrieve informations about the areas of the media currently buffered. See bug 623121. API: gst_query_parse_buffering_ranges API: gst_query_set_buffering_ranges
Created attachment 169149 [details] [review] queue2: buffering_ranges query support Fixes bug 623121
Review of attachment 169148 [details] [review]: ::: gst/gstquery.c @@ +1304,3 @@ + gst_structure_id_set (structure, GST_QUARK (BUFFERING_RANGES), + G_TYPE_VALUE_ARRAY, ranges_copy, NULL); + g_value_array_free (ranges_copy); Why do you copy the array twice? One time manually and one time with gst_structure_id_set() @@ +1329,3 @@ + value = gst_structure_id_get_value (structure, GST_QUARK (BUFFERING_RANGES)); + ranges = g_value_array_copy ((GValueArray *) g_value_get_boxed (value)); + return ranges; For other queries, events, etc the API usually is, that the parsed data is owned by the query and only valid as long as the query is there.
(In reply to comment #9) > Review of attachment 169148 [details] [review]: > > ::: gst/gstquery.c > @@ +1304,3 @@ > + gst_structure_id_set (structure, GST_QUARK (BUFFERING_RANGES), > + G_TYPE_VALUE_ARRAY, ranges_copy, NULL); > + g_value_array_free (ranges_copy); > > Why do you copy the array twice? One time manually and one time with > gst_structure_id_set() > Heh, yeah sorry ;) Will be fixed in next iteration of the patch. > @@ +1329,3 @@ > + value = gst_structure_id_get_value (structure, GST_QUARK > (BUFFERING_RANGES)); > + ranges = g_value_array_copy ((GValueArray *) g_value_get_boxed (value)); > + return ranges; > > For other queries, events, etc the API usually is, that the parsed data is > owned by the query and only valid as long as the query is there. Well the other queries handle simpler types, at least from what I read in gstquery.c... I don't really see how to avoid the copy here.
(In reply to comment #10) > > @@ +1329,3 @@ > > + value = gst_structure_id_get_value (structure, GST_QUARK > > (BUFFERING_RANGES)); > > + ranges = g_value_array_copy ((GValueArray *) g_value_get_boxed (value)); > > + return ranges; > > > > For other queries, events, etc the API usually is, that the parsed data is > > owned by the query and only valid as long as the query is there. > > Well the other queries handle simpler types, at least from what I read in > gstquery.c... I don't really see how to avoid the copy here. Just "return (GValueArray *) g_value_get_boxed (value)" I was thinking of the _get_structure() methods of event, query and message or the taglist getters. OTOH gst_query_get_uri() returns a copy. We need a second opinion here :)
Hrm, I really dislike that we use/expose a GValueArray in the API here. How do you feel about something like instead? void gst_query_add_buffering_byte_range (query, start, stop); gboolean gst_query_get_buffering_byte_range (query, &start, &stop, n); That could then internally still use a GValueArray if needed (or a G_TYPE_POINTER with plain malloced array, not sure it matters).
> gboolean gst_query_get_buffering_byte_range (query, &start, &stop, n); Or gboolean gst_query_get_buffering_byte_range (query, n, &start, &stop);
That could work too, I guess... is everybody ok with Tim's proposal?
Yes, sounds good. That would be a bit like the formats query
Right, then I'd remove "byte" and add a format argument: void gst_query_add_buffering_range (query, format, start, stop); gboolean gst_query_get_buffering_range (query, format, n, &start, &stop); I'll go ahead with that unless someone have something to object ;)
(In reply to comment #16) > Right, then I'd remove "byte" and add a format argument: > > void gst_query_add_buffering_range (query, format, start, stop); > > gboolean gst_query_get_buffering_range (query, format, n, &start, &stop); > Now that I thought more about it, it feels weird to have format there. It's ambiguous because there's already a format member in the query structure. So what about this? void gst_query_add_buffering_range (query, start, stop); gboolean gst_query_get_buffering_range (query, n, &start, &stop); Sebastian also asked for: gint gst_query_get_n_buffering_ranges (query); And add_buffering_range should also ensure the internal array of ranges is correctly sorted by start positions.
Sounds good
> Now that I thought more about it, it feels weird to have format there. It's > ambiguous because there's already a format member in the query structure. So > what about this? > > void gst_query_add_buffering_range (query, start, stop); > > gboolean gst_query_get_buffering_range (query, n, &start, &stop); Right, that's why I used _byte_range here, because it's always in bytes regardless of the format of the query, and to set these function apart from the existing gst_query_set_buffering_range (query, format, start, stop, est_total); gst_query_parse_buffering_range (query, format, start, stop, est_total);
> Right, that's why I used _byte_range here, because it's always in bytes Ok, scratch that, it could also be in PERCENT or TIME or whatever, so I guess the values are in the format of the query after all. Sorry for the noise.
Created attachment 169227 [details] [review] gstquery: new buffering_ranges API Added a new query type to retrieve informations about the areas of the media currently buffered. See bug 623121. API: gst_query_add_buffering_range API: gst_query_get_n_buffering_ranges API: gst_query_parse_nth_buffering_range
Comment on attachment 169149 [details] [review] queue2: buffering_ranges query support I have updated that one but I want to add FORMAT_TIME support before sending it.
Created attachment 169344 [details] [review] queue2: buffering_ranges query support Fixes bug 623121
commit 410ca5c164d308a8f18758c6dbb86336aa7b335d Author: Philippe Normand <pnormand@igalia.com> Date: Tue Aug 31 11:37:42 2010 +0200 queue2: buffering_ranges query support Fixes bug 623121 commit 36e1ad94e1775e866cd207c3b7260814d53178d2 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Sep 3 19:58:49 2010 +0200 gstquery: Only fill the start/stop values of the buffering ranges if a non-N commit 9ef1c4707906ff7cfdb61ea04e31f42152962194 Author: Philippe Normand <pnormand@igalia.com> Date: Tue Aug 31 11:35:12 2010 +0200 gstquery: new buffering_ranges API Added a new query type to retrieve informations about the areas of the media currently buffered. See bug 623121. API: gst_query_add_buffering_range API: gst_query_get_n_buffering_ranges API: gst_query_parse_nth_buffering_range