[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 #12 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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? 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/6dd6720e/attachment-0001.html>