Bug 132869 – [Texmap][GStreamer] Add support to upload more color formats into the texture

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Víctor M. Jáquez L.]

Right now the the GStreamer video sink supports BGRx and BGRA and the normal code path is to upload the buffer into a texture subimage. 

Nevertheless the BGRx support is broken when the padding value comes with 0x00 or random values (which are valid), the texture needs to be 0xff. Probably we would need a shader to fix this.

If so, it would make sense to support the upload of other color formats (with more planes), such as yuv or nv12.

[Zan Dobersek]

Created attachment 364675 [details]
WIP

Work-in-progress, plenty of problems remain, but I420 format gets rendered about right.

[Zan Dobersek]

Created attachment 369494 [details]
WIP

[Nicolas Dufresne]

Any reason to aim for such a limited implementation ? libgstgl is a stable API since 1.14, so it it's safe to start using it. What Cervo is doing is to use "glsinkbin sink=appsink", then you just need to listen for the context sharing to allow GStreamer to create a shared context from the WebKit GL Context. Additonally, you could announce support for gl_sync_meta through a pad probe implementing allocation query, and then wait on the fence in your render function. That would help throughput and enable support for Android CODEC at the same time. From this, you'll gain from much more then just I420, since you'll also gain support for DMABuf importation, VAAPI Surface support etc.

[Zan Dobersek]

