[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 #13 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 264681 [details]
> > Updated patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=264681&action=review
> > 
> > > 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:
> > 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?

We should actually check if m_unlocked is true, instead.

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

-- 
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/22f1ac45/attachment.html>