[libcamera-devel,3/5] mapped_framebuffer: Introduce MappedVectorBuffer

13574 diff mbox

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.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/mapped_framebuffer.h |  9 +++++++++
 src/libcamera/mapped_framebuffer.cpp            | 17 +++++++++++++++++
 2 files changed, 26 insertions(+)

Hi Hiro,

Thank you for the patch.

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.

> Signed-off-by: Hirokazu Honda <hiroh@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 */

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@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 */