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

[Jacopo Mondi]

Hi Harvey

On Wed, Dec 04, 2024 at 04:36:30PM +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 | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 497d363d6..dd2c603e0 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request)

You should update the comment before the if()

>  				<< " not successfully completed: "
>  				<< request->status();
>
> -		abortRequest(descriptor);
> -		completeDescriptor(descriptor);
> -
> -		return;

Is it ok to continue and try to fetch metadata from a Request in error
status ?

Also, the

	if (request->status() != Request::RequestComplete) {

case, doesn't fall in

/**
 * S6. Error management:
 *

 ...

 * - The failure of an entire capture to occur must be reported by the HAL by
 *   calling notify() with ERROR_REQUEST. Individual errors for the result
 *   metadata or the output buffers must not be reported in this case.

I think the

		abortRequest(descriptor);
		completeDescriptor(descriptor);

sequence should be kept here ?

Specifically, if you don't call completeDescriptor() the descriptor
will remain in pending state and stall the queue of complete
descriptors


> +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>  	}
>
>  	/*
> @@ -1239,7 +1236,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;

To be honest I don't see what we gain by delaying the notification


>
>  		/*
>  		 * The camera framework expects an empty metadata pack on error.
> @@ -1325,6 +1322,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
> +		 * because the capture is cancelled by the camera. Only notify
> +		 * it when the final result is sent, since Android will ignore
> +		 * the following metadata.
> +		 */
> +		if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)
> +			notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);

If my understanding of the specs if correct, I think in case of

	if (request->status() != Request::RequestComplete) {

the right error code to notify is ERROR_REQUEST, so if my
understanding is correct, I would rather drop this whole change.

Or am I mistaken maybe ? Have you got any failure in CTS or other
tests because of this ?

>  	}
>  }
>
> --
> 2.47.0.338.g60cca15819-goog
>