[libcamera-devel] [PATCH 19/21] qcam: viewfinder: Avoid memory copy when conversion isn't needed
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 23 18:23:06 CET 2020
Hi Laurent,
On 23/03/2020 14:22, Laurent Pinchart wrote:
> If the frame buffer format is identical to the display format, the
> viewfinder still invokes the converter to perform what is essentially a
> slow memcpy(). Make is possible to skip that operation by creating a
s/is/it/
> QImage referencing the buffer memory instead. A reference to the frame
> buffer is kept internally, and released when the next buffer is queued,
> pushing the current one out.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Some questions ... but nothing blocking I don't think
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> src/qcam/viewfinder.cpp | 59 +++++++++++++++++++++++++++++------------
> src/qcam/viewfinder.h | 1 +
> 2 files changed, 43 insertions(+), 17 deletions(-)
>
> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> index 31b358da47dc..4c35659e24aa 100644
> --- a/src/qcam/viewfinder.cpp
> +++ b/src/qcam/viewfinder.cpp
> @@ -7,6 +7,8 @@
>
> #include "viewfinder.h"
>
> +#include <utility>
> +
> #include <QImage>
> #include <QImageWriter>
> #include <QMutexLocker>
> @@ -16,7 +18,7 @@
> #include "format_converter.h"
>
> ViewFinder::ViewFinder(QWidget *parent)
> - : QWidget(parent)
> + : QWidget(parent), buffer_(nullptr)
> {
> }
>
> @@ -27,17 +29,23 @@ ViewFinder::~ViewFinder()
> int ViewFinder::setFormat(const libcamera::PixelFormat &format,
> const QSize &size)
> {
> - int ret;
> + image_ = QImage();
> +
> + /*
> + * If format conversion is needed, configure the converter and allocate
> + * the destination image.
> + */
> + if (format != DRM_FORMAT_ARGB8888) {
Is this the only format which doesn't require converting? Can Qt support
any other formats for direct render? (I'm assuming things like a
compatible format which doesn't have alpha blending or such, but that
alpha is ignored perhaps?)
> + int ret = converter_.configure(format, size);
> + if (ret < 0)
> + return ret;
>
> - ret = converter_.configure(format, size);
> - if (ret < 0)
> - return ret;
> + image_ = QImage(size, QImage::Format_RGB32);
> + }
>
> format_ = format;
> size_ = size;
>
> - image_ = QImage(size_, QImage::Format_RGB32);
> -
> updateGeometry();
> return 0;
> }
> @@ -49,19 +57,36 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> return;
> }
>
> - QMutexLocker locker(&mutex_);
> -
> - /*
> - * \todo We're not supposed to block the pipeline handler thread
> - * for long, implement a better way to save images without
> - * impacting performances.
> - */
> + unsigned char *memory = static_cast<unsigned char *>(map->memory);
> + size_t size = buffer->metadata().planes[0].bytesused;
> +
> + {
> + QMutexLocker locker(&mutex_);
> +
> + if (format_ == DRM_FORMAT_ARGB8888) {
> + /*
> + * If the frame format is identical to the display
> + * format, create a QImage that references the frame
> + * and store a reference to the frame buffer. The
> + * previously stored frame buffer, if any, will be
> + * released.
> + */
> + image_ = QImage(memory, size_.width(), size_.height(),
> + size / size_.height(), QImage::Format_RGB32);
Is that stride 'guaranteed'?
Or should we get it from somewhere else?
If we can't get this from the buffer - do we need to add a todo note for
the future?
> + std::swap(buffer, buffer_);
Do we need to release buffer_ on stop at all?
> + } else {
> + /*
> + * Otherwise, convert the format and release the frame
> + * buffer immediately.
> + */
> + converter_.convert(memory, size, &image_);
> + }
> + }
>
> - converter_.convert(static_cast<unsigned char *>(map->memory),
> - buffer->metadata().planes[0].bytesused, &image_);
> update();
>
> - renderComplete(buffer);
> + if (buffer)
> + renderComplete(buffer);
> }
>
> QImage ViewFinder::getCurrentImage()
> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> index 54c0fa9dd7a0..b5153160f70e 100644
> --- a/src/qcam/viewfinder.h
> +++ b/src/qcam/viewfinder.h
> @@ -52,6 +52,7 @@ private:
> libcamera::PixelFormat format_;
> QSize size_;
>
> + libcamera::FrameBuffer *buffer_;
> QImage image_;
> QMutex mutex_; /* Prevent concurrent access to image_ */
> };
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list