Patch Detail
Show a patch.
GET /api/1.1/patches/14297/?format=api
{ "id": 14297, "url": "https://patchwork.libcamera.org/api/1.1/patches/14297/?format=api", "web_url": "https://patchwork.libcamera.org/patch/14297/", "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": "<20211025203833.122460-4-umang.jain@ideasonboard.com>", "date": "2021-10-25T20:38:29", "name": "[libcamera-devel,v7,3/7] android: camera_device: Refactor descriptor status and sendCaptureResults()", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "de3ea04f83667f156a7ded670f4342b93815b9e9", "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/14297/mbox/", "series": [ { "id": 2653, "url": "https://patchwork.libcamera.org/api/1.1/series/2653/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=2653", "date": "2021-10-25T20:38:26", "name": "Async Post Processor", "version": 7, "mbox": "https://patchwork.libcamera.org/series/2653/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/14297/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/14297/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 0ADF8BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 25 Oct 2021 20:38:50 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BC0816487B;\n\tMon, 25 Oct 2021 22:38:49 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EDFD560125\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 25 Oct 2021 22:38:48 +0200 (CEST)", "from perceval.ideasonboard.com (unknown [103.251.226.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 979F9E0A;\n\tMon, 25 Oct 2021 22:38:47 +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=\"Gyyp9EOm\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635194328;\n\tbh=i3xgVjRQSzYJcumu/ZRrhup36MHLEmP+pOYX6h/PmB8=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=Gyyp9EOmIZD+3oTgh7Fae8n9y4cFOL8kPIa1BkOmQzFoXd+r/9YwH4R/3ZmaNQ+wf\n\t1R03hyt1fMv6EszDDxeMrHUBfsnr9Sx8xTUTI6vxtb0TYbSLfFbfTxjXuyquPIC8hk\n\tNrzmST37FY4QXwJzluXOyFIza07XOC2QJY4J8qK8=", "From": "Umang Jain <umang.jain@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Tue, 26 Oct 2021 02:08:29 +0530", "Message-Id": "<20211025203833.122460-4-umang.jain@ideasonboard.com>", "X-Mailer": "git-send-email 2.31.1", "In-Reply-To": "<20211025203833.122460-1-umang.jain@ideasonboard.com>", "References": "<20211025203833.122460-1-umang.jain@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v7 3/7] android: camera_device: Refactor\n\tdescriptor status and sendCaptureResults()", "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": "Currently, we use Camera3RequestDescriptor::Status to determine:\n- When the descriptor has been completely processed by HAL\n- Whether any errors were encountered, during its processing\n\nBoth of these are essential to know whether the descriptor is eligible\nto call process_capture_results() through sendCaptureResults().\nWhen a status(Success/Error) is set on the descriptor, it is ready to\nbe sent back via sendCaptureResults(). However, this might lead to\nundesired results especially when sendCaptureResults() runs in a\ndifferent thread (for e.g. stream's post-processor async completion\nslot).\n\nThis patch decouples the descriptor status (Success/Error) from the\ndescriptor's completion status (pending or complete). The advantage\nof this is we can set the completion status when the descriptor has\nbeen processed fully by the layer and we can set the error status on\nthe descriptor wherever an error is encountered, throughout the\nlifetime descriptor in the HAL layer.\n\nWhile at it, introduce a wrapper completeDescriptor() around\nsendCaptureResults(). completeDescriptor() as the name suggests will\nmark the descriptor as complete, so it is ready to be sent back.\nThe locking mechanism is moved from sendCaptureResults() to this wrapper\nsince the intention is to use completeDescriptor() in place of existing\nsendCaptureResults() calls.\n\nAlso make sure the sequence of abortRequest() call happens in the same\norder at all places i.e. after its added to the descriptors_ queue. Fix\none of the abortRequest() call accordingly.\n\nSigned-off-by: Umang Jain <umang.jain@ideasonboard.com>\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\nReviewed-by: Hirokazu Honda <hiroh@chromium.org>\n---\n src/android/camera_device.cpp | 29 ++++++++++++++---------------\n src/android/camera_device.h | 1 +\n src/android/camera_request.cpp | 2 +-\n src/android/camera_request.h | 6 +++---\n 4 files changed, 19 insertions(+), 19 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 806b4090..bf9a2e69 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -982,13 +982,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \tMutexLocker stateLock(stateMutex_);\n \n \tif (state_ == State::Flushing) {\n-\t\tabortRequest(descriptor.get());\n+\t\tCamera3RequestDescriptor *rawDescriptor = descriptor.get();\n \t\t{\n \t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n \t\t\tdescriptors_.push(std::move(descriptor));\n \t\t}\n-\n-\t\tsendCaptureResults();\n+\t\tabortRequest(rawDescriptor);\n+\t\tcompleteDescriptor(rawDescriptor);\n \n \t\treturn 0;\n \t}\n@@ -1079,7 +1079,7 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\t\t<< request->status();\n \n \t\tabortRequest(descriptor);\n-\t\tsendCaptureResults();\n+\t\tcompleteDescriptor(descriptor);\n \n \t\treturn;\n \t}\n@@ -1129,6 +1129,7 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n \t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n+\t\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n \t\t\tcontinue;\n \t\t}\n \n@@ -1145,27 +1146,27 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n \t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n+\t\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n \t\t}\n \t}\n \n-\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n+\tcompleteDescriptor(descriptor);\n+}\n+\n+void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n+{\n+\tMutexLocker lock(descriptorsMutex_);\n+\tdescriptor->complete_ = true;\n+\n \tsendCaptureResults();\n }\n \n void CameraDevice::sendCaptureResults()\n {\n-\tMutexLocker lock(descriptorsMutex_);\n \twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n \t\tauto descriptor = std::move(descriptors_.front());\n \t\tdescriptors_.pop();\n \n-\t\t/*\n-\t\t * \\todo Releasing and re-acquiring the critical section for\n-\t\t * every request completion (grain-locking) might have an\n-\t\t * impact on performance which should be measured.\n-\t\t */\n-\t\tlock.unlock();\n-\n \t\tcamera3_capture_result_t captureResult = {};\n \n \t\tcaptureResult.frame_number = descriptor->frameNumber_;\n@@ -1201,8 +1202,6 @@ void CameraDevice::sendCaptureResults()\n \t\t\tcaptureResult.partial_result = 1;\n \n \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n-\n-\t\tlock.lock();\n \t}\n }\n \ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex 863cf414..e544f2bd 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -93,6 +93,7 @@ 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 \tvoid sendCaptureResults();\n \tstd::unique_ptr<CameraMetadata> getResultMetadata(\n \t\tconst Camera3RequestDescriptor &descriptor) const;\ndiff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\nindex faa85ada..16cf266f 100644\n--- a/src/android/camera_request.cpp\n+++ b/src/android/camera_request.cpp\n@@ -36,7 +36,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n \t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n \n \t\tbuffers_.push_back({ stream, buffer.buffer, nullptr,\n-\t\t\t\t buffer.acquire_fence, Status::Pending });\n+\t\t\t\t buffer.acquire_fence, Status::Success });\n \t}\n \n \t/* Clone the controls associated with the camera3 request. */\ndiff --git a/src/android/camera_request.h b/src/android/camera_request.h\nindex 05dabf89..4d80ef32 100644\n--- a/src/android/camera_request.h\n+++ b/src/android/camera_request.h\n@@ -26,7 +26,6 @@ class Camera3RequestDescriptor\n {\n public:\n \tenum class Status {\n-\t\tPending,\n \t\tSuccess,\n \t\tError,\n \t};\n@@ -43,7 +42,7 @@ public:\n \t\t\t\t const camera3_capture_request_t *camera3Request);\n \t~Camera3RequestDescriptor();\n \n-\tbool isPending() const { return status_ == Status::Pending; }\n+\tbool isPending() const { return !complete_; }\n \n \tuint32_t frameNumber_ = 0;\n \n@@ -53,7 +52,8 @@ public:\n \tstd::unique_ptr<CaptureRequest> request_;\n \tstd::unique_ptr<CameraMetadata> resultMetadata_;\n \n-\tStatus status_ = Status::Pending;\n+\tbool complete_ = false;\n+\tStatus status_ = Status::Success;\n \n private:\n \tLIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)\n", "prefixes": [ "libcamera-devel", "v7", "3/7" ] }