Patch Detail
Show a patch.
GET /api/patches/13980/?format=api
{ "id": 13980, "url": "https://patchwork.libcamera.org/api/patches/13980/?format=api", "web_url": "https://patchwork.libcamera.org/patch/13980/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/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": "<20210929133030.401961-3-umang.jain@ideasonboard.com>", "date": "2021-09-29T13:30:28", "name": "[libcamera-devel,v4,2/4] android: camera_device: Transform descriptors_ map to queue", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "0222f718184db8ed99cfb6952d2e28558aaa96b5", "submitter": { "id": 86, "url": "https://patchwork.libcamera.org/api/people/86/?format=api", "name": "Umang Jain", "email": "umang.jain@ideasonboard.com" }, "delegate": { "id": 12, "url": "https://patchwork.libcamera.org/api/users/12/?format=api", "username": "uajain", "first_name": "Umang", "last_name": "Jain", "email": "umang.jain@ideasonboard.com" }, "mbox": "https://patchwork.libcamera.org/patch/13980/mbox/", "series": [ { "id": 2586, "url": "https://patchwork.libcamera.org/api/series/2586/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=2586", "date": "2021-09-29T13:30:26", "name": "Camera3RequestDescriptors std::map => std::queue", "version": 4, "mbox": "https://patchwork.libcamera.org/series/2586/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/13980/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/13980/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 700F2C3243\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 29 Sep 2021 13:30:45 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37737691B3;\n\tWed, 29 Sep 2021 15:30:45 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3CBFD69185\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 29 Sep 2021 15:30:43 +0200 (CEST)", "from perceval.ideasonboard.com (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DBAAD3F0;\n\tWed, 29 Sep 2021 15:30:41 +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=\"XbLulT7T\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632922242;\n\tbh=7ZyppM6edO48Cu3Y23/5fQf52bQTRLnMPptnujUKfuU=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=XbLulT7TYAnz8vqcIYfe0L34swI52g3jAiM4TCKGVR9PyimeJyha4u18rJepAKDLz\n\tRUgGkMBlOShKasK/tP6vp7fS2FpOeUKOa3AstCOPxitYOaRQSS9bWV/6IsURtwm/Ve\n\tN47icpsSdV8R60dFJ30KK7QUzzwTom+6EZ8u9ElA=", "From": "Umang Jain <umang.jain@ideasonboard.com>", "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", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v4 2/4] android: camera_device: Transform\n\tdescriptors_ map to 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": "The descriptors_ map holds Camera3RequestDescriptor(s) which are\nper-capture requests placed by the framework to libcamera HAL.\nCameraDevice::requestComplete() looks for the descriptor for which the\ncamera request has been completed and removes it from the map.\nSince the requests are placed in form of FIFO and the framework expects\nthe order of completion to be FIFO as well, this calls for a need of\na queue rather than a std::map.\n\nThis patch still keeps the same lifetime of Camera3RequestDescriptor as\nbefore i.e. in the requestComplete(). Previously, a descriptor was\nextracted from the map and its lifetime was bound to requestComplete().\nThe lifetime is kept same by manually calling .pop_front() on the\nqueue. In the subsequent commit, this is likely to change with a\ncentralized location of dropping descriptors from the queue for request\ncompletion.\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 | 93 +++++++++++++++++++----------------\n src/android/camera_device.h | 3 +-\n 2 files changed, 53 insertions(+), 43 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 9287ea07..e815f7a0 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -457,7 +457,8 @@ void CameraDevice::stop()\n \tworker_.stop();\n \tcamera_->stop();\n \n-\tdescriptors_.clear();\n+\tdescriptors_ = {};\n+\n \tstate_ = State::Stopped;\n }\n \n@@ -955,7 +956,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t * The descriptor and the associated memory reserved here are freed\n \t * at request complete time.\n \t */\n-\tCamera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n+\tauto descriptor = std::make_unique<Camera3RequestDescriptor>(camera_.get(),\n+\t\t\t\t\t\t\t\t camera3Request);\n \n \t/*\n \t * \\todo The Android request model is incremental, settings passed in\n@@ -966,12 +968,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \tif (camera3Request->settings)\n \t\tlastSettings_ = camera3Request->settings;\n \telse\n-\t\tdescriptor.settings_ = lastSettings_;\n+\t\tdescriptor->settings_ = lastSettings_;\n+\n+\tLOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n+\t\t\t<< \" with \" << descriptor->buffers_.size() << \" streams\";\n \n-\tLOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n-\t\t\t<< \" with \" << descriptor.buffers_.size() << \" streams\";\n-\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n-\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n+\tfor (const auto &[i, camera3Buffer] : utils::enumerate(descriptor->buffers_)) {\n \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n \t\tCameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);\n \n@@ -1007,12 +1009,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t\t * associate it with the Camera3RequestDescriptor for\n \t\t\t * lifetime management only.\n \t\t\t */\n-\t\t\tdescriptor.frameBuffers_.push_back(\n+\t\t\tdescriptor->frameBuffers_.push_back(\n \t\t\t\tcreateFrameBuffer(*camera3Buffer.buffer,\n \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n \t\t\t\t\t\t cameraStream->configuration().size));\n \n-\t\t\tbuffer = descriptor.frameBuffers_.back().get();\n+\t\t\tbuffer = descriptor->frameBuffers_.back().get();\n \t\t\tacquireFence = camera3Buffer.acquire_fence;\n \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n \t\t\tbreak;\n@@ -1035,7 +1037,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t\treturn -ENOMEM;\n \t\t}\n \n-\t\tdescriptor.request_->addBuffer(cameraStream->stream(), buffer,\n+\t\tdescriptor->request_->addBuffer(cameraStream->stream(), buffer,\n \t\t\t\t\t acquireFence);\n \t}\n \n@@ -1043,7 +1045,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t * Translate controls from Android to libcamera and queue the request\n \t * to the CameraWorker thread.\n \t */\n-\tint ret = processControls(&descriptor);\n+\tint ret = processControls(descriptor.get());\n \tif (ret)\n \t\treturn ret;\n \n@@ -1071,11 +1073,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\tstate_ = State::Running;\n \t}\n \n-\tCaptureRequest *request = descriptor.request_.get();\n+\tCaptureRequest *request = descriptor->request_.get();\n \n \t{\n \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n-\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n+\t\tdescriptors_.push(std::move(descriptor));\n \t}\n \n \tworker_.queueRequest(request);\n@@ -1085,26 +1087,28 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \n void CameraDevice::requestComplete(Request *request)\n {\n-\tdecltype(descriptors_)::node_type node;\n+\tCamera3RequestDescriptor *descriptor;\n \t{\n \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n-\t\tauto it = descriptors_.find(request->cookie());\n-\t\tif (it == descriptors_.end()) {\n-\t\t\t/*\n-\t\t\t * \\todo Clarify if the Camera has to be closed on\n-\t\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n-\t\t\t * Error.\n-\t\t\t */\n-\t\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n-\t\t\tLOG(HAL, Fatal)\n-\t\t\t\t<< \"Unknown request: \" << request->cookie();\n+\t\tASSERT(!descriptors_.empty());\n+\t\tdescriptor = descriptors_.front().get();\n+\t}\n \n-\t\t\treturn;\n-\t\t}\n+\tif (descriptor->request_->cookie() != request->cookie()) {\n+\t\t/*\n+\t\t * \\todo Clarify if the Camera has to be closed on\n+\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n+\t\t * Error.\n+\t\t */\n+\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n+\t\tLOG(HAL, Fatal)\n+\t\t\t<< \"Out-of-order completion for request \"\n+\t\t\t<< utils::hex(request->cookie());\n \n-\t\tnode = descriptors_.extract(it);\n+\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n+\t\tdescriptors_.pop();\n+\t\treturn;\n \t}\n-\tCamera3RequestDescriptor &descriptor = node.mapped();\n \n \t/*\n \t * Prepare the capture result for the Android camera stack.\n@@ -1113,9 +1117,9 @@ void CameraDevice::requestComplete(Request *request)\n \t * post-processing/compression fails.\n \t */\n \tcamera3_capture_result_t 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+\tcaptureResult.frame_number = descriptor->frameNumber_;\n+\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n+\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n \t\tCameraStream *cameraStream =\n \t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n \n@@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request *request)\n \t\tbuffer.release_fence = -1;\n \t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n \t}\n-\tcaptureResult.output_buffers = descriptor.buffers_.data();\n+\tcaptureResult.output_buffers = descriptor->buffers_.data();\n \tcaptureResult.partial_result = 1;\n \n \t/*\n@@ -1147,11 +1151,11 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\t\t<< \" not successfully completed: \"\n \t\t\t\t<< request->status();\n \n-\t\tnotifyError(descriptor.frameNumber_, nullptr,\n+\t\tnotifyError(descriptor->frameNumber_, nullptr,\n \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n \n \t\tcaptureResult.partial_result = 0;\n-\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n+\t\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n \t\t\t/*\n \t\t\t * Signal to the framework it has to handle fences that\n \t\t\t * have not been waited on by setting the release fence\n@@ -1164,6 +1168,8 @@ void CameraDevice::requestComplete(Request *request)\n \n \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n \n+\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n+\t\tdescriptors_.pop();\n \t\treturn;\n \t}\n \n@@ -1175,10 +1181,10 @@ void CameraDevice::requestComplete(Request *request)\n \t */\n \tuint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()\n \t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n-\tnotifyShutter(descriptor.frameNumber_, sensorTimestamp);\n+\tnotifyShutter(descriptor->frameNumber_, sensorTimestamp);\n \n \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n-\t\t\t<< descriptor.buffers_.size() << \" streams\";\n+\t\t\t<< descriptor->buffers_.size() << \" streams\";\n \n \t/*\n \t * Generate the metadata associated with the captured buffers.\n@@ -1186,16 +1192,16 @@ void CameraDevice::requestComplete(Request *request)\n \t * Notify if the metadata generation has failed, but continue processing\n \t * buffers and return an empty metadata pack.\n \t */\n-\tstd::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);\n+\tstd::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor);\n \tif (!resultMetadata) {\n-\t\tnotifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n+\t\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n \n \t\t/* The camera framework expects an empty metadata pack on error. */\n \t\tresultMetadata = std::make_unique<CameraMetadata>(0, 0);\n \t}\n \n \t/* Handle post-processing. */\n-\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n+\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n \t\tCameraStream *cameraStream =\n \t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n \n@@ -1206,13 +1212,13 @@ void CameraDevice::requestComplete(Request *request)\n \t\tif (!src) {\n \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n-\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n+\t\t\tnotifyError(descriptor->frameNumber_, buffer.stream,\n \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n \t\t\tcontinue;\n \t\t}\n \n \t\tint ret = cameraStream->process(*src, buffer,\n-\t\t\t\t\t\tdescriptor.settings_,\n+\t\t\t\t\t\tdescriptor->settings_,\n \t\t\t\t\t\tresultMetadata.get());\n \t\t/*\n \t\t * Return the FrameBuffer to the CameraStream now that we're\n@@ -1223,13 +1229,16 @@ void CameraDevice::requestComplete(Request *request)\n \n \t\tif (ret) {\n \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n-\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n+\t\t\tnotifyError(descriptor->frameNumber_, buffer.stream,\n \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n \t\t}\n \t}\n \n \tcaptureResult.result = resultMetadata->get();\n \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n+\n+\tMutexLocker descriptorsLock(descriptorsMutex_);\n+\tdescriptors_.pop();\n }\n \n std::string CameraDevice::logPrefix() const\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex 43eb5895..9ec510d5 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -10,6 +10,7 @@\n #include <map>\n #include <memory>\n #include <mutex>\n+#include <queue>\n #include <vector>\n \n #include <hardware/camera3.h>\n@@ -125,7 +126,7 @@ private:\n \tstd::vector<CameraStream> streams_;\n \n \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n-\tstd::map<uint64_t, Camera3RequestDescriptor> descriptors_;\n+\tstd::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;\n \n \tstd::string maker_;\n \tstd::string model_;\n", "prefixes": [ "libcamera-devel", "v4", "2/4" ] }