[libcamera-devel,v3,1/8] android: Rework request completion notification

12352 diff mbox series

The current implementation of CameraDevice::requestComplete() which
handles event notification and calls the framework capture result
callback does not handle error notification precisely enough.

In detail:
- Error notification is an asynchronous callback that has to be notified
  to the framework as soon as an error condition is detected, and it
  independent from the process_capture_result() callback

- Error notification requires the HAL to report the precise error cause,
  by specifying the correct CAMERA3_MSG_ERROR_* error code.

The current implementation only notifies errors of type
CAMERA3_MSG_ERROR_REQUEST at the end of the procedure, before the
callback invocation.

Rework the procedure to:

- Notify CAMERA3_MSG_ERROR_DEVICE and perform library tear-down in case
  a Fatal error is detected

- Notify CAMERA3_MSG_ERROR_REQUEST if the libcamera::Request::status is
  different than RequestCompleted and immediately call
  process_capture_result() with all buffers in error state.

- Notify the shutter event as soon as possible

- Notify CAMERA3_MSG_ERROR_RESULT in case the metadata cannot be
  generated correctly and call process_capture_result() with the right
  buffer state regardless of metadata availability.

- Notify CAMERA3_MSG_ERROR_BUFFER for buffers whose post-processing
  failed

While at it, return the CameraStream buffer by calling
cameraStream->putBuffer() regardless of the post-processing result.

No regression detected when running CTS in LIMITED mode.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 138 +++++++++++++++++++++-------------
 src/android/camera_device.h   |   3 +-
 2 files changed, 86 insertions(+), 55 deletions(-)

Hi Jacopo,

Thanks for your work.

On 2021-05-21 17:42:20 +0200, Jacopo Mondi wrote:
> The current implementation of CameraDevice::requestComplete() which
> handles event notification and calls the framework capture result
> callback does not handle error notification precisely enough.
> 
> In detail:
> - Error notification is an asynchronous callback that has to be notified
>   to the framework as soon as an error condition is detected, and it
>   independent from the process_capture_result() callback
> 
> - Error notification requires the HAL to report the precise error cause,
>   by specifying the correct CAMERA3_MSG_ERROR_* error code.
> 
> The current implementation only notifies errors of type
> CAMERA3_MSG_ERROR_REQUEST at the end of the procedure, before the
> callback invocation.
> 
> Rework the procedure to:
> 
> - Notify CAMERA3_MSG_ERROR_DEVICE and perform library tear-down in case
>   a Fatal error is detected
> 
> - Notify CAMERA3_MSG_ERROR_REQUEST if the libcamera::Request::status is
>   different than RequestCompleted and immediately call
>   process_capture_result() with all buffers in error state.
> 
> - Notify the shutter event as soon as possible
> 
> - Notify CAMERA3_MSG_ERROR_RESULT in case the metadata cannot be
>   generated correctly and call process_capture_result() with the right
>   buffer state regardless of metadata availability.
> 
> - Notify CAMERA3_MSG_ERROR_BUFFER for buffers whose post-processing
>   failed
> 
> While at it, return the CameraStream buffer by calling
> cameraStream->putBuffer() regardless of the post-processing result.
> 
> No regression detected when running CTS in LIMITED mode.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

