[libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a static function to check a buffer validness

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:
> This adds a static function to CameraBuffer class that checks if
> a buffer_handle is valid with a platform dependent framework.
> For example, the function validates a buffer using
> cros::CameraBufferManager on ChromeOS.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_buffer.h              |  6 ++++++
>  src/android/mm/cros_camera_buffer.cpp    | 18 ++++++++++++++++++
>  src/android/mm/generic_camera_buffer.cpp |  9 +++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 7e8970b4..ade082c3 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -20,6 +20,8 @@ public:
>  	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
>  	~CameraBuffer();
>  
> +	static bool isValidBuffer(buffer_hanlde_t camera3Buffer);
> +
>  	bool isValid() const;
>  
>  	unsigned int numPlanes() const;
> @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)	\
>  CameraBuffer::~CameraBuffer()						\
>  {									\
>  }									\
> +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer)		\
> +{									\
> +	return Private::isValidBuffer(buffer)				\
> +}									\
>  bool CameraBuffer::isValid() const					\
>  {									\
>  	const Private *const d = LIBCAMERA_D_PTR();			\
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index 1a4fd5d1..f8449dfd 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -24,6 +24,8 @@ public:
>  		buffer_handle_t camera3Buffer, int flags);
>  	~Private();
>  
> +	static bool isValidBuffer(buffer_handle_t camera3Buffer);
> +
>  	bool isValid() const { return valid_; }
>  
>  	unsigned int numPlanes() const;
> @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
>  	return bufferManager_->GetPlaneSize(handle_, 0);
>  }
>  
> +/* static */
> +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)
> +{
> +	cros::CameraBufferManager *bufferManager =
> +		cros::CameraBufferManager::GetInstance();
> +
> +	int ret = bufferManager->Register(camera3Buffer);
> +	if (ret) {
> +		return false;
> +	}

No need for braces.

> +
> +	bufferManager->Deregister(camera3Buffer);

This seems quite inefficient, as it will lead to registering all
buffers, even the ones we don't need to map to the CPU. For buffers that
we need to map to the CPU, we will register and unregister them here,
and then register them later.

Could we do better than this ?

> +
> +	return true;
> +}
> +
>  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index 929e078a..07a07372 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -24,6 +24,8 @@ public:
>  		buffer_handle_t camera3Buffer, int flags);
>  	~Private();
>  
> +	static bool isValidBuffer(buffer_handle_t camera3Buffer);
> +
>  	unsigned int numPlanes() const;
>  
>  	Span<uint8_t> plane(unsigned int plane);
> @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
>  				      maxJpegBufferSize);
>  }
>  
> +/* static */
> +bool CameraBuffer::Private::isValidBuffer(
> +	[[maybe_unused]] buffer_handle_t camera3Buffer)
> +{
> +	return true;
> +}
> +
>  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION

-- 
Regards,

Laurent Pinchart