[{"id":19719,"web_url":"https://patchwork.libcamera.org/comment/19719/","msgid":"","date":"2021-09-20T19:06:32","subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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 Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:\n> The descriptors_ map hold Camera3RequestDescriptor(s) which are\n> per-capture request 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> However, this approach has its limitations going forwards.\n> \n> In subsequent commits, the post-processing operation which happens\n> in requestComplete() synchronously, is going to be run asynchronously.\n> Therefore, instead of a map for descriptors, a queue makes more sense\n> going forwards and the framework expects capture results to be received\n> in the same order as they were queued. When the async processing is\n> completed, the descriptor queue is inspected to send back the capture\n> results and then de-queued. This helps to maintain the order of sending\n> back the capture results whilst preventing unnecessary complexity of\n> using a map.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------\n> src/android/camera_device.h | 5 +-\n> 2 files changed, 48 insertions(+), 46 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f461e14c..0562c225 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -926,7 +926,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> @@ -937,12 +939,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> -\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> -\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\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> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> \n> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\tbuffer = createFrameBuffer(*camera3Buffer.buffer,\n> \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n> \t\t\t\t\t\t cameraStream->configuration().size);\n> -\t\t\tdescriptor.frameBuffers_.emplace_back(buffer);\n> +\t\t\tdescriptor->frameBuffers_.emplace_back(buffer);\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> \t\t\tbreak;\n> \n> @@ -999,7 +1001,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\tcamera3Buffer.acquire_fence);\n> \t}\n> \n> @@ -1007,7 +1009,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> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tstate_ = State::Running;\n> \t}\n> \n> -\tworker_.queueRequest(descriptor.request_.get());\n> +\tworker_.queueRequest(descriptor->request_.get());\n> \n> \t{\n> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> +\t\tdescriptors_.push_back(std::move(descriptor));\n> \t}\n\nWe have a race condition here, worker_.queueRequest() should go after\nadding the request to the queue. Could you fix it in a patch on top ?\n\n> \n> \treturn 0;\n> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> void CameraDevice::requestComplete(Request *request)\n> {\n> -\tdecltype(descriptors_)::node_type node;\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> +\tif (descriptors_.empty())\n> +\t\treturn;\n> \n> -\t\t\treturn;\n> -\t\t}\n> +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\n\nThis needs to be protected by descriptorsMutex_.\n\n\tCamera3RequestDescriptor *descriptor;\n\n\t{\n\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n\t\tdescriptor = descriptors_.front().get();\n\t}\n\n> +\tif (descriptor->request_->cookie() != request->cookie()) {\n\nThis is correct as long as we handle post-processing synchronously.\nLet's see how it evolves in subsequent patches.\n\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<< \"Unknown request: \" << request->cookie();\n\nI'd change the message to\n\n\t\t\t<< \"Out-of-order completion for request \"\n\t\t\t<< request->cookie();\n\nBy the way, with the cookie containing a pointer, I think it would be\nmore readable in hex. Maybe a patch on top to use utils::hex() ?\n\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> @@ -1075,14 +1073,14 @@ 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\tbuffer.acquire_fence = -1;\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> @@ -1094,14 +1092,15 @@ 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\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> \n> +\t\tdescriptors_.pop_front();\n\nI'm slightly concerned that in some paths we could complete the request\nbut forget to remove it from the queue. Maybe wrapping\ncallbacks_->process_capture_result() and descriptors_.pop_front() in a\nfunction would be good. Let's see how it looks like with the whole\nseries applied.\n\n> \t\treturn;\n> \t}\n> \n> @@ -1113,10 +1112,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> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)\n> \t */\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 any JPEG compression. */\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> @@ -1144,13 +1143,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.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> @@ -1161,13 +1160,15 @@ 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> +\tdescriptors_.pop_front();\n> }\n> \n> std::string CameraDevice::logPrefix() const\n> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> * Produce a set of fixed result metadata.\n> */\n> std::unique_ptr\n> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n> {\n> -\tconst ControlList &metadata = descriptor.request_->metadata();\n> -\tconst CameraMetadata &settings = descriptor.settings_;\n> +\tconst ControlList &metadata = descriptor->request_->metadata();\n> +\tconst CameraMetadata &settings = descriptor->settings_;\n> \tcamera_metadata_ro_entry_t entry;\n> \tbool found;\n> \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index a5576927..9c895b25 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -7,6 +7,7 @@\n> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> #define __ANDROID_CAMERA_DEVICE_H__\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -103,7 +104,7 @@ private:\n> \t\t\t camera3_error_msg_code code);\n> \tint processControls(Camera3RequestDescriptor *descriptor);\n> \tstd::unique_ptr getResultMetadata(\n> -\t\tconst Camera3RequestDescriptor &descriptor) const;\n> +\t\tconst Camera3RequestDescriptor *descriptor) const;\n> \n> \tunsigned int id_;\n> \tcamera3_device_t camera3Device_;\n> @@ -123,7 +124,7 @@ private:\n> \tstd::vector streams_;\n> \n> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> -\tstd::map descriptors_;\n> +\tstd::deque> descriptors_;\n\nCould we use a std::queue instead ? It will be implemented on top of a\nstd::deque, so there will be no change in performances, but it gives us\nthe semantics we need (essentially, a FIFO).\n\nReviewed-by: Laurent Pinchart \n\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 5E5CEBF01C\n\tfor ;\n\tMon, 20 Sep 2021 19:07:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B3D1F6918A;\n\tMon, 20 Sep 2021 21:07: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 636E169186\n\tfor ;\n\tMon, 20 Sep 2021 21:07:03 +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 C2461E57;\n\tMon, 20 Sep 2021 21:07:02 +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=\"bXJEwXt+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632164823;\n\tbh=C5n7DH5LilsDYgYWhCQ+FW2qPXV5SP6HpCBp49ACQ2k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bXJEwXt+rAagZJQ3zkc0Myd8QTYodcVHX8WUN928dgI+wMijnmIGNQ77c38drTZ2E\n\taP22A01vSCTzci4pOP8unjpUkOx1GV8Mk+Ip+RKuQNPFSgn+6md7rMLbM10ie89Kd5\n\tKgjTkbBuC3emwXiNBvQ9wSsQkIi+uoQKrcOOzJ4Q=","Date":"Mon, 20 Sep 2021 22:06:32 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210920173752.1346190-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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":19722,"web_url":"https://patchwork.libcamera.org/comment/19722/","msgid":"<9ee9737e-277c-e313-2698-5e05ea1bda8f@ideasonboard.com>","date":"2021-09-21T04:26:22","subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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 Laurent,\n\nThanks for the review.\n\nOn 9/21/21 12:36 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:\n>> The descriptors_ map hold Camera3RequestDescriptor(s) which are\n>> per-capture request 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>> However, this approach has its limitations going forwards.\n>>\n>> In subsequent commits, the post-processing operation which happens\n>> in requestComplete() synchronously, is going to be run asynchronously.\n>> Therefore, instead of a map for descriptors, a queue makes more sense\n>> going forwards and the framework expects capture results to be received\n>> in the same order as they were queued. When the async processing is\n>> completed, the descriptor queue is inspected to send back the capture\n>> results and then de-queued. This helps to maintain the order of sending\n>> back the capture results whilst preventing unnecessary complexity of\n>> using a map.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------\n>> src/android/camera_device.h | 5 +-\n>> 2 files changed, 48 insertions(+), 46 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index f461e14c..0562c225 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -926,7 +926,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>> @@ -937,12 +939,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>> -\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n>> -\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\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>> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n>> \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n>> \n>> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\t\tbuffer = createFrameBuffer(*camera3Buffer.buffer,\n>> \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n>> \t\t\t\t\t\t cameraStream->configuration().size);\n>> -\t\t\tdescriptor.frameBuffers_.emplace_back(buffer);\n>> +\t\t\tdescriptor->frameBuffers_.emplace_back(buffer);\n>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n>> \t\t\tbreak;\n>> \n>> @@ -999,7 +1001,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\tcamera3Buffer.acquire_fence);\n>> \t}\n>> \n>> @@ -1007,7 +1009,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>> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\tstate_ = State::Running;\n>> \t}\n>> \n>> -\tworker_.queueRequest(descriptor.request_.get());\n>> +\tworker_.queueRequest(descriptor->request_.get());\n>> \n>> \t{\n>> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>> -\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>> +\t\tdescriptors_.push_back(std::move(descriptor));\n>> \t}\n> We have a race condition here, worker_.queueRequest() should go after\n> adding the request to the queue. Could you fix it in a patch on top ?\n\n\nDo you mean the race condition is existing already, with the \ndescriptors_ map (that has been removed from this patch)?\n\nYes, I can introduce a patch before this one, that fixes the race first \nin the map itself. Is my understanding correct?\n\n>\n>> \n>> \treturn 0;\n>> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \n>> void CameraDevice::requestComplete(Request *request)\n>> {\n>> -\tdecltype(descriptors_)::node_type node;\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>> +\tif (descriptors_.empty())\n>> +\t\treturn;\n>> \n>> -\t\t\treturn;\n>> -\t\t}\n>> +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\n> This needs to be protected by descriptorsMutex_.\n>\n> \tCamera3RequestDescriptor *descriptor;\n>\n> \t{\n> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> \t\tdescriptor = descriptors_.front().get();\n> \t}\n>\n>> +\tif (descriptor->request_->cookie() != request->cookie()) {\n> This is correct as long as we handle post-processing synchronously.\n> Let's see how it evolves in subsequent patches.\n\n\nWhy not valid for async post-processing?\n\nSo this is requestComplete() function, invoked whenever a \nlibcamera::Request is completed by libcamera::Camera. The completion is \nguaranteed to be done in order, right ? Later in this function, the \npost-processing shall happen (sync or async).\n\n>\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<< \"Unknown request: \" << request->cookie();\n> I'd change the message to\n>\n> \t\t\t<< \"Out-of-order completion for request \"\n> \t\t\t<< request->cookie();\n>\n> By the way, with the cookie containing a pointer, I think it would be\n> more readable in hex. Maybe a patch on top to use utils::hex() ?\n\n\nMakes sense, I'll double check\n\n>\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>> @@ -1075,14 +1073,14 @@ 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\tbuffer.acquire_fence = -1;\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>> @@ -1094,14 +1092,15 @@ 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\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> \n>> +\t\tdescriptors_.pop_front();\n> I'm slightly concerned that in some paths we could complete the request\n> but forget to remove it from the queue. Maybe wrapping\n> callbacks_->process_capture_result() and descriptors_.pop_front() in a\n> function would be good. Let's see how it looks like with the whole\n> series applied.\n\n\nThis is a good point and has been address already via \nsendCaptureResults() in subsequent patches.\n\nHowever, this particular error path you have pointed out here, is the \n/only/ part where queue.front() is dropped, other than ofcoourse in \nsendCaptureResults(). hmm, I'll take a look if I can figure it out too \nand have a singular place of processsing the queue.\n\n>\n>> \t\treturn;\n>> \t}\n>> \n>> @@ -1113,10 +1112,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>> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)\n>> \t */\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 any JPEG compression. */\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>> @@ -1144,13 +1143,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.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>> @@ -1161,13 +1160,15 @@ 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>> +\tdescriptors_.pop_front();\n>> }\n>> \n>> std::string CameraDevice::logPrefix() const\n>> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n>> * Produce a set of fixed result metadata.\n>> */\n>> std::unique_ptr\n>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n>> {\n>> -\tconst ControlList &metadata = descriptor.request_->metadata();\n>> -\tconst CameraMetadata &settings = descriptor.settings_;\n>> +\tconst ControlList &metadata = descriptor->request_->metadata();\n>> +\tconst CameraMetadata &settings = descriptor->settings_;\n>> \tcamera_metadata_ro_entry_t entry;\n>> \tbool found;\n>> \n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index a5576927..9c895b25 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -7,6 +7,7 @@\n>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n>> #define __ANDROID_CAMERA_DEVICE_H__\n>> \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -103,7 +104,7 @@ private:\n>> \t\t\t camera3_error_msg_code code);\n>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>> \tstd::unique_ptr getResultMetadata(\n>> -\t\tconst Camera3RequestDescriptor &descriptor) const;\n>> +\t\tconst Camera3RequestDescriptor *descriptor) const;\n>> \n>> \tunsigned int id_;\n>> \tcamera3_device_t camera3Device_;\n>> @@ -123,7 +124,7 @@ private:\n>> \tstd::vector streams_;\n>> \n>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>> -\tstd::map descriptors_;\n>> +\tstd::deque> descriptors_;\n> Could we use a std::queue instead ? It will be implemented on top of a\n> std::deque, so there will be no change in performances, but it gives us\n> the semantics we need (essentially, a FIFO).\n\n\nYes, great! The deque is coming from earlier version where we need to \niterate over the queue.\n\nI see, no place as such in v3, where the queue is iterated upon, so we \ncan surely use std::queue.\n\n>\n> Reviewed-by: Laurent Pinchart \n>\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 F02EFBF01C\n\tfor ;\n\tTue, 21 Sep 2021 04:26:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33B0F6918A;\n\tTue, 21 Sep 2021 06:26:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EED6C6012D\n\tfor ;\n\tTue, 21 Sep 2021 06:26:27 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.144])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C58622BA;\n\tTue, 21 Sep 2021 06:26:26 +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=\"Tx9QmzIT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632198387;\n\tbh=FdM3Ku3EqaQkp0mKha89CpxJYClzePFMwfyT2G4GVLw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Tx9QmzIT3hbGgWEuVM9CqD+nli+7ynV+8HUpVLsYsvF42kx3GHyln+znEkF4/rlm7\n\tQ1p1q/g80P+7c2RUGP0eyDpEnEDWMfAvlgv7a8aFQqnQkYGEo+i6eHbdXtka0oBMIh\n\tCHUwZmM+ygxG+sqgOsTesoLiXFc8q1HQ/Rsci57A=","To":"Laurent Pinchart ","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-3-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<9ee9737e-277c-e313-2698-5e05ea1bda8f@ideasonboard.com>","Date":"Tue, 21 Sep 2021 09:56:22 +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 02/10] 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":19728,"web_url":"https://patchwork.libcamera.org/comment/19728/","msgid":"<20210921110536.bimygraccq7ceurl@uno.localdomain>","date":"2021-09-21T11:05:36","subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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, Laurent\n\nOn Mon, Sep 20, 2021 at 10:06:32PM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:\n> > The descriptors_ map hold Camera3RequestDescriptor(s) which are\n> > per-capture request 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> > However, this approach has its limitations going forwards.\n> >\n> > In subsequent commits, the post-processing operation which happens\n> > in requestComplete() synchronously, is going to be run asynchronously.\n> > Therefore, instead of a map for descriptors, a queue makes more sense\n> > going forwards and the framework expects capture results to be received\n> > in the same order as they were queued. When the async processing is\n> > completed, the descriptor queue is inspected to send back the capture\n> > results and then de-queued. This helps to maintain the order of sending\n> > back the capture results whilst preventing unnecessary complexity of\n> > using a map.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------\n> > src/android/camera_device.h | 5 +-\n> > 2 files changed, 48 insertions(+), 46 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f461e14c..0562c225 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -926,7 +926,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> > @@ -937,12 +939,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> > -\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> > -\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\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> > \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> > \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> >\n> > @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t\tbuffer = createFrameBuffer(*camera3Buffer.buffer,\n> > \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n> > \t\t\t\t\t\t cameraStream->configuration().size);\n> > -\t\t\tdescriptor.frameBuffers_.emplace_back(buffer);\n> > +\t\t\tdescriptor->frameBuffers_.emplace_back(buffer);\n> > \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > \t\t\tbreak;\n> >\n> > @@ -999,7 +1001,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\tcamera3Buffer.acquire_fence);\n> > \t}\n> >\n> > @@ -1007,7 +1009,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> > @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\tstate_ = State::Running;\n> > \t}\n> >\n> > -\tworker_.queueRequest(descriptor.request_.get());\n> > +\tworker_.queueRequest(descriptor->request_.get());\n> >\n> > \t{\n> > \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > -\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > +\t\tdescriptors_.push_back(std::move(descriptor));\n> > \t}\n>\n> We have a race condition here, worker_.queueRequest() should go after\n> adding the request to the queue. Could you fix it in a patch on top ?\n>\n> >\n> > \treturn 0;\n> > @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >\n> > void CameraDevice::requestComplete(Request *request)\n> > {\n> > -\tdecltype(descriptors_)::node_type node;\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> > +\tif (descriptors_.empty())\n> > +\t\treturn;\n> >\n> > -\t\t\treturn;\n> > -\t\t}\n> > +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\n>\n> This needs to be protected by descriptorsMutex_.\n>\n> \tCamera3RequestDescriptor *descriptor;\n>\n> \t{\n> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> \t\tdescriptor = descriptors_.front().get();\n> \t}\n\nMmm, is this enough ?\n\nWe would get a reference to a unique_ptr<> stored in the global\ndescriptors_, which a concurrent call to configureStreams() (which\ncalls stop() and then descriptors_.clear()) or to close() (which again\ncalls stop()) would invalidate.\n\nI think we should either:\n1) Lock the whole requestComplete() function\n which is bad, as it could take a while. However if we'll get to a\n point where we can interrupt async processing could be acceptable\n\n2) Extract the descriptor from the queue and handle its lifetime\n inside this function, which I'm not yet clear on what implies.\n\nThat said, if we're going to move descriptor away from the queue, we\ncould as well create a dedicated struct/class with a different queue\nif it turns out to be simpler.\n\n>\n> > +\tif (descriptor->request_->cookie() != request->cookie()) {\n>\n> This is correct as long as we handle post-processing synchronously.\n> Let's see how it evolves in subsequent patches.\n>\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<< \"Unknown request: \" << request->cookie();\n>\n> I'd change the message to\n>\n> \t\t\t<< \"Out-of-order completion for request \"\n> \t\t\t<< request->cookie();\n>\n> By the way, with the cookie containing a pointer, I think it would be\n> more readable in hex. Maybe a patch on top to use utils::hex() ?\n>\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> > @@ -1075,14 +1073,14 @@ 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\tbuffer.acquire_fence = -1;\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> > @@ -1094,14 +1092,15 @@ 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\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >\n> > +\t\tdescriptors_.pop_front();\n>\n> I'm slightly concerned that in some paths we could complete the request\n> but forget to remove it from the queue. Maybe wrapping\n> callbacks_->process_capture_result() and descriptors_.pop_front() in a\n> function would be good. Let's see how it looks like with the whole\n> series applied.\n>\n> > \t\treturn;\n> > \t}\n> >\n> > @@ -1113,10 +1112,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> > @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)\n> > \t */\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 any JPEG compression. */\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> > @@ -1144,13 +1143,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.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> > @@ -1161,13 +1160,15 @@ 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> > +\tdescriptors_.pop_front();\n> > }\n> >\n> > std::string CameraDevice::logPrefix() const\n> > @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > * Produce a set of fixed result metadata.\n> > */\n> > std::unique_ptr\n> > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n> > {\n> > -\tconst ControlList &metadata = descriptor.request_->metadata();\n> > -\tconst CameraMetadata &settings = descriptor.settings_;\n> > +\tconst ControlList &metadata = descriptor->request_->metadata();\n> > +\tconst CameraMetadata &settings = descriptor->settings_;\n> > \tcamera_metadata_ro_entry_t entry;\n> > \tbool found;\n> >\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index a5576927..9c895b25 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > #define __ANDROID_CAMERA_DEVICE_H__\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -103,7 +104,7 @@ private:\n> > \t\t\t camera3_error_msg_code code);\n> > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > \tstd::unique_ptr getResultMetadata(\n> > -\t\tconst Camera3RequestDescriptor &descriptor) const;\n> > +\t\tconst Camera3RequestDescriptor *descriptor) const;\n> >\n> > \tunsigned int id_;\n> > \tcamera3_device_t camera3Device_;\n> > @@ -123,7 +124,7 @@ private:\n> > \tstd::vector streams_;\n> >\n> > \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> > -\tstd::map descriptors_;\n> > +\tstd::deque> descriptors_;\n>\n> Could we use a std::queue instead ? It will be implemented on top of a\n> std::deque, so there will be no change in performances, but it gives us\n> the semantics we need (essentially, a FIFO).\n>\n> Reviewed-by: Laurent Pinchart \n>\n> >\n> > \tstd::string maker_;\n> > \tstd::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 70217BDC71\n\tfor ;\n\tTue, 21 Sep 2021 11:04:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 849756918C;\n\tTue, 21 Sep 2021 13:04:52 +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 82FC460247\n\tfor ;\n\tTue, 21 Sep 2021 13:04:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id C0EF41C000C;\n\tTue, 21 Sep 2021 11:04:49 +0000 (UTC)"],"Date":"Tue, 21 Sep 2021 13:05:36 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210921110536.bimygraccq7ceurl@uno.localdomain>","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-3-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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":19756,"web_url":"https://patchwork.libcamera.org/comment/19756/","msgid":"","date":"2021-09-21T22:16:59","subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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\nOn Tue, Sep 21, 2021 at 09:56:22AM +0530, Umang Jain wrote:\n> On 9/21/21 12:36 AM, Laurent Pinchart wrote:\n> > On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:\n> >> The descriptors_ map hold Camera3RequestDescriptor(s) which are\n> >> per-capture request 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> >> However, this approach has its limitations going forwards.\n> >>\n> >> In subsequent commits, the post-processing operation which happens\n> >> in requestComplete() synchronously, is going to be run asynchronously.\n> >> Therefore, instead of a map for descriptors, a queue makes more sense\n> >> going forwards and the framework expects capture results to be received\n> >> in the same order as they were queued. When the async processing is\n> >> completed, the descriptor queue is inspected to send back the capture\n> >> results and then de-queued. This helps to maintain the order of sending\n> >> back the capture results whilst preventing unnecessary complexity of\n> >> using a map.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------\n> >> src/android/camera_device.h | 5 +-\n> >> 2 files changed, 48 insertions(+), 46 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index f461e14c..0562c225 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -926,7 +926,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> >> @@ -937,12 +939,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> >> -\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> >> -\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\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> >> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> >> \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> >> \n> >> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \t\t\tbuffer = createFrameBuffer(*camera3Buffer.buffer,\n> >> \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n> >> \t\t\t\t\t\t cameraStream->configuration().size);\n> >> -\t\t\tdescriptor.frameBuffers_.emplace_back(buffer);\n> >> +\t\t\tdescriptor->frameBuffers_.emplace_back(buffer);\n> >> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> >> \t\t\tbreak;\n> >> \n> >> @@ -999,7 +1001,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\tcamera3Buffer.acquire_fence);\n> >> \t}\n> >> \n> >> @@ -1007,7 +1009,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> >> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \t\tstate_ = State::Running;\n> >> \t}\n> >> \n> >> -\tworker_.queueRequest(descriptor.request_.get());\n> >> +\tworker_.queueRequest(descriptor->request_.get());\n> >> \n> >> \t{\n> >> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> >> -\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> >> +\t\tdescriptors_.push_back(std::move(descriptor));\n> >> \t}\n> >\n> > We have a race condition here, worker_.queueRequest() should go after\n> > adding the request to the queue. Could you fix it in a patch on top ?\n> \n> Do you mean the race condition is existing already, with the \n> descriptors_ map (that has been removed from this patch)?\n\nCorrect, it's already here.\n\n> Yes, I can introduce a patch before this one, that fixes the race first \n> in the map itself. Is my understanding correct?\n\nSounds good to me. It should be a small patch.\n\n> >> \n> >> \treturn 0;\n> >> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \n> >> void CameraDevice::requestComplete(Request *request)\n> >> {\n> >> -\tdecltype(descriptors_)::node_type node;\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> >> +\tif (descriptors_.empty())\n> >> +\t\treturn;\n> >> \n> >> -\t\t\treturn;\n> >> -\t\t}\n> >> +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\n> >\n> > This needs to be protected by descriptorsMutex_.\n> >\n> > \tCamera3RequestDescriptor *descriptor;\n> >\n> > \t{\n> > \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > \t\tdescriptor = descriptors_.front().get();\n> > \t}\n> >\n> >> +\tif (descriptor->request_->cookie() != request->cookie()) {\n> >\n> > This is correct as long as we handle post-processing synchronously.\n> > Let's see how it evolves in subsequent patches.\n> \n> Why not valid for async post-processing?\n> \n> So this is requestComplete() function, invoked whenever a \n> libcamera::Request is completed by libcamera::Camera. The completion is \n> guaranteed to be done in order, right ? Later in this function, the \n> post-processing shall happen (sync or async).\n\nWhen we'll move to async post-processing, the request at the front of\nthe queue will be a request undergoing post-processing. libcamera may\nsignal completion of the next request before the post-processing is\ncomplete, so the check will fail.\n\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<< \"Unknown request: \" << request->cookie();\n> >\n> > I'd change the message to\n> >\n> > \t\t\t<< \"Out-of-order completion for request \"\n> > \t\t\t<< request->cookie();\n> >\n> > By the way, with the cookie containing a pointer, I think it would be\n> > more readable in hex. Maybe a patch on top to use utils::hex() ?\n> \n> Makes sense, I'll double check\n> \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> >> @@ -1075,14 +1073,14 @@ 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\tbuffer.acquire_fence = -1;\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> >> @@ -1094,14 +1092,15 @@ 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\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >> \n> >> +\t\tdescriptors_.pop_front();\n> >\n> > I'm slightly concerned that in some paths we could complete the request\n> > but forget to remove it from the queue. Maybe wrapping\n> > callbacks_->process_capture_result() and descriptors_.pop_front() in a\n> > function would be good. Let's see how it looks like with the whole\n> > series applied.\n> \n> This is a good point and has been address already via \n> sendCaptureResults() in subsequent patches.\n> \n> However, this particular error path you have pointed out here, is the \n> /only/ part where queue.front() is dropped, other than ofcoourse in \n> sendCaptureResults(). hmm, I'll take a look if I can figure it out too \n> and have a singular place of processsing the queue.\n> \n> >> \t\treturn;\n> >> \t}\n> >> \n> >> @@ -1113,10 +1112,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> >> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t */\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 any JPEG compression. */\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> >> @@ -1144,13 +1143,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.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> >> @@ -1161,13 +1160,15 @@ 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> >> +\tdescriptors_.pop_front();\n> >> }\n> >> \n> >> std::string CameraDevice::logPrefix() const\n> >> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> >> * Produce a set of fixed result metadata.\n> >> */\n> >> std::unique_ptr\n> >> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> >> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n> >> {\n> >> -\tconst ControlList &metadata = descriptor.request_->metadata();\n> >> -\tconst CameraMetadata &settings = descriptor.settings_;\n> >> +\tconst ControlList &metadata = descriptor->request_->metadata();\n> >> +\tconst CameraMetadata &settings = descriptor->settings_;\n> >> \tcamera_metadata_ro_entry_t entry;\n> >> \tbool found;\n> >> \n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index a5576927..9c895b25 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -7,6 +7,7 @@\n> >> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> >> #define __ANDROID_CAMERA_DEVICE_H__\n> >> \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> @@ -103,7 +104,7 @@ private:\n> >> \t\t\t camera3_error_msg_code code);\n> >> \tint processControls(Camera3RequestDescriptor *descriptor);\n> >> \tstd::unique_ptr getResultMetadata(\n> >> -\t\tconst Camera3RequestDescriptor &descriptor) const;\n> >> +\t\tconst Camera3RequestDescriptor *descriptor) const;\n> >> \n> >> \tunsigned int id_;\n> >> \tcamera3_device_t camera3Device_;\n> >> @@ -123,7 +124,7 @@ private:\n> >> \tstd::vector streams_;\n> >> \n> >> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >> -\tstd::map descriptors_;\n> >> +\tstd::deque> descriptors_;\n> >\n> > Could we use a std::queue instead ? It will be implemented on top of a\n> > std::deque, so there will be no change in performances, but it gives us\n> > the semantics we need (essentially, a FIFO).\n> \n> Yes, great! The deque is coming from earlier version where we need to \n> iterate over the queue.\n> \n> I see, no place as such in v3, where the queue is iterated upon, so we \n> can surely use std::queue.\n> \n> > Reviewed-by: Laurent Pinchart \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 B4FD6BF01C\n\tfor ;\n\tTue, 21 Sep 2021 22:17:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E75D66918E;\n\tWed, 22 Sep 2021 00:17:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC39269186\n\tfor ;\n\tWed, 22 Sep 2021 00:17:30 +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 5E037291;\n\tWed, 22 Sep 2021 00:17:30 +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=\"MvzaXDMp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632262650;\n\tbh=fEzMnbU8auE3uiWNLk1fwEJRpd8u6x8quycKdgFRguI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MvzaXDMphANG4KvBruTMKgJvDOD7lsKVmBIPiHMeO5SWVNbTm+uO9pdRF5g0y9T7Z\n\tBxpYX/iNI8oaXezQjqcYryNkdsTDkLh1lDRLPVMPG5fat+Q6ua4GKXgcjutVhu6x+t\n\tEGkHaSmNy1VSqcDlkEQkMU4I4WXSQHvsdMprHmIM=","Date":"Wed, 22 Sep 2021 01:16:59 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-3-umang.jain@ideasonboard.com>\n\t\n\t<9ee9737e-277c-e313-2698-5e05ea1bda8f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<9ee9737e-277c-e313-2698-5e05ea1bda8f@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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":19757,"web_url":"https://patchwork.libcamera.org/comment/19757/","msgid":"","date":"2021-09-21T22:31:32","subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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 Jacopo,\n\nOn Tue, Sep 21, 2021 at 01:05:36PM +0200, Jacopo Mondi wrote:\n> On Mon, Sep 20, 2021 at 10:06:32PM +0300, Laurent Pinchart wrote:\n> > On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:\n> > > The descriptors_ map hold Camera3RequestDescriptor(s) which are\n> > > per-capture request 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> > > However, this approach has its limitations going forwards.\n> > >\n> > > In subsequent commits, the post-processing operation which happens\n> > > in requestComplete() synchronously, is going to be run asynchronously.\n> > > Therefore, instead of a map for descriptors, a queue makes more sense\n> > > going forwards and the framework expects capture results to be received\n> > > in the same order as they were queued. When the async processing is\n> > > completed, the descriptor queue is inspected to send back the capture\n> > > results and then de-queued. This helps to maintain the order of sending\n> > > back the capture results whilst preventing unnecessary complexity of\n> > > using a map.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------\n> > > src/android/camera_device.h | 5 +-\n> > > 2 files changed, 48 insertions(+), 46 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index f461e14c..0562c225 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -926,7 +926,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> > > @@ -937,12 +939,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> > > -\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> > > -\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\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> > > \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> > > \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> > >\n> > > @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \t\t\tbuffer = createFrameBuffer(*camera3Buffer.buffer,\n> > > \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n> > > \t\t\t\t\t\t cameraStream->configuration().size);\n> > > -\t\t\tdescriptor.frameBuffers_.emplace_back(buffer);\n> > > +\t\t\tdescriptor->frameBuffers_.emplace_back(buffer);\n> > > \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > > \t\t\tbreak;\n> > >\n> > > @@ -999,7 +1001,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\tcamera3Buffer.acquire_fence);\n> > > \t}\n> > >\n> > > @@ -1007,7 +1009,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> > > @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \t\tstate_ = State::Running;\n> > > \t}\n> > >\n> > > -\tworker_.queueRequest(descriptor.request_.get());\n> > > +\tworker_.queueRequest(descriptor->request_.get());\n> > >\n> > > \t{\n> > > \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > > -\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > > +\t\tdescriptors_.push_back(std::move(descriptor));\n> > > \t}\n> >\n> > We have a race condition here, worker_.queueRequest() should go after\n> > adding the request to the queue. Could you fix it in a patch on top ?\n> >\n> > >\n> > > \treturn 0;\n> > > @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >\n> > > void CameraDevice::requestComplete(Request *request)\n> > > {\n> > > -\tdecltype(descriptors_)::node_type node;\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> > > +\tif (descriptors_.empty())\n> > > +\t\treturn;\n> > >\n> > > -\t\t\treturn;\n> > > -\t\t}\n> > > +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\n> >\n> > This needs to be protected by descriptorsMutex_.\n> >\n> > \tCamera3RequestDescriptor *descriptor;\n> >\n> > \t{\n> > \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > \t\tdescriptor = descriptors_.front().get();\n> > \t}\n> \n> Mmm, is this enough ?\n> \n> We would get a reference to a unique_ptr<> stored in the global\n> descriptors_, which a concurrent call to configureStreams() (which\n> calls stop() and then descriptors_.clear()) or to close() (which again\n> calls stop()) would invalidate.\n> \n> I think we should either:\n> 1) Lock the whole requestComplete() function\n> which is bad, as it could take a while. However if we'll get to a\n> point where we can interrupt async processing could be acceptable\n> \n> 2) Extract the descriptor from the queue and handle its lifetime\n> inside this function, which I'm not yet clear on what implies.\n> \n> That said, if we're going to move descriptor away from the queue, we\n> could as well create a dedicated struct/class with a different queue\n> if it turns out to be simpler.\n\nAs discussed offline, stop() (and flush()) needs to wait for all pending\nrequests to complete (possibly with an error status for the requests\nthat get cancelled) before we can proceed. Otherwise, if we reported to\nthe camera service that all requests are completed while some are\nactually in flight, the camera service could reuse (or free) buffers\nthat we're writing to.\n\n> > > +\tif (descriptor->request_->cookie() != request->cookie()) {\n> >\n> > This is correct as long as we handle post-processing synchronously.\n> > Let's see how it evolves in subsequent patches.\n> >\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<< \"Unknown request: \" << request->cookie();\n> >\n> > I'd change the message to\n> >\n> > \t\t\t<< \"Out-of-order completion for request \"\n> > \t\t\t<< request->cookie();\n> >\n> > By the way, with the cookie containing a pointer, I think it would be\n> > more readable in hex. Maybe a patch on top to use utils::hex() ?\n> >\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> > > @@ -1075,14 +1073,14 @@ 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\tbuffer.acquire_fence = -1;\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> > > @@ -1094,14 +1092,15 @@ 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\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > >\n> > > +\t\tdescriptors_.pop_front();\n> >\n> > I'm slightly concerned that in some paths we could complete the request\n> > but forget to remove it from the queue. Maybe wrapping\n> > callbacks_->process_capture_result() and descriptors_.pop_front() in a\n> > function would be good. Let's see how it looks like with the whole\n> > series applied.\n> >\n> > > \t\treturn;\n> > > \t}\n> > >\n> > > @@ -1113,10 +1112,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> > > @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)\n> > > \t */\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 any JPEG compression. */\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> > > @@ -1144,13 +1143,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.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> > > @@ -1161,13 +1160,15 @@ 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> > > +\tdescriptors_.pop_front();\n> > > }\n> > >\n> > > std::string CameraDevice::logPrefix() const\n> > > @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > > * Produce a set of fixed result metadata.\n> > > */\n> > > std::unique_ptr\n> > > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> > > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n> > > {\n> > > -\tconst ControlList &metadata = descriptor.request_->metadata();\n> > > -\tconst CameraMetadata &settings = descriptor.settings_;\n> > > +\tconst ControlList &metadata = descriptor->request_->metadata();\n> > > +\tconst CameraMetadata &settings = descriptor->settings_;\n> > > \tcamera_metadata_ro_entry_t entry;\n> > > \tbool found;\n> > >\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index a5576927..9c895b25 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -7,6 +7,7 @@\n> > > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > > #define __ANDROID_CAMERA_DEVICE_H__\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -103,7 +104,7 @@ private:\n> > > \t\t\t camera3_error_msg_code code);\n> > > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > > \tstd::unique_ptr getResultMetadata(\n> > > -\t\tconst Camera3RequestDescriptor &descriptor) const;\n> > > +\t\tconst Camera3RequestDescriptor *descriptor) const;\n> > >\n> > > \tunsigned int id_;\n> > > \tcamera3_device_t camera3Device_;\n> > > @@ -123,7 +124,7 @@ private:\n> > > \tstd::vector streams_;\n> > >\n> > > \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> > > -\tstd::map descriptors_;\n> > > +\tstd::deque> descriptors_;\n> >\n> > Could we use a std::queue instead ? It will be implemented on top of a\n> > std::deque, so there will be no change in performances, but it gives us\n> > the semantics we need (essentially, a FIFO).\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\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 DE757BDC71\n\tfor ;\n\tTue, 21 Sep 2021 22:32:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 434DA6918C;\n\tWed, 22 Sep 2021 00:32:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DF6069186\n\tfor ;\n\tWed, 22 Sep 2021 00:32:03 +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 F41CB291;\n\tWed, 22 Sep 2021 00:32:02 +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=\"BZyOBQCd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632263523;\n\tbh=Ojo7bQLwVa7jU70JMYDKobxoVDIGiG+nYPNStvSN0ew=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BZyOBQCdrLRuc7rhx5Trq4rwoo0mDUcrkISuP0bUWvhyIYWONRN78zMeBwtDUpRiL\n\tZYma2m2WDXmfqgpH0wFsM1mpwjuxcBFWqGiNMPybLRLiHbzkKxOkLKmpSLdp/yw6bu\n\tmQQBUry17o52WaJj5oorCYz3X3zlM4luufS3j5jY=","Date":"Wed, 22 Sep 2021 01:31:32 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-3-umang.jain@ideasonboard.com>\n\t\n\t<20210921110536.bimygraccq7ceurl@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210921110536.bimygraccq7ceurl@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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":19779,"web_url":"https://patchwork.libcamera.org/comment/19779/","msgid":"<79e080a2-163f-b75e-7033-0ebf91d1d612@ideasonboard.com>","date":"2021-09-22T08:28:19","subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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 Laurent,\n\nThanks for the review.\n\nOn 9/22/21 3:46 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Tue, Sep 21, 2021 at 09:56:22AM +0530, Umang Jain wrote:\n>> On 9/21/21 12:36 AM, Laurent Pinchart wrote:\n>>> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:\n>>>> The descriptors_ map hold Camera3RequestDescriptor(s) which are\n>>>> per-capture request 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>>>> However, this approach has its limitations going forwards.\n>>>>\n>>>> In subsequent commits, the post-processing operation which happens\n>>>> in requestComplete() synchronously, is going to be run asynchronously.\n>>>> Therefore, instead of a map for descriptors, a queue makes more sense\n>>>> going forwards and the framework expects capture results to be received\n>>>> in the same order as they were queued. When the async processing is\n>>>> completed, the descriptor queue is inspected to send back the capture\n>>>> results and then de-queued. This helps to maintain the order of sending\n>>>> back the capture results whilst preventing unnecessary complexity of\n>>>> using a map.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------\n>>>> src/android/camera_device.h | 5 +-\n>>>> 2 files changed, 48 insertions(+), 46 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index f461e14c..0562c225 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -926,7 +926,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>>>> @@ -937,12 +939,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>>>> -\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n>>>> -\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\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>>>> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n>>>> \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n>>>> \n>>>> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> \t\t\tbuffer = createFrameBuffer(*camera3Buffer.buffer,\n>>>> \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n>>>> \t\t\t\t\t\t cameraStream->configuration().size);\n>>>> -\t\t\tdescriptor.frameBuffers_.emplace_back(buffer);\n>>>> +\t\t\tdescriptor->frameBuffers_.emplace_back(buffer);\n>>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n>>>> \t\t\tbreak;\n>>>> \n>>>> @@ -999,7 +1001,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\tcamera3Buffer.acquire_fence);\n>>>> \t}\n>>>> \n>>>> @@ -1007,7 +1009,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>>>> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> \t\tstate_ = State::Running;\n>>>> \t}\n>>>> \n>>>> -\tworker_.queueRequest(descriptor.request_.get());\n>>>> +\tworker_.queueRequest(descriptor->request_.get());\n>>>> \n>>>> \t{\n>>>> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>>>> -\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>>>> +\t\tdescriptors_.push_back(std::move(descriptor));\n>>>> \t}\n>>> We have a race condition here, worker_.queueRequest() should go after\n>>> adding the request to the queue. Could you fix it in a patch on top ?\n>> Do you mean the race condition is existing already, with the\n>> descriptors_ map (that has been removed from this patch)?\n> Correct, it's already here.\n>\n>> Yes, I can introduce a patch before this one, that fixes the race first\n>> in the map itself. Is my understanding correct?\n> Sounds good to me. It should be a small patch.\n>\n>>>> \n>>>> \treturn 0;\n>>>> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> \n>>>> void CameraDevice::requestComplete(Request *request)\n>>>> {\n>>>> -\tdecltype(descriptors_)::node_type node;\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>>>> +\tif (descriptors_.empty())\n>>>> +\t\treturn;\n>>>> \n>>>> -\t\t\treturn;\n>>>> -\t\t}\n>>>> +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\n>>> This needs to be protected by descriptorsMutex_.\n>>>\n>>> \tCamera3RequestDescriptor *descriptor;\n>>>\n>>> \t{\n>>> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>>> \t\tdescriptor = descriptors_.front().get();\n>>> \t}\n>>>\n>>>> +\tif (descriptor->request_->cookie() != request->cookie()) {\n>>> This is correct as long as we handle post-processing synchronously.\n>>> Let's see how it evolves in subsequent patches.\n>> Why not valid for async post-processing?\n>>\n>> So this is requestComplete() function, invoked whenever a\n>> libcamera::Request is completed by libcamera::Camera. The completion is\n>> guaranteed to be done in order, right ? Later in this function, the\n>> post-processing shall happen (sync or async).\n> When we'll move to async post-processing, the request at the front of\n> the queue will be a request undergoing post-processing. libcamera may\n> signal completion of the next request before the post-processing is\n> complete, so the check will fail.\n\n\nYes, you are right, my bad :S\n\n>\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<< \"Unknown request: \" << request->cookie();\n>>> I'd change the message to\n>>>\n>>> \t\t\t<< \"Out-of-order completion for request \"\n>>> \t\t\t<< request->cookie();\n>>>\n>>> By the way, with the cookie containing a pointer, I think it would be\n>>> more readable in hex. Maybe a patch on top to use utils::hex() ?\n>> Makes sense, I'll double check\n>>\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>>>> @@ -1075,14 +1073,14 @@ 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\tbuffer.acquire_fence = -1;\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>>>> @@ -1094,14 +1092,15 @@ 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\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>>> \n>>>> +\t\tdescriptors_.pop_front();\n>>> I'm slightly concerned that in some paths we could complete the request\n>>> but forget to remove it from the queue. Maybe wrapping\n>>> callbacks_->process_capture_result() and descriptors_.pop_front() in a\n>>> function would be good. Let's see how it looks like with the whole\n>>> series applied.\n>> This is a good point and has been address already via\n>> sendCaptureResults() in subsequent patches.\n>>\n>> However, this particular error path you have pointed out here, is the\n>> /only/ part where queue.front() is dropped, other than ofcoourse in\n>> sendCaptureResults(). hmm, I'll take a look if I can figure it out too\n>> and have a singular place of processsing the queue.\n>>\n>>>> \t\treturn;\n>>>> \t}\n>>>> \n>>>> @@ -1113,10 +1112,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>>>> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)\n>>>> \t */\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 any JPEG compression. */\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>>>> @@ -1144,13 +1143,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.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>>>> @@ -1161,13 +1160,15 @@ 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>>>> +\tdescriptors_.pop_front();\n>>>> }\n>>>> \n>>>> std::string CameraDevice::logPrefix() const\n>>>> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n>>>> * Produce a set of fixed result metadata.\n>>>> */\n>>>> std::unique_ptr\n>>>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n>>>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n>>>> {\n>>>> -\tconst ControlList &metadata = descriptor.request_->metadata();\n>>>> -\tconst CameraMetadata &settings = descriptor.settings_;\n>>>> +\tconst ControlList &metadata = descriptor->request_->metadata();\n>>>> +\tconst CameraMetadata &settings = descriptor->settings_;\n>>>> \tcamera_metadata_ro_entry_t entry;\n>>>> \tbool found;\n>>>> \n>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>>> index a5576927..9c895b25 100644\n>>>> --- a/src/android/camera_device.h\n>>>> +++ b/src/android/camera_device.h\n>>>> @@ -7,6 +7,7 @@\n>>>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n>>>> #define __ANDROID_CAMERA_DEVICE_H__\n>>>> \n>>>> +#include \n>>>> #include \n>>>> #include \n>>>> #include \n>>>> @@ -103,7 +104,7 @@ private:\n>>>> \t\t\t camera3_error_msg_code code);\n>>>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>>>> \tstd::unique_ptr getResultMetadata(\n>>>> -\t\tconst Camera3RequestDescriptor &descriptor) const;\n>>>> +\t\tconst Camera3RequestDescriptor *descriptor) const;\n>>>> \n>>>> \tunsigned int id_;\n>>>> \tcamera3_device_t camera3Device_;\n>>>> @@ -123,7 +124,7 @@ private:\n>>>> \tstd::vector streams_;\n>>>> \n>>>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>>>> -\tstd::map descriptors_;\n>>>> +\tstd::deque> descriptors_;\n>>> Could we use a std::queue instead ? It will be implemented on top of a\n>>> std::deque, so there will be no change in performances, but it gives us\n>>> the semantics we need (essentially, a FIFO).\n>> Yes, great! The deque is coming from earlier version where we need to\n>> iterate over the queue.\n>>\n>> I see, no place as such in v3, where the queue is iterated upon, so we\n>> can surely use std::queue.\n>>\n>>> Reviewed-by: Laurent Pinchart \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 9FB15BF01C\n\tfor ;\n\tWed, 22 Sep 2021 08:28:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC3B56918C;\n\tWed, 22 Sep 2021 10:28:26 +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 76CBF687DD\n\tfor ;\n\tWed, 22 Sep 2021 10:28:25 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.207])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15A98F1;\n\tWed, 22 Sep 2021 10:28:23 +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=\"n3v795rM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632299304;\n\tbh=X422BRxnoijj6t50A+Rid9+o8BPDrVuBCdZJayQmqRY=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=n3v795rMIdx2z092Hj48UbSC/M5ttBja0ldvN0j7W6fYk1apKmVFJW9bmt7bnPHAd\n\tT1MssyZ0MvqSdOa2SA/62Hj6lgzdG7LsARpKwzPAoBttVAilGVEF+oRTR/tSMk3Iq1\n\tlPXpNvSC5O2y0FnTFIxwPuFUM3Kb6PqB2BVRduo4=","To":"Laurent Pinchart ","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-3-umang.jain@ideasonboard.com>\n\t\n\t<9ee9737e-277c-e313-2698-5e05ea1bda8f@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<79e080a2-163f-b75e-7033-0ebf91d1d612@ideasonboard.com>","Date":"Wed, 22 Sep 2021 13:58:19 +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 02/10] 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":19856,"web_url":"https://patchwork.libcamera.org/comment/19856/","msgid":"","date":"2021-09-27T05:59:25","subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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 22, 2021 at 5:28 PM Umang Jain wrote:\n>\n> Hi Laurent,\n>\n> Thanks for the review.\n>\n> On 9/22/21 3:46 AM, Laurent Pinchart wrote:\n> > Hi Umang,\n> >\n> > On Tue, Sep 21, 2021 at 09:56:22AM +0530, Umang Jain wrote:\n> >> On 9/21/21 12:36 AM, Laurent Pinchart wrote:\n> >>> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:\n> >>>> The descriptors_ map hold Camera3RequestDescriptor(s) which are\n> >>>> per-capture request 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> >>>> However, this approach has its limitations going forwards.\n> >>>>\n> >>>> In subsequent commits, the post-processing operation which happens\n> >>>> in requestComplete() synchronously, is going to be run asynchronously.\n> >>>> Therefore, instead of a map for descriptors, a queue makes more sense\n> >>>> going forwards and the framework expects capture results to be received\n> >>>> in the same order as they were queued. When the async processing is\n> >>>> completed, the descriptor queue is inspected to send back the capture\n> >>>> results and then de-queued. This helps to maintain the order of sending\n> >>>> back the capture results whilst preventing unnecessary complexity of\n> >>>> using a map.\n> >>>>\n> >>>> Signed-off-by: Umang Jain \n> >>>> ---\n> >>>> src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------\n> >>>> src/android/camera_device.h | 5 +-\n> >>>> 2 files changed, 48 insertions(+), 46 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index f461e14c..0562c225 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -926,7 +926,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 here.\n\n> >>>>\n> >>>> /*\n> >>>> * \\todo The Android request model is incremental, settings passed in\n> >>>> @@ -937,12 +939,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> >>>> - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> >>>> - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\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> >>>> camera3_stream *camera3Stream = camera3Buffer.stream;\n> >>>> CameraStream *cameraStream = static_cast(camera3Stream->priv);\n> >>>>\n> >>>> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>> buffer = createFrameBuffer(*camera3Buffer.buffer,\n> >>>> cameraStream->configuration().pixelFormat,\n> >>>> cameraStream->configuration().size);\n> >>>> - descriptor.frameBuffers_.emplace_back(buffer);\n> >>>> + descriptor->frameBuffers_.emplace_back(buffer);\n> >>>> LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> >>>> break;\n> >>>>\n> >>>> @@ -999,7 +1001,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> >>>> camera3Buffer.acquire_fence);\n> >>>> }\n> >>>>\n> >>>> @@ -1007,7 +1009,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> >>>> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>> state_ = State::Running;\n> >>>> }\n> >>>>\n> >>>> - worker_.queueRequest(descriptor.request_.get());\n> >>>> + worker_.queueRequest(descriptor->request_.get());\n> >>>>\n> >>>> {\n> >>>> MutexLocker descriptorsLock(descriptorsMutex_);\n> >>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> >>>> + descriptors_.push_back(std::move(descriptor));\n> >>>> }\n> >>> We have a race condition here, worker_.queueRequest() should go after\n> >>> adding the request to the queue. Could you fix it in a patch on top ?\n> >> Do you mean the race condition is existing already, with the\n> >> descriptors_ map (that has been removed from this patch)?\n> > Correct, it's already here.\n> >\n> >> Yes, I can introduce a patch before this one, that fixes the race first\n> >> in the map itself. Is my understanding correct?\n> > Sounds good to me. It should be a small patch.\n> >\n> >>>>\n> >>>> return 0;\n> >>>> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>>\n> >>>> void CameraDevice::requestComplete(Request *request)\n> >>>> {\n> >>>> - decltype(descriptors_)::node_type node;\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> >>>> + if (descriptors_.empty())\n> >>>> + return;\n> >>>>\n> >>>> - return;\n> >>>> - }\n> >>>> + Camera3RequestDescriptor *descriptor = descriptors_.front().get();\n> >>> This needs to be protected by descriptorsMutex_.\n> >>>\n> >>> Camera3RequestDescriptor *descriptor;\n> >>>\n> >>> {\n> >>> MutexLocker descriptorsLock(descriptorsMutex_);\n> >>> descriptor = descriptors_.front().get();\n> >>> }\n> >>>\n> >>>> + if (descriptor->request_->cookie() != request->cookie()) {\n> >>> This is correct as long as we handle post-processing synchronously.\n> >>> Let's see how it evolves in subsequent patches.\n> >> Why not valid for async post-processing?\n> >>\n> >> So this is requestComplete() function, invoked whenever a\n> >> libcamera::Request is completed by libcamera::Camera. The completion is\n> >> guaranteed to be done in order, right ? Later in this function, the\n> >> post-processing shall happen (sync or async).\n> > When we'll move to async post-processing, the request at the front of\n> > the queue will be a request undergoing post-processing. libcamera may\n> > signal completion of the next request before the post-processing is\n> > complete, so the check will fail.\n>\n>\n> Yes, you are right, my bad :S\n>\n> >\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> >>> I'd change the message to\n> >>>\n> >>> << \"Out-of-order completion for request \"\n> >>> << request->cookie();\n> >>>\n> >>> By the way, with the cookie containing a pointer, I think it would be\n> >>> more readable in hex. Maybe a patch on top to use utils::hex() ?\n> >> Makes sense, I'll double check\n> >>\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> >>>> @@ -1075,14 +1073,14 @@ 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> >>>> buffer.acquire_fence = -1;\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> >>>> @@ -1094,14 +1092,15 @@ 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> >>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >>>> callbacks_->process_capture_result(callbacks_, &captureResult);\n> >>>>\n> >>>> + descriptors_.pop_front();\n> >>> I'm slightly concerned that in some paths we could complete the request\n> >>> but forget to remove it from the queue. Maybe wrapping\n> >>> callbacks_->process_capture_result() and descriptors_.pop_front() in a\n> >>> function would be good. Let's see how it looks like with the whole\n> >>> series applied.\n> >> This is a good point and has been address already via\n> >> sendCaptureResults() in subsequent patches.\n> >>\n> >> However, this particular error path you have pointed out here, is the\n> >> /only/ part where queue.front() is dropped, other than ofcoourse in\n> >> sendCaptureResults(). hmm, I'll take a look if I can figure it out too\n> >> and have a singular place of processsing the queue.\n> >>\n> >>>> return;\n> >>>> }\n> >>>>\n> >>>> @@ -1113,10 +1112,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> >>>> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)\n> >>>> */\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 any JPEG compression. */\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> >>>> @@ -1144,13 +1143,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.buffer,\n> >>>> - descriptor.settings_,\n> >>>> + descriptor->settings_,\n> >>>> resultMetadata.get());\n> >>>> /*\n> >>>> * Return the FrameBuffer to the CameraStream now that we're\n> >>>> @@ -1161,13 +1160,15 @@ 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> >>>> + descriptors_.pop_front();\n> >>>> }\n> >>>>\n> >>>> std::string CameraDevice::logPrefix() const\n> >>>> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> >>>> * Produce a set of fixed result metadata.\n> >>>> */\n> >>>> std::unique_ptr\n> >>>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> >>>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n> >>>> {\n> >>>> - const ControlList &metadata = descriptor.request_->metadata();\n> >>>> - const CameraMetadata &settings = descriptor.settings_;\n> >>>> + const ControlList &metadata = descriptor->request_->metadata();\n> >>>> + const CameraMetadata &settings = descriptor->settings_;\n> >>>> camera_metadata_ro_entry_t entry;\n> >>>> bool found;\n> >>>>\n> >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >>>> index a5576927..9c895b25 100644\n> >>>> --- a/src/android/camera_device.h\n> >>>> +++ b/src/android/camera_device.h\n> >>>> @@ -7,6 +7,7 @@\n> >>>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> >>>> #define __ANDROID_CAMERA_DEVICE_H__\n> >>>>\n> >>>> +#include \n> >>>> #include \n> >>>> #include \n> >>>> #include \n> >>>> @@ -103,7 +104,7 @@ private:\n> >>>> camera3_error_msg_code code);\n> >>>> int processControls(Camera3RequestDescriptor *descriptor);\n> >>>> std::unique_ptr getResultMetadata(\n> >>>> - const Camera3RequestDescriptor &descriptor) const;\n> >>>> + const Camera3RequestDescriptor *descriptor) const;\n> >>>>\n> >>>> unsigned int id_;\n> >>>> camera3_device_t camera3Device_;\n> >>>> @@ -123,7 +124,7 @@ private:\n> >>>> std::vector streams_;\n> >>>>\n> >>>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >>>> - std::map descriptors_;\n> >>>> + std::deque> descriptors_;\n> >>> Could we use a std::queue instead ? It will be implemented on top of a\n> >>> std::deque, so there will be no change in performances, but it gives us\n> >>> the semantics we need (essentially, a FIFO).\n> >> Yes, great! The deque is coming from earlier version where we need to\n> >> iterate over the queue.\n> >>\n> >> I see, no place as such in v3, where the queue is iterated upon, so we\n> >> can surely use std::queue.\n> >>\n> >>> Reviewed-by: Laurent Pinchart \n\nAll the problems I noticed have been pointed out by Laurent and Jacopo.\nI look forward to the next version.\n\n-Hiro\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 84912BF01C\n\tfor ;\n\tMon, 27 Sep 2021 05:59:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E44BF6918E;\n\tMon, 27 Sep 2021 07:59:37 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B67F8684C8\n\tfor ;\n\tMon, 27 Sep 2021 07:59:36 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id b26so16070639edt.0\n\tfor ;\n\tSun, 26 Sep 2021 22:59:36 -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=\"UEBiaYVI\"; 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=EX9UEkWDamd59hijLPY7FkBrF5SKtgjSuFa4Uw+jrkY=;\n\tb=UEBiaYVI+XYsPMwAW+js1F/un6BRxg9ZURbHAzV42Z5pZEBZ6hM6PU2PWuzGNEcQy+\n\tg3EAGIpjlzuiuNE7e80ms2xwVUCkoxF3PIfYqdkv/bOO52W+WO57ssVKa/ANGqrKQqEY\n\tVZ8IkD8xzXWh0krQ2dYTgpKGnRyZXH/4R14EE=","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=EX9UEkWDamd59hijLPY7FkBrF5SKtgjSuFa4Uw+jrkY=;\n\tb=4k4mBXtlqCeJyH7goJHFrGQJysQNZs0Gs1Q10B58sxqbNRI/7ayWrcg4lg/AlUtCA8\n\tNaRVAuyVEfy5NCJ2zCp4E8fnXN5pWef0ZzOmptQGRGBC40mrRGBtuD46ys1Xcn6NDhpi\n\ta+oMgCqWHlOAoLIl85YPZXmBmJqxeckQaN6yybsLSLBI15IxIo2BndxSLlhmEiDcy+qM\n\thCTQ9N9FWhNBS/W7UTjz57B5OeqZz3fv/scjgZf+1WLP7WbE0aBZB+W8/je1E+YzlfO7\n\tdrlHisQQ8R5XWNM9BK+rtNA64NUY48ov+gPvB5m9k/pa2i0/YdgOUVCDQAKsDHQ441Lm\n\tNjcg==","X-Gm-Message-State":"AOAM531YDJPJfQTZEkglkz1sN6wj+qsO2LCb99BX3ErZNtSYCyA7oQeB\n\tUYYvCLxJj17+rcuJ47A5Y0aorAVjuVCDOAot/DloEbNDNwg=","X-Google-Smtp-Source":"ABdhPJzxdwPPIpcyuzVNuUfTy3kYZdDbNQIsgr3cHU9tKQ6f6sF7IprRLlD1zpQTwORnFB/144GGIQ/3ooaT6r1WwrE=","X-Received":"by 2002:a17:906:280f:: with SMTP id\n\tr15mr11824266ejc.559.1632722376159; \n\tSun, 26 Sep 2021 22:59:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-3-umang.jain@ideasonboard.com>\n\t\n\t<9ee9737e-277c-e313-2698-5e05ea1bda8f@ideasonboard.com>\n\t\n\t<79e080a2-163f-b75e-7033-0ebf91d1d612@ideasonboard.com>","In-Reply-To":"<79e080a2-163f-b75e-7033-0ebf91d1d612@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 27 Sep 2021 14:59:25 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 02/10] 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\" "}}]