I can't build master as it's broken for my environment but I trust this 
compiles :-)

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/android/camera_device.cpp | 138 +++++++++++++++++++++-------------
>  src/android/camera_device.h   |   3 +-
>  2 files changed, 86 insertions(+), 55 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b32e8be59134..bd96b355ea92 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1956,17 +1956,20 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  
>  void CameraDevice::requestComplete(Request *request)
>  {
> -	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> -	std::unique_ptr<CameraMetadata> resultMetadata;
> -
>  	decltype(descriptors_)::node_type node;
>  	{
>  		std::scoped_lock<std::mutex> lock(mutex_);
>  		auto it = descriptors_.find(request->cookie());
>  		if (it == descriptors_.end()) {
> +			/*
> +			 * \todo Clarify if the Camera has to be closed on
> +			 * ERROR_DEVICE and possibly demote the Fatal to simple
> +			 * Error.
> +			 */
> +			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
>  			LOG(HAL, Fatal)
>  				<< "Unknown request: " << request->cookie();
> -			status = CAMERA3_BUFFER_STATUS_ERROR;
> +
>  			return;
>  		}
>  
> @@ -1974,16 +1977,72 @@ void CameraDevice::requestComplete(Request *request)
>  	}
>  	Camera3RequestDescriptor &descriptor = node.mapped();
>  
> +	/*
> +	 * Prepare the capture result for the Android camera stack.
> +	 *
> +	 * The buffer status is set to OK and later changed to ERROR if
> +	 * post-processing/compression fails.
> +	 */
> +	camera3_capture_result_t captureResult = {};
> +	captureResult.frame_number = descriptor.frameNumber_;
> +	captureResult.num_output_buffers = descriptor.buffers_.size();
> +	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> +		buffer.acquire_fence = -1;
> +		buffer.release_fence = -1;
> +		buffer.status = CAMERA3_BUFFER_STATUS_OK;
> +	}
> +	captureResult.output_buffers = descriptor.buffers_.data();
> +	captureResult.partial_result = 1;
> +
> +	/*
> +	 * If the Request has failed, abort the request by notifying the error
> +	 * and complete the request with all buffers in error state.
> +	 */
>  	if (request->status() != Request::RequestComplete) {
> -		LOG(HAL, Error) << "Request not successfully completed: "
> +		LOG(HAL, Error) << "Request " << request->cookie()
> +				<< " not successfully completed: "
>  				<< request->status();
> -		status = CAMERA3_BUFFER_STATUS_ERROR;
> +
> +		notifyError(descriptor.frameNumber_,
> +			    descriptor.buffers_[0].stream,
> +			    CAMERA3_MSG_ERROR_REQUEST);
> +
> +		captureResult.partial_result = 0;
> +		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +		callbacks_->process_capture_result(callbacks_, &captureResult);
> +
> +		return;
>  	}
>  
> +	/*
> +	 * Notify shutter as soon as we have verified we have a valid request.
> +	 *
> +	 * \todo The shutter event notification should be sent to the framework
> +	 * as soon as possible, earlier than request completion time.
> +	 */
> +	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> +							 .get(controls::SensorTimestamp));
> +	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> +
>  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
>  			<< descriptor.buffers_.size() << " streams";
>  
> -	resultMetadata = getResultMetadata(descriptor);
> +	/*
> +	 * Generate the metadata associated with the captured buffers.
> +	 *
> +	 * Notify if the metadata generation has failed, but continue processing
> +	 * buffers and return an empty metadata pack.
> +	 */
> +	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> +	if (!resultMetadata) {
> +		notifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,
> +			    CAMERA3_MSG_ERROR_RESULT);
> +
> +		/* The camera framework expects an empy metadata pack on error. */
> +		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> +	}
> +	captureResult.result = resultMetadata->get();
>  
>  	/* Handle any JPEG compression. */
>  	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> @@ -1996,56 +2055,27 @@ void CameraDevice::requestComplete(Request *request)
>  		FrameBuffer *src = request->findBuffer(cameraStream->stream());
>  		if (!src) {
>  			LOG(HAL, Error) << "Failed to find a source stream buffer";
> +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +			notifyError(descriptor.frameNumber_, buffer.stream,
> +				    CAMERA3_MSG_ERROR_BUFFER);
>  			continue;
>  		}
>  
> -		int ret = cameraStream->process(*src,
> -						*buffer.buffer,
> +		int ret = cameraStream->process(*src, *buffer.buffer,
>  						descriptor.settings_,
>  						resultMetadata.get());
> -		if (ret) {
> -			status = CAMERA3_BUFFER_STATUS_ERROR;
> -			continue;
> -		}
> -
>  		/*
>  		 * Return the FrameBuffer to the CameraStream now that we're
>  		 * done processing it.
>  		 */
>  		if (cameraStream->type() == CameraStream::Type::Internal)
>  			cameraStream->putBuffer(src);
> -	}
>  
> -	/* Prepare to call back the Android camera stack. */
> -	camera3_capture_result_t captureResult = {};
> -	captureResult.frame_number = descriptor.frameNumber_;
> -	captureResult.num_output_buffers = descriptor.buffers_.size();
> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> -		buffer.acquire_fence = -1;
> -		buffer.release_fence = -1;
> -		buffer.status = status;
> -	}
> -	captureResult.output_buffers = descriptor.buffers_.data();
> -
> -	if (status == CAMERA3_BUFFER_STATUS_OK) {
> -		uint64_t timestamp =
> -			static_cast<uint64_t>(request->metadata()
> -					      .get(controls::SensorTimestamp));
> -		notifyShutter(descriptor.frameNumber_, timestamp);
> -
> -		captureResult.partial_result = 1;
> -		captureResult.result = resultMetadata->get();
> -	}
> -
> -	if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
> -		/* \todo Improve error handling. In case we notify an error
> -		 * because the metadata generation fails, a shutter event has
> -		 * already been notified for this frame number before the error
> -		 * is here signalled. Make sure the error path plays well with
> -		 * the camera stack state machine.
> -		 */
> -		notifyError(descriptor.frameNumber_,
> -			    descriptor.buffers_[0].stream);
> +		if (ret) {
> +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +			notifyError(descriptor.frameNumber_, buffer.stream,
> +				    CAMERA3_MSG_ERROR_BUFFER);
> +		}
>  	}
>  
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
> @@ -2067,23 +2097,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
>  	callbacks_->notify(callbacks_, &notify);
>  }
>  
> -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> +			       camera3_error_msg_code code)
>  {
>  	camera3_notify_msg_t notify = {};
>  
> -	/*
> -	 * \todo Report and identify the stream number or configuration to
> -	 * clarify the stream that failed.
> -	 */
> -	LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
> -			<< toPixelFormat(stream->format).toString() << ")";
> -
>  	notify.type = CAMERA3_MSG_ERROR;
>  	notify.message.error.error_stream = stream;
>  	notify.message.error.frame_number = frameNumber;
> -	notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;
> +	notify.message.error.error_code = code;
>  
>  	callbacks_->notify(callbacks_, &notify);
> +
> +	if (stream)
> +		LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
> +				<< toPixelFormat(stream->format).toString() << ")";
> +	else
> +		LOG(HAL, Error) << "Fatal error occurred on device";
>  }
>  
>  /*
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index ae162a452499..a34e8a2cd900 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -100,7 +100,8 @@ private:
>  
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> -	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> +	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> +			 camera3_error_msg_code code);
>  	std::unique_ptr<CameraMetadata> requestTemplatePreview();
>  	std::unique_ptr<CameraMetadata> requestTemplateVideo();
>  	libcamera::PixelFormat toPixelFormat(int format) const;
> -- 
> 2.31.1
>

Hi Jacopo,

Thank you for the patch.

On Fri, May 21, 2021 at 05:42:20PM +0200, Jacopo Mondi wrote:
> The current implementation of CameraDevice::requestComplete() which
> handles event notification and calls the framework capture result
> callback does not handle error notification precisely enough.
> 
> In detail:
> - Error notification is an asynchronous callback that has to be notified
>   to the framework as soon as an error condition is detected, and it
>   independent from the process_capture_result() callback
> 
> - Error notification requires the HAL to report the precise error cause,
>   by specifying the correct CAMERA3_MSG_ERROR_* error code.
> 
> The current implementation only notifies errors of type
> CAMERA3_MSG_ERROR_REQUEST at the end of the procedure, before the
> callback invocation.
> 
> Rework the procedure to:
> 
> - Notify CAMERA3_MSG_ERROR_DEVICE and perform library tear-down in case
>   a Fatal error is detected
> 
> - Notify CAMERA3_MSG_ERROR_REQUEST if the libcamera::Request::status is
>   different than RequestCompleted and immediately call
>   process_capture_result() with all buffers in error state.
> 
> - Notify the shutter event as soon as possible
> 
> - Notify CAMERA3_MSG_ERROR_RESULT in case the metadata cannot be
>   generated correctly and call process_capture_result() with the right
>   buffer state regardless of metadata availability.
> 
> - Notify CAMERA3_MSG_ERROR_BUFFER for buffers whose post-processing
>   failed
> 
> While at it, return the CameraStream buffer by calling
> cameraStream->putBuffer() regardless of the post-processing result.
> 
> No regression detected when running CTS in LIMITED mode.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 138 +++++++++++++++++++++-------------
>  src/android/camera_device.h   |   3 +-
>  2 files changed, 86 insertions(+), 55 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b32e8be59134..bd96b355ea92 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1956,17 +1956,20 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  
>  void CameraDevice::requestComplete(Request *request)
>  {
> -	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> -	std::unique_ptr<CameraMetadata> resultMetadata;
> -
>  	decltype(descriptors_)::node_type node;
>  	{
>  		std::scoped_lock<std::mutex> lock(mutex_);
>  		auto it = descriptors_.find(request->cookie());
>  		if (it == descriptors_.end()) {
> +			/*
> +			 * \todo Clarify if the Camera has to be closed on
> +			 * ERROR_DEVICE and possibly demote the Fatal to simple
> +			 * Error.
> +			 */
> +			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
>  			LOG(HAL, Fatal)
>  				<< "Unknown request: " << request->cookie();

