Patch Detail
Show a patch.
GET /api/1.1/patches/14081/?format=api
{ "id": 14081, "url": "https://patchwork.libcamera.org/api/1.1/patches/14081/?format=api", "web_url": "https://patchwork.libcamera.org/patch/14081/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20211011073505.243864-7-umang.jain@ideasonboard.com>", "date": "2021-10-11T07:35:04", "name": "[libcamera-devel,v4,6/7] android: camera_device: Protect descriptor status_ with lock", "commit_ref": null, "pull_url": null, "state": "changes-requested", "archived": false, "hash": "88661037d3c8bde34da3138ed67140d24149808d", "submitter": { "id": 86, "url": "https://patchwork.libcamera.org/api/1.1/people/86/?format=api", "name": "Umang Jain", "email": "umang.jain@ideasonboard.com" }, "delegate": { "id": 12, "url": "https://patchwork.libcamera.org/api/1.1/users/12/?format=api", "username": "uajain", "first_name": "Umang", "last_name": "Jain", "email": "umang.jain@ideasonboard.com" }, "mbox": "https://patchwork.libcamera.org/patch/14081/mbox/", "series": [ { "id": 2611, "url": "https://patchwork.libcamera.org/api/1.1/series/2611/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=2611", "date": "2021-10-11T07:34:58", "name": "Async Post Processor", "version": 4, "mbox": "https://patchwork.libcamera.org/series/2611/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/14081/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/14081/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "X-Original-To": "parsemail@patchwork.libcamera.org", "Delivered-To": "parsemail@patchwork.libcamera.org", "Received": [ "from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2A31BC323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 11 Oct 2021 07:35:29 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0F0B68F58;\n\tMon, 11 Oct 2021 09:35:28 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61BF868F51\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 11 Oct 2021 09:35:26 +0200 (CEST)", "from perceval.ideasonboard.com (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 748872BD;\n\tMon, 11 Oct 2021 09:35:25 +0200 (CEST)" ], "Authentication-Results": "lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sDOQFpqn\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633937726;\n\tbh=lZjbcbQa+fNMcgjQZHeZxVksBy9hBGcRD5K59GP0k70=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=sDOQFpqnO6q3xTR4PaCgVV0rSu2Ss6DNAp+2aqCZH9DzelkktUe/KALCldyrogT22\n\ts+jH6xQVIxaPs1jcwe+j58dneyLks6t0ZDE5h1SgGj1RjXuvRgmi/tLsq0SiRa1aJX\n\tCKWMKx7Si2b9M4Q1prxYjIWPQbxari7yvr0BYXAc=", "From": "Umang Jain <umang.jain@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Mon, 11 Oct 2021 13:05:04 +0530", "Message-Id": "<20211011073505.243864-7-umang.jain@ideasonboard.com>", "X-Mailer": "git-send-email 2.31.1", "In-Reply-To": "<20211011073505.243864-1-umang.jain@ideasonboard.com>", "References": "<20211011073505.243864-1-umang.jain@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v4 6/7] android: camera_device: Protect\n\tdescriptor status_ with lock", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nThe Camera3RequestDescriptor::status_ is checked as a stop condition for\nthe sendCaptureResults() loop, protected by the descriptorsMutex_. The\nstatus is however not set with the mutex locked, which can cause a race\ncondition with a concurrent sendCaptureResults() call (from the\npost-processor thread for instance).\n\nThis should be harmless in practice, as the reader thread will either\nsee the old status (Pending) and stop iterating over descriptors, or the\nnew status and continue. Still, if the Camera3RequestDescriptor state\nmachine were to change in the future, this could introduce hard to debug\nissues. Close the race window by always setting the status with the lock\ntaken.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n src/android/camera_device.cpp | 27 +++++++++++++++++++--------\n src/android/camera_device.h | 4 +++-\n 2 files changed, 22 insertions(+), 9 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 61b902ad..3541a74b 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -877,8 +877,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n \t}\n \tresult.output_buffers = resultBuffers.data();\n-\n-\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n }\n \n bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n@@ -1058,8 +1056,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \n \tif (state_ == State::Flushing) {\n \t\tabortRequest(descriptor.get());\n+\n \t\t{\n \t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n+\t\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n \t\t\tdescriptors_.push(std::move(descriptor));\n \t\t}\n \n@@ -1154,8 +1154,7 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n \t\t}\n \n-\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n-\t\tsendCaptureResults();\n+\t\tcompleteDescriptor(descriptor, Camera3RequestDescriptor::Status::Error);\n \n \t\treturn;\n \t}\n@@ -1215,13 +1214,24 @@ void CameraDevice::requestComplete(Request *request)\n \t}\n \n \tcaptureResult.result = descriptor->resultMetadata_->get();\n-\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n+\n+\tcompleteDescriptor(descriptor, Camera3RequestDescriptor::Status::Success);\n+}\n+\n+void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,\n+\t\t\t\t Camera3RequestDescriptor::Status status)\n+{\n+\tMutexLocker lock(descriptorsMutex_);\n+\tdescriptor->status_ = status;\n+\tlock.unlock();\n+\n \tsendCaptureResults();\n }\n \n void CameraDevice::sendCaptureResults()\n {\n \tMutexLocker lock(descriptorsMutex_);\n+\n \twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n \t\tauto descriptor = std::move(descriptors_.front());\n \t\tdescriptors_.pop();\n@@ -1262,12 +1272,13 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n \tif (cameraStream->type() == CameraStream::Type::Internal)\n \t\tcameraStream->putBuffer(request->internalBuffer_);\n \n+\tCamera3RequestDescriptor::Status descriptorStatus;\n \tif (status == PostProcessor::Status::Success)\n-\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n+\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Success;\n \telse\n-\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n+\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Error;\n \n-\tsendCaptureResults();\n+\tcompleteDescriptor(request, descriptorStatus);\n }\n \n std::string CameraDevice::logPrefix() const\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex 725a0618..0ce6caeb 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -126,6 +126,8 @@ private:\n \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n \t\t\t camera3_error_msg_code code) const;\n \tint processControls(Camera3RequestDescriptor *descriptor);\n+\tvoid completeDescriptor(Camera3RequestDescriptor *descriptor,\n+\t\t\t\tCamera3RequestDescriptor::Status status);\n \tvoid sendCaptureResults();\n \tstd::unique_ptr<CameraMetadata> getResultMetadata(\n \t\tconst Camera3RequestDescriptor &descriptor) const;\n@@ -147,7 +149,7 @@ private:\n \n \tstd::vector<CameraStream> streams_;\n \n-\tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n+\tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_ and Camera3RequestDescriptor::status_. */\n \tstd::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;\n \n \tstd::string maker_;\n", "prefixes": [ "libcamera-devel", "v4", "6/7" ] }