Bug 195948 – [GStreamer] Switch back to webkitwebsrc for adaptive streaming fragments downloading

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Philippe Normand]

.

[Philippe Normand]

Created attachment 365172 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 365172 [details]
Patch

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

It is not necessarily wrong but I think we should handle the context in a different and probably more efficient way.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1342
> +            const char* uri = gst_structure_get_string(structure, "uri");
> +            const char* redirectionUri = gst_structure_get_string(structure, "redirection-uri");
> +            uri = redirectionUri ? redirectionUri : uri;

You can do:

const char* redirectionUri = gst_structure_get_string(structure, "redirection-uri");
uri = redirectionUri ? redirectionUri : gst_structure_get_string(structure, "uri");

and you might be saving one call.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1347
> +                auto origin = SecurityOrigin::create(url);
> +                m_origins.add(WTFMove(origin));

m_origins.add(SecurityOrigin::create(url));

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:346
> +    if (!g_strcmp0(contextType, WEBKIT_WEB_SRC_PLAYER_CONTEXT_TYPE_NAME)) {
> +        GRefPtr<GstContext> context = adoptGRef(gst_context_new(WEBKIT_WEB_SRC_PLAYER_CONTEXT_TYPE_NAME, FALSE));
> +        GstStructure* contextStructure = gst_context_writable_structure(context.get());
> +
> +        ASSERT(m_player);
> +        gst_structure_set(contextStructure, "player", G_TYPE_POINTER, m_player, nullptr);
> +        gst_element_set_context(GST_ELEMENT(GST_MESSAGE_SRC(message)), context.get());
> +        return true;
> +    }

This code is ok but unnecessary here.

GstContext is meant to the set on pipelines so that it is the pipeline that dispatches the context to the elements when they are created and added to it.

The pipeline cannot exist without the player private,  the private creates the pipeline and the player is (or should be) set to the private prior to that. So when the pipeline is created, we can set the context to it immediately (fire and forget!). Once the elements are created and added to it, the context should be immediately available.

Summing up, IMHO you should neither listen to this message nor send it (read my comment below) and just set the context to the pipeline when the pipeline is created.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:165
>      gst_element_class_add_static_pad_template(eklass, &srcTemplate);
>  
> +    gst_element_class_add_static_pad_template(eklass, &srcTemplate);

Am I seeing double here?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:498
> +    if (!priv->player) {
> +        GRefPtr<GstQuery> query = adoptGRef(gst_query_new_context(WEBKIT_WEB_SRC_PLAYER_CONTEXT_TYPE_NAME));
> +        if (gst_pad_peer_query(GST_BASE_SRC_PAD(baseSrc), query.get())) {
> +            GstContext* context;
> +
> +            gst_query_parse_context(query.get(), &context);
> +            gst_element_set_context(GST_ELEMENT_CAST(src), context);
> +        } else
> +            gst_element_post_message(GST_ELEMENT_CAST(src), gst_message_new_need_context(GST_OBJECT_CAST(src), WEBKIT_WEB_SRC_PLAYER_CONTEXT_TYPE_NAME));
> +    }

I doubt this is ever run. You could keep it but I'd suggest replacing it with a [RELEASE_]ASSERT(priv->player) because this is dead code.

My rationale behind  this is that GStreamer doc does indeed suggest querying for context if you don't have it. Here the context should be available just after the element is created and added to the pipeline, if the context was already set to the pipeline (see my comment above).

You can add a comment to explain this.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:768
> -    static const char* protocols[] = {"webkit+http", "webkit+https", "webkit+blob", nullptr };
> +    static const char* protocols[] = {"http", "https", "blob", nullptr };

Really nice to know that we are not leaking these element anymore cause this leaves in the web process :D

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:886
> +    long long length = response.expectedContentLength();

uint64_t?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:920
> +    auto scopeExit = makeScopeExit([&] {
> +        GstStructure* structure = gst_structure_copy(src->priv->httpHeaders.get());
> +        gst_element_post_message(GST_ELEMENT_CAST(src), gst_message_new_element(GST_OBJECT_CAST(src), structure));
> +    });

Are you ok with this running at the end of the function, AFTER the completion handler?

[Philippe Normand]

Comment on attachment 365172 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:346
>> +    }
> 
> This code is ok but unnecessary here.
> 
> GstContext is meant to the set on pipelines so that it is the pipeline that dispatches the context to the elements when they are created and added to it.
> 
> The pipeline cannot exist without the player private,  the private creates the pipeline and the player is (or should be) set to the private prior to that. So when the pipeline is created, we can set the context to it immediately (fire and forget!). Once the elements are created and added to it, the context should be immediately available.
> 
> Summing up, IMHO you should neither listen to this message nor send it (read my comment below) and just set the context to the pipeline when the pipeline is created.

This code is needed because secondary webkitwebsrc elements created by adaptivedemux/uridownloader have no relationship whatsoever with the first webkitwebsrc that was used to download the manifest. Anyway, I tried what you suggest, set the context on the whole pipeline when it was created, and secondary webkitwebsrc element now crashes because of the RELEASE_ASSERT I added in webKitWebSrcStart(), which means the element context is not set early enough.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:920
>> +    });
> 
> Are you ok with this running at the end of the function, AFTER the completion handler?

Yes!

[Xabier Rodríguez Calvar]

Comment on attachment 365172 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:346
>>> +    }
>> 
>> This code is ok but unnecessary here.
>> 
>> GstContext is meant to the set on pipelines so that it is the pipeline that dispatches the context to the elements when they are created and added to it.
>> 
>> The pipeline cannot exist without the player private,  the private creates the pipeline and the player is (or should be) set to the private prior to that. So when the pipeline is created, we can set the context to it immediately (fire and forget!). Once the elements are created and added to it, the context should be immediately available.
>> 
>> Summing up, IMHO you should neither listen to this message nor send it (read my comment below) and just set the context to the pipeline when the pipeline is created.
> 
> This code is needed because secondary webkitwebsrc elements created by adaptivedemux/uridownloader have no relationship whatsoever with the first webkitwebsrc that was used to download the manifest. Anyway, I tried what you suggest, set the context on the whole pipeline when it was created, and secondary webkitwebsrc element now crashes because of the RELEASE_ASSERT I added in webKitWebSrcStart(), which means the element context is not set early enough.

I understand, then we're cool regarding the messages, queries and context. Please fix the other minor things and we're good to go

[Philippe Normand]

Committed r243197: <https://trac.webkit.org/changeset/243197>

[Radar WebKit Bug Importer]

<rdar://problem/49057856>