[webkit-reviews] review denied: [Bug 193715] [WPE] Implement GStreamer based holepunch : [Attachment 359874] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Miguel Gomez
<magomez at igalia.com>'s request for review:
Bug 193715: [WPE] Implement GStreamer based holepunch
https://bugs.webkit.org/show_bug.cgi?id=193715

Attachment 359874: Patch

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




--- Comment #4 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 359874
  --> https://bugs.webkit.org/attachment.cgi?id=359874
Patch

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

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
498
> +    return FloatSize(HOLEPUNCH_DEFAULT_FRAME_WIDTH,
HOLEPUNCH_DEFAULT_FRAME_HEIGHT);

Considering that this is the only place where you use the default width and
height, you could have a FloatSize s_holePunchDefaultSize, you could even make
it static here and remove the defines from above.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1119
> +	       LockHolder holder(proxy.lock());
> +	       std::unique_ptr<TextureMapperPlatformLayerBuffer> layerBuffer =
std::make_unique<TextureMapperPlatformLayerBuffer>(0, m_size,
TextureMapperGL::ShouldNotBlend, GL_DONT_CARE);
> +	       layerBuffer->setVideoSink(videoSink);
> +	       proxy.pushNextBuffer(WTFMove(layerBuffer));

I see this lines repeated a lot. I think you could just create a new method in
the proxy called pushNextEmptyBufferWithVideoSink (name to be improved) or if
you thing the arch does not allow it, create a helper function.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:507
> +void TextureMapperGL::drawSolidColor(const FloatRect& rect, const
TransformationMatrix& matrix, const Color& color, bool blend)

This parameter should be called shouldBlend (everywhere you use it)

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:42
> +static void setRectangleToVideoSink(GstElement* videoSink, IntRect rect)
> +{
> +    // Here goes the platform-dependant code to set to the videoSink the
size
> +    // and position of the video rendering window. Mark them unused as
default.
> +    UNUSED_PARAM(videoSink);
> +    UNUSED_PARAM(rect);

And here what is more concerning to me: this is introducing a new upstream
video drawing path to be used downstream and that is completely untested.
Believe me that I understand the need of reducing deltas ;) but this shouldn't
go untested, to say the least.