Bug 164585 – [GStreamer][OWR] poor video rendering in apprtc

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

[Philippe Normand]

The apprtc service uses 3 video elements in total, one for local, one for remote and one called preview. During a call only remote and preview are displayed, preview being linked to the same mediastream as local. The consequence is that 2 OWR video renderers of the same source are created. When gst-gl is enabled this isn't a problem but when it is disabled a performance issue appears and the webkit video sink starts dropping frames.

The solution would be to have the video renderer shared between the 2 media players in this scenario.

[Philippe Normand]

Created attachment 294355 [details]
patch

[Build Bot]

Comment on attachment 294355 [details]
patch

Attachment 294355 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2488595

New failing tests:
storage/domstorage/sessionstorage/blocked-file-access.html

[Build Bot]

Created attachment 294361 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Xabier Rodríguez Calvar]

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

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

[Philippe Normand]

Created attachment 294481 [details]
patch

[Philippe Normand]

Comment on attachment 294355 [details]
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!

[Xabier Rodríguez Calvar]

Comment on attachment 294481 [details]
patch

(In reply to comment #6)
> 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.

Ok. If you fix any of the other things in landing time, please add a bug for this and a comment on the code.

[Philippe Normand]

Committed r208676: <http://trac.webkit.org/changeset/208676>