Bug 195453 – [GStreamer][v4l2] Synchronous video texture flushing support

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

The v4l2 video decoder currently requires that downstream users of the graphics resources complete any pending draw call and release resources before returning from the DRAIN query.

[Philippe Normand]

Created attachment 364003 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 364003 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:225
> +        g_signal_handlers_disconnect_by_func(GST_BIN_CAST(m_pipeline.get()), reinterpret_cast<gpointer>(playbinDeepElementAddedCallback), this);

This is not necessary because of the line above, that disconnects all functions with this as data.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1756
> +void MediaPlayerPrivateGStreamer::playbinDeepElementAddedCallback(GstBin*, GstBin* subBin, GstElement* element, MediaPlayerPrivateGStreamer* player)

Please, let's make a lambda of this.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1767
> +    GUniquePtr<gchar> elementName(gst_element_get_name(element));

char

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:850
> +    auto wait = m_videoDecoderIsVideo4Linux;

Nit: Lol, seriously? Do we really need to "auto" a "bool" ? :-ppppp

Anyway, the variable name should be shouldWait.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:863
> +        [wait](TextureMapperPlatformLayerProxy& proxy)

Values are captured with copy by default, you shouldn't need to copy in before. If should be enough if you just capture the variable or even by specifying [=] as the default capture. If you think you really want to pass down something different than the m_isVideoDecoderV4L2, I would do [shouldLock = m_isVideoDecoderV4L2]. Mind the ! cause I can be a bit lost with suggested name changes and bool meanings.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:872
> +            if (!wait) {
> +                LockHolder locker(proxy.lock());
>  
> -            if (proxy.isActive())
> -                proxy.dropCurrentBufferWhilePreservingTexture();
> +                if (proxy.isActive())
> +                    proxy.dropCurrentBufferWhilePreservingTexture(wait);
> +
> +            } else if (proxy.isActive())
> +                proxy.dropCurrentBufferWhilePreservingTexture(wait);

I would manually lock and unlock to avoid duplicating the function body. And another question, shouldn't it be if (wait) instead of if (!wait), either that or the variable name is misleading.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:286
> +    bool m_videoDecoderIsVideo4Linux { false };

m_isVideoDecoderVideo4Linux or m_isVideoDecoderV4L2

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:191
> +void TextureMapperPlatformLayerProxy::dropCurrentBufferWhilePreservingTexture(bool wait)

shouldWait

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:194
> +    if (!wait)
> +        ASSERT(m_lock.isHeld());

I have no strong feelings about this, but it looks like it would be interesting to flag the whole if as:

#ifndef ASSERT_DISABLED
if (wait)
    ASSERT...
#endif

Again, is the if correct? Shouldn't it be the other way around? Isn't it shouldLock a better name?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:213
> +                if (wait) {
> +                    LockHolder locker2(m_conditionLock);
> +                    m_bufferDropped = true;
> +                    m_condition.notifyAll();
> +                }

I would move this out of this if to a ScopeExit or makeScopeExit with a lambda, that way you avoid this here and below, having it centralized in only one place.

[Philippe Normand]

