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.
Created attachment 294355 [details] patch
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
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
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?
Created attachment 294481 [details] patch
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!
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.
Committed r208676: <http://trac.webkit.org/changeset/208676>