(In reply to Nicolas Dufresne from comment #3)
> Any reason to aim for such a limited implementation ?

No, other YUV formats will be added in later.

> libgstgl is a stable
> API since 1.14, so it it's safe to start using it. What Cervo is doing is to
> use "glsinkbin sink=appsink", then you just need to listen for the context
> sharing to allow GStreamer to create a shared context from the WebKit GL
> Context.

That's what we do already.

> Additonally, you could announce support for gl_sync_meta through a
> pad probe implementing allocation query, and then wait on the fence in your
> render function.

This is the next requirement, as the rendering is currently overstepping the management of decoded data. But fences aren't generally available on all target systems, so this will have to fall back to the basic glFinish().

> That would help throughput and enable support for Android
> CODEC at the same time. From this, you'll gain from much more then just
> I420, since you'll also gain support for DMABuf importation, VAAPI Surface
> support etc.

Idea is to handle I420 (and other YUV formats eventually, and other formats as required in the future) manually in order to avoid the additional color conversion step done in glcolorconvert that would be needed if only RGB were handled.

[Víctor M. Jáquez L.]

(In reply to Nicolas Dufresne from comment #3)
> Any reason to aim for such a limited implementation ? libgstgl is a stable
> API since 1.14, so it it's safe to start using it. What Cervo is doing is to
> use "glsinkbin sink=appsink", 

I tried this, but glsinkbin uses glcolorbalance which src pad pushes RGBA textures, while WebKit only renders efficiently BGRx or BGRA (little endian) -- I don't know the reasons behind it, perhaps Zan can shed some light :)

Thus, so far, glsinkbin is not an option right now.

> then you just need to listen for the context
> sharing to allow GStreamer to create a shared context from the WebKit GL
> Context. Additonally, you could announce support for gl_sync_meta through a
> pad probe implementing allocation query, and then wait on the fence in your
> render function. That would help throughput and enable support for Android
> CODEC at the same time. From this, you'll gain from much more then just
> I420, since you'll also gain support for DMABuf importation, VAAPI Surface
> support etc.

Regarding the gl fence and sync, I worked on it a bit: https://bugs.webkit.org/show_bug.cgi?id=196200

I still have a patch for it around, but I realized that WebKit (at least my setup) uses OpenGL profile 3.0 while the sync and fence appeared in 3.2.

Another related patch is https://bugs.webkit.org/show_bug.cgi?id=196966 (which is still a work-in-progress)

[Nicolas Dufresne]

GStreamer should fallback for the fences based on support and if support is advertise in the allocation query.

You can replace glsinkbin with something smaller too:

  glupload ! glcolorconvert !

These two will take care of the GL upload and color conversion. glcolorconvert have support for BGRx and BGRA target, even though, as you said, pretty much everything else hardcode RGBA right now.

It seems better to try to get that to work then to replace with a partial implementation with limited color formats and no support for colorimetry.

[Philippe Normand]

(In reply to Nicolas Dufresne from comment #6)
> GStreamer should fallback for the fences based on support and if support is
> advertise in the allocation query.
> 
> You can replace glsinkbin with something smaller too:
> 
>   glupload ! glcolorconvert !
> 

We already do that.

[Nicolas Dufresne]

Then I still don't see the point why it make sense to have multi-planar support here. Why don't you just fixate the caps to GL/BGRA, what does it gives you to add planar format here and to replicated all the convertion shaders ?

[Zan Dobersek]

(In reply to Nicolas Dufresne from comment #8)
> Then I still don't see the point why it make sense to have multi-planar
> support here. Why don't you just fixate the caps to GL/BGRA, what does it
> gives you to add planar format here and to replicated all the convertion
> shaders ?

Because we want to avoid the convertion shaders. The extra color convert step can be prohibitively expensive.

[Nicolas Dufresne]

That I agree, notably on IMX.6. That being said, you should at the very least read and support the color range, otherwise the result could endup being extremely bad.

[Zan Dobersek]

Created attachment 372694 [details]
WIP

Avoids flickering issues by enforcing a CPU-bound wait on the GstGLSyncMeta object after mapping. This approach should be revisited.

[Chris Lord]

Created attachment 376065 [details]
Patch

[EWS Watchlist]

Attachment 376065 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:119:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:124:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:129:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Lord]

I've updated and rebased this patch and added support for all of the formats that glupload supports, except for the interleaved YUY2/UYVY formats (which can continue to be handled by glcolorconvert).

Things I'm uncertain about:

- Does endianness affect the order of yuv in uploaded textures? I assume not or they'd have different format names (like RGB vs. BGR), but it'd be good to know.

- I had to reintroduce the ImxVPU flag as it handles YUV differently, but I have no way of testing whether this current patch works correctly.

- The code checker was flagging up the asserts I added in TextureMapperPlatformLayerBuffer because I explicitly check for zero, but it makes sense in the context of a compound boolean statement. Would we really want to replace this with a '!' ?

Note that I don't read the colorimetry data, but instead mirror the behaviour of glcolorconvert (https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/blob/master/gst-libs/gst/gl/gstglcolorconvert.c#L74) - I took a crack at reading the colorimetry data to do the conversion, but GStreamer has no API to obtain a simple matrix to do the entire colour conversion and doesn't expose the exact formula or numbers it uses to do the colour gamma correction, so I don't think it's currently possible to do this in a shader only with data provided by the GStreamer API (and if we have to have a big matrix of numbers, I think doing what glcolorconvert does rather than something different makes sense).

[Víctor M. Jáquez L.]

Comment on attachment 376065 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:171
> +                gst_gl_sync_meta_wait_cpu(meta, context);

IMO this shall be delayed until almost painting the whole GL scene. Perhaps adding a virtual method in TextureMapperPlatformLayerBuffer::UnmanagedBufferDataHolder

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:404
> +    if (!g_strcmp0(contextType, "gst.gl.local_context")) {

I don't see any need to create and control of the internal pipeline GL context. 

We can just let the gstreamer's pipeline handle it and query it if we need id (and we will need it). There are two ways:

1. Query 'context' property in glupload (only available at playing)
2. get it from the GstGLBaseMemory https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/merge_requests/156/diffs?commit_id=b5aa304f201d0805de2b33ab97d293654baa12cd#e60aa49aa8b41c83d9ee803694fed70f9e47698e_161_155

[Chris Lord]

Created attachment 376496 [details]
Patch

[Chris Lord]

(In reply to Víctor M. Jáquez L. from comment #15)
> Comment on attachment 376065 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376065&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:171
> > +                gst_gl_sync_meta_wait_cpu(meta, context);
> 
> IMO this shall be delayed until almost painting the whole GL scene. Perhaps
> adding a virtual method in
> TextureMapperPlatformLayerBuffer::UnmanagedBufferDataHolder
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:404
> > +    if (!g_strcmp0(contextType, "gst.gl.local_context")) {
> 
> I don't see any need to create and control of the internal pipeline GL
> context. 
> 
> We can just let the gstreamer's pipeline handle it and query it if we need
> id (and we will need it). There are two ways:
> 
> 1. Query 'context' property in glupload (only available at playing)
> 2. get it from the GstGLBaseMemory
> https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/merge_requests/156/
> diffs?commit_id=b5aa304f201d0805de2b33ab97d293654baa12cd#e60aa49aa8b41c83d9ee
> 803694fed70f9e47698e_161_155

Addressed these comments (hopefully), used the second method of getting the internal GL context.

[Chris Lord]

Created attachment 376751 [details]
Patch

[Chris Lord]

Created attachment 377333 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 377333 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:191
> +    bool hasMappedTextures() const { return m_hasMappedTextures; }

doesHaveMappedTextures, same for the attribute.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:571
> +    std::vector<std::pair<GLuint, GLuint> > texturesAndSamplers = {
> +        { textures[0], program->samplerYLocation() },
> +        { textures[1], program->samplerULocation() },
> +        { textures[2], program->samplerVLocation() }
> +    };

5 lines too much, 2 likes ok, I guess 1 line two long

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:620
> +    std::vector<std::pair<GLuint, GLuint> > texturesAndSamplers = {
> +        { textures[0], program->samplerYLocation() },
> +        { textures[1], program->samplerULocation() }
> +    };

2 lines?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.h:34
> +#include <vector>

I'd rather use wtf/Vector instead.

[Zan Dobersek]

Comment on attachment 377333 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:620
>> +    };
> 
> 2 lines?

I think the current layout here and above keeps things much more readable.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:804
> +        for (unsigned i = 0; i < texturesAndSamplers.size(); ++i) {
> +            auto& textureAndSampler = texturesAndSamplers[i];

Here you can use the range-based for-loop:

    for (auto& textreAndSampler : texturesAndSamplers) { }

>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.h:34
>> +#include <vector>
> 
> I'd rather use wtf/Vector instead.

Yes, please use Vector.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:60
> +TextureMapperPlatformLayerBuffer::~TextureMapperPlatformLayerBuffer()
> +{
> +}

Nit: TextureMapperPlatformLayerBuffer::~TextureMapperPlatformLayerBuffer() = default;

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:54
> +        int numberOfPlanes;
> +        std::array<GLuint, 3> planes;
> +        std::array<int, 3> yuvPlane;
> +        std::array<int, 3> yuvPlaneOffset;
> +        std::array<GLfloat, 9> yuvToRgbMatrix;

Can we keep numberOfPlanes, yuvPlane and yuvPlaneOffset values as unsigned integers here? These are gathered from GstVideoFormatInfo where they're all unsigned.

[Chris Lord]

Created attachment 377882 [details]
Patch

[Chris Lord]

Created attachment 377890 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377890 [details]
Patch

Clearing flags on attachment: 377890

Committed r249427: <https://trac.webkit.org/changeset/249427>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54974233>