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

[Jacopo Mondi]

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