[webkit-reviews] review granted: [Bug 199998] REGRESSION(r243058): [GStreamer] WebKitWebSrc's internal queue can exhaust the WebProcess memory : [Attachment 374885] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 199998: REGRESSION(r243058): [GStreamer] WebKitWebSrc's internal queue can
exhaust the WebProcess memory
https://bugs.webkit.org/show_bug.cgi?id=199998

Attachment 374885: Patch

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




--- Comment #3 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 374885
  --> https://bugs.webkit.org/attachment.cgi?id=374885
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?