[libcamera-devel] [PATCH v2.2 21/21] qcam: viewfinder: Add support for more native formats

[Laurent Pinchart]

Hi Kieran,

On Tue, Mar 24, 2020 at 08:44:47AM +0000, Kieran Bingham wrote:
> On 23/03/2020 23:28, Laurent Pinchart wrote:
> > Qt supports more 24-bit and 32-bit RGB formats for native painting. If
> > the frame buffer pixel format matches any of them, disable the converter
> > and create a QImage in the appropriate format.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> > Changes since v2:
> > 
> > - Add more formats
> > - Remove unnecessary parentheses
> > 
> > Changes since v1:
> > 
> > - Rename formats to nativeFormats
> > ---
> >  src/qcam/viewfinder.cpp | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> > index 45e226b58135..7e7407f56e4c 100644
> > --- a/src/qcam/viewfinder.cpp
> > +++ b/src/qcam/viewfinder.cpp
> > @@ -7,16 +7,34 @@
> >  
> >  #include "viewfinder.h"
> >  
> > +#include <stdint.h>
> >  #include <utility>
> >  
> >  #include <QImage>
> >  #include <QImageWriter>
> > +#include <QMap>
> >  #include <QMutexLocker>
> >  #include <QPainter>
> >  #include <QtDebug>
> >  
> >  #include "format_converter.h"
> >  
> > +static const QMap<uint32_t, QImage::Format> nativeFormats
> > +{
> > +#if QT_VERSION >= QT_VERSION_CHECK(5, 2, 0)
> > +#if QBYTE_ORDER == Q_LITTLE_ENDIAN
> > +	{ DRM_FORMAT_ABGR8888, QImage::Format_RGBA8888 },
> > +#else
> > +	{ DRM_FORMAT_RGBA8888, QImage::Format_RGBA8888 },
> > +#endif
> 
> There's no else attributed to the QT_VERSION_CHECK, so we will not
> support QImage::Format_RGBA8888 at all on version < 5.2.0...
> 
> Presumably that's not an issue.

We don't really have a choice though:

"QImage::Format_RGBA8888	The image is stored using a 32-bit
byte-ordered RGBA format (8-8-8-8). Unlike ARGB32 this is a byte-ordered
format, which means the 32bit encoding differs between big endian and
little endian architectures, being respectively (0xRRGGBBAA) and
(0xAABBGGRR). The order of the colors is the same on any architecture if
read as bytes 0xRR,0xGG,0xBB,0xAA. (added in Qt 5.2)"

> > +#endif
> 
> Can we explain (in a comment perhaps) why we need to handle byte order
> for QImage::Format_RGBA8888, but not QImage::Format_RGB32 ?

After thinking more about it, I think the implementation here is
incorrect. This particular format does not depend on byte ordering,
while all the other do. I'll drop big endian handling for now, we can
add that later for all the formats after researching it.

> > +	{ DRM_FORMAT_ARGB8888, QImage::Format_RGB32 },
> 
> > +#if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
> > +	{ DRM_FORMAT_BGR888, QImage::Format_BGR888 },
> > +#endif
> > +	{ DRM_FORMAT_RGB888, QImage::Format_RGB888 },
> > +};
> > +
> >  ViewFinder::ViewFinder(QWidget *parent)
> >  	: QWidget(parent), buffer_(nullptr)
> >  {
> > @@ -36,7 +54,7 @@ int ViewFinder::setFormat(const libcamera::PixelFormat &format,
> >  	 * If format conversion is needed, configure the converter and allocate
> >  	 * the destination image.
> >  	 */
> > -	if (format != DRM_FORMAT_ARGB8888) {
> > +	if (!nativeFormats.contains(format)) {
> >  		int ret = converter_.configure(format, size);
> >  		if (ret < 0)
> >  			return ret;
> > @@ -64,7 +82,7 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> >  	{
> >  		QMutexLocker locker(&mutex_);
> >  
> > -		if (format_ == DRM_FORMAT_ARGB8888) {
> > +		if (nativeFormats.contains(format_)) {
> >  			/*
> >  			 * If the frame format is identical to the display
> >  			 * format, create a QImage that references the frame
> > @@ -76,7 +94,8 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> >  			 * computing it naively
> >  			 */
> >  			image_ = QImage(memory, size_.width(), size_.height(),
> > -					size / size_.height(), QImage::Format_RGB32);
> > +					size / size_.height(),
> > +					nativeFormats[format_]);
> >  			std::swap(buffer, buffer_);
> >  		} else {
> >  			/*

-- 
Regards,

Laurent Pinchart