[libcamera-devel] [PATCH 3/5] mapped_framebuffer: Introduce MappedVectorBuffer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 13 05:51:07 CEST 2021
Hi Hiro,
On Wed, Sep 01, 2021 at 12:45:02AM +0300, Laurent Pinchart wrote:
> On Tue, Aug 31, 2021 at 03:34:36PM +0900, Hirokazu Honda wrote:
> > MappedFrameBuffer is a main class of deriving MappedBuffer. It works
> > with FrameBuffer, so the source planes must be given as file
> > descriptors.
> > MappedBuffer interface is a generic class of providing accessing
> > planes in memory. This introduces a new class deriving MappedBuffer,
> > MappedVectorBuffer. It is created with the plane data in heap,
> > concretely, std::vector<uint8_t> and owns it.
>
> The MappedBuffer interface is geared towards performing memory mappings
> (hence the maps_ member in the base class), which isn't applicable here.
> I see why you want this in this series, but I'm not sure it's the best
> solution. It feels that what you need is a class that represents an
> image accessible by the CPU, regardless of how the memory was obtained.
> I fear that creating such a generic image class will quickly grow out of
> control and become a fully fledged CPU image API.
>
> I'll have to sleep over this.
I think an Image class would be the best way forward. We have an initial
implementation in the cam application, but it needs some rework. Here's
a copy of the relevant part of the discussion during the review of that
class:
The Image class is an interface to provide access to pixel data. In
its current form it's constructed from a FrameBuffer, but I'd like the
ability to construct it from a byte array as well. [...] It would
allow the JPEG compression in the Android HAL to use an Image as the
source, regardless of whether the compresses the still capture (coming
from libcamera in a FrameBuffer) or the thumbnail (downscaled in
software and stored in a std::vector<uint8_t>).
What I still haven't determined is whether the Image class should be
an interface with pure virtual functions only, implemented by
subclasses such as FrameBufferImage or MemoryImage, or if it should
contain the data as well, populated by the different constructors.
I've also started to think about how to perform the mapping. For
FrameBuffer objects constructed from Android buffers, the mapping
should be delegated to gralloc on Android and to the
CameraBufferManager on Chrome OS. For FrameBuffer objects constructed
internally by the V4L2VideoDevice (and in particular the ones exposes
to applications with FrameBufferAllocator), the code [in the Image
class] should be correct. For other types of FrameBuffer objects
supplied by applications, another method of mapping may be needed. I'm
not sure yet how to best handle that, and if we'll need a
FrameBufferMapper object that FrameBuffer instances will reference.
I'm afraid I don't have answers to the open question, I think they
require prototyping, especially when it comes to how to map a
FrameBuffer provided by an application.
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> > include/libcamera/internal/mapped_framebuffer.h | 9 +++++++++
> > src/libcamera/mapped_framebuffer.cpp | 17 +++++++++++++++++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> > index 70ffbcbe..7deecded 100644
> > --- a/include/libcamera/internal/mapped_framebuffer.h
> > +++ b/include/libcamera/internal/mapped_framebuffer.h
> > @@ -59,6 +59,15 @@ public:
> >
> > LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)
> >
> > +class MappedVectorBuffer : public MappedBuffer
> > +{
> > +public:
> > + MappedVectorBuffer(std::vector<std::vector<uint8_t>> &&planes);
> > +
> > +private:
> > + std::vector<std::vector<uint8_t>> data_;
> > +};
> > +
> > } /* namespace libcamera */
> >
> > #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */
> > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> > index 464d35fe..b09ad41a 100644
> > --- a/src/libcamera/mapped_framebuffer.cpp
> > +++ b/src/libcamera/mapped_framebuffer.cpp
> > @@ -240,4 +240,21 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
> > }
> > }
> >
> > +/**
> > + * \class MappedVectorBuffer
> > + * \brief Owns planes data in heap and provides the MappedBuffer interface
> > + */
> > +
> > +/**
> > + * \brief Takes the ownership of the passed \a planes
> > + * \param[in] planes the plane data that shall be owned by MappedVectorBuffer
> > + */
> > +MappedVectorBuffer::MappedVectorBuffer(std::vector<std::vector<uint8_t>> &&planes)
> > + : data_(std::move(planes))
> > +{
> > + planes_.reserve(data_.size());
> > + for (auto &plane : data_)
> > + planes_.emplace_back(plane.data(), plane.size());
> > +}
> > +
> > } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list