Bug 30004 – [GStreamer] Should handle BUFFERING messages

• Component: Media
• Status: RESOLVED FIXED

[Sebastian Dröge (slomo)]

Hi,
currently there's no handling of BUFFERING messages. These say something about the current buffering state, i.e. if things are still being buffered, if enough is buffered, etc. This is probably quite interesting for Webkit ;)

[Sebastian Dröge (slomo)]

This might be useful for the MediaPlayer::BytesLoaded() and to correctly set enoughData/futureData things.

[Philippe Normand]

Maybe we could enable progressive downloading/on-disk buffering like Totem does now, what do you think slomo?

[Philippe Normand]

Ping slomo :)

I actually wrote a patch for on-disk buffering, would you care to have a look? ;)

[Philippe Normand]

Created attachment 46055 [details]
initial support for on-disk buffering of videos

[Philippe Normand]

Created attachment 46056 [details]
draw the buffering status in the media controls

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46055 [details] without any errors.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46056 [details] without any errors.

[Sebastian Dröge (slomo)]

Why is there the 200ms timeout when receiving a buffering message instead of using a idle callback or a timeout of 0? Also async. bus signals/watches are always going through the main thread, no need to do a timeout just for GTK threading requirements.

When the duration changes after a buffering query, shouldn't you call durationChanged() to signal the change?

Apart from that it looks good (except some code style issues), but I don't know the media player API and if all this is the desired behaviour :)

Oh and one small thing... the queueing/buffering is done by playbin2, not decodebin2 as the comment says ;)

[Sebastian Dröge (slomo)]

...and buffering also happens when no temp file name exists, for example when playing a random HTTP stream without a temporary file. In those cases you want to show the buffering state too I think.

[Philippe Normand]

(In reply to comment #8)
> Why is there the 200ms timeout when receiving a buffering message instead of
> using a idle callback or a timeout of 0? Also async. bus signals/watches are
> always going through the main thread, no need to do a timeout just for GTK
> threading requirements.
> 

If I set the timeout to 0 the source is immediately removed because the stop value of the buffering range is -1.

> When the duration changes after a buffering query, shouldn't you call
> durationChanged() to signal the change?
> 

yes :)

> Apart from that it looks good (except some code style issues), but I don't know
> the media player API and if all this is the desired behaviour :)
> 

Well the style-bot didn't complain ;) But what is in your mind?

> Oh and one small thing... the queueing/buffering is done by playbin2, not
> decodebin2 as the comment says ;)

Oops right, I confused with uridecodebin in fact. The queue2 behavior is set there, as far as I could see in the code, right?

