Bug 39053 – [GStreamer] cache media duration in READY instead of PLAYING

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

[Philippe Normand]

In most cases the media duration is already available when the pipeline is in GST_STATE_READY. However the current code only caches it when the pipeline reaches GST_STATE_PLAYING. We should cache it in READY to avoid some duration queries.

[Philippe Normand]

Created attachment 55958 [details]
proposed patch

[Eric Seidel (no email)]

I'm not enough of a media expert to know.

[Eric Carlson]

Comment on attachment 55958 [details]
proposed patch

>  
> +        // Cache the media duration if it's known when the pipeline is
> +        // ready for playback.
> +        if (state >= GST_STATE_READY && !m_mediaDuration) {
> +            float newDuration = duration();
> +            m_mediaDurationKnown = !isinf(newDuration);
> +            if (m_mediaDurationKnown)
> +                m_mediaDuration = newDuration;
> +        }
> +

I don't think this, or the old code actually, will do what you want because duration() always returns inf until m_mediaDurationKnown:

    // Media duration query failed already, don't attempt new useless queries.
    if (!m_mediaDurationKnown)
        return numeric_limits<float>::infinity();

It looks like it will work if you call durationChanged() instead. This will also inform HTMLMediaElement of the duration change, which this change also doesn't do.

[Philippe Normand]

Hum I don't agree ;)

This really caches the duration. m_mediaDurationKnown is initially true and m_mediaDuration is 0. So until m_mediaDurationKnown is set, a real duration query will be performed on the pipeline.

It would be much more simpler to have that logic in duration() itself but it's a const method, m_mediaDurationKnown can't be set inside :/

[Eric Carlson]

(In reply to comment #4)
> Hum I don't agree ;)
> 
>  m_mediaDurationKnown is initially true

Ah, I see this now. "m_mediaDurationKnown" is probably not the best name for the flag because it is set when you *don't* know the duration.

In any case durationChanged() has similar logic plus it informs HTMLMediaElement when that the duration has changed. Is there any reason to not call it instead?

[Philippe Normand]

(In reply to comment #5)
> (In reply to comment #4)
> > Hum I don't agree ;)
> > 
> >  m_mediaDurationKnown is initially true
> 
> Ah, I see this now. "m_mediaDurationKnown" is probably not the best name for the flag because it is set when you *don't* know the duration.
> 

Right, I guess I should rename it, it is confusing indeed.

> In any case durationChanged() has similar logic plus it informs HTMLMediaElement when that the duration has changed. Is there any reason to not call it instead?

Yes. I did that at first, but IIRC durationchanged was emitted without the duration having actually changed. I will try to do some more tests with that approach.

[Philippe Normand]

Created attachment 66480 [details]
proposed patch

[WebKit Review Bot]

Attachment 66480 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:111:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 66480 [details]
proposed patch

+        // Attempt to cache the duration, without emitting the
+        // durationchange event because it most likely was first sent
+        // by the media element when the ready state reached HAVE_METADATA.
+        durationChanged(false);
+

...

-    m_player->durationChanged();
+    if ((m_mediaDuration != previousDuration) && emitEvent)
+        m_player->durationChanged();
I don't understand why you would *ever* want to suppress the event. If you don't send the event when the duration changes, you won't ever send it unless the duration changes again. 

Why not always send it when m_mediaDuration != previousDuration?

[Philippe Normand]

(In reply to comment #9)

> I don't understand why you would *ever* want to suppress the event. If you don't send the event when the duration changes, you won't ever send it unless the duration changes again. 
> 
> Why not always send it when m_mediaDuration != previousDuration?

It is already sent once by the HTMLMediaElement when it reaches HAVE_METADATA. Without using that flag in durationChanged() I can see double emission of the event in 5 media tests at least.

[Eric Carlson]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > I don't understand why you would *ever* want to suppress the event. If you don't send the event when the duration changes, you won't ever send it unless the duration changes again. 
> > 
> > Why not always send it when m_mediaDuration != previousDuration?
> 
> It is already sent once by the HTMLMediaElement when it reaches HAVE_METADATA. Without using 
> that flag in durationChanged() I can see double emission of the event in 5 media tests at least.

Those darned tests :-). 

The point I was trying to make is that the existing logic makes it possible to miss sending a durationchange event if the reported duration ever changes unexpectedly. For example, the duration of a variable rate mp3 file is only known after looking at every packet so some media engines estimate the duration after looking at a portion of the file and refine the estimate as more data is examined. Even if GStreamer doesn't do this now, relying on the current behavior seems risky

At the very least your comment, "... because it most likely was first sent by the media element ...", is misleading if you mean it always happens.

[Philippe Normand]

Created attachment 66865 [details]
proposed patch

[Eric Carlson]

Comment on attachment 66865 [details]
proposed patch

Nice simplification!

r=me

[Philippe Normand]

Committed r66987: <http://trac.webkit.org/changeset/66987>