Message ID | 20200323232801.12010-1-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Laurent, 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@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@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. > +#endif Can we explain (in a comment perhaps) why we need to handle byte order for QImage::Format_RGBA8888, but not QImage::Format_RGB32 ? > + { 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 { > /* >
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@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@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 { > > /*
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 +#endif + { 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 { /*