Bug 195631 – [GStreamer] Rewrite HTTP source element using pushsrc base class

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

If we want to use webkitwebsrc in adaptivedemux (HLS, DASH, etc) we need a source element that behaves like souphttpsrc, which is implemented using pushsrc. This rewrite might also fix some seek issues...

[Philippe Normand]

Created attachment 364407 [details]
Patch

[Philippe Normand]

The diff differs *a lot* from the current implementation. The review might be easier by looking directly at the new versionn of the cpp file.

[EWS Watchlist]

Attachment 364407 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:342:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

Comment on attachment 364407 [details]
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 <[e-mail]>");

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

[Philippe Normand]

Comment on attachment 364407 [details]
Patch

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

>> 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.

It means the adapter doesn't have enough data in store to fill a full buffer of basesrc block-size, so the next buffer taken from the adapter will drain the adapter and have a smaller size than the previous buffers. Actually I found a way to get rid of this var :)

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:569
>> +    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

Will do

[Philippe Normand]

I found a few regressions when running the WPT harness... I'll upload a new patch soon hopefully.

[Philippe Normand]

Created attachment 364970 [details]
Patch

[EWS Watchlist]

Attachment 364970 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:329:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

Comment on attachment 364970 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:72
> +        notify(notificationType, [functor = WTFMove(callbackFunctor), condition = &condition, mutex = &mutex] {

nit: isn't it enough with writing ..., &condition, &muter] ? Then they should be passed by reference.

> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:74
> +            LockHolder holder(*mutex);

Nit: I don't think you need the * here (if you pass parameters as reference instead of pointer copy as you're doing above).

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

isDurationSet :)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:371
> +            if (available && (available < size))

nit: do you need the inner () ?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:936
> +        gst_structure_set(httpHeaders, "uri", G_TYPE_STRING, priv->originalURI.data(),
> +            "http-status-code", G_TYPE_UINT, response.httpStatusCode(), nullptr);

Nit: this can probably be just one-line.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:951
> +            bool ok = false;
> +            uint64_t convertedValue = header.value.toUInt64(&ok);
> +            if (ok)
> +                gst_structure_set(headers.get(), header.key.utf8().data(), G_TYPE_UINT64, convertedValue, nullptr);
> +            else
> +                gst_structure_set(headers.get(), header.key.utf8().data(), G_TYPE_STRING, header.value.utf8().data(), nullptr);

Why do you do this?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:960
> +        gst_element_post_message(GST_ELEMENT_CAST(src), gst_message_new_element(GST_OBJECT_CAST(src),
> +            gst_structure_copy(httpHeaders)));

Nit: this can probably be just one-line.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:979
> +    if (G_LIKELY (priv->requestedPosition == priv->readPosition))

I think we have LIKELY in WebKit

[Philippe Normand]

Comment on attachment 364970 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:951
>> +                gst_structure_set(headers.get(), header.key.utf8().data(), G_TYPE_STRING, header.value.utf8().data(), nullptr);
> 
> Why do you do this?

Because the player checks the content-length as an uint64, so I thought why not try to convert the fields that store numbers, for consistency.

[Philippe Normand]

Committed r243058: <https://trac.webkit.org/changeset/243058>

[Radar WebKit Bug Importer]

<rdar://problem/48975084>