(In reply to comment #9)
> ...and buffering also happens when no temp file name exists, for example when
> playing a random HTTP stream without a temporary file. In those cases you want
> to show the buffering state too I think.

Hm yes, so I guess I can rely on m_startedBuffering instead.

[Philippe Normand]

Created attachment 46205 [details]
initial support for on-disk buffering of videos

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:572:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:572:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:1091:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3

[Philippe Normand]

Created attachment 46262 [details]
initial support for on-disk buffering of videos

[Sebastian Dröge (slomo)]

Ok, it's all good then :)

[Gustavo Noronha (kov)]

Comment on attachment 46262 [details]
initial support for on-disk buffering of videos

 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.

[Philippe Normand]

(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.

[Xan Lopez]

Comment on attachment 46262 [details]
initial support for on-disk buffering of videos

Just a few nitpicks on top of gustavo's comments.

>+#include <glib/gstdio.h>

You should not include anything other than the toplevel glib header.

> #include <gst/gst.h>
> #include <gst/interfaces/mixer.h>
> #include <gst/interfaces/xoverlay.h>
>@@ -48,6 +49,18 @@
> #include <math.h>
> #include <wtf/gtk/GOwnPtr.h>
> 

I have been informed that this stuff is not public and that you have to copy it like this in order to use it, but a comment saying just that would be good IMHO.

>+// GstPlayFlags flags from playbin2
>+typedef enum {
>+    GST_PLAY_FLAG_VIDEO         = 0x01,
>+    GST_PLAY_FLAG_AUDIO         = 0x02,
>+    GST_PLAY_FLAG_TEXT          = 0x04,
>+    GST_PLAY_FLAG_VIS           = 0x08,
>+    GST_PLAY_FLAG_SOFT_VOLUME   = 0x10,
>+    GST_PLAY_FLAG_NATIVE_AUDIO  = 0x20,
>+    GST_PLAY_FLAG_NATIVE_VIDEO  = 0x40,
>+    GST_PLAY_FLAG_DOWNLOAD      = 0x80
>+} GstPlayFlags;
>+



>+void MediaPlayerPrivate::processBufferingStats(GstMessage* message)
>+{
>+    GstBufferingMode mode;
>+
>+    gst_message_parse_buffering_stats(message, &mode, 0, 0, 0);
>+    if (mode != GST_BUFFERING_DOWNLOAD)
>+        return;
>+
>+    if (!m_startedBuffering) {
>+        m_startedBuffering = true;
>+
>+        if (m_fillTimeoutId > 0) {
>+            g_source_remove(m_fillTimeoutId);
>+            m_fillTimeoutId = 0;

I guess there's really no need to set the id to anything if you are going to write over it in the next line.

>+        }
>+
>+        m_fillTimeoutId = g_timeout_add(200, (GSourceFunc) bufferingTimeoutCallback, this);
>+    }
>+}
>+
>+bool MediaPlayerPrivate::queryBufferingStats()
>+{
>+    GstQuery* query = gst_query_new_buffering(GST_FORMAT_PERCENT);
>+
>+    if (gst_element_query(m_playBin, query)) {
>+        gint64 start, stop;
>+        gdouble fillStatus;
>+
>+        gst_query_parse_buffering_range(query, 0, &start, &stop, 0);
>+
>+        if (stop != -1)
>+            fillStatus = 100.0 * stop / GST_FORMAT_PERCENT_MAX;
>+        else
>+            fillStatus = 100.0;
>+
>+        LOG_VERBOSE(Media, "Download buffer filled up to %f%%", fillStatus);
>+
>+        // Update maxTimeLoaded only if the media duration is
>+        // available. Otherwise we can't compute it.
>+        if (!m_mediaDuration) {
>+            float newDuration = duration();
>+            if (!isinf(newDuration)) {
>+                m_mediaDuration = newDuration;
>+                durationChanged();
>+            }
>+        }
>+
>+        if (m_mediaDuration) {
>+            m_maxTimeLoaded = static_cast<float>((fillStatus * m_mediaDuration) / 100.0);
>+            LOG_VERBOSE(Media, "Updated maxTimeLoaded: %f", m_maxTimeLoaded);
>+        }
>+
>+        if (fillStatus == 100.0) {
>+            // Buffering is done, remove the fill source from the main loop.
>+            m_fillTimeoutId = 0;

Unless I'm missing something you are not actually removing any source here.


>+            gst_query_unref(query);
>+
>+            // Media is now fully loaded. It will play even if network
>+            // connection is cut.
>+            m_networkState = MediaPlayer::Loaded;
>+            m_player->networkStateChanged();
>+
>+            return FALSE;
>+        }
>+
>+        if ((fillStatus > 0) && (m_readyState < MediaPlayer::HaveFutureData)) {
>+            // Buffering started, we should now have enough data to
>+            // start playback if it was requested.
>+            m_readyState = MediaPlayer::HaveFutureData;
>+            m_player->readyStateChanged();
>+            if (m_paused && m_playRequired) {
>+                m_playRequired = false;
>+                gst_element_set_state(m_playBin, GST_STATE_PLAYING);
>+            }
>+        }
>+    }
>+
>+    gst_query_unref(query);
>+    return TRUE;
>+}
>+

[Philippe Normand]

(In reply to comment #17)
> (From update of attachment 46262 [details])
> Just a few nitpicks on top of gustavo's comments.
> 
> >+#include <glib/gstdio.h>
> 
> You should not include anything other than the toplevel glib header.
> 

Right and in fact Bastien Nocera told me that I should not need to manually remove the temp file so this include should be removed in the next iteration of the patch :)

> > #include <gst/gst.h>
> > #include <gst/interfaces/mixer.h>
> > #include <gst/interfaces/xoverlay.h>
> >@@ -48,6 +49,18 @@
> > #include <math.h>
> > #include <wtf/gtk/GOwnPtr.h>
> > 
> 
> I have been informed that this stuff is not public and that you have to copy it
> like this in order to use it, but a comment saying just that would be good
> IMHO.
> 

Yes sorry I will add a comment.

> >+// GstPlayFlags flags from playbin2
> >+typedef enum {
> >+    GST_PLAY_FLAG_VIDEO         = 0x01,
> >+    GST_PLAY_FLAG_AUDIO         = 0x02,
> >+    GST_PLAY_FLAG_TEXT          = 0x04,
> >+    GST_PLAY_FLAG_VIS           = 0x08,
> >+    GST_PLAY_FLAG_SOFT_VOLUME   = 0x10,
> >+    GST_PLAY_FLAG_NATIVE_AUDIO  = 0x20,
> >+    GST_PLAY_FLAG_NATIVE_VIDEO  = 0x40,
> >+    GST_PLAY_FLAG_DOWNLOAD      = 0x80
> >+} GstPlayFlags;
> >+
> 
> 
> 
> >+void MediaPlayerPrivate::processBufferingStats(GstMessage* message)
> >+{
> >+    GstBufferingMode mode;
> >+
> >+    gst_message_parse_buffering_stats(message, &mode, 0, 0, 0);
> >+    if (mode != GST_BUFFERING_DOWNLOAD)
> >+        return;
> >+
> >+    if (!m_startedBuffering) {
> >+        m_startedBuffering = true;
> >+
> >+        if (m_fillTimeoutId > 0) {
> >+            g_source_remove(m_fillTimeoutId);
> >+            m_fillTimeoutId = 0;
> 
> I guess there's really no need to set the id to anything if you are going to
> write over it in the next line.
> 

I guess yes :)

> >+        }
> >+
> >+        m_fillTimeoutId = g_timeout_add(200, (GSourceFunc) bufferingTimeoutCallback, this);
> >+    }
> >+}
> >+
> >+bool MediaPlayerPrivate::queryBufferingStats()
> >+{
> >+    GstQuery* query = gst_query_new_buffering(GST_FORMAT_PERCENT);
> >+
> >+    if (gst_element_query(m_playBin, query)) {
> >+        gint64 start, stop;
> >+        gdouble fillStatus;
> >+
> >+        gst_query_parse_buffering_range(query, 0, &start, &stop, 0);
> >+
> >+        if (stop != -1)
> >+            fillStatus = 100.0 * stop / GST_FORMAT_PERCENT_MAX;
> >+        else
> >+            fillStatus = 100.0;
> >+
> >+        LOG_VERBOSE(Media, "Download buffer filled up to %f%%", fillStatus);
> >+
> >+        // Update maxTimeLoaded only if the media duration is
> >+        // available. Otherwise we can't compute it.
> >+        if (!m_mediaDuration) {
> >+            float newDuration = duration();
> >+            if (!isinf(newDuration)) {
> >+                m_mediaDuration = newDuration;
> >+                durationChanged();
> >+            }
> >+        }
> >+
> >+        if (m_mediaDuration) {
> >+            m_maxTimeLoaded = static_cast<float>((fillStatus * m_mediaDuration) / 100.0);
> >+            LOG_VERBOSE(Media, "Updated maxTimeLoaded: %f", m_maxTimeLoaded);
> >+        }
> >+
> >+        if (fillStatus == 100.0) {
> >+            // Buffering is done, remove the fill source from the main loop.
> >+            m_fillTimeoutId = 0;
> 
> Unless I'm missing something you are not actually removing any source here.
> 
> 

if this callback returns FALSE (see few lines above) the source will be removed from the main loop.

> >+            gst_query_unref(query);
> >+
> >+            // Media is now fully loaded. It will play even if network
> >+            // connection is cut.
> >+            m_networkState = MediaPlayer::Loaded;
> >+            m_player->networkStateChanged();
> >+
> >+            return FALSE;
> >+        }
> >+
> >+        if ((fillStatus > 0) && (m_readyState < MediaPlayer::HaveFutureData)) {
> >+            // Buffering started, we should now have enough data to
> >+            // start playback if it was requested.
> >+            m_readyState = MediaPlayer::HaveFutureData;
> >+            m_player->readyStateChanged();
> >+            if (m_paused && m_playRequired) {
> >+                m_playRequired = false;
> >+                gst_element_set_state(m_playBin, GST_STATE_PLAYING);
> >+            }
> >+        }
> >+    }
> >+
> >+    gst_query_unref(query);
> >+    return TRUE;
> >+}
> >+

[Sebastian Dröge (slomo)]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 46262 [details] [details])
> > Just a few nitpicks on top of gustavo's comments.
> > 
> > >+#include <glib/gstdio.h>
> > 
> > You should not include anything other than the toplevel glib header.
> > 
> 
> Right and in fact Bastien Nocera told me that I should not need to manually
> remove the temp file so this include should be removed in the next iteration of
> the patch :)

