[PATCH v4 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST when a request fails

[Jacopo Mondi]

Hi Harvey

On Tue, Dec 10, 2024 at 02:23:58PM +0000, Harvey Yang wrote:
> According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used
> for requests that have not done any processing. When a request is
> completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead.
>
> To avoid code duplication, when CameraMetadata cannot be generated,
> CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result.
>
> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  src/android/camera_device.cpp | 40 ++++++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f3f570544..3b10f207e 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1182,6 +1182,18 @@ void CameraDevice::requestComplete(Request *request)
>  	 * post-processing/compression fails.
>  	 */
>  	for (auto &buffer : descriptor->buffers_) {
> +		for (auto &[_, frameBuffer] : request->buffers()) {

I've been thinking about ways to avoid the double lookup here, we
don't have that many requests or descriptor, but avoiding doing it for every
request might be desirable.

I've been trying to do so by setting a pointer to a StreamBuffer as
libcamera::FrameBuffer::cookie and retrieve it here. However, as
now multiple Android streams can map to the same FrameBuffer there's
no 1-to-1 association anymore between a StreamBuffer and a
FrameBuffer...

Too bad, I would have liked to avoid this

> +			if (buffer.srcBuffer != frameBuffer &&
> +			    buffer.frameBuffer.get() != frameBuffer)
> +				continue;
> +
> +			StreamBuffer::Status status = StreamBuffer::Status::Success;
> +			if (frameBuffer->metadata().status != FrameMetadata::FrameSuccess) {
> +				status = StreamBuffer::Status::Error;
> +			}

No {} for single line statements

> +			setBufferStatus(buffer, status);
> +		}
> +
>  		CameraStream *stream = buffer.stream;

Can you move this after the comment block if you have to resend ?

>
>  		/*
> @@ -1198,22 +1210,18 @@ void CameraDevice::requestComplete(Request *request)
>  			if (fence)
>  				buffer.fence = fence->release();
>  		}
> -		buffer.status = StreamBuffer::Status::Success;
>  	}
>
>  	/*
> -	 * If the Request has failed, abort the request by notifying the error
> -	 * and complete the request with all buffers in error state.
> +	 * If the Request has failed, complete the request with all buffers in
> +	 * error state and notify an error result.

With this patch buffers are completed indivdually in error state just
below. I would move the below if() before inspecting the single
buffers or even drop it completely and just rely on the above loop, after
all if one buffer has failed the whole request has failed and you now
set its state to Camera3RequestDescriptor::Status::Error in the above
call to setBufferStatus()

>  	 */
>  	if (request->status() != Request::RequestComplete) {
>  		LOG(HAL, Error) << "Request " << request->cookie()
>  				<< " not successfully completed: "
>  				<< request->status();
>
> -		abortRequest(descriptor);
> -		completeDescriptor(descriptor);
> -
> -		return;
> +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>  	}
>
>  	/*
> @@ -1238,7 +1246,7 @@ void CameraDevice::requestComplete(Request *request)
>  	 */
>  	descriptor->resultMetadata_ = getResultMetadata(*descriptor);
>  	if (!descriptor->resultMetadata_) {
> -		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>
>  		/*
>  		 * The camera framework expects an empty metadata pack on error.
> @@ -1271,7 +1279,13 @@ void CameraDevice::requestComplete(Request *request)
>  			continue;
>  		}
>
> +		if (buffer->status == StreamBuffer::Status::Error) {
> +			iter = descriptor->pendingStreamsToProcess_.erase(iter);
> +			continue;
> +		}

nit: could you move this before the previous if() ? There's no point
in searching the FrameBuffer if we know the stream has failed ?

> +
>  		++iter;
> +
>  		int ret = stream->process(buffer);
>  		if (ret) {
>  			setBufferStatus(*buffer, StreamBuffer::Status::Error);
> @@ -1324,6 +1338,16 @@ void CameraDevice::sendCaptureResults()
>  		descriptors_.pop();
>
>  		sendCaptureResult(descriptor.get());
> +
> +		/*
> +		 * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some
> +		 * of the expected result metadata might not be available

metdata and buffers ?

> +		 * because the capture is cancelled by the camera. Only notify
> +		 * it when the final result is sent, since Android will ignore
> +		 * the following metadata.
> +		 */

I would drop "since ..."

All minors, next one should be good to go

Thanks
  j

> +		if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)
> +			notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
>  	}
>  }
>
> --
> 2.47.0.338.g60cca15819-goog
>