[libcamera-devel] [PATCH 07/12] android: camera_buffer: Implement PIMPL pattern

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote:
> In order to prepare to support more memory backends, make the
> CameraBuffer class implement the PIMPL (pointer-to-implementation)
> pattern.

Is this the right design pattern for the job ? The pimpl/d-pointer
pattern is used to hide the implementation from the interface of the
class, mostly to help maintaining ABI compatibility. In this specific
case, what you need is likely just an overload, with a base class
defining virtual methods.

Another option, if you want to simplify the implementation (I'm actually
not entirely sure it would be simpler) is to define the interface in
camera_buffer.h, and multiple implementations in different .cpp files,
all named CameraBuffer. The .cpp file corresponding to the platform
would then be selected at compilation time. The drawback is that you
then would need to duplicate all functions in all implementations, even
the ones that could be shared.

> Define the CameraBuffer class interface whose actual implementation is
> delegated to an inner CameraBufferImpl 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               | 12 ++++
>  src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++-
>  2 files changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 0590cd84652b..2a91e6a3c9c1 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer
>  public:
>  	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
>  	~CameraBuffer();
> +
> +	bool isValid() const;
> +
> +	unsigned int numPlanes() const;
> +	ssize_t planeSize(unsigned int plane) const;
> +
> +	const uint8_t *plane(unsigned int plane) const;
> +	uint8_t *plane(unsigned int plane);
> +
> +private:
> +	class CameraBufferImpl;
> +	CameraBufferImpl *impl_;
>  };
>  
>  #endif /* __ANDROID_CAMERA_BUFFER_H__ */
> diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp
> index 807304a9e42d..10a43a61bd4d 100644
> --- a/src/android/mm/android_generic_buffer.cpp
> +++ b/src/android/mm/android_generic_buffer.cpp
> @@ -13,7 +13,21 @@ using namespace libcamera;
>  
>  LOG_DECLARE_CATEGORY(HAL)
>  
> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer
> +{
> +public:
> +	CameraBufferImpl(buffer_handle_t camera3Buffer, int flags);
> +	~CameraBufferImpl();
> +
> +	unsigned int numPlanes() const;
> +	ssize_t planeSize(unsigned int plane) const;
> +
> +	const uint8_t *plane(unsigned int plane) const;
> +	uint8_t *plane(unsigned int plane);
> +};
> +
> +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,
> +						 int flags)
>  {
>  	maps_.reserve(camera3Buffer->numFds);
>  	error_ = 0;
> @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
>  	}
>  }
>  
> +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()
> +{
> +}
> +
> +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const
> +{
> +	return maps_.size();
> +}
> +
> +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const
> +{
> +	if (plane >= maps_.size())
> +		return -EINVAL;
> +
> +	return maps_[plane].size();
> +}
> +
> +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const
> +{
> +	if (plane >= maps_.size())
> +		return nullptr;
> +
> +	return maps_[plane].data();
> +}
> +
> +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)
> +{
> +	if (plane >= maps_.size())
> +		return nullptr;
> +
> +	return maps_[plane].data();
> +}
> +
> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> +	: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))
> +{
> +}
> +
>  CameraBuffer::~CameraBuffer()
>  {
> +	delete impl_;
> +}
> +
> +bool CameraBuffer::isValid() const
> +{
> +	return impl_->isValid();
> +}
> +
> +unsigned int CameraBuffer::numPlanes() const
> +{
> +	return impl_->numPlanes();
> +}
> +
> +ssize_t CameraBuffer::planeSize(unsigned int plane) const
> +{
> +	return impl_->planeSize(plane);
> +}
> +
> +const uint8_t *CameraBuffer::plane(unsigned int plane) const
> +{
> +	return impl_->plane(plane);
> +}
> +
> +uint8_t *CameraBuffer::plane(unsigned int plane)
> +{
> +	return impl_->plane(plane);
>  }

-- 
Regards,

Laurent Pinchart