After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 623121 - [queue2] downloaded areas of the media are not exposed
[queue2] downloaded areas of the media are not exposed
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 627826
Blocks:
 
 
Reported: 2010-06-29 11:00 UTC by Philippe Normand
Modified: 2010-09-03 18:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstvalue: new GstInt64Range type (27.93 KB, patch)
2010-08-24 10:21 UTC, Philippe Normand
none Details | Review
gstquery: new buffering_ranges API (7.50 KB, patch)
2010-08-31 10:21 UTC, Philippe Normand
reviewed Details | Review
queue2: buffering_ranges query support (4.01 KB, patch)
2010-08-31 10:22 UTC, Philippe Normand
none Details | Review
gstquery: new buffering_ranges API (9.30 KB, patch)
2010-09-01 10:57 UTC, Philippe Normand
committed Details | Review
queue2: buffering_ranges query support (3.42 KB, patch)
2010-09-02 10:39 UTC, Philippe Normand
committed Details | Review

Description Philippe Normand 2010-06-29 11:00:10 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.
Comment 1 Sebastian Dröge (slomo) 2010-06-29 11:13:18 UTC
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
Comment 2 Wim Taymans 2010-06-29 11:28:24 UTC
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.
Comment 3 Philippe Normand 2010-06-29 11:41:56 UTC
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.
Comment 4 Philippe Normand 2010-08-17 10:10:20 UTC
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);
Comment 5 Sebastian Dröge (slomo) 2010-08-19 10:39:06 UTC
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 :)
Comment 6 Philippe Normand 2010-08-24 10:21:56 UTC
Created attachment 168629 [details] [review]
gstvalue: new GstInt64Range type
Comment 7 Philippe Normand 2010-08-31 10:21:43 UTC
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
Comment 8 Philippe Normand 2010-08-31 10:22:02 UTC
Created attachment 169149 [details] [review]
queue2: buffering_ranges query support

Fixes bug 623121
Comment 9 Sebastian Dröge (slomo) 2010-08-31 10:32:53 UTC
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.
Comment 10 Philippe Normand 2010-08-31 12:37:26 UTC
(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.
Comment 11 Sebastian Dröge (slomo) 2010-08-31 12:41:53 UTC
(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 :)
Comment 12 Tim-Philipp Müller 2010-08-31 15:53:24 UTC
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).
Comment 13 Tim-Philipp Müller 2010-08-31 15:54:35 UTC
>   gboolean gst_query_get_buffering_byte_range (query, &start, &stop, n);

Or  gboolean gst_query_get_buffering_byte_range (query, n, &start, &stop);
Comment 14 Philippe Normand 2010-09-01 07:13:26 UTC
That could work too, I guess... is everybody ok with Tim's proposal?
Comment 15 Sebastian Dröge (slomo) 2010-09-01 07:33:18 UTC
Yes, sounds good. That would be a bit like the formats query
Comment 16 Philippe Normand 2010-09-01 07:52:28 UTC
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 ;)
Comment 17 Philippe Normand 2010-09-01 08:59:08 UTC
(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.
Comment 18 Sebastian Dröge (slomo) 2010-09-01 09:17:42 UTC
Sounds good
Comment 19 Tim-Philipp Müller 2010-09-01 09:33:58 UTC
> 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);
Comment 20 Tim-Philipp Müller 2010-09-01 09:48:25 UTC
> 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.
Comment 21 Philippe Normand 2010-09-01 10:57:51 UTC
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 22 Philippe Normand 2010-09-01 10:59:24 UTC
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.
Comment 23 Philippe Normand 2010-09-02 10:39:50 UTC
Created attachment 169344 [details] [review]
queue2: buffering_ranges query support

Fixes bug 623121
Comment 24 Sebastian Dröge (slomo) 2010-09-03 18:09:03 UTC
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