[{"id":20007,"web_url":"https://patchwork.libcamera.org/comment/20007/","msgid":"<20210929184011.gutadrufccmlhhaz@uno.localdomain>","date":"2021-09-29T18:40:11","subject":"Re: [libcamera-devel] [PATCH v4 2/4] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Wed, Sep 29, 2021 at 07:00:28PM +0530, Umang Jain wrote:\n> The descriptors_ map holds Camera3RequestDescriptor(s) which are\n> per-capture requests placed by the framework to libcamera HAL.\n> CameraDevice::requestComplete() looks for the descriptor for which the\n> camera request has been completed and removes it from the map.\n> Since the requests are placed in form of FIFO and the framework expects\n> the order of completion to be FIFO as well, this calls for a need of\n> a queue rather than a std::map.\n>\n> This patch still keeps the same lifetime of Camera3RequestDescriptor as\n> before i.e. in the requestComplete(). Previously, a descriptor was\n> extracted from the map and its lifetime was bound to requestComplete().\n> The lifetime is kept same by manually calling .pop_front() on the\n\nis kept the same\n\n> queue. In the subsequent commit, this is likely to change with a\n> centralized location of dropping descriptors from the queue for request\n> completion.\n>\n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n   j\n\n> ---\n>  src/android/camera_device.cpp | 93 +++++++++++++++++++----------------\n>  src/android/camera_device.h   |  3 +-\n>  2 files changed, 53 insertions(+), 43 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 9287ea07..e815f7a0 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -457,7 +457,8 @@ void CameraDevice::stop()\n>  \tworker_.stop();\n>  \tcamera_->stop();\n>\n> -\tdescriptors_.clear();\n> +\tdescriptors_ = {};\n> +\n>  \tstate_ = State::Stopped;\n>  }\n>\n> @@ -955,7 +956,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t * The descriptor and the associated memory reserved here are freed\n>  \t * at request complete time.\n>  \t */\n> -\tCamera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n> +\tauto descriptor = std::make_unique<Camera3RequestDescriptor>(camera_.get(),\n> +\t\t\t\t\t\t\t\t     camera3Request);\n>\n>  \t/*\n>  \t * \\todo The Android request model is incremental, settings passed in\n> @@ -966,12 +968,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \tif (camera3Request->settings)\n>  \t\tlastSettings_ = camera3Request->settings;\n>  \telse\n> -\t\tdescriptor.settings_ = lastSettings_;\n> +\t\tdescriptor->settings_ = lastSettings_;\n> +\n> +\tLOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> +\t\t\t<< \" with \" << descriptor->buffers_.size() << \" streams\";\n>\n> -\tLOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n> -\t\t\t<< \" with \" << descriptor.buffers_.size() << \" streams\";\n> -\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> -\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> +\tfor (const auto &[i, camera3Buffer] : utils::enumerate(descriptor->buffers_)) {\n>  \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n>  \t\tCameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);\n>\n> @@ -1007,12 +1009,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t\t\t * associate it with the Camera3RequestDescriptor for\n>  \t\t\t * lifetime management only.\n>  \t\t\t */\n> -\t\t\tdescriptor.frameBuffers_.push_back(\n> +\t\t\tdescriptor->frameBuffers_.push_back(\n>  \t\t\t\tcreateFrameBuffer(*camera3Buffer.buffer,\n>  \t\t\t\t\t\t  cameraStream->configuration().pixelFormat,\n>  \t\t\t\t\t\t  cameraStream->configuration().size));\n>\n> -\t\t\tbuffer = descriptor.frameBuffers_.back().get();\n> +\t\t\tbuffer = descriptor->frameBuffers_.back().get();\n>  \t\t\tacquireFence = camera3Buffer.acquire_fence;\n>  \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n>  \t\t\tbreak;\n> @@ -1035,7 +1037,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t\t\treturn -ENOMEM;\n>  \t\t}\n>\n> -\t\tdescriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> +\t\tdescriptor->request_->addBuffer(cameraStream->stream(), buffer,\n>  \t\t\t\t\t       acquireFence);\n>  \t}\n>\n> @@ -1043,7 +1045,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t * Translate controls from Android to libcamera and queue the request\n>  \t * to the CameraWorker thread.\n>  \t */\n> -\tint ret = processControls(&descriptor);\n> +\tint ret = processControls(descriptor.get());\n>  \tif (ret)\n>  \t\treturn ret;\n>\n> @@ -1071,11 +1073,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t\tstate_ = State::Running;\n>  \t}\n>\n> -\tCaptureRequest *request = descriptor.request_.get();\n> +\tCaptureRequest *request = descriptor->request_.get();\n>\n>  \t{\n>  \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> +\t\tdescriptors_.push(std::move(descriptor));\n>  \t}\n>\n>  \tworker_.queueRequest(request);\n> @@ -1085,26 +1087,28 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n>  void CameraDevice::requestComplete(Request *request)\n>  {\n> -\tdecltype(descriptors_)::node_type node;\n> +\tCamera3RequestDescriptor *descriptor;\n>  \t{\n>  \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\tauto it = descriptors_.find(request->cookie());\n> -\t\tif (it == descriptors_.end()) {\n> -\t\t\t/*\n> -\t\t\t * \\todo Clarify if the Camera has to be closed on\n> -\t\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> -\t\t\t * Error.\n> -\t\t\t */\n> -\t\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> -\t\t\tLOG(HAL, Fatal)\n> -\t\t\t\t<< \"Unknown request: \" << request->cookie();\n> +\t\tASSERT(!descriptors_.empty());\n> +\t\tdescriptor = descriptors_.front().get();\n> +\t}\n>\n> -\t\t\treturn;\n> -\t\t}\n> +\tif (descriptor->request_->cookie() != request->cookie()) {\n> +\t\t/*\n> +\t\t * \\todo Clarify if the Camera has to be closed on\n> +\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> +\t\t * Error.\n> +\t\t */\n> +\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> +\t\tLOG(HAL, Fatal)\n> +\t\t\t<< \"Out-of-order completion for request \"\n> +\t\t\t<< utils::hex(request->cookie());\n>\n> -\t\tnode = descriptors_.extract(it);\n> +\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> +\t\tdescriptors_.pop();\n> +\t\treturn;\n>  \t}\n> -\tCamera3RequestDescriptor &descriptor = node.mapped();\n>\n>  \t/*\n>  \t * Prepare the capture result for the Android camera stack.\n> @@ -1113,9 +1117,9 @@ void CameraDevice::requestComplete(Request *request)\n>  \t * post-processing/compression fails.\n>  \t */\n>  \tcamera3_capture_result_t captureResult = {};\n> -\tcaptureResult.frame_number = descriptor.frameNumber_;\n> -\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\tcaptureResult.frame_number = descriptor->frameNumber_;\n> +\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> +\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>  \t\tCameraStream *cameraStream =\n>  \t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n>\n> @@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request *request)\n>  \t\tbuffer.release_fence = -1;\n>  \t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n>  \t}\n> -\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> +\tcaptureResult.output_buffers = descriptor->buffers_.data();\n>  \tcaptureResult.partial_result = 1;\n>\n>  \t/*\n> @@ -1147,11 +1151,11 @@ void CameraDevice::requestComplete(Request *request)\n>  \t\t\t\t<< \" not successfully completed: \"\n>  \t\t\t\t<< request->status();\n>\n> -\t\tnotifyError(descriptor.frameNumber_, nullptr,\n> +\t\tnotifyError(descriptor->frameNumber_, nullptr,\n>  \t\t\t    CAMERA3_MSG_ERROR_REQUEST);\n>\n>  \t\tcaptureResult.partial_result = 0;\n> -\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\t\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>  \t\t\t/*\n>  \t\t\t * Signal to the framework it has to handle fences that\n>  \t\t\t * have not been waited on by setting the release fence\n> @@ -1164,6 +1168,8 @@ void CameraDevice::requestComplete(Request *request)\n>\n>  \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>\n> +\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> +\t\tdescriptors_.pop();\n>  \t\treturn;\n>  \t}\n>\n> @@ -1175,10 +1181,10 @@ void CameraDevice::requestComplete(Request *request)\n>  \t */\n>  \tuint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()\n>  \t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n> -\tnotifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> +\tnotifyShutter(descriptor->frameNumber_, sensorTimestamp);\n>\n>  \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> -\t\t\t<< descriptor.buffers_.size() << \" streams\";\n> +\t\t\t<< descriptor->buffers_.size() << \" streams\";\n>\n>  \t/*\n>  \t * Generate the metadata associated with the captured buffers.\n> @@ -1186,16 +1192,16 @@ void CameraDevice::requestComplete(Request *request)\n>  \t * Notify if the metadata generation has failed, but continue processing\n>  \t * buffers and return an empty metadata pack.\n>  \t */\n> -\tstd::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);\n> +\tstd::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor);\n>  \tif (!resultMetadata) {\n> -\t\tnotifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> +\t\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n>\n>  \t\t/* The camera framework expects an empty metadata pack on error. */\n>  \t\tresultMetadata = std::make_unique<CameraMetadata>(0, 0);\n>  \t}\n>\n>  \t/* Handle post-processing. */\n> -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>  \t\tCameraStream *cameraStream =\n>  \t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n>\n> @@ -1206,13 +1212,13 @@ void CameraDevice::requestComplete(Request *request)\n>  \t\tif (!src) {\n>  \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n>  \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> +\t\t\tnotifyError(descriptor->frameNumber_, buffer.stream,\n>  \t\t\t\t    CAMERA3_MSG_ERROR_BUFFER);\n>  \t\t\tcontinue;\n>  \t\t}\n>\n>  \t\tint ret = cameraStream->process(*src, buffer,\n> -\t\t\t\t\t\tdescriptor.settings_,\n> +\t\t\t\t\t\tdescriptor->settings_,\n>  \t\t\t\t\t\tresultMetadata.get());\n>  \t\t/*\n>  \t\t * Return the FrameBuffer to the CameraStream now that we're\n> @@ -1223,13 +1229,16 @@ void CameraDevice::requestComplete(Request *request)\n>\n>  \t\tif (ret) {\n>  \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> +\t\t\tnotifyError(descriptor->frameNumber_, buffer.stream,\n>  \t\t\t\t    CAMERA3_MSG_ERROR_BUFFER);\n>  \t\t}\n>  \t}\n>\n>  \tcaptureResult.result = resultMetadata->get();\n>  \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\n> +\tMutexLocker descriptorsLock(descriptorsMutex_);\n> +\tdescriptors_.pop();\n>  }\n>\n>  std::string CameraDevice::logPrefix() const\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 43eb5895..9ec510d5 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -10,6 +10,7 @@\n>  #include <map>\n>  #include <memory>\n>  #include <mutex>\n> +#include <queue>\n>  #include <vector>\n>\n>  #include <hardware/camera3.h>\n> @@ -125,7 +126,7 @@ private:\n>  \tstd::vector<CameraStream> streams_;\n>\n>  \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> -\tstd::map<uint64_t, Camera3RequestDescriptor> descriptors_;\n> +\tstd::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;\n>\n>  \tstd::string maker_;\n>  \tstd::string model_;\n> --\n> 2.31.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 065D5BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 29 Sep 2021 18:39:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 609A9691AA;\n\tWed, 29 Sep 2021 20:39:26 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3136A69185\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 29 Sep 2021 20:39:25 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 95DE5100003;\n\tWed, 29 Sep 2021 18:39:24 +0000 (UTC)"],"Date":"Wed, 29 Sep 2021 20:40:11 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<20210929184011.gutadrufccmlhhaz@uno.localdomain>","References":"<20210929133030.401961-1-umang.jain@ideasonboard.com>\n\t<20210929133030.401961-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210929133030.401961-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 2/4] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]