Patch Detail
Show a patch.
GET /api/1.1/patches/14289/?format=api
{ "id": 14289, "url": "https://patchwork.libcamera.org/api/1.1/patches/14289/?format=api", "web_url": "https://patchwork.libcamera.org/patch/14289/", "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": "<20211023103302.152769-4-umang.jain@ideasonboard.com>", "date": "2021-10-23T10:32:58", "name": "[libcamera-devel,v6,3/7] android: camera_device: Refactor descriptor status and sendCaptureResults()", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "aa4540c4f80ddec0145bbd465774a7471bc06d7f", "submitter": { "id": 86, "url": "https://patchwork.libcamera.org/api/1.1/people/86/?format=api", "name": "Umang Jain", "email": "umang.jain@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/14289/mbox/", "series": [ { "id": 2652, "url": "https://patchwork.libcamera.org/api/1.1/series/2652/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=2652", "date": "2021-10-23T10:32:55", "name": "Async Post Processor", "version": 6, "mbox": "https://patchwork.libcamera.org/series/2652/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/14289/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/14289/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 314DDBDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 23 Oct 2021 10:33:19 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E304B60129;\n\tSat, 23 Oct 2021 12:33:18 +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 D6F0D6486D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 23 Oct 2021 12:33:15 +0200 (CEST)", "from perceval.ideasonboard.com (unknown [103.251.226.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D77CC547;\n\tSat, 23 Oct 2021 12:33:14 +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=\"eb9mVE5C\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634985195;\n\tbh=G06W9ZadYI3FT7xONrL5GjiZvjR7RdUAUJDMOf/S70A=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=eb9mVE5C8UxZOhhRmccxotpoehOq54nGAciz5Lfbe6xPFCn63wmzpx9micPcRyatw\n\twiL44BgIKXpVqRjBShQKR1BgH+zZrCI9TeqiRMWzwOrbu9LVcSkKYZMxgti5RlwuSv\n\t3Db7n5mxKU1ZetZMqJElBkgPKT4hDp276+8+USgU=", "From": "Umang Jain <umang.jain@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Sat, 23 Oct 2021 16:02:58 +0530", "Message-Id": "<20211023103302.152769-4-umang.jain@ideasonboard.com>", "X-Mailer": "git-send-email 2.31.1", "In-Reply-To": "<20211023103302.152769-1-umang.jain@ideasonboard.com>", "References": "<20211023103302.152769-1-umang.jain@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v6 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\nSigned-off-by: Umang Jain <umang.jain@ideasonboard.com>\n---\n src/android/camera_device.cpp | 33 ++++++++++++++++++---------------\n src/android/camera_device.h | 1 +\n src/android/camera_request.cpp | 2 +-\n src/android/camera_request.h | 7 ++++---\n 4 files changed, 24 insertions(+), 19 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 806b4090..f4d4fb09 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\tabortRequest(rawDescriptor);\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\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@@ -1117,6 +1117,7 @@ void CameraDevice::requestComplete(Request *request)\n \t}\n \n \t/* Handle post-processing. */\n+\tbool hasPostProcessingErrors = false;\n \tfor (auto &buffer : descriptor->buffers_) {\n \t\tCameraStream *stream = buffer.stream;\n \n@@ -1129,6 +1130,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\thasPostProcessingErrors = true;\n \t\t\tcontinue;\n \t\t}\n \n@@ -1143,29 +1145,32 @@ void CameraDevice::requestComplete(Request *request)\n \n \t\tif (ret) {\n \t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n+\t\t\thasPostProcessingErrors = true;\n \t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n \t\t}\n \t}\n \n-\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n+\tif (hasPostProcessingErrors)\n+\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n+\n+\tcompleteDescriptor(descriptor);\n+}\n+\n+void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n+{\n+\tMutexLocker lock(descriptorsMutex_);\n+\tdescriptor->completeDescriptor();\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 +1206,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..904886da 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,8 @@ 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+\tvoid completeDescriptor() { complete_ = true; }\n \n \tuint32_t frameNumber_ = 0;\n \n@@ -53,7 +53,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", "v6", "3/7" ] }