Comment on attachment 364003 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:866
> +            if (!wait) {
> +                LockHolder locker(proxy.lock());

the proxy should NOT be locked if we synchronously flush, otherwise there's a deadlock...

[Philippe Normand]

Comment on attachment 364003 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:286
>> +    bool m_videoDecoderIsVideo4Linux { false };
> 
> m_isVideoDecoderVideo4Linux or m_isVideoDecoderV4L2

Where does this recommendation to prefix boolean variables with "is" comes from?

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 364003 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364003&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:286
> >> +    bool m_videoDecoderIsVideo4Linux { false };
> > 
> > m_isVideoDecoderVideo4Linux or m_isVideoDecoderV4L2
> 
> Where does this recommendation to prefix boolean variables with "is" comes
> from?

https://webkit.org/code-style-guidelines/#names

Precede boolean values with words like “is” and “did”.

(In reply to Philippe Normand from comment #3)
> Comment on attachment 364003 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364003&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:866
> > +            if (!wait) {
> > +                LockHolder locker(proxy.lock());
> 
> the proxy should NOT be locked if we synchronously flush, otherwise there's
> a deadlock...

Not denying that, just the name is misleading, at least for me

[Philippe Normand]

Comment on attachment 364003 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:286
>>>> +    bool m_videoDecoderIsVideo4Linux { false };
>>> 
>>> m_isVideoDecoderVideo4Linux or m_isVideoDecoderV4L2
>> 
>> Where does this recommendation to prefix boolean variables with "is" comes from?
> 
> https://webkit.org/code-style-guidelines/#names
> 
> Precede boolean values with words like “is” and “did”.
> 
> (In reply to Philippe Normand from comment #3)

That's a strange guideline, I wonder if those were actually reviewed, I can't find where anyway :)

[Philippe Normand]

Created attachment 364246 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 364246 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:228
> +    m_bufferDropped = false;

Shouldn't this line be protected by the lock?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h:91
> +    Lock m_conditionLock;
> +    Condition m_condition;
> +    bool m_bufferDropped { false };

m_[wasB|b]ufferDroppedLock, m_[wasB|b]ufferDroppedCondition, m_wasBufferDropped.

[Philippe Normand]

Committed r242701: <https://trac.webkit.org/changeset/242701>

[Zan Dobersek]

This commit is causing tests on GTK and WPE to timeout/crash.
https://build.webkit.org/builders/WPE%20Linux%2064-bit%20Release%20%28Tests%29/builds/12603
https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/9953

[Radar WebKit Bug Importer]

<rdar://problem/48776515>

[Zan Dobersek]

(In reply to Zan Dobersek from comment #10)
> This commit is causing tests on GTK and WPE to timeout/crash.
> https://build.webkit.org/builders/WPE%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/12603
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/9953

Rolled out in r242725.
https://trac.webkit.org/changeset/242725/webkit

[Philippe Normand]

Created attachment 364367 [details]
Patch

[Philippe Normand]

Comment on attachment 364367 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:277
> +    if (m_isVideoDecoderVideo4Linux)
> +        flushCurrentBuffer();
> +#endif
>  #if USE(TEXTURE_MAPPER_GL) && USE(NICOSIA)
>      downcast<Nicosia::ContentLayerTextureMapperImpl>(m_nicosiaLayer->impl()).invalidateClient();

What broke the tests is an unconditional asynchronous flush followed by invalidation of the platform layer proxy, leading to a  deadlock. So now we'll do a synchronous flush only on v4l2 case.

[Xabier Rodríguez Calvar]

Comment on attachment 364367 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:868
> +            if (!shouldWait)
> +                proxy.lock().lock();

I think there's a problem here when I told you to remove the LockHolder and do a manual lock, that we materialized one of the problems of not using a LockHolder, that is that the lock might be held forever. It looks like know you're holding the lock and never releasing it.

Let's turn again to a LockHolder but let's do it like:
LockHolder locker(!shouldWait ? proxy.lock() : nullptr);

This way the code is still not repeated and we hold the lock when we should, releasing it when we should as well.

Am I analizing it wrong?

[Philippe Normand]

Comment on attachment 364367 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:868
>> +                proxy.lock().lock();
> 
> I think there's a problem here when I told you to remove the LockHolder and do a manual lock, that we materialized one of the problems of not using a LockHolder, that is that the lock might be held forever. It looks like know you're holding the lock and never releasing it.
> 
> Let's turn again to a LockHolder but let's do it like:
> LockHolder locker(!shouldWait ? proxy.lock() : nullptr);
> 
> This way the code is still not repeated and we hold the lock when we should, releasing it when we should as well.
> 
> Am I analizing it wrong?

error: operands to ?: have different types ‘WTF::Lock’ and ‘std::nullptr_t’

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #16)
> > Let's turn again to a LockHolder but let's do it like:
> > LockHolder locker(!shouldWait ? proxy.lock() : nullptr);
> 
> error: operands to ?: have different types ‘WTF::Lock’ and ‘std::nullptr_t’

    explicit Locker(T& lockable) : m_lockable(&lockable) { lock(); }
    explicit Locker(T* lockable) : m_lockable(lockable) { lock(); }

LockHolder locker(!shouldWait ? &proxy.lock() : nullptr);

It's a patter I have already seen somewhere in the code.

[Philippe Normand]

Created attachment 364374 [details]
Patch

[Philippe Normand]

Committed r242793: <https://trac.webkit.org/changeset/242793>