| Message ID | 20200323142205.28342-20-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Some questions ... but nothing blocking I don't think Reviewed-by: Kieran Bingham <kieran.bingham@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_ */ > }; >
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@ideasonboard.com> > > Some questions ... but nothing blocking I don't think > > Reviewed-by: Kieran Bingham <kieran.bingham@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_ */ > > };
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) { + 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); + std::swap(buffer, buffer_); + } 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_ */ };
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 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@ideasonboard.com> --- src/qcam/viewfinder.cpp | 59 +++++++++++++++++++++++++++++------------ src/qcam/viewfinder.h | 1 + 2 files changed, 43 insertions(+), 17 deletions(-)