Bug 205275 – [GStreamer] Make source aware of buffered time

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Xabier Rodríguez Calvar]

[GStreamer] Make source aware of buffered time

[Xabier Rodríguez Calvar]

Created attachment 385769 [details]
Patch

[Philippe Normand]

Before doing a review, I have a couple of questions :)
Why adding so much buffering logic to the element? Shouldn't the stream/download buffering logic be fixed instead in the player?

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #2)
> Before doing a review, I have a couple of questions :)
> Why adding so much buffering logic to the element? Shouldn't the
> stream/download buffering logic be fixed instead in the player?

We might a bit of logic to the player, like the one checking how far we have buffered, but the logic to initiate and stop the load should be, IMHO, in the source because it is the source that knows when it gets buffers and when it pushes them, the one that can block on the streaming thread, etc.

[Charlie Turner]

Comment on attachment 385769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385769&action=review

I agree with Phil at a high-level, this element looks to have too much knowledge of the rest of the pipeline configuration and the player. What concerns me is the added complexity, when we know souphttpsrc works well and doesn't have so much complication imo. I believe our buffering code in the player is what needs more attention, and that in turn would fix the pathological issues with this element.

> Source/WebCore/ChangeLog:9
> +        in pushing data downstream. We don't read fast buffers anymore

What are fast buffers and fast data?

> Source/WebCore/ChangeLog:30
> +        (webKitWebSrcCreate): "Defast", otherwise we might be waiting for

What is defast?

> Source/WebCore/ChangeLog:39
> +        adjustment improved.

Please explain this.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2285
> +    m_didDownloadFinish = percentage == 100 && (!WEBKIT_IS_WEB_SRC(m_source.get()) || !webKitSrcIsWaitingForData(WEBKIT_WEB_SRC_CAST(m_source.get())));

I find the didDownloadFinish flag confusing. It seems we need to more clear about what posted the given percentage, if its a downloadbuffer then it makes sense, but if it's a queue somewhere in the pipeline it does not. I feel like these two concepts need to made more explicit in the player, and that would help our buffering bugs, since it looks like we mix these two modes up.

It is easier to read this as percentage==100 && isWebSrcWaitingForData(), where the cast check is performed in its own function. Even further, it strongly suggests to me we need to refactor all the buffering code into something that is self-aware of what the pipeline configuration is, so that this logic is not so twisted in with the rest of the player logic.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2294
> +        if (percentage == 100)

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2872
> +        webKitWebSrcSetOnDiskBufferingEnabled(WEBKIT_WEB_SRC_CAST(m_source.get()), shouldDownload);

It seems wrong for a source to need to know whether downstream is disk buffering or RAM buffering, souphttpsrc doesn't need to AFAICT, and doesn't have these behaviours.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:55
> +// Minimal buffered seconds.

This comment is not helping me understand the purpose of these variables.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:84
> +    static constexpr float s_reduceBlocksizeLimit { 0.5 };

These constants were taken from souphttpsrc, does that also stall out on the respective bugs? Because our block size algorithm is (should be) identical, so it concerns me that we're performing magic here and don't know exactly what is going wrong.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:361
> +static WTF::Optional<bool> WARN_UNUSED_RETURN areNextSecondsBuffered(WebKitWebSrc* src, const Seconds& seconds)

