Bug 29999 – [GStreamer] Should handle unknown duration better

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

[Sebastian Dröge (slomo)]

Hi,
currently there's only a check for -1 duration if GStreamer before 0.10.23 is used.

This is wrong, you should always check if

a) returned duration is != -1 (<= 0 is wrong, it could be <-1 because in TIME format it really is an unsigned integer with -1 as special value)
b) fmt is still GST_FORMAT_TIME

in case a) and b) are not true the duration is simply unknown.

[Sebastian Dröge (slomo)]

Sames goes for the query_duration() calls in BYTE format btw.

[Benjamin Otte]

Created attachment 40703 [details]
patch for duration query

Here's a patch for the time duration query. I'm not flagging it for review yet even though I'm pretty sure it's correct, but I'd like Sebastian to look it over just to be sure.

[Sebastian Dröge (slomo)]

Almost correct, yes. Just remove the complete GStreamer version check and only check for time!=GST_CLOCK_TIME_NONE. "negative" times are always ok because the time really is a unsigned integer.

[Benjamin Otte]

Created attachment 40785 [details]
patch for duration query, try 2

[Sebastian Dröge (slomo)]

Looks good to me

[Jan Alonzo]

Comment on attachment 40785 [details]
patch for duration query, try 2


> +    if (!gst_element_query_duration(m_playBin, &timeFormat, &timeLength) ||
> +        timeFormat != GST_FORMAT_TIME ||
> +        timeLength == GST_CLOCK_TIME_NONE) {

We don't normally break the expression into multiple lines. I'll fix it up when landing.

Is it possible to test this or is this covered by our existing tests already?

r=me.

[Jan Alonzo]

(In reply to comment #6)
> (From update of attachment 40785 [details])
> 
> > +    if (!gst_element_query_duration(m_playBin, &timeFormat, &timeLength) ||
> > +        timeFormat != GST_FORMAT_TIME ||
> > +        timeLength == GST_CLOCK_TIME_NONE) {
> 
> We don't normally break the expression into multiple lines. I'll fix it up when
> landing.
> 
> Is it possible to test this or is this covered by our existing tests already?
> 
> r=me.

Landed as http://trac.webkit.org/changeset/49774