[libcamera-devel] [PATCH] android: Rework request completion notification
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu May 6 10:31:10 CEST 2021
Hi Jacopo,
Thanks for your work.
On 2021-05-04 17:58:08 +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 close the camera in case a Fatal
> error is detected (Fatal does not perform library teardown in
> production builds)
>
> - 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>
> ---
> src/android/camera_device.cpp | 128 ++++++++++++++++++++--------------
> src/android/camera_device.h | 3 +-
> 2 files changed, 76 insertions(+), 55 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f1f26c775611..5588e99b6eba 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -2025,9 +2025,6 @@ 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_);
> @@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request *request)
> if (it == descriptors_.end()) {
> LOG(HAL, Fatal)
> << "Unknown request: " << request->cookie();
> - status = CAMERA3_BUFFER_STATUS_ERROR;
> + close();
> + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> return;
> }
>
> @@ -2043,16 +2041,67 @@ void CameraDevice::requestComplete(Request *request)
> }
> Camera3RequestDescriptor &descriptor = node.mapped();
>
> + /*
> + * Prepare the capture result to call back the Android camera stack with.
nit: 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);
> +
> + 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);
> + captureResult.result = resultMetadata->get();
Hum if !resultMetadata will not resultMetadata->get() give us a nullptr
exception here?
>
> /* Handle any JPEG compression. */
> for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> @@ -2065,56 +2114,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);
> @@ -2136,23 +2156,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> callbacks_->notify(callbacks_, ¬ify);
> }
>
> -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_, ¬ify);
> +
> + 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 23457e47767a..8d5da8bc59e1 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -101,7 +101,8 @@ private:
> std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list