<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GStreamer][OWR] poor video rendering in apprtc"
href="https://bugs.webkit.org/show_bug.cgi?id=164585#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GStreamer][OWR] poor video rendering in apprtc"
href="https://bugs.webkit.org/show_bug.cgi?id=164585">bug 164585</a>
from <span class="vcard"><a class="email" href="mailto:pnormand@igalia.com" title="Philippe Normand <pnormand@igalia.com>"> <span class="fn">Philippe Normand</span></a>
</span></b>
<pre>Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=294355&action=diff" name="attach_294355" title="patch">attachment 294355</a> <a href="attachment.cgi?id=294355&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=294355&action=review">https://bugs.webkit.org/attachment.cgi?id=294355&action=review</a>
<span class="quote">>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:343
>> + GstElement* sink;
>
> This needs to be GRefPtr, why?
>
> createGLAppSink and gst_bin_new return a floating ref.
> g_object_get_ returns a hard ref.
>
> I didn't dig into the code of g_object_new/set to know what happens with an object passed as argument to the constructor, if the ref is sinked or not, but g_object_get returns a hard ref so we need to change this.
>
> I am going thru all this and even with this naïve code, the ref counting is incredibly hard to follow :) Not saying that is wrong or that there is another better way to do, just stating the fact, specially because OWR is undocumented and you have to check the code directly</span >
If this needs to be done, can it be done in a follow-up bug please. This patch doesn't introduce new leaks, I'm ok fixing the one you found below, which was introduced in an older commit though. But changing the video sink management across 3 media players is beyond the scope of this issue, in my opinion.
<span class="quote">>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:362
>> + GRefPtr<GstPad> pad = gst_element_get_static_pad(gldownload, "sink");
>
> gst_element_get_static_pad is transfer full, you need to adopt the reference here. If it were transfer floating it wouldn't be a problem cause the ref_sink would just unfloat the reference, but being full, you need to take it for yourself by adopting it.
>
> gst_ghost_pad_new is transfer none, so it will ref if it needs that reference.</span >
👍
<span class="quote">>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:371
>> + m_streamPrivate->setVideoRenderer(m_videoRenderer.get(), MediaPlayerPrivateGStreamerBase::videoSink());
>
> videoSink seems to be a protected method defined at Base. You don't seem to need to use the class name here.</span >
👍
<span class="quote">>> Source/WebCore/platform/mediastream/MediaStreamPrivate.h:115
>> + GRefPtr<OwrGstVideoRenderer> m_gstVideoRenderer = nullptr;
>
> Isn't this setting to null redundant?</span >
I suppose it is!</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>