Bug 206003 – [GStreamer] Rework WebKitWebSrc to improve robustness

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

[Xabier Rodríguez Calvar]

[GStreamer] Rework WebKitWebSrc to improve robustness

[Xabier Rodríguez Calvar]

Created attachment 387224 [details]
Patch

[Charlie Turner]

Comment on attachment 387224 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        waiting for more buffers that could be retrieved with the those

typo, that
type, the those

> Source/WebCore/ChangeLog:13
> +        the API that is not marked as fast.

that are not

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2618
> +            if ((m_isBuffering && !m_isLiveStream) || !m_playbackRate) {

Why do we not pause buffering live streams?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:361
> +        " (min %u)", boolForPrinting(priv->isDownloadSuspended), boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->haveSize)

why do we format MAX SIZE in min %u?

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

s/doesHaveEOS/haveEOS/

I understand the EOS condition, but it's not clear why the absence of a Content-Length, not being seekable and/or being a small resource forbids us from stopping/restarting a download.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:374
> +    GST_TRACE_OBJECT(src, "queue size %" G_GUINT64_FORMAT " (min %1.0lf)", queueSize

I don't think you need %lf, just %f is fine?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:375
> +        , priv->size * HIGH_QUEUE_FACTOR_THRESHOLD * LOW_QUEUE_FACTOR_THRESHOLD);

This seems like an uncharacteristic line break for you :-)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:399
> +        , SMALL_MEDIA_RESOURCE_MAX_SIZE);

Ditto MIN/MAX question

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

Why can we stop/restart in haveEOS now, but not in restartLoaderIfNeeded? There's duplicated checks in these mentioned methods you should take care of.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:415
> +    priv->downloadStartTime = WallTime();

This looks like it is downloadStopTime, not start time...

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:429
>              GST_DEBUG_OBJECT(src, "Seeking, flushing adapter");

We are clearing now, not flushing. There seems to be some differences in behaviour here that are not obvious, particularly around pts/dts distance accounting the gstadapter. I don't know what effects this might have.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:450
> +    // We don't read the GstAdapter methods marked as fast anymore because sometimes it was slower as we were waiting for more buffers that could be

than could be

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:451
> +    // retrieved with the those methods to be available and we had enough if we retrieved it with the API that is not marked as fast.

error, the those. Please reword the comment. Also it's not clear what parts of this method the comment is even tied to anymore.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:453
> +        , boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->isDownloadSuspended));

Again, I'm not sure what our code style is, I get told to put these all on one line, but I prefer breaking like this, so....

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:474
> +            GST_TRACE_OBJECT(src, "available %" G_GSIZE_FORMAT, queueSize);

available BYTES, helps log readers

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:481
> +            // 1. We are the end of the known size of the file.

We are at the end of the file with a known size

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:482
> +            // 2. Download is not suspended and no more data is arriving. We cannot wait forever, 10x1s seems safe and sensible.

The download, more data are arriving

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:487
> +            GST_TRACE_OBJECT(src, "is download suspended %s, tried %u", boolForPrinting(priv->isDownloadSuspended), retries + 1);

isDownloadSuspended? %s, num retries: %u (makes it easier on the log reader)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1152
> +    // that first package delivery would include the time of sending out the request and getting the data back. Since we can't distingüish the

encoding issue

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1157
> +        GST_TRACE_OBJECT(src, "downloaded %u bytes in %lf seconds =~ %1.0lf bytes/second", priv->totalDownloadedBytes, timeSinceStart, priv->totalDownloadedBytes / timeSinceStart);

I'm always nervous of unchecked division by zero, not matter how unlikely. Also the downloadStartTime being set in stopLoaderIfNecessary is smelling off here. Also %f instead of %lf

[Xabier Rodríguez Calvar]

Comment on attachment 387224 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2618
>> +            if ((m_isBuffering && !m_isLiveStream) || !m_playbackRate) {
> 
> Why do we not pause buffering live streams?

This fixes a bug that was introduced during restyling. I think I found an issue during testing and realized of the regression.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:361
>> +        " (min %u)", boolForPrinting(priv->isDownloadSuspended), boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->haveSize)
> 
> why do we format MAX SIZE in min %u?