For the same reason as explained below, I'd move the LOG() before
notifyError().

> -			status = CAMERA3_BUFFER_STATUS_ERROR;
> +
>  			return;
>  		}
>  
> @@ -1974,16 +1977,72 @@ void CameraDevice::requestComplete(Request *request)
>  	}
>  	Camera3RequestDescriptor &descriptor = node.mapped();
>  
> +	/*
> +	 * Prepare the capture result for the Android camera stack.
> +	 *
> +	 * The buffer status is set to OK and later changed to ERROR if
> +	 * post-processing/compression fails.
> +	 */
> +	camera3_capture_result_t captureResult = {};
> +	captureResult.frame_number = descriptor.frameNumber_;
> +	captureResult.num_output_buffers = descriptor.buffers_.size();
> +	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> +		buffer.acquire_fence = -1;
> +		buffer.release_fence = -1;
> +		buffer.status = CAMERA3_BUFFER_STATUS_OK;
> +	}
> +	captureResult.output_buffers = descriptor.buffers_.data();
> +	captureResult.partial_result = 1;
> +
> +	/*
> +	 * If the Request has failed, abort the request by notifying the error
> +	 * and complete the request with all buffers in error state.
> +	 */
>  	if (request->status() != Request::RequestComplete) {
> -		LOG(HAL, Error) << "Request not successfully completed: "
> +		LOG(HAL, Error) << "Request " << request->cookie()
> +				<< " not successfully completed: "
>  				<< request->status();
> -		status = CAMERA3_BUFFER_STATUS_ERROR;
> +
> +		notifyError(descriptor.frameNumber_,
> +			    descriptor.buffers_[0].stream,

Shouldn't you pass nullptr for the stream pointer in this case ?

> +			    CAMERA3_MSG_ERROR_REQUEST);
> +
> +		captureResult.partial_result = 0;
> +		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +		callbacks_->process_capture_result(callbacks_, &captureResult);
> +
> +		return;
>  	}
>  
> +	/*
> +	 * Notify shutter as soon as we have verified we have a valid request.
> +	 *
> +	 * \todo The shutter event notification should be sent to the framework
> +	 * as soon as possible, earlier than request completion time.
> +	 */
> +	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> +							 .get(controls::SensorTimestamp));
> +	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> +
>  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
>  			<< descriptor.buffers_.size() << " streams";
>  
> -	resultMetadata = getResultMetadata(descriptor);
> +	/*
> +	 * Generate the metadata associated with the captured buffers.
> +	 *
> +	 * Notify if the metadata generation has failed, but continue processing
> +	 * buffers and return an empty metadata pack.
> +	 */
> +	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> +	if (!resultMetadata) {
> +		notifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,

Same here, nullptr for the stream ?

> +			    CAMERA3_MSG_ERROR_RESULT);
> +
> +		/* The camera framework expects an empy metadata pack on error. */
> +		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> +	}
> +	captureResult.result = resultMetadata->get();
>  
>  	/* Handle any JPEG compression. */
>  	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> @@ -1996,56 +2055,27 @@ void CameraDevice::requestComplete(Request *request)
>  		FrameBuffer *src = request->findBuffer(cameraStream->stream());
>  		if (!src) {
>  			LOG(HAL, Error) << "Failed to find a source stream buffer";
> +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +			notifyError(descriptor.frameNumber_, buffer.stream,
> +				    CAMERA3_MSG_ERROR_BUFFER);
>  			continue;
>  		}
>  
> -		int ret = cameraStream->process(*src,
> -						*buffer.buffer,
> +		int ret = cameraStream->process(*src, *buffer.buffer,
>  						descriptor.settings_,
>  						resultMetadata.get());
> -		if (ret) {
> -			status = CAMERA3_BUFFER_STATUS_ERROR;
> -			continue;
> -		}
> -
>  		/*
>  		 * Return the FrameBuffer to the CameraStream now that we're
>  		 * done processing it.
>  		 */
>  		if (cameraStream->type() == CameraStream::Type::Internal)
>  			cameraStream->putBuffer(src);
> -	}
>  
> -	/* Prepare to call back the Android camera stack. */
> -	camera3_capture_result_t captureResult = {};
> -	captureResult.frame_number = descriptor.frameNumber_;
> -	captureResult.num_output_buffers = descriptor.buffers_.size();
> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> -		buffer.acquire_fence = -1;
> -		buffer.release_fence = -1;
> -		buffer.status = status;
> -	}
> -	captureResult.output_buffers = descriptor.buffers_.data();
> -
> -	if (status == CAMERA3_BUFFER_STATUS_OK) {
> -		uint64_t timestamp =
> -			static_cast<uint64_t>(request->metadata()
> -					      .get(controls::SensorTimestamp));
> -		notifyShutter(descriptor.frameNumber_, timestamp);
> -
> -		captureResult.partial_result = 1;
> -		captureResult.result = resultMetadata->get();
> -	}
> -
> -	if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
> -		/* \todo Improve error handling. In case we notify an error
> -		 * because the metadata generation fails, a shutter event has
> -		 * already been notified for this frame number before the error
> -		 * is here signalled. Make sure the error path plays well with
> -		 * the camera stack state machine.
> -		 */
> -		notifyError(descriptor.frameNumber_,
> -			    descriptor.buffers_[0].stream);
> +		if (ret) {
> +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +			notifyError(descriptor.frameNumber_, buffer.stream,
> +				    CAMERA3_MSG_ERROR_BUFFER);
> +		}
>  	}
>  
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
> @@ -2067,23 +2097,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
>  	callbacks_->notify(callbacks_, &notify);
>  }
>  
> -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> +			       camera3_error_msg_code code)
>  {
>  	camera3_notify_msg_t notify = {};
>  
> -	/*
> -	 * \todo Report and identify the stream number or configuration to
> -	 * clarify the stream that failed.
> -	 */
> -	LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
> -			<< toPixelFormat(stream->format).toString() << ")";
> -
>  	notify.type = CAMERA3_MSG_ERROR;
>  	notify.message.error.error_stream = stream;
>  	notify.message.error.frame_number = frameNumber;
> -	notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;
> +	notify.message.error.error_code = code;
>  
>  	callbacks_->notify(callbacks_, &notify);
> +
> +	if (stream)
> +		LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
> +				<< toPixelFormat(stream->format).toString() << ")";
> +	else
> +		LOG(HAL, Error) << "Fatal error occurred on device";

I'd keep this at the top of the function. Otherwise, notifying the
camera server first, we could get a call from the camera service
synchronously that would result in other messages being logged before
our error message. For instance, we could have

	Closing device
	Fatal error occurred on device

The other way around would be better.

Very nice refactoring overall.

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

>  }
>  
>  /*
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index ae162a452499..a34e8a2cd900 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -100,7 +100,8 @@ private:
>  
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> -	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> +	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> +			 camera3_error_msg_code code);
>  	std::unique_ptr<CameraMetadata> requestTemplatePreview();
>  	std::unique_ptr<CameraMetadata> requestTemplateVideo();
>  	libcamera::PixelFormat toPixelFormat(int format) const;

Hi Laurent,

On Sun, May 23, 2021 at 08:50:32PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, May 21, 2021 at 05:42:20PM +0200, Jacopo Mondi wrote:
> > The current implementation of CameraDevice::requestComplete() which
> > handles event notification and calls the framework capture result
> > callback does not handle error notification precisely enough.
> >
> > In detail:
> > - Error notification is an asynchronous callback that has to be notified
> >   to the framework as soon as an error condition is detected, and it
> >   independent from the process_capture_result() callback
> >
> > - Error notification requires the HAL to report the precise error cause,
> >   by specifying the correct CAMERA3_MSG_ERROR_* error code.
> >
> > The current implementation only notifies errors of type
> > CAMERA3_MSG_ERROR_REQUEST at the end of the procedure, before the
> > callback invocation.
> >
> > Rework the procedure to:
> >
> > - Notify CAMERA3_MSG_ERROR_DEVICE and perform library tear-down in case
> >   a Fatal error is detected
> >
> > - Notify CAMERA3_MSG_ERROR_REQUEST if the libcamera::Request::status is
> >   different than RequestCompleted and immediately call
> >   process_capture_result() with all buffers in error state.
> >
> > - Notify the shutter event as soon as possible
> >
> > - Notify CAMERA3_MSG_ERROR_RESULT in case the metadata cannot be
> >   generated correctly and call process_capture_result() with the right
> >   buffer state regardless of metadata availability.
> >
> > - Notify CAMERA3_MSG_ERROR_BUFFER for buffers whose post-processing
> >   failed
> >
> > While at it, return the CameraStream buffer by calling
> > cameraStream->putBuffer() regardless of the post-processing result.
> >
> > No regression detected when running CTS in LIMITED mode.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_device.cpp | 138 +++++++++++++++++++++-------------
> >  src/android/camera_device.h   |   3 +-
> >  2 files changed, 86 insertions(+), 55 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index b32e8be59134..bd96b355ea92 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1956,17 +1956,20 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >
> >  void CameraDevice::requestComplete(Request *request)
> >  {
> > -	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> > -	std::unique_ptr<CameraMetadata> resultMetadata;
> > -
> >  	decltype(descriptors_)::node_type node;
> >  	{
> >  		std::scoped_lock<std::mutex> lock(mutex_);
> >  		auto it = descriptors_.find(request->cookie());
> >  		if (it == descriptors_.end()) {
> > +			/*
> > +			 * \todo Clarify if the Camera has to be closed on
> > +			 * ERROR_DEVICE and possibly demote the Fatal to simple
> > +			 * Error.
> > +			 */
> > +			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> >  			LOG(HAL, Fatal)
> >  				<< "Unknown request: " << request->cookie();
>
> For the same reason as explained below, I'd move the LOG() before
> notifyError().

