[GStreamerGL] Release GstVideoFrame when there is a flush event from the pipeline
Created attachment 310807 [details] Patch
Created attachment 311826 [details] Patch
(In reply to Gwang Yoon Hwang from comment #2) > Created attachment 311826 [details] > Patch Fixed a memory leak while handling flush event from the gst pipeline.
Comment on attachment 311826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311826&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:151 > +void TextureMapperPlatformLayerProxy::appendToUnusedBuffers(std::unique_ptr<TextureMapperPlatformLayerBuffer> buffer) > +{ > + ASSERT(m_compositorThreadID == WTF::currentThread()); > + RunLoop::main().dispatch([this, protectedThis = makeRef(*this), buffer = WTFMove(buffer)] () mutable { > + m_usedBuffers.append(WTFMove(buffer)); > + scheduleReleaseUnusedBuffers(); > + }); > +} Why does this have to be handled on the main thread?
Comment on attachment 311826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311826&action=review >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:151 >> +} > > Why does this have to be handled on the main thread? AFAIU, RunLoop::TimerBase::start is not thread safe, no?
Comment on attachment 311826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311826&action=review >>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:151 >>> +} >> >> Why does this have to be handled on the main thread? > > AFAIU, RunLoop::TimerBase::start is not thread safe, no? Internally it is due to GSource being used underneath. But even externally, it's protected through m_lock. There's other problems in this class related to this, for instance the timer shouldn't be scheduled if the Proxy object isn't active anymore, otherwise the object can be destructed on one thread while releaseUnusedBuffersTimerFired() runs on a different thread. But this shouldn't need to dispatch to any other thread.
(In reply to Zan Dobersek from comment #6) > There's other problems in this class related to this, for instance the timer > shouldn't be scheduled if the Proxy object isn't active anymore, otherwise > the object can be destructed on one thread while > releaseUnusedBuffersTimerFired() runs on a different thread. But this > shouldn't need to dispatch to any other thread. Will you handle making another bug report to make sure this doesn't get forgotten?
Created attachment 312177 [details] Patch
(In reply to Zan Dobersek from comment #6) > Comment on attachment 311826 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=311826&action=review > > >>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:151 > >>> +} > >> > >> Why does this have to be handled on the main thread? > > > > AFAIU, RunLoop::TimerBase::start is not thread safe, no? > > Internally it is due to GSource being used underneath. But even externally, > it's protected through m_lock. > It is correct. Since PlatformLayerProxy uses locks to protect runLoop, it is thread-safe. It is a side-topic, but we doesn't use locks in RunLoop::TimerBase::start while updating m_fireInterval and m_isRepeating. I think it is not thread safe at this point. https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/glib/RunLoopGLib.cpp#L204 Anyway, I modified to fire the timer in compositing thread, also I changed release timer to be attached to the compositing run loop. Now all operations about m_usedBuffers works on the compositing thread. > There's other problems in this class related to this, for instance the timer > shouldn't be scheduled if the Proxy object isn't active anymore, otherwise > the object can be destructed on one thread while > releaseUnusedBuffersTimerFired() runs on a different thread. But this > shouldn't need to dispatch to any other thread. I thought a bit more deeper about this problem. But when the proxy became invalidated, we removes and stops all timers and then destructs objects on the compositing thread, so I think it is quite okay at this point. But still needs improvements, regarding ::getAvailableBuffers, I made a patch to use the TexmapGL's bitmap texture pool, so it can be handled at different bug.
Comment on attachment 312177 [details] Patch Clearing flags on attachment: 312177 Committed r218170: <http://trac.webkit.org/changeset/218170>
All reviewed patches have been landed. Closing bug.