Bug 220463

Created attachment 417382 [details]
Patch

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?

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 :)

Created attachment 417620 [details]
Patch

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.

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

<rdar://problem/73239616>