[libcamera-devel] [PATCH 09/10] android: mm: Provide helper macro for PIMPL

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 01, 2021 at 04:01:10PM +0100, Jacopo Mondi wrote:
> Each memory backend has to declare a CameraBuffer class implementation
> that bridges the API calls to each CameraBufferImpl implementation.
> 
> As the code is likely the same for most (if not all) backends, provide
> a convenience macro that expands to the CameraBuffer class declaration.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_buffer.h              | 28 ++++++++++++++++++++
>  src/android/mm/generic_camera_buffer.cpp | 33 +-----------------------
>  2 files changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 2311cdaf96b2..ea73bf5b0ede 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -28,4 +28,32 @@ public:
>  	libcamera::Span<uint8_t> plane(unsigned int plane);
>  };
>  
> +#define PUBLIC_CAMERA_BUFFER						\

Maybe PUBLIC_CAMERA_BUFFER_IMPLEMENTATION ? Up to you.

> +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            \

I think indentation is incorrect here.

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

And here too.

I would normally say it would be nice to avoid duplicating the
implementation, but we only compile one of the .cpp file, so there's
actually no duplication in the binary.

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

> +{                                                                       \
> +	Private *const d = LIBCAMERA_D_PTR();				\
> +	return d->plane(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 ea85be805260..5ea1339bc0ec 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -86,35 +86,4 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  	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);
> -}
> -
> -Span<uint8_t> CameraBuffer::plane(unsigned int plane)
> -{
> -	Private *const d = LIBCAMERA_D_PTR();
> -	return d->plane(plane);
> -}
> +PUBLIC_CAMERA_BUFFER

-- 
Regards,

Laurent Pinchart