Bug 195453 - [GStreamer][v4l2] Synchronous video texture flushing support
Summary: [GStreamer][v4l2] Synchronous video texture flushing support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 01:37 PST by Philippe Normand
Modified: 2019-03-12 07:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.70 KB, patch)
2019-03-08 03:24 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (11.27 KB, patch)
2019-03-11 05:44 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (11.52 KB, patch)
2019-03-12 02:04 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (11.52 KB, patch)
2019-03-12 03:17 PDT, Philippe Normand
calvaris: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2019-03-08 01:37:51 PST
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.
Comment 1 Philippe Normand 2019-03-08 03:24:37 PST
Created attachment 364003 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2019-03-11 03:55:43 PDT
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.
Comment 3 Philippe Normand 2019-03-11 04:06:00 PDT
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...
Comment 4 Philippe Normand 2019-03-11 04:55:43 PDT
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?
Comment 5 Xabier Rodríguez Calvar 2019-03-11 05:10:12 PDT
(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
Comment 6 Philippe Normand 2019-03-11 05:32:46 PDT
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 :)
Comment 7 Philippe Normand 2019-03-11 05:44:52 PDT
Created attachment 364246 [details]
Patch
Comment 8 Xabier Rodríguez Calvar 2019-03-11 07:07:21 PDT
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.
Comment 9 Philippe Normand 2019-03-11 07:41:57 PDT
Committed r242701: <https://trac.webkit.org/changeset/242701>
Comment 11 Radar WebKit Bug Importer 2019-03-11 12:13:24 PDT
<rdar://problem/48776515>
Comment 12 Zan Dobersek 2019-03-11 12:22:48 PDT
(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
Comment 13 Philippe Normand 2019-03-12 02:04:51 PDT
Created attachment 364367 [details]
Patch
Comment 14 Philippe Normand 2019-03-12 02:07:21 PDT
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.
Comment 15 Xabier Rodríguez Calvar 2019-03-12 02:38:29 PDT
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?
Comment 16 Philippe Normand 2019-03-12 02:52:31 PDT
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’
Comment 17 Xabier Rodríguez Calvar 2019-03-12 03:09:27 PDT
(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.
Comment 18 Philippe Normand 2019-03-12 03:17:04 PDT
Created attachment 364374 [details]
Patch
Comment 19 Philippe Normand 2019-03-12 07:17:21 PDT
Committed r242793: <https://trac.webkit.org/changeset/242793>