[libcamera-devel] [PATCH v2] android: Rework request completion notification
Hirokazu Honda
hiroh at chromium.org
Fri May 7 05:50:13 CEST 2021
Hi Jacopo, thank you for the patch.
On Fri, May 7, 2021 at 12:00 AM Jacopo Mondi <jacopo at jmondi.org> 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>
> ---
>
> v2:
> - Rework a comment as suggested by Niklas
> - As Niklas reported, dereferencing a unique_ptr<> which owns a nullptr has
> an undefined behaviour. Replace that pattern by resetting the
> resultMetadata
> unique_ptr<> with a new one holding an empty CameraMetadata, as the
> framework
> requires on error.
>
> camera3.h:
> * If there was an error producing the result metadata, result must be
> an
> * empty metadata buffer, and notify() must be called with
> ERROR_RESULT.
>
>
> 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();
>
> ---
> src/android/camera_device.cpp | 132 ++++++++++++++++++++--------------
> src/android/camera_device.h | 3 +-
> 2 files changed, 80 insertions(+), 55 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f1f26c775611..ea7ec8c05f88 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();
>
close() calls Camera::stop(). Camera::stop() cancels the pending request
and requestComplete() will be invoked. (this is not done so cf.
https://patchwork.libcamera.org/project/libcamera/list/?series=1954)
Furthermore, Camera::stop() is a blocking function. So I think calling
close() here possibly causes the infinite-loop, e.g. when descriptors_ is
somehow empty.
Ah, but LOG(HAL, Fatal) causes the crash libcamera process, hmm..
> + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> return;
> }
>
> @@ -2043,16 +2041,71 @@ 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);
> +
> + 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_) {
> @@ -2065,56 +2118,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;
>
Is this still continue? Perhaps return?
> }
>
> - 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);
>
same here.
-Hiro
+ }
> }
>
> callbacks_->process_capture_result(callbacks_, &captureResult);
> @@ -2136,23 +2160,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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210507/c5fd4de8/attachment-0001.htm>
More information about the libcamera-devel
mailing list