[Webkit-unassigned] [Bug 30004] [GStreamer] Should handle BUFFERING messages

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30004





--- Comment #16 from Philippe Normand <pnormand at igalia.com>  2010-01-20 03:42:21 PST ---
(In reply to comment #15)
> (From update of attachment 46262 [details])
>  605         // Update maxTimeLoaded only if the media duration is
>  606         // available. Otherwise we can't compute it.
> 
> This comment seems misplaced. It should be above this, shouldn't it?:
> 
>  615         if (m_mediaDuration) {
>  616             m_maxTimeLoaded = static_cast<float>((fillStatus *
> m_mediaDuration) / 100.0);
>  617             LOG_VERBOSE(Media, "Updated maxTimeLoaded: %f",
> m_maxTimeLoaded);
>  618         }
> 

Right :)

>  588 bool MediaPlayerPrivate::queryBufferingStats()
> [...]
>  625             // Media is now fully loaded. It will play even if network
>  626             // connection is cut.
>  627             m_networkState = MediaPlayer::Loaded;
>  628             m_player->networkStateChanged();
> 
> I think this function is doing more than it should, and stepping into the
> updateStates function's resposibilities. Updating the states should be a
> monopoly of updateStates, and this function should just help track the
> information it needs to decide, and call it (notice that you end up duplicating
> some of this logic inside updateStates, though with less accuracy if you don't
> have the duration!).
> 

You are right on this too. 

>  745     bool completelyLoaded = maxTimeLoaded() == duration();
> 
> This means you should probably use a member variable to track whether you are
> fully loaded, not something you calculate inside updateStates.
> 

Huh? If I did "if (maxTimeLoaded() == duration())" would you have ticked too? I
think having a member variable is kinda overkill in this context ;)

>  1089     // Activate on-disk buffering and load the media uri.
>  1090     g_object_set(m_playBin, "uri", url.utf8().data(), "flags", flags |
> GST_PLAY_FLAG_DOWNLOAD, NULL);
> 
> The MediaPlayer has an 'autobuffer' property. You may want to look at what it
> is for, and how it affects this work. Also looking at this seems to be useful:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#attr-media-autobuffer
> 

Absolutely right. So I implemented setAutobuffer() but it was never called, so
I opened Bug 33889.

> I am not sure we want to always enable buffering.The same goes for
> playRequired: I wrote a very dumb patch months ago that had similar behavior,
> but bdash was not sure it was the intended behavior, and wanted to know where
> in the spec the behavior was defined, which I think makes sense - we want to be
> compatible here; all such policy should already be encoded inside the element
> or the cross-platform media player.
> 

Yep I can remove that playRequired member variable. I added it in the first
place because GStreamer will internally pause the pipeline at the beginning of
on-disk buffering if not enough data has been pulled. So I think i can test
both m_paused and the fillTimeoutId value.

>  244     , m_mediaDuration(0.0)
> 
> I don't think we really need this. Is it just for caching the duration? Why not
> cache it using a static variable inside duration()? I feel having two sources
> for this information is wrong.

Inspiration for this one came from the QTKit media player which caches the
duration too. I don't think I can store it as a static variable inside
duration() because it is updated at other places of the code.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.