<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GStreamer] Use RunLoop::Timer instead of GMainLoopSource in video sink"
href="https://bugs.webkit.org/show_bug.cgi?id=150807#c15">Comment # 15</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GStreamer] Use RunLoop::Timer instead of GMainLoopSource in video sink"
href="https://bugs.webkit.org/show_bug.cgi?id=150807">bug 150807</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=150807#c14">comment #14</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=264681&action=diff" name="attach_264681" title="Updated patch">attachment 264681</a> <a href="attachment.cgi?id=264681&action=edit" title="Updated patch">[details]</a></span>
> Updated patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=264681&action=review">https://bugs.webkit.org/attachment.cgi?id=264681&action=review</a>
>
> >>>> 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: <a href="http://trac.webkit.org/browser/trunk/Source/bmalloc/bmalloc/Heap.h#L57">http://trac.webkit.org/browser/trunk/Source/bmalloc/bmalloc/Heap.h#L57</a>)
> >>
> >> 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. </span >
Yes, that's whay I think we can handle that case by simply returning early from requestRender() if the state changed by another thread.
<span class="quote">> All in all, having the lock out of control during this function now
> definitely changes behavior. I'm not fine with that.</span >
Ok, I agree this is probably out of scope of this bug that is supposed to be only a refactoring.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>