[webkit-reviews] review denied: [Bug 195948] [GStreamer] Switch back to webkitwebsrc for adaptive streaming fragments downloading : [Attachment 365172] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 195948: [GStreamer] Switch back to webkitwebsrc for adaptive streaming
fragments downloading
https://bugs.webkit.org/show_bug.cgi?id=195948

Attachment 365172: Patch

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




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