Bug 132869 - [Texmap][GStreamer] Add support to upload more color formats into the texture
Summary: [Texmap][GStreamer] Add support to upload more color formats into the texture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks: 200914 200922
  Show dependency treegraph
 
Reported: 2014-05-13 08:32 PDT by Víctor M. Jáquez L.
Modified: 2019-09-03 07:01 PDT (History)
9 users (show)

See Also:


Attachments
WIP (28.93 KB, patch)
2019-03-14 12:10 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP (29.94 KB, patch)
2019-05-09 09:37 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP (36.26 KB, patch)
2019-06-23 01:33 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (49.70 KB, patch)
2019-08-12 08:49 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (43.13 KB, patch)
2019-08-16 06:52 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (43.29 KB, patch)
2019-08-20 01:34 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (41.05 KB, patch)
2019-08-27 03:44 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (40.93 KB, patch)
2019-09-03 03:34 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (41.00 KB, patch)
2019-09-03 05:33 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Víctor M. Jáquez L. 2014-05-13 08:32:23 PDT
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.
Comment 1 Zan Dobersek 2019-03-14 12:10:17 PDT
Created attachment 364675 [details]
WIP

Work-in-progress, plenty of problems remain, but I420 format gets rendered about right.
Comment 2 Zan Dobersek 2019-05-09 09:37:15 PDT
Created attachment 369494 [details]
WIP
Comment 3 Nicolas Dufresne 2019-05-09 12:39:47 PDT
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.
Comment 4 Zan Dobersek 2019-05-10 01:29:12 PDT
(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.
Comment 5 Víctor M. Jáquez L. 2019-05-10 03:39:49 PDT
(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)
Comment 6 Nicolas Dufresne 2019-05-10 05:10:13 PDT
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.
Comment 7 Philippe Normand 2019-05-10 05:29:46 PDT
(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.
Comment 8 Nicolas Dufresne 2019-05-10 05:56:51 PDT
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 ?
Comment 9 Zan Dobersek 2019-05-10 10:20:31 PDT
(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.
Comment 10 Nicolas Dufresne 2019-05-10 10:34:04 PDT
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.
Comment 11 Zan Dobersek 2019-06-23 01:33:59 PDT
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.
Comment 12 Chris Lord 2019-08-12 08:49:10 PDT
Created attachment 376065 [details]
Patch
Comment 13 Build Bot 2019-08-12 08:52:32 PDT
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.
Comment 14 Chris Lord 2019-08-12 08:58:08 PDT
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).
Comment 15 Víctor M. Jáquez L. 2019-08-15 08:57:55 PDT
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
Comment 16 Chris Lord 2019-08-16 06:52:31 PDT
Created attachment 376496 [details]
Patch
Comment 17 Chris Lord 2019-08-16 06:57:01 PDT
(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.
Comment 18 Chris Lord 2019-08-20 01:34:35 PDT
Created attachment 376751 [details]
Patch
Comment 19 Chris Lord 2019-08-27 03:44:44 PDT
Created attachment 377333 [details]
Patch
Comment 20 Xabier Rodríguez Calvar 2019-08-30 04:02:23 PDT
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.
Comment 21 Zan Dobersek 2019-09-03 00:04:59 PDT
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.
Comment 22 Chris Lord 2019-09-03 03:34:27 PDT
Created attachment 377882 [details]
Patch
Comment 23 Chris Lord 2019-09-03 05:33:41 PDT
Created attachment 377890 [details]
Patch
Comment 24 WebKit Commit Bot 2019-09-03 07:00:45 PDT
Comment on attachment 377890 [details]
Patch

Clearing flags on attachment: 377890

Committed r249427: <https://trac.webkit.org/changeset/249427>
Comment 25 WebKit Commit Bot 2019-09-03 07:00:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Radar WebKit Bug Importer 2019-09-03 07:01:18 PDT
<rdar://problem/54974233>