[{"id":19948,"web_url":"https://patchwork.libcamera.org/comment/19948/","msgid":"","date":"2021-09-28T21:07:04","subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Tue, Sep 28, 2021 at 09:55:35PM +0530, Umang Jain wrote:\n> The descriptors_ map holds Camera3RequestDescriptor(s) which are\n> per-capture requests placed by the framework to libcamera HAL.\n> CameraDevice::requestComplete() looks for the descriptor for which the\n> camera request has been completed and removes it from the map.\n> Since the requests are placed in form of FIFO and the framework expects\n> the order of completion to be FIFO as well, this calls for a need of\n> a queue rather than a std::map.\n> \n> This patch still keeps the same lifetime of Camera3RequestDescriptor as\n> before i.e. in the requestComplete(). Previously, a descriptor was\n> extracted from the map and its lifetime was bound to requestComplete().\n> The lifetime is kept same by manually calling .pop_front() on the\n> queue. In the subsequent commit, this is likely to change with a\n> centralized location of dropping descriptors from the queue for request\n> completion.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 92 +++++++++++++++++++----------------\n> src/android/camera_device.h | 3 +-\n> 2 files changed, 52 insertions(+), 43 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 9287ea07..a3b8a549 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -457,7 +457,9 @@ void CameraDevice::stop()\n> \tworker_.stop();\n> \tcamera_->stop();\n> \n> -\tdescriptors_.clear();\n> +\twhile (!descriptors_.empty())\n> +\t\tdescriptors_.pop();\n\nYou can simplify this with\n\n\tdescriptors_ = {};\n\n> +\n> \tstate_ = State::Stopped;\n> }\n> \n> @@ -955,7 +957,9 @@ 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> +\tstd::unique_ptr descriptor =\n> +\t\tstd::make_unique(camera_.get(),\n> +\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 +970,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(camera3Stream->priv);\n> \n> @@ -1007,12 +1011,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 +1039,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 +1047,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 +1075,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 +1089,26 @@ 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\ns/://\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t\t<< utils::hex(request->cookie());\n> \n> -\t\tnode = descriptors_.extract(it);\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(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,7 @@ void CameraDevice::requestComplete(Request *request)\n> \n> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> \n> +\t\tdescriptors_.pop();\n> \t\treturn;\n> \t}\n> \n> @@ -1175,10 +1180,10 @@ void CameraDevice::requestComplete(Request *request)\n> \t */\n> \tuint64_t sensorTimestamp = static_cast(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 +1191,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 resultMetadata = getResultMetadata(descriptor);\n> +\tstd::unique_ptr 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(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(buffer.stream->priv);\n> \n> @@ -1206,13 +1211,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 +1228,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\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 43eb5895..9ec510d5 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -125,7 +126,7 @@ private:\n> \tstd::vector streams_;\n> \n> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> -\tstd::map descriptors_;\n> +\tstd::queue> descriptors_;\n> \n> \tstd::string maker_;\n> \tstd::string model_;","headers":{"Return-Path":"","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 29237BDC71\n\tfor ;\n\tTue, 28 Sep 2021 21:07:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F66E69193;\n\tTue, 28 Sep 2021 23:07:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A83BD69188\n\tfor ;\n\tTue, 28 Sep 2021 23:07:06 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0C43B3F1;\n\tTue, 28 Sep 2021 23:07:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UM5Cn5SR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632863226;\n\tbh=kvVLudlqTGXp5M1R0n6H4alapXKhdh63r50sEDqiEV0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UM5Cn5SRcGlv7GNZnENsxDs8QZZpmEUfDdFVk7jVTpBzqyBXJsCVwTUC0lvIY7bKB\n\tg6y+oXUsni4OJCV6NKgkFh6vHNidkgPzlIOOlwdfMVdnQbLQrnB/tFeMgOvvvVaOpE\n\tfRRHgscj80BTAsGL+6gbHmri7nfHxKW7k1KrFhz0=","Date":"Wed, 29 Sep 2021 00:07:04 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210928162536.227475-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19953,"web_url":"https://patchwork.libcamera.org/comment/19953/","msgid":"","date":"2021-09-28T23:29:50","subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Wed, Sep 29, 2021 at 6:07 AM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Sep 28, 2021 at 09:55:35PM +0530, Umang Jain wrote:\n> > The descriptors_ map holds Camera3RequestDescriptor(s) which are\n> > per-capture requests placed by the framework to libcamera HAL.\n> > CameraDevice::requestComplete() looks for the descriptor for which the\n> > camera request has been completed and removes it from the map.\n> > Since the requests are placed in form of FIFO and the framework expects\n> > the order of completion to be FIFO as well, this calls for a need of\n> > a queue rather than a std::map.\n> >\n> > This patch still keeps the same lifetime of Camera3RequestDescriptor as\n> > before i.e. in the requestComplete(). Previously, a descriptor was\n> > extracted from the map and its lifetime was bound to requestComplete().\n> > The lifetime is kept same by manually calling .pop_front() on the\n> > queue. In the subsequent commit, this is likely to change with a\n> > centralized location of dropping descriptors from the queue for request\n> > completion.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 92 +++++++++++++++++++----------------\n> > src/android/camera_device.h | 3 +-\n> > 2 files changed, 52 insertions(+), 43 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 9287ea07..a3b8a549 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -457,7 +457,9 @@ void CameraDevice::stop()\n> > worker_.stop();\n> > camera_->stop();\n> >\n> > - descriptors_.clear();\n> > + while (!descriptors_.empty())\n> > + descriptors_.pop();\n>\n> You can simplify this with\n>\n> descriptors_ = {};\n>\n> > +\n> > state_ = State::Stopped;\n> > }\n> >\n> > @@ -955,7 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > * The descriptor and the associated memory reserved here are freed\n> > * at request complete time.\n> > */\n> > - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n> > + std::unique_ptr descriptor =\n> > + std::make_unique(camera_.get(),\n> > + camera3Request);\n\nnit: I would use auto in the type declaration in using\nstd::make_unique because the type is obvious from the right formula.\n\n> >\n> > /*\n> > * \\todo The Android request model is incremental, settings passed in\n> > @@ -966,12 +970,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > if (camera3Request->settings)\n> > lastSettings_ = camera3Request->settings;\n> > else\n> > - descriptor.settings_ = lastSettings_;\n> > + descriptor->settings_ = lastSettings_;\n> > +\n> > + LOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> > + << \" with \" << descriptor->buffers_.size() << \" streams\";\n> >\n> > - LOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n> > - << \" with \" << descriptor.buffers_.size() << \" streams\";\n> > - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> > - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> > + for (const auto &[i, camera3Buffer] : utils::enumerate(descriptor->buffers_)) {\n> > camera3_stream *camera3Stream = camera3Buffer.stream;\n> > CameraStream *cameraStream = static_cast(camera3Stream->priv);\n> >\n> > @@ -1007,12 +1011,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > * associate it with the Camera3RequestDescriptor for\n> > * lifetime management only.\n> > */\n> > - descriptor.frameBuffers_.push_back(\n> > + descriptor->frameBuffers_.push_back(\n> > createFrameBuffer(*camera3Buffer.buffer,\n> > cameraStream->configuration().pixelFormat,\n> > cameraStream->configuration().size));\n> >\n> > - buffer = descriptor.frameBuffers_.back().get();\n> > + buffer = descriptor->frameBuffers_.back().get();\n> > acquireFence = camera3Buffer.acquire_fence;\n> > LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > break;\n> > @@ -1035,7 +1039,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > return -ENOMEM;\n> > }\n> >\n> > - descriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> > + descriptor->request_->addBuffer(cameraStream->stream(), buffer,\n> > acquireFence);\n> > }\n> >\n> > @@ -1043,7 +1047,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > * Translate controls from Android to libcamera and queue the request\n> > * to the CameraWorker thread.\n> > */\n> > - int ret = processControls(&descriptor);\n> > + int ret = processControls(descriptor.get());\n> > if (ret)\n> > return ret;\n> >\n> > @@ -1071,11 +1075,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > state_ = State::Running;\n> > }\n> >\n> > - CaptureRequest *request = descriptor.request_.get();\n> > + CaptureRequest *request = descriptor->request_.get();\n> >\n> > {\n> > MutexLocker descriptorsLock(descriptorsMutex_);\n> > - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > + descriptors_.push(std::move(descriptor));\n> > }\n> >\n> > worker_.queueRequest(request);\n> > @@ -1085,26 +1089,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >\n> > void CameraDevice::requestComplete(Request *request)\n> > {\n> > - decltype(descriptors_)::node_type node;\n> > + Camera3RequestDescriptor *descriptor;\n> > {\n> > MutexLocker descriptorsLock(descriptorsMutex_);\n> > - auto it = descriptors_.find(request->cookie());\n> > - if (it == descriptors_.end()) {\n> > - /*\n> > - * \\todo Clarify if the Camera has to be closed on\n> > - * ERROR_DEVICE and possibly demote the Fatal to simple\n> > - * Error.\n> > - */\n> > - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > - LOG(HAL, Fatal)\n> > - << \"Unknown request: \" << request->cookie();\n> > + ASSERT(!descriptors_.empty());\n> > + descriptor = descriptors_.front().get();\n\nWhy isn't descriptor popped here?\nIs there any case that a descriptor should be kept?\nThe original code removes the descriptor from descriptors_.\nThis changes the logic.\nI also think removing here is safe because although a pointer of node\nin std::queue() is not invalidated by push operation, but indeed can\nbe invalidated by pop operation.\nSo removing here (while acquiring a lock) avoids the problem of using\nan invalid pointer in the case descriptors_ are cleared in parallel.\n\n-Hiro\n> > + }\n> >\n> > - return;\n> > - }\n> > + if (descriptor->request_->cookie() != request->cookie()) {\n> > + /*\n> > + * \\todo Clarify if the Camera has to be closed on\n> > + * ERROR_DEVICE and possibly demote the Fatal to simple\n> > + * Error.\n> > + */\n> > + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > + LOG(HAL, Fatal)\n> > + << \"Out-of-order completion for request: \"\n>\n> s/://\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > + << utils::hex(request->cookie());\n> >\n> > - node = descriptors_.extract(it);\n> > + return;\n> > }\n> > - Camera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > /*\n> > * Prepare the capture result for the Android camera stack.\n> > @@ -1113,9 +1117,9 @@ void CameraDevice::requestComplete(Request *request)\n> > * post-processing/compression fails.\n> > */\n> > camera3_capture_result_t captureResult = {};\n> > - captureResult.frame_number = descriptor.frameNumber_;\n> > - captureResult.num_output_buffers = descriptor.buffers_.size();\n> > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > + captureResult.frame_number = descriptor->frameNumber_;\n> > + captureResult.num_output_buffers = descriptor->buffers_.size();\n> > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > CameraStream *cameraStream =\n> > static_cast(buffer.stream->priv);\n> >\n> > @@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request *request)\n> > buffer.release_fence = -1;\n> > buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > }\n> > - captureResult.output_buffers = descriptor.buffers_.data();\n> > + captureResult.output_buffers = descriptor->buffers_.data();\n> > captureResult.partial_result = 1;\n> >\n> > /*\n> > @@ -1147,11 +1151,11 @@ void CameraDevice::requestComplete(Request *request)\n> > << \" not successfully completed: \"\n> > << request->status();\n> >\n> > - notifyError(descriptor.frameNumber_, nullptr,\n> > + notifyError(descriptor->frameNumber_, nullptr,\n> > CAMERA3_MSG_ERROR_REQUEST);\n> >\n> > captureResult.partial_result = 0;\n> > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > /*\n> > * Signal to the framework it has to handle fences that\n> > * have not been waited on by setting the release fence\n> > @@ -1164,6 +1168,7 @@ void CameraDevice::requestComplete(Request *request)\n> >\n> > callbacks_->process_capture_result(callbacks_, &captureResult);\n> >\n\nA lock is not acquired here?\nMutexLocker descriptorsLock(descriptorsMutex_);\n\n> > + descriptors_.pop();\n> > return;\n> > }\n> >\n> > @@ -1175,10 +1180,10 @@ void CameraDevice::requestComplete(Request *request)\n> > */\n> > uint64_t sensorTimestamp = static_cast(request->metadata()\n> > .get(controls::SensorTimestamp));\n> > - notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> > + notifyShutter(descriptor->frameNumber_, sensorTimestamp);\n> >\n> > LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> > - << descriptor.buffers_.size() << \" streams\";\n> > + << descriptor->buffers_.size() << \" streams\";\n> >\n> > /*\n> > * Generate the metadata associated with the captured buffers.\n> > @@ -1186,16 +1191,16 @@ void CameraDevice::requestComplete(Request *request)\n> > * Notify if the metadata generation has failed, but continue processing\n> > * buffers and return an empty metadata pack.\n> > */\n> > - std::unique_ptr resultMetadata = getResultMetadata(descriptor);\n> > + std::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n> > if (!resultMetadata) {\n> > - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> >\n> > /* The camera framework expects an empty metadata pack on error. */\n> > resultMetadata = std::make_unique(0, 0);\n> > }\n> >\n> > /* Handle post-processing. */\n> > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > CameraStream *cameraStream =\n> > static_cast(buffer.stream->priv);\n> >\n> > @@ -1206,13 +1211,13 @@ void CameraDevice::requestComplete(Request *request)\n> > if (!src) {\n> > LOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> > buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > - notifyError(descriptor.frameNumber_, buffer.stream,\n> > + notifyError(descriptor->frameNumber_, buffer.stream,\n> > CAMERA3_MSG_ERROR_BUFFER);\n> > continue;\n> > }\n> >\n> > int ret = cameraStream->process(*src, buffer,\n> > - descriptor.settings_,\n> > + descriptor->settings_,\n> > resultMetadata.get());\n> > /*\n> > * Return the FrameBuffer to the CameraStream now that we're\n> > @@ -1223,13 +1228,16 @@ void CameraDevice::requestComplete(Request *request)\n> >\n> > if (ret) {\n> > buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > - notifyError(descriptor.frameNumber_, buffer.stream,\n> > + notifyError(descriptor->frameNumber_, buffer.stream,\n> > CAMERA3_MSG_ERROR_BUFFER);\n> > }\n> > }\n> >\n> > captureResult.result = resultMetadata->get();\n> > callbacks_->process_capture_result(callbacks_, &captureResult);\n> > +\n> > + MutexLocker descriptorsLock(descriptorsMutex_);\n> > + descriptors_.pop();\n> > }\n> >\n> > std::string CameraDevice::logPrefix() const\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 43eb5895..9ec510d5 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -125,7 +126,7 @@ private:\n> > std::vector streams_;\n> >\n> > libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> > - std::map descriptors_;\n> > + std::queue> descriptors_;\n> >\n> > std::string maker_;\n> > std::string model_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","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 785C0C3243\n\tfor ;\n\tTue, 28 Sep 2021 23:30:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F32B869193;\n\tWed, 29 Sep 2021 01:30:02 +0200 (CEST)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98F8E6012C\n\tfor ;\n\tWed, 29 Sep 2021 01:30:01 +0200 (CEST)","by mail-ed1-x532.google.com with SMTP id v10so1332276edj.10\n\tfor ;\n\tTue, 28 Sep 2021 16:30:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"jd2GV0+y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=9IOs1z6UII0rRWSsfCqr7CHJIsoegt9tdcRhsCexCNU=;\n\tb=jd2GV0+yaJZqrTduwFKSmYLmGSkiDgDgRWSLV8Dpcbu5rIXPyrx5zVE5lFu1MrCju/\n\tFqj0zUxwb7OVTziE/Scf6/NoWvS0hcTBGX2ptMRePv3rJPim5qAIDirJtrAzYuhtKLyS\n\tITZbVolbL4DSGb752AW2cyyHyWTMYz2Zbf+Io=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=9IOs1z6UII0rRWSsfCqr7CHJIsoegt9tdcRhsCexCNU=;\n\tb=8LBXDeixYqVo8bloagRIXQC5jHlQCxpDLs0Dm/nOIzkb8qPKc5vIA5YFFP8cBkvzO7\n\ty3B/35MGkSCtXuij+Qe66JH/2KfloVThxn7RvihvukbwMMj8sNGB2Rmj7YpfEJz/Tiaq\n\tK0IWb60YAuzv7hewMgW1F0UIdbaLWH4cTICWsTDbizQbCT2JuvVCsPcSg3o2yQ1iN/me\n\t/N9n1F6wO4KM6b/cj6n8htah49hdnXc7oLb4JbkQ//a6PsYxDunFhECQHGHsASAFeklY\n\tB3/4RmqpNTZkRF1pwGAnkRvUhQzRDn91EsUEf1ayaFnWgidvaw/hU5qNee+jX2rxOx8F\n\tTb+Q==","X-Gm-Message-State":"AOAM530rO1AvDbaFT0CJeZ0hUHRwvPhERxS5u8z2xosE61A0z7RWr5WP\n\tEA2W1JwkRDf6PbkxuYFbOMGeh3mABOX22tbYYJQNfj4x6LZ4wg==","X-Google-Smtp-Source":"ABdhPJxtPw3tBvRbwloD/whUxvz+X4lGCgmgwJKRMBsrQp8QyOMGtIpXcJ/au0YFBXohxcq7Dow8kWoIUXbZfi05/1Y=","X-Received":"by 2002:a17:906:280f:: with SMTP id\n\tr15mr9619303ejc.559.1632871801083; \n\tTue, 28 Sep 2021 16:30:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-3-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 29 Sep 2021 08:29:50 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19956,"web_url":"https://patchwork.libcamera.org/comment/19956/","msgid":"<305fe168-0479-e85f-d6f5-2e08d4d8cff4@ideasonboard.com>","date":"2021-09-29T05:40:08","subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 9/29/21 4:59 AM, Hirokazu Honda wrote:\n> Hi Umang, thank you for the patch.\n>\n> On Wed, Sep 29, 2021 at 6:07 AM Laurent Pinchart\n> wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> On Tue, Sep 28, 2021 at 09:55:35PM +0530, Umang Jain wrote:\n>>> The descriptors_ map holds Camera3RequestDescriptor(s) which are\n>>> per-capture requests placed by the framework to libcamera HAL.\n>>> CameraDevice::requestComplete() looks for the descriptor for which the\n>>> camera request has been completed and removes it from the map.\n>>> Since the requests are placed in form of FIFO and the framework expects\n>>> the order of completion to be FIFO as well, this calls for a need of\n>>> a queue rather than a std::map.\n>>>\n>>> This patch still keeps the same lifetime of Camera3RequestDescriptor as\n>>> before i.e. in the requestComplete(). Previously, a descriptor was\n>>> extracted from the map and its lifetime was bound to requestComplete().\n>>> The lifetime is kept same by manually calling .pop_front() on the\n>>> queue. In the subsequent commit, this is likely to change with a\n>>> centralized location of dropping descriptors from the queue for request\n>>> completion.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> src/android/camera_device.cpp | 92 +++++++++++++++++++----------------\n>>> src/android/camera_device.h | 3 +-\n>>> 2 files changed, 52 insertions(+), 43 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index 9287ea07..a3b8a549 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -457,7 +457,9 @@ void CameraDevice::stop()\n>>> worker_.stop();\n>>> camera_->stop();\n>>>\n>>> - descriptors_.clear();\n>>> + while (!descriptors_.empty())\n>>> + descriptors_.pop();\n>> You can simplify this with\n>>\n>> descriptors_ = {};\n>>\n>>> +\n>>> state_ = State::Stopped;\n>>> }\n>>>\n>>> @@ -955,7 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>> * The descriptor and the associated memory reserved here are freed\n>>> * at request complete time.\n>>> */\n>>> - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n>>> + std::unique_ptr descriptor =\n>>> + std::make_unique(camera_.get(),\n>>> + camera3Request);\n> nit: I would use auto in the type declaration in using\n> std::make_unique because the type is obvious from the right formula.\n>\n>>> /*\n>>> * \\todo The Android request model is incremental, settings passed in\n>>> @@ -966,12 +970,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>> if (camera3Request->settings)\n>>> lastSettings_ = camera3Request->settings;\n>>> else\n>>> - descriptor.settings_ = lastSettings_;\n>>> + descriptor->settings_ = lastSettings_;\n>>> +\n>>> + LOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n>>> + << \" with \" << descriptor->buffers_.size() << \" streams\";\n>>>\n>>> - LOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n>>> - << \" with \" << descriptor.buffers_.size() << \" streams\";\n>>> - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n>>> - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n>>> + for (const auto &[i, camera3Buffer] : utils::enumerate(descriptor->buffers_)) {\n>>> camera3_stream *camera3Stream = camera3Buffer.stream;\n>>> CameraStream *cameraStream = static_cast(camera3Stream->priv);\n>>>\n>>> @@ -1007,12 +1011,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>> * associate it with the Camera3RequestDescriptor for\n>>> * lifetime management only.\n>>> */\n>>> - descriptor.frameBuffers_.push_back(\n>>> + descriptor->frameBuffers_.push_back(\n>>> createFrameBuffer(*camera3Buffer.buffer,\n>>> cameraStream->configuration().pixelFormat,\n>>> cameraStream->configuration().size));\n>>>\n>>> - buffer = descriptor.frameBuffers_.back().get();\n>>> + buffer = descriptor->frameBuffers_.back().get();\n>>> acquireFence = camera3Buffer.acquire_fence;\n>>> LOG(HAL, Debug) << ss.str() << \" (direct)\";\n>>> break;\n>>> @@ -1035,7 +1039,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>> return -ENOMEM;\n>>> }\n>>>\n>>> - descriptor.request_->addBuffer(cameraStream->stream(), buffer,\n>>> + descriptor->request_->addBuffer(cameraStream->stream(), buffer,\n>>> acquireFence);\n>>> }\n>>>\n>>> @@ -1043,7 +1047,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>> * Translate controls from Android to libcamera and queue the request\n>>> * to the CameraWorker thread.\n>>> */\n>>> - int ret = processControls(&descriptor);\n>>> + int ret = processControls(descriptor.get());\n>>> if (ret)\n>>> return ret;\n>>>\n>>> @@ -1071,11 +1075,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>> state_ = State::Running;\n>>> }\n>>>\n>>> - CaptureRequest *request = descriptor.request_.get();\n>>> + CaptureRequest *request = descriptor->request_.get();\n>>>\n>>> {\n>>> MutexLocker descriptorsLock(descriptorsMutex_);\n>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>>> + descriptors_.push(std::move(descriptor));\n>>> }\n>>>\n>>> worker_.queueRequest(request);\n>>> @@ -1085,26 +1089,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>\n>>> void CameraDevice::requestComplete(Request *request)\n>>> {\n>>> - decltype(descriptors_)::node_type node;\n>>> + Camera3RequestDescriptor *descriptor;\n>>> {\n>>> MutexLocker descriptorsLock(descriptorsMutex_);\n>>> - auto it = descriptors_.find(request->cookie());\n>>> - if (it == descriptors_.end()) {\n>>> - /*\n>>> - * \\todo Clarify if the Camera has to be closed on\n>>> - * ERROR_DEVICE and possibly demote the Fatal to simple\n>>> - * Error.\n>>> - */\n>>> - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>>> - LOG(HAL, Fatal)\n>>> - << \"Unknown request: \" << request->cookie();\n>>> + ASSERT(!descriptors_.empty());\n>>> + descriptor = descriptors_.front().get();\n> Why isn't descriptor popped here?\n> Is there any case that a descriptor should be kept?\n> The original code removes the descriptor from descriptors_.\n> This changes the logic.\n\n\nA bit yes, but it does respect the lifetime of the descriptor as before \n(as the std::map)\n\nProvided that this is a transient patch that works on descriptors_, I \nprefer to be a little flexible here with logic. The popping of \ndescriptors_ has been moved to a centralized location in 3/3\n\n> I also think removing here is safe because although a pointer of node\n> in std::queue() is not invalidated by push operation, but indeed can\n> be invalidated by pop operation.\n> So removing here (while acquiring a lock) avoids the problem of using\n> an invalid pointer in the case descriptors_ are cleared in parallel.\n\n\nYes there might be cases in the future where this can happen, I can \nthink of some stop() or flush() clearing the descriptors_, but we are \ngoing to address them step by step.\n\nThe clearing of descriptors_ is something that shouldn't be happening by \ndesign, so we need to handle it as case-by-case basis (which is one the \ngoals we intend to pursue as part of this HAL rework), in near-future \npatch series.\n\n\n>\n> -Hiro\n>>> + }\n>>>\n>>> - return;\n>>> - }\n>>> + if (descriptor->request_->cookie() != request->cookie()) {\n>>> + /*\n>>> + * \\todo Clarify if the Camera has to be closed on\n>>> + * ERROR_DEVICE and possibly demote the Fatal to simple\n>>> + * Error.\n>>> + */\n>>> + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>>> + LOG(HAL, Fatal)\n>>> + << \"Out-of-order completion for request: \"\n>> s/://\n>>\n>> Reviewed-by: Laurent Pinchart \n>>\n>>> + << utils::hex(request->cookie());\n>>>\n>>> - node = descriptors_.extract(it);\n>>> + return;\n>>> }\n>>> - Camera3RequestDescriptor &descriptor = node.mapped();\n>>>\n>>> /*\n>>> * Prepare the capture result for the Android camera stack.\n>>> @@ -1113,9 +1117,9 @@ void CameraDevice::requestComplete(Request *request)\n>>> * post-processing/compression fails.\n>>> */\n>>> camera3_capture_result_t captureResult = {};\n>>> - captureResult.frame_number = descriptor.frameNumber_;\n>>> - captureResult.num_output_buffers = descriptor.buffers_.size();\n>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n>>> + captureResult.frame_number = descriptor->frameNumber_;\n>>> + captureResult.num_output_buffers = descriptor->buffers_.size();\n>>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>>> CameraStream *cameraStream =\n>>> static_cast(buffer.stream->priv);\n>>>\n>>> @@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request *request)\n>>> buffer.release_fence = -1;\n>>> buffer.status = CAMERA3_BUFFER_STATUS_OK;\n>>> }\n>>> - captureResult.output_buffers = descriptor.buffers_.data();\n>>> + captureResult.output_buffers = descriptor->buffers_.data();\n>>> captureResult.partial_result = 1;\n>>>\n>>> /*\n>>> @@ -1147,11 +1151,11 @@ void CameraDevice::requestComplete(Request *request)\n>>> << \" not successfully completed: \"\n>>> << request->status();\n>>>\n>>> - notifyError(descriptor.frameNumber_, nullptr,\n>>> + notifyError(descriptor->frameNumber_, nullptr,\n>>> CAMERA3_MSG_ERROR_REQUEST);\n>>>\n>>> captureResult.partial_result = 0;\n>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n>>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>>> /*\n>>> * Signal to the framework it has to handle fences that\n>>> * have not been waited on by setting the release fence\n>>> @@ -1164,6 +1168,7 @@ void CameraDevice::requestComplete(Request *request)\n>>>\n>>> callbacks_->process_capture_result(callbacks_, &captureResult);\n>>>\n> A lock is not acquired here?\n> MutexLocker descriptorsLock(descriptorsMutex_);\n\n\nops, yeah, I'll fix it!\n\n>\n>>> + descriptors_.pop();\n>>> return;\n>>> }\n>>>\n>>> @@ -1175,10 +1180,10 @@ void CameraDevice::requestComplete(Request *request)\n>>> */\n>>> uint64_t sensorTimestamp = static_cast(request->metadata()\n>>> .get(controls::SensorTimestamp));\n>>> - notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n>>> + notifyShutter(descriptor->frameNumber_, sensorTimestamp);\n>>>\n>>> LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n>>> - << descriptor.buffers_.size() << \" streams\";\n>>> + << descriptor->buffers_.size() << \" streams\";\n>>>\n>>> /*\n>>> * Generate the metadata associated with the captured buffers.\n>>> @@ -1186,16 +1191,16 @@ void CameraDevice::requestComplete(Request *request)\n>>> * Notify if the metadata generation has failed, but continue processing\n>>> * buffers and return an empty metadata pack.\n>>> */\n>>> - std::unique_ptr resultMetadata = getResultMetadata(descriptor);\n>>> + std::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n>>> if (!resultMetadata) {\n>>> - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n>>> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n>>>\n>>> /* The camera framework expects an empty metadata pack on error. */\n>>> resultMetadata = std::make_unique(0, 0);\n>>> }\n>>>\n>>> /* Handle post-processing. */\n>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n>>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>>> CameraStream *cameraStream =\n>>> static_cast(buffer.stream->priv);\n>>>\n>>> @@ -1206,13 +1211,13 @@ void CameraDevice::requestComplete(Request *request)\n>>> if (!src) {\n>>> LOG(HAL, Error) << \"Failed to find a source stream buffer\";\n>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>> - notifyError(descriptor.frameNumber_, buffer.stream,\n>>> + notifyError(descriptor->frameNumber_, buffer.stream,\n>>> CAMERA3_MSG_ERROR_BUFFER);\n>>> continue;\n>>> }\n>>>\n>>> int ret = cameraStream->process(*src, buffer,\n>>> - descriptor.settings_,\n>>> + descriptor->settings_,\n>>> resultMetadata.get());\n>>> /*\n>>> * Return the FrameBuffer to the CameraStream now that we're\n>>> @@ -1223,13 +1228,16 @@ void CameraDevice::requestComplete(Request *request)\n>>>\n>>> if (ret) {\n>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>> - notifyError(descriptor.frameNumber_, buffer.stream,\n>>> + notifyError(descriptor->frameNumber_, buffer.stream,\n>>> CAMERA3_MSG_ERROR_BUFFER);\n>>> }\n>>> }\n>>>\n>>> captureResult.result = resultMetadata->get();\n>>> callbacks_->process_capture_result(callbacks_, &captureResult);\n>>> +\n>>> + MutexLocker descriptorsLock(descriptorsMutex_);\n>>> + descriptors_.pop();\n>>> }\n>>>\n>>> std::string CameraDevice::logPrefix() const\n>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>> index 43eb5895..9ec510d5 100644\n>>> --- a/src/android/camera_device.h\n>>> +++ b/src/android/camera_device.h\n>>> @@ -10,6 +10,7 @@\n>>> #include \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>>\n>>> #include \n>>> @@ -125,7 +126,7 @@ private:\n>>> std::vector streams_;\n>>>\n>>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>>> - std::map descriptors_;\n>>> + std::queue> descriptors_;\n>>>\n>>> std::string maker_;\n>>> std::string model_;\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart","headers":{"Return-Path":"","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 65AD8BDC71\n\tfor ;\n\tWed, 29 Sep 2021 05:40:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D86C69193;\n\tWed, 29 Sep 2021 07:40:15 +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 DD1B7602DD\n\tfor ;\n\tWed, 29 Sep 2021 07:40:13 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 66E89D87;\n\tWed, 29 Sep 2021 07:40:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nFAWS6uT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632894013;\n\tbh=IaHyZuZT3yWod2qqOotDxLZ6HE5Dht4IEA4BdDRz83s=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=nFAWS6uTHz4TpDQVsaPiMDpT63NdTfndq2f9CMMgmRzvy4bsAxs1G96H82Nonm2qG\n\tGemL+Gbs1q3eM7Pt//VTCLBIQRKrn7dtPaLAZtP+xWKrXaYhJfYuzv41sxCiWHarX+\n\tVxY37Cbe1Y7J+rfH3WzoaXvEq2LdVB2GIiZtPTvo=","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-3-umang.jain@ideasonboard.com>\n\t\n\t","From":"Umang Jain ","Message-ID":"<305fe168-0479-e85f-d6f5-2e08d4d8cff4@ideasonboard.com>","Date":"Wed, 29 Sep 2021 11:10:08 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19958,"web_url":"https://patchwork.libcamera.org/comment/19958/","msgid":"","date":"2021-09-29T05:45:46","subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 9/29/21 11:10 AM, Umang Jain wrote:\n> Hi Hiro,\n>\n> On 9/29/21 4:59 AM, Hirokazu Honda wrote:\n>> Hi Umang, thank you for the patch.\n>>\n>> On Wed, Sep 29, 2021 at 6:07 AM Laurent Pinchart\n>> wrote:\n>>> Hi Umang,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Tue, Sep 28, 2021 at 09:55:35PM +0530, Umang Jain wrote:\n>>>> The descriptors_ map holds Camera3RequestDescriptor(s) which are\n>>>> per-capture requests placed by the framework to libcamera HAL.\n>>>> CameraDevice::requestComplete() looks for the descriptor for which the\n>>>> camera request has been completed and removes it from the map.\n>>>> Since the requests are placed in form of FIFO and the framework \n>>>> expects\n>>>> the order of completion to be FIFO as well, this calls for a need of\n>>>> a queue rather than a std::map.\n>>>>\n>>>> This patch still keeps the same lifetime of \n>>>> Camera3RequestDescriptor as\n>>>> before i.e. in the requestComplete(). Previously, a descriptor was\n>>>> extracted from the map and its lifetime was bound to \n>>>> requestComplete().\n>>>> The lifetime is kept same by manually calling .pop_front() on the\n>>>> queue. In the subsequent commit, this is likely to change with a\n>>>> centralized location of dropping descriptors from the queue for \n>>>> request\n>>>> completion.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>>   src/android/camera_device.cpp | 92 \n>>>> +++++++++++++++++++----------------\n>>>>   src/android/camera_device.h   |  3 +-\n>>>>   2 files changed, 52 insertions(+), 43 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp \n>>>> b/src/android/camera_device.cpp\n>>>> index 9287ea07..a3b8a549 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -457,7 +457,9 @@ void CameraDevice::stop()\n>>>>        worker_.stop();\n>>>>        camera_->stop();\n>>>>\n>>>> -     descriptors_.clear();\n>>>> +     while (!descriptors_.empty())\n>>>> +             descriptors_.pop();\n>>> You can simplify this with\n>>>\n>>>          descriptors_ = {};\n>>>\n>>>> +\n>>>>        state_ = State::Stopped;\n>>>>   }\n>>>>\n>>>> @@ -955,7 +957,9 @@ int \n>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>> *camera3Reques\n>>>>         * The descriptor and the associated memory reserved here \n>>>> are freed\n>>>>         * at request complete time.\n>>>>         */\n>>>> -     Camera3RequestDescriptor descriptor(camera_.get(), \n>>>> camera3Request);\n>>>> +     std::unique_ptr descriptor =\n>>>> + std::make_unique(camera_.get(),\n>>>> + camera3Request);\n>> nit: I would use auto in the type declaration in using\n>> std::make_unique because the type is obvious from the right formula.\n>>\n>>>>        /*\n>>>>         * \\todo The Android request model is incremental, settings \n>>>> passed in\n>>>> @@ -966,12 +970,12 @@ int \n>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>> *camera3Reques\n>>>>        if (camera3Request->settings)\n>>>>                lastSettings_ = camera3Request->settings;\n>>>>        else\n>>>> -             descriptor.settings_ = lastSettings_;\n>>>> +             descriptor->settings_ = lastSettings_;\n>>>> +\n>>>> +     LOG(HAL, Debug) << \"Queueing request \" << \n>>>> descriptor->request_->cookie()\n>>>> +                     << \" with \" << descriptor->buffers_.size() << \n>>>> \" streams\";\n>>>>\n>>>> -     LOG(HAL, Debug) << \"Queueing request \" << \n>>>> descriptor.request_->cookie()\n>>>> -                     << \" with \" << descriptor.buffers_.size() << \n>>>> \" streams\";\n>>>> -     for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n>>>> -             const camera3_stream_buffer_t &camera3Buffer = \n>>>> descriptor.buffers_[i];\n>>>> +     for (const auto &[i, camera3Buffer] : \n>>>> utils::enumerate(descriptor->buffers_)) {\n>>>>                camera3_stream *camera3Stream = camera3Buffer.stream;\n>>>>                CameraStream *cameraStream = \n>>>> static_cast(camera3Stream->priv);\n>>>>\n>>>> @@ -1007,12 +1011,12 @@ int \n>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>> *camera3Reques\n>>>>                         * associate it with the \n>>>> Camera3RequestDescriptor for\n>>>>                         * lifetime management only.\n>>>>                         */\n>>>> -                     descriptor.frameBuffers_.push_back(\n>>>> + descriptor->frameBuffers_.push_back(\n>>>> createFrameBuffer(*camera3Buffer.buffer,\n>>>> cameraStream->configuration().pixelFormat,\n>>>> cameraStream->configuration().size));\n>>>>\n>>>> -                     buffer = descriptor.frameBuffers_.back().get();\n>>>> +                     buffer = descriptor->frameBuffers_.back().get();\n>>>>                        acquireFence = camera3Buffer.acquire_fence;\n>>>>                        LOG(HAL, Debug) << ss.str() << \" (direct)\";\n>>>>                        break;\n>>>> @@ -1035,7 +1039,7 @@ int \n>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>> *camera3Reques\n>>>>                        return -ENOMEM;\n>>>>                }\n>>>>\n>>>> - descriptor.request_->addBuffer(cameraStream->stream(), buffer,\n>>>> + descriptor->request_->addBuffer(cameraStream->stream(), buffer,\n>>>>                                               acquireFence);\n>>>>        }\n>>>>\n>>>> @@ -1043,7 +1047,7 @@ int \n>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>> *camera3Reques\n>>>>         * Translate controls from Android to libcamera and queue \n>>>> the request\n>>>>         * to the CameraWorker thread.\n>>>>         */\n>>>> -     int ret = processControls(&descriptor);\n>>>> +     int ret = processControls(descriptor.get());\n>>>>        if (ret)\n>>>>                return ret;\n>>>>\n>>>> @@ -1071,11 +1075,11 @@ int \n>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>> *camera3Reques\n>>>>                state_ = State::Running;\n>>>>        }\n>>>>\n>>>> -     CaptureRequest *request = descriptor.request_.get();\n>>>> +     CaptureRequest *request = descriptor->request_.get();\n>>>>\n>>>>        {\n>>>>                MutexLocker descriptorsLock(descriptorsMutex_);\n>>>> -             descriptors_[descriptor.request_->cookie()] = \n>>>> std::move(descriptor);\n>>>> +             descriptors_.push(std::move(descriptor));\n>>>>        }\n>>>>\n>>>>        worker_.queueRequest(request);\n>>>> @@ -1085,26 +1089,26 @@ int \n>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>> *camera3Reques\n>>>>\n>>>>   void CameraDevice::requestComplete(Request *request)\n>>>>   {\n>>>> -     decltype(descriptors_)::node_type node;\n>>>> +     Camera3RequestDescriptor *descriptor;\n>>>>        {\n>>>>                MutexLocker descriptorsLock(descriptorsMutex_);\n>>>> -             auto it = descriptors_.find(request->cookie());\n>>>> -             if (it == descriptors_.end()) {\n>>>> -                     /*\n>>>> -                      * \\todo Clarify if the Camera has to be \n>>>> closed on\n>>>> -                      * ERROR_DEVICE and possibly demote the Fatal \n>>>> to simple\n>>>> -                      * Error.\n>>>> -                      */\n>>>> -                     notifyError(0, nullptr, \n>>>> CAMERA3_MSG_ERROR_DEVICE);\n>>>> -                     LOG(HAL, Fatal)\n>>>> -                             << \"Unknown request: \" << \n>>>> request->cookie();\n>>>> +             ASSERT(!descriptors_.empty());\n>>>> +             descriptor = descriptors_.front().get();\n>> Why isn't descriptor popped here?\n>> Is there any case that a descriptor should be kept?\n>> The original code removes the descriptor from descriptors_.\n>> This changes the logic.\n>\n>\n> A bit yes, but it does respect the lifetime of the descriptor as \n> before (as the std::map)\n>\n> Provided that this is a transient patch that works on descriptors_, I \n> prefer to be a little flexible here with logic. The popping of \n> descriptors_ has been moved to a centralized location in 3/3\n>\n>> I also think removing here is safe because although a pointer of node\n>> in std::queue() is not invalidated by push operation, but indeed can\n>> be invalidated by pop operation.\n>> So removing here (while acquiring a lock) avoids the problem of using\n>> an invalid pointer in the case descriptors_ are cleared in parallel.\n>\n>\n> Yes there might be cases in the future where this can happen, I can \n> think of some stop() or flush() clearing the descriptors_, but we are \n> going to address them step by step.\n>\n> The clearing of descriptors_ is something that shouldn't be happening \n> by design, so we need to handle it as case-by-case basis (which is one \n> the goals we intend to pursue as part of this HAL rework), in \n> near-future patch series.\n\n\ns/ shouldn't be happening / shouldn't be happening in parallel /\n\n>\n>\n>>\n>> -Hiro\n>>>> +     }\n>>>>\n>>>> -                     return;\n>>>> -             }\n>>>> +     if (descriptor->request_->cookie() != request->cookie()) {\n>>>> +             /*\n>>>> +              * \\todo Clarify if the Camera has to be closed on\n>>>> +              * ERROR_DEVICE and possibly demote the Fatal to simple\n>>>> +              * Error.\n>>>> +              */\n>>>> +             notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>>>> +             LOG(HAL, Fatal)\n>>>> +                     << \"Out-of-order completion for request: \"\n>>> s/://\n>>>\n>>> Reviewed-by: Laurent Pinchart \n>>>\n>>>> +                     << utils::hex(request->cookie());\n>>>>\n>>>> -             node = descriptors_.extract(it);\n>>>> +             return;\n>>>>        }\n>>>> -     Camera3RequestDescriptor &descriptor = node.mapped();\n>>>>\n>>>>        /*\n>>>>         * Prepare the capture result for the Android camera stack.\n>>>> @@ -1113,9 +1117,9 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>         * post-processing/compression fails.\n>>>>         */\n>>>>        camera3_capture_result_t captureResult = {};\n>>>> -     captureResult.frame_number = descriptor.frameNumber_;\n>>>> -     captureResult.num_output_buffers = descriptor.buffers_.size();\n>>>> -     for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n>>>> +     captureResult.frame_number = descriptor->frameNumber_;\n>>>> +     captureResult.num_output_buffers = descriptor->buffers_.size();\n>>>> +     for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>>>>                CameraStream *cameraStream =\n>>>>                        static_cast>>> *>(buffer.stream->priv);\n>>>>\n>>>> @@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>                buffer.release_fence = -1;\n>>>>                buffer.status = CAMERA3_BUFFER_STATUS_OK;\n>>>>        }\n>>>> -     captureResult.output_buffers = descriptor.buffers_.data();\n>>>> +     captureResult.output_buffers = descriptor->buffers_.data();\n>>>>        captureResult.partial_result = 1;\n>>>>\n>>>>        /*\n>>>> @@ -1147,11 +1151,11 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>                                << \" not successfully completed: \"\n>>>>                                << request->status();\n>>>>\n>>>> -             notifyError(descriptor.frameNumber_, nullptr,\n>>>> +             notifyError(descriptor->frameNumber_, nullptr,\n>>>>                            CAMERA3_MSG_ERROR_REQUEST);\n>>>>\n>>>>                captureResult.partial_result = 0;\n>>>> -             for (camera3_stream_buffer_t &buffer : \n>>>> descriptor.buffers_) {\n>>>> +             for (camera3_stream_buffer_t &buffer : \n>>>> descriptor->buffers_) {\n>>>>                        /*\n>>>>                         * Signal to the framework it has to handle \n>>>> fences that\n>>>>                         * have not been waited on by setting the \n>>>> release fence\n>>>> @@ -1164,6 +1168,7 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>\n>>>> callbacks_->process_capture_result(callbacks_, &captureResult);\n>>>>\n>> A lock is not acquired here?\n>> MutexLocker descriptorsLock(descriptorsMutex_);\n>\n>\n> ops, yeah, I'll fix it!\n>\n>>\n>>>> +             descriptors_.pop();\n>>>>                return;\n>>>>        }\n>>>>\n>>>> @@ -1175,10 +1180,10 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>         */\n>>>>        uint64_t sensorTimestamp = \n>>>> static_cast(request->metadata()\n>>>> .get(controls::SensorTimestamp));\n>>>> -     notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n>>>> +     notifyShutter(descriptor->frameNumber_, sensorTimestamp);\n>>>>\n>>>>        LOG(HAL, Debug) << \"Request \" << request->cookie() << \" \n>>>> completed with \"\n>>>> -                     << descriptor.buffers_.size() << \" streams\";\n>>>> +                     << descriptor->buffers_.size() << \" streams\";\n>>>>\n>>>>        /*\n>>>>         * Generate the metadata associated with the captured buffers.\n>>>> @@ -1186,16 +1191,16 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>         * Notify if the metadata generation has failed, but \n>>>> continue processing\n>>>>         * buffers and return an empty metadata pack.\n>>>>         */\n>>>> -     std::unique_ptr resultMetadata = \n>>>> getResultMetadata(descriptor);\n>>>> +     std::unique_ptr resultMetadata = \n>>>> getResultMetadata(*descriptor);\n>>>>        if (!resultMetadata) {\n>>>> -             notifyError(descriptor.frameNumber_, nullptr, \n>>>> CAMERA3_MSG_ERROR_RESULT);\n>>>> +             notifyError(descriptor->frameNumber_, nullptr, \n>>>> CAMERA3_MSG_ERROR_RESULT);\n>>>>\n>>>>                /* The camera framework expects an empty metadata \n>>>> pack on error. */\n>>>>                resultMetadata = std::make_unique(0, \n>>>> 0);\n>>>>        }\n>>>>\n>>>>        /* Handle post-processing. */\n>>>> -     for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n>>>> +     for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>>>>                CameraStream *cameraStream =\n>>>>                        static_cast>>> *>(buffer.stream->priv);\n>>>>\n>>>> @@ -1206,13 +1211,13 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>                if (!src) {\n>>>>                        LOG(HAL, Error) << \"Failed to find a source \n>>>> stream buffer\";\n>>>>                        buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>> -                     notifyError(descriptor.frameNumber_, \n>>>> buffer.stream,\n>>>> + notifyError(descriptor->frameNumber_, buffer.stream,\n>>>> CAMERA3_MSG_ERROR_BUFFER);\n>>>>                        continue;\n>>>>                }\n>>>>\n>>>>                int ret = cameraStream->process(*src, buffer,\n>>>> - descriptor.settings_,\n>>>> + descriptor->settings_,\n>>>> resultMetadata.get());\n>>>>                /*\n>>>>                 * Return the FrameBuffer to the CameraStream now \n>>>> that we're\n>>>> @@ -1223,13 +1228,16 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>\n>>>>                if (ret) {\n>>>>                        buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>> -                     notifyError(descriptor.frameNumber_, \n>>>> buffer.stream,\n>>>> + notifyError(descriptor->frameNumber_, buffer.stream,\n>>>> CAMERA3_MSG_ERROR_BUFFER);\n>>>>                }\n>>>>        }\n>>>>\n>>>>        captureResult.result = resultMetadata->get();\n>>>>        callbacks_->process_capture_result(callbacks_, &captureResult);\n>>>> +\n>>>> +     MutexLocker descriptorsLock(descriptorsMutex_);\n>>>> +     descriptors_.pop();\n>>>>   }\n>>>>\n>>>>   std::string CameraDevice::logPrefix() const\n>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>>> index 43eb5895..9ec510d5 100644\n>>>> --- a/src/android/camera_device.h\n>>>> +++ b/src/android/camera_device.h\n>>>> @@ -10,6 +10,7 @@\n>>>>   #include \n>>>>   #include \n>>>>   #include \n>>>> +#include \n>>>>   #include \n>>>>\n>>>>   #include \n>>>> @@ -125,7 +126,7 @@ private:\n>>>>        std::vector streams_;\n>>>>\n>>>>        libcamera::Mutex descriptorsMutex_; /* Protects \n>>>> descriptors_. */\n>>>> -     std::map descriptors_;\n>>>> + std::queue> descriptors_;\n>>>>\n>>>>        std::string maker_;\n>>>>        std::string model_;\n>>> -- \n>>> Regards,\n>>>\n>>> Laurent Pinchart","headers":{"Return-Path":"","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 2BD02BDC71\n\tfor ;\n\tWed, 29 Sep 2021 05:45:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 978EE69193;\n\tWed, 29 Sep 2021 07:45:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81E30602DD\n\tfor ;\n\tWed, 29 Sep 2021 07:45:53 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C80E9D87;\n\tWed, 29 Sep 2021 07:45:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sz7Dx5R3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632894353;\n\tbh=QLCgY06qD2C2g+qfXRSDNoGH4MjvAlNPK1QNPMfpHp0=;\n\th=Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=sz7Dx5R3frqPUlXbnbIwZF3W6pi9YjaH6Uc9O1Lo6nynLcb9kD+X0rPJqEtkUxopk\n\ttVIuaJLiyuMAzdcYMPsY+h7mRA0jsFIt7UbEwSznQIdzNOWoTBofjMgbgZvBQyFbuF\n\tnCq62QhXIMYM+3umFY1o9rzHhKfVsxgp4/0JrsvY=","From":"Umang Jain ","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-3-umang.jain@ideasonboard.com>\n\t\n\t\n\t<305fe168-0479-e85f-d6f5-2e08d4d8cff4@ideasonboard.com>","Message-ID":"","Date":"Wed, 29 Sep 2021 11:15:46 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<305fe168-0479-e85f-d6f5-2e08d4d8cff4@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19961,"web_url":"https://patchwork.libcamera.org/comment/19961/","msgid":"","date":"2021-09-29T07:02:49","subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Sep 29, 2021 at 11:10:08AM +0530, Umang Jain wrote:\n> On 9/29/21 4:59 AM, Hirokazu Honda wrote:\n> > On Wed, Sep 29, 2021 at 6:07 AM Laurent Pinchart wrote:\n> >> On Tue, Sep 28, 2021 at 09:55:35PM +0530, Umang Jain wrote:\n> >>> The descriptors_ map holds Camera3RequestDescriptor(s) which are\n> >>> per-capture requests placed by the framework to libcamera HAL.\n> >>> CameraDevice::requestComplete() looks for the descriptor for which the\n> >>> camera request has been completed and removes it from the map.\n> >>> Since the requests are placed in form of FIFO and the framework expects\n> >>> the order of completion to be FIFO as well, this calls for a need of\n> >>> a queue rather than a std::map.\n> >>>\n> >>> This patch still keeps the same lifetime of Camera3RequestDescriptor as\n> >>> before i.e. in the requestComplete(). Previously, a descriptor was\n> >>> extracted from the map and its lifetime was bound to requestComplete().\n> >>> The lifetime is kept same by manually calling .pop_front() on the\n> >>> queue. In the subsequent commit, this is likely to change with a\n> >>> centralized location of dropping descriptors from the queue for request\n> >>> completion.\n> >>>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> src/android/camera_device.cpp | 92 +++++++++++++++++++----------------\n> >>> src/android/camera_device.h | 3 +-\n> >>> 2 files changed, 52 insertions(+), 43 deletions(-)\n> >>>\n> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>> index 9287ea07..a3b8a549 100644\n> >>> --- a/src/android/camera_device.cpp\n> >>> +++ b/src/android/camera_device.cpp\n> >>> @@ -457,7 +457,9 @@ void CameraDevice::stop()\n> >>> worker_.stop();\n> >>> camera_->stop();\n> >>>\n> >>> - descriptors_.clear();\n> >>> + while (!descriptors_.empty())\n> >>> + descriptors_.pop();\n> >>\n> >> You can simplify this with\n> >>\n> >> descriptors_ = {};\n> >>\n> >>> +\n> >>> state_ = State::Stopped;\n> >>> }\n> >>>\n> >>> @@ -955,7 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>> * The descriptor and the associated memory reserved here are freed\n> >>> * at request complete time.\n> >>> */\n> >>> - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n> >>> + std::unique_ptr descriptor =\n> >>> + std::make_unique(camera_.get(),\n> >>> + camera3Request);\n> >\n> > nit: I would use auto in the type declaration in using\n> > std::make_unique because the type is obvious from the right formula.\n> >\n> >>> /*\n> >>> * \\todo The Android request model is incremental, settings passed in\n> >>> @@ -966,12 +970,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>> if (camera3Request->settings)\n> >>> lastSettings_ = camera3Request->settings;\n> >>> else\n> >>> - descriptor.settings_ = lastSettings_;\n> >>> + descriptor->settings_ = lastSettings_;\n> >>> +\n> >>> + LOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> >>> + << \" with \" << descriptor->buffers_.size() << \" streams\";\n> >>>\n> >>> - LOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n> >>> - << \" with \" << descriptor.buffers_.size() << \" streams\";\n> >>> - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> >>> - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> >>> + for (const auto &[i, camera3Buffer] : utils::enumerate(descriptor->buffers_)) {\n> >>> camera3_stream *camera3Stream = camera3Buffer.stream;\n> >>> CameraStream *cameraStream = static_cast(camera3Stream->priv);\n> >>>\n> >>> @@ -1007,12 +1011,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>> * associate it with the Camera3RequestDescriptor for\n> >>> * lifetime management only.\n> >>> */\n> >>> - descriptor.frameBuffers_.push_back(\n> >>> + descriptor->frameBuffers_.push_back(\n> >>> createFrameBuffer(*camera3Buffer.buffer,\n> >>> cameraStream->configuration().pixelFormat,\n> >>> cameraStream->configuration().size));\n> >>>\n> >>> - buffer = descriptor.frameBuffers_.back().get();\n> >>> + buffer = descriptor->frameBuffers_.back().get();\n> >>> acquireFence = camera3Buffer.acquire_fence;\n> >>> LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> >>> break;\n> >>> @@ -1035,7 +1039,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>> return -ENOMEM;\n> >>> }\n> >>>\n> >>> - descriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> >>> + descriptor->request_->addBuffer(cameraStream->stream(), buffer,\n> >>> acquireFence);\n> >>> }\n> >>>\n> >>> @@ -1043,7 +1047,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>> * Translate controls from Android to libcamera and queue the request\n> >>> * to the CameraWorker thread.\n> >>> */\n> >>> - int ret = processControls(&descriptor);\n> >>> + int ret = processControls(descriptor.get());\n> >>> if (ret)\n> >>> return ret;\n> >>>\n> >>> @@ -1071,11 +1075,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>> state_ = State::Running;\n> >>> }\n> >>>\n> >>> - CaptureRequest *request = descriptor.request_.get();\n> >>> + CaptureRequest *request = descriptor->request_.get();\n> >>>\n> >>> {\n> >>> MutexLocker descriptorsLock(descriptorsMutex_);\n> >>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> >>> + descriptors_.push(std::move(descriptor));\n> >>> }\n> >>>\n> >>> worker_.queueRequest(request);\n> >>> @@ -1085,26 +1089,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>\n> >>> void CameraDevice::requestComplete(Request *request)\n> >>> {\n> >>> - decltype(descriptors_)::node_type node;\n> >>> + Camera3RequestDescriptor *descriptor;\n> >>> {\n> >>> MutexLocker descriptorsLock(descriptorsMutex_);\n> >>> - auto it = descriptors_.find(request->cookie());\n> >>> - if (it == descriptors_.end()) {\n> >>> - /*\n> >>> - * \\todo Clarify if the Camera has to be closed on\n> >>> - * ERROR_DEVICE and possibly demote the Fatal to simple\n> >>> - * Error.\n> >>> - */\n> >>> - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> >>> - LOG(HAL, Fatal)\n> >>> - << \"Unknown request: \" << request->cookie();\n> >>> + ASSERT(!descriptors_.empty());\n> >>> + descriptor = descriptors_.front().get();\n> >\n> > Why isn't descriptor popped here?\n> > Is there any case that a descriptor should be kept?\n> > The original code removes the descriptor from descriptors_.\n> > This changes the logic.\n> \n> A bit yes, but it does respect the lifetime of the descriptor as before \n> (as the std::map)\n> \n> Provided that this is a transient patch that works on descriptors_, I \n> prefer to be a little flexible here with logic. The popping of \n> descriptors_ has been moved to a centralized location in 3/3\n\nLet's also note that the upcoming async post-processing series will\nrequire keeping the descriptor in the queue until the post-processing is\ncomplete.\n\n> > I also think removing here is safe because although a pointer of node\n> > in std::queue() is not invalidated by push operation, but indeed can\n> > be invalidated by pop operation.\n> > So removing here (while acquiring a lock) avoids the problem of using\n> > an invalid pointer in the case descriptors_ are cleared in parallel.\n> \n> Yes there might be cases in the future where this can happen, I can \n> think of some stop() or flush() clearing the descriptors_, but we are \n> going to address them step by step.\n> \n> The clearing of descriptors_ is something that shouldn't be happening by \n> design, so we need to handle it as case-by-case basis (which is one the \n> goals we intend to pursue as part of this HAL rework), in near-future \n> patch series.\n> \n> >>> + }\n> >>>\n> >>> - return;\n> >>> - }\n> >>> + if (descriptor->request_->cookie() != request->cookie()) {\n> >>> + /*\n> >>> + * \\todo Clarify if the Camera has to be closed on\n> >>> + * ERROR_DEVICE and possibly demote the Fatal to simple\n> >>> + * Error.\n> >>> + */\n> >>> + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> >>> + LOG(HAL, Fatal)\n> >>> + << \"Out-of-order completion for request: \"\n> >> s/://\n> >>\n> >> Reviewed-by: Laurent Pinchart \n> >>\n> >>> + << utils::hex(request->cookie());\n> >>>\n> >>> - node = descriptors_.extract(it);\n> >>> + return;\n> >>> }\n> >>> - Camera3RequestDescriptor &descriptor = node.mapped();\n> >>>\n> >>> /*\n> >>> * Prepare the capture result for the Android camera stack.\n> >>> @@ -1113,9 +1117,9 @@ void CameraDevice::requestComplete(Request *request)\n> >>> * post-processing/compression fails.\n> >>> */\n> >>> camera3_capture_result_t captureResult = {};\n> >>> - captureResult.frame_number = descriptor.frameNumber_;\n> >>> - captureResult.num_output_buffers = descriptor.buffers_.size();\n> >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> >>> + captureResult.frame_number = descriptor->frameNumber_;\n> >>> + captureResult.num_output_buffers = descriptor->buffers_.size();\n> >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> >>> CameraStream *cameraStream =\n> >>> static_cast(buffer.stream->priv);\n> >>>\n> >>> @@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request *request)\n> >>> buffer.release_fence = -1;\n> >>> buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> >>> }\n> >>> - captureResult.output_buffers = descriptor.buffers_.data();\n> >>> + captureResult.output_buffers = descriptor->buffers_.data();\n> >>> captureResult.partial_result = 1;\n> >>>\n> >>> /*\n> >>> @@ -1147,11 +1151,11 @@ void CameraDevice::requestComplete(Request *request)\n> >>> << \" not successfully completed: \"\n> >>> << request->status();\n> >>>\n> >>> - notifyError(descriptor.frameNumber_, nullptr,\n> >>> + notifyError(descriptor->frameNumber_, nullptr,\n> >>> CAMERA3_MSG_ERROR_REQUEST);\n> >>>\n> >>> captureResult.partial_result = 0;\n> >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> >>> /*\n> >>> * Signal to the framework it has to handle fences that\n> >>> * have not been waited on by setting the release fence\n> >>> @@ -1164,6 +1168,7 @@ void CameraDevice::requestComplete(Request *request)\n> >>>\n> >>> callbacks_->process_capture_result(callbacks_, &captureResult);\n> >>>\n> >\n> > A lock is not acquired here?\n> > MutexLocker descriptorsLock(descriptorsMutex_);\n> \n> ops, yeah, I'll fix it!\n> \n> >>> + descriptors_.pop();\n> >>> return;\n> >>> }\n> >>>\n> >>> @@ -1175,10 +1180,10 @@ void CameraDevice::requestComplete(Request *request)\n> >>> */\n> >>> uint64_t sensorTimestamp = static_cast(request->metadata()\n> >>> .get(controls::SensorTimestamp));\n> >>> - notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> >>> + notifyShutter(descriptor->frameNumber_, sensorTimestamp);\n> >>>\n> >>> LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> >>> - << descriptor.buffers_.size() << \" streams\";\n> >>> + << descriptor->buffers_.size() << \" streams\";\n> >>>\n> >>> /*\n> >>> * Generate the metadata associated with the captured buffers.\n> >>> @@ -1186,16 +1191,16 @@ void CameraDevice::requestComplete(Request *request)\n> >>> * Notify if the metadata generation has failed, but continue processing\n> >>> * buffers and return an empty metadata pack.\n> >>> */\n> >>> - std::unique_ptr resultMetadata = getResultMetadata(descriptor);\n> >>> + std::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n> >>> if (!resultMetadata) {\n> >>> - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> >>> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> >>>\n> >>> /* The camera framework expects an empty metadata pack on error. */\n> >>> resultMetadata = std::make_unique(0, 0);\n> >>> }\n> >>>\n> >>> /* Handle post-processing. */\n> >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> >>> CameraStream *cameraStream =\n> >>> static_cast(buffer.stream->priv);\n> >>>\n> >>> @@ -1206,13 +1211,13 @@ void CameraDevice::requestComplete(Request *request)\n> >>> if (!src) {\n> >>> LOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> >>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >>> - notifyError(descriptor.frameNumber_, buffer.stream,\n> >>> + notifyError(descriptor->frameNumber_, buffer.stream,\n> >>> CAMERA3_MSG_ERROR_BUFFER);\n> >>> continue;\n> >>> }\n> >>>\n> >>> int ret = cameraStream->process(*src, buffer,\n> >>> - descriptor.settings_,\n> >>> + descriptor->settings_,\n> >>> resultMetadata.get());\n> >>> /*\n> >>> * Return the FrameBuffer to the CameraStream now that we're\n> >>> @@ -1223,13 +1228,16 @@ void CameraDevice::requestComplete(Request *request)\n> >>>\n> >>> if (ret) {\n> >>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >>> - notifyError(descriptor.frameNumber_, buffer.stream,\n> >>> + notifyError(descriptor->frameNumber_, buffer.stream,\n> >>> CAMERA3_MSG_ERROR_BUFFER);\n> >>> }\n> >>> }\n> >>>\n> >>> captureResult.result = resultMetadata->get();\n> >>> callbacks_->process_capture_result(callbacks_, &captureResult);\n> >>> +\n> >>> + MutexLocker descriptorsLock(descriptorsMutex_);\n> >>> + descriptors_.pop();\n> >>> }\n> >>>\n> >>> std::string CameraDevice::logPrefix() const\n> >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >>> index 43eb5895..9ec510d5 100644\n> >>> --- a/src/android/camera_device.h\n> >>> +++ b/src/android/camera_device.h\n> >>> @@ -10,6 +10,7 @@\n> >>> #include \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>>\n> >>> #include \n> >>> @@ -125,7 +126,7 @@ private:\n> >>> std::vector streams_;\n> >>>\n> >>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >>> - std::map descriptors_;\n> >>> + std::queue> descriptors_;\n> >>>\n> >>> std::string maker_;\n> >>> std::string model_;","headers":{"Return-Path":"","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 5CBDCBDC71\n\tfor ;\n\tWed, 29 Sep 2021 07:02:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D304F6918B;\n\tWed, 29 Sep 2021 09:02:53 +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 A56796918A\n\tfor ;\n\tWed, 29 Sep 2021 09:02:51 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 07849DEE;\n\tWed, 29 Sep 2021 09:02:50 +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=\"UpRb/dku\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632898971;\n\tbh=1fZezhu/yCt4kYxSSqoyjeFRNlqWGwBj2RAAuRE/uA8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UpRb/dkun/jfUqmxee1W7Do6uNbgPuDoVGRotzQw7HrR2pAenj4cLxWFN1QBVeD0+\n\tLG7HwCa+2ii5XbEnsYHRuqiaZYLQ9LpZH/Ao/Z1EerGpFda2DlQb6A9/wQWM/6QA/O\n\tn8qt3DmlnIossZduFSxpoCC/ZIM1e/RWU7Wr0kGA=","Date":"Wed, 29 Sep 2021 10:02:49 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-3-umang.jain@ideasonboard.com>\n\t\n\t\n\t<305fe168-0479-e85f-d6f5-2e08d4d8cff4@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<305fe168-0479-e85f-d6f5-2e08d4d8cff4@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19963,"web_url":"https://patchwork.libcamera.org/comment/19963/","msgid":"","date":"2021-09-29T07:18:38","subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for replying.\n\nOn Wed, Sep 29, 2021 at 4:02 PM Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> On Wed, Sep 29, 2021 at 11:10:08AM +0530, Umang Jain wrote:\n> > On 9/29/21 4:59 AM, Hirokazu Honda wrote:\n> > > On Wed, Sep 29, 2021 at 6:07 AM Laurent Pinchart wrote:\n> > >> On Tue, Sep 28, 2021 at 09:55:35PM +0530, Umang Jain wrote:\n> > >>> The descriptors_ map holds Camera3RequestDescriptor(s) which are\n> > >>> per-capture requests placed by the framework to libcamera HAL.\n> > >>> CameraDevice::requestComplete() looks for the descriptor for which the\n> > >>> camera request has been completed and removes it from the map.\n> > >>> Since the requests are placed in form of FIFO and the framework expects\n> > >>> the order of completion to be FIFO as well, this calls for a need of\n> > >>> a queue rather than a std::map.\n> > >>>\n> > >>> This patch still keeps the same lifetime of Camera3RequestDescriptor as\n> > >>> before i.e. in the requestComplete(). Previously, a descriptor was\n> > >>> extracted from the map and its lifetime was bound to requestComplete().\n> > >>> The lifetime is kept same by manually calling .pop_front() on the\n> > >>> queue. In the subsequent commit, this is likely to change with a\n> > >>> centralized location of dropping descriptors from the queue for request\n> > >>> completion.\n> > >>>\n> > >>> Signed-off-by: Umang Jain \n> > >>> ---\n> > >>> src/android/camera_device.cpp | 92 +++++++++++++++++++----------------\n> > >>> src/android/camera_device.h | 3 +-\n> > >>> 2 files changed, 52 insertions(+), 43 deletions(-)\n> > >>>\n> > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > >>> index 9287ea07..a3b8a549 100644\n> > >>> --- a/src/android/camera_device.cpp\n> > >>> +++ b/src/android/camera_device.cpp\n> > >>> @@ -457,7 +457,9 @@ void CameraDevice::stop()\n> > >>> worker_.stop();\n> > >>> camera_->stop();\n> > >>>\n> > >>> - descriptors_.clear();\n> > >>> + while (!descriptors_.empty())\n> > >>> + descriptors_.pop();\n> > >>\n> > >> You can simplify this with\n> > >>\n> > >> descriptors_ = {};\n> > >>\n> > >>> +\n> > >>> state_ = State::Stopped;\n> > >>> }\n> > >>>\n> > >>> @@ -955,7 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >>> * The descriptor and the associated memory reserved here are freed\n> > >>> * at request complete time.\n> > >>> */\n> > >>> - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n> > >>> + std::unique_ptr descriptor =\n> > >>> + std::make_unique(camera_.get(),\n> > >>> + camera3Request);\n> > >\n> > > nit: I would use auto in the type declaration in using\n> > > std::make_unique because the type is obvious from the right formula.\n> > >\n> > >>> /*\n> > >>> * \\todo The Android request model is incremental, settings passed in\n> > >>> @@ -966,12 +970,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >>> if (camera3Request->settings)\n> > >>> lastSettings_ = camera3Request->settings;\n> > >>> else\n> > >>> - descriptor.settings_ = lastSettings_;\n> > >>> + descriptor->settings_ = lastSettings_;\n> > >>> +\n> > >>> + LOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> > >>> + << \" with \" << descriptor->buffers_.size() << \" streams\";\n> > >>>\n> > >>> - LOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n> > >>> - << \" with \" << descriptor.buffers_.size() << \" streams\";\n> > >>> - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> > >>> - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> > >>> + for (const auto &[i, camera3Buffer] : utils::enumerate(descriptor->buffers_)) {\n> > >>> camera3_stream *camera3Stream = camera3Buffer.stream;\n> > >>> CameraStream *cameraStream = static_cast(camera3Stream->priv);\n> > >>>\n> > >>> @@ -1007,12 +1011,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >>> * associate it with the Camera3RequestDescriptor for\n> > >>> * lifetime management only.\n> > >>> */\n> > >>> - descriptor.frameBuffers_.push_back(\n> > >>> + descriptor->frameBuffers_.push_back(\n> > >>> createFrameBuffer(*camera3Buffer.buffer,\n> > >>> cameraStream->configuration().pixelFormat,\n> > >>> cameraStream->configuration().size));\n> > >>>\n> > >>> - buffer = descriptor.frameBuffers_.back().get();\n> > >>> + buffer = descriptor->frameBuffers_.back().get();\n> > >>> acquireFence = camera3Buffer.acquire_fence;\n> > >>> LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > >>> break;\n> > >>> @@ -1035,7 +1039,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >>> return -ENOMEM;\n> > >>> }\n> > >>>\n> > >>> - descriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> > >>> + descriptor->request_->addBuffer(cameraStream->stream(), buffer,\n> > >>> acquireFence);\n> > >>> }\n> > >>>\n> > >>> @@ -1043,7 +1047,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >>> * Translate controls from Android to libcamera and queue the request\n> > >>> * to the CameraWorker thread.\n> > >>> */\n> > >>> - int ret = processControls(&descriptor);\n> > >>> + int ret = processControls(descriptor.get());\n> > >>> if (ret)\n> > >>> return ret;\n> > >>>\n> > >>> @@ -1071,11 +1075,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >>> state_ = State::Running;\n> > >>> }\n> > >>>\n> > >>> - CaptureRequest *request = descriptor.request_.get();\n> > >>> + CaptureRequest *request = descriptor->request_.get();\n> > >>>\n> > >>> {\n> > >>> MutexLocker descriptorsLock(descriptorsMutex_);\n> > >>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > >>> + descriptors_.push(std::move(descriptor));\n> > >>> }\n> > >>>\n> > >>> worker_.queueRequest(request);\n> > >>> @@ -1085,26 +1089,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >>>\n> > >>> void CameraDevice::requestComplete(Request *request)\n> > >>> {\n> > >>> - decltype(descriptors_)::node_type node;\n> > >>> + Camera3RequestDescriptor *descriptor;\n> > >>> {\n> > >>> MutexLocker descriptorsLock(descriptorsMutex_);\n> > >>> - auto it = descriptors_.find(request->cookie());\n> > >>> - if (it == descriptors_.end()) {\n> > >>> - /*\n> > >>> - * \\todo Clarify if the Camera has to be closed on\n> > >>> - * ERROR_DEVICE and possibly demote the Fatal to simple\n> > >>> - * Error.\n> > >>> - */\n> > >>> - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > >>> - LOG(HAL, Fatal)\n> > >>> - << \"Unknown request: \" << request->cookie();\n> > >>> + ASSERT(!descriptors_.empty());\n> > >>> + descriptor = descriptors_.front().get();\n> > >\n> > > Why isn't descriptor popped here?\n> > > Is there any case that a descriptor should be kept?\n> > > The original code removes the descriptor from descriptors_.\n> > > This changes the logic.\n> >\n> > A bit yes, but it does respect the lifetime of the descriptor as before\n> > (as the std::map)\n> >\n> > Provided that this is a transient patch that works on descriptors_, I\n> > prefer to be a little flexible here with logic. The popping of\n> > descriptors_ has been moved to a centralized location in 3/3\n>\n> Let's also note that the upcoming async post-processing series will\n> require keeping the descriptor in the queue until the post-processing is\n> complete.\n>\n> > > I also think removing here is safe because although a pointer of node\n> > > in std::queue() is not invalidated by push operation, but indeed can\n> > > be invalidated by pop operation.\n> > > So removing here (while acquiring a lock) avoids the problem of using\n> > > an invalid pointer in the case descriptors_ are cleared in parallel.\n> >\n> > Yes there might be cases in the future where this can happen, I can\n> > think of some stop() or flush() clearing the descriptors_, but we are\n> > going to address them step by step.\n> >\n\nDoesn't the issue of clearing in parallel happen at this moment?\n\n> > The clearing of descriptors_ is something that shouldn't be happening by\n> > design, so we need to handle it as case-by-case basis (which is one the\n> > goals we intend to pursue as part of this HAL rework), in near-future\n> > patch series.\n> >\n> > >>> + }\n> > >>>\n> > >>> - return;\n> > >>> - }\n> > >>> + if (descriptor->request_->cookie() != request->cookie()) {\n> > >>> + /*\n> > >>> + * \\todo Clarify if the Camera has to be closed on\n> > >>> + * ERROR_DEVICE and possibly demote the Fatal to simple\n> > >>> + * Error.\n> > >>> + */\n> > >>> + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > >>> + LOG(HAL, Fatal)\n> > >>> + << \"Out-of-order completion for request: \"\n> > >> s/://\n> > >>\n> > >> Reviewed-by: Laurent Pinchart \n> > >>\n> > >>> + << utils::hex(request->cookie());\n> > >>>\n> > >>> - node = descriptors_.extract(it);\n> > >>> + return;\n> > >>> }\n> > >>> - Camera3RequestDescriptor &descriptor = node.mapped();\n> > >>>\n> > >>> /*\n> > >>> * Prepare the capture result for the Android camera stack.\n> > >>> @@ -1113,9 +1117,9 @@ void CameraDevice::requestComplete(Request *request)\n> > >>> * post-processing/compression fails.\n> > >>> */\n> > >>> camera3_capture_result_t captureResult = {};\n> > >>> - captureResult.frame_number = descriptor.frameNumber_;\n> > >>> - captureResult.num_output_buffers = descriptor.buffers_.size();\n> > >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > >>> + captureResult.frame_number = descriptor->frameNumber_;\n> > >>> + captureResult.num_output_buffers = descriptor->buffers_.size();\n> > >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > >>> CameraStream *cameraStream =\n> > >>> static_cast(buffer.stream->priv);\n> > >>>\n> > >>> @@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request *request)\n> > >>> buffer.release_fence = -1;\n> > >>> buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > >>> }\n> > >>> - captureResult.output_buffers = descriptor.buffers_.data();\n> > >>> + captureResult.output_buffers = descriptor->buffers_.data();\n> > >>> captureResult.partial_result = 1;\n> > >>>\n> > >>> /*\n> > >>> @@ -1147,11 +1151,11 @@ void CameraDevice::requestComplete(Request *request)\n> > >>> << \" not successfully completed: \"\n> > >>> << request->status();\n> > >>>\n> > >>> - notifyError(descriptor.frameNumber_, nullptr,\n> > >>> + notifyError(descriptor->frameNumber_, nullptr,\n> > >>> CAMERA3_MSG_ERROR_REQUEST);\n> > >>>\n> > >>> captureResult.partial_result = 0;\n> > >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > >>> /*\n> > >>> * Signal to the framework it has to handle fences that\n> > >>> * have not been waited on by setting the release fence\n> > >>> @@ -1164,6 +1168,7 @@ void CameraDevice::requestComplete(Request *request)\n> > >>>\n> > >>> callbacks_->process_capture_result(callbacks_, &captureResult);\n> > >>>\n> > >\n> > > A lock is not acquired here?\n> > > MutexLocker descriptorsLock(descriptorsMutex_);\n> >\n> > ops, yeah, I'll fix it!\n> >\n> > >>> + descriptors_.pop();\n> > >>> return;\n> > >>> }\n> > >>>\n> > >>> @@ -1175,10 +1180,10 @@ void CameraDevice::requestComplete(Request *request)\n> > >>> */\n> > >>> uint64_t sensorTimestamp = static_cast(request->metadata()\n> > >>> .get(controls::SensorTimestamp));\n> > >>> - notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> > >>> + notifyShutter(descriptor->frameNumber_, sensorTimestamp);\n> > >>>\n> > >>> LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> > >>> - << descriptor.buffers_.size() << \" streams\";\n> > >>> + << descriptor->buffers_.size() << \" streams\";\n> > >>>\n> > >>> /*\n> > >>> * Generate the metadata associated with the captured buffers.\n> > >>> @@ -1186,16 +1191,16 @@ void CameraDevice::requestComplete(Request *request)\n> > >>> * Notify if the metadata generation has failed, but continue processing\n> > >>> * buffers and return an empty metadata pack.\n> > >>> */\n> > >>> - std::unique_ptr resultMetadata = getResultMetadata(descriptor);\n> > >>> + std::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n> > >>> if (!resultMetadata) {\n> > >>> - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > >>> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > >>>\n> > >>> /* The camera framework expects an empty metadata pack on error. */\n> > >>> resultMetadata = std::make_unique(0, 0);\n> > >>> }\n> > >>>\n> > >>> /* Handle post-processing. */\n> > >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > >>> CameraStream *cameraStream =\n> > >>> static_cast(buffer.stream->priv);\n> > >>>\n> > >>> @@ -1206,13 +1211,13 @@ void CameraDevice::requestComplete(Request *request)\n> > >>> if (!src) {\n> > >>> LOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> > >>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > >>> - notifyError(descriptor.frameNumber_, buffer.stream,\n> > >>> + notifyError(descriptor->frameNumber_, buffer.stream,\n> > >>> CAMERA3_MSG_ERROR_BUFFER);\n> > >>> continue;\n> > >>> }\n> > >>>\n> > >>> int ret = cameraStream->process(*src, buffer,\n> > >>> - descriptor.settings_,\n> > >>> + descriptor->settings_,\n> > >>> resultMetadata.get());\n> > >>> /*\n> > >>> * Return the FrameBuffer to the CameraStream now that we're\n> > >>> @@ -1223,13 +1228,16 @@ void CameraDevice::requestComplete(Request *request)\n> > >>>\n> > >>> if (ret) {\n> > >>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > >>> - notifyError(descriptor.frameNumber_, buffer.stream,\n> > >>> + notifyError(descriptor->frameNumber_, buffer.stream,\n> > >>> CAMERA3_MSG_ERROR_BUFFER);\n> > >>> }\n> > >>> }\n> > >>>\n> > >>> captureResult.result = resultMetadata->get();\n> > >>> callbacks_->process_capture_result(callbacks_, &captureResult);\n> > >>> +\n> > >>> + MutexLocker descriptorsLock(descriptorsMutex_);\n> > >>> + descriptors_.pop();\n> > >>> }\n> > >>>\n> > >>> std::string CameraDevice::logPrefix() const\n> > >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > >>> index 43eb5895..9ec510d5 100644\n> > >>> --- a/src/android/camera_device.h\n> > >>> +++ b/src/android/camera_device.h\n> > >>> @@ -10,6 +10,7 @@\n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>>\n> > >>> #include \n> > >>> @@ -125,7 +126,7 @@ private:\n> > >>> std::vector streams_;\n> > >>>\n> > >>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> > >>> - std::map descriptors_;\n> > >>> + std::queue> descriptors_;\n> > >>>\n> > >>> std::string maker_;\n> > >>> std::string model_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","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 4D137BDC71\n\tfor ;\n\tWed, 29 Sep 2021 07:18:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1615E69193;\n\tWed, 29 Sep 2021 09:18:51 +0200 (CEST)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5978F6918A\n\tfor ;\n\tWed, 29 Sep 2021 09:18:49 +0200 (CEST)","by mail-ed1-x533.google.com with SMTP id r18so4758298edv.12\n\tfor ;\n\tWed, 29 Sep 2021 00:18:49 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Ii2Ex7yj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=tLR0zSwuWTc7oD69a4yt5yaH5UcIp5C4GX4SC2oXBQk=;\n\tb=Ii2Ex7yj6UFTDkHauYN6ldCIZK8MBsLJcLEk9cBlsS6WV3okFQBUWitpW1V6v4iQ/W\n\ta4Saz+J9zKsw3pQZaZD0ijRQPGFMmn9QJiB/ZMgZ8KliTRVwGPYlNTltSFGV0FinQeQm\n\twIvTBfZSyoSbdJlabxPJ7qWX9LB9eFU+6NT30=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=tLR0zSwuWTc7oD69a4yt5yaH5UcIp5C4GX4SC2oXBQk=;\n\tb=DG8WQFvSezK/Gq88xmYPThc7kXvtBsaDMwzZPWY0wbnWuoifDbMi4MDoEKW+3pz7fE\n\tAdT5fhd2XDj/EiWoD2GtySTQQu2t1ue41WRnqpcqVWO1RAPuvtoNqHirbeQQDh1sDRJD\n\teqyxPVAPVMD27zS6iM7qFiArK0p/QqujILfXmXEbUPbY2ZTdS+CiEb+PulaFoh7H60vf\n\tczgT8pn2Eb9I1SRTKAgNPPON1T/w4rWZisORwfj3dCtDCFIHoTntwAKPhH6wMncr9UIg\n\tXNUFPrkSo/AVu+WHAbYQa8htkpmA9ZiT7rMQ78rSsIx+3d49+c42ifycOFq7iis4I/Rg\n\tul8A==","X-Gm-Message-State":"AOAM533/uhYkAkq8eQPUVs36aMDesHzu3ymNQ3NG0P9Q2bDCbiotAz9+\n\ttdXd4XqrwFwhSPjIok4tTzMc8zF4n3Y1SV5JrJTi4g==","X-Google-Smtp-Source":"ABdhPJyuK6m7nfoqZ8IERKdY4nwoJUWit5bqwJDuoyyLn6Cp49FDxPkq/Q+Hq6pxcn56HVU7B7J8uU/xCZ4YXkUPsBo=","X-Received":"by 2002:a17:906:3adb:: with SMTP id\n\tz27mr11901667ejd.291.1632899928742; \n\tWed, 29 Sep 2021 00:18:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-3-umang.jain@ideasonboard.com>\n\t\n\t\n\t<305fe168-0479-e85f-d6f5-2e08d4d8cff4@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 29 Sep 2021 16:18:38 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]