Bug 199998 – REGRESSION(r243058): [GStreamer] WebKitWebSrc's internal queue can exhaust the WebProcess memory

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

We need a mechanism similar to appsrc's enough-data and need-data signals because currently if a large media is played from the network, webkitwebsrc will download it as fast as possible, even if the element is paused and the adapter size can easily run out of control.

[Philippe Normand]

I have started a patch btw.

[Philippe Normand]

Created attachment 374885 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 374885 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:50
> +// Never pause download of media resources smaller than 2MiB.
> +#define SMALL_MEDIA_RESOURCE_MAX_SIZE 2 * 1024 * 1024
> +
> +// Keep at most 2% of the full, non-small, media resource buffered. When this
> +// threshold is reached, the download task is paused.
> +#define HIGH_QUEUE_FACTOR_THRESHOLD 0.02
> +
> +// Keep at least 20% of maximum queue size buffered. When this threshold is
> +// reached, the download task resumes.
> +#define LOW_QUEUE_FACTOR_THRESHOLD 0.2

I am not against this now, but maybe in the medium-short term we could make this a websetting?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:143
> +    bool downloadingSuspended { false };

isDownloadSuspended

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:466
> +            if (!priv->doesHaveEOS && priv->haveSize && priv->isSeekable
> +                && (priv->size > SMALL_MEDIA_RESOURCE_MAX_SIZE) && priv->readPosition
> +                && (priv->readPosition != priv->size)
> +                && (priv->queueSize < (priv->size * HIGH_QUEUE_FACTOR_THRESHOLD * LOW_QUEUE_FACTOR_THRESHOLD))
> +                && (GST_STATE(src) == GST_STATE_PLAYING) && priv->downloadingSuspended) {

You can use longer lines here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1121
> +        if (priv->haveSize && (priv->size > SMALL_MEDIA_RESOURCE_MAX_SIZE) && (priv->queueSize > (priv->size * HIGH_QUEUE_FACTOR_THRESHOLD))
> +            && !priv->downloadingSuspended && priv->isSeekable) {

Maybe a longer line?

[Philippe Normand]

Comment on attachment 374885 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:50
>> +#define LOW_QUEUE_FACTOR_THRESHOLD 0.2
> 
> I am not against this now, but maybe in the medium-short term we could make this a websetting?

"this" being? Maybe it makes sense for the "small media resource max-size" define, but I'm not sure about the 2 others.

[Charlie Turner]

Comment on attachment 374885 [details]
Patch

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

I'm interested how you experimentally came up with these heuristics. If it's just of a local disk load as described in the commit message, that seems unrealistic. Did you consult any prior arts for their magical numbers at all?

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:466
>> +                && (GST_STATE(src) == GST_STATE_PLAYING) && priv->downloadingSuspended) {
> 
> You can use longer lines here.

Id vote against that, if it must be changed a function or two would be more comfortable. It's much easier to scan for the conditional parts when they are indented vertically like this than mixed up in a very long line imo.

[Philippe Normand]

(In reply to Charlie Turner from comment #5)
> Comment on attachment 374885 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374885&action=review
> 
> I'm interested how you experimentally came up with these heuristics. If it's
> just of a local disk load as described in the commit message, that seems
> unrealistic.

Why?

> Did you consult any prior arts for their magical numbers at all?
> 

No. I couldn't find "prior arts" beyond the appsrc-based webkitwebsrc element which was maintaining an even smaller internal queue.

This was tested with huge resources (1.3GiB) and smaller resources (up to a few hundreds MiB). The most important issue is fixed (download never pausing), I agree the "magic" values can be refined but I would defer it to a follow-up patch.

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 374885 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374885&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:50
> >> +#define LOW_QUEUE_FACTOR_THRESHOLD 0.2
> > 
> > I am not against this now, but maybe in the medium-short term we could make this a websetting?
> 
> "this" being? Maybe it makes sense for the "small media resource max-size"
> define, but I'm not sure about the 2 others.

Well, I don't have a strong opinion on this but it looks to me like these three values could be configurable in runtime. I am not against setting them on compile time though.

[Charlie Turner]

(In reply to Philippe Normand from comment #6)
> (In reply to Charlie Turner from comment #5)
> > Comment on attachment 374885 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=374885&action=review
> > 
> > I'm interested how you experimentally came up with these heuristics. If it's
> > just of a local disk load as described in the commit message, that seems
> > unrealistic.
> 
> Why?

It's an ideal environment where the network is not changing in quality. I'd intuit that the values would be dynamic rather than static to account for network fluctations, perhaps based on moving averages. But...

> 
> > Did you consult any prior arts for their magical numbers at all?
> > 
> 
> No. I couldn't find "prior arts" beyond the appsrc-based webkitwebsrc
> element which was maintaining an even smaller internal queue.
> 
> This was tested with huge resources (1.3GiB) and smaller resources (up to a
> few hundreds MiB). The most important issue is fixed (download never
> pausing), I agree the "magic" values can be refined but I would defer it to
> a follow-up patch.

OK then, I guess you've done some manual testing on a variety of resources and network conditions. It is hard to add tests for the sorts of things I mention above so we'll address them as-and-when.

[Philippe Normand]

Committed r247903: <https://trac.webkit.org/changeset/247903>

[Radar WebKit Bug Importer]

<rdar://problem/53661333>