[webkit-reviews] review granted: [Bug 195631] [GStreamer] Rewrite HTTP source element using pushsrc base class : [Attachment 364407] 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 195631: [GStreamer] Rewrite HTTP source element using pushsrc base class
https://bugs.webkit.org/show_bug.cgi?id=195631

Attachment 364407: Patch

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




--- Comment #4 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 364407
  --> https://bugs.webkit.org/attachment.cgi?id=364407
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:87
> +    bool headersReceived;

wereHeadersReceived

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:91
> +    bool responseReceived;

wasResponseReceived

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:92
> +    bool headersReceived;
> +    Condition headersCondition;
> +    Lock responseLock;
> +
> +    bool responseReceived;
> +    Condition responseCondition;

These variables and their locks and conditions seem a bit unsorted, specially
if you consider what seem to be random empty lines.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:101
> +    bool haveEOS;

doesHaveEOS

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:102
> +    bool flushing;

isFlushing

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:106
> +    guint64 readPosition;
> +    guint64 requestedPosition;
> +    guint64 stopPosition;

uint64_t

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:112
> +    bool seekable;

isSeekable

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:114
> +    bool isSeeking;
> +    bool wasSeeking;

\o/

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:122
> +    guint64 queueSize;

uint64_t

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:171
>      GstElementClass* eklass = GST_ELEMENT_CLASS(klass);
> +    GstBaseSrcClass* baseSrcClass = GST_BASE_SRC_CLASS(klass);
> +    GstPushSrcClass* pushSrcClass = GST_PUSH_SRC_CLASS(klass);

I think we should move this vars to the place they are first used.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:181
> +	   "Sebastian Dröge <sebastian.droege at collabora.co.uk>");

You're doing a substantial rewrite, I think you can take the ownership of this
element, specially considering the outdated email address.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:246
> +    priv->queueSize = 0;

You can probably initialize this and many other attributes with { } in the
declaration.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:248
> +    priv->wasSeeking = false;

Ditto

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:349
> +	   guint64 requestedPosition = priv->requestedPosition;

uint64_t

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:361
> +	   while (!priv->responseReceived) {
> +	       if (priv->flushing)
> +		   break;
> +	       priv->responseCondition.wait(priv->responseLock);
> +	   }

Can't we do something like this here or am I missing anything?

priv->responseCondition.wait(priv->responseLock, [priv] () { return
priv->responseReceived || priv->flushing; });

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:377
> +    bool lastNonEmptyBuffer = false;

I don't understand the meaning of this variable so I won't try to rename it my
self but it should surely begin with is,should, etc.

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

size_t

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:395
> +		   isAdapterDrained = true;
> +		   break;

Nit: you can make the while (available < size && !isAdapterDrained) and avoid
the break

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:550
> +	   GUniquePtr<gchar> val(g_strdup_printf("bytes=%" G_GUINT64_FORMAT
"-", priv->requestedPosition));

char and val -> formatedRange

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:569
> +    Lock mutex;
> +    Condition condition;
> +    priv->notifier->notify(MainThreadSourceNotification::Start, [protector,
request = WTFMove(request), condition = &condition, mutex = &mutex] {

I think it might be interesting to abstract this behind the notifier and have a
notifyAndWait. That way you wouldn't need the mutex and the condition, you
could handle those inside that method.

If you decide to do this, I would like to review again

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:605
> +    priv->notifier->notify(MainThreadSourceNotification::Stop, [protector,
keepAlive = priv->keepAlive, condition = &condition, mutex = &mutex] {

ditto

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1010
> +    guint64 newPosition = priv->readPosition + length;

uint64_t

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1015
> +    guint64 newSize = 0;

uint64_t