[libcamera-devel] [PATCH 7/7] qcam: viewfinder_gl: Add support for RGB formats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Nov 4 11:19:15 CET 2020
Hi Kieran,
On Wed, Nov 04, 2020 at 09:49:43AM +0000, Kieran Bingham wrote:
> On 04/11/2020 09:32, Andrey Konovalov wrote:
> > On 04.11.2020 05:09, Laurent Pinchart wrote:
> >> On Tue, Nov 03, 2020 at 11:00:28PM +0000, Kieran Bingham wrote:
> >>> On 03/11/2020 15:50, Laurent Pinchart wrote:
> >>>> Add support for 24-bit and 32-bit RGB formats. The fragment samples the
> >>>> texture and reorders the components, using a pattern set through the
> >>>> RGB_PATTERN macro.
> >>>>
> >>>> An alternative to manual reordering in the shader would be to set the
> >>>> texture swizzling mask. This is however not available in OpenGL ES
> >>>> before version 3.0, which we don't mandate at the moment.
> >>>>
> >>>> Only the BGR888 and RGB888 formats have been tested, with the vimc
> >>>> pipeline handler.
> >>>>
> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>>> ---
> >>>> src/qcam/assets/shader/RGB.frag | 22 +++++++++
> >>>> src/qcam/assets/shader/shaders.qrc | 1 +
> >>>> src/qcam/viewfinder_gl.cpp | 71 ++++++++++++++++++++++++++++--
> >>>> 3 files changed, 91 insertions(+), 3 deletions(-)
> >>>> create mode 100644 src/qcam/assets/shader/RGB.frag
> >>>>
> >>>> diff --git a/src/qcam/assets/shader/RGB.frag
> >>>> b/src/qcam/assets/shader/RGB.frag
> >>>> new file mode 100644
> >>>> index 000000000000..4c374ac98095
> >>>> --- /dev/null
> >>>> +++ b/src/qcam/assets/shader/RGB.frag
> >>>> @@ -0,0 +1,22 @@
> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>> +/*
> >>>> + * Copyright (C) 2020, Laurent Pinchart
> >>>> + *
> >>>> + * RGB.frag - Fragment shader code for RGB formats
> >>>> + */
> >>>> +
> >>>> +#ifdef GL_ES
> >>>> +precision mediump float;
> >>>> +#endif
> >>>> +
> >>>> +varying vec2 textureOut;
> >>>> +uniform sampler2D tex_y;
> >>>> +
> >>>> +void main(void)
> >>>> +{
> >>>> + vec3 rgb;
> >>>> +
> >>>> + rgb = texture2D(tex_y, textureOut).RGB_PATTERN;
> >>>> +
> >>>> + gl_FragColor = vec4(rgb, 1.0);
> >>>> +}
> >>>> diff --git a/src/qcam/assets/shader/shaders.qrc
> >>>> b/src/qcam/assets/shader/shaders.qrc
> >>>> index 863109146281..8a8f9de1a5fa 100644
> >>>> --- a/src/qcam/assets/shader/shaders.qrc
> >>>> +++ b/src/qcam/assets/shader/shaders.qrc
> >>>> @@ -1,6 +1,7 @@
> >>>> <!-- SPDX-License-Identifier: LGPL-2.1-or-later -->
> >>>> <!DOCTYPE RCC><RCC version="1.0">
> >>>> <qresource>
> >>>> + <file>RGB.frag</file>
> >>>> <file>YUV_2_planes.frag</file>
> >>>> <file>YUV_3_planes.frag</file>
> >>>> <file>YUV_packed.frag</file>
> >>>> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> >>>> index cbc1365500f5..5d9b442e7985 100644
> >>>> --- a/src/qcam/viewfinder_gl.cpp
> >>>> +++ b/src/qcam/viewfinder_gl.cpp
> >>>> @@ -14,21 +14,28 @@
> >>>> #include <libcamera/formats.h>
> >>>> static const QList<libcamera::PixelFormat> supportedFormats{
> >>>> - /* Packed (single plane) */
> >>>> + /* YUV - packed (single plane) */
> >>>> libcamera::formats::UYVY,
> >>>> libcamera::formats::VYUY,
> >>>> libcamera::formats::YUYV,
> >>>> libcamera::formats::YVYU,
> >>>> - /* Semi planar (two planes) */
> >>>> + /* YUV - semi planar (two planes) */
> >>>> libcamera::formats::NV12,
> >>>> libcamera::formats::NV21,
> >>>> libcamera::formats::NV16,
> >>>> libcamera::formats::NV61,
> >>>> libcamera::formats::NV24,
> >>>> libcamera::formats::NV42,
> >>>> - /* Fully planar (three planes) */
> >>>> + /* YUV - fully planar (three planes) */
> >>>> libcamera::formats::YUV420,
> >>>> libcamera::formats::YVU420,
> >>>> + /* RGB */
> >>>> + libcamera::formats::ABGR8888,
> >>>> + libcamera::formats::ARGB8888,
> >>>> + libcamera::formats::BGRA8888,
> >>>> + libcamera::formats::RGBA8888,
> >>>> + libcamera::formats::BGR888,
> >>>> + libcamera::formats::RGB888,
> >>>> };
> >>>> ViewFinderGL::ViewFinderGL(QWidget *parent)
> >>>> @@ -172,6 +179,30 @@ bool ViewFinderGL::selectFormat(const
> >>>> libcamera::PixelFormat &format)
> >>>> fragmentShaderDefines_.append("#define YUV_PATTERN_YVYU");
> >>>> fragmentShaderFile_ = ":YUV_packed.frag";
> >>>> break;
> >>>> + case libcamera::formats::ABGR8888:
> >>>> + fragmentShaderDefines_.append("#define RGB_PATTERN rgb");
> >>>
> >>> pattern rgb, the same as BGR888, so I presume the A is stripped by not
> >>> being used as a channel or some other magic elsewhere?
> >>>
> >>>> + fragmentShaderFile_ = ":RGB.frag";
> >>>> + break;
> >>>> + case libcamera::formats::ARGB8888:
> >>>> + fragmentShaderDefines_.append("#define RGB_PATTERN bgr");
> >>>> + fragmentShaderFile_ = ":RGB.frag";
> >>>> + break;
> >>>> + case libcamera::formats::BGRA8888:
> >>>> + fragmentShaderDefines_.append("#define RGB_PATTERN gba");
> >>>
> >>> Yet here it's specified.... at the end
> >>>
> >>>> + fragmentShaderFile_ = ":RGB.frag";
> >>>> + break;
> >>>> + case libcamera::formats::RGBA8888:
> >>>> + fragmentShaderDefines_.append("#define RGB_PATTERN abg");
> >>>
> >>> but here at the beginning?
> >>>
> >>> What is the definition of the RGB_PATTERNs?
> >>> They don't seem to have a pattern that makes sense to me yet.
> >>
> >> The shader language accesses vector components by name. The texture2D()
> >> function, which samples a texture, returns a vec4, a 4-components
> >> floating point vector. Multiple component names are supported:
> >>
> >> - {x, y, z, w}
> >> - {r, g, b, a}
> >> - {s, t, p, q}
> >>
>
> I wish they also had a set of component names which were simply 'linear'
> (e, f, g, h} for instance
>
> (also because those are unused above and 'almost' linear in a qwerty
> keyboard ;D)
>
> Or using an index value 1,2,3,4 but I can see how using numeric indexes
> here could cause difficulty
>
> >> .x, .r and .s are all equivalent to [0] in C syntax, and similarly for
> >> other names. The reason different names are defined and alias each other
> >> is that vectors can store different types of data, and standardizing on
> >> {r, g, b, a} would lead to weird-looking code when the vector stores a
> >> 3D coordinnate for instance ({s, t, p, q} are used as a convention for
> >> texture coordinates).
> >>
> >> The indexing syntax also supports multiple components to be selected:
> >>
> >> vec4 colour(1.0, 2.0, 3.0, 4.0);
> >> vec2 value;
> >>
> >> value = colour.rb;
> >>
> >> // value now contains (1.0, 3.0)
> >>
> >> The RGB fragment shader code simply contains
> >>
> >> vec3 rgb;
> >>
> >> rgb = texture2D(tex_y, textureOut).RGB_PATTERN;
> >>
> >> The pattern thus defines the components mapping.
> >>
> >> One additional piece of information that is required is how OpenGL
> >> expects texture data to be layed out in memory. The texture is created
> >> with GL_RGBA and GL_UNSIGNED_BYTE, which means that the R, G, B and A
> >> components are layed down in that order, in one byte each. The RGBA8888
> >> format, has the opposite convention, it stores RGBA in that order in a
> >> 32-bit word that is then stored in memory in little endian format
> >
> > - Ah... I've missed that little endian part of the puzzle, and have got
> > confused too.
> >
> > Thanks a lot for the detailed explanation!
>
> Agreed. Should we add all that to the commit message?
> It's a really useful explanation - it seems valuable to keep it.
I knew you would say that :-)
I'm not sure we really need to document the OpenGL API and the shader
language in the commit message though. I've expanded the first paragraph
to the following:
"Add support for 24-bit and 32-bit RGB formats. The fragment samples the
texture and reorders the components, using a pattern set through the
RGB_PATTERN macro. The pattern stores the shader vec4 element indices
(named {r, g, b, a} by convention, for elements 0 to 3) to be extracted
from the texture samples, when interpreted by OpenGL as RGBA.
Note that, as textures are created with GL_UNSIGNED_BYTE, the RGBA
order corresponds to bytes in memory, while the libcamera formats are
named based on the components order in a 32-bit word stored in memory in
little endian format."
I think this provides enough context, please please let me know if you
disagree.
> > Reviewed-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>
> And this too now ;-)
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >> , so
> >> bytes contains A, B, G and R in that order.
> >>
> >> So, taking the RGBA8888 example, we have ABGR in memory, which is read
> >> in a vec4. We want to extract RGB (in that order) in the shader, so we
> >> need components [3], [2] and [1] of the vector, which are expressed as
> >> .abg.
> >>
> >> I hope this makes it clearer.
> >>
> >>>> + fragmentShaderFile_ = ":RGB.frag";
> >>>> + break;
> >>>> + case libcamera::formats::BGR888:
> >>>> + fragmentShaderDefines_.append("#define RGB_PATTERN rgb");
> >>>> + fragmentShaderFile_ = ":RGB.frag";
> >>>> + break;
> >>>> + case libcamera::formats::RGB888:
> >>>> + fragmentShaderDefines_.append("#define RGB_PATTERN bgr");
> >>>> + fragmentShaderFile_ = ":RGB.frag";
> >>>> + break;
> >>>> default:
> >>>> ret = false;
> >>>> qWarning() << "[ViewFinderGL]:"
> >>>> @@ -481,6 +512,40 @@ void ViewFinderGL::doRender()
> >>>> 1.0f / (size_.width() / 2 - 1));
> >>>> break;
> >>>> + case libcamera::formats::ABGR8888:
> >>>> + case libcamera::formats::ARGB8888:
> >>>> + case libcamera::formats::BGRA8888:
> >>>> + case libcamera::formats::RGBA8888:
> >>>> + glActiveTexture(GL_TEXTURE0);
> >>>> + configureTexture(*textures_[0]);
> >>>> + glTexImage2D(GL_TEXTURE_2D,
> >>>> + 0,
> >>>> + GL_RGBA,
> >>>> + size_.width(),
> >>>> + size_.height(),
> >>>> + 0,
> >>>> + GL_RGBA,
> >>>> + GL_UNSIGNED_BYTE,
> >>>> + data_);
> >>>> + shaderProgram_.setUniformValue(textureUniformY_, 0);
> >>>> + break;
> >>>> +
> >>>> + case libcamera::formats::BGR888:
> >>>> + case libcamera::formats::RGB888:
> >>>> + glActiveTexture(GL_TEXTURE0);
> >>>> + configureTexture(*textures_[0]);
> >>>> + glTexImage2D(GL_TEXTURE_2D,
> >>>> + 0,
> >>>> + GL_RGB,
> >>>> + size_.width(),
> >>>> + size_.height(),
> >>>> + 0,
> >>>> + GL_RGB,
> >>>> + GL_UNSIGNED_BYTE,
> >>>> + data_);
> >>>> + shaderProgram_.setUniformValue(textureUniformY_, 0);
> >>>> + break;
> >>>> +
> >>>> default:
> >>>> break;
> >>>> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list