Bug 220463 – [GStreamer] Clean-up the TextCombiner

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

Coding style does not conform WebKit's, and it should use the WEBKIT_DEFINE_TYPE machinery.

[Philippe Normand]

Created attachment 417382 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 417382 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:121
> +    auto internalPad = adoptGRef(gst_element_request_pad(combiner->priv->combinerElement.get(), templ, name, caps));
> +    g_object_set(WEBKIT_TEXT_COMBINER_PAD(ghostPad), "inner-combiner-pad", internalPad.leakRef(), nullptr);

You're probably astonished that I tell you this but I don't think you need to create a smart ptr to just release it in the next line.

> Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:142
> +    gst_element_release_request_pad(combiner->priv->combinerElement.get(), webKitTextCombinerPadLeakInternalPadRef(combinerPad));

Aren't we leaking here? gst_element_release_request_pad doc says "This does not unref the pad. If the pad was created by using gst_element_request_pad(), gst_element_release_request_pad() needs to be followed by gst_object_unref() to free the pad."

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

You don't need this if you include it in the header.

> Source/WebCore/platform/graphics/gstreamer/TextCombinerPadGStreamer.h:31
> +#include "GRefPtrGStreamer.h"

GStreamerCommon.h?

[Philippe Normand]

Comment on attachment 417382 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/TextCombinerPadGStreamer.h:31
>> +#include "GRefPtrGStreamer.h"
> 
> GStreamerCommon.h?

I was profiling the impact of including that header recently, here it adds a 2 seconds wall-time overhead. So unless really needed I now prefer to not include it directly, there is a way to reduce the impact it has but this is another rabbit hole :)

[Philippe Normand]

Created attachment 417620 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 417620 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:142
> +    auto* internalPad = webKitTextCombinerPadLeakInternalPadRef(combinerPad);
> +    gst_element_release_request_pad(combiner->priv->combinerElement.get(), internalPad);
> +    gst_object_unref(internalPad);

Nit: I would adopt it here not unref. I would try to avoid manual unrefs as much as possible cause they can lead to leaks if we change the code around and forget to call it when it is due.

[Philippe Normand]

Committed r271512: <https://trac.webkit.org/changeset/271512>

[Radar WebKit Bug Importer]

<rdar://problem/73239616>