[libcamera-devel] [v2 PATCH] android: mm: cros_camera_buffer: Log failure error on cleanup

[Kieran Bingham]

On 07/09/2021 15:59, Umang Jain wrote:
> Failure can still happen by CameraBufferManager during Unlock() and/or
> Deregister() of camera3Buffer handles. We should be logging those
> errors as well.
> 

I am suspicious that the reason you've seen these errors is that you are
double-free'ing the CameraBuffer (which is why you get an invalid handle).

I think having the error prints are better than not having them though,
as it should there was a failure that was otherwise silent.

I'm surprised there wasn't a segfault or such in fact.

But there's no direct issue or fault with this patch - It makes me go
back to wanting to put a double-free protection/assertion on the
Extensible class though..

But for here:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> Changes in v2:
> - Remove a debug log that creeped in by mistake
> 
> I have been able to spot one of the failure which is happening on my
> in-developement async post-processing. It is due a failure in
> Deregister(). It is intermittent and non-fatal as far as I can see.
> 
> Having a failure log in HAL, apart from https://paste.debian.net/1210728/
> helps to know the exact place from where the error is originating from
> libcamera HAL.
> ---
>  src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index ec45e04c..86770135 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -73,10 +73,20 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  
>  CameraBuffer::Private::~Private()
>  {
> -	if (mapped_)
> -		bufferManager_->Unlock(handle_);
> -	if (registered_)
> -		bufferManager_->Deregister(handle_);
> +	int ret;
> +	if (mapped_) {
> +		ret = bufferManager_->Unlock(handle_);
> +		if (ret != 0)
> +			LOG(HAL, Error) << "Failed to unlock buffer: "
> +					<< strerror(-ret);
> +	}
> +
> +	if (registered_) {
> +		ret = bufferManager_->Deregister(handle_);
> +		if (ret != 0)
> +			LOG(HAL, Error) << "Failed to deregister buffer: "
> +					<< strerror(-ret);
> +	}
>  }
>  
>  unsigned int CameraBuffer::Private::numPlanes() const
>