[webkit-reviews] review denied: [Bug 200914] [GStreamer] Add support to copy YUV video textures into platform textures : [Attachment 376858] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Chris Lord
<clord at igalia.com>'s request for review:
Bug 200914: [GStreamer] Add support to copy YUV video textures into platform
textures
https://bugs.webkit.org/show_bug.cgi?id=200914

Attachment 376858: Patch

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




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

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

Besides my review, I guess this should be checked by Miguel or Zan as well.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
219
> +    std::unique_ptr<TextureMapperPlatformLayerBuffer>
getPlatformLayerBuffer(bool treatAsRGB)

platformLayerBuffer and shouldBeTreatedAsRGB

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
229
> +		   Buffer::TextureVariant { Buffer::RGBTexture {
*static_cast<GLuint*>(m_videoFrame.data[0]) } },
> +		   m_size, m_flags, GraphicsContext3D::RGBA);

Nit: this can be just one line.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
236
> +	       int nPlanes = GST_VIDEO_INFO_N_PLANES(&m_videoFrame.info);

numberOfPlanes

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
239
> +	       std::array<int, 3> yuvPOffset;

yuvPlaneOffset?

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
255
> +		   // Default to bt601. This is the same behaviour as
GStreamer's glcolorconvert element

Period at the end.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
266
> +	       return makeUnique<Buffer>(
> +		   Buffer::TextureVariant {
> +		       Buffer::YUVTexture { nPlanes, planes, yuvPlane,
yuvPOffset, yuvToRgb } },
> +		   m_size, m_flags, GraphicsContext3D::RGBA);

Line lengths need to be reworked here. I think all could fit it one.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
998
> +    frameHolder->SyncWaitCPU();

Where is this defined? besides the name should be a bit more explanatory (I
guess adding more words will help)

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1030
> +    frameHolder->SyncWaitCPU();

ditto

>
Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:164
> +    WTF::switchOn(inputTexture.getTextureVariant(),
> +	   [&](const Buffer::RGBTexture&)
> +	   {

These three lines could be collapsed into one. Of course we'd need to reindent
the function.

>
Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:167
> +	   },
> +	   [&](const Buffer::YUVTexture& texture)

Collapse.

>
Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:226
> +	   [&](const Buffer::RGBTexture& texture)
> +	   {

lines

>
Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:236
> +	   [&](const Buffer::YUVTexture& texture)
> +	   {

lines

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:94
> +    const TextureVariant& getTextureVariant() { return m_variant; }

textureVariant