Bug 204273 – [GStreamer] Pipeline lifetime tracking

• Component: Platform
• Status: RESOLVED DUPLICATE

[Philippe Normand]

This would be needed for the WebInspector GStreamer tab.

[Philippe Normand]

Created attachment 383697 [details]
Patch

[Philippe Normand]

Created attachment 383698 [details]
Patch

[Philippe Normand]

Created attachment 383700 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 383700 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:389
> +    String name = gst_object_get_name(GST_OBJECT_CAST(pipeline));

You're leaking the name here. gst_object_get_name returns a transfer full and the String constructor is going to copy it.

You can either use GUniquePtr or something like:
String name(StringImpl::createWithoutCopying(gst_object_get_name...));

Besides, I would recommend first return on !GST_IS_ELEMENT (maybe G_RETURN_IF_FAIL) just in case we get funny things. With nulls the best we can get is a critical on gst_object_get_name and the worst a crash if checks are disabled (rare but possible). By the code we're using it looks like we are not getting nulls but for the future, better safe than sorry.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:401
> +    String name = gst_object_get_name(GST_OBJECT_CAST(pipeline));

Ditto.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:415
> +    Vector<String> names;
> +    for (auto name : s_webKitGStreamerPipelines.keys())
> +        names.append(name);

We can try to save some allocations if we check the size first, allocate the vector and then appendUnchecked.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:424
> +        return "";

emptyString()? Or even better nullString().

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:429
> +    GRefPtr<GstElement> child = gst_bin_get_by_name(pipeline, subBinName.utf8().data());

gst_bin_get_by_name is transfer full, you need to adopt here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:433
> +    return "";

Ditto

[Philippe Normand]

Created attachment 384038 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 384038 [details]
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.

[Philippe Normand]

*** This bug has been marked as a duplicate of bug 204619 ***