I think we name of the constant is misleading. For me is the MINIMAL size under which we don't stop/restart.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:363
>> +    if (priv->doesHaveEOS || !priv->haveSize || !priv->isSeekable || priv->size <= SMALL_MEDIA_RESOURCE_MAX_SIZE) {
> 
> s/doesHaveEOS/haveEOS/
> 
> I understand the EOS condition, but it's not clear why the absence of a Content-Length, not being seekable and/or being a small resource forbids us from stopping/restarting a download.

In the case of a small resource it is just a matter of not bothering. If it's small we load it all and are happy. The case of being seekable is because we need to specify which range we want and if we can't do range requests, we can't seek.

These checks where already in the old code I just refactored it.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:375
>> +        , priv->size * HIGH_QUEUE_FACTOR_THRESHOLD * LOW_QUEUE_FACTOR_THRESHOLD);
> 
> This seems like an uncharacteristic line break for you :-)

Right? :) My limit is 150 chars.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:400
>> +    if (!priv->haveSize || !priv->isSeekable || priv->size <= SMALL_MEDIA_RESOURCE_MAX_SIZE) {
> 
> Why can we stop/restart in haveEOS now, but not in restartLoaderIfNeeded? There's duplicated checks in these mentioned methods you should take care of.

Again, these checks were already in the original code, I just moved them around.

For me the sense of this is that when stopping we can't (or shouldn't) be in EOS as we are downloading. In the case of restart, if we already signalled EOS to the pipeline so it does not make sense to push any more data downstream so we are going to abort any restarting attempts.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:415
>> +    priv->downloadStartTime = WallTime();
> 
> This looks like it is downloadStopTime, not start time...

No, it's start time. It's the time we consider we started the download.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:429
>>              GST_DEBUG_OBJECT(src, "Seeking, flushing adapter");
> 
> We are clearing now, not flushing. There seems to be some differences in behaviour here that are not obvious, particularly around pts/dts distance accounting the gstadapter. I don't know what effects this might have.

queueSize matches gst_adapter_available, which is the total size of bytes stored in it. Flushing the queuSize is the same as clearing and that's why I also removed queueSize, it was redundant and even gst_adapter_available is quick to call.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:453
>> +        , boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->isDownloadSuspended));
> 
> Again, I'm not sure what our code style is, I get told to put these all on one line, but I prefer breaking like this, so....

The style does not set any fixed size, only something like "they should fit a reasonably screen size". I set my limit to 150.

[Charlie Turner]

Informal r+ from then.

[Xabier Rodríguez Calvar]

Created attachment 387243 [details]
Patch

[Philippe Normand]

Comment on attachment 387243 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +

Remove this extra blank line please.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:148
> +    WallTime downloadStartTime { WallTime() };

Is the default constructor here really wise to use? I mean, the download hasn't started exactly when this struct is being initialized, right?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:149
> +    guint totalDownloadedBytes { 0 };

Shouldn't we use uint64_t here?

[Xabier Rodríguez Calvar]

Comment on attachment 387243 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +
> 
> Remove this extra blank line please.

Sure.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:148
>> +    WallTime downloadStartTime { WallTime() };
> 
> Is the default constructor here really wise to use? I mean, the download hasn't started exactly when this struct is being initialized, right?

No, this is invalid, it will be initialized to now the first time we receive data.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:149
>> +    guint totalDownloadedBytes { 0 };
> 
> Shouldn't we use uint64_t here?

Yes.

[Philippe Normand]

Comment on attachment 387243 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:148
>>> +    WallTime downloadStartTime { WallTime() };
>> 
>> Is the default constructor here really wise to use? I mean, the download hasn't started exactly when this struct is being initialized, right?
> 
> No, this is invalid, it will be initialized to now the first time we receive data.

Perhaps use WallTime::nan() then?

[Xabier Rodríguez Calvar]

Created attachment 387511 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387511 [details]
Patch

Clearing flags on attachment: 387511

Committed r254503: <https://trac.webkit.org/changeset/254503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58562312>