<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#c13">Comment # 13</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#c12">comment #12</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=150807#c11">comment #11</a>)
> > 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
> > > static GstFlowReturn webkitVideoSinkRender(GstBaseSink* baseSink, GstBuffer* buffer)
> > > {
> > > WebKitVideoSink* sink = WEBKIT_VIDEO_SINK(baseSink);
> > > WebKitVideoSinkPrivate* priv = sink->priv;
> > > -
> > > - WTF::GMutexLocker<GMutex> lock(priv->sampleMutex);
> > > -
> > > - if (priv->unlocked)
> > > + if (priv->scheduler.isUnlocked())
> > > 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?</span >
We should actually check if m_unlocked is true, instead.
<span class="quote">> 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.</span ></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>