Actually queue2 is supposed to not remove that temporary file, see https://bugzilla.gnome.org/show_bug.cgi?id=607739

[Philippe Normand]

Created attachment 47342 [details]
initial support for on-disk buffering of videos

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:617:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1


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

[Philippe Normand]

Created attachment 47507 [details]
initial support for on-disk buffering of videos

[Gustavo Noronha (kov)]

Comment on attachment 46056 [details]
draw the buffering status in the media controls

 680     // Don't bother drawing an empty area.
 681     if (!bufferedRect.isEmpty()) {

This should be an early return for the true case instead.

7  Time: 2.2
 7 Time: 2.1

Why has this changed?

Patch looks good, overall, so just change the if to an early return, please, and add a comment regarding the change in drag-timebar.

[Gustavo Noronha (kov)]

Comment on attachment 47507 [details]
initial support for on-disk buffering of videos

 642     if (gst_element_query(m_playBin, query)) {
 643         gint64 start, stop;
 644 

I think this should become an early return. It will make the code more readable. This would also enable us to make the following code much more readable:

 664         if (m_fillStatus == 100.0) {
 665             // Buffering is done, remove the fill source from the main loop.
 666             m_fillTimeoutId = 0;
 667 
 668             // Media is now fully loaded. It will play even if network
 669             // connection is cut.
 670             updateStates();
 671 
 672             gst_query_unref(query);
 673             return FALSE;
 674         }
 675         updateStates();
 676     }
 677 
 678     gst_query_unref(query);
 679     return TRUE;

Maybe you will be able to call updateStates outside that if (the m_fillStatus == 100.0 one), since you are going to call it anyway, right? And you can also call gst_query_unref(query) before that if. And then you'll have a much more pleasant code flow, with a simple if that sets the variable, you will be able to unconditionaly return FALSE in the end, because the early return already handled the TRUE case.

 682 void MediaPlayerPrivate::updateDownloadLocation(gchar* location)
 683 {
 684     if (m_downloadFilename)
 685         g_free(m_downloadFilename);
 686     LOG_VERBOSE(Media, "Download filename: %s", location);
 687     m_downloadFilename = location;
 688 }

I wonder if m_downloadFilename could be a GRefPtr. That would make the code a bit cleaner. Code looks quite good, except for the function I was talking about above. I'll say r- to see one more iteration of it before this lands. Thanks!

[Philippe Normand]

Created attachment 47849 [details]
initial support for on-disk buffering of videos

[Philippe Normand]

Created attachment 47854 [details]
initial support for on-disk buffering of videos

Now without the unused downloadFile code.

[Eric Seidel (no email)]

Attachment 46056 [details] was posted by a committer and has review+, assigning to Philippe Normand for commit.

[Gustavo Noronha (kov)]

Comment on attachment 47854 [details]
initial support for on-disk buffering of videos

 722     bool continueStatsGathering = TRUE;
 723     // Media is now fully loaded. It will play even if network
 724     // connection is cut.
 725     if (m_fillStatus == 100.0) {
 726         // Buffering is done, remove the fill source from the main loop.
 727         m_fillTimeoutId = 0;
 728         continueStatsGathering = FALSE;
 729     }
 730 
 731     updateStates();
 732     return continueStatsGathering;

I was rather thinking of something along these lines, instead:

    if (m_fillStatus != 100.0) {
        updateStates();
        return TRUE;
    }

    m_fillTimeoutId = 0;
    updateStates();
    return FALSE;

 840         } else if (state == GST_STATE_PAUSED)
