Patch Detail
Show a patch.
GET /api/1.1/patches/13976/?format=api
{ "id": 13976, "url": "https://patchwork.libcamera.org/api/1.1/patches/13976/?format=api", "web_url": "https://patchwork.libcamera.org/patch/13976/", "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": "<20210929071708.299946-4-umang.jain@ideasonboard.com>", "date": "2021-09-29T07:17:08", "name": "[libcamera-devel,v3,3/3] android: camera_device: Send capture results by inspecting the queue", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "5bf250b8d6e0c838baca38a4030c48adae7c02c2", "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/13976/mbox/", "series": [ { "id": 2584, "url": "https://patchwork.libcamera.org/api/1.1/series/2584/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=2584", "date": "2021-09-29T07:17:05", "name": "Camera3RequestDescriptors std::map => std::queue", "version": 3, "mbox": "https://patchwork.libcamera.org/series/2584/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/13976/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/13976/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 9B164BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 29 Sep 2021 07:17:23 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39E746919A;\n\tWed, 29 Sep 2021 09:17:23 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4B1F6918A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 29 Sep 2021 09:17:21 +0200 (CEST)", "from perceval.ideasonboard.com (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F2BEEDEE;\n\tWed, 29 Sep 2021 09:17:20 +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=\"HqopFQim\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632899841;\n\tbh=Rdi2TWRnIZPkKJJm3VjaTixodkm04NK1Y6nCZheFYso=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=HqopFQim/sFeCqr9R+gZ5krO0tihi+RBdMhP8d/AztUIqu8E4Y+lanqatzpxDWxEt\n\tYML4JeLA53ieh+kujg2miAyYFGjg+1fQxhwEDOYKE7K66L+Re3/bVuI+YcRjzkwnM2\n\tmZvrpgp/UXr5eKl4dTcWlfPSS36qQDuzHWwy025s=", "From": "Umang Jain <umang.jain@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Wed, 29 Sep 2021 12:47:08 +0530", "Message-Id": "<20210929071708.299946-4-umang.jain@ideasonboard.com>", "X-Mailer": "git-send-email 2.31.1", "In-Reply-To": "<20210929071708.299946-1-umang.jain@ideasonboard.com>", "References": "<20210929071708.299946-1-umang.jain@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue", "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": "There is a possibility that an out-of-order completion of capture\nrequest happens by calling process_capture_result() directly on error\npaths. The framework expects that errors should be notified as soon as\npossible, but the request completion order should remain intact.\nAn existing instance of this is abortRequest(), which sends the capture\nresults on flushing state, without considering order-of-completion.\n\nSince we have a queue of Camera3RequestDescriptor tracking each\ncapture request placed by framework to libcamera HAL, we should be only\nsending back capture results from a single location, by inspecting\nthe queue. As per the patch, this now happens in\nCameraDevice::sendCaptureResults().\n\nEach descriptor is now equipped with its own status to denote whether\nthe capture request is complete and ready to send back to the framework\nor needs to be waited upon. This ensures that the order of completion is\nrespected for the requests.\n\nSince we are fixing out-of-order request completion in abortRequest(),\nchange the function to read from the Camera3RequestDescriptor directly,\ninstead of camera3_capture_request_t. The descriptor should have all the\ninformation necessary to set the request buffers' state to error.\n\nSigned-off-by: Umang Jain <umang.jain@ideasonboard.com>\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------\n src/android/camera_device.h | 14 +++++++++-\n 2 files changed, 48 insertions(+), 18 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 499baec8..fab5a854 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n \treturn 0;\n }\n \n-void CameraDevice::abortRequest(camera3_capture_request_t *request)\n+void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)\n {\n-\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n+\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n \n-\tcamera3_capture_result_t result = {};\n-\tresult.num_output_buffers = request->num_output_buffers;\n-\tresult.frame_number = request->frame_number;\n+\tcamera3_capture_result_t &result = descriptor->captureResult_;\n+\tresult.num_output_buffers = descriptor->buffers_.size();\n+\tresult.frame_number = descriptor->frameNumber_;\n \tresult.partial_result = 0;\n \n \tstd::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);\n \tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n-\t\tbuffer = request->output_buffers[i];\n-\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n+\t\tbuffer = descriptor->buffers_[i];\n+\t\tbuffer.release_fence = descriptor->buffers_[i].acquire_fence;\n \t\tbuffer.acquire_fence = -1;\n \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n \t}\n \tresult.output_buffers = resultBuffers.data();\n \n-\tcallbacks_->process_capture_result(callbacks_, &result);\n+\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n }\n \n bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n@@ -1051,13 +1051,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\treturn ret;\n \n \t/*\n-\t * If flush is in progress abort the request. If the camera has been\n-\t * stopped we have to re-start it to be able to process the request.\n+\t * If flush is in progress abort the request and push the descriptor to\n+\t * the queue. If the camera has been stopped we have to re-start it to\n+\t * be able to process the request.\n \t */\n \tMutexLocker stateLock(stateMutex_);\n \n \tif (state_ == State::Flushing) {\n-\t\tabortRequest(camera3Request);\n+\t\tabortRequest(descriptor.get());\n+\t\t{\n+\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n+\t\t\tdescriptors_.push(std::move(descriptor));\n+\t\t}\n+\t\tsendCaptureResults();\n \t\treturn 0;\n \t}\n \n@@ -1115,7 +1121,7 @@ void CameraDevice::requestComplete(Request *request)\n \t * The buffer status is set to OK and later changed to ERROR if\n \t * post-processing/compression fails.\n \t */\n-\tcamera3_capture_result_t captureResult = {};\n+\tcamera3_capture_result_t &captureResult = descriptor->captureResult_;\n \tcaptureResult.frame_number = descriptor->frameNumber_;\n \tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n@@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n \t\t}\n \n-\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n+\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n+\t\tsendCaptureResults();\n \n-\t\tdescriptors_.pop();\n \t\treturn;\n \t}\n \n@@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)\n \t}\n \n \tcaptureResult.result = resultMetadata->get();\n-\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n+\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n+\tsendCaptureResults();\n+}\n \n-\tMutexLocker descriptorsLock(descriptorsMutex_);\n-\tdescriptors_.pop();\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\tlock.unlock();\n+\t\tcallbacks_->process_capture_result(callbacks_,\n+\t\t\t\t\t\t &descriptor->captureResult_);\n+\t\tlock.lock();\n+\t}\n }\n \n std::string CameraDevice::logPrefix() const\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex 9ec510d5..69c78f08 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -74,17 +74,28 @@ private:\n \tCameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);\n \n \tstruct Camera3RequestDescriptor {\n+\t\tenum class Status {\n+\t\t\tPending,\n+\t\t\tSuccess,\n+\t\t\tError,\n+\t\t};\n+\n \t\tCamera3RequestDescriptor() = default;\n \t\t~Camera3RequestDescriptor() = default;\n \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n \t\t\t\t\t const camera3_capture_request_t *camera3Request);\n \t\tCamera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n \n+\t\tbool isPending() const { return status_ == Status::Pending; }\n+\n \t\tuint32_t frameNumber_ = 0;\n \t\tstd::vector<camera3_stream_buffer_t> buffers_;\n \t\tstd::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;\n \t\tCameraMetadata settings_;\n \t\tstd::unique_ptr<CaptureRequest> request_;\n+\n+\t\tcamera3_capture_result_t captureResult_ = {};\n+\t\tStatus status_ = Status::Pending;\n \t};\n \n \tenum class State {\n@@ -99,12 +110,13 @@ private:\n \tcreateFrameBuffer(const buffer_handle_t camera3buffer,\n \t\t\t libcamera::PixelFormat pixelFormat,\n \t\t\t const libcamera::Size &size);\n-\tvoid abortRequest(camera3_capture_request_t *request);\n+\tvoid abortRequest(Camera3RequestDescriptor *descriptor);\n \tbool isValidRequest(camera3_capture_request_t *request) const;\n \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n \t\t\t camera3_error_msg_code code);\n \tint processControls(Camera3RequestDescriptor *descriptor);\n+\tvoid sendCaptureResults();\n \tstd::unique_ptr<CameraMetadata> getResultMetadata(\n \t\tconst Camera3RequestDescriptor &descriptor) const;\n \n", "prefixes": [ "libcamera-devel", "v3", "3/3" ] }