[{"id":19896,"web_url":"https://patchwork.libcamera.org/comment/19896/","msgid":"<20210927203825.od72g5aeuddmy4kp@uno.localdomain>","date":"2021-09-27T20:38:25","subject":"Re: [libcamera-devel] [PATCH v1 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Mon, Sep 27, 2021 at 04:41:48PM +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> Since, the request is 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 is\n> extracted from the map and its lifetime is bound to requestComplete().\n> The lifetime is kept same by manually calling .pop_front() on the\n> queue. In the subsequent commit, this is likely to change with a\n> centralized location of dropping descriptors from the queue for request\n> completion.\n>\n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> ---\n>  src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------\n>  src/android/camera_device.h   |  5 +-\n>  2 files changed, 49 insertions(+), 47 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 8b5dd7a1..b0b7f4fd 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -955,7 +955,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<Camera3RequestDescriptor> descriptor =\n> +\t\tstd::make_unique<Camera3RequestDescriptor>(camera_.get(),\n> +\t\t\t\t\t\t\t   camera3Request);\n>\n>  \t/*\n>  \t * \\todo The Android request model is incremental, settings passed in\n> @@ -966,12 +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> -\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\nMany times I touched this file I wanted to insert an empty line here\nbut didn't dare not to receive an \"is this related?\" comment, but if\nyou have more guts than me, please go ahead. I surely won't complain :D\n\n\n> +\tfor (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {\n\nShould we also take the occasion to\n\n\tfor (auto &[i, camera3Buffer] : utils::enumerate(descriptor.buffers_)) {\n\n\n> +\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];\n\nand drop this ?\n\n>  \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n>  \t\tCameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);\n>\n> @@ -1003,12 +1005,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\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n>  \t\t\tbreak;\n>\n> @@ -1030,7 +1032,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> @@ -1038,7 +1040,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> @@ -1066,11 +1068,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_back(std::move(descriptor));\n>  \t}\n>\n>  \tworker_.queueRequest(request);\n> @@ -1080,31 +1082,27 @@ 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\nAccess to descriptor_ should be locked.\n\nCan't you\n\n\tstd::unique_ptr<Camera3RequestDescriptor> descriptor;\n\t{\n\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n\n\t\tif (descriptors_.empty())\n\t\t\treturn;\n\n\t\tdescriptor = std::move(descriptors_.front());\n\t\tdescriptors_.pop_front();\n\t}\n\nSo that you have descrptor managed by a unique_ptr here and you can\nsave the pop_front() at the end ?\n\n>\n> -\t\t\treturn;\n> -\t\t}\n> +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\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<< \"Unknown request: \" << request->cookie();\n>\n> -\t\tnode = descriptors_.extract(it);\n> +\t\treturn;\n>  \t}\n> -\tCamera3RequestDescriptor &descriptor = node.mapped();\n>\n>  \tcamera3_capture_result_t captureResult = {};\n> -\tcaptureResult.frame_number = descriptor.frameNumber_;\n> -\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> -\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> +\tcaptureResult.frame_number = descriptor->frameNumber_;\n> +\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> +\tcaptureResult.output_buffers = descriptor->buffers_.data();\n>\n>  \t/*\n>  \t * If the Request has failed, abort the request by notifying the error\n> @@ -1115,11 +1113,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\tCameraStream *cameraStream =\n>  \t\t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n>\n> @@ -1142,6 +1140,7 @@ void CameraDevice::requestComplete(Request *request)\n>  \t\t}\n>  \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>\n> +\t\tdescriptors_.pop_front();\n>  \t\treturn;\n>  \t}\n>\n> @@ -1153,10 +1152,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> @@ -1166,14 +1165,14 @@ void CameraDevice::requestComplete(Request *request)\n>  \t */\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> @@ -1184,13 +1183,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> @@ -1201,7 +1200,7 @@ 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> @@ -1210,7 +1209,7 @@ void CameraDevice::requestComplete(Request *request)\n>  \t * Finalize the capture result by setting fences and buffer status\n>  \t * before executing the callback.\n>  \t */\n> -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\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> @@ -1219,6 +1218,8 @@ void CameraDevice::requestComplete(Request *request)\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> @@ -1254,10 +1255,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n>   * Produce a set of fixed result metadata.\n>   */\n>  std::unique_ptr<CameraMetadata>\n> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n\nNot sure it's better to make a pointer here, or keep a reference and\ndereference the argument in the caller... A minor anyway...\n\n\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 43eb5895..5889a0e7 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 <deque>\n>  #include <map>\n>  #include <memory>\n>  #include <mutex>\n> @@ -105,7 +106,7 @@ private:\n>  \t\t\t camera3_error_msg_code code);\n>  \tint processControls(Camera3RequestDescriptor *descriptor);\n>  \tstd::unique_ptr<CameraMetadata> getResultMetadata(\n> -\t\tconst Camera3RequestDescriptor &descriptor) const;\n> +\t\tconst Camera3RequestDescriptor *descriptor) const;\n>\n>  \tunsigned int id_;\n>  \tcamera3_device_t camera3Device_;\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::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;\n>\n>  \tstd::string maker_;\n>  \tstd::string model_;\n> --\n> 2.31.1\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 7A2BDC3243\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 27 Sep 2021 20:37:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 061E26918B;\n\tMon, 27 Sep 2021 22:37:39 +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 08B0A6012C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Sep 2021 22:37:38 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 7D7F7100005;\n\tMon, 27 Sep 2021 20:37:37 +0000 (UTC)"],"Date":"Mon, 27 Sep 2021 22:38:25 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<20210927203825.od72g5aeuddmy4kp@uno.localdomain>","References":"<20210927111149.692004-1-umang.jain@ideasonboard.com>\n\t<20210927111149.692004-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927111149.692004-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<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>"}},{"id":19900,"web_url":"https://patchwork.libcamera.org/comment/19900/","msgid":"<YVJEfL7wU+i+bOca@pendragon.ideasonboard.com>","date":"2021-09-27T22:23:56","subject":"Re: [libcamera-devel] [PATCH v1 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Sep 27, 2021 at 10:38:25PM +0200, Jacopo Mondi wrote:\n> On Mon, Sep 27, 2021 at 04:41:48PM +0530, Umang Jain wrote:\n> > The descriptors_ map hold Camera3RequestDescriptor(s) which are\n\ns/hold/holds/\n\n> > per-capture request placed by the framework to libcamera HAL.\n\ns/request/requests/\n\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 request is placed in form of FIFO and the framework expects\n\ns/Since, the request is/Since the requests are/\n\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 is\n\ns/ is/ was/\n\n> > extracted from the map and its lifetime is bound to requestComplete().\n\ns/ is / was /\n\n> > The lifetime is kept same by manually calling .pop_front() on the\n> > queue. In the subsequent commit, this is likely to change with a\n> > centralized location of dropping descriptors from the queue for request\n> > completion.\n> >\n> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> > ---\n> >  src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------\n> >  src/android/camera_device.h   |  5 +-\n> >  2 files changed, 49 insertions(+), 47 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 8b5dd7a1..b0b7f4fd 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -955,7 +955,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<Camera3RequestDescriptor> descriptor =\n> > +\t\tstd::make_unique<Camera3RequestDescriptor>(camera_.get(),\n> > +\t\t\t\t\t\t\t   camera3Request);\n> >\n> >  \t/*\n> >  \t * \\todo The Android request model is incremental, settings passed in\n> > @@ -966,12 +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> > -\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> \n> Many times I touched this file I wanted to insert an empty line here\n> but didn't dare not to receive an \"is this related?\" comment, but if\n> you have more guts than me, please go ahead. I surely won't complain :D\n\nNeither will I :-)\n\n> > +\tfor (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {\n> \n> Should we also take the occasion to\n> \n> \tfor (auto &[i, camera3Buffer] : utils::enumerate(descriptor.buffers_)) {\n> \n> > +\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];\n> \n> and drop this ?\n\nFine with me.\n\n> >  \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> >  \t\tCameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);\n> >\n> > @@ -1003,12 +1005,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\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> >  \t\t\tbreak;\n> >\n> > @@ -1030,7 +1032,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> > @@ -1038,7 +1040,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> > @@ -1066,11 +1068,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_back(std::move(descriptor));\n> >  \t}\n> >\n> >  \tworker_.queueRequest(request);\n> > @@ -1080,31 +1082,27 @@ 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> Access to descriptor_ should be locked.\n> \n> Can't you\n> \n> \tstd::unique_ptr<Camera3RequestDescriptor> descriptor;\n> \t{\n> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> \n> \t\tif (descriptors_.empty())\n> \t\t\treturn;\n> \n> \t\tdescriptor = std::move(descriptors_.front());\n> \t\tdescriptors_.pop_front();\n> \t}\n> \n> So that you have descrptor managed by a unique_ptr here and you can\n> save the pop_front() at the end ?\n\nIt may conflict with patch 3/3. Another option is\n\n \tCamera3RequestDescriptor *descriptor;\n\n \t{\n \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n \t\tASSERT(!descriptors_.empty());\n \t\tdescriptor = descriptors_.front().get();\n \t}\n\nwith a pop at the end.\n\nI've used an ASSERT() here as it's really not supposed to happen, but\nmaybe an error message would be better ? In that case, you should call\nnotifyError(). I don't think we would be able to recover from this.\n\n> > -\t\t\treturn;\n> > -\t\t}\n> > +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\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<< \"Unknown request: \" << request->cookie();\n> >\n> > -\t\tnode = descriptors_.extract(it);\n> > +\t\treturn;\n> >  \t}\n> > -\tCamera3RequestDescriptor &descriptor = node.mapped();\n> >\n> >  \tcamera3_capture_result_t captureResult = {};\n> > -\tcaptureResult.frame_number = descriptor.frameNumber_;\n> > -\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> > -\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> > +\tcaptureResult.frame_number = descriptor->frameNumber_;\n> > +\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> > +\tcaptureResult.output_buffers = descriptor->buffers_.data();\n> >\n> >  \t/*\n> >  \t * If the Request has failed, abort the request by notifying the error\n> > @@ -1115,11 +1113,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\tCameraStream *cameraStream =\n> >  \t\t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n> >\n> > @@ -1142,6 +1140,7 @@ void CameraDevice::requestComplete(Request *request)\n> >  \t\t}\n> >  \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >\n> > +\t\tdescriptors_.pop_front();\n> >  \t\treturn;\n> >  \t}\n> >\n> > @@ -1153,10 +1152,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> > @@ -1166,14 +1165,14 @@ void CameraDevice::requestComplete(Request *request)\n> >  \t */\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> > @@ -1184,13 +1183,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> > @@ -1201,7 +1200,7 @@ 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> > @@ -1210,7 +1209,7 @@ void CameraDevice::requestComplete(Request *request)\n> >  \t * Finalize the capture result by setting fences and buffer status\n> >  \t * before executing the callback.\n> >  \t */\n> > -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\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> > @@ -1219,6 +1218,8 @@ void CameraDevice::requestComplete(Request *request)\n> >\n> >  \tcaptureResult.result = resultMetadata->get();\n> >  \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > +\n\nYou need\n\n\tMutexLocker descriptorsLock(descriptorsMutex_);\n\nhere.\n\n> > +\tdescriptors_.pop_front();\n> >  }\n> >\n> >  std::string CameraDevice::logPrefix() const\n> > @@ -1254,10 +1255,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> >   * Produce a set of fixed result metadata.\n> >   */\n> >  std::unique_ptr<CameraMetadata>\n> > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n> \n> Not sure it's better to make a pointer here, or keep a reference and\n> dereference the argument in the caller... A minor anyway...\n\nKeeping a reference would be better I think, as the argument can't be\nnull.\n\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 43eb5895..5889a0e7 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 <deque>\n> >  #include <map>\n> >  #include <memory>\n> >  #include <mutex>\n> > @@ -105,7 +106,7 @@ private:\n> >  \t\t\t camera3_error_msg_code code);\n> >  \tint processControls(Camera3RequestDescriptor *descriptor);\n> >  \tstd::unique_ptr<CameraMetadata> getResultMetadata(\n> > -\t\tconst Camera3RequestDescriptor &descriptor) const;\n> > +\t\tconst Camera3RequestDescriptor *descriptor) const;\n> >\n> >  \tunsigned int id_;\n> >  \tcamera3_device_t camera3Device_;\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::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;\n\nYou can use std::queue as it's a FIFO.\n\n> >\n> >  \tstd::string maker_;\n> >  \tstd::string model_;","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 179EFBDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 27 Sep 2021 22:24:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DB846918E;\n\tTue, 28 Sep 2021 00:24:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B29546012D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 28 Sep 2021 00:24: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 2A2A43F1;\n\tTue, 28 Sep 2021 00:24:03 +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=\"GcPO95/s\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632781443;\n\tbh=zAAmuWZNlLGRx2MQXm7E/zNNN5fHTOlIdamKDPYB5Cw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GcPO95/sZxQd0BP3Dr8hWEdMz81h5XXuIh4XkLU0g7oKuugRzW4uReWwBM/DFLFDf\n\t1Z7j47pNhnJf5fRt2eCF/W1XBaJF8VzBhAyPyK2KIyq36yjr+9kLcqJIGjU/HpzUw1\n\tmpIPwMBp9CZHBIW/Fuc+Pa5BMRkKncoK0PLH6mHI=","Date":"Tue, 28 Sep 2021 01:23:56 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YVJEfL7wU+i+bOca@pendragon.ideasonboard.com>","References":"<20210927111149.692004-1-umang.jain@ideasonboard.com>\n\t<20210927111149.692004-3-umang.jain@ideasonboard.com>\n\t<20210927203825.od72g5aeuddmy4kp@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927203825.od72g5aeuddmy4kp@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<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>"}},{"id":19926,"web_url":"https://patchwork.libcamera.org/comment/19926/","msgid":"<ebf335b3-3201-33e7-0a13-bfbf9e4a38f7@ideasonboard.com>","date":"2021-09-28T13:15:44","subject":"Re: [libcamera-devel] [PATCH v1 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/28/21 3:53 AM, Laurent Pinchart wrote:\n> Hello,\n>\n> On Mon, Sep 27, 2021 at 10:38:25PM +0200, Jacopo Mondi wrote:\n>> On Mon, Sep 27, 2021 at 04:41:48PM +0530, Umang Jain wrote:\n>>> The descriptors_ map hold Camera3RequestDescriptor(s) which are\n> s/hold/holds/\n>\n>>> per-capture request placed by the framework to libcamera HAL.\n> s/request/requests/\n>\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 request is placed in form of FIFO and the framework expects\n> s/Since, the request is/Since the requests are/\n>\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 is\n> s/ is/ was/\n>\n>>> extracted from the map and its lifetime is bound to requestComplete().\n> s/ is / was /\n>\n>>> The lifetime is kept same by manually calling .pop_front() on the\n>>> queue. In the subsequent commit, this is likely to change with a\n>>> centralized location of dropping descriptors from the queue for request\n>>> completion.\n>>>\n>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>>> ---\n>>>   src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------\n>>>   src/android/camera_device.h   |  5 +-\n>>>   2 files changed, 49 insertions(+), 47 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index 8b5dd7a1..b0b7f4fd 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -955,7 +955,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<Camera3RequestDescriptor> descriptor =\n>>> +\t\tstd::make_unique<Camera3RequestDescriptor>(camera_.get(),\n>>> +\t\t\t\t\t\t\t   camera3Request);\n>>>\n>>>   \t/*\n>>>   \t * \\todo The Android request model is incremental, settings passed in\n>>> @@ -966,12 +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>>> -\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>> Many times I touched this file I wanted to insert an empty line here\n>> but didn't dare not to receive an \"is this related?\" comment, but if\n>> you have more guts than me, please go ahead. I surely won't complain :D\n> Neither will I :-)\n>\n>>> +\tfor (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {\n>> Should we also take the occasion to\n>>\n>> \tfor (auto &[i, camera3Buffer] : utils::enumerate(descriptor.buffers_)) {\n>>\n>>> +\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];\n>> and drop this ?\n> Fine with me.\n>\n>>>   \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n>>>   \t\tCameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);\n>>>\n>>> @@ -1003,12 +1005,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\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n>>>   \t\t\tbreak;\n>>>\n>>> @@ -1030,7 +1032,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>>> @@ -1038,7 +1040,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>>> @@ -1066,11 +1068,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_back(std::move(descriptor));\n>>>   \t}\n>>>\n>>>   \tworker_.queueRequest(request);\n>>> @@ -1080,31 +1082,27 @@ 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>> Access to descriptor_ should be locked.\n>>\n>> Can't you\n>>\n>> \tstd::unique_ptr<Camera3RequestDescriptor> descriptor;\n>> \t{\n>> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>>\n>> \t\tif (descriptors_.empty())\n>> \t\t\treturn;\n>>\n>> \t\tdescriptor = std::move(descriptors_.front());\n>> \t\tdescriptors_.pop_front();\n>> \t}\n>>\n>> So that you have descrptor managed by a unique_ptr here and you can\n>> save the pop_front() at the end ?\n> It may conflict with patch 3/3. Another option is\n>\n>   \tCamera3RequestDescriptor *descriptor;\n>\n>   \t{\n>   \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>   \t\tASSERT(!descriptors_.empty());\n>   \t\tdescriptor = descriptors_.front().get();\n>   \t}\n>\n> with a pop at the end.\n\n\nYes, I am inclined towards popping the descriptor at the end\n\n>\n> I've used an ASSERT() here as it's really not supposed to happen, but\n> maybe an error message would be better ? In that case, you should call\n> notifyError(). I don't think we would be able to recover from this.\n\n\nIndeed, there was another suggestion on this in async post-processing \nseries, which I am inclining towards.\n\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\n\nWe are logging with Fatal currently, so it's similar to ASSERT with an \nerror log printed. I will keep the choice of Fatal over ASSERT(), should \nbe ok?\n\n>\n>>> -\t\t\treturn;\n>>> -\t\t}\n>>> +\tCamera3RequestDescriptor *descriptor = descriptors_.front().get();\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<< \"Unknown request: \" << request->cookie();\n>>>\n>>> -\t\tnode = descriptors_.extract(it);\n>>> +\t\treturn;\n>>>   \t}\n>>> -\tCamera3RequestDescriptor &descriptor = node.mapped();\n>>>\n>>>   \tcamera3_capture_result_t captureResult = {};\n>>> -\tcaptureResult.frame_number = descriptor.frameNumber_;\n>>> -\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n>>> -\tcaptureResult.output_buffers = descriptor.buffers_.data();\n>>> +\tcaptureResult.frame_number = descriptor->frameNumber_;\n>>> +\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n>>> +\tcaptureResult.output_buffers = descriptor->buffers_.data();\n>>>\n>>>   \t/*\n>>>   \t * If the Request has failed, abort the request by notifying the error\n>>> @@ -1115,11 +1113,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\tCameraStream *cameraStream =\n>>>   \t\t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n>>>\n>>> @@ -1142,6 +1140,7 @@ void CameraDevice::requestComplete(Request *request)\n>>>   \t\t}\n>>>   \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>>\n>>> +\t\tdescriptors_.pop_front();\n>>>   \t\treturn;\n>>>   \t}\n>>>\n>>> @@ -1153,10 +1152,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>>> @@ -1166,14 +1165,14 @@ void CameraDevice::requestComplete(Request *request)\n>>>   \t */\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>>> @@ -1184,13 +1183,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>>> @@ -1201,7 +1200,7 @@ 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>>> @@ -1210,7 +1209,7 @@ void CameraDevice::requestComplete(Request *request)\n>>>   \t * Finalize the capture result by setting fences and buffer status\n>>>   \t * before executing the callback.\n>>>   \t */\n>>> -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\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>>> @@ -1219,6 +1218,8 @@ void CameraDevice::requestComplete(Request *request)\n>>>\n>>>   \tcaptureResult.result = resultMetadata->get();\n>>>   \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>> +\n> You need\n>\n> \tMutexLocker descriptorsLock(descriptorsMutex_);\n>\n> here.\n>\n>>> +\tdescriptors_.pop_front();\n>>>   }\n>>>\n>>>   std::string CameraDevice::logPrefix() const\n>>> @@ -1254,10 +1255,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n>>>    * Produce a set of fixed result metadata.\n>>>    */\n>>>   std::unique_ptr<CameraMetadata>\n>>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n>>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const\n>> Not sure it's better to make a pointer here, or keep a reference and\n>> dereference the argument in the caller... A minor anyway...\n> Keeping a reference would be better I think, as the argument can't be\n> null.\n>\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 43eb5895..5889a0e7 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 <deque>\n>>>   #include <map>\n>>>   #include <memory>\n>>>   #include <mutex>\n>>> @@ -105,7 +106,7 @@ private:\n>>>   \t\t\t camera3_error_msg_code code);\n>>>   \tint processControls(Camera3RequestDescriptor *descriptor);\n>>>   \tstd::unique_ptr<CameraMetadata> getResultMetadata(\n>>> -\t\tconst Camera3RequestDescriptor &descriptor) const;\n>>> +\t\tconst Camera3RequestDescriptor *descriptor) const;\n>>>\n>>>   \tunsigned int id_;\n>>>   \tcamera3_device_t camera3Device_;\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::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;\n> You can use std::queue as it's a FIFO.\n\n\nAck,\n\n>\n>>>   \tstd::string maker_;\n>>>   \tstd::string model_;","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 CD44BBDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 28 Sep 2021 13:15:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 069866918C;\n\tTue, 28 Sep 2021 15:15:52 +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 CB15369185\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 28 Sep 2021 15:15:49 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9FFD33F1;\n\tTue, 28 Sep 2021 15:15:48 +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=\"ao1RYsOQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632834949;\n\tbh=mfVMIdCks+MolQl7HDwegZ7qG4oorHzr/7WBW1D84kg=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ao1RYsOQ7+Kc1OdNVbbV30lrb5Ie2IigOT9s7LhpJAcCrPsCe9rtcjTcW3Ve+qvEn\n\tzrmp3DkxzbdTllxE+O8e7lwlelrCu1NE1kLuBTWJQDSmggjnuJWxpRAtnk5BFHdEnT\n\tD/7NlDxbIz0/wBiAJYmtetYaLUhkfTx8NftXheA4=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tJacopo Mondi <jacopo@jmondi.org>","References":"<20210927111149.692004-1-umang.jain@ideasonboard.com>\n\t<20210927111149.692004-3-umang.jain@ideasonboard.com>\n\t<20210927203825.od72g5aeuddmy4kp@uno.localdomain>\n\t<YVJEfL7wU+i+bOca@pendragon.ideasonboard.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<ebf335b3-3201-33e7-0a13-bfbf9e4a38f7@ideasonboard.com>","Date":"Tue, 28 Sep 2021 18:45:44 +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":"<YVJEfL7wU+i+bOca@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] android: camera_device:\n\tTransform descriptors_ map to queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<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>"}}]