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

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Sun, Apr 04, 2021 at 01:22:47AM +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().

I'll add the following paragraph to describe the change in the
destructor:

While the cros::CameraBufferManager Unlock() and Deregister() functions
should be able to handle buffers that haven't been locked and
registered, this isn't an API guarantee, and errors will be logged.
Avoid this by skipping unlocking and unregistration of buffers that
haven't been locked or registered.

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> ---
>  src/android/mm/cros_camera_buffer.cpp | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index 7df4f47c..2a82d4b7 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -37,6 +37,7 @@ private:
>  	buffer_handle_t handle_;
>  	unsigned int numPlanes_;
>  	bool valid_;
> +	bool registered_;
>  	union {
>  		void *addr;
>  		android_ycbcr ycbcr;
> @@ -49,16 +50,21 @@ private:
>  CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
>  			       buffer_handle_t camera3Buffer, int flags)
>  	: Extensible::Private(cameraBuffer), handle_(camera3Buffer),
> -	  numPlanes_(0), valid_(false)
> +	  numPlanes_(0), valid_(false), registered(false)
>  {
>  	bufferManager_ = cros::CameraBufferManager::GetInstance();
> 
> -	bufferManager_->Register(camera3Buffer);
> +	int ret = bufferManager_->Register(camera3Buffer);
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed registering a buffer: " << ret;
> +		return;
> +	}
> 
> +	registered_ = true;
>  	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 +73,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;
> @@ -85,8 +91,10 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> 
>  CameraBuffer::Private::~Private()
>  {
> -	bufferManager_->Unlock(handle_);
> -	bufferManager_->Deregister(handle_);
> +	if (valid_)
> +		bufferManager_->Unlock(handle_);
> +	if (registered_)
> +		bufferManager_->Deregister(handle_);
>  }
> 
>  unsigned int CameraBuffer::Private::numPlanes() const

-- 
Regards,

Laurent Pinchart