[{"id":19964,"web_url":"https://patchwork.libcamera.org/comment/19964/","msgid":"","date":"2021-09-29T07:25:01","subject":"Re: [libcamera-devel] [PATCH v3 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 Wed, Sep 29, 2021 at 12:47:07PM +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> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 91 +++++++++++++++++++----------------\n> src/android/camera_device.h | 3 +-\n> 2 files changed, 51 insertions(+), 43 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 9287ea07..499baec8 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -457,7 +457,8 @@ void CameraDevice::stop()\n> \tworker_.stop();\n> \tcamera_->stop();\n> \n> -\tdescriptors_.clear();\n> +\tdescriptors_ = {};\n> +\n> \tstate_ = State::Stopped;\n> }\n> \n> @@ -955,7 +956,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> +\tauto 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 +969,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 +1010,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 +1038,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 +1046,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 +1074,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 +1088,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> +\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 +1116,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 +1138,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 +1150,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 +1167,7 @@ void CameraDevice::requestComplete(Request *request)\n> \n> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> \n\nMissing lock ?\n\n> +\t\tdescriptors_.pop();\n> \t\treturn;\n> \t}\n> \n> @@ -1175,10 +1179,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 +1190,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 +1210,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 +1227,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 ED8F9C3243\n\tfor ;\n\tWed, 29 Sep 2021 07:25:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F21A69191;\n\tWed, 29 Sep 2021 09:25:05 +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 AE88C6918A\n\tfor ;\n\tWed, 29 Sep 2021 09:25:04 +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 212C83F0;\n\tWed, 29 Sep 2021 09:25:04 +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=\"q39NyncO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632900304;\n\tbh=q1/CULbs5y1zRqEBghqLTpVa1TKJy4Cftz9kGrZ1vjU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=q39NyncOSmzNGAeGOaEYvWNij07zhQZjOBwlJUm2KfS+f5ocQ0d+oP/Ce4McW0T9N\n\tNWardC7eeknAvt9AKba4ViSGmY7RLpqh/8/W2a15PG7pXpCc0ITDb0Nevpvg3Jt0nC\n\tG0k3meSN/OMQr+pqhsNFfoE7KwQLmHHltGFwO+zU=","Date":"Wed, 29 Sep 2021 10:25:01 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210929071708.299946-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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":19965,"web_url":"https://patchwork.libcamera.org/comment/19965/","msgid":"","date":"2021-09-29T07:28:35","subject":"Re: [libcamera-devel] [PATCH v3 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 12:55 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 29, 2021 at 12:47:07PM +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>> Reviewed-by: Laurent Pinchart \n>> ---\n>> src/android/camera_device.cpp | 91 +++++++++++++++++++----------------\n>> src/android/camera_device.h | 3 +-\n>> 2 files changed, 51 insertions(+), 43 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 9287ea07..499baec8 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -457,7 +457,8 @@ void CameraDevice::stop()\n>> \tworker_.stop();\n>> \tcamera_->stop();\n>> \n>> -\tdescriptors_.clear();\n>> +\tdescriptors_ = {};\n>> +\n>> \tstate_ = State::Stopped;\n>> }\n>> \n>> @@ -955,7 +956,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>> +\tauto 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 +969,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 +1010,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 +1038,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 +1046,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 +1074,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 +1088,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>> +\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 +1116,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 +1138,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 +1150,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 +1167,7 @@ void CameraDevice::requestComplete(Request *request)\n>> \n>> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> \n> Missing lock ?\n\n\nhuh, I had a patch which I squashed, rebase fiasco :-S\nI'll fix it.\n\n>\n>> +\t\tdescriptors_.pop();\n>> \t\treturn;\n>> \t}\n>> \n>> @@ -1175,10 +1179,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 +1190,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 +1210,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 +1227,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 A10CDBDC71\n\tfor ;\n\tWed, 29 Sep 2021 07:28:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0197669191;\n\tWed, 29 Sep 2021 09:28:42 +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 709106918A\n\tfor ;\n\tWed, 29 Sep 2021 09:28:40 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7791DDEE;\n\tWed, 29 Sep 2021 09:28:39 +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=\"De3C5PyG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632900520;\n\tbh=bfOiM0nKkj9g4CTxCS7FxJVIpDLl135FpAswu8tPH2o=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=De3C5PyG6U9ko2qGv+dNM5GINdNinC6q+ELSvQbKBfhxWRh2MjmKWolzrA6rL48nN\n\tN2+Xowr7LW7Fs82Viv1LCz6aWpborwRWL8OzsE8RfslYqLBC0671OBCFb69wW71QY2\n\ty9m4M6J19Bdf5q3VmjNw/tXOTDtwWLxgjFF0r/oo=","To":"Laurent Pinchart ","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-3-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Wed, 29 Sep 2021 12:58:35 +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 v3 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":19970,"web_url":"https://patchwork.libcamera.org/comment/19970/","msgid":"<20210929080017.kavsj4okgr7xdd7k@uno.localdomain>","date":"2021-09-29T08:00:17","subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Wed, Sep 29, 2021 at 12:47:07PM +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> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 91 +++++++++++++++++++----------------\n> src/android/camera_device.h | 3 +-\n> 2 files changed, 51 insertions(+), 43 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 9287ea07..499baec8 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -457,7 +457,8 @@ void CameraDevice::stop()\n> \tworker_.stop();\n> \tcamera_->stop();\n>\n> -\tdescriptors_.clear();\n> +\tdescriptors_ = {};\n> +\n> \tstate_ = State::Stopped;\n> }\n>\n> @@ -955,7 +956,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> +\tauto descriptor =\n> +\t\tstd::make_unique(camera_.get(),\n> +\t\t\t\t\t\t\t camera3Request);\n\nFits on the previous line ?\n>\n> \t/*\n> \t * \\todo The Android request model is incremental, settings passed in\n> @@ -966,12 +969,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\nThanks!\n\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\nNicer!\n\n> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n>\n> @@ -1007,12 +1010,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 +1038,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 +1046,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 +1074,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 +1088,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\nI was about to comment that this is dangerously racy, as stop() clears\nthe descriptors_ queue. Laurent confirmed me offline that not only\nlibcamera::Camera::stop() completes all the in-flight requests, but\nthat the slots connected to the Camera::requestCompleted signal are\ninvoked in blocking mode. Hence, it is safe to keep the descriptor in\nthe queue for the duration of the slot\n\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\nFits on the previous line as well ?\n\n> +\t\t\t<< utils::hex(request->cookie());\n>\n> -\t\tnode = descriptors_.extract(it);\n> +\t\treturn;\n\nIn production builds Fatal won't tear down the library. I think after\nthe framework receives an CAMERA3_MSG_ERROR_DEVICE it will probably\njust close the camera, but in any case, shouldn't you pop() the queue\nhere ?\n\n> \t}\n> -\tCamera3RequestDescriptor &descriptor = node.mapped();\n>\n> \t/*\n> \t * Prepare the capture result for the Android camera stack.\n> @@ -1113,9 +1116,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 +1138,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 +1150,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 +1167,7 @@ void CameraDevice::requestComplete(Request *request)\n>\n> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>\n> +\t\tdescriptors_.pop();\n\nThis should be locked\n\n> \t\treturn;\n> \t}\n>\n> @@ -1175,10 +1179,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 +1190,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 +1210,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 +1227,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_;\n> --\n> 2.31.0\n>","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 A0068BDC71\n\tfor ;\n\tWed, 29 Sep 2021 07:59:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F033869191;\n\tWed, 29 Sep 2021 09:59:31 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED2D26918A\n\tfor ;\n\tWed, 29 Sep 2021 09:59:30 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 761FB1C0003;\n\tWed, 29 Sep 2021 07:59:30 +0000 (UTC)"],"Date":"Wed, 29 Sep 2021 10:00:17 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210929080017.kavsj4okgr7xdd7k@uno.localdomain>","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210929071708.299946-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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":19971,"web_url":"https://patchwork.libcamera.org/comment/19971/","msgid":"<0be486f4-0325-02a3-1a02-d50c47797847@ideasonboard.com>","date":"2021-09-29T08:05:04","subject":"Re: [libcamera-devel] [PATCH v3 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 Jacopo\n\nOn 9/29/21 1:30 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Wed, Sep 29, 2021 at 12:47:07PM +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>> Reviewed-by: Laurent Pinchart \n>> ---\n>> src/android/camera_device.cpp | 91 +++++++++++++++++++----------------\n>> src/android/camera_device.h | 3 +-\n>> 2 files changed, 51 insertions(+), 43 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 9287ea07..499baec8 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -457,7 +457,8 @@ void CameraDevice::stop()\n>> \tworker_.stop();\n>> \tcamera_->stop();\n>>\n>> -\tdescriptors_.clear();\n>> +\tdescriptors_ = {};\n>> +\n>> \tstate_ = State::Stopped;\n>> }\n>>\n>> @@ -955,7 +956,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>> +\tauto descriptor =\n>> +\t\tstd::make_unique(camera_.get(),\n>> +\t\t\t\t\t\t\t camera3Request);\n> Fits on the previous line ?\n>> \t/*\n>> \t * \\todo The Android request model is incremental, settings passed in\n>> @@ -966,12 +969,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> Thanks!\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> Nicer!\n>\n>> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n>> \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n>>\n>> @@ -1007,12 +1010,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 +1038,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 +1046,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 +1074,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 +1088,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> I was about to comment that this is dangerously racy, as stop() clears\n> the descriptors_ queue. Laurent confirmed me offline that not only\n> libcamera::Camera::stop() completes all the in-flight requests, but\n> that the slots connected to the Camera::requestCompleted signal are\n> invoked in blocking mode. Hence, it is safe to keep the descriptor in\n> the queue for the duration of the slot\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> Fits on the previous line as well ?\n\n\nhmm yeah\n\n>\n>> +\t\t\t<< utils::hex(request->cookie());\n>>\n>> -\t\tnode = descriptors_.extract(it);\n>> +\t\treturn;\n> In production builds Fatal won't tear down the library. I think after\n\n\nOh, I thought any Fatal would/should tear down everything. In this \ncontext, yes it makes sense to pop the descriptor here\n\n> the framework receives an CAMERA3_MSG_ERROR_DEVICE it will probably\n> just close the camera, but in any case, shouldn't you pop() the queue\n> here ?\n>\n>> \t}\n>> -\tCamera3RequestDescriptor &descriptor = node.mapped();\n>>\n>> \t/*\n>> \t * Prepare the capture result for the Android camera stack.\n>> @@ -1113,9 +1116,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 +1138,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 +1150,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 +1167,7 @@ void CameraDevice::requestComplete(Request *request)\n>>\n>> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>\n>> +\t\tdescriptors_.pop();\n> This should be locked\n>\n>> \t\treturn;\n>> \t}\n>>\n>> @@ -1175,10 +1179,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 +1190,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 +1210,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 +1227,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_;\n>> --\n>> 2.31.0\n>>","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 DC2E6C3243\n\tfor ;\n\tWed, 29 Sep 2021 08:05:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 678F169191;\n\tWed, 29 Sep 2021 10:05:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 954D86918A\n\tfor ;\n\tWed, 29 Sep 2021 10:05:08 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B045A3F0;\n\tWed, 29 Sep 2021 10:05:07 +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=\"YW40NmXW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632902708;\n\tbh=x7UboMBNfJYo8SgNEiKwsTvF//O1FUJeM8AuuI1LdUo=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=YW40NmXW+V2onnBkaKq+PT5gq4smClJQJ7ATgdaO+h4FYV3L7T9nx1dxkIbBOxB10\n\tDxcqy7o7gazv3jY7abOYfCckplTBimnL+9npfNeIVlJPdVdEdg3h76bXcqhI6T1UgG\n\tBZGEivrZfdxZYmOM6NzmOm+nIVMOQr5oxv4WJygM=","To":"Jacopo Mondi ","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-3-umang.jain@ideasonboard.com>\n\t<20210929080017.kavsj4okgr7xdd7k@uno.localdomain>","From":"Umang Jain ","Message-ID":"<0be486f4-0325-02a3-1a02-d50c47797847@ideasonboard.com>","Date":"Wed, 29 Sep 2021 13:35:04 +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":"<20210929080017.kavsj4okgr7xdd7k@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 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":19975,"web_url":"https://patchwork.libcamera.org/comment/19975/","msgid":"","date":"2021-09-29T08:37:37","subject":"Re: [libcamera-devel] [PATCH v3 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":"On Wed, Sep 29, 2021 at 10:00:17AM +0200, Jacopo Mondi wrote:\n> Hi Umang,\n> \n> On Wed, Sep 29, 2021 at 12:47:07PM +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> > Reviewed-by: Laurent Pinchart \n> > ---\n> > src/android/camera_device.cpp | 91 +++++++++++++++++++----------------\n> > src/android/camera_device.h | 3 +-\n> > 2 files changed, 51 insertions(+), 43 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 9287ea07..499baec8 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -457,7 +457,8 @@ void CameraDevice::stop()\n> > \tworker_.stop();\n> > \tcamera_->stop();\n> >\n> > -\tdescriptors_.clear();\n> > +\tdescriptors_ = {};\n> > +\n> > \tstate_ = State::Stopped;\n> > }\n> >\n> > @@ -955,7 +956,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> > +\tauto descriptor =\n> > +\t\tstd::make_unique(camera_.get(),\n> > +\t\t\t\t\t\t\t camera3Request);\n> \n> Fits on the previous line ?\n> >\n> > \t/*\n> > \t * \\todo The Android request model is incremental, settings passed in\n> > @@ -966,12 +969,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> Thanks!\n> \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> \n> Nicer!\n> \n> > \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> > \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> >\n> > @@ -1007,12 +1010,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 +1038,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 +1046,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 +1074,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 +1088,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> I was about to comment that this is dangerously racy, as stop() clears\n> the descriptors_ queue. Laurent confirmed me offline that not only\n> libcamera::Camera::stop() completes all the in-flight requests, but\n> that the slots connected to the Camera::requestCompleted signal are\n> invoked in blocking mode. Hence, it is safe to keep the descriptor in\n> the queue for the duration of the slot\n\nJust to be precise, the request completeion signals are emitted in\nCamera::stop(), and the slots are called synchronously because the\nreceiver is not bound to a thread. If we ever move CameraDevice to a\nthread, it won't necessarily be true anymore.\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> \n> Fits on the previous line as well ?\n\nNitpicking a bit, when we split a log message on multiple lines, we\nusually do it as Umang has done here.\n\n> > +\t\t\t<< utils::hex(request->cookie());\n> >\n> > -\t\tnode = descriptors_.extract(it);\n> > +\t\treturn;\n> \n> In production builds Fatal won't tear down the library. I think after\n> the framework receives an CAMERA3_MSG_ERROR_DEVICE it will probably\n> just close the camera, but in any case, shouldn't you pop() the queue\n> here ?\n\nIf we hit this case I think it's pretty much game over in any case. We\ncan pop the descriptor, it won't hurt, but I don't think it will make a\nbig difference.\n\n> > \t}\n> > -\tCamera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > \t/*\n> > \t * Prepare the capture result for the Android camera stack.\n> > @@ -1113,9 +1116,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 +1138,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 +1150,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 +1167,7 @@ void CameraDevice::requestComplete(Request *request)\n> >\n> > \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >\n> > +\t\tdescriptors_.pop();\n> \n> This should be locked\n> \n> > \t\treturn;\n> > \t}\n> >\n> > @@ -1175,10 +1179,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 +1190,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 +1210,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 +1227,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 72D94C3243\n\tfor ;\n\tWed, 29 Sep 2021 08:37:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5675691AC;\n\tWed, 29 Sep 2021 10:37:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD2116919D\n\tfor ;\n\tWed, 29 Sep 2021 10:37:39 +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 334983F0;\n\tWed, 29 Sep 2021 10:37:39 +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=\"psMsaLXr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632904659;\n\tbh=1BkwIhQwlZ7dyF5h6UFKa63k22jNQODPI1Nin0+8avM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=psMsaLXrI7eidqCbb+0co+QOo03xeew2KLUMpOQcAxAKwUI9lRsDrircnoarxpPvg\n\t7mPG1BIZbbitDiM/cApP6IIMBpvvHMzdV3IVa+ICJSpCN1hnjS62Kg1K/ZFS0uSV0i\n\tJL6awGA7HnmGRyOJceRufBeKJopWMSQVqR2uaBws=","Date":"Wed, 29 Sep 2021 11:37:37 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-3-umang.jain@ideasonboard.com>\n\t<20210929080017.kavsj4okgr7xdd7k@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210929080017.kavsj4okgr7xdd7k@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]