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

[bugzilla-daemon at webkit.org]

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


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #46262|review?                     |review-
               Flag|                            |




--- Comment #15 from Gustavo Noronha (kov) <gns at gnome.org>  2010-01-19 10:59:20 PST ---
(From update of attachment 46262)
 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         }

 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!).

 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.

 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

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.

 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.

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