[libcamera-devel,3/3] qcam: viewfinder_qt: Support X RGB variants

16550 diff mbox series

Support the X variants of the RGB pixel formats
along side the equivalent Alpha component based versions.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/qcam/viewfinder_qt.cpp | 2 ++
 1 file changed, 2 insertions(+)

Hi Kieran,

Thank you for the patch.

On Tue, Jul 05, 2022 at 04:31:11PM +0100, Kieran Bingham via libcamera-devel wrote:
> Support the X variants of the RGB pixel formats
> along side the equivalent Alpha component based versions.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/qcam/viewfinder_qt.cpp | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp
> index 27955e3f9593..14ecd70a839e 100644
> --- a/src/qcam/viewfinder_qt.cpp
> +++ b/src/qcam/viewfinder_qt.cpp
> @@ -28,8 +28,10 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats
>  {
>  #if QT_VERSION >= QT_VERSION_CHECK(5, 2, 0)
>  	{ libcamera::formats::ABGR8888, QImage::Format_RGBA8888 },
> +	{ libcamera::formats::XBGR8888, QImage::Format_RGBA8888 },

There's a QImage::Format_RGBX8888 format which may be better suited
(perhaps for both libcamera::formats::ABGR8888 and
libcamera::formats::XBGR8888).

>  #endif
>  	{ libcamera::formats::ARGB8888, QImage::Format_RGB32 },
> +	{ libcamera::formats::XRGB8888, QImage::Format_RGB32 },

Will Qt ignore the alpha channel when rendering ?

>  #if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
>  	{ libcamera::formats::RGB888, QImage::Format_BGR888 },
>  #endif

Quoting Laurent Pinchart (2022-07-05 21:59:59)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Jul 05, 2022 at 04:31:11PM +0100, Kieran Bingham via libcamera-devel wrote:
> > Support the X variants of the RGB pixel formats
> > along side the equivalent Alpha component based versions.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/qcam/viewfinder_qt.cpp | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp
> > index 27955e3f9593..14ecd70a839e 100644
> > --- a/src/qcam/viewfinder_qt.cpp
> > +++ b/src/qcam/viewfinder_qt.cpp
> > @@ -28,8 +28,10 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats
> >  {
> >  #if QT_VERSION >= QT_VERSION_CHECK(5, 2, 0)
> >       { libcamera::formats::ABGR8888, QImage::Format_RGBA8888 },
> > +     { libcamera::formats::XBGR8888, QImage::Format_RGBA8888 },
> 
> There's a QImage::Format_RGBX8888 format which may be better suited
> (perhaps for both libcamera::formats::ABGR8888 and
> libcamera::formats::XBGR8888).

In these formats, (in this patch) both of these when tested with vivid
show a 'transparent' frame. I.e. it's not visible, but working.

With QImage::Format_RGBX8888 - it's much better. I'll update to use
that. We want to ignore the Alpha channel in our case.

> 
> >  #endif
> >       { libcamera::formats::ARGB8888, QImage::Format_RGB32 },
> > +     { libcamera::formats::XRGB8888, QImage::Format_RGB32 },
> 
> Will Qt ignore the alpha channel when rendering ?

Both of these function with an image displayed from vivid. (I.e. leads
me to believe that the Alpha is ignored, but I haven't read the Qt
code).

> 
> >  #if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
> >       { libcamera::formats::RGB888, QImage::Format_BGR888 },
> >  #endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi Kieran,

On Wed, Jul 06, 2022 at 09:57:19AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-07-05 21:59:59)
> > On Tue, Jul 05, 2022 at 04:31:11PM +0100, Kieran Bingham via libcamera-devel wrote:
> > > Support the X variants of the RGB pixel formats
> > > along side the equivalent Alpha component based versions.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/qcam/viewfinder_qt.cpp | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp
> > > index 27955e3f9593..14ecd70a839e 100644
> > > --- a/src/qcam/viewfinder_qt.cpp
> > > +++ b/src/qcam/viewfinder_qt.cpp
> > > @@ -28,8 +28,10 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats
> > >  {
> > >  #if QT_VERSION >= QT_VERSION_CHECK(5, 2, 0)
> > >       { libcamera::formats::ABGR8888, QImage::Format_RGBA8888 },
> > > +     { libcamera::formats::XBGR8888, QImage::Format_RGBA8888 },
> > 
> > There's a QImage::Format_RGBX8888 format which may be better suited
> > (perhaps for both libcamera::formats::ABGR8888 and
> > libcamera::formats::XBGR8888).
> 
> In these formats, (in this patch) both of these when tested with vivid
> show a 'transparent' frame. I.e. it's not visible, but working.
> 
> With QImage::Format_RGBX8888 - it's much better. I'll update to use
> that. We want to ignore the Alpha channel in our case.
> 
> > 
> > >  #endif
> > >       { libcamera::formats::ARGB8888, QImage::Format_RGB32 },
> > > +     { libcamera::formats::XRGB8888, QImage::Format_RGB32 },
> > 
> > Will Qt ignore the alpha channel when rendering ?
> 
> Both of these function with an image displayed from vivid. (I.e. leads
> me to believe that the Alpha is ignored, but I haven't read the Qt
> code).

According to https://doc.qt.io/qt-5/qimage.html#Format-enum,
QImage::Format_RGB32 is defined as "The image is stored using a 32-bit
RGB format (0xffRRGGBB)" and QImage::Format_RGBX8888 as "The image is
stored using a 32-bit byte-ordered RGB(x) format (8-8-8-8). This is the
same as the Format_RGBA8888 except alpha must always be 255.". That's
certainly better than the alpha-enabled equivalents, it's just a bit of
a shame that the documentation doesn't state that implementations must
ignore the alpha component.

> > >  #if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
> > >       { libcamera::formats::RGB888, QImage::Format_BGR888 },
> > >  #endif