[libcamera-devel] [PATCH] qcam: Replace MappedBuffer with Span<uint8_t>
Hirokazu Honda
hiroh at chromium.org
Fri Aug 20 10:49:40 CEST 2021
Hi Laurent, thank you for the patch.
On Wed, Aug 18, 2021 at 6:08 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Wed, Aug 18, 2021 at 10:02:40AM +0100, Kieran Bingham wrote:
> > On 18/08/2021 01:22, Laurent Pinchart wrote:
> > > The MappedBuffer structure is a custom container that binds a data
> > > pointer with a length. This is exactly what Span is. Use it instead.
> >
> > It also potentially conflicts (thought different namespaces, and code
> > tree so it's fine) with the one in mapped_framebuffer.h - so I'm happy
> > to see this change.
> >
> > Of course I see potential to use MappedFrameBuffer in the future, if it
> > ever gets exposed as an API (either through libcamera, or a helpers
> > library) to avoid duplication of support for mapping buffers, and allow
> > support for multiplanar formats... but that's all futureware so:
>
> I have an increasingly strong feeling it will happen one way or another
> :-)
>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > > src/qcam/main_window.cpp | 17 +++++++++--------
> > > src/qcam/main_window.h | 2 +-
> > > src/qcam/viewfinder.h | 9 +++------
> > > src/qcam/viewfinder_gl.cpp | 5 +++--
> > > src/qcam/viewfinder_gl.h | 2 +-
> > > src/qcam/viewfinder_qt.cpp | 5 +++--
> > > src/qcam/viewfinder_qt.h | 2 +-
> > > 7 files changed, 21 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index 39d034de6bb2..1adaae60d83b 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -474,7 +474,8 @@ int MainWindow::startCapture()
> > > const FrameBuffer::Plane &plane = buffer->planes().front();
> > > void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> > > plane.fd.fd(), 0);
> > > - mappedBuffers_[buffer.get()] = { memory, plane.length };
> > > + mappedBuffers_[buffer.get()] = { static_cast<uint8_t *>(memory),
> > > + plane.length };
> > >
> > > /* Store buffers on the free list. */
> > > freeBuffers_[stream].enqueue(buffer.get());
> > > @@ -537,8 +538,8 @@ error:
> > > requests_.clear();
> > >
> > > for (auto &iter : mappedBuffers_) {
> > > - const MappedBuffer &buffer = iter.second;
> > > - munmap(buffer.memory, buffer.size);
> > > + const Span<uint8_t> &buffer = iter.second;
> > > + munmap(buffer.data(), buffer.size());
> > > }
> > > mappedBuffers_.clear();
> > >
> > > @@ -573,8 +574,8 @@ void MainWindow::stopCapture()
> > > camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
> > >
> > > for (auto &iter : mappedBuffers_) {
> > > - const MappedBuffer &buffer = iter.second;
> > > - munmap(buffer.memory, buffer.size);
> > > + const Span<uint8_t> &buffer = iter.second;
> > > + munmap(buffer.data(), buffer.size());
> > > }
> > > mappedBuffers_.clear();
> > >
> > > @@ -673,10 +674,10 @@ void MainWindow::processRaw(FrameBuffer *buffer,
> > > "DNG Files (*.dng)");
> > >
> > > if (!filename.isEmpty()) {
> > > - const MappedBuffer &mapped = mappedBuffers_[buffer];
> > > + const Span<uint8_t> &mapped = mappedBuffers_[buffer];
> > > DNGWriter::write(filename.toStdString().c_str(), camera_.get(),
> > > rawStream_->configuration(), metadata, buffer,
> > > - mapped.memory);
> > > + mapped.data());
> > > }
> > > #endif
> > >
> > > @@ -753,7 +754,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > << "fps:" << Qt::fixed << qSetRealNumberPrecision(2) << fps;
> > >
> > > /* Render the frame on the viewfinder. */
> > > - viewfinder_->render(buffer, &mappedBuffers_[buffer]);
> > > + viewfinder_->render(buffer, mappedBuffers_[buffer]);
> > > }
> > >
> > > void MainWindow::queueRequest(FrameBuffer *buffer)
> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > index 85d56ce49abe..6788de8ddde9 100644
> > > --- a/src/qcam/main_window.h
> > > +++ b/src/qcam/main_window.h
> > > @@ -106,7 +106,7 @@ private:
> > > FrameBufferAllocator *allocator_;
> > >
> > > std::unique_ptr<CameraConfiguration> config_;
> > > - std::map<FrameBuffer *, MappedBuffer> mappedBuffers_;
> > > + std::map<FrameBuffer *, Span<uint8_t>> mappedBuffers_;
> > >
> > > /* Capture state, buffers queue and statistics */
> > > bool isCapturing_;
> > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> > > index 46747c227d0c..42d40f1f33f0 100644
> > > --- a/src/qcam/viewfinder.h
> > > +++ b/src/qcam/viewfinder.h
> > > @@ -11,14 +11,11 @@
> > > #include <QList>
> > > #include <QSize>
> > >
> > > +#include <libcamera/base/span.h>
> > > +
> > > #include <libcamera/formats.h>
> > > #include <libcamera/framebuffer.h>
> > >
> > > -struct MappedBuffer {
> > > - void *memory;
> > > - size_t size;
> > > -};
> > > -
> > > class ViewFinder
> > > {
> > > public:
> > > @@ -27,7 +24,7 @@ public:
> > > virtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0;
> > >
> > > virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;
> > > - virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;
> > > + virtual void render(libcamera::FrameBuffer *buffer, libcamera::Span<uint8_t> mem) = 0;
> > > virtual void stop() = 0;
> > >
> > > virtual QImage getCurrentImage() = 0;
> > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> > > index add87db88207..40226601f9fd 100644
> > > --- a/src/qcam/viewfinder_gl.cpp
> > > +++ b/src/qcam/viewfinder_gl.cpp
> > > @@ -110,7 +110,8 @@ QImage ViewFinderGL::getCurrentImage()
> > > return grabFramebuffer();
> > > }
> > >
> > > -void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer,
> > > + libcamera::Span<uint8_t> mem)
> > > {
> > > if (buffer->planes().size() != 1) {
> > > qWarning() << "Multi-planar buffers are not supported";
> > > @@ -120,7 +121,7 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > if (buffer_)
> > > renderComplete(buffer_);
> > >
> > > - data_ = static_cast<unsigned char *>(map->memory);
> > > + data_ = mem.data();
> > > /*
> > > * \todo Get the stride from the buffer instead of computing it naively
> > > */
> > > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h
> > > index 4a0f8ca58f29..3334549e0be4 100644
> > > --- a/src/qcam/viewfinder_gl.h
> > > +++ b/src/qcam/viewfinder_gl.h
> > > @@ -39,7 +39,7 @@ public:
> > > const QList<libcamera::PixelFormat> &nativeFormats() const override;
> > >
> > > int setFormat(const libcamera::PixelFormat &format, const QSize &size) override;
> > > - void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;
> > > + void render(libcamera::FrameBuffer *buffer, libcamera::Span<uint8_t> mem) override;
> > > void stop() override;
> > >
> > > QImage getCurrentImage() override;
> > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp
> > > index e436714c6bdb..efa1d412584b 100644
> > > --- a/src/qcam/viewfinder_qt.cpp
> > > +++ b/src/qcam/viewfinder_qt.cpp
> > > @@ -78,14 +78,15 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,
> > > return 0;
> > > }
> > >
> > > -void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > +void ViewFinderQt::render(libcamera::FrameBuffer *buffer,
> > > + libcamera::Span<uint8_t> mem)
> > > {
> > > if (buffer->planes().size() != 1) {
> > > qWarning() << "Multi-planar buffers are not supported";
> > > return;
> > > }
> > >
> > > - unsigned char *memory = static_cast<unsigned char *>(map->memory);
> > > + unsigned char *memory = mem.data();
> > > size_t size = buffer->metadata().planes[0].bytesused;
> > >
> > > {
> > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h
> > > index 501c72a70dec..1a569b9cee6e 100644
> > > --- a/src/qcam/viewfinder_qt.h
> > > +++ b/src/qcam/viewfinder_qt.h
> > > @@ -32,7 +32,7 @@ public:
> > > const QList<libcamera::PixelFormat> &nativeFormats() const override;
> > >
> > > int setFormat(const libcamera::PixelFormat &format, const QSize &size) override;
> > > - void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;
> > > + void render(libcamera::FrameBuffer *buffer, libcamera::Span<uint8_t> mem) override;
> > > void stop() override;
> > >
> > > QImage getCurrentImage() override;
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list