Bug 150807 – [GStreamer] Use RunLoop::Timer instead of GMainLoopSource in video sink

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We could use RunLoop::Timer here, since we always wait until the smaple is actually rendered we don't really need either a thread safe main loop source, nor cancelling if already requested and other things GMainLoopSource does.

[Carlos Garcia Campos]

Created attachment 264600 [details]
Patch

[Darin Adler]

Comment on attachment 264600 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Since we always wait until the smaple is actually rendered we

sample is misspelled here

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:78
> +        // Use a higher priority than WebCore timers (G_PRIORITY_HIGH_IDLE + 20).
> +        m_timer.setPriority(G_PRIORITY_HIGH_IDLE + 19);

This code is compiled for EFL, too. But this code is glib-specific. Is that OK for that port? Compile failed.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:140
> +struct _WebKitVideoSinkPrivate {

This peculiar use of a leading underscore followed by a capital letter trespasses on namespace that is reserved for compiler implementation, I believe.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 264600 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264600&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Since we always wait until the smaple is actually rendered we
> 
> sample is misspelled here

Oops. I'll fix it.

> > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:78
> > +        // Use a higher priority than WebCore timers (G_PRIORITY_HIGH_IDLE + 20).
> > +        m_timer.setPriority(G_PRIORITY_HIGH_IDLE + 19);
> 
> This code is compiled for EFL, too. But this code is glib-specific. Is that
> OK for that port? Compile failed.

Yes, I forgot to add platform ifdef there.

> > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:140
> > +struct _WebKitVideoSinkPrivate {
> 
> This peculiar use of a leading underscore followed by a capital letter
> trespasses on namespace that is reserved for compiler implementation, I
> believe.

This is the normal way to declare the structs in GObject. They are forward declared as typedef struct _Foo Foo; and then struct _Foo { }

[Zan Dobersek]

Comment on attachment 264600 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:90
> +        m_timer.stop();
> +        LockHolder locker(m_sampleMutex);

Any reason behind this ordering? The m_timer is being operated on under a lock in requestRender().

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:145
> +        if (currentCaps)
> +            gst_caps_unref(currentCaps);

Why is this being dereferenced in the constructor?

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 264600 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264600&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:90
> > +        m_timer.stop();
> > +        LockHolder locker(m_sampleMutex);
> 
> Any reason behind this ordering? The m_timer is being operated on under a
> lock in requestRender().

Tried to follow what previous code did. We don't need the lock to protect the timer I think

> > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:145
> > +        if (currentCaps)
> > +            gst_caps_unref(currentCaps);
> 
> Why is this being dereferenced in the constructor?

Oh, good catch, that was supposed to be in the destructor :-P

[Carlos Garcia Campos]

Created attachment 264681 [details]
Updated patch

[Zan Dobersek]

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:92
> +        m_timer.stop();
> +        LockHolder locker(m_sampleMutex);

I'm still stuck on this. in requestRender() m_timer operation is protected, here it's not.

Regarding previous behavior, timeoutSource was never cancelled after scheduling, except when WebKitVideoSinkPrivate was destroyed. This technically deviates from that.

[Carlos Garcia Campos]

(In reply to comment #7)
> 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:92
> > +        m_timer.stop();
> > +        LockHolder locker(m_sampleMutex);
> 
> I'm still stuck on this. in requestRender() m_timer operation is protected,
> here it's not.

It's not actually the timer what is protected, but the sample, we need to hold the lock for the condition wait. 

> Regarding previous behavior, timeoutSource was never cancelled after
> scheduling, except when WebKitVideoSinkPrivate was destroyed. This
> technically deviates from that.

That's right, but I think it was not desirable. We always wait until the timer is actually fired. That means that if unlockSampleMutex() was called for whatever reason before the timer is fired, the sample is destroyed and the condition was released, so we don't keep waiting anymore. But the timer will be fired anyway, but will return early because sample is NULL (and unlocked = true). Stopping the timer we just avoid the callback to be called just to return.

[Zan Dobersek]

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:92
>>> +        LockHolder locker(m_sampleMutex);
>> 
>> I'm still stuck on this. in requestRender() m_timer operation is protected, here it's not.
>> 
>> Regarding previous behavior, timeoutSource was never cancelled after scheduling, except when WebKitVideoSinkPrivate was destroyed. This technically deviates from that.
> 
> It's not actually the timer what is protected, but the sample, we need to hold the lock for the condition wait.

The timer is protected in requestRender(), and I don't see any reason why it wouldn't be protected here.

[Carlos Garcia Campos]

(In reply to comment #9)
> 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:92
> >>> +        LockHolder locker(m_sampleMutex);
> >> 
> >> I'm still stuck on this. in requestRender() m_timer operation is protected, here it's not.
> >> 
> >> Regarding previous behavior, timeoutSource was never cancelled after scheduling, except when WebKitVideoSinkPrivate was destroyed. This technically deviates from that.
> > 
> > It's not actually the timer what is protected, but the sample, we need to hold the lock for the condition wait.
> 
> The timer is protected in requestRender(), and I don't see any reason why it
> wouldn't be protected here.

It's protected only because it needs to happen between changing the sample and the condition wait. In stop() we can simply cancel the timer before waiting to acquire the lock. I can move it inside the protection if you think it's a problem

[Zan Dobersek]

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)

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Zan Dobersek]

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

[Carlos Garcia Campos]

(In reply to comment #14)
> 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
> >>>>          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. 

Yes, that's whay I think we can handle that case by simply returning early from requestRender() if the state changed by another thread.

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

Ok, I agree this is probably out of scope of this bug that is supposed to be only a refactoring.

[Carlos Garcia Campos]

Created attachment 264784 [details]
Updated patch

Ensure the lock is held while request rendering

[Zan Dobersek]

Comment on attachment 264784 [details]
Updated patch

Looks OK, I'll leave it to Phil to r+.

[Philippe Normand]

Comment on attachment 264784 [details]
Updated patch

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

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:78
> +#if PLATFORM(GTK)

USE(GLIB) && !PLATFORM(EFL) please :)

[Zan Dobersek]

Comment on attachment 264784 [details]
Updated patch

r=me if Phil doesn't object.

[Carlos Garcia Campos]

(In reply to comment #18)
> Comment on attachment 264784 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264784&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:78
> > +#if PLATFORM(GTK)
> 
> USE(GLIB) && !PLATFORM(EFL) please :)

I don't think this is correct in this particular case. It's true that setPriority  is defined that way in WTF, but here there's also the priority set that only makes sense in GTK+, because of the MainThreadSharedTimerGtk implementation and the priorities used by GTK+ itself. This behaviour is very GTK+ specific

[Carlos Garcia Campos]

Committed r192094: <http://trac.webkit.org/changeset/192094>