[libcamera-devel] [PATCH 06/10] android: camera_buffer: Implement libcamera::Extensible

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 01, 2021 at 04:01:07PM +0100, Jacopo Mondi wrote:
> In order to prepare to support more memory backends, make the
> CameraBuffer class implement the PIMPL (pointer-to-implementation)
> pattern by inheriting from the libcamera::Extensible class.
> 
> Temporary maintain libcamera::MappedBuffer as the CameraBuffer base
> class to maintain compatibility of the CameraStream::process() interface
> that requires a MappedBuffer * as second argument and will be converted
> to use a CameraBuffer in the next patch.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_buffer.h              | 14 ++++-
>  src/android/mm/generic_camera_buffer.cpp | 74 +++++++++++++++++++++++-
>  2 files changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 0590cd84652b..ca4f4527c690 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -9,13 +9,25 @@
>  
>  #include <hardware/camera3.h>
>  
> +#include <libcamera/class.h>
>  #include <libcamera/internal/buffer.h>
> +#include <libcamera/span.h>
>  
> -class CameraBuffer : public libcamera::MappedBuffer
> +class CameraBuffer final : public libcamera::Extensible,
> +			   public libcamera::MappedBuffer
>  {
> +	LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> +
>  public:
>  	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
>  	~CameraBuffer();
> +
> +	bool isValid() const;
> +
> +	unsigned int numPlanes() const;
> +
> +	libcamera::Span<const uint8_t> plane(unsigned int plane) const;
> +	libcamera::Span<uint8_t> plane(unsigned int plane);
>  };
>  
>  #endif /* __ANDROID_CAMERA_BUFFER_H__ */
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index d1407f9cddf4..eedf16b76542 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -13,7 +13,25 @@ using namespace libcamera;
>  
>  LOG_DECLARE_CATEGORY(HAL)
>  
> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> +class CameraBuffer::Private : public Extensible::Private,
> +			      public libcamera::MappedBuffer
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
> +
> +public:
> +	Private(CameraBuffer *cameraBuffer,
> +		buffer_handle_t camera3Buffer, int flags);
> +	~Private();
> +
> +	unsigned int numPlanes() const;
> +
> +	Span<const uint8_t> plane(unsigned int plane) const;
> +	Span<uint8_t> plane(unsigned int plane);
> +};
> +
> +CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> +			       buffer_handle_t camera3Buffer, int flags)
> +	: Extensible::Private(cameraBuffer)
>  {
>  	maps_.reserve(camera3Buffer->numFds);
>  	error_ = 0;
> @@ -42,6 +60,60 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
>  	}
>  }
>  
> +CameraBuffer::Private::~Private()
> +{
> +}
> +
> +unsigned int CameraBuffer::Private::numPlanes() const
> +{
> +	return maps_.size();
> +}
> +
> +Span<const uint8_t> CameraBuffer::Private::plane(unsigned int plane) const
> +{
> +	if (plane >= maps_.size())
> +		return {};
> +
> +	return maps_[plane];
> +}
> +
> +Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> +{
> +	if (plane >= maps_.size())
> +		return {};
> +
> +	return maps_[plane];
> +}
> +
> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> +	: Extensible(new Private(this, camera3Buffer, flags))
> +{
> +}
> +
>  CameraBuffer::~CameraBuffer()
>  {
>  }
> +
> +bool CameraBuffer::isValid() const
> +{
> +	const Private *const d = LIBCAMERA_D_PTR();
> +	return d->isValid();
> +}
> +
> +unsigned int CameraBuffer::numPlanes() const
> +{
> +	const Private *const d = LIBCAMERA_D_PTR();
> +	return d->numPlanes();
> +}
> +
> +Span<const uint8_t> CameraBuffer::plane(unsigned int plane) const
> +{
> +	const Private *const d = LIBCAMERA_D_PTR();
> +	return d->plane(plane);

How about implementing this as

	return const_cast<Private *>(d)->plane(plane);

to avoid two plane() function in the private class ? You could then drop
the const version of Private::plane() (it would be possible to do it the
other way around, but conceptually, keeping the non-const version is
better I think as we return a pointer to non-const memory).

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +}
> +
> +Span<uint8_t> CameraBuffer::plane(unsigned int plane)
> +{
> +	Private *const d = LIBCAMERA_D_PTR();
> +	return d->plane(plane);
> +}

-- 
Regards,

Laurent Pinchart