[webkit-reviews] review denied: [Bug 193715] [WPE] Implement GStreamer based holepunch : [Attachment 360459] 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 360459: Patch

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




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

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

We're getting there...

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:98
> +#if USE(GSTREAMER_HOLEPUNCH)
> +    if (m_extraFlags & TextureMapperGL::ShouldNotBlend) {
> +	   ASSERT(m_videoSink && !m_texture);
> +	   texmapGL.drawSolidColor(targetRect, modelViewMatrix, Color(0, 0, 0,
0), false);
> +	   setRectangleToVideoSink(m_videoSink.get(),
enclosingIntRect(modelViewMatrix.mapRect(targetRect)));
> +	   return;
> +    }
> +#endif

This is a bit of layer violation, we should probably create a client interface
for this class that shoudl be implemented by the player and do this job there.

> Source/WebCore/rendering/RenderVideo.cpp:166
> +#if PLATFORM(WPE) && USE(GSTREAMER_HOLEPUNCH)
> +    // When using holepunch, skip all the intrinsic size calculations and
just report the <video>
> +    // element size. The external application or the custom sink will be in
charge of scaling the video
> +    // frames appropriately.
> +    return snappedIntRect(contentBoxRect());
> +#endif

I also feel we should abstract this behind the player.

> Source/cmake/OptionsWPE.cmake:50
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_HOLEPUNCH "Whether to enable GStreamer
holepunch" PUBLIC OFF)

This is a functionality that can't work on its own, it needs something else
(downstream code) to be complete, so I don't think it's a good idea to make
this PUBLIC. It should be definitely PRIVATE, IMHO.