[webkit-reviews] review denied: [Bug 195453] [GStreamer][v4l2] Synchronous video texture flushing support : [Attachment 364003] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 195453: [GStreamer][v4l2] Synchronous video texture flushing support
https://bugs.webkit.org/show_bug.cgi?id=195453

Attachment 364003: Patch

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




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 364003
  --> https://bugs.webkit.org/attachment.cgi?id=364003
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:28
6
> +    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.