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

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




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

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

Good, though there are some bits missing, we are getting there!

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1107
> +
> +

You can probably remove one of these two lines

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1108
> +class GSTHolepunchClient : public
TextureMapperPlatformLayerBuffer::HolepunchClient {

GStreamerHolePunchClient

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1110
> +    GSTHolepunchClient(GstElement* videoSink) : m_videoSink(videoSink) { };

I would recommend receiving here GRefPtr<GstElement>&& and moving it into the
attribute.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1114
> +    void setVideoRectangle(IntRect rect) override
> +    {
> +	   setRectangleToVideoSink(m_videoSink.get(), rect);
> +    }

You can make this final and it is ok to put all in a single line.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1119
> +GstElement* MediaPlayerPrivateGStreamerBase::createHolepunchVideoSink()

createHolePunchVideoSink

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1128
> +void MediaPlayerPrivateGStreamerBase::pushNextHolepunchBuffer()

pushNextHolePunchBuffer

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1414
> +bool MediaPlayerPrivateGStreamerBase::shouldIgnoreIntrinsicSize()
> +{
> +#if USE(GSTREAMER_HOLEPUNCH)
> +    // When using holepunch, tell RenderVideo to ignore the intrinsicSize
and just use the size defined through
> +    // css. This is because the platform videoSink will deal with the resize
and positioning of the video frames
> +    // inside the assigned video rectangle.
> +    return true;
> +#endif
> +
> +    return false;
> +}
> +

Please, remove this because we can handle it in the .h

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:17
2
> +    bool shouldIgnoreIntrinsicSize() override;

Please, make this final, flag it with #GSTREAMER_HOLEPUNCH and return true. If
the hole punch compile switch is not on, then the default false of
MediaPlayerPrivate.h will be in place.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:18
0
> +    GstElement* createHolepunchVideoSink();
> +    void pushNextHolepunchBuffer();

As I said above Holepunch should be capitalized as HolePunch everywhere.

>
Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:82
> +#if USE(GSTREAMER_HOLEPUNCH)
> +    if (m_extraFlags & TextureMapperGL::ShouldNotBlend) {

My idea was to remove this compile switch from here and avoid involving
GStreamer in this non media class. If you write it as if (m_holePunchClient &&
m_extraFlags... you can remove it.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:68
> +#if USE(GSTREAMER_HOLEPUNCH)

Ditto.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:69
> +    class HolepunchClient {

HolePunchClient

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:74
> +    void setHolepunchClient(std::unique_ptr<HolepunchClient> client) {
m_holepunchClient = WTFMove(client); }

This should be std::unique_ptr<HolepunchClient>&& client to make the
std::unique_ptr in the stack of the caller all the way down moved to here.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:89
> +#if USE(GSTREAMER_HOLEPUNCH)

Ditto