[Webkit-unassigned] [Bug 150807] [GStreamer] Use RunLoop::Timer instead of GMainLoopSource in video sink

[bugzilla-daemon at webkit.org]

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

--- Comment #14 from Zan Dobersek <zan at falconsigh.net> ---
Comment on attachment 264681
  --> https://bugs.webkit.org/attachment.cgi?id=264681
Updated patch

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

>>>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:180
>>>>          return GST_FLOW_OK;
>>> 
>>> This is another problem.
>>> 
>>> Before, the sampleMutex was locked throughout this function. Now it's only locked when querying the unlocked status of the scheduler, and when requesting the render callback. So this now requires two locks per webkitVideoSinkRender() invocation, and it also allows for the scheduler state to change during these locks.
>>> 
>>> I would expose a `LockHolder lock();` on the VideoRenderRequestScheduler interface, and the have the public methods accept LockHolder& as their first parameter. That way the caller is in complete control of the scope of the lock.
>>> (Inspired by bmalloc: http://trac.webkit.org/browser/trunk/Source/bmalloc/bmalloc/Heap.h#L57)
>> 
>> hmm, the mutex is supposed to protect the sample and unlocked, and non of those change between the isUnlocked and requestRender in that function. However, you are right that the scheduler state could change. Wouldn't it be better to takes that into account instead and return early from requestRender if sample is NULL? In the case stop() is called before requestRender(), if we have the lock, the timer will start and right after we wait, the scheduler will stop. If we don't have the lock, we don't even start the timer.
> 
> We should actually check if m_unlocked is true, instead.

The sample and unlock don't change during this function, or rather they're not changed directly by us. It's still entirely possible for some other thread to acquire the lock during this function call. 

All in all, having the lock out of control during this function now definitely changes behavior. I'm not fine with that.

-- 
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/20151104/7585ee3a/attachment.html>