[libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as invalid if cros::CameraBufferManager::Register() fails

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

I'd write the subject line as "android: mm: cros: Handle buffer
registration failure" to shorten it.

On Fri, Apr 02, 2021 at 01:03:40PM +0900, Hirokazu Honda wrote:
> cros::CameraBufferManager::Register() fails if a buffer handle
> is invalid. We should mark CameraBuffer as invalid on the failure
> of Register().
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/mm/cros_camera_buffer.cpp | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index 7df4f47c..5f98ee44 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -53,12 +53,16 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
>  {
>  	bufferManager_ = cros::CameraBufferManager::GetInstance();
>  
> -	bufferManager_->Register(camera3Buffer);
> +	int ret = bufferManager_->Register(camera3Buffer);
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed registering a buffer: " << ret;

s/a buffer/buffer/

This looks good,

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

I'll apply the patch if you're OK with the proposed changes.

One additional question though. The CameraBuffer::Private destructors
calls

	bufferManager_->Unlock(handle_);
	bufferManager_->Deregister(handle_);

unconditionally. Is that safe to do if the buffer hasn't been registered
and locked properly ?

> +		return;
> +	}
>  
>  	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
>  	switch (numPlanes_) {
>  	case 1: {
> -		int ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> +		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
>  		if (ret) {
>  			LOG(HAL, Error) << "Single plane buffer mapping failed";
>  			return;
> @@ -67,8 +71,8 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
>  	}
>  	case 2:
>  	case 3: {
> -		int ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> -						    &mem.ycbcr);
> +		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> +						&mem.ycbcr);
>  		if (ret) {
>  			LOG(HAL, Error) << "YCbCr buffer mapping failed";
>  			return;

-- 
Regards,

Laurent Pinchart