From patchwork Tue Sep 28 16:25:36 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 13972 X-Patchwork-Delegate: umang.jain@ideasonboard.com Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id EC4FDC3243 for ; Tue, 28 Sep 2021 16:26:22 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id ACA1869191; Tue, 28 Sep 2021 18:26:22 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="X+tELMke"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E6A469188 for ; Tue, 28 Sep 2021 18:26:22 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [IPv6:2405:204:8500:b152:4879:ab55:a9fb:2b15]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 3FC683F1; Tue, 28 Sep 2021 18:26:06 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1632846381; bh=wHxoNWgcPVtkASppd4qr+E6KCcn4LDJ/b6FMWSNmLQg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=X+tELMkeztq0Qdy2kIgRAikVEYfwjXAmcYpOS4MyEVsZG6qzM9DxNE+J+bj2MW9Bs qYj24kY4fVgVkSSrXnTnEttyHWRE1wc0q3O8xgTHf5rXpHeozAbJ6MgP0XhxMC+ZLB nMZMKRsVKl5V4NIu8pFdsBw8RfrXCj8xleVUW4z8= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Tue, 28 Sep 2021 21:55:36 +0530 Message-Id: <20210928162536.227475-4-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210928162536.227475-1-umang.jain@ideasonboard.com> References: <20210928162536.227475-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 3/3] android: camera_device: Send capture results by inspecting the queue X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 or needs to be waited upon. This ensures that the order of completion is respected for the requests. Signed-off-by: Umang Jain Reviewed-by: Laurent Pinchart --- src/android/camera_device.cpp | 44 +++++++++++++++++++++++++---------- src/android/camera_device.h | 15 +++++++++++- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index a3b8a549..83030366 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -861,11 +861,12 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) return 0; } -void CameraDevice::abortRequest(camera3_capture_request_t *request) +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor, + camera3_capture_request_t *request) { notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); - camera3_capture_result_t result = {}; + camera3_capture_result_t &result = descriptor->captureResult_; result.num_output_buffers = request->num_output_buffers; result.frame_number = request->frame_number; result.partial_result = 0; @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request) } 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 @@ -1052,13 +1053,19 @@ 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 abort the request and push the descriptor to + * the queue. 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(descriptors_.back().get(), camera3Request); + { + MutexLocker descriptorsLock(descriptorsMutex_); + descriptors_.push(std::move(descriptor)); + } + sendCaptureResults(); return 0; } @@ -1116,7 +1123,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,9 +1173,9 @@ void CameraDevice::requestComplete(Request *request) buffer.status = CAMERA3_BUFFER_STATUS_ERROR; } - callbacks_->process_capture_result(callbacks_, &captureResult); + descriptor->status_ = Camera3RequestDescriptor::Status::Error; + sendCaptureResults(); - descriptors_.pop(); return; } @@ -1234,10 +1241,23 @@ 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()) { + std::unique_ptr descriptor = + std::move(descriptors_.front()); + descriptors_.pop(); + + 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 9ec510d5..dbfa7431 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 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 buffers_; std::vector> frameBuffers_; CameraMetadata settings_; std::unique_ptr request_; + + camera3_capture_result_t captureResult_ = {}; + Status status_ = Status::Pending; }; enum class State { @@ -99,12 +110,14 @@ private: createFrameBuffer(const buffer_handle_t camera3buffer, libcamera::PixelFormat pixelFormat, const libcamera::Size &size); - void abortRequest(camera3_capture_request_t *request); + void abortRequest(Camera3RequestDescriptor *descriptor, + camera3_capture_request_t *request); 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); int processControls(Camera3RequestDescriptor *descriptor); + void sendCaptureResults(); std::unique_ptr getResultMetadata( const Camera3RequestDescriptor &descriptor) const;