749841             m_paused = true;

So, will this only be true when GStreamer itself paused the stream because buffering is required? It doesn't look that we are regressing anything in our support of the spec here, but I'd be more confident if we could get someone who actually understands the spec to check on the state changes. I'll try to get hold of Eric Carlson =D.

[Philippe Normand]

(In reply to comment #28)
> (From update of attachment 47854 [details])
>  722     bool continueStatsGathering = TRUE;
>  723     // Media is now fully loaded. It will play even if network
>  724     // connection is cut.
>  725     if (m_fillStatus == 100.0) {
>  726         // Buffering is done, remove the fill source from the main loop.
>  727         m_fillTimeoutId = 0;
>  728         continueStatsGathering = FALSE;
>  729     }
>  730 
>  731     updateStates();
>  732     return continueStatsGathering;
> 
> I was rather thinking of something along these lines, instead:
> 
>     if (m_fillStatus != 100.0) {
>         updateStates();
>         return TRUE;
>     }
> 
>     m_fillTimeoutId = 0;
>     updateStates();
>     return FALSE;

Looks much cleaner indeed ;)
> 
>  840         } else if (state == GST_STATE_PAUSED)
> 749841             m_paused = true;
> 

Well in current HEAD of the code m_paused is set to true when state is != GST_STATE_PLAYING. So, to be fully compliant with current behavior i can change the test to use <= GST_STATE_PAUSED. What do you think?


