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.
Created attachment 364003 [details] Patch
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 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 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?
(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 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 :)
Created attachment 364246 [details] Patch
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.
Committed r242701: <https://trac.webkit.org/changeset/242701>
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
<rdar://problem/48776515>
(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
Created attachment 364367 [details] Patch
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 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 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’
(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.
Created attachment 364374 [details] Patch
Committed r242793: <https://trac.webkit.org/changeset/242793>