From patchwork Wed Sep 29 13:30:27 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 13979 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 ED839C3243 for ; Wed, 29 Sep 2021 13:30:42 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id B4F93691AF; Wed, 29 Sep 2021 15:30:42 +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="Gk2CeTCV"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 339DB6918F for ; Wed, 29 Sep 2021 15:30:41 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.9]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 84BBA3F0; Wed, 29 Sep 2021 15:30:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1632922240; bh=R64R4rEVFjSz/TePOq9XUeQVBzYRoU7JxHp9gBe/Cmo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Gk2CeTCV4YGHlBwZ78gkfRJQW5nePgCI4vc0d2jb/FvgsHri4D7/PbyBaeXEjo+AK QxZb9qkK++CQFAIiCbDHkHo7POc17MnePOd30ycna11aHl0p5J1bLHrdsnsVqiNWDD hU7f6aEaeB9EIRSjJp2dCnaE2Faq+xLLF26xkpr4= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 29 Sep 2021 19:00:27 +0530 Message-Id: <20210929133030.401961-2-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210929133030.401961-1-umang.jain@ideasonboard.com> References: <20210929133030.401961-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v4 1/4] android: camera_worker: Use Camera3RequestDescriptor as cookie 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" Use Camera3RequestDescriptor as cookie for the Capture Request. The cookie is used to lookup descriptors map in CameraDevice::requestComplete(). The map will be transformed to a queue in subsequent commit. Signed-off-by: Umang Jain Reviewed-by: Laurent Pinchart Reviewed-by: Hirokazu Honda Reviewed-by: Jacopo Mondi --- src/android/camera_device.cpp | 3 ++- src/android/camera_worker.cpp | 4 ++-- src/android/camera_worker.h | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 0fe11fe8..9287ea07 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -244,7 +244,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor( * Create the CaptureRequest, stored as a unique_ptr<> to tie its * lifetime to the descriptor. */ - request_ = std::make_unique(camera); + request_ = std::make_unique(camera, + reinterpret_cast(this)); } /* diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp index 91313183..dabb3054 100644 --- a/src/android/camera_worker.cpp +++ b/src/android/camera_worker.cpp @@ -27,10 +27,10 @@ LOG_DECLARE_CATEGORY(HAL) * by the CameraWorker which queues it to the libcamera::Camera after handling * fences. */ -CaptureRequest::CaptureRequest(Camera *camera) +CaptureRequest::CaptureRequest(Camera *camera, uint64_t cookie) : camera_(camera) { - request_ = camera_->createRequest(reinterpret_cast(this)); + request_ = camera_->createRequest(cookie); } void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence) diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h index 67ae50bd..c94f1632 100644 --- a/src/android/camera_worker.h +++ b/src/android/camera_worker.h @@ -8,6 +8,7 @@ #define __ANDROID_CAMERA_WORKER_H__ #include +#include #include #include @@ -22,7 +23,7 @@ class CameraDevice; class CaptureRequest { public: - CaptureRequest(libcamera::Camera *camera); + CaptureRequest(libcamera::Camera *camera, uint64_t cookie); const std::vector &fences() const { return acquireFences_; } libcamera::ControlList &controls() { return request_->controls(); } From patchwork Wed Sep 29 13:30:28 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 13980 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 700F2C3243 for ; Wed, 29 Sep 2021 13:30:45 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 37737691B3; Wed, 29 Sep 2021 15:30:45 +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="XbLulT7T"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3CBFD69185 for ; Wed, 29 Sep 2021 15:30:43 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.9]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id DBAAD3F0; Wed, 29 Sep 2021 15:30:41 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1632922242; bh=7ZyppM6edO48Cu3Y23/5fQf52bQTRLnMPptnujUKfuU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=XbLulT7TYAnz8vqcIYfe0L34swI52g3jAiM4TCKGVR9PyimeJyha4u18rJepAKDLz RUgGkMBlOShKasK/tP6vp7fS2FpOeUKOa3AstCOPxitYOaRQSS9bWV/6IsURtwm/Ve N47icpsSdV8R60dFJ30KK7QUzzwTom+6EZ8u9ElA= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 29 Sep 2021 19:00:28 +0530 Message-Id: <20210929133030.401961-3-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210929133030.401961-1-umang.jain@ideasonboard.com> References: <20210929133030.401961-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v4 2/4] android: camera_device: Transform descriptors_ map to 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" The descriptors_ map holds Camera3RequestDescriptor(s) which are per-capture requests placed by the framework to libcamera HAL. CameraDevice::requestComplete() looks for the descriptor for which the camera request has been completed and removes it from the map. Since the requests are placed in form of FIFO and the framework expects the order of completion to be FIFO as well, this calls for a need of a queue rather than a std::map. This patch still keeps the same lifetime of Camera3RequestDescriptor as before i.e. in the requestComplete(). Previously, a descriptor was extracted from the map and its lifetime was bound to requestComplete(). The lifetime is kept same by manually calling .pop_front() on the queue. In the subsequent commit, this is likely to change with a centralized location of dropping descriptors from the queue for request completion. Signed-off-by: Umang Jain Reviewed-by: Laurent Pinchart Reviewed-by: Jacopo Mondi --- src/android/camera_device.cpp | 93 +++++++++++++++++++---------------- src/android/camera_device.h | 3 +- 2 files changed, 53 insertions(+), 43 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 9287ea07..e815f7a0 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -457,7 +457,8 @@ void CameraDevice::stop() worker_.stop(); camera_->stop(); - descriptors_.clear(); + descriptors_ = {}; + state_ = State::Stopped; } @@ -955,7 +956,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * The descriptor and the associated memory reserved here are freed * at request complete time. */ - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request); + auto descriptor = std::make_unique(camera_.get(), + camera3Request); /* * \todo The Android request model is incremental, settings passed in @@ -966,12 +968,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques if (camera3Request->settings) lastSettings_ = camera3Request->settings; else - descriptor.settings_ = lastSettings_; + descriptor->settings_ = lastSettings_; + + LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() + << " with " << descriptor->buffers_.size() << " streams"; - LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie() - << " with " << descriptor.buffers_.size() << " streams"; - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; + for (const auto &[i, camera3Buffer] : utils::enumerate(descriptor->buffers_)) { camera3_stream *camera3Stream = camera3Buffer.stream; CameraStream *cameraStream = static_cast(camera3Stream->priv); @@ -1007,12 +1009,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * associate it with the Camera3RequestDescriptor for * lifetime management only. */ - descriptor.frameBuffers_.push_back( + descriptor->frameBuffers_.push_back( createFrameBuffer(*camera3Buffer.buffer, cameraStream->configuration().pixelFormat, cameraStream->configuration().size)); - buffer = descriptor.frameBuffers_.back().get(); + buffer = descriptor->frameBuffers_.back().get(); acquireFence = camera3Buffer.acquire_fence; LOG(HAL, Debug) << ss.str() << " (direct)"; break; @@ -1035,7 +1037,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques return -ENOMEM; } - descriptor.request_->addBuffer(cameraStream->stream(), buffer, + descriptor->request_->addBuffer(cameraStream->stream(), buffer, acquireFence); } @@ -1043,7 +1045,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * Translate controls from Android to libcamera and queue the request * to the CameraWorker thread. */ - int ret = processControls(&descriptor); + int ret = processControls(descriptor.get()); if (ret) return ret; @@ -1071,11 +1073,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques state_ = State::Running; } - CaptureRequest *request = descriptor.request_.get(); + CaptureRequest *request = descriptor->request_.get(); { MutexLocker descriptorsLock(descriptorsMutex_); - descriptors_[descriptor.request_->cookie()] = std::move(descriptor); + descriptors_.push(std::move(descriptor)); } worker_.queueRequest(request); @@ -1085,26 +1087,28 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques void CameraDevice::requestComplete(Request *request) { - decltype(descriptors_)::node_type node; + Camera3RequestDescriptor *descriptor; { MutexLocker descriptorsLock(descriptorsMutex_); - 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(); + ASSERT(!descriptors_.empty()); + descriptor = descriptors_.front().get(); + } - return; - } + if (descriptor->request_->cookie() != request->cookie()) { + /* + * \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) + << "Out-of-order completion for request " + << utils::hex(request->cookie()); - node = descriptors_.extract(it); + MutexLocker descriptorsLock(descriptorsMutex_); + descriptors_.pop(); + return; } - Camera3RequestDescriptor &descriptor = node.mapped(); /* * Prepare the capture result for the Android camera stack. @@ -1113,9 +1117,9 @@ void CameraDevice::requestComplete(Request *request) * 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_) { + captureResult.frame_number = descriptor->frameNumber_; + captureResult.num_output_buffers = descriptor->buffers_.size(); + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { CameraStream *cameraStream = static_cast(buffer.stream->priv); @@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request *request) buffer.release_fence = -1; buffer.status = CAMERA3_BUFFER_STATUS_OK; } - captureResult.output_buffers = descriptor.buffers_.data(); + captureResult.output_buffers = descriptor->buffers_.data(); captureResult.partial_result = 1; /* @@ -1147,11 +1151,11 @@ void CameraDevice::requestComplete(Request *request) << " not successfully completed: " << request->status(); - notifyError(descriptor.frameNumber_, nullptr, + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); captureResult.partial_result = 0; - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { /* * Signal to the framework it has to handle fences that * have not been waited on by setting the release fence @@ -1164,6 +1168,8 @@ void CameraDevice::requestComplete(Request *request) callbacks_->process_capture_result(callbacks_, &captureResult); + MutexLocker descriptorsLock(descriptorsMutex_); + descriptors_.pop(); return; } @@ -1175,10 +1181,10 @@ void CameraDevice::requestComplete(Request *request) */ uint64_t sensorTimestamp = static_cast(request->metadata() .get(controls::SensorTimestamp)); - notifyShutter(descriptor.frameNumber_, sensorTimestamp); + notifyShutter(descriptor->frameNumber_, sensorTimestamp); LOG(HAL, Debug) << "Request " << request->cookie() << " completed with " - << descriptor.buffers_.size() << " streams"; + << descriptor->buffers_.size() << " streams"; /* * Generate the metadata associated with the captured buffers. @@ -1186,16 +1192,16 @@ void CameraDevice::requestComplete(Request *request) * Notify if the metadata generation has failed, but continue processing * buffers and return an empty metadata pack. */ - std::unique_ptr resultMetadata = getResultMetadata(descriptor); + std::unique_ptr resultMetadata = getResultMetadata(*descriptor); if (!resultMetadata) { - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); /* The camera framework expects an empty metadata pack on error. */ resultMetadata = std::make_unique(0, 0); } /* Handle post-processing. */ - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { CameraStream *cameraStream = static_cast(buffer.stream->priv); @@ -1206,13 +1212,13 @@ void CameraDevice::requestComplete(Request *request) if (!src) { LOG(HAL, Error) << "Failed to find a source stream buffer"; buffer.status = CAMERA3_BUFFER_STATUS_ERROR; - notifyError(descriptor.frameNumber_, buffer.stream, + notifyError(descriptor->frameNumber_, buffer.stream, CAMERA3_MSG_ERROR_BUFFER); continue; } int ret = cameraStream->process(*src, buffer, - descriptor.settings_, + descriptor->settings_, resultMetadata.get()); /* * Return the FrameBuffer to the CameraStream now that we're @@ -1223,13 +1229,16 @@ void CameraDevice::requestComplete(Request *request) if (ret) { buffer.status = CAMERA3_BUFFER_STATUS_ERROR; - notifyError(descriptor.frameNumber_, buffer.stream, + notifyError(descriptor->frameNumber_, buffer.stream, CAMERA3_MSG_ERROR_BUFFER); } } captureResult.result = resultMetadata->get(); callbacks_->process_capture_result(callbacks_, &captureResult); + + MutexLocker descriptorsLock(descriptorsMutex_); + descriptors_.pop(); } std::string CameraDevice::logPrefix() const diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 43eb5895..9ec510d5 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -125,7 +126,7 @@ private: std::vector streams_; libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ - std::map descriptors_; + std::queue> descriptors_; std::string maker_; std::string model_; From patchwork Wed Sep 29 13:30:29 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 13981 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 0C766C3243 for ; Wed, 29 Sep 2021 13:30:47 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id D439E691B7; Wed, 29 Sep 2021 15:30:46 +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="Tr6nGDQD"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id EB94A691A3 for ; Wed, 29 Sep 2021 15:30:44 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.9]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id CFA4F3F0; Wed, 29 Sep 2021 15:30:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1632922244; bh=HF2RWqvIo4vAjF6D7HhHAwYGz2JFm7Rf7UHkRkaVYUo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Tr6nGDQDg3h8m5cQl/sQRLeMwMHJIB4FWHxz9DU4YBdxRc6Kw4uFYaClHjapDgRRw ugvFuzAGHrfftBWXAznxGvOFLoR6hepnKuKusE8juSM2owf9qSlyAOafLCSKQ8Sz/c knovSIVABalUmhB8Oe+vFjA8uqEZ7nSxiOpJIcak= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 29 Sep 2021 19:00:29 +0530 Message-Id: <20210929133030.401961-4-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210929133030.401961-1-umang.jain@ideasonboard.com> References: <20210929133030.401961-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v4 3/4] android: camera_device: Mark abortRequest() and notifyError() as const 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" abortRequest() and notifyError() do not access any members of CameraDevice hence, these functions can be const. Signed-off-by: Umang Jain Reviewed-by: Laurent Pinchart Reviewed-by: Jacopo Mondi --- src/android/camera_device.cpp | 4 ++-- src/android/camera_device.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index e815f7a0..45350563 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -860,7 +860,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) return 0; } -void CameraDevice::abortRequest(camera3_capture_request_t *request) +void CameraDevice::abortRequest(camera3_capture_request_t *request) const { notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); @@ -1258,7 +1258,7 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp) } void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream, - camera3_error_msg_code code) + camera3_error_msg_code code) const { camera3_notify_msg_t notify = {}; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 9ec510d5..85497921 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -99,11 +99,11 @@ private: createFrameBuffer(const buffer_handle_t camera3buffer, libcamera::PixelFormat pixelFormat, const libcamera::Size &size); - void abortRequest(camera3_capture_request_t *request); + void abortRequest(camera3_capture_request_t *request) 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); + camera3_error_msg_code code) const; int processControls(Camera3RequestDescriptor *descriptor); std::unique_ptr getResultMetadata( const Camera3RequestDescriptor &descriptor) const; From patchwork Wed Sep 29 13:30:30 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 13982 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 56AE4C3243 for ; Wed, 29 Sep 2021 13:30:49 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 27CD7691AB; Wed, 29 Sep 2021 15:30:49 +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="HErSRMcy"; 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 B2D146918F for ; Wed, 29 Sep 2021 15:30:46 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.9]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8FCDD3F0; Wed, 29 Sep 2021 15:30:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1632922246; bh=JvPeVAAWFiqOVCeIY68LKZ1JzVfroOyC6PQCpRA1fNc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=HErSRMcyTJE0FYGNECD7i0KcJuAzlp9cgcvTLXkyqSJQj/syQ4n0HuongVvSQuumo ASvzBKYQadvzALVAbXeHYeHYVXOUlwIJ8xIDfEjtRvXZPnogAImib7oVsZu5wm+lBk UGMtoE9Er3uOmu6CWORL/QGUROGobYGKxojLPRNc= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 29 Sep 2021 19:00:30 +0530 Message-Id: <20210929133030.401961-5-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210929133030.401961-1-umang.jain@ideasonboard.com> References: <20210929133030.401961-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v4 4/4] 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. 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 Reviewed-by: Laurent Pinchart Reviewed-by: Hirokazu Honda Reviewed-by: Jacopo Mondi --- 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 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. + */ + 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 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,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 getResultMetadata( const Camera3RequestDescriptor &descriptor) const;