Bug 172427 – [GStreamerGL] Release GstVideoFrame when there is a flush event from the pipeline

• Component: New Bugs
• Status: RESOLVED FIXED

[Gwang Yoon Hwang]

[GStreamerGL] Release GstVideoFrame when there is a flush event from the pipeline

[Gwang Yoon Hwang]

Created attachment 310807 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 311826 [details]
Patch

[Gwang Yoon Hwang]

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

[Zan Dobersek]

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?

[Gwang Yoon Hwang]

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?

[Zan Dobersek]

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.

[Michael Catanzaro]

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

[Gwang Yoon Hwang]

Created attachment 312177 [details]
Patch

[Gwang Yoon Hwang]

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

[WebKit Commit Bot]

Comment on attachment 312177 [details]
Patch

Clearing flags on attachment: 312177

Committed r218170: <http://trac.webkit.org/changeset/218170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.