[libcamera-devel] [PATCH v4 4/4] android: camera_device: Send capture results by inspecting the queue
Jacopo Mondi
jacopo at jmondi.org
Wed Sep 29 20:50:26 CEST 2021
Hi Umang,
On Wed, Sep 29, 2021 at 07:00:30PM +0530, Umang Jain wrote:
> There is a possibility that an out-of-order completion of capture
> request happens by calling process_capture_result() directly on error
> paths. The framework expects that errors should be notified as soon as
> possible, but the request completion order should remain intact.
> An existing instance of this is abortRequest(), which sends the capture
> results on flushing state, without considering order-of-completion.
>
> Since we have a queue of Camera3RequestDescriptor tracking each
> capture request placed by framework to libcamera HAL, we should be only
> sending back capture results from a single location, by inspecting
> the queue. As per the patch, this now happens in
> CameraDevice::sendCaptureResults().
>
> Each descriptor is now equipped with its own status to denote whether
> the capture request is complete and ready to send back to the framework
ready to be sent
> or needs to be waited upon. This ensures that the order of completion is
> respected for the requests.
>
> Since we are fixing out-of-order request completion in abortRequest(),
> change the function to read from the Camera3RequestDescriptor directly,
> instead of camera3_capture_request_t. The descriptor should have all the
> information necessary to set the request buffers' state to error.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> src/android/camera_device.cpp | 59 ++++++++++++++++++++++++-----------
> src/android/camera_device.h | 14 ++++++++-
> 2 files changed, 54 insertions(+), 19 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 45350563..38c64bb7 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> return 0;
> }
>
> -void CameraDevice::abortRequest(camera3_capture_request_t *request) const
> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> {
> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>
> - camera3_capture_result_t result = {};
> - result.num_output_buffers = request->num_output_buffers;
> - result.frame_number = request->frame_number;
> + camera3_capture_result_t &result = descriptor->captureResult_;
> + result.num_output_buffers = descriptor->buffers_.size();
> + result.frame_number = descriptor->frameNumber_;
> result.partial_result = 0;
>
> std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);
> for (auto [i, buffer] : utils::enumerate(resultBuffers)) {
> - buffer = request->output_buffers[i];
> - buffer.release_fence = request->output_buffers[i].acquire_fence;
> + buffer = descriptor->buffers_[i];
> + buffer.release_fence = descriptor->buffers_[i].acquire_fence;
> buffer.acquire_fence = -1;
> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> }
> result.output_buffers = resultBuffers.data();
>
> - callbacks_->process_capture_result(callbacks_, &result);
> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> }
>
> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> @@ -1050,13 +1050,21 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> return ret;
>
> /*
> - * If flush is in progress abort the request. If the camera has been
> - * stopped we have to re-start it to be able to process the request.
> + * If flush is in progress set the request status to error and place it
> + * on the queue to be later completed. If the camera has been stopped we
> + * have to re-start it to be able to process the request.
> */
> MutexLocker stateLock(stateMutex_);
>
> if (state_ == State::Flushing) {
> - abortRequest(camera3Request);
> + abortRequest(descriptor.get());
> + {
> + MutexLocker descriptorsLock(descriptorsMutex_);
> + descriptors_.push(std::move(descriptor));
> + }
> +
> + sendCaptureResults();
> +
> return 0;
> }
>
> @@ -1116,7 +1124,7 @@ void CameraDevice::requestComplete(Request *request)
> * The buffer status is set to OK and later changed to ERROR if
> * post-processing/compression fails.
> */
> - camera3_capture_result_t captureResult = {};
> + camera3_capture_result_t &captureResult = descriptor->captureResult_;
> captureResult.frame_number = descriptor->frameNumber_;
> captureResult.num_output_buffers = descriptor->buffers_.size();
> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> @@ -1166,10 +1174,9 @@ void CameraDevice::requestComplete(Request *request)
> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> }
>
> - callbacks_->process_capture_result(callbacks_, &captureResult);
> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> + sendCaptureResults();
>
> - MutexLocker descriptorsLock(descriptorsMutex_);
> - descriptors_.pop();
> return;
> }
>
> @@ -1235,10 +1242,26 @@ void CameraDevice::requestComplete(Request *request)
> }
>
> captureResult.result = resultMetadata->get();
> - callbacks_->process_capture_result(callbacks_, &captureResult);
> + descriptor->status_ = Camera3RequestDescriptor::Status::Success;
> + sendCaptureResults();
> +}
>
> - MutexLocker descriptorsLock(descriptorsMutex_);
> - descriptors_.pop();
> +void CameraDevice::sendCaptureResults()
> +{
> + MutexLocker lock(descriptorsMutex_);
> + while (!descriptors_.empty() && !descriptors_.front()->isPending()) {
> + auto descriptor = std::move(descriptors_.front());
> + descriptors_.pop();
> +
> + /*
> + * \todo Measure performance impact of grain locking here against
> + * coarse locking.
I find this a bit hard to parse. Maybe
* \todo Releasing and re-acquiring the critical
* section for every request completion might have an
* impact on performaces which should be measured.
Up to you, really.
All minors might be optionally fixed when applying
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> + */
> + lock.unlock();
> + callbacks_->process_capture_result(callbacks_,
> + &descriptor->captureResult_);
> + lock.lock();
> + }
> }
>
> std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 85497921..b7d774fe 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -74,17 +74,28 @@ private:
> CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
>
> struct Camera3RequestDescriptor {
> + enum class Status {
> + Pending,
> + Success,
> + Error,
> + };
> +
> Camera3RequestDescriptor() = default;
> ~Camera3RequestDescriptor() = default;
> Camera3RequestDescriptor(libcamera::Camera *camera,
> const camera3_capture_request_t *camera3Request);
> Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
>
> + bool isPending() const { return status_ == Status::Pending; }
> +
> uint32_t frameNumber_ = 0;
> std::vector<camera3_stream_buffer_t> buffers_;
> std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> CameraMetadata settings_;
> std::unique_ptr<CaptureRequest> request_;
> +
> + camera3_capture_result_t captureResult_ = {};
> + Status status_ = Status::Pending;
> };
>
> enum class State {
> @@ -99,12 +110,13 @@ private:
> createFrameBuffer(const buffer_handle_t camera3buffer,
> libcamera::PixelFormat pixelFormat,
> const libcamera::Size &size);
> - void abortRequest(camera3_capture_request_t *request) const;
> + void abortRequest(Camera3RequestDescriptor *descriptor) const;
> bool isValidRequest(camera3_capture_request_t *request) const;
> void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> camera3_error_msg_code code) const;
> int processControls(Camera3RequestDescriptor *descriptor);
> + void sendCaptureResults();
> std::unique_ptr<CameraMetadata> getResultMetadata(
> const Camera3RequestDescriptor &descriptor) const;
>
> --
> 2.31.0
>
More information about the libcamera-devel
mailing list