Bug 200914

Summary: [GStreamer] Add support to copy YUV video textures into platform textures
Product: WebKit Reporter: Chris Lord <clord>
Component: WPE WebKitAssignee: Chris Lord <clord>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, commit-queue, ggaren, magomez, sabouhallawa, simon.fraser, ysuzuki, zan
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201184
https://bugs.webkit.org/show_bug.cgi?id=201422
Bug Depends on: 132869    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Lord 2019-08-20 01:24:01 PDT
Bug 132869 adds shaders to render YUV video textures to avoid having to render to an extra off-screen surface. The patch on that bug only implements the playback portion and YUV videos become broken/non-working on accelerated and unaccelerated canvases. This bug tracks the work to enable YUV video textures in accelerated canvases (so WebGL and accelerated 2D canvas).
Comment 1 Chris Lord 2019-08-20 01:42:50 PDT
Created attachment 376753 [details]
Patch
Comment 2 Chris Lord 2019-08-21 03:17:45 PDT
Created attachment 376858 [details]
Patch
Comment 3 Xabier Rodríguez Calvar 2019-08-26 07:42:51 PDT
Comment on attachment 376858 [details]
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
Comment 4 Chris Lord 2019-08-27 03:49:52 PDT
Created attachment 377334 [details]
Patch
Comment 5 Xabier Rodríguez Calvar 2019-08-27 08:15:44 PDT
Comment on attachment 377334 [details]
Patch

Beautiful! Let's see what Miguel or Zan have to say about this cause they know more of the subject.
Comment 6 Miguel Gomez 2019-09-02 06:56:19 PDT
(In reply to Xabier Rodríguez Calvar from comment #5)
> Comment on attachment 377334 [details]
> Patch
> 
> Beautiful! Let's see what Miguel or Zan have to say about this cause they
> know more of the subject.

LGTM. Awesome work Chris!
Comment 7 Chris Lord 2019-09-03 03:36:01 PDT
Created attachment 377883 [details]
Patch
Comment 8 Chris Lord 2019-09-03 05:35:41 PDT
Created attachment 377891 [details]
Patch
Comment 9 WebKit Commit Bot 2019-09-03 07:15:04 PDT
Comment on attachment 377891 [details]
Patch

Clearing flags on attachment: 377891

Committed r249428: <https://trac.webkit.org/changeset/249428>
Comment 10 WebKit Commit Bot 2019-09-03 07:15:06 PDT
All reviewed patches have been landed.  Closing bug.