[webkit-reviews] review denied: [Bug 30004] [GStreamer] Should handle BUFFERING messages : [Attachment 46262] initial support for on-disk buffering of videos

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 30004: [GStreamer] Should handle BUFFERING messages
https://bugs.webkit.org/show_bug.cgi?id=30004

Attachment 46262: initial support for on-disk buffering of videos
https://bugs.webkit.org/attachment.cgi?id=46262&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
 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-med
ia-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.