[webkit-reviews] review denied: [Bug 204273] [GStreamer] Pipeline lifetime tracking : [Attachment 384038] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied  review:
Bug 204273: [GStreamer] Pipeline lifetime tracking
https://bugs.webkit.org/show_bug.cgi?id=204273

Attachment 384038: Patch

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




--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 384038
  --> https://bugs.webkit.org/attachment.cgi?id=384038
Patch

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

I think it's easier to understand this patch if it also includes the inspector
part, so I would merge both patches into one.

> Source/WebCore/html/HTMLMediaElement.cpp:7230
> +void HTMLMediaElement::pipelineCreated(GstElement* pipeline)
> +{
> +    if (!document().page())
> +	   return;
> +
> +    registerGStreamerPipeline(pipeline, document().page());
> +}
> +
> +void HTMLMediaElement::pipelineWillBeDestroyed(GstElement* pipeline)
> +{
> +    if (!document().page())
> +	   return;
> +
> +    unregisterGStreamerPipeline(pipeline, document().page());

We only need to access the page here to notify the inspector about changes in
the pipelines. I think it would be simpler if the register/unregister happens
in the platform code, without using the page, and then use the media player to
otify the inspector. So, this would be reduced to a single method
HTMLMediaElement::gstreamerPipelinesChanged() (for example) that could notify
the inspector directly. That way we don't need any GST code here.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:28
> +#include "Document.h"

This is a layering violation

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:32
> +#include "PlatformMediaSessionManager.h"

Ditto.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:98
> +    m_pipeline = gst_pipeline_new("webaudio-playback");
> +
> +    auto& manager = PlatformMediaSessionManager::sharedManager();
> +    auto* session = manager.currentSession();
> +    if (session) {
> +	   auto& client = session->client();
> +	   auto* document = client.hostingDocument();
> +	   if (document)
> +	       registerGStreamerPipeline(m_pipeline, document->page());
> +    }

Unfortunately we can't use PlatformMediaSessionManager from platform code, we
need to find another way to notify the inspector when this pipeline is created.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:155
> +    auto& manager = PlatformMediaSessionManager::sharedManager();
> +    auto* session = manager.currentSession();
> +    if (session) {
> +	   auto& client = session->client();
> +	   auto* document = client.hostingDocument();
> +	   if (document)
> +	       unregisterGStreamerPipeline(m_pipeline, document->page());
> +    }

Ditto.

> Source/WebCore/platform/graphics/MediaPlayer.h:61
> +#if USE(GSTREAMER)
> +typedef struct _GstElement GstElement;
> +#endif

This shouldn't be needed if we split the register/unregister and the inspector
notification.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:28
> +#include "Page.h"

This is a layering violation too.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:385
> +HashMap<String, GstBin*> s_webKitGStreamerPipelines;

We normally use NeverDestroy for this kind of global maps, with a functions to
get a reference to it instead of suing a global variable. Use CString instead
of String, since we are storing utf8 strings.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:390
> +    if (!GST_IS_ELEMENT(pipeline))
> +	   return;

I would assert instead, this is not expected to happen I guess.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:396
> +    if (s_webKitGStreamerPipelines.contains(name.get()))
> +	   return;
> +
> +    s_webKitGStreamerPipelines.add(name.get(), GST_BIN_CAST(pipeline));

You can do this with a single call to add:

auto addResult = pipelinesMap().add(name.get(), nullptr);
if (!addResult.isNewEntry)
    return;
addResult.iterator->value = GST_BIN_CAST(pipeline);

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:398
> +    // TODO: Hook to InspectorInstrumentation here.

Use FIXME: instead of TODO

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:411
> +    if (!s_webKitGStreamerPipelines.contains(name.get()))
> +	   return;
> +
> +    s_webKitGStreamerPipelines.remove(name.get());

Use take().

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:417
> +Vector<String> gstPipelinesNames()

gstreamerPipelineNames() and Use CString

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:427
> +String dumpGStreamerPipeline(String name, String subBinName)

const String& name, const String& subBinName. Return a CString

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:434
> +	   return gst_debug_bin_to_dot_data(pipeline,
GST_DEBUG_GRAPH_SHOW_ALL);

You are leaking the result of  gst_debug_bin_to_dot_data().

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:437
> +    if (child && GST_IS_BIN(child.get()))

GST_IS_BIN() already does the null check

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:438
> +	   return gst_debug_bin_to_dot_data(GST_BIN_CAST(child.get()),
GST_DEBUG_GRAPH_SHOW_ALL);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:35
> +class Page;

Layering violation.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2413
> +	   if (m_player)
> +	       m_player->client().pipelineWillBeDestroyed(m_pipeline.get());

So here you would call the unregister directly and then
m_player->client().gstreamerPipelinesChanaged().

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
349
> +    if (m_pipeline) {
> +	   if (m_player)
> +	       m_player->client().pipelineWillBeDestroyed(m_pipeline.get());
>	   gst_element_set_state(m_pipeline.get(), GST_STATE_NULL);
> +    }

Ditto.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
358
> +    if (m_player)
> +	   m_player->client().pipelineCreated(m_pipeline.get());

Adn same for the register

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:121
> +   
m_playerPrivate->mediaPlayer()->client().pipelineCreated(m_pipeline.get());

Ditto.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:247
> +	   if (m_playerPrivate)
> +	      
m_playerPrivate->mediaPlayer()->client().pipelineWillBeDestroyed(m_pipeline.get
());

Ditto.