This logic seems like it should be in the player.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:413
> +    GST_TRACE_OBJECT(src, "is download suspented %s, does have EOS %s, does have size %s, is seekable %s, size %" G_GUINT64_FORMAT

suspented typo

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:417
> +    if (priv->doesHaveEOS || !priv->haveSize || !priv->isSeekable || priv->size <= SMALL_MEDIA_RESOURCE_MAX_SIZE || !priv->readPosition

Personal preference, but conditionals this long are a code smell for me, can we refactor it into more meaningful predicates?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:429
> +    if (isEnoughTimeBuffered.hasValue()) {

The ternary logic here is weird, we're not using the false case, only the true and unknown cases.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:467
> +    if (isEnoughTimeBuffered.hasValue()) {

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:497
> +            gst_adapter_clear(priv->adapter.get());

This changes seems like it invalidates the remaining comment?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-387
> -        return GST_FLOW_FLUSHING;

A comment about the change in flushing behaviour should be in the changelog, is the flushing behaviour directly related to this bug?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-400
> -        size_t available = gst_adapter_available_fast(priv->adapter.get());

A comment about the switch from the fast to slow should be added.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:559
> +            } else if (!priv->isDownloadSuspended && ++retries >= 10)

This loop is very tricky, it would be a good idea to break it out into separate components if you can.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-920
> -    if (bytesRead >= blocksize * s_growBlocksizeLimit) {

Why?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1229
> +    // Rough bandwidth calculation. We don't calculate since we issue the request because that would include sending the request, etc. This way we can

"We don't calculate since we issue the request because that would include sending the request", please can you try to rephrase this? I'm not sure what you're trying to say.

[Charlie Turner]

(In reply to Charlie Turner from comment #4)
> > Source/WebCore/ChangeLog:9
> > +        in pushing data downstream. We don't read fast buffers anymore
> 
> What are fast buffers and fast data?
> 
> > Source/WebCore/ChangeLog:30
> > +        (webKitWebSrcCreate): "Defast", otherwise we might be waiting for
> 
> What is defast?

It just occurred to me these are buffers from the adapter's fast API, but lets make the language clearer :)

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #4)
> Comment on attachment 385769 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385769&action=review
> 
> I agree with Phil at a high-level, this element looks to have too much
> knowledge of the rest of the pipeline configuration and the player. What
> concerns me is the added complexity, when we know souphttpsrc works well and
> doesn't have so much complication imo. I believe our buffering code in the
> player is what needs more attention, and that in turn would fix the
> pathological issues with this element.

I agree we could do more things in the player and I am open to change this to do that but there are some others that should be done in the source. Examples of things are:

* Removing the using fast API of the adapter. This was causing the source to wait for data when it was clearly able to provide more.
* We need to increase the wait. There are certain cases where we will be getting EOS from the source while waiting for data and 400ms are clearly not enough during network load.
* Blocking the main thread and allowing an empty adapter because sometimes we have data that are not pushing.

I'll comment on the things I DON'T agree with.

> > Source/WebCore/ChangeLog:9
> > +        in pushing data downstream. We don't read fast buffers anymore
> 
> What are fast buffers and fast data?

The "fast" adapter API, yes.

> > Source/WebCore/ChangeLog:30
> > +        (webKitWebSrcCreate): "Defast", otherwise we might be waiting for
> 
> What is defast?
> 
> > Source/WebCore/ChangeLog:39
> > +        adjustment improved.
> 
> Please explain this.

In changelog or here? I adjusted the parameters by needing to pass the read limit twice instead of one and considering that we are going to reduce the blocksize when we go down half of the block size by when reading (twice already).

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2285
> > +    m_didDownloadFinish = percentage == 100 && (!WEBKIT_IS_WEB_SRC(m_source.get()) || !webKitSrcIsWaitingForData(WEBKIT_WEB_SRC_CAST(m_source.get())));
> 
> I find the didDownloadFinish flag confusing. It seems we need to more clear
> about what posted the given percentage, if its a downloadbuffer then it
> makes sense, but if it's a queue somewhere in the pipeline it does not. I
> feel like these two concepts need to made more explicit in the player, and
> that would help our buffering bugs, since it looks like we mix these two
> modes up.
> 
> It is easier to read this as percentage==100 && isWebSrcWaitingForData(),
> where the cast check is performed in its own function. Even further, it
> strongly suggests to me we need to refactor all the buffering code into
> something that is self-aware of what the pipeline configuration is, so that
> this logic is not so twisted in with the rest of the player logic.
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2294
> > +        if (percentage == 100)
> 
> Ditto.

The problem of all this is that the percentages are a bit confusing. Percents are sent in a different way depending on the download mode. If it is inactive it is the queue just downstream the source that answers that and it is 100% when it is full, which is different from having download enabled.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2872
> > +        webKitWebSrcSetOnDiskBufferingEnabled(WEBKIT_WEB_SRC_CAST(m_source.get()), shouldDownload);
> 
> It seems wrong for a source to need to know whether downstream is disk
> buffering or RAM buffering, souphttpsrc doesn't need to AFAICT, and doesn't
> have these behaviours.

If we don't have download enabled, any logic based on something different than percentages is useless because it is the queue that is going to limit us. That's why I introduced this. I prefer a timed logic because it is what MSE providers do, by wanting to have always 30s, which motivated my limits as well.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:55
> > +// Minimal buffered seconds.
> 
> This comment is not helping me understand the purpose of these variables.

Again, do you want me to change the comment or an explanation here?

These are the limits that the stop/restart functions are going to use:
* 15s for initial playback, otherwise the chrome is going to take too long to "fill the download slider".
* we stop when we have 30s or more and we restart when we have less than 25s.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:84
> > +    static constexpr float s_reduceBlocksizeLimit { 0.5 };
> 
> These constants were taken from souphttpsrc, does that also stall out on the
> respective bugs? Because our block size algorithm is (should be) identical,
> so it concerns me that we're performing magic here and don't know exactly
> what is going wrong.

From the behavior I saw while debugging this bug, I had the empirical impression that these values make more sente because otherwise the blocksize can be increased quite quickly and we are not able to fulfill it almost ever.

I don't think this has a big impact as I think modifying some other pieces was more important to achieve a better behavior.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:429
> > +    if (isEnoughTimeBuffered.hasValue()) {
> 
> The ternary logic here is weird, we're not using the false case, only the
> true and unknown cases.
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:467
> > +    if (isEnoughTimeBuffered.hasValue()) {
> 
> Ditto.

We are using it! If we don't have a duration we fallback to queue sizes. If we have it, we bail out if we are inside parameters, otherwise we don't bail out and to the bottom of the function.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-387
> > -        return GST_FLOW_FLUSHING;
> 
> A comment about the change in flushing behaviour should be in the changelog,
> is the flushing behaviour directly related to this bug?

No, the behavior is not changed. I only move it because with my changes it might happen that we are blocking the streaming thread while a seek or flush comes and when that happens we don't want to continue, we want to flush, and continuing is what was happening before.
 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-400
> > -        size_t available = gst_adapter_available_fast(priv->adapter.get());
> 
> A comment about the switch from the fast to slow should be added.

I can do it if necessary but an detailed read to the fast API makes you realize that for some cases it is not that fast and I think we were using it wrong before. I don't think it is needed.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:559
> > +            } else if (!priv->isDownloadSuspended && ++retries >= 10)
> 
> This loop is very tricky, it would be a good idea to break it out into
> separate components if you can.

I tried to respect the former structure but sure I can try it.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-920
> > -    if (bytesRead >= blocksize * s_growBlocksizeLimit) {
> 
> Why?

For me it makes no sense to increase something that it's inside limits. = is inside limits.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1229
> > +    // Rough bandwidth calculation. We don't calculate since we issue the request because that would include sending the request, etc. This way we can
> 
> "We don't calculate since we issue the request because that would include
> sending the request", please can you try to rephrase this? I'm not sure what
> you're trying to say.

Yes, I can, I guess you want it in the comment as well but the the case is that I don't know when the first batch of data is sent by the server, only when we receive data or when we request it so if reset the counters when I issue the request I will be including the time of sending the request to the server and I am interested in getting only the bandwidth from the server to us. For that I discard the first data pack and begin to count from there. It's just an aproximation but it is better aproximated this way, I think.

[Charlie Turner]

Comment on attachment 385769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385769&action=review

>>> Source/WebCore/ChangeLog:9
>>> +        in pushing data downstream. We don't read fast buffers anymore
>> 
>> What are fast buffers and fast data?
> 
> The "fast" adapter API, yes.

OK, this all makes sense now.

>>> Source/WebCore/ChangeLog:30
>>> +        (webKitWebSrcCreate): "Defast", otherwise we might be waiting for
>> 
>> What is defast?
> 
> In changelog or here? I adjusted the parameters by needing to pass the read limit twice instead of one and considering that we are going to reduce the blocksize when we go down half of the block size by when reading (twice already).

I think this is in response to something else, but please use the API names rather than "Defast" which at first I read as a typo.

>> Source/WebCore/ChangeLog:39
>> +        adjustment improved.
> 
> Please explain this.

Think this was what you were referring to. In the ChangeLog would be great so we have a record of the reasons for our algorithm changes.

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2285
>>> +    m_didDownloadFinish = percentage == 100 && (!WEBKIT_IS_WEB_SRC(m_source.get()) || !webKitSrcIsWaitingForData(WEBKIT_WEB_SRC_CAST(m_source.get())));
>> 
>> I find the didDownloadFinish flag confusing. It seems we need to more clear about what posted the given percentage, if its a downloadbuffer then it makes sense, but if it's a queue somewhere in the pipeline it does not. I feel like these two concepts need to made more explicit in the player, and that would help our buffering bugs, since it looks like we mix these two modes up.
>> 
>> It is easier to read this as percentage==100 && isWebSrcWaitingForData(), where the cast check is performed in its own function. Even further, it strongly suggests to me we need to refactor all the buffering code into something that is self-aware of what the pipeline configuration is, so that this logic is not so twisted in with the rest of the player logic.
> 
> The problem of all this is that the percentages are a bit confusing. Percents are sent in a different way depending on the download mode. If it is inactive it is the queue just downstream the source that answers that and it is 100% when it is full, which is different from having download enabled.

I agree, perhaps we can use better variable names to help keep this clear in the code.

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2872
>>> +        webKitWebSrcSetOnDiskBufferingEnabled(WEBKIT_WEB_SRC_CAST(m_source.get()), shouldDownload);
>> 
>> It seems wrong for a source to need to know whether downstream is disk buffering or RAM buffering, souphttpsrc doesn't need to AFAICT, and doesn't have these behaviours.
> 
> If we don't have download enabled, any logic based on something different than percentages is useless because it is the queue that is going to limit us. That's why I introduced this. I prefer a timed logic because it is what MSE providers do, by wanting to have always 30s, which motivated my limits as well.

My understanding is that the pipeline should be giving us enough information through BUFFERING messages to keep the pipeline from stalling out, if it is not then I would say that's a bug in GStreamer. I don't think the web src should need knowledge of MSE at all, it's not its responsibility. If we need specific amounts of queued data for that case, why can't we configure the GstQueue2 appropriately rather than modifying the source?

>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:55
>>> +// Minimal buffered seconds.
>> 
>> This comment is not helping me understand the purpose of these variables.
> 
> Again, do you want me to change the comment or an explanation here?
> 
> These are the limits that the stop/restart functions are going to use:
> * 15s for initial playback, otherwise the chrome is going to take too long to "fill the download slider".
> * we stop when we have 30s or more and we restart when we have less than 25s.

In the code please. An explanation of the higher-level algorithmic changes can go in the ChangeLog.

>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:84
>>> +    static constexpr float s_reduceBlocksizeLimit { 0.5 };
>> 
>> These constants were taken from souphttpsrc, does that also stall out on the respective bugs? Because our block size algorithm is (should be) identical, so it concerns me that we're performing magic here and don't know exactly what is going wrong.
> 
> From the behavior I saw while debugging this bug, I had the empirical impression that these values make more sente because otherwise the blocksize can be increased quite quickly and we are not able to fulfill it almost ever.
> 
> I don't think this has a big impact as I think modifying some other pieces was more important to achieve a better behavior.

I'm a little concerned we can't keep the buffers full, strange that soup can be I can accept we perhaps have differences in our net code and the direct use of the soup API in gstsouphttpsrc.

>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:429
>>> +    if (isEnoughTimeBuffered.hasValue()) {
>> 
>> The ternary logic here is weird, we're not using the false case, only the true and unknown cases.
> 
> We are using it! If we don't have a duration we fallback to queue sizes. If we have it, we bail out if we are inside parameters, otherwise we don't bail out and to the bottom of the function.

OK

>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-387
>>> -        return GST_FLOW_FLUSHING;
>> 
>> A comment about the change in flushing behaviour should be in the changelog, is the flushing behaviour directly related to this bug?
> 
> No, the behavior is not changed. I only move it because with my changes it might happen that we are blocking the streaming thread while a seek or flush comes and when that happens we don't want to continue, we want to flush, and continuing is what was happening before.

OK, it looks like the old implementation was not respecting flushes while waiting for the adaptor to fill up, but now we are which seems like a good thing.

>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-400
>>> -        size_t available = gst_adapter_available_fast(priv->adapter.get());
>> 
>> A comment about the switch from the fast to slow should be added.
> 
> I can do it if necessary but an detailed read to the fast API makes you realize that for some cases it is not that fast and I think we were using it wrong before. I don't think it is needed.

I would add a note, it will save the next programmer from having to do a detailed read of the fast API ;)

>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1229
>>> +    // Rough bandwidth calculation. We don't calculate since we issue the request because that would include sending the request, etc. This way we can
>> 
>> "We don't calculate since we issue the request because that would include sending the request", please can you try to rephrase this? I'm not sure what you're trying to say.
> 
> Yes, I can, I guess you want it in the comment as well but the the case is that I don't know when the first batch of data is sent by the server, only when we receive data or when we request it so if reset the counters when I issue the request I will be including the time of sending the request to the server and I am interested in getting only the bandwidth from the server to us. For that I discard the first data pack and begin to count from there. It's just an aproximation but it is better aproximated this way, I think.

Gotcha, bandwidth approximation is tricky, seems like something our network code in WebKit should be providing for us (ResourceLoadStatistics?). Anyway, not a big deal for now, just again it feels like the wrong place for this.

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #7)
> >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2872
> >>> +        webKitWebSrcSetOnDiskBufferingEnabled(WEBKIT_WEB_SRC_CAST(m_source.get()), shouldDownload);
> >> 
> >> It seems wrong for a source to need to know whether downstream is disk buffering or RAM buffering, souphttpsrc doesn't need to AFAICT, and doesn't have these behaviours.
> > 
> > If we don't have download enabled, any logic based on something different than percentages is useless because it is the queue that is going to limit us. That's why I introduced this. I prefer a timed logic because it is what MSE providers do, by wanting to have always 30s, which motivated my limits as well.
> 
> My understanding is that the pipeline should be giving us enough information
> through BUFFERING messages to keep the pipeline from stalling out, if it is
> not then I would say that's a bug in GStreamer. I don't think the web src
> should need knowledge of MSE at all, it's not its responsibility. If we need
> specific amounts of queued data for that case, why can't we configure the
> GstQueue2 appropriately rather than modifying the source?

When I talk about MSE is just an example of how MSE players behave, they try to keep a certain amount of time buffered instead of relying just on percentages. For me this is the proper behavior because we get percentages of the queue2 size and in the two use cases:

* Having download enabled, we might not stop the downloading because the queue is rarely full, meaning that we are going to download the full file and keep it on disk, increasing the disk size and maybe using a lot of bandwidth. Imagine the case of two hours 1080p video. We could certainly download that, keep it on disk and then play it without relying on the network but the user could decide after 15 minutes that they are not interested. We have misused the bandwidth and maybe we got (if network is fast enough) the whole movie). MSE players keep 30s ahead.

* Having download disabled would mean that we would rely on the queue size that can be a problem as well because the queue size can be very small and some videos that be not enough and we might stall. Actually this patch does not fix this because I found some problem that prevented me to trust this calculation. I might revisit this topic.

> >>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:84
> >>> +    static constexpr float s_reduceBlocksizeLimit { 0.5 };
> >> 
> >> These constants were taken from souphttpsrc, does that also stall out on the respective bugs? Because our block size algorithm is (should be) identical, so it concerns me that we're performing magic here and don't know exactly what is going wrong.
> > 
> > From the behavior I saw while debugging this bug, I had the empirical impression that these values make more sente because otherwise the blocksize can be increased quite quickly and we are not able to fulfill it almost ever.
> > 
> > I don't think this has a big impact as I think modifying some other pieces was more important to achieve a better behavior.
> 
> I'm a little concerned we can't keep the buffers full, strange that soup can
> be I can accept we perhaps have differences in our net code and the direct
> use of the soup API in gstsouphttpsrc.

I didn't read soupsrc code but if it behaves the same (or similar) to what we did, in the cases I tried we almost never provide full buffers for two reasons, size grows too fast and they have a hard time to be reduced but with the behavior of "waiting a bit and then pushing what we have" you have the impression of being pumping all the time. I don't know if they use the fast API, but not using it also helps.

> >>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1229
> >>> +    // Rough bandwidth calculation. We don't calculate since we issue the request because that would include sending the request, etc. This way we can
> >> 
> >> "We don't calculate since we issue the request because that would include sending the request", please can you try to rephrase this? I'm not sure what you're trying to say.
> > 
> > Yes, I can, I guess you want it in the comment as well but the the case is that I don't know when the first batch of data is sent by the server, only when we receive data or when we request it so if reset the counters when I issue the request I will be including the time of sending the request to the server and I am interested in getting only the bandwidth from the server to us. For that I discard the first data pack and begin to count from there. It's just an aproximation but it is better aproximated this way, I think.
> 
> Gotcha, bandwidth approximation is tricky, seems like something our network
> code in WebKit should be providing for us (ResourceLoadStatistics?). Anyway,
> not a big deal for now, just again it feels like the wrong place for this.

Well, I guess I could try to investigate that but in this case, for me the important thing here was able to provide an idea of it is was the source that was not pushing things fast enough (which happened with the fast API) or it was the network thas was lagging behind.

[Xabier Rodríguez Calvar]

Created attachment 386442 [details]
Patch

Honored some style comments

[Thibault Saunier]

Comment on attachment 385769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385769&action=review

>>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2285
>>>> +    m_didDownloadFinish = percentage == 100 && (!WEBKIT_IS_WEB_SRC(m_source.get()) || !webKitSrcIsWaitingForData(WEBKIT_WEB_SRC_CAST(m_source.get())));
>>> 
>>> I find the didDownloadFinish flag confusing. It seems we need to more clear about what posted the given percentage, if its a downloadbuffer then it makes sense, but if it's a queue somewhere in the pipeline it does not. I feel like these two concepts need to made more explicit in the player, and that would help our buffering bugs, since it looks like we mix these two modes up.
>>> 
>>> It is easier to read this as percentage==100 && isWebSrcWaitingForData(), where the cast check is performed in its own function. Even further, it strongly suggests to me we need to refactor all the buffering code into something that is self-aware of what the pipeline configuration is, so that this logic is not so twisted in with the rest of the player logic.
>> 
>> The problem of all this is that the percentages are a bit confusing. Percents are sent in a different way depending on the download mode. If it is inactive it is the queue just downstream the source that answers that and it is 100% when it is full, which is different from having download enabled.
> 
> I agree, perhaps we can use better variable names to help keep this clear in the code.

Note that I wrote a commit on top of yours: https://github.com/thiblahute/webkit/commit/245e80aadf35ee1e6a731c613e793c03a3d7f840 I am not proposing as it is quite integrated with your changes but I think we should discuss both commits here.

My approach here is to remove WebkitWebSrc specific code here as it is not needed anymore: https://github.com/thiblahute/webkit/commit/245e80aadf35ee1e6a731c613e793c03a3d7f840#diff-5ee59a48f2b7b824ae265766f42c0752R2307

The main issue you are working around is something I fixed in that on top commit, see the ChangeLog there:

> (WebCore::MediaPlayerPrivateGStreamer::fillTimerFired): Query buffering on the pipeline not the source
>    otherwise GstBaseSrc returns some useless values before `downloadbuffer` actually gives us the
>    info about DOWNLOAD buffering status. Also ignores response if they are not in DOWNLOAD mode as those
>    will end up screwing our buffering management algorithm.

Before that change we were dealing with meaningless values here and our buffering algorithm was breaking badly.

>>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2872
>>>> +        webKitWebSrcSetOnDiskBufferingEnabled(WEBKIT_WEB_SRC_CAST(m_source.get()), shouldDownload);
>>> 
>>> It seems wrong for a source to need to know whether downstream is disk buffering or RAM buffering, souphttpsrc doesn't need to AFAICT, and doesn't have these behaviours.
>> 
>> If we don't have download enabled, any logic based on something different than percentages is useless because it is the queue that is going to limit us. That's why I introduced this. I prefer a timed logic because it is what MSE providers do, by wanting to have always 30s, which motivated my limits as well.
> 
> My understanding is that the pipeline should be giving us enough information through BUFFERING messages to keep the pipeline from stalling out, if it is not then I would say that's a bug in GStreamer. I don't think the web src should need knowledge of MSE at all, it's not its responsibility. If we need specific amounts of queued data for that case, why can't we configure the GstQueue2 appropriately rather than modifying the source?

In my commit on top I kept the idea of having a different behavior based on whether we are using `downloadbuffer` or not, but drastically simplified the logic by saying that if we are buffering on disk, we can download as fast as possible as downstream will never block as it will just save the stream on disk as fast as possible.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:361
>> +static WTF::Optional<bool> WARN_UNUSED_RETURN areNextSecondsBuffered(WebKitWebSrc* src, const Seconds& seconds)
> 
> This logic seems like it should be in the player.

In my commit I just removed that logic all together as previously explained.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:413
>> +    GST_TRACE_OBJECT(src, "is download suspented %s, does have EOS %s, does have size %s, is seekable %s, size %" G_GUINT64_FORMAT
> 
> suspented typo

s/suspented/suspended/

>>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-400
>>>> -        size_t available = gst_adapter_available_fast(priv->adapter.get());
>>> 
>>> A comment about the switch from the fast to slow should be added.
>> 
>> I can do it if necessary but an detailed read to the fast API makes you realize that for some cases it is not that fast and I think we were using it wrong before. I don't think it is needed.
> 
> I would add a note, it will save the next programmer from having to do a detailed read of the fast API ;)

I do not think using the fast

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:535
> +                // Position is not going to move when download is suspended and we are PAUSED, so we can block here without checking the position.

In PAUSED with a `downloadbuffer` we should not block at all as the stream will just be downloaded on disk, I do not think that condition is a good idea.

[Philippe Normand]

Comment on attachment 385769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385769&action=review

>>>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-400
>>>>> -        size_t available = gst_adapter_available_fast(priv->adapter.get());
>>>> 
>>>> A comment about the switch from the fast to slow should be added.
>>> 
>>> I can do it if necessary but an detailed read to the fast API makes you realize that for some cases it is not that fast and I think we were using it wrong before. I don't think it is needed.
>> 
>> I would add a note, it will save the next programmer from having to do a detailed read of the fast API ;)
> 
> I do not think using the fast

The suspense is killing me. Can you finish the sentence? :)

[Thibault Saunier]

(In reply to Philippe Normand from comment #11)
> Comment on attachment 385769 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385769&action=review
> 
> >>>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-400
> >>>>> -        size_t available = gst_adapter_available_fast(priv->adapter.get());
> >>>> 
> >>>> A comment about the switch from the fast to slow should be added.
> >>> 
> >>> I can do it if necessary but an detailed read to the fast API makes you realize that for some cases it is not that fast and I think we were using it wrong before. I don't think it is needed.
> >> 
> >> I would add a note, it will save the next programmer from having to do a detailed read of the fast API ;)
> > 
> > I do not think using the fast
> 
> The suspense is killing me. Can you finish the sentence? :)

hehe, I think the idea was that I do not see why we use that API here, but then decided that I would just not comment here but forgot to remove the start of my sentence :-)

[Xabier Rodríguez Calvar]

(In reply to Thibault Saunier from comment #10)
> In my commit on top I kept the idea of having a different behavior based on
> whether we are using `downloadbuffer` or not, but drastically simplified the
> logic by saying that if we are buffering on disk, we can download as fast as
> possible as downstream will never block as it will just save the stream on
> disk as fast as possible.

And here is where I disagree. I don't think we want to download as fast as possible and that's why I implemented the time based solution. I think downloading as fast as possible can be a problem in disk limited devices.

[Xabier Rodríguez Calvar]

Reopening to attach new patch.

[Xabier Rodríguez Calvar]

Created attachment 387223 [details]
Patch

[Xabier Rodríguez Calvar]

Wrong bug, webkit-patch tricked me. Sorry for the noise