> So, will this only be true when GStreamer itself paused the stream because
> buffering is required? It doesn't look that we are regressing anything in our
> support of the spec here, but I'd be more confident if we could get someone who
> actually understands the spec to check on the state changes. I'll try to get
> hold of Eric Carlson =D.

[Gustavo Noronha (kov)]

(In reply to comment #29)
> >  840         } else if (state == GST_STATE_PAUSED)
> > 749841             m_paused = true;
> > 
> 
> Well in current HEAD of the code m_paused is set to true when state is !=
> GST_STATE_PLAYING. So, to be fully compliant with current behavior i can change
> the test to use <= GST_STATE_PAUSED. What do you think?

This means the same as != GST_STATE_PLAYING, so I guess we could drop the change, then?

I talked to Eric Carlson about helping review the state changes, he said he'd look, I'll add him in CC, so he knows what the bug number is =D.

[Philippe Normand]

(In reply to comment #27)
> Attachment 46056 [details] was posted by a committer and has review+, assigning to
> Philippe Normand for commit.

I am waiting the other patch in this bug gets approved so I can land both at
same time. They go together and were splitted in first place to ease the review
process.

[Philippe Normand]

Created attachment 48142 [details]
initial support for on-disk buffering of videos

Updated with latest kov's suggestions

[Eric Carlson]

Comment on attachment 48142 [details]
initial support for on-disk buffering of videos

+        [GStreamer] Should handle BUFFERING messages
+        https://bugs.webkit.org/show_bug.cgi?id=30004
+
+        Initial support for on-disk buffering of videos. This works only
+        for Quicktime and flv though.
+
+        * platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:
+        (WebCore::mediaPlayerPrivateMessageCallback):
+        (WebCore::bufferingTimeoutCallback):
+        (WebCore::MediaPlayerPrivate::MediaPlayerPrivate):
+        (WebCore::MediaPlayerPrivate::~MediaPlayerPrivate):
+        (WebCore::MediaPlayerPrivate::load):
+        (WebCore::MediaPlayerPrivate::duration):
+        (WebCore::MediaPlayerPrivate::processBufferingStats):
+        (WebCore::MediaPlayerPrivate::queryBufferingStats):
+        (WebCore::MediaPlayerPrivate::maxTimeSeekable):
+        (WebCore::MediaPlayerPrivate::maxTimeLoaded):
+        (WebCore::MediaPlayerPrivate::bytesLoaded):
+        (WebCore::MediaPlayerPrivate::totalBytes):
+        (WebCore::MediaPlayerPrivate::updateStates):
+        (WebCore::MediaPlayerPrivate::durationChanged):
+        (WebCore::MediaPlayerPrivate::setAutobuffer):
+        (WebCore::MediaPlayerPrivate::createGSTPlayBin):
+        * platform/graphics/gtk/MediaPlayerPrivateGStreamer.h:
+
It would be nice to have comments about what changed in each method.

 
+    , m_maxTimeLoaded(0.0)
+    , m_fillStatus(0.0)
+
".0" shouldn't be necessary.

 
+void MediaPlayerPrivate::processBufferingStats(GstMessage* message)
+{
+    GstBufferingMode mode;
+
+    gst_message_parse_buffering_stats(message, &mode, 0, 0, 0);
+    if (mode != GST_BUFFERING_DOWNLOAD)
+        return;

No need to do this if if m_startedBuffering is true, might as well return immediately.

+
+    if (!m_startedBuffering) {
+        m_startedBuffering = true;
+
+        if (m_fillTimeoutId > 0)
+            g_source_remove(m_fillTimeoutId);

Is it legal for m_fillTimeoutId to be non-zero when m_startedBuffering is false? If not, maybe ASSERT(!m_fillTimeoutId).


+
+    bool continueStatsGathering = TRUE;
+    // Media is now fully loaded. It will play even if network
+    // connection is cut.
+    if (m_fillStatus == 100.0) {
+        // Buffering is done, remove the fill source from the main loop.
+        m_fillTimeoutId = 0;

No need to g_source_remove()? Does returning false from the callback automatically remove the source?


     // infinite duration means live stream
+    if (isinf(duration()))
+        return 0.0;
+
Don't you want to return infinity() here?

 
+            void setAutobuffer(bool);
+            void processBufferingStats(GstMessage* message);
+            bool queryBufferingStats();
+            void createGSTPlayBin();

Is there any reason these new methods, and all methods really, shouldn't be private?
 
Also noticed this in examining the source:

> void MediaPlayerPrivate::didEnd()
>     if (now > 0)
>         m_mediaDuration = now;
>
If this changes m_mediaDuration, you should call durationChanged()


I know almost nothing about the gstreamer API so someone else should check to make sure the new code is OK, but but r=me on the other changes

[Eric Seidel (no email)]

One patch per bug is generally preferred as it can be hard to tell what has been landed and what has not.

[Eric Seidel (no email)]

I really mean "one change per bug", which can end up needing several patches before the final one actually gets landed and the bug is closed.

[Gustavo Noronha (kov)]

(In reply to comment #33)
> +    bool continueStatsGathering = TRUE;
> +    // Media is now fully loaded. It will play even if network
> +    // connection is cut.
> +    if (m_fillStatus == 100.0) {
> +        // Buffering is done, remove the fill source from the main loop.
> +        m_fillTimeoutId = 0;
> 
> No need to g_source_remove()? Does returning false from the callback
> automatically remove the source?

Yes =)

[Philippe Normand]

(In reply to comment #33)
> (From update of attachment 48142 [details])
> 
> 
> +void MediaPlayerPrivate::processBufferingStats(GstMessage* message)
> +{
> +    GstBufferingMode mode;
> +
> +    gst_message_parse_buffering_stats(message, &mode, 0, 0, 0);
> +    if (mode != GST_BUFFERING_DOWNLOAD)
> +        return;
> 
> No need to do this if if m_startedBuffering is true, might as well return
> immediately.
> 

It is necessary because that method will be called even in the case where on-disk buffering is disabled. So before proceeding further we need to make sure the buffering message is of the correct type.

> +
> +    if (!m_startedBuffering) {
> +        m_startedBuffering = true;
> +
> +        if (m_fillTimeoutId > 0)
> +            g_source_remove(m_fillTimeoutId);
> 
> Is it legal for m_fillTimeoutId to be non-zero when m_startedBuffering is
> false? If not, maybe ASSERT(!m_fillTimeoutId).
> 

Hum I realize we never set m_startedBuffering to false. I can do it at least when buffering ended. But I prefer to keep the cleanup of m_fillTimeoutId here.

> 
> +
> +    bool continueStatsGathering = TRUE;
> +    // Media is now fully loaded. It will play even if network
> +    // connection is cut.
> +    if (m_fillStatus == 100.0) {
> +        // Buffering is done, remove the fill source from the main loop.
> +        m_fillTimeoutId = 0;
> 
> No need to g_source_remove()? Does returning false from the callback
> automatically remove the source?
> 
> 
>      // infinite duration means live stream
> +    if (isinf(duration()))
> +        return 0.0;
> +
> Don't you want to return infinity() here?
> 

Well it would at least break HTMLMediaElement::seekable() I think. Currently if maxTimeSeekable() returns 0 an empty time range is created. Also the mac private player has the same code ;)

> 
> +            void setAutobuffer(bool);
> +            void processBufferingStats(GstMessage* message);
> +            bool queryBufferingStats();
> +            void createGSTPlayBin();
> 
> Is there any reason these new methods, and all methods really, shouldn't be
> private?
> 

createGSTPlayBin() is private already.
setAutobuffer() is public and used by MediaPlayer
queryBufferingStats() can be made private :)
processBufferingStats has to remain public I'm afraid. It is called from the callback bufferingTimeoutCallback().

> Also noticed this in examining the source:
> 
> > void MediaPlayerPrivate::didEnd()
> >     if (now > 0)
> >         m_mediaDuration = now;
> >
> If this changes m_mediaDuration, you should call durationChanged()
> 

no durationChanged(), it is explained in the comment:

    // EOS was reached but in case of reverse playback the position is
    // not always 0. So to not confuse the HTMLMediaElement we
    // synchronize position and duration values.

but I can call  m_player->durationChanged() though ;)

> 
> I know almost nothing about the gstreamer API so someone else should check to
> make sure the new code is OK, but but r=me on the other changes

[Philippe Normand]

The merge of the new WebKit src gstreamer element uncovered a bug in appsrc making this patch unusable at the moment. See https://bugzilla.gnome.org/show_bug.cgi?id=609423

[Philippe Normand]

Created attachment 48630 [details]
initial support for on-disk buffering of videos

updated after Eric's review

[Eric Carlson]

Comment on attachment 48630 [details]
initial support for on-disk buffering of videos

I am giving r+ assuming the GStreamer specific parts have been vetted by someone that understands it a lot more than I do ;-)

r=me

[Philippe Normand]

(In reply to comment #40)
> (From update of attachment 48630 [details])
> I am giving r+ assuming the GStreamer specific parts have been vetted by
> someone that understands it a lot more than I do ;-)
> 
> r=me

Thanks all :) I will wait Bug 34881 to land this though:

[Philippe Normand]

Created attachment 48744 [details]
initial support for on-disk buffering of videos

Rebased patch against master and bumped gst requirements to 0.10.25 as
discussed on friday with Gustavo.

[Gustavo Noronha (kov)]

Comment on attachment 48744 [details]
initial support for on-disk buffering of videos

This review is just a renewal of Eric Carlson's, checking the version bump alone. Please commit the removal of the mistakenly committed redefine in a separate commit.

[Philippe Normand]

Landed in r54878 :) Thanks all!