I've taken all other suggestions in but this one. As the log is Fatal,
I would notify the framework first.

Thanks
   j

>
> > -			status = CAMERA3_BUFFER_STATUS_ERROR;
> > +
> >  			return;
> >  		}
> >
> > @@ -1974,16 +1977,72 @@ void CameraDevice::requestComplete(Request *request)
> >  	}
> >  	Camera3RequestDescriptor &descriptor = node.mapped();
> >
> > +	/*
> > +	 * Prepare the capture result for the Android camera stack.
> > +	 *
> > +	 * The buffer status is set to OK and later changed to ERROR if
> > +	 * post-processing/compression fails.
> > +	 */
> > +	camera3_capture_result_t captureResult = {};
> > +	captureResult.frame_number = descriptor.frameNumber_;
> > +	captureResult.num_output_buffers = descriptor.buffers_.size();
> > +	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > +		buffer.acquire_fence = -1;
> > +		buffer.release_fence = -1;
> > +		buffer.status = CAMERA3_BUFFER_STATUS_OK;
> > +	}
> > +	captureResult.output_buffers = descriptor.buffers_.data();
> > +	captureResult.partial_result = 1;
> > +
> > +	/*
> > +	 * If the Request has failed, abort the request by notifying the error
> > +	 * and complete the request with all buffers in error state.
> > +	 */
> >  	if (request->status() != Request::RequestComplete) {
> > -		LOG(HAL, Error) << "Request not successfully completed: "
> > +		LOG(HAL, Error) << "Request " << request->cookie()
> > +				<< " not successfully completed: "
> >  				<< request->status();
> > -		status = CAMERA3_BUFFER_STATUS_ERROR;
> > +
> > +		notifyError(descriptor.frameNumber_,
> > +			    descriptor.buffers_[0].stream,
>
> Shouldn't you pass nullptr for the stream pointer in this case ?
>
> > +			    CAMERA3_MSG_ERROR_REQUEST);
> > +
> > +		captureResult.partial_result = 0;
> > +		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> > +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +		callbacks_->process_capture_result(callbacks_, &captureResult);
> > +
> > +		return;
> >  	}
> >
> > +	/*
> > +	 * Notify shutter as soon as we have verified we have a valid request.
> > +	 *
> > +	 * \todo The shutter event notification should be sent to the framework
> > +	 * as soon as possible, earlier than request completion time.
> > +	 */
> > +	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> > +							 .get(controls::SensorTimestamp));
> > +	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> > +
> >  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> >  			<< descriptor.buffers_.size() << " streams";
> >
> > -	resultMetadata = getResultMetadata(descriptor);
> > +	/*
> > +	 * Generate the metadata associated with the captured buffers.
> > +	 *
> > +	 * Notify if the metadata generation has failed, but continue processing
> > +	 * buffers and return an empty metadata pack.
> > +	 */
> > +	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> > +	if (!resultMetadata) {
> > +		notifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,
>
> Same here, nullptr for the stream ?
>
> > +			    CAMERA3_MSG_ERROR_RESULT);
> > +
> > +		/* The camera framework expects an empy metadata pack on error. */
> > +		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> > +	}
> > +	captureResult.result = resultMetadata->get();
> >
> >  	/* Handle any JPEG compression. */
> >  	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > @@ -1996,56 +2055,27 @@ void CameraDevice::requestComplete(Request *request)
> >  		FrameBuffer *src = request->findBuffer(cameraStream->stream());
> >  		if (!src) {
> >  			LOG(HAL, Error) << "Failed to find a source stream buffer";
> > +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +			notifyError(descriptor.frameNumber_, buffer.stream,
> > +				    CAMERA3_MSG_ERROR_BUFFER);
> >  			continue;
> >  		}
> >
> > -		int ret = cameraStream->process(*src,
> > -						*buffer.buffer,
> > +		int ret = cameraStream->process(*src, *buffer.buffer,
> >  						descriptor.settings_,
> >  						resultMetadata.get());
> > -		if (ret) {
> > -			status = CAMERA3_BUFFER_STATUS_ERROR;
> > -			continue;
> > -		}
> > -
> >  		/*
> >  		 * Return the FrameBuffer to the CameraStream now that we're
> >  		 * done processing it.
> >  		 */
> >  		if (cameraStream->type() == CameraStream::Type::Internal)
> >  			cameraStream->putBuffer(src);
> > -	}
> >
> > -	/* Prepare to call back the Android camera stack. */
> > -	camera3_capture_result_t captureResult = {};
> > -	captureResult.frame_number = descriptor.frameNumber_;
> > -	captureResult.num_output_buffers = descriptor.buffers_.size();
> > -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > -		buffer.acquire_fence = -1;
> > -		buffer.release_fence = -1;
> > -		buffer.status = status;
> > -	}
> > -	captureResult.output_buffers = descriptor.buffers_.data();
> > -
> > -	if (status == CAMERA3_BUFFER_STATUS_OK) {
> > -		uint64_t timestamp =
> > -			static_cast<uint64_t>(request->metadata()
> > -					      .get(controls::SensorTimestamp));
> > -		notifyShutter(descriptor.frameNumber_, timestamp);
> > -
> > -		captureResult.partial_result = 1;
> > -		captureResult.result = resultMetadata->get();
> > -	}
> > -
> > -	if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
> > -		/* \todo Improve error handling. In case we notify an error
> > -		 * because the metadata generation fails, a shutter event has
> > -		 * already been notified for this frame number before the error
> > -		 * is here signalled. Make sure the error path plays well with
> > -		 * the camera stack state machine.
> > -		 */
> > -		notifyError(descriptor.frameNumber_,
> > -			    descriptor.buffers_[0].stream);
> > +		if (ret) {
> > +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +			notifyError(descriptor.frameNumber_, buffer.stream,
> > +				    CAMERA3_MSG_ERROR_BUFFER);
> > +		}
> >  	}
> >
> >  	callbacks_->process_capture_result(callbacks_, &captureResult);
> > @@ -2067,23 +2097,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> >  	callbacks_->notify(callbacks_, &notify);
> >  }
> >
> > -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> > +			       camera3_error_msg_code code)
> >  {
> >  	camera3_notify_msg_t notify = {};
> >
> > -	/*
> > -	 * \todo Report and identify the stream number or configuration to
> > -	 * clarify the stream that failed.
> > -	 */
> > -	LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
> > -			<< toPixelFormat(stream->format).toString() << ")";
> > -
> >  	notify.type = CAMERA3_MSG_ERROR;
> >  	notify.message.error.error_stream = stream;
> >  	notify.message.error.frame_number = frameNumber;
> > -	notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;
> > +	notify.message.error.error_code = code;
> >
> >  	callbacks_->notify(callbacks_, &notify);
> > +
> > +	if (stream)
> > +		LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
> > +				<< toPixelFormat(stream->format).toString() << ")";
> > +	else
> > +		LOG(HAL, Error) << "Fatal error occurred on device";
>
> I'd keep this at the top of the function. Otherwise, notifying the
> camera server first, we could get a call from the camera service
> synchronously that would result in other messages being logged before
> our error message. For instance, we could have
>
> 	Closing device
> 	Fatal error occurred on device
>
> The other way around would be better.
>
> Very nice refactoring overall.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >  }
> >
> >  /*
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index ae162a452499..a34e8a2cd900 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -100,7 +100,8 @@ private:
> >
> >  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> >  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > -	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > +	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> > +			 camera3_error_msg_code code);
> >  	std::unique_ptr<CameraMetadata> requestTemplatePreview();
> >  	std::unique_ptr<CameraMetadata> requestTemplateVideo();
> >  	libcamera::PixelFormat toPixelFormat(int format) const;
>
> --
> Regards,
>
> Laurent Pinchart