[Webkit-unassigned] [Bug 164585] [GStreamer][OWR] poor video rendering in apprtc

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=164585

--- Comment #6 from Philippe Normand <pnormand at igalia.com> ---
Comment on attachment 294355
  --> https://bugs.webkit.org/attachment.cgi?id=294355
patch

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

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

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.

>> 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.

👍

>> 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.

👍

>> Source/WebCore/platform/mediastream/MediaStreamPrivate.h:115
>> +    GRefPtr<OwrGstVideoRenderer> m_gstVideoRenderer = nullptr;
> 
> Isn't this setting to null redundant?

I suppose it is!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161111/682749e8/attachment-0001.html>