[{"id":32419,"web_url":"https://patchwork.libcamera.org/comment/32419/","msgid":"","date":"2024-11-28T09:00:59","subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:\n> From: Han-Lin Chen \n>\n> Allows pipeline handler to signal metadata completion by adding the\n> following signals to Camera:\n>\n> Signal metadataAvailable;\n>\n> Together with the bufferCompleted signal, the pipeline handler is allowed to\n> return buffers and partial metadata at any stage of processing.\n>\n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> ---\n> include/libcamera/camera.h | 1 +\n> include/libcamera/internal/pipeline_handler.h | 1 +\n> include/libcamera/internal/request.h | 4 ++\n> include/libcamera/request.h | 1 +\n> src/libcamera/camera.cpp | 6 +++\n> src/libcamera/pipeline_handler.cpp | 41 +++++++++++++++++++\n> src/libcamera/request.cpp | 21 ++++++++++\n> 7 files changed, 75 insertions(+)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 94cee7bd8..eb7cdf81b 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -122,6 +122,7 @@ public:\n>\n> \tconst std::string &id() const;\n>\n> +\tSignal metadataAvailable;\n> \tSignal bufferCompleted;\n> \tSignal requestCompleted;\n> \tSignal<> disconnected;\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index fb28a18d0..6c6cad66f 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -58,6 +58,7 @@ public:\n> \tvoid registerRequest(Request *request);\n> \tvoid queueRequest(Request *request);\n>\n> +\tvoid completeMetadata(Request *request, const ControlList &metadata);\n\nnit: \"complete\" seems to suggest we're done with it, while instead\nthe function should probably follow the signal name\n\"metadataAvailable()\"\n\n> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> \tvoid completeRequest(Request *request);\n> \tvoid cancelRequest(Request *request);\n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> index 4e7d05b1e..286cd9d76 100644\n> --- a/include/libcamera/internal/request.h\n> +++ b/include/libcamera/internal/request.h\n> @@ -43,6 +43,8 @@ public:\n> \tvoid prepare(std::chrono::milliseconds timeout = 0ms);\n> \tSignal<> prepared;\n>\n> +\tControlList addCompletedMetadata(const ControlList &metadata);\n> +\n> private:\n> \tfriend class PipelineHandler;\n> \tfriend std::ostream &operator<<(std::ostream &out, const Request &r);\n> @@ -60,6 +62,8 @@ private:\n> \tstd::unordered_set pending_;\n> \tstd::map> notifiers_;\n> \tstd::unique_ptr timer_;\n> +\n> +\tstd::unordered_set completedMetadata_;\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index e214a9d13..2c78d9bb4 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +#include \n\nNot necessary anymore\n\n>\n> #include \n> #include \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 7507e9dda..22484721a 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -892,6 +892,12 @@ const std::string &Camera::id() const\n> * completed\n> */\n>\n> +/**\n> + * \\var Camera::metadataAvailable\n> + * \\brief Signal emitted when some metadata for a request is available as a\n> + * partial result\n> + */\n> +\n> /**\n> * \\var Camera::requestCompleted\n> * \\brief Signal emitted when a request queued to the camera has completed\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 991b06f26..4ba96cfa2 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> \treturn request->_d()->completeBuffer(buffer);\n> }\n>\n> +/**\n> + * \\brief Complete part of metadata for a request\n> + * \\param[in] request The request the metadata belongs to\n> + * \\param[in] metadata The partial metadata that has completed\n> + *\n> + * This function could be called by pipeline handlers to signal availability of\n> + * \\a metadata before \\a request completes. Early metadata completion allows to\n> + * notify applications about the availability of a partial metadata buffer\n> + * before the associated Request has completed.\n> + *\n> + * A metadata key is expected to be completed at most once. If it's completed\n> + * more than once, the key will be dropped since the second time.\n\n* A metadata is expected to be completed at most once. If completed\n* more than once it will be ignored.\n\n> + *\n> + * \\context This function shall be called from the CameraManager thread.\n> + */\n> +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> +{\n> +\tconst ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> +\tif (!validMetadata.empty()) {\n> +\t\trequest->metadata().merge(validMetadata);\n> +\n> +\t\tCamera *camera = request->_d()->camera();\n> +\t\tcamera->metadataAvailable.emit(request, validMetadata);\n> +\t}\n> +}\n> +\n> /**\n> * \\brief Signal request completion\n> * \\param[in] request The request that has completed\n> @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n>\n> \tCamera::Private *data = camera->_d();\n\nDo not break the\n\n\tCamera::Private *data = camera->_d();\n\n\twhile (!data->queuedRequests_.empty()) {\n\nsequence\n\nMove the below hunk before the declaration of *data\n>\n> +\t/*\n> +\t * Collect metadata which is not yet completed by the Camera, and\n> +\t * create one partial result to cover the missing metadata before\n> +\t * completing the whole request. This guarantees the aggregation of\n> +\t * metadata in completed partial results equals to the global metadata\n> +\t * in the request.\n> +\t *\n> +\t * \\todo: Forbid merging metadata into request.metadata() directly and\n> +\t * force calling completeMetadata() to report metadata.\n\nThis seems to suggest all pipelines will go through partial metadata\ncompletion. I'm not 100% this will be the case.\n\n> +\t */\n> +\tconst ControlList validMetadata = request->_d()->addCompletedMetadata(\n> +\t\trequest->metadata());\n> +\tif (!validMetadata.empty())\n> +\t\tcamera->metadataAvailable.emit(request, validMetadata);\n> +\n> \twhile (!data->queuedRequests_.empty()) {\n> \t\tRequest *req = data->queuedRequests_.front();\n> \t\tif (req->status() == Request::RequestPending)\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 8c56ed30d..ae5cdeb19 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -178,6 +178,7 @@ void Request::Private::reset()\n> \tpending_.clear();\n> \tnotifiers_.clear();\n> \ttimer_.reset();\n> +\tcompletedMetadata_.clear();\n> }\n>\n> /*\n> @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)\n> * if they have failed preparing.\n> */\n>\n> +/**\n> + * \\brief Add completed metadata, as a partial result\n\nWhat about:\n\n * \\brief Accumulate metadata keys and compute a list of not yet notified metadata\n\n> + * \\param[in] metadata The metadata completed\n\nThe completed metadata list\n\n> + *\n> + * Request will record the entries that has been sent to the application, to\n> + * prevent duplicated controls.\n\nWhat about:\n\n * Accumulate the metadata keys from \\a metadata in an internal list of\n * completed metadata and compute a list of metadata that has not yet been\n * notified to the application.\n *\n * A metadata can only be notified once and gets ignored if completed multiple\n * times.\n\n> + *\n> + * \\return ControlList that hasn't been completed before\n\n\\return A ControlList of metadata that hasn't been notified to the\napplication yet\n\n> + */\n> +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> +{\n> +\tControlList resultMetadata;\n> +\tfor (auto &[id, value] : metadata) {\n> +\t\tif (!completedMetadata_.count(id))\n\nI might have missed where the id is added to completedMetadata_ ?\n\nThanks\n j\n\n> +\t\t\tresultMetadata.set(id, value);\n> +\t}\n> +\n> +\treturn resultMetadata;\n> +}\n> +\n> void Request::Private::notifierActivated(FrameBuffer *buffer)\n> {\n> \t/* Close the fence if successfully signalled. */\n> --\n> 2.47.0.338.g60cca15819-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5CBC4BD78E\n\tfor ;\n\tThu, 28 Nov 2024 09:01:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4517865FF2;\n\tThu, 28 Nov 2024 10:01:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1EFCE60531\n\tfor ;\n\tThu, 28 Nov 2024 10:01:03 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3AD29842;\n\tThu, 28 Nov 2024 10:00:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oEy7sHWn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732784439;\n\tbh=UfQFYbDno4u4LBJibTmr2DORqeJCcCOPkKfMz4C9RNs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oEy7sHWngzWEMRIihjQN3398cqjjPCfrqBjcEr/ZW3XvvryoaQo9W6FShRy8F0/4L\n\t8lBVZXpL4MiWpSS+gIACWyoxKLoxnVqvSlBXY03amTbjU4AtOQx/7RV/tgLM6Pf0eC\n\tKSXBAvZzFhYjlImker7HaBbzcFpe2udT9S9lDup4=","Date":"Thu, 28 Nov 2024 10:00:59 +0100","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","Message-ID":"","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-2-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241127092632.3145984-2-chenghaoyang@chromium.org>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32531,"web_url":"https://patchwork.libcamera.org/comment/32531/","msgid":"","date":"2024-12-05T05:22:32","subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nWill upload a new version in another series.\n\nOn Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:\n> > From: Han-Lin Chen \n> >\n> > Allows pipeline handler to signal metadata completion by adding the\n> > following signals to Camera:\n> >\n> > Signal metadataAvailable;\n> >\n> > Together with the bufferCompleted signal, the pipeline handler is allowed to\n> > return buffers and partial metadata at any stage of processing.\n> >\n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > ---\n> > include/libcamera/camera.h | 1 +\n> > include/libcamera/internal/pipeline_handler.h | 1 +\n> > include/libcamera/internal/request.h | 4 ++\n> > include/libcamera/request.h | 1 +\n> > src/libcamera/camera.cpp | 6 +++\n> > src/libcamera/pipeline_handler.cpp | 41 +++++++++++++++++++\n> > src/libcamera/request.cpp | 21 ++++++++++\n> > 7 files changed, 75 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 94cee7bd8..eb7cdf81b 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -122,6 +122,7 @@ public:\n> >\n> > const std::string &id() const;\n> >\n> > + Signal metadataAvailable;\n> > Signal bufferCompleted;\n> > Signal requestCompleted;\n> > Signal<> disconnected;\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index fb28a18d0..6c6cad66f 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -58,6 +58,7 @@ public:\n> > void registerRequest(Request *request);\n> > void queueRequest(Request *request);\n> >\n> > + void completeMetadata(Request *request, const ControlList &metadata);\n>\n> nit: \"complete\" seems to suggest we're done with it, while instead\n> the function should probably follow the signal name\n> \"metadataAvailable()\"\n\nDone\n\n>\n> > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > void completeRequest(Request *request);\n> > void cancelRequest(Request *request);\n> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > index 4e7d05b1e..286cd9d76 100644\n> > --- a/include/libcamera/internal/request.h\n> > +++ b/include/libcamera/internal/request.h\n> > @@ -43,6 +43,8 @@ public:\n> > void prepare(std::chrono::milliseconds timeout = 0ms);\n> > Signal<> prepared;\n> >\n> > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > +\n> > private:\n> > friend class PipelineHandler;\n> > friend std::ostream &operator<<(std::ostream &out, const Request &r);\n> > @@ -60,6 +62,8 @@ private:\n> > std::unordered_set pending_;\n> > std::map> notifiers_;\n> > std::unique_ptr timer_;\n> > +\n> > + std::unordered_set completedMetadata_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index e214a9d13..2c78d9bb4 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -12,6 +12,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n>\n> Not necessary anymore\n\nRemoved\n\n>\n> >\n> > #include \n> > #include \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 7507e9dda..22484721a 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -892,6 +892,12 @@ const std::string &Camera::id() const\n> > * completed\n> > */\n> >\n> > +/**\n> > + * \\var Camera::metadataAvailable\n> > + * \\brief Signal emitted when some metadata for a request is available as a\n> > + * partial result\n> > + */\n> > +\n> > /**\n> > * \\var Camera::requestCompleted\n> > * \\brief Signal emitted when a request queued to the camera has completed\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 991b06f26..4ba96cfa2 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> > return request->_d()->completeBuffer(buffer);\n> > }\n> >\n> > +/**\n> > + * \\brief Complete part of metadata for a request\n> > + * \\param[in] request The request the metadata belongs to\n> > + * \\param[in] metadata The partial metadata that has completed\n> > + *\n> > + * This function could be called by pipeline handlers to signal availability of\n> > + * \\a metadata before \\a request completes. Early metadata completion allows to\n> > + * notify applications about the availability of a partial metadata buffer\n> > + * before the associated Request has completed.\n> > + *\n> > + * A metadata key is expected to be completed at most once. If it's completed\n> > + * more than once, the key will be dropped since the second time.\n>\n> * A metadata is expected to be completed at most once. If completed\n> * more than once it will be ignored.\n\nUpdated.\n\n>\n> > + *\n> > + * \\context This function shall be called from the CameraManager thread.\n> > + */\n> > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > +{\n> > + const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > + if (!validMetadata.empty()) {\n> > + request->metadata().merge(validMetadata);\n> > +\n> > + Camera *camera = request->_d()->camera();\n> > + camera->metadataAvailable.emit(request, validMetadata);\n> > + }\n> > +}\n> > +\n> > /**\n> > * \\brief Signal request completion\n> > * \\param[in] request The request that has completed\n> > @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> >\n> > Camera::Private *data = camera->_d();\n>\n> Do not break the\n>\n> Camera::Private *data = camera->_d();\n>\n> while (!data->queuedRequests_.empty()) {\n>\n> sequence\n>\n> Move the below hunk before the declaration of *data\n\nDone\n\n> >\n> > + /*\n> > + * Collect metadata which is not yet completed by the Camera, and\n> > + * create one partial result to cover the missing metadata before\n> > + * completing the whole request. This guarantees the aggregation of\n> > + * metadata in completed partial results equals to the global metadata\n> > + * in the request.\n> > + *\n> > + * \\todo: Forbid merging metadata into request.metadata() directly and\n> > + * force calling completeMetadata() to report metadata.\n>\n> This seems to suggest all pipelines will go through partial metadata\n> completion. I'm not 100% this will be the case.\n\nLet me remove the todo then?\n\n>\n> > + */\n> > + const ControlList validMetadata = request->_d()->addCompletedMetadata(\n> > + request->metadata());\n> > + if (!validMetadata.empty())\n> > + camera->metadataAvailable.emit(request, validMetadata);\n> > +\n> > while (!data->queuedRequests_.empty()) {\n> > Request *req = data->queuedRequests_.front();\n> > if (req->status() == Request::RequestPending)\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 8c56ed30d..ae5cdeb19 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -178,6 +178,7 @@ void Request::Private::reset()\n> > pending_.clear();\n> > notifiers_.clear();\n> > timer_.reset();\n> > + completedMetadata_.clear();\n> > }\n> >\n> > /*\n> > @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)\n> > * if they have failed preparing.\n> > */\n> >\n> > +/**\n> > + * \\brief Add completed metadata, as a partial result\n>\n> What about:\n>\n> * \\brief Accumulate metadata keys and compute a list of not yet notified metadata\n\nThanks! Done.\n\n>\n> > + * \\param[in] metadata The metadata completed\n>\n> The completed metadata list\n\nUpdated.\n\n>\n> > + *\n> > + * Request will record the entries that has been sent to the application, to\n> > + * prevent duplicated controls.\n>\n> What about:\n>\n> * Accumulate the metadata keys from \\a metadata in an internal list of\n> * completed metadata and compute a list of metadata that has not yet been\n> * notified to the application.\n> *\n> * A metadata can only be notified once and gets ignored if completed multiple\n> * times.\n\nThanks! Done.\n\n>\n> > + *\n> > + * \\return ControlList that hasn't been completed before\n>\n> \\return A ControlList of metadata that hasn't been notified to the\n> application yet\n\nDone\n\n>\n> > + */\n> > +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > +{\n> > + ControlList resultMetadata;\n> > + for (auto &[id, value] : metadata) {\n> > + if (!completedMetadata_.count(id))\n>\n> I might have missed where the id is added to completedMetadata_ ?\n\nNah, I should've added it here.\nUpdated.\n\nActually, I'm considering if we should keep the values in\n`completedMetadata_`, and refresh `Request::metadata()` when calling\nsignal requestCompleted, to prevent the pipeline handler updating\nmetadata tags that have been sent to applications. This would ensure\nthe alignment of metadata between signal metadataAvailable and the\ncompleted metadata in Request.\nWDYT?\n\nBR,\nHarvey\n\n\n>\n> Thanks\n> j\n>\n> > + resultMetadata.set(id, value);\n> > + }\n> > +\n> > + return resultMetadata;\n> > +}\n> > +\n> > void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > {\n> > /* Close the fence if successfully signalled. */\n> > --\n> > 2.47.0.338.g60cca15819-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B636EC323E\n\tfor ;\n\tThu, 5 Dec 2024 05:22:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C323F660E1;\n\tThu, 5 Dec 2024 06:22:46 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1B74618AF\n\tfor ;\n\tThu, 5 Dec 2024 06:22:44 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id\n\t38308e7fff4ca-2ffdd9fc913so5013711fa.3\n\tfor ;\n\tWed, 04 Dec 2024 21:22:44 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"CSsA6+5G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733376164; x=1733980964;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=8hlUBp1GgOo6d6X2GD2EZ5NstU2tqpQSW/DJWRhPhgc=;\n\tb=CSsA6+5GAILM2AU5r78hRGo11lYCDo1mGaJq1C4AJ6pcueGAcLE/putVJ7DcHkAFx0\n\tGm/DNO3PdsOKekGjg0P9eZdYe89EonE9mB/Sc1taPqArlTWrCRLWexH0DGs9B7JO68CK\n\tO3vKEPROPHdhNwqbuFTxFx/Bu8iM6uYhbH7uY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733376164; x=1733980964;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=8hlUBp1GgOo6d6X2GD2EZ5NstU2tqpQSW/DJWRhPhgc=;\n\tb=Q9nOzFZwuQ75HVWkZwaoHRAGPYRhyLjByMZNkVAK05zvtd548qtnnnwMPCTf+8H4a+\n\t9q6azO0UsB7Yn4bnZ1x4GjdhJjx1B/p8KHkZBuqxPGfmkKfAGJimF0nVLXKyhjmop8xL\n\t8OjCB80fMoWlryCxsgV72v+2K840RHx7pD0AjZKcSWk25ECvw0WpDVkJ3vlr3LFFvaDV\n\t2fWloNFwJCo9oPr4vPeBaOi6gwvznLaGtbjey08dLKrJFlNi+jIK8143IbfRCqpJUC1E\n\tyHLFwseyLzAHftPPr3Tvy+FR+jpukeDb4RWUI9PUP1EyddThP5koxS+gO6aXpNQ1ifJP\n\tLKBw==","X-Gm-Message-State":"AOJu0Yz0w0yWrfllO+4vqhmpSh4xM56FamHIcUs/jstca5TejbsbHt6r\n\tdqpuPOIl5sTI+enZ/+9fzMcS7+aeAocua69HfvRl6FFypeGfPSyY1KU8HoAf/f35kH7BsQecr7P\n\tGq1/ALVtAqmYztUpWNa+gnQCmY20t4/SlbMVG","X-Gm-Gg":"ASbGnctsXwGF6wBKmLE1yFPw7gFsDl4ZRcUfdq1BcCMOZKn+1SG/ueOqE9liC2MT4Ld\n\th1jKr6t5GnN/pvJscqURqqQ42MRKWV+kMoX/w5GEppMAjpaEscMwNbB9bQQc=","X-Google-Smtp-Source":"AGHT+IF5MGm3aDOuHdnjl6BQWOL5hx2G6ZrmQNrYVq5SivQ0z/PZa3eDjvCq9dO5VoMHPuZgmgQZRmNAE/XlqrQZEF4=","X-Received":"by 2002:a05:651c:b1f:b0:2ff:ef38:6d66 with SMTP id\n\t38308e7fff4ca-30009c0cac3mr53656711fa.2.1733376163704;\n\tWed, 04 Dec 2024 21:22:43 -0800 (PST)","MIME-Version":"1.0","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-2-chenghaoyang@chromium.org>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 5 Dec 2024 13:22:32 +0800","Message-ID":"","Subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32532,"web_url":"https://patchwork.libcamera.org/comment/32532/","msgid":"<7wgvh2ukgfron75f3cjnrfmn6yzqec6z6f4avlnz7u6ltdhnbp@ghsdq2a4lbnb>","date":"2024-12-05T08:00:35","subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> Will upload a new version in another series.\n>\n> On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Harvey\n> >\n> > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:\n> > > From: Han-Lin Chen \n> > >\n> > > Allows pipeline handler to signal metadata completion by adding the\n> > > following signals to Camera:\n> > >\n> > > Signal metadataAvailable;\n> > >\n> > > Together with the bufferCompleted signal, the pipeline handler is allowed to\n> > > return buffers and partial metadata at any stage of processing.\n> > >\n> > > Signed-off-by: Han-Lin Chen \n> > > Co-developed-by: Harvey Yang \n> > > ---\n> > > include/libcamera/camera.h | 1 +\n> > > include/libcamera/internal/pipeline_handler.h | 1 +\n> > > include/libcamera/internal/request.h | 4 ++\n> > > include/libcamera/request.h | 1 +\n> > > src/libcamera/camera.cpp | 6 +++\n> > > src/libcamera/pipeline_handler.cpp | 41 +++++++++++++++++++\n> > > src/libcamera/request.cpp | 21 ++++++++++\n> > > 7 files changed, 75 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 94cee7bd8..eb7cdf81b 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -122,6 +122,7 @@ public:\n> > >\n> > > const std::string &id() const;\n> > >\n> > > + Signal metadataAvailable;\n> > > Signal bufferCompleted;\n> > > Signal requestCompleted;\n> > > Signal<> disconnected;\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index fb28a18d0..6c6cad66f 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -58,6 +58,7 @@ public:\n> > > void registerRequest(Request *request);\n> > > void queueRequest(Request *request);\n> > >\n> > > + void completeMetadata(Request *request, const ControlList &metadata);\n> >\n> > nit: \"complete\" seems to suggest we're done with it, while instead\n> > the function should probably follow the signal name\n> > \"metadataAvailable()\"\n>\n> Done\n>\n> >\n> > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > void completeRequest(Request *request);\n> > > void cancelRequest(Request *request);\n> > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > index 4e7d05b1e..286cd9d76 100644\n> > > --- a/include/libcamera/internal/request.h\n> > > +++ b/include/libcamera/internal/request.h\n> > > @@ -43,6 +43,8 @@ public:\n> > > void prepare(std::chrono::milliseconds timeout = 0ms);\n> > > Signal<> prepared;\n> > >\n> > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > +\n> > > private:\n> > > friend class PipelineHandler;\n> > > friend std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > @@ -60,6 +62,8 @@ private:\n> > > std::unordered_set pending_;\n> > > std::map> notifiers_;\n> > > std::unique_ptr timer_;\n> > > +\n> > > + std::unordered_set completedMetadata_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index e214a9d13..2c78d9bb4 100644\n> > > --- a/include/libcamera/request.h\n> > > +++ b/include/libcamera/request.h\n> > > @@ -12,6 +12,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> >\n> > Not necessary anymore\n>\n> Removed\n>\n> >\n> > >\n> > > #include \n> > > #include \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 7507e9dda..22484721a 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const\n> > > * completed\n> > > */\n> > >\n> > > +/**\n> > > + * \\var Camera::metadataAvailable\n> > > + * \\brief Signal emitted when some metadata for a request is available as a\n> > > + * partial result\n> > > + */\n> > > +\n> > > /**\n> > > * \\var Camera::requestCompleted\n> > > * \\brief Signal emitted when a request queued to the camera has completed\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 991b06f26..4ba96cfa2 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> > > return request->_d()->completeBuffer(buffer);\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Complete part of metadata for a request\n> > > + * \\param[in] request The request the metadata belongs to\n> > > + * \\param[in] metadata The partial metadata that has completed\n> > > + *\n> > > + * This function could be called by pipeline handlers to signal availability of\n> > > + * \\a metadata before \\a request completes. Early metadata completion allows to\n> > > + * notify applications about the availability of a partial metadata buffer\n> > > + * before the associated Request has completed.\n> > > + *\n> > > + * A metadata key is expected to be completed at most once. If it's completed\n> > > + * more than once, the key will be dropped since the second time.\n> >\n> > * A metadata is expected to be completed at most once. If completed\n> > * more than once it will be ignored.\n>\n> Updated.\n>\n> >\n> > > + *\n> > > + * \\context This function shall be called from the CameraManager thread.\n> > > + */\n> > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > +{\n> > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > + if (!validMetadata.empty()) {\n> > > + request->metadata().merge(validMetadata);\n> > > +\n> > > + Camera *camera = request->_d()->camera();\n> > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > + }\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Signal request completion\n> > > * \\param[in] request The request that has completed\n> > > @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > >\n> > > Camera::Private *data = camera->_d();\n> >\n> > Do not break the\n> >\n> > Camera::Private *data = camera->_d();\n> >\n> > while (!data->queuedRequests_.empty()) {\n> >\n> > sequence\n> >\n> > Move the below hunk before the declaration of *data\n>\n> Done\n>\n> > >\n> > > + /*\n> > > + * Collect metadata which is not yet completed by the Camera, and\n> > > + * create one partial result to cover the missing metadata before\n> > > + * completing the whole request. This guarantees the aggregation of\n> > > + * metadata in completed partial results equals to the global metadata\n> > > + * in the request.\n> > > + *\n> > > + * \\todo: Forbid merging metadata into request.metadata() directly and\n> > > + * force calling completeMetadata() to report metadata.\n> >\n> > This seems to suggest all pipelines will go through partial metadata\n> > completion. I'm not 100% this will be the case.\n>\n> Let me remove the todo then?\n>\n> >\n> > > + */\n> > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(\n> > > + request->metadata());\n> > > + if (!validMetadata.empty())\n> > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > +\n> > > while (!data->queuedRequests_.empty()) {\n> > > Request *req = data->queuedRequests_.front();\n> > > if (req->status() == Request::RequestPending)\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index 8c56ed30d..ae5cdeb19 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -178,6 +178,7 @@ void Request::Private::reset()\n> > > pending_.clear();\n> > > notifiers_.clear();\n> > > timer_.reset();\n> > > + completedMetadata_.clear();\n> > > }\n> > >\n> > > /*\n> > > @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)\n> > > * if they have failed preparing.\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Add completed metadata, as a partial result\n> >\n> > What about:\n> >\n> > * \\brief Accumulate metadata keys and compute a list of not yet notified metadata\n>\n> Thanks! Done.\n>\n> >\n> > > + * \\param[in] metadata The metadata completed\n> >\n> > The completed metadata list\n>\n> Updated.\n>\n> >\n> > > + *\n> > > + * Request will record the entries that has been sent to the application, to\n> > > + * prevent duplicated controls.\n> >\n> > What about:\n> >\n> > * Accumulate the metadata keys from \\a metadata in an internal list of\n> > * completed metadata and compute a list of metadata that has not yet been\n> > * notified to the application.\n> > *\n> > * A metadata can only be notified once and gets ignored if completed multiple\n> > * times.\n>\n> Thanks! Done.\n>\n> >\n> > > + *\n> > > + * \\return ControlList that hasn't been completed before\n> >\n> > \\return A ControlList of metadata that hasn't been notified to the\n> > application yet\n>\n> Done\n>\n> >\n> > > + */\n> > > +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > +{\n> > > + ControlList resultMetadata;\n> > > + for (auto &[id, value] : metadata) {\n> > > + if (!completedMetadata_.count(id))\n> >\n> > I might have missed where the id is added to completedMetadata_ ?\n>\n> Nah, I should've added it here.\n> Updated.\n>\n> Actually, I'm considering if we should keep the values in\n> `completedMetadata_`, and refresh `Request::metadata()` when calling\n> signal requestCompleted, to prevent the pipeline handler updating\n\nIs this an \"OR keep ids in completedMetadata_ OR accumulate them in\nRequest::metadata()\" ?\n\n> metadata tags that have been sent to applications. This would ensure\n> the alignment of metadata between signal metadataAvailable and the\n> completed metadata in Request.\n> WDYT?\n\nUnless there are reasons I am missing why this isn't possible, I think\nhaving the accumulated metadata in Request::metadata() is a good idea.\n\nEven more so as you're already doing it in\nPipelineHandler::completeMetadata()\n\n void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n {\n const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n if (!validMetadata.empty()) {\n request->metadata().merge(validMetadata);\n\nHence I think you can use Request::metadata_ to check what metadata\nhave already been completed with something like\n\nControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n{\n\tControlList resultMetadata;\n\tfor (auto &[id, value] : metadata) {\n\t\tif (!metadata_.contains(id) && !resultMetadata.count(id))\n\t\t\tresultMetadata.set(id, value);\n\t}\n\n metadata_.merge(resultMetadata);\n\n\treturn resultMetadata;\n}\n\nDo you think this could work ?\n\nThanks\n j\n\n>\n> BR,\n> Harvey\n>\n>\n> >\n> > Thanks\n> > j\n> >\n> > > + resultMetadata.set(id, value);\n> > > + }\n> > > +\n> > > + return resultMetadata;\n> > > +}\n> > > +\n> > > void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > > {\n> > > /* Close the fence if successfully signalled. */\n> > > --\n> > > 2.47.0.338.g60cca15819-goog\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E2DFCC323E\n\tfor ;\n\tThu, 5 Dec 2024 08:00:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2312E660E1;\n\tThu, 5 Dec 2024 09:00:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D7EB618B3\n\tfor ;\n\tThu, 5 Dec 2024 09:00:39 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:6462:5de2:459e:1ee6:26ea:2d31])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 970832B3;\n\tThu, 5 Dec 2024 09:00:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bO2rLUP0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733385610;\n\tbh=vbbtOUynV820aJgem3MTR5NgUSAMSy+OAsrFZZ9dfqk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bO2rLUP0Qp/+nia4gluZismycKbEX1IgHwa0Ncd5yvuBGysBcg+bIxp+vC4s/UFNe\n\tAaCYmUZOlktxheTM45llYEIDupRKfJIl2zNsIeZBjjzkl3c8VRmQRxP/jfylqyEVfH\n\tVNsZ4s1Sqx3H8k6XKkNWOp+S2SQaDFxtWmbrJFIE=","Date":"Thu, 5 Dec 2024 09:00:35 +0100","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","Message-ID":"<7wgvh2ukgfron75f3cjnrfmn6yzqec6z6f4avlnz7u6ltdhnbp@ghsdq2a4lbnb>","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-2-chenghaoyang@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32540,"web_url":"https://patchwork.libcamera.org/comment/32540/","msgid":"","date":"2024-12-05T12:46:52","subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > Will upload a new version in another series.\n> >\n> > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Harvey\n> > >\n> > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:\n> > > > From: Han-Lin Chen \n> > > >\n> > > > Allows pipeline handler to signal metadata completion by adding the\n> > > > following signals to Camera:\n> > > >\n> > > > Signal metadataAvailable;\n> > > >\n> > > > Together with the bufferCompleted signal, the pipeline handler is allowed to\n> > > > return buffers and partial metadata at any stage of processing.\n> > > >\n> > > > Signed-off-by: Han-Lin Chen \n> > > > Co-developed-by: Harvey Yang \n> > > > ---\n> > > > include/libcamera/camera.h | 1 +\n> > > > include/libcamera/internal/pipeline_handler.h | 1 +\n> > > > include/libcamera/internal/request.h | 4 ++\n> > > > include/libcamera/request.h | 1 +\n> > > > src/libcamera/camera.cpp | 6 +++\n> > > > src/libcamera/pipeline_handler.cpp | 41 +++++++++++++++++++\n> > > > src/libcamera/request.cpp | 21 ++++++++++\n> > > > 7 files changed, 75 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > index 94cee7bd8..eb7cdf81b 100644\n> > > > --- a/include/libcamera/camera.h\n> > > > +++ b/include/libcamera/camera.h\n> > > > @@ -122,6 +122,7 @@ public:\n> > > >\n> > > > const std::string &id() const;\n> > > >\n> > > > + Signal metadataAvailable;\n> > > > Signal bufferCompleted;\n> > > > Signal requestCompleted;\n> > > > Signal<> disconnected;\n> > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > index fb28a18d0..6c6cad66f 100644\n> > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > @@ -58,6 +58,7 @@ public:\n> > > > void registerRequest(Request *request);\n> > > > void queueRequest(Request *request);\n> > > >\n> > > > + void completeMetadata(Request *request, const ControlList &metadata);\n> > >\n> > > nit: \"complete\" seems to suggest we're done with it, while instead\n> > > the function should probably follow the signal name\n> > > \"metadataAvailable()\"\n> >\n> > Done\n> >\n> > >\n> > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > void completeRequest(Request *request);\n> > > > void cancelRequest(Request *request);\n> > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > > index 4e7d05b1e..286cd9d76 100644\n> > > > --- a/include/libcamera/internal/request.h\n> > > > +++ b/include/libcamera/internal/request.h\n> > > > @@ -43,6 +43,8 @@ public:\n> > > > void prepare(std::chrono::milliseconds timeout = 0ms);\n> > > > Signal<> prepared;\n> > > >\n> > > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > > +\n> > > > private:\n> > > > friend class PipelineHandler;\n> > > > friend std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > > @@ -60,6 +62,8 @@ private:\n> > > > std::unordered_set pending_;\n> > > > std::map> notifiers_;\n> > > > std::unique_ptr timer_;\n> > > > +\n> > > > + std::unordered_set completedMetadata_;\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > index e214a9d13..2c78d9bb4 100644\n> > > > --- a/include/libcamera/request.h\n> > > > +++ b/include/libcamera/request.h\n> > > > @@ -12,6 +12,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > >\n> > > Not necessary anymore\n> >\n> > Removed\n> >\n> > >\n> > > >\n> > > > #include \n> > > > #include \n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index 7507e9dda..22484721a 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const\n> > > > * completed\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\var Camera::metadataAvailable\n> > > > + * \\brief Signal emitted when some metadata for a request is available as a\n> > > > + * partial result\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\var Camera::requestCompleted\n> > > > * \\brief Signal emitted when a request queued to the camera has completed\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > index 991b06f26..4ba96cfa2 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> > > > return request->_d()->completeBuffer(buffer);\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Complete part of metadata for a request\n> > > > + * \\param[in] request The request the metadata belongs to\n> > > > + * \\param[in] metadata The partial metadata that has completed\n> > > > + *\n> > > > + * This function could be called by pipeline handlers to signal availability of\n> > > > + * \\a metadata before \\a request completes. Early metadata completion allows to\n> > > > + * notify applications about the availability of a partial metadata buffer\n> > > > + * before the associated Request has completed.\n> > > > + *\n> > > > + * A metadata key is expected to be completed at most once. If it's completed\n> > > > + * more than once, the key will be dropped since the second time.\n> > >\n> > > * A metadata is expected to be completed at most once. If completed\n> > > * more than once it will be ignored.\n> >\n> > Updated.\n> >\n> > >\n> > > > + *\n> > > > + * \\context This function shall be called from the CameraManager thread.\n> > > > + */\n> > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > > +{\n> > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > > + if (!validMetadata.empty()) {\n> > > > + request->metadata().merge(validMetadata);\n> > > > +\n> > > > + Camera *camera = request->_d()->camera();\n> > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > + }\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Signal request completion\n> > > > * \\param[in] request The request that has completed\n> > > > @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > > >\n> > > > Camera::Private *data = camera->_d();\n> > >\n> > > Do not break the\n> > >\n> > > Camera::Private *data = camera->_d();\n> > >\n> > > while (!data->queuedRequests_.empty()) {\n> > >\n> > > sequence\n> > >\n> > > Move the below hunk before the declaration of *data\n> >\n> > Done\n> >\n> > > >\n> > > > + /*\n> > > > + * Collect metadata which is not yet completed by the Camera, and\n> > > > + * create one partial result to cover the missing metadata before\n> > > > + * completing the whole request. This guarantees the aggregation of\n> > > > + * metadata in completed partial results equals to the global metadata\n> > > > + * in the request.\n> > > > + *\n> > > > + * \\todo: Forbid merging metadata into request.metadata() directly and\n> > > > + * force calling completeMetadata() to report metadata.\n> > >\n> > > This seems to suggest all pipelines will go through partial metadata\n> > > completion. I'm not 100% this will be the case.\n> >\n> > Let me remove the todo then?\n> >\n> > >\n> > > > + */\n> > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(\n> > > > + request->metadata());\n> > > > + if (!validMetadata.empty())\n> > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > +\n> > > > while (!data->queuedRequests_.empty()) {\n> > > > Request *req = data->queuedRequests_.front();\n> > > > if (req->status() == Request::RequestPending)\n> > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > index 8c56ed30d..ae5cdeb19 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -178,6 +178,7 @@ void Request::Private::reset()\n> > > > pending_.clear();\n> > > > notifiers_.clear();\n> > > > timer_.reset();\n> > > > + completedMetadata_.clear();\n> > > > }\n> > > >\n> > > > /*\n> > > > @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)\n> > > > * if they have failed preparing.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief Add completed metadata, as a partial result\n> > >\n> > > What about:\n> > >\n> > > * \\brief Accumulate metadata keys and compute a list of not yet notified metadata\n> >\n> > Thanks! Done.\n> >\n> > >\n> > > > + * \\param[in] metadata The metadata completed\n> > >\n> > > The completed metadata list\n> >\n> > Updated.\n> >\n> > >\n> > > > + *\n> > > > + * Request will record the entries that has been sent to the application, to\n> > > > + * prevent duplicated controls.\n> > >\n> > > What about:\n> > >\n> > > * Accumulate the metadata keys from \\a metadata in an internal list of\n> > > * completed metadata and compute a list of metadata that has not yet been\n> > > * notified to the application.\n> > > *\n> > > * A metadata can only be notified once and gets ignored if completed multiple\n> > > * times.\n> >\n> > Thanks! Done.\n> >\n> > >\n> > > > + *\n> > > > + * \\return ControlList that hasn't been completed before\n> > >\n> > > \\return A ControlList of metadata that hasn't been notified to the\n> > > application yet\n> >\n> > Done\n> >\n> > >\n> > > > + */\n> > > > +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > > +{\n> > > > + ControlList resultMetadata;\n> > > > + for (auto &[id, value] : metadata) {\n> > > > + if (!completedMetadata_.count(id))\n> > >\n> > > I might have missed where the id is added to completedMetadata_ ?\n> >\n> > Nah, I should've added it here.\n> > Updated.\n> >\n> > Actually, I'm considering if we should keep the values in\n> > `completedMetadata_`, and refresh `Request::metadata()` when calling\n> > signal requestCompleted, to prevent the pipeline handler updating\n>\n> Is this an \"OR keep ids in completedMetadata_ OR accumulate them in\n> Request::metadata()\" ?\n\nI meant keeping ids & values in `completedMetadata_`.\n\n>\n> > metadata tags that have been sent to applications. This would ensure\n> > the alignment of metadata between signal metadataAvailable and the\n> > completed metadata in Request.\n> > WDYT?\n>\n> Unless there are reasons I am missing why this isn't possible, I think\n> having the accumulated metadata in Request::metadata() is a good idea.\n>\n> Even more so as you're already doing it in\n> PipelineHandler::completeMetadata()\n>\n> void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> {\n> const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> if (!validMetadata.empty()) {\n> request->metadata().merge(validMetadata);\n>\n> Hence I think you can use Request::metadata_ to check what metadata\n> have already been completed with something like\n>\n> ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> {\n> ControlList resultMetadata;\n> for (auto &[id, value] : metadata) {\n> if (!metadata_.contains(id) && !resultMetadata.count(id))\n> resultMetadata.set(id, value);\n> }\n>\n> metadata_.merge(resultMetadata);\n>\n> return resultMetadata;\n> }\n>\n> Do you think this could work ?\n\nExactly, that's what I was thinking. Thanks!\nOnly a nit that we need:\n`metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`\n\nWill include this in the next version.\n\nBR,\nHarvey\n\n>\n> Thanks\n> j\n>\n> >\n> > BR,\n> > Harvey\n> >\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > + resultMetadata.set(id, value);\n> > > > + }\n> > > > +\n> > > > + return resultMetadata;\n> > > > +}\n> > > > +\n> > > > void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > > > {\n> > > > /* Close the fence if successfully signalled. */\n> > > > --\n> > > > 2.47.0.338.g60cca15819-goog\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6B1ADC323E\n\tfor ;\n\tThu, 5 Dec 2024 12:47:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0CDD6608C;\n\tThu, 5 Dec 2024 13:47:07 +0100 (CET)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 980516608C\n\tfor ;\n\tThu, 5 Dec 2024 13:47:04 +0100 (CET)","by mail-lj1-x22b.google.com with SMTP id\n\t38308e7fff4ca-2ffc1009a06so6545451fa.2\n\tfor ;\n\tThu, 05 Dec 2024 04:47:04 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"lq/nx7/t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733402824; x=1734007624;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=bT6edA9L/yqlMmpe6pKPiqewf4Bi3T/MrG6aK6emrgk=;\n\tb=lq/nx7/tmTrK8pACwqyQrc5L13KqzUECJ/XesAk0j05ALjmD5pfN8fM8CTTxeGmPcq\n\tInNPrwjQl07DGBSqp4T9gm6CaQj6jfyqz1L3iVw2k4GphPuubbFzD/aOIG1OfVPS034L\n\tiRzeUPr4LQBeXbMiUxn1GuWN3iySxMijwhWCE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733402824; x=1734007624;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=bT6edA9L/yqlMmpe6pKPiqewf4Bi3T/MrG6aK6emrgk=;\n\tb=gaskXjRL+JvCc1nQlfA4P2K+tYwPed3bc5NgbD/4SEtRv3nP+p4mKM7Ox3xgc1v9gH\n\tO1Tc127lexkTIZbVFTKcxdqYz4geQUSDSSEQxihYG8yGg5F7oXrwZt+mIafYIUeO1Dh7\n\tEFp7ZnUWBbRPLbqcIGIrqk26m09x0V5LaUzfliwSH9FLm23eGMPD1XNdhOGyAgFqWQN+\n\t7qcu4iujK369lvzx6IjH42A5e9rQSrsRppM0vnFospdHmamPyBUbvIjOzwJ8yMwrbv66\n\t1x/PeEPS4SFmuvoEdLtRJQ8/I6BdGBLJ0cKJfd/Fio5p/uj4i105znxQRVySQ+XsV/8y\n\t0opg==","X-Gm-Message-State":"AOJu0YydQY7401tsOpp3dJPzUU5UGK//evJvpMZdZ/OOBQZXgWsnqs3d\n\tck+xZcgZjtIXV96RxWSQziyBCu0Z3UnRyc/n9VryLiDtIaUCvG2yFQy492+RohpfwimKYIItAa0\n\trvftVJFmugG/DpjpZn+eVU/GhjOLVUHo32+IQ/a8VVpOMN7zE0xXN","X-Gm-Gg":"ASbGnct1SrZqpcdwAwjfGWZcQlvaryeJqWBladJspzXYDvgVAMXQohi+0OCYtt5u7j4\n\tX90Yhi/EQ/rFuDxgqaqWQcM60WnpU+TPBNZytlcsIaF6Tb5OvayPkitfc+Q7KBvorQw==","X-Google-Smtp-Source":"AGHT+IEMPiMfbI2MBFopf/hJFUM93ZWWa2r98wM+HfGzD6gzs+GPduwaBA5varAWqQDCbZUXPj7CLRBqwAeQ4MxvvJo=","X-Received":"by 2002:a05:651c:1988:b0:300:2524:84f2 with SMTP id\n\t38308e7fff4ca-300252487b1mr6517911fa.2.1733402823734; Thu, 05 Dec 2024\n\t04:47:03 -0800 (PST)","MIME-Version":"1.0","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-2-chenghaoyang@chromium.org>\n\t\n\t\n\t<7wgvh2ukgfron75f3cjnrfmn6yzqec6z6f4avlnz7u6ltdhnbp@ghsdq2a4lbnb>","In-Reply-To":"<7wgvh2ukgfron75f3cjnrfmn6yzqec6z6f4avlnz7u6ltdhnbp@ghsdq2a4lbnb>","From":"Cheng-Hao Yang ","Date":"Thu, 5 Dec 2024 20:46:52 +0800","Message-ID":"","Subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32544,"web_url":"https://patchwork.libcamera.org/comment/32544/","msgid":"","date":"2024-12-05T14:57:48","subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Dec 5, 2024 at 8:46 PM Cheng-Hao Yang wrote:\n>\n> Hi Jacopo,\n>\n> On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Harvey\n> >\n> > On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:\n> > > Hi Jacopo,\n> > >\n> > > Will upload a new version in another series.\n> > >\n> > > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi Harvey\n> > > >\n> > > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:\n> > > > > From: Han-Lin Chen \n> > > > >\n> > > > > Allows pipeline handler to signal metadata completion by adding the\n> > > > > following signals to Camera:\n> > > > >\n> > > > > Signal metadataAvailable;\n> > > > >\n> > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to\n> > > > > return buffers and partial metadata at any stage of processing.\n> > > > >\n> > > > > Signed-off-by: Han-Lin Chen \n> > > > > Co-developed-by: Harvey Yang \n> > > > > ---\n> > > > > include/libcamera/camera.h | 1 +\n> > > > > include/libcamera/internal/pipeline_handler.h | 1 +\n> > > > > include/libcamera/internal/request.h | 4 ++\n> > > > > include/libcamera/request.h | 1 +\n> > > > > src/libcamera/camera.cpp | 6 +++\n> > > > > src/libcamera/pipeline_handler.cpp | 41 +++++++++++++++++++\n> > > > > src/libcamera/request.cpp | 21 ++++++++++\n> > > > > 7 files changed, 75 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > > index 94cee7bd8..eb7cdf81b 100644\n> > > > > --- a/include/libcamera/camera.h\n> > > > > +++ b/include/libcamera/camera.h\n> > > > > @@ -122,6 +122,7 @@ public:\n> > > > >\n> > > > > const std::string &id() const;\n> > > > >\n> > > > > + Signal metadataAvailable;\n> > > > > Signal bufferCompleted;\n> > > > > Signal requestCompleted;\n> > > > > Signal<> disconnected;\n> > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > index fb28a18d0..6c6cad66f 100644\n> > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > @@ -58,6 +58,7 @@ public:\n> > > > > void registerRequest(Request *request);\n> > > > > void queueRequest(Request *request);\n> > > > >\n> > > > > + void completeMetadata(Request *request, const ControlList &metadata);\n> > > >\n> > > > nit: \"complete\" seems to suggest we're done with it, while instead\n> > > > the function should probably follow the signal name\n> > > > \"metadataAvailable()\"\n> > >\n> > > Done\n> > >\n> > > >\n> > > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > > void completeRequest(Request *request);\n> > > > > void cancelRequest(Request *request);\n> > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > > > index 4e7d05b1e..286cd9d76 100644\n> > > > > --- a/include/libcamera/internal/request.h\n> > > > > +++ b/include/libcamera/internal/request.h\n> > > > > @@ -43,6 +43,8 @@ public:\n> > > > > void prepare(std::chrono::milliseconds timeout = 0ms);\n> > > > > Signal<> prepared;\n> > > > >\n> > > > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > > > +\n> > > > > private:\n> > > > > friend class PipelineHandler;\n> > > > > friend std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > > > @@ -60,6 +62,8 @@ private:\n> > > > > std::unordered_set pending_;\n> > > > > std::map> notifiers_;\n> > > > > std::unique_ptr timer_;\n> > > > > +\n> > > > > + std::unordered_set completedMetadata_;\n> > > > > };\n> > > > >\n> > > > > } /* namespace libcamera */\n> > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > > index e214a9d13..2c78d9bb4 100644\n> > > > > --- a/include/libcamera/request.h\n> > > > > +++ b/include/libcamera/request.h\n> > > > > @@ -12,6 +12,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > >\n> > > > Not necessary anymore\n> > >\n> > > Removed\n> > >\n> > > >\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index 7507e9dda..22484721a 100644\n> > > > > --- a/src/libcamera/camera.cpp\n> > > > > +++ b/src/libcamera/camera.cpp\n> > > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const\n> > > > > * completed\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\var Camera::metadataAvailable\n> > > > > + * \\brief Signal emitted when some metadata for a request is available as a\n> > > > > + * partial result\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\var Camera::requestCompleted\n> > > > > * \\brief Signal emitted when a request queued to the camera has completed\n> > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > index 991b06f26..4ba96cfa2 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> > > > > return request->_d()->completeBuffer(buffer);\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Complete part of metadata for a request\n> > > > > + * \\param[in] request The request the metadata belongs to\n> > > > > + * \\param[in] metadata The partial metadata that has completed\n> > > > > + *\n> > > > > + * This function could be called by pipeline handlers to signal availability of\n> > > > > + * \\a metadata before \\a request completes. Early metadata completion allows to\n> > > > > + * notify applications about the availability of a partial metadata buffer\n> > > > > + * before the associated Request has completed.\n> > > > > + *\n> > > > > + * A metadata key is expected to be completed at most once. If it's completed\n> > > > > + * more than once, the key will be dropped since the second time.\n> > > >\n> > > > * A metadata is expected to be completed at most once. If completed\n> > > > * more than once it will be ignored.\n> > >\n> > > Updated.\n> > >\n> > > >\n> > > > > + *\n> > > > > + * \\context This function shall be called from the CameraManager thread.\n> > > > > + */\n> > > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > > > +{\n> > > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > > > + if (!validMetadata.empty()) {\n> > > > > + request->metadata().merge(validMetadata);\n> > > > > +\n> > > > > + Camera *camera = request->_d()->camera();\n> > > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Signal request completion\n> > > > > * \\param[in] request The request that has completed\n> > > > > @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > >\n> > > > > Camera::Private *data = camera->_d();\n> > > >\n> > > > Do not break the\n> > > >\n> > > > Camera::Private *data = camera->_d();\n> > > >\n> > > > while (!data->queuedRequests_.empty()) {\n> > > >\n> > > > sequence\n> > > >\n> > > > Move the below hunk before the declaration of *data\n> > >\n> > > Done\n> > >\n> > > > >\n> > > > > + /*\n> > > > > + * Collect metadata which is not yet completed by the Camera, and\n> > > > > + * create one partial result to cover the missing metadata before\n> > > > > + * completing the whole request. This guarantees the aggregation of\n> > > > > + * metadata in completed partial results equals to the global metadata\n> > > > > + * in the request.\n> > > > > + *\n> > > > > + * \\todo: Forbid merging metadata into request.metadata() directly and\n> > > > > + * force calling completeMetadata() to report metadata.\n> > > >\n> > > > This seems to suggest all pipelines will go through partial metadata\n> > > > completion. I'm not 100% this will be the case.\n> > >\n> > > Let me remove the todo then?\n> > >\n> > > >\n> > > > > + */\n> > > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(\n> > > > > + request->metadata());\n> > > > > + if (!validMetadata.empty())\n> > > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > > +\n> > > > > while (!data->queuedRequests_.empty()) {\n> > > > > Request *req = data->queuedRequests_.front();\n> > > > > if (req->status() == Request::RequestPending)\n> > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > index 8c56ed30d..ae5cdeb19 100644\n> > > > > --- a/src/libcamera/request.cpp\n> > > > > +++ b/src/libcamera/request.cpp\n> > > > > @@ -178,6 +178,7 @@ void Request::Private::reset()\n> > > > > pending_.clear();\n> > > > > notifiers_.clear();\n> > > > > timer_.reset();\n> > > > > + completedMetadata_.clear();\n> > > > > }\n> > > > >\n> > > > > /*\n> > > > > @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)\n> > > > > * if they have failed preparing.\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Add completed metadata, as a partial result\n> > > >\n> > > > What about:\n> > > >\n> > > > * \\brief Accumulate metadata keys and compute a list of not yet notified metadata\n> > >\n> > > Thanks! Done.\n> > >\n> > > >\n> > > > > + * \\param[in] metadata The metadata completed\n> > > >\n> > > > The completed metadata list\n> > >\n> > > Updated.\n> > >\n> > > >\n> > > > > + *\n> > > > > + * Request will record the entries that has been sent to the application, to\n> > > > > + * prevent duplicated controls.\n> > > >\n> > > > What about:\n> > > >\n> > > > * Accumulate the metadata keys from \\a metadata in an internal list of\n> > > > * completed metadata and compute a list of metadata that has not yet been\n> > > > * notified to the application.\n> > > > *\n> > > > * A metadata can only be notified once and gets ignored if completed multiple\n> > > > * times.\n> > >\n> > > Thanks! Done.\n> > >\n> > > >\n> > > > > + *\n> > > > > + * \\return ControlList that hasn't been completed before\n> > > >\n> > > > \\return A ControlList of metadata that hasn't been notified to the\n> > > > application yet\n> > >\n> > > Done\n> > >\n> > > >\n> > > > > + */\n> > > > > +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > > > +{\n> > > > > + ControlList resultMetadata;\n> > > > > + for (auto &[id, value] : metadata) {\n> > > > > + if (!completedMetadata_.count(id))\n> > > >\n> > > > I might have missed where the id is added to completedMetadata_ ?\n> > >\n> > > Nah, I should've added it here.\n> > > Updated.\n> > >\n> > > Actually, I'm considering if we should keep the values in\n> > > `completedMetadata_`, and refresh `Request::metadata()` when calling\n> > > signal requestCompleted, to prevent the pipeline handler updating\n> >\n> > Is this an \"OR keep ids in completedMetadata_ OR accumulate them in\n> > Request::metadata()\" ?\n>\n> I meant keeping ids & values in `completedMetadata_`.\n>\n> >\n> > > metadata tags that have been sent to applications. This would ensure\n> > > the alignment of metadata between signal metadataAvailable and the\n> > > completed metadata in Request.\n> > > WDYT?\n> >\n> > Unless there are reasons I am missing why this isn't possible, I think\n> > having the accumulated metadata in Request::metadata() is a good idea.\n> >\n> > Even more so as you're already doing it in\n> > PipelineHandler::completeMetadata()\n> >\n> > void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > {\n> > const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > if (!validMetadata.empty()) {\n> > request->metadata().merge(validMetadata);\n> >\n> > Hence I think you can use Request::metadata_ to check what metadata\n> > have already been completed with something like\n> >\n> > ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > {\n> > ControlList resultMetadata;\n> > for (auto &[id, value] : metadata) {\n> > if (!metadata_.contains(id) && !resultMetadata.count(id))\n> > resultMetadata.set(id, value);\n> > }\n> >\n> > metadata_.merge(resultMetadata);\n> >\n> > return resultMetadata;\n> > }\n> >\n> > Do you think this could work ?\n>\n> Exactly, that's what I was thinking. Thanks!\n> Only a nit that we need:\n> `metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`\n>\n> Will include this in the next version.\n\nSorry, I just found that this may result in performance issues, when\nwe overwrite metadata values that are large in size repetitively. How\nabout we overwrite it once in `PipelineHandler::requestComplete`, and\nexpect applications wouldn't use `Request::metadata()` directly before\nsignal requestCompleted is invoked?\n\nBR,\nHarvey\n\n>\n> BR,\n> Harvey\n>\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > BR,\n> > > Harvey\n> > >\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > + resultMetadata.set(id, value);\n> > > > > + }\n> > > > > +\n> > > > > + return resultMetadata;\n> > > > > +}\n> > > > > +\n> > > > > void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > > > > {\n> > > > > /* Close the fence if successfully signalled. */\n> > > > > --\n> > > > > 2.47.0.338.g60cca15819-goog\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7ACFEC323E\n\tfor ;\n\tThu, 5 Dec 2024 14:58:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71349660FD;\n\tThu, 5 Dec 2024 15:58:01 +0100 (CET)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 123F96608C\n\tfor ;\n\tThu, 5 Dec 2024 15:58:00 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id\n\t38308e7fff4ca-2ffbf4580cbso10370751fa.2\n\tfor ;\n\tThu, 05 Dec 2024 06:58:00 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"H3kS6dTk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733410679; x=1734015479;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=+UYA2ZAJdxcGGNpkEJI2oBezE9nTfz16V+CeAjoqdFo=;\n\tb=H3kS6dTkPcfqnOMU41lNZfxN5lUIvwqaAMv1vw/xRG5PWobsD9hEc2iSr9hozc80d7\n\trD/ne5bSRKEgVhzDyUiEXtMoLAiy3pMcZk15wVj9oW6Y/UWjtsE9BCzd5E5SPvyd/L1a\n\tNgDE6LsSA2M0Xs3g69SEgKhhEHNv0XD00pcg0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733410679; x=1734015479;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=+UYA2ZAJdxcGGNpkEJI2oBezE9nTfz16V+CeAjoqdFo=;\n\tb=pzaMWl6KYkZhzg7G4kptI3PXpHs2h08Bz+BtcRYqtv5PSdjBxhkvG2t4LQcbISpNZ3\n\tKlMF8X427lXGRyhG3YQc6Oi9emfXsLrt67MKKSDrQP54HYVoSCUlykywpXxLoKxprwxN\n\tnHYnHP1GJMmo/CYcx4UXyChGWxa548Wvo2TpqbbKmdaGXKpUNq1WDwF3hiQk+EPQnERt\n\tLMuPREyzpJNMK5MHFNAxiqi7/bhDF5+mTElREOSyQtE+VFEm1OfFfsm02+jyW5ABxRTg\n\tEaaJpbJVHDitS4WFkzFXN6WgjCkTGosRj6NBc1zY/dGo77Kb7WoI6L4KdBI8Mk0RbBd1\n\tqHvw==","X-Gm-Message-State":"AOJu0YwYEkMqg8FUs2adJRtSoSTzA4SDhpu27cTiHAxx1pivtGlu+TNj\n\tpufu4ZJj+B0c9CT8bcBfhfNg41Dwpt6fb0A7HtKP3PrZpbdCMAunTeK8TrdJ7pJeIndOV3jKZrS\n\t8y3vXAWj2nu9Z3M9soM8pspPQo7/sLBeRknZ9ZIru0mcmvBzPGw==","X-Gm-Gg":"ASbGnctPT/KHe1Vxc73kv60ILvRF8XYutQpVZvckCpImvIvLBrnZcxfWctwGNpo/83N\n\tfWoWNru3ICRd/W0ng6amPaZtAYThSY3O18uTKd+RsoVqx9Nrj5JhoU6VRDpM/MHKYfw==","X-Google-Smtp-Source":"AGHT+IH+C4bAul6VGGKDoGPVqqz3WzCDodrIxzTW10RqCdRpMWLe54wpiJiQ+5rQALFm72/aNKomdRYD9MA0JBfJSoU=","X-Received":"by 2002:a05:651c:1987:b0:300:1d00:8fcf with SMTP id\n\t38308e7fff4ca-3001d00959cmr26596441fa.36.1733410679122;\n\tThu, 05 Dec 2024 06:57:59 -0800 (PST)","MIME-Version":"1.0","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-2-chenghaoyang@chromium.org>\n\t\n\t\n\t<7wgvh2ukgfron75f3cjnrfmn6yzqec6z6f4avlnz7u6ltdhnbp@ghsdq2a4lbnb>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 5 Dec 2024 22:57:48 +0800","Message-ID":"","Subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32545,"web_url":"https://patchwork.libcamera.org/comment/32545/","msgid":"<27xrxiauhjwjhzntw3tbdrguzsbzjqtcfeh2xd2naiqcurhnlj@6ntery3oesby>","date":"2024-12-05T15:21:48","subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Thu, Dec 05, 2024 at 10:57:48PM +0800, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> On Thu, Dec 5, 2024 at 8:46 PM Cheng-Hao Yang wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Harvey\n> > >\n> > > On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Will upload a new version in another series.\n> > > >\n> > > > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi\n> > > > wrote:\n> > > > >\n> > > > > Hi Harvey\n> > > > >\n> > > > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:\n> > > > > > From: Han-Lin Chen \n> > > > > >\n> > > > > > Allows pipeline handler to signal metadata completion by adding the\n> > > > > > following signals to Camera:\n> > > > > >\n> > > > > > Signal metadataAvailable;\n> > > > > >\n> > > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to\n> > > > > > return buffers and partial metadata at any stage of processing.\n> > > > > >\n> > > > > > Signed-off-by: Han-Lin Chen \n> > > > > > Co-developed-by: Harvey Yang \n> > > > > > ---\n> > > > > > include/libcamera/camera.h | 1 +\n> > > > > > include/libcamera/internal/pipeline_handler.h | 1 +\n> > > > > > include/libcamera/internal/request.h | 4 ++\n> > > > > > include/libcamera/request.h | 1 +\n> > > > > > src/libcamera/camera.cpp | 6 +++\n> > > > > > src/libcamera/pipeline_handler.cpp | 41 +++++++++++++++++++\n> > > > > > src/libcamera/request.cpp | 21 ++++++++++\n> > > > > > 7 files changed, 75 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > > > index 94cee7bd8..eb7cdf81b 100644\n> > > > > > --- a/include/libcamera/camera.h\n> > > > > > +++ b/include/libcamera/camera.h\n> > > > > > @@ -122,6 +122,7 @@ public:\n> > > > > >\n> > > > > > const std::string &id() const;\n> > > > > >\n> > > > > > + Signal metadataAvailable;\n> > > > > > Signal bufferCompleted;\n> > > > > > Signal requestCompleted;\n> > > > > > Signal<> disconnected;\n> > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > > index fb28a18d0..6c6cad66f 100644\n> > > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > > @@ -58,6 +58,7 @@ public:\n> > > > > > void registerRequest(Request *request);\n> > > > > > void queueRequest(Request *request);\n> > > > > >\n> > > > > > + void completeMetadata(Request *request, const ControlList &metadata);\n> > > > >\n> > > > > nit: \"complete\" seems to suggest we're done with it, while instead\n> > > > > the function should probably follow the signal name\n> > > > > \"metadataAvailable()\"\n> > > >\n> > > > Done\n> > > >\n> > > > >\n> > > > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > > > void completeRequest(Request *request);\n> > > > > > void cancelRequest(Request *request);\n> > > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > > > > index 4e7d05b1e..286cd9d76 100644\n> > > > > > --- a/include/libcamera/internal/request.h\n> > > > > > +++ b/include/libcamera/internal/request.h\n> > > > > > @@ -43,6 +43,8 @@ public:\n> > > > > > void prepare(std::chrono::milliseconds timeout = 0ms);\n> > > > > > Signal<> prepared;\n> > > > > >\n> > > > > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > > > > +\n> > > > > > private:\n> > > > > > friend class PipelineHandler;\n> > > > > > friend std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > > > > @@ -60,6 +62,8 @@ private:\n> > > > > > std::unordered_set pending_;\n> > > > > > std::map> notifiers_;\n> > > > > > std::unique_ptr timer_;\n> > > > > > +\n> > > > > > + std::unordered_set completedMetadata_;\n> > > > > > };\n> > > > > >\n> > > > > > } /* namespace libcamera */\n> > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > > > index e214a9d13..2c78d9bb4 100644\n> > > > > > --- a/include/libcamera/request.h\n> > > > > > +++ b/include/libcamera/request.h\n> > > > > > @@ -12,6 +12,7 @@\n> > > > > > #include \n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > >\n> > > > > Not necessary anymore\n> > > >\n> > > > Removed\n> > > >\n> > > > >\n> > > > > >\n> > > > > > #include \n> > > > > > #include \n> > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > > index 7507e9dda..22484721a 100644\n> > > > > > --- a/src/libcamera/camera.cpp\n> > > > > > +++ b/src/libcamera/camera.cpp\n> > > > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const\n> > > > > > * completed\n> > > > > > */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\var Camera::metadataAvailable\n> > > > > > + * \\brief Signal emitted when some metadata for a request is available as a\n> > > > > > + * partial result\n> > > > > > + */\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\var Camera::requestCompleted\n> > > > > > * \\brief Signal emitted when a request queued to the camera has completed\n> > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > > index 991b06f26..4ba96cfa2 100644\n> > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> > > > > > return request->_d()->completeBuffer(buffer);\n> > > > > > }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Complete part of metadata for a request\n> > > > > > + * \\param[in] request The request the metadata belongs to\n> > > > > > + * \\param[in] metadata The partial metadata that has completed\n> > > > > > + *\n> > > > > > + * This function could be called by pipeline handlers to signal availability of\n> > > > > > + * \\a metadata before \\a request completes. Early metadata completion allows to\n> > > > > > + * notify applications about the availability of a partial metadata buffer\n> > > > > > + * before the associated Request has completed.\n> > > > > > + *\n> > > > > > + * A metadata key is expected to be completed at most once. If it's completed\n> > > > > > + * more than once, the key will be dropped since the second time.\n> > > > >\n> > > > > * A metadata is expected to be completed at most once. If completed\n> > > > > * more than once it will be ignored.\n> > > >\n> > > > Updated.\n> > > >\n> > > > >\n> > > > > > + *\n> > > > > > + * \\context This function shall be called from the CameraManager thread.\n> > > > > > + */\n> > > > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > > > > +{\n> > > > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > > > > + if (!validMetadata.empty()) {\n> > > > > > + request->metadata().merge(validMetadata);\n> > > > > > +\n> > > > > > + Camera *camera = request->_d()->camera();\n> > > > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > > > + }\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\brief Signal request completion\n> > > > > > * \\param[in] request The request that has completed\n> > > > > > @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > > >\n> > > > > > Camera::Private *data = camera->_d();\n> > > > >\n> > > > > Do not break the\n> > > > >\n> > > > > Camera::Private *data = camera->_d();\n> > > > >\n> > > > > while (!data->queuedRequests_.empty()) {\n> > > > >\n> > > > > sequence\n> > > > >\n> > > > > Move the below hunk before the declaration of *data\n> > > >\n> > > > Done\n> > > >\n> > > > > >\n> > > > > > + /*\n> > > > > > + * Collect metadata which is not yet completed by the Camera, and\n> > > > > > + * create one partial result to cover the missing metadata before\n> > > > > > + * completing the whole request. This guarantees the aggregation of\n> > > > > > + * metadata in completed partial results equals to the global metadata\n> > > > > > + * in the request.\n> > > > > > + *\n> > > > > > + * \\todo: Forbid merging metadata into request.metadata() directly and\n> > > > > > + * force calling completeMetadata() to report metadata.\n> > > > >\n> > > > > This seems to suggest all pipelines will go through partial metadata\n> > > > > completion. I'm not 100% this will be the case.\n> > > >\n> > > > Let me remove the todo then?\n> > > >\n> > > > >\n> > > > > > + */\n> > > > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(\n> > > > > > + request->metadata());\n> > > > > > + if (!validMetadata.empty())\n> > > > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > > > +\n> > > > > > while (!data->queuedRequests_.empty()) {\n> > > > > > Request *req = data->queuedRequests_.front();\n> > > > > > if (req->status() == Request::RequestPending)\n> > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > > index 8c56ed30d..ae5cdeb19 100644\n> > > > > > --- a/src/libcamera/request.cpp\n> > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > @@ -178,6 +178,7 @@ void Request::Private::reset()\n> > > > > > pending_.clear();\n> > > > > > notifiers_.clear();\n> > > > > > timer_.reset();\n> > > > > > + completedMetadata_.clear();\n> > > > > > }\n> > > > > >\n> > > > > > /*\n> > > > > > @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)\n> > > > > > * if they have failed preparing.\n> > > > > > */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Add completed metadata, as a partial result\n> > > > >\n> > > > > What about:\n> > > > >\n> > > > > * \\brief Accumulate metadata keys and compute a list of not yet notified metadata\n> > > >\n> > > > Thanks! Done.\n> > > >\n> > > > >\n> > > > > > + * \\param[in] metadata The metadata completed\n> > > > >\n> > > > > The completed metadata list\n> > > >\n> > > > Updated.\n> > > >\n> > > > >\n> > > > > > + *\n> > > > > > + * Request will record the entries that has been sent to the application, to\n> > > > > > + * prevent duplicated controls.\n> > > > >\n> > > > > What about:\n> > > > >\n> > > > > * Accumulate the metadata keys from \\a metadata in an internal list of\n> > > > > * completed metadata and compute a list of metadata that has not yet been\n> > > > > * notified to the application.\n> > > > > *\n> > > > > * A metadata can only be notified once and gets ignored if completed multiple\n> > > > > * times.\n> > > >\n> > > > Thanks! Done.\n> > > >\n> > > > >\n> > > > > > + *\n> > > > > > + * \\return ControlList that hasn't been completed before\n> > > > >\n> > > > > \\return A ControlList of metadata that hasn't been notified to the\n> > > > > application yet\n> > > >\n> > > > Done\n> > > >\n> > > > >\n> > > > > > + */\n> > > > > > +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > > > > +{\n> > > > > > + ControlList resultMetadata;\n> > > > > > + for (auto &[id, value] : metadata) {\n> > > > > > + if (!completedMetadata_.count(id))\n> > > > >\n> > > > > I might have missed where the id is added to completedMetadata_ ?\n> > > >\n> > > > Nah, I should've added it here.\n> > > > Updated.\n> > > >\n> > > > Actually, I'm considering if we should keep the values in\n> > > > `completedMetadata_`, and refresh `Request::metadata()` when calling\n> > > > signal requestCompleted, to prevent the pipeline handler updating\n> > >\n> > > Is this an \"OR keep ids in completedMetadata_ OR accumulate them in\n> > > Request::metadata()\" ?\n> >\n> > I meant keeping ids & values in `completedMetadata_`.\n> >\n> > >\n> > > > metadata tags that have been sent to applications. This would ensure\n> > > > the alignment of metadata between signal metadataAvailable and the\n> > > > completed metadata in Request.\n> > > > WDYT?\n> > >\n> > > Unless there are reasons I am missing why this isn't possible, I think\n> > > having the accumulated metadata in Request::metadata() is a good idea.\n> > >\n> > > Even more so as you're already doing it in\n> > > PipelineHandler::completeMetadata()\n> > >\n> > > void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > {\n> > > const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > if (!validMetadata.empty()) {\n> > > request->metadata().merge(validMetadata);\n> > >\n> > > Hence I think you can use Request::metadata_ to check what metadata\n> > > have already been completed with something like\n> > >\n> > > ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > {\n> > > ControlList resultMetadata;\n> > > for (auto &[id, value] : metadata) {\n> > > if (!metadata_.contains(id) && !resultMetadata.count(id))\n> > > resultMetadata.set(id, value);\n> > > }\n> > >\n> > > metadata_.merge(resultMetadata);\n> > >\n> > > return resultMetadata;\n> > > }\n> > >\n> > > Do you think this could work ?\n> >\n> > Exactly, that's what I was thinking. Thanks!\n> > Only a nit that we need:\n> > `metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`\n> >\n> > Will include this in the next version.\n>\n> Sorry, I just found that this may result in performance issues, when\n> we overwrite metadata values that are large in size repetitively. How\n> about we overwrite it once in `PipelineHandler::requestComplete`, and\n> expect applications wouldn't use `Request::metadata()` directly before\n> signal requestCompleted is invoked?\n\nI'm sorry but doesn't\n\n ControlList resultMetadata;\n for (auto &[id, value] : metadata) {\n if (!metadata_.contains(id) && !resultMetadata.count(id))\n resultMetadata.set(id, value);\n }\n\n metadata_.merge(resultMetadata);\n\nmean we never overwrite an entry which is already part of metadata_ ?\n\n>\n> BR,\n> Harvey\n>\n> >\n> > BR,\n> > Harvey\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > BR,\n> > > > Harvey\n> > > >\n> > > >\n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > > + resultMetadata.set(id, value);\n> > > > > > + }\n> > > > > > +\n> > > > > > + return resultMetadata;\n> > > > > > +}\n> > > > > > +\n> > > > > > void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > > > > > {\n> > > > > > /* Close the fence if successfully signalled. */\n> > > > > > --\n> > > > > > 2.47.0.338.g60cca15819-goog\n> > > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF677BDB1C\n\tfor ;\n\tThu, 5 Dec 2024 15:21:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B173566104;\n\tThu, 5 Dec 2024 16:21:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C1676608C\n\tfor ;\n\tThu, 5 Dec 2024 16:21:52 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E6412B3;\n\tThu, 5 Dec 2024 16:21:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aSMZFzB4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733412083;\n\tbh=1ioICl7bK8wDfS7A9nO7uv4QkA5JQu0Ci/6ItGEoeOQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aSMZFzB4tFD5ETeYlr8NXZol9F+rgXbAcZlOErg8vvpppGG2E8Jb1XzXn4JwZwmbn\n\tuG2jFX/350UfItIlMC28gKMjTTqeZEZNAgtxhPLX4uBw8g6B/2xXC2zQjoQftvkJmv\n\te9FrHrXRMOUMZAN9R/F193T0W2CX72Q1Snychd5s=","Date":"Thu, 5 Dec 2024 16:21:48 +0100","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","Message-ID":"<27xrxiauhjwjhzntw3tbdrguzsbzjqtcfeh2xd2naiqcurhnlj@6ntery3oesby>","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-2-chenghaoyang@chromium.org>\n\t\n\t\n\t<7wgvh2ukgfron75f3cjnrfmn6yzqec6z6f4avlnz7u6ltdhnbp@ghsdq2a4lbnb>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32546,"web_url":"https://patchwork.libcamera.org/comment/32546/","msgid":"","date":"2024-12-05T15:40:18","subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Dec 5, 2024 at 11:21 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Thu, Dec 05, 2024 at 10:57:48PM +0800, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > On Thu, Dec 5, 2024 at 8:46 PM Cheng-Hao Yang wrote:\n> > >\n> > > Hi Jacopo,\n> > >\n> > > On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi Harvey\n> > > >\n> > > > On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Will upload a new version in another series.\n> > > > >\n> > > > > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi\n> > > > > wrote:\n> > > > > >\n> > > > > > Hi Harvey\n> > > > > >\n> > > > > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:\n> > > > > > > From: Han-Lin Chen \n> > > > > > >\n> > > > > > > Allows pipeline handler to signal metadata completion by adding the\n> > > > > > > following signals to Camera:\n> > > > > > >\n> > > > > > > Signal metadataAvailable;\n> > > > > > >\n> > > > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to\n> > > > > > > return buffers and partial metadata at any stage of processing.\n> > > > > > >\n> > > > > > > Signed-off-by: Han-Lin Chen \n> > > > > > > Co-developed-by: Harvey Yang \n> > > > > > > ---\n> > > > > > > include/libcamera/camera.h | 1 +\n> > > > > > > include/libcamera/internal/pipeline_handler.h | 1 +\n> > > > > > > include/libcamera/internal/request.h | 4 ++\n> > > > > > > include/libcamera/request.h | 1 +\n> > > > > > > src/libcamera/camera.cpp | 6 +++\n> > > > > > > src/libcamera/pipeline_handler.cpp | 41 +++++++++++++++++++\n> > > > > > > src/libcamera/request.cpp | 21 ++++++++++\n> > > > > > > 7 files changed, 75 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > > > > index 94cee7bd8..eb7cdf81b 100644\n> > > > > > > --- a/include/libcamera/camera.h\n> > > > > > > +++ b/include/libcamera/camera.h\n> > > > > > > @@ -122,6 +122,7 @@ public:\n> > > > > > >\n> > > > > > > const std::string &id() const;\n> > > > > > >\n> > > > > > > + Signal metadataAvailable;\n> > > > > > > Signal bufferCompleted;\n> > > > > > > Signal requestCompleted;\n> > > > > > > Signal<> disconnected;\n> > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > > > index fb28a18d0..6c6cad66f 100644\n> > > > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > > > @@ -58,6 +58,7 @@ public:\n> > > > > > > void registerRequest(Request *request);\n> > > > > > > void queueRequest(Request *request);\n> > > > > > >\n> > > > > > > + void completeMetadata(Request *request, const ControlList &metadata);\n> > > > > >\n> > > > > > nit: \"complete\" seems to suggest we're done with it, while instead\n> > > > > > the function should probably follow the signal name\n> > > > > > \"metadataAvailable()\"\n> > > > >\n> > > > > Done\n> > > > >\n> > > > > >\n> > > > > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > > > > void completeRequest(Request *request);\n> > > > > > > void cancelRequest(Request *request);\n> > > > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > > > > > index 4e7d05b1e..286cd9d76 100644\n> > > > > > > --- a/include/libcamera/internal/request.h\n> > > > > > > +++ b/include/libcamera/internal/request.h\n> > > > > > > @@ -43,6 +43,8 @@ public:\n> > > > > > > void prepare(std::chrono::milliseconds timeout = 0ms);\n> > > > > > > Signal<> prepared;\n> > > > > > >\n> > > > > > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > > > > > +\n> > > > > > > private:\n> > > > > > > friend class PipelineHandler;\n> > > > > > > friend std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > > > > > @@ -60,6 +62,8 @@ private:\n> > > > > > > std::unordered_set pending_;\n> > > > > > > std::map> notifiers_;\n> > > > > > > std::unique_ptr timer_;\n> > > > > > > +\n> > > > > > > + std::unordered_set completedMetadata_;\n> > > > > > > };\n> > > > > > >\n> > > > > > > } /* namespace libcamera */\n> > > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > > > > index e214a9d13..2c78d9bb4 100644\n> > > > > > > --- a/include/libcamera/request.h\n> > > > > > > +++ b/include/libcamera/request.h\n> > > > > > > @@ -12,6 +12,7 @@\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > >\n> > > > > > Not necessary anymore\n> > > > >\n> > > > > Removed\n> > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > > > index 7507e9dda..22484721a 100644\n> > > > > > > --- a/src/libcamera/camera.cpp\n> > > > > > > +++ b/src/libcamera/camera.cpp\n> > > > > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const\n> > > > > > > * completed\n> > > > > > > */\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\var Camera::metadataAvailable\n> > > > > > > + * \\brief Signal emitted when some metadata for a request is available as a\n> > > > > > > + * partial result\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > /**\n> > > > > > > * \\var Camera::requestCompleted\n> > > > > > > * \\brief Signal emitted when a request queued to the camera has completed\n> > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > > > index 991b06f26..4ba96cfa2 100644\n> > > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > > @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> > > > > > > return request->_d()->completeBuffer(buffer);\n> > > > > > > }\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\brief Complete part of metadata for a request\n> > > > > > > + * \\param[in] request The request the metadata belongs to\n> > > > > > > + * \\param[in] metadata The partial metadata that has completed\n> > > > > > > + *\n> > > > > > > + * This function could be called by pipeline handlers to signal availability of\n> > > > > > > + * \\a metadata before \\a request completes. Early metadata completion allows to\n> > > > > > > + * notify applications about the availability of a partial metadata buffer\n> > > > > > > + * before the associated Request has completed.\n> > > > > > > + *\n> > > > > > > + * A metadata key is expected to be completed at most once. If it's completed\n> > > > > > > + * more than once, the key will be dropped since the second time.\n> > > > > >\n> > > > > > * A metadata is expected to be completed at most once. If completed\n> > > > > > * more than once it will be ignored.\n> > > > >\n> > > > > Updated.\n> > > > >\n> > > > > >\n> > > > > > > + *\n> > > > > > > + * \\context This function shall be called from the CameraManager thread.\n> > > > > > > + */\n> > > > > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > > > > > +{\n> > > > > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > > > > > + if (!validMetadata.empty()) {\n> > > > > > > + request->metadata().merge(validMetadata);\n> > > > > > > +\n> > > > > > > + Camera *camera = request->_d()->camera();\n> > > > > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > > > > + }\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > /**\n> > > > > > > * \\brief Signal request completion\n> > > > > > > * \\param[in] request The request that has completed\n> > > > > > > @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > > > >\n> > > > > > > Camera::Private *data = camera->_d();\n> > > > > >\n> > > > > > Do not break the\n> > > > > >\n> > > > > > Camera::Private *data = camera->_d();\n> > > > > >\n> > > > > > while (!data->queuedRequests_.empty()) {\n> > > > > >\n> > > > > > sequence\n> > > > > >\n> > > > > > Move the below hunk before the declaration of *data\n> > > > >\n> > > > > Done\n> > > > >\n> > > > > > >\n> > > > > > > + /*\n> > > > > > > + * Collect metadata which is not yet completed by the Camera, and\n> > > > > > > + * create one partial result to cover the missing metadata before\n> > > > > > > + * completing the whole request. This guarantees the aggregation of\n> > > > > > > + * metadata in completed partial results equals to the global metadata\n> > > > > > > + * in the request.\n> > > > > > > + *\n> > > > > > > + * \\todo: Forbid merging metadata into request.metadata() directly and\n> > > > > > > + * force calling completeMetadata() to report metadata.\n> > > > > >\n> > > > > > This seems to suggest all pipelines will go through partial metadata\n> > > > > > completion. I'm not 100% this will be the case.\n> > > > >\n> > > > > Let me remove the todo then?\n> > > > >\n> > > > > >\n> > > > > > > + */\n> > > > > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(\n> > > > > > > + request->metadata());\n> > > > > > > + if (!validMetadata.empty())\n> > > > > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > > > > +\n> > > > > > > while (!data->queuedRequests_.empty()) {\n> > > > > > > Request *req = data->queuedRequests_.front();\n> > > > > > > if (req->status() == Request::RequestPending)\n> > > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > > > index 8c56ed30d..ae5cdeb19 100644\n> > > > > > > --- a/src/libcamera/request.cpp\n> > > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > > @@ -178,6 +178,7 @@ void Request::Private::reset()\n> > > > > > > pending_.clear();\n> > > > > > > notifiers_.clear();\n> > > > > > > timer_.reset();\n> > > > > > > + completedMetadata_.clear();\n> > > > > > > }\n> > > > > > >\n> > > > > > > /*\n> > > > > > > @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)\n> > > > > > > * if they have failed preparing.\n> > > > > > > */\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\brief Add completed metadata, as a partial result\n> > > > > >\n> > > > > > What about:\n> > > > > >\n> > > > > > * \\brief Accumulate metadata keys and compute a list of not yet notified metadata\n> > > > >\n> > > > > Thanks! Done.\n> > > > >\n> > > > > >\n> > > > > > > + * \\param[in] metadata The metadata completed\n> > > > > >\n> > > > > > The completed metadata list\n> > > > >\n> > > > > Updated.\n> > > > >\n> > > > > >\n> > > > > > > + *\n> > > > > > > + * Request will record the entries that has been sent to the application, to\n> > > > > > > + * prevent duplicated controls.\n> > > > > >\n> > > > > > What about:\n> > > > > >\n> > > > > > * Accumulate the metadata keys from \\a metadata in an internal list of\n> > > > > > * completed metadata and compute a list of metadata that has not yet been\n> > > > > > * notified to the application.\n> > > > > > *\n> > > > > > * A metadata can only be notified once and gets ignored if completed multiple\n> > > > > > * times.\n> > > > >\n> > > > > Thanks! Done.\n> > > > >\n> > > > > >\n> > > > > > > + *\n> > > > > > > + * \\return ControlList that hasn't been completed before\n> > > > > >\n> > > > > > \\return A ControlList of metadata that hasn't been notified to the\n> > > > > > application yet\n> > > > >\n> > > > > Done\n> > > > >\n> > > > > >\n> > > > > > > + */\n> > > > > > > +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > > > > > +{\n> > > > > > > + ControlList resultMetadata;\n> > > > > > > + for (auto &[id, value] : metadata) {\n> > > > > > > + if (!completedMetadata_.count(id))\n> > > > > >\n> > > > > > I might have missed where the id is added to completedMetadata_ ?\n> > > > >\n> > > > > Nah, I should've added it here.\n> > > > > Updated.\n> > > > >\n> > > > > Actually, I'm considering if we should keep the values in\n> > > > > `completedMetadata_`, and refresh `Request::metadata()` when calling\n> > > > > signal requestCompleted, to prevent the pipeline handler updating\n> > > >\n> > > > Is this an \"OR keep ids in completedMetadata_ OR accumulate them in\n> > > > Request::metadata()\" ?\n> > >\n> > > I meant keeping ids & values in `completedMetadata_`.\n> > >\n> > > >\n> > > > > metadata tags that have been sent to applications. This would ensure\n> > > > > the alignment of metadata between signal metadataAvailable and the\n> > > > > completed metadata in Request.\n> > > > > WDYT?\n> > > >\n> > > > Unless there are reasons I am missing why this isn't possible, I think\n> > > > having the accumulated metadata in Request::metadata() is a good idea.\n> > > >\n> > > > Even more so as you're already doing it in\n> > > > PipelineHandler::completeMetadata()\n> > > >\n> > > > void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > > {\n> > > > const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > > if (!validMetadata.empty()) {\n> > > > request->metadata().merge(validMetadata);\n> > > >\n> > > > Hence I think you can use Request::metadata_ to check what metadata\n> > > > have already been completed with something like\n> > > >\n> > > > ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > > {\n> > > > ControlList resultMetadata;\n> > > > for (auto &[id, value] : metadata) {\n> > > > if (!metadata_.contains(id) && !resultMetadata.count(id))\n> > > > resultMetadata.set(id, value);\n> > > > }\n> > > >\n> > > > metadata_.merge(resultMetadata);\n> > > >\n> > > > return resultMetadata;\n> > > > }\n> > > >\n> > > > Do you think this could work ?\n> > >\n> > > Exactly, that's what I was thinking. Thanks!\n> > > Only a nit that we need:\n> > > `metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`\n> > >\n> > > Will include this in the next version.\n> >\n> > Sorry, I just found that this may result in performance issues, when\n> > we overwrite metadata values that are large in size repetitively. How\n> > about we overwrite it once in `PipelineHandler::requestComplete`, and\n> > expect applications wouldn't use `Request::metadata()` directly before\n> > signal requestCompleted is invoked?\n>\n> I'm sorry but doesn't\n>\n> ControlList resultMetadata;\n> for (auto &[id, value] : metadata) {\n> if (!metadata_.contains(id) && !resultMetadata.count(id))\n> resultMetadata.set(id, value);\n> }\n>\n> metadata_.merge(resultMetadata);\n>\n> mean we never overwrite an entry which is already part of metadata_ ?\n\nAh got it. I misunderstood your code.\n\nRegarding `metadata_`, I assume you meant `_o()->metadata_`.\n\nThe problem is, currently pipeline handler implementations directly\nupdate `Request::metadata_` instead of using\n`PipelineHandler::metadataAvailable()`. What I want to do is, if a\npipeline handler implementation uses\n`PipelineHandler::metadataAvailable()` to complete metadata id A and\nvalue V1, and modify `Request::metadata_->set(A, V2)`, how do we\nrevert the value from V2 to V1 in\n`PipelineHandler::completeRequest()`?\n\nAlso, in your implementation, if a pipeline handler implementation\ndoes `Request::metadata_->set(A, V2)`, and uses\n`PipelineHandler::metadataAvailable()` to complete metadata id A and\nvalue V1, [A, V1] will be sent to the application with the new signal,\nwhile `Request::metadata_.get(A)` will remain V2.\n\nWDYT?\n\nBR,\nHarvey\n\n>\n> >\n> > BR,\n> > Harvey\n> >\n> > >\n> > > BR,\n> > > Harvey\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > >\n> > > > > BR,\n> > > > > Harvey\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > > > + resultMetadata.set(id, value);\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + return resultMetadata;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > > > > > > {\n> > > > > > > /* Close the fence if successfully signalled. */\n> > > > > > > --\n> > > > > > > 2.47.0.338.g60cca15819-goog\n> > > > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DB1E1C323E\n\tfor ;\n\tThu, 5 Dec 2024 15:40:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF6A266109;\n\tThu, 5 Dec 2024 16:40:32 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1C1A6608C\n\tfor ;\n\tThu, 5 Dec 2024 16:40:30 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id\n\t38308e7fff4ca-2f75c56f16aso9040141fa.0\n\tfor ;\n\tThu, 05 Dec 2024 07:40:30 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"UU1ouEdG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733413230; x=1734018030;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=TBID+SZfOnLxKMxLmjD4y+cJFaws0wIbCX1Gfe/INlE=;\n\tb=UU1ouEdGIpin/yGbTlLxp7+Yhut8GyqVv46+yG/SZLEWWZ3/GiD+46X4wn+UY9d3+F\n\tAWSIVvUMgWCbME9Xu1b64jw7ltHC5d60ZwLWWPoqLGj6vnTg1AIR21m4T6SYlGxKjGdB\n\tVJgnc46RcvJAxur56E0+LJ7hgiYOx2VzhhdBM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733413230; x=1734018030;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=TBID+SZfOnLxKMxLmjD4y+cJFaws0wIbCX1Gfe/INlE=;\n\tb=juw2/a12A3LsvsanC81vRuOOl+8UuoVDlxKuBvHNK0ftc+dFdycVvKVDs5XuprT4LE\n\tgpfSH7l0Frc/PCB6iy/F0L/wEGYzFoabZ+livL6DHuIEOEhxFR7C3Gmgafd/mputiDNT\n\tKwKY4F5YH4OTLAxleBiVJJ7DTtpnyMlI1fAsdmaABUp2U/EKrJY9+XyU7bO+T+tENG95\n\trqKD1fwQmxM8Culh5z1ieZqqcZZlh7x1cttt9o24J6+FG1E9IAfYHorQGL7fcj0Imrwi\n\tS6DLH5io3FLSQrtr2ZsQa1QCzOHDUQa8pjp7SepsSHb7k1owCatkSPbYr4kjsqHU5yU7\n\tDOUA==","X-Gm-Message-State":"AOJu0YzMc3/JnlSDiv/1QAXsz9ePkuig+acN48k0h3G5Y6BzcnABa0ct\n\tne6KrG9FQl3ERl5I4mhq2LYS4rJB493BTjLketSjZ+PtlXxlF8TFyLi/YkCBtvAIOOXjKXqXSzx\n\tcjcPc0jri/JnVYkVtWEIzs+x8XZXdxOVOp+qr","X-Gm-Gg":"ASbGnctSkGPh2P4TddPDFA6/h2TgMv/zae6bsANR1CQCRuuD3zP1WGQkzPYHkjxN3bc\n\ti/AXm+dD02n0u4tUJWVFBkBVn/hTiETbi1Zy1oDvyOieoWucIwwVHDlsfA9oJwI6NDw==","X-Google-Smtp-Source":"AGHT+IEixA21BvuhxAoBlrv5ETsqbyR423Aa+lFaoCK1Ze80gYBiOL0qM0pHMvVPnMej7MxsqpPsF8RLP+6EALLqQJc=","X-Received":"by 2002:a05:651c:154f:b0:300:1c7a:e22c with SMTP id\n\t38308e7fff4ca-3001c7ae65dmr17262101fa.4.1733413229897;\n\tThu, 05 Dec 2024 07:40:29 -0800 (PST)","MIME-Version":"1.0","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-2-chenghaoyang@chromium.org>\n\t\n\t\n\t<7wgvh2ukgfron75f3cjnrfmn6yzqec6z6f4avlnz7u6ltdhnbp@ghsdq2a4lbnb>\n\t\n\t\n\t<27xrxiauhjwjhzntw3tbdrguzsbzjqtcfeh2xd2naiqcurhnlj@6ntery3oesby>","In-Reply-To":"<27xrxiauhjwjhzntw3tbdrguzsbzjqtcfeh2xd2naiqcurhnlj@6ntery3oesby>","From":"Cheng-Hao Yang ","Date":"Thu, 5 Dec 2024 23:40:18 +0800","Message-ID":"","Subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32550,"web_url":"https://patchwork.libcamera.org/comment/32550/","msgid":"","date":"2024-12-05T19:04:58","subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"HI Harvey,\n\nOn Thu, Dec 05, 2024 at 11:40:18PM +0800, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> On Thu, Dec 5, 2024 at 11:21 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Harvey\n> >\n> > On Thu, Dec 05, 2024 at 10:57:48PM +0800, Cheng-Hao Yang wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Thu, Dec 5, 2024 at 8:46 PM Cheng-Hao Yang wrote:\n> > > >\n> > > > Hi Jacopo,\n> > > >\n> > > > On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi\n> > > > wrote:\n> > > > >\n> > > > > Hi Harvey\n> > > > >\n> > > > > On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:\n> > > > > > Hi Jacopo,\n> > > > > >\n> > > > > > Will upload a new version in another series.\n> > > > > >\n> > > > > > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi\n> > > > > > wrote:\n> > > > > > >\n> > > > > > > Hi Harvey\n> > > > > > >\n> > > > > > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:\n> > > > > > > > From: Han-Lin Chen \n> > > > > > > >\n> > > > > > > > Allows pipeline handler to signal metadata completion by adding the\n> > > > > > > > following signals to Camera:\n> > > > > > > >\n> > > > > > > > Signal metadataAvailable;\n> > > > > > > >\n> > > > > > > > Together with the bufferCompleted signal, the pipeline handler is allowed to\n> > > > > > > > return buffers and partial metadata at any stage of processing.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Han-Lin Chen \n> > > > > > > > Co-developed-by: Harvey Yang \n> > > > > > > > ---\n> > > > > > > > include/libcamera/camera.h | 1 +\n> > > > > > > > include/libcamera/internal/pipeline_handler.h | 1 +\n> > > > > > > > include/libcamera/internal/request.h | 4 ++\n> > > > > > > > include/libcamera/request.h | 1 +\n> > > > > > > > src/libcamera/camera.cpp | 6 +++\n> > > > > > > > src/libcamera/pipeline_handler.cpp | 41 +++++++++++++++++++\n> > > > > > > > src/libcamera/request.cpp | 21 ++++++++++\n> > > > > > > > 7 files changed, 75 insertions(+)\n> > > > > > > >\n> > > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > > > > > index 94cee7bd8..eb7cdf81b 100644\n> > > > > > > > --- a/include/libcamera/camera.h\n> > > > > > > > +++ b/include/libcamera/camera.h\n> > > > > > > > @@ -122,6 +122,7 @@ public:\n> > > > > > > >\n> > > > > > > > const std::string &id() const;\n> > > > > > > >\n> > > > > > > > + Signal metadataAvailable;\n> > > > > > > > Signal bufferCompleted;\n> > > > > > > > Signal requestCompleted;\n> > > > > > > > Signal<> disconnected;\n> > > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > > > > index fb28a18d0..6c6cad66f 100644\n> > > > > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > > > > @@ -58,6 +58,7 @@ public:\n> > > > > > > > void registerRequest(Request *request);\n> > > > > > > > void queueRequest(Request *request);\n> > > > > > > >\n> > > > > > > > + void completeMetadata(Request *request, const ControlList &metadata);\n> > > > > > >\n> > > > > > > nit: \"complete\" seems to suggest we're done with it, while instead\n> > > > > > > the function should probably follow the signal name\n> > > > > > > \"metadataAvailable()\"\n> > > > > >\n> > > > > > Done\n> > > > > >\n> > > > > > >\n> > > > > > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > > > > > void completeRequest(Request *request);\n> > > > > > > > void cancelRequest(Request *request);\n> > > > > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > > > > > > index 4e7d05b1e..286cd9d76 100644\n> > > > > > > > --- a/include/libcamera/internal/request.h\n> > > > > > > > +++ b/include/libcamera/internal/request.h\n> > > > > > > > @@ -43,6 +43,8 @@ public:\n> > > > > > > > void prepare(std::chrono::milliseconds timeout = 0ms);\n> > > > > > > > Signal<> prepared;\n> > > > > > > >\n> > > > > > > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > > > > > > +\n> > > > > > > > private:\n> > > > > > > > friend class PipelineHandler;\n> > > > > > > > friend std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > > > > > > @@ -60,6 +62,8 @@ private:\n> > > > > > > > std::unordered_set pending_;\n> > > > > > > > std::map> notifiers_;\n> > > > > > > > std::unique_ptr timer_;\n> > > > > > > > +\n> > > > > > > > + std::unordered_set completedMetadata_;\n> > > > > > > > };\n> > > > > > > >\n> > > > > > > > } /* namespace libcamera */\n> > > > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > > > > > index e214a9d13..2c78d9bb4 100644\n> > > > > > > > --- a/include/libcamera/request.h\n> > > > > > > > +++ b/include/libcamera/request.h\n> > > > > > > > @@ -12,6 +12,7 @@\n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > > +#include \n> > > > > > >\n> > > > > > > Not necessary anymore\n> > > > > >\n> > > > > > Removed\n> > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > > > > index 7507e9dda..22484721a 100644\n> > > > > > > > --- a/src/libcamera/camera.cpp\n> > > > > > > > +++ b/src/libcamera/camera.cpp\n> > > > > > > > @@ -892,6 +892,12 @@ const std::string &Camera::id() const\n> > > > > > > > * completed\n> > > > > > > > */\n> > > > > > > >\n> > > > > > > > +/**\n> > > > > > > > + * \\var Camera::metadataAvailable\n> > > > > > > > + * \\brief Signal emitted when some metadata for a request is available as a\n> > > > > > > > + * partial result\n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > /**\n> > > > > > > > * \\var Camera::requestCompleted\n> > > > > > > > * \\brief Signal emitted when a request queued to the camera has completed\n> > > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > > > > index 991b06f26..4ba96cfa2 100644\n> > > > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > > > @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> > > > > > > > return request->_d()->completeBuffer(buffer);\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Complete part of metadata for a request\n> > > > > > > > + * \\param[in] request The request the metadata belongs to\n> > > > > > > > + * \\param[in] metadata The partial metadata that has completed\n> > > > > > > > + *\n> > > > > > > > + * This function could be called by pipeline handlers to signal availability of\n> > > > > > > > + * \\a metadata before \\a request completes. Early metadata completion allows to\n> > > > > > > > + * notify applications about the availability of a partial metadata buffer\n> > > > > > > > + * before the associated Request has completed.\n> > > > > > > > + *\n> > > > > > > > + * A metadata key is expected to be completed at most once. If it's completed\n> > > > > > > > + * more than once, the key will be dropped since the second time.\n> > > > > > >\n> > > > > > > * A metadata is expected to be completed at most once. If completed\n> > > > > > > * more than once it will be ignored.\n> > > > > >\n> > > > > > Updated.\n> > > > > >\n> > > > > > >\n> > > > > > > > + *\n> > > > > > > > + * \\context This function shall be called from the CameraManager thread.\n> > > > > > > > + */\n> > > > > > > > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > > > > > > +{\n> > > > > > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > > > > > > + if (!validMetadata.empty()) {\n> > > > > > > > + request->metadata().merge(validMetadata);\n> > > > > > > > +\n> > > > > > > > + Camera *camera = request->_d()->camera();\n> > > > > > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > > > > > + }\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > /**\n> > > > > > > > * \\brief Signal request completion\n> > > > > > > > * \\param[in] request The request that has completed\n> > > > > > > > @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > > > > >\n> > > > > > > > Camera::Private *data = camera->_d();\n> > > > > > >\n> > > > > > > Do not break the\n> > > > > > >\n> > > > > > > Camera::Private *data = camera->_d();\n> > > > > > >\n> > > > > > > while (!data->queuedRequests_.empty()) {\n> > > > > > >\n> > > > > > > sequence\n> > > > > > >\n> > > > > > > Move the below hunk before the declaration of *data\n> > > > > >\n> > > > > > Done\n> > > > > >\n> > > > > > > >\n> > > > > > > > + /*\n> > > > > > > > + * Collect metadata which is not yet completed by the Camera, and\n> > > > > > > > + * create one partial result to cover the missing metadata before\n> > > > > > > > + * completing the whole request. This guarantees the aggregation of\n> > > > > > > > + * metadata in completed partial results equals to the global metadata\n> > > > > > > > + * in the request.\n> > > > > > > > + *\n> > > > > > > > + * \\todo: Forbid merging metadata into request.metadata() directly and\n> > > > > > > > + * force calling completeMetadata() to report metadata.\n> > > > > > >\n> > > > > > > This seems to suggest all pipelines will go through partial metadata\n> > > > > > > completion. I'm not 100% this will be the case.\n> > > > > >\n> > > > > > Let me remove the todo then?\n> > > > > >\n> > > > > > >\n> > > > > > > > + */\n> > > > > > > > + const ControlList validMetadata = request->_d()->addCompletedMetadata(\n> > > > > > > > + request->metadata());\n> > > > > > > > + if (!validMetadata.empty())\n> > > > > > > > + camera->metadataAvailable.emit(request, validMetadata);\n> > > > > > > > +\n> > > > > > > > while (!data->queuedRequests_.empty()) {\n> > > > > > > > Request *req = data->queuedRequests_.front();\n> > > > > > > > if (req->status() == Request::RequestPending)\n> > > > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > > > > index 8c56ed30d..ae5cdeb19 100644\n> > > > > > > > --- a/src/libcamera/request.cpp\n> > > > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > > > @@ -178,6 +178,7 @@ void Request::Private::reset()\n> > > > > > > > pending_.clear();\n> > > > > > > > notifiers_.clear();\n> > > > > > > > timer_.reset();\n> > > > > > > > + completedMetadata_.clear();\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > /*\n> > > > > > > > @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)\n> > > > > > > > * if they have failed preparing.\n> > > > > > > > */\n> > > > > > > >\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Add completed metadata, as a partial result\n> > > > > > >\n> > > > > > > What about:\n> > > > > > >\n> > > > > > > * \\brief Accumulate metadata keys and compute a list of not yet notified metadata\n> > > > > >\n> > > > > > Thanks! Done.\n> > > > > >\n> > > > > > >\n> > > > > > > > + * \\param[in] metadata The metadata completed\n> > > > > > >\n> > > > > > > The completed metadata list\n> > > > > >\n> > > > > > Updated.\n> > > > > >\n> > > > > > >\n> > > > > > > > + *\n> > > > > > > > + * Request will record the entries that has been sent to the application, to\n> > > > > > > > + * prevent duplicated controls.\n> > > > > > >\n> > > > > > > What about:\n> > > > > > >\n> > > > > > > * Accumulate the metadata keys from \\a metadata in an internal list of\n> > > > > > > * completed metadata and compute a list of metadata that has not yet been\n> > > > > > > * notified to the application.\n> > > > > > > *\n> > > > > > > * A metadata can only be notified once and gets ignored if completed multiple\n> > > > > > > * times.\n> > > > > >\n> > > > > > Thanks! Done.\n> > > > > >\n> > > > > > >\n> > > > > > > > + *\n> > > > > > > > + * \\return ControlList that hasn't been completed before\n> > > > > > >\n> > > > > > > \\return A ControlList of metadata that hasn't been notified to the\n> > > > > > > application yet\n> > > > > >\n> > > > > > Done\n> > > > > >\n> > > > > > >\n> > > > > > > > + */\n> > > > > > > > +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > > > > > > +{\n> > > > > > > > + ControlList resultMetadata;\n> > > > > > > > + for (auto &[id, value] : metadata) {\n> > > > > > > > + if (!completedMetadata_.count(id))\n> > > > > > >\n> > > > > > > I might have missed where the id is added to completedMetadata_ ?\n> > > > > >\n> > > > > > Nah, I should've added it here.\n> > > > > > Updated.\n> > > > > >\n> > > > > > Actually, I'm considering if we should keep the values in\n> > > > > > `completedMetadata_`, and refresh `Request::metadata()` when calling\n> > > > > > signal requestCompleted, to prevent the pipeline handler updating\n> > > > >\n> > > > > Is this an \"OR keep ids in completedMetadata_ OR accumulate them in\n> > > > > Request::metadata()\" ?\n> > > >\n> > > > I meant keeping ids & values in `completedMetadata_`.\n> > > >\n> > > > >\n> > > > > > metadata tags that have been sent to applications. This would ensure\n> > > > > > the alignment of metadata between signal metadataAvailable and the\n> > > > > > completed metadata in Request.\n> > > > > > WDYT?\n> > > > >\n> > > > > Unless there are reasons I am missing why this isn't possible, I think\n> > > > > having the accumulated metadata in Request::metadata() is a good idea.\n> > > > >\n> > > > > Even more so as you're already doing it in\n> > > > > PipelineHandler::completeMetadata()\n> > > > >\n> > > > > void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)\n> > > > > {\n> > > > > const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);\n> > > > > if (!validMetadata.empty()) {\n> > > > > request->metadata().merge(validMetadata);\n> > > > >\n> > > > > Hence I think you can use Request::metadata_ to check what metadata\n> > > > > have already been completed with something like\n> > > > >\n> > > > > ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)\n> > > > > {\n> > > > > ControlList resultMetadata;\n> > > > > for (auto &[id, value] : metadata) {\n> > > > > if (!metadata_.contains(id) && !resultMetadata.count(id))\n> > > > > resultMetadata.set(id, value);\n> > > > > }\n> > > > >\n> > > > > metadata_.merge(resultMetadata);\n> > > > >\n> > > > > return resultMetadata;\n> > > > > }\n> > > > >\n> > > > > Do you think this could work ?\n> > > >\n> > > > Exactly, that's what I was thinking. Thanks!\n> > > > Only a nit that we need:\n> > > > `metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`\n> > > >\n> > > > Will include this in the next version.\n> > >\n> > > Sorry, I just found that this may result in performance issues, when\n> > > we overwrite metadata values that are large in size repetitively. How\n> > > about we overwrite it once in `PipelineHandler::requestComplete`, and\n> > > expect applications wouldn't use `Request::metadata()` directly before\n> > > signal requestCompleted is invoked?\n> >\n> > I'm sorry but doesn't\n> >\n> > ControlList resultMetadata;\n> > for (auto &[id, value] : metadata) {\n> > if (!metadata_.contains(id) && !resultMetadata.count(id))\n> > resultMetadata.set(id, value);\n> > }\n> >\n> > metadata_.merge(resultMetadata);\n> >\n> > mean we never overwrite an entry which is already part of metadata_ ?\n>\n> Ah got it. I misunderstood your code.\n>\n> Regarding `metadata_`, I assume you meant `_o()->metadata_`.\n>\n> The problem is, currently pipeline handler implementations directly\n> update `Request::metadata_` instead of using\n> `PipelineHandler::metadataAvailable()`. What I want to do is, if a\n> pipeline handler implementation uses\n> `PipelineHandler::metadataAvailable()` to complete metadata id A and\n> value V1, and modify `Request::metadata_->set(A, V2)`, how do we\n> revert the value from V2 to V1 in\n> `PipelineHandler::completeRequest()`?\n>\n> Also, in your implementation, if a pipeline handler implementation\n> does `Request::metadata_->set(A, V2)`, and uses\n> `PipelineHandler::metadataAvailable()` to complete metadata id A and\n> value V1, [A, V1] will be sent to the application with the new signal,\n> while `Request::metadata_.get(A)` will remain V2.\n>\n> WDYT?\n\ngive me a few days and I'll get back with a more consistent\nproposal. I admit I have lost track of this code snippets sent\nover email :)\n\nThanks\n j\n\n>\n> BR,\n> Harvey\n>\n> >\n> > >\n> > > BR,\n> > > Harvey\n> > >\n> > > >\n> > > > BR,\n> > > > Harvey\n> > > >\n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > >\n> > > > > > BR,\n> > > > > > Harvey\n> > > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > Thanks\n> > > > > > > j\n> > > > > > >\n> > > > > > > > + resultMetadata.set(id, value);\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > + return resultMetadata;\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > > > > > > > {\n> > > > > > > > /* Close the fence if successfully signalled. */\n> > > > > > > > --\n> > > > > > > > 2.47.0.338.g60cca15819-goog\n> > > > > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DDCD5BDB1C\n\tfor ;\n\tThu, 5 Dec 2024 19:05:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D68A56610E;\n\tThu, 5 Dec 2024 20:05:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9E6D6608C\n\tfor ;\n\tThu, 5 Dec 2024 20:05:01 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A2E4E2B3;\n\tThu, 5 Dec 2024 20:04:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BT0FvKae\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733425472;\n\tbh=leoTDMh3Ae7EJBvgn1o27ocjraKISH7sFaSVBF6KO9U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BT0FvKae9eCNCx9gjkpsNE5jI/jLn8LCo4iWDngqXqbI2wAm92QthRNApUQ1UYJFM\n\t9DTgxXJWcEJ+DPAw3EOvb9oEeMKda8ttu0KtA7qkOCzryO1cNYd40SHS2ozt0Rdnlv\n\tqStbDeadR9PB0b9w1Dd79nfyhL3JYoxg/yzE5iB0=","Date":"Thu, 5 Dec 2024 20:04:58 +0100","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 1/9] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","Message-ID":"","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-2-chenghaoyang@chromium.org>\n\t\n\t\n\t<7wgvh2ukgfron75f3cjnrfmn6yzqec6z6f4avlnz7u6ltdhnbp@ghsdq2a4lbnb>\n\t\n\t\n\t<27xrxiauhjwjhzntw3tbdrguzsbzjqtcfeh2xd2naiqcurhnlj@6ntery3oesby>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]