[libcamera-devel] [PATCH 19/21] qcam: viewfinder: Avoid memory copy when conversion isn't needed
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 23 18:27:16 CET 2020
Hi Kieran,
On Mon, Mar 23, 2020 at 05:23:06PM +0000, Kieran Bingham wrote:
> 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?)
See the last patch in the series :-)
> > + 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?
We should get it from the buffer, but we don't support strides yet. I'll
add a todo.
> > + std::swap(buffer, buffer_);
>
> Do we need to release buffer_ on stop at all?
Yes. It's in the next patch, I'll move it here.
> > + } 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list