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

Niklas Söderlund niklas.soderlund at ragnatech.se
Sat May 22 11:21:39 CEST 2021


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 at jmondi.org>
> Reviewed-by: Hirokazu Honda <hiroh at 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 at 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
> 

-- 
Regards,
Niklas Söderlund


More information about the libcamera-devel mailing list