[{"id":32131,"web_url":"https://patchwork.libcamera.org/comment/32131/","msgid":"","date":"2024-11-13T07:00:52","subject":"Re: [PATCH 1/1] 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":"Gentle ping: May we review this patch recently, so that I can continue\nuploading the corresponding patches in Android Adapter?\nThanks :)\n\nBR,\nHarvey\n\nOn Thu, Sep 12, 2024 at 12:57 PM Harvey Yang wrote:\n>\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 metadataCompleted;\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/request.h | 5 +++\n> src/libcamera/camera.cpp | 6 +++\n> src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++\n> src/libcamera/request.cpp | 20 ++++++++++\n> 6 files changed, 70 insertions(+)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 94cee7bd..08c5c58c 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 metadataCompleted;\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 0d380803..1af63a23 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> bool completeBuffer(Request *request, FrameBuffer *buffer);\n> void completeRequest(Request *request);\n>\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index e214a9d1..0200f4a1 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> #include \n> #include \n> @@ -64,6 +65,8 @@ public:\n>\n> std::string toString() const;\n>\n> + ControlList addCompletedMetadata(const ControlList &metadata);\n> +\n> private:\n> LIBCAMERA_DISABLE_COPY(Request)\n>\n> @@ -73,6 +76,8 @@ private:\n>\n> const uint64_t cookie_;\n> Status status_;\n> +\n> + std::unordered_set completedMetadata_;\n> };\n>\n> std::ostream &operator<<(std::ostream &out, const Request &r);\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index a86f552a..5ffae23e 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::metadataCompleted\n> + * \\brief Signal emitted when some metadata for a request is completed 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 e5940469..5d2999cb 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -535,6 +535,28 @@ 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 buffer belongs to\n> + * \\param[in] metadata The partial metadata that has completed\n> + *\n> + * This function could be called by pipeline handlers to signal completion of\n> + * the \\a metadata part of the \\a request. It notifies applications of metadata\n> + * completion.\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->addCompletedMetadata(metadata);\n> + if (!validMetadata.empty()) {\n> + request->metadata().merge(validMetadata);\n> +\n> + Camera *camera = request->_d()->camera();\n> + camera->metadataCompleted.emit(request, validMetadata);\n> + }\n> +}\n> +\n> /**\n> * \\brief Signal request completion\n> * \\param[in] request The request that has completed\n> @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n>\n> Camera::Private *data = camera->_d();\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> + const ControlList &validMetadata = request->addCompletedMetadata(\n> + request->metadata());\n> + if (!validMetadata.empty())\n> + camera->metadataCompleted.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 8c56ed30..3ce23a8e 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -598,6 +598,26 @@ std::string Request::toString() const\n> return ss.str();\n> }\n>\n> +/**\n> + * \\brief Add completed metadata, as a partial result\n> + * \\param[in] metadata The metadata completed\n> + *\n> + * Request will record the entries that has been sent to the application, to\n> + * prevent duplicated controls.\n> + *\n> + * \\return ControlList that hasn't been completed before\n> + */\n> +ControlList Request::addCompletedMetadata(const ControlList &metadata)\n> +{\n> + ControlList resultMetadata;\n> + for (auto &[id, value] : metadata) {\n> + if (!completedMetadata_.count(id))\n> + resultMetadata.set(id, value);\n> + }\n> +\n> + return resultMetadata;\n> +}\n> +\n> /**\n> * \\brief Insert a text representation of a Request into an output stream\n> * \\param[in] out The output stream\n> --\n> 2.46.0.598.g6f2099f65c-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 08CC0C324C\n\tfor ;\n\tWed, 13 Nov 2024 07:01:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F4BE657FE;\n\tWed, 13 Nov 2024 08:01:06 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EE71657FA\n\tfor ;\n\tWed, 13 Nov 2024 08:01:04 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2fb443746b8so50609831fa.0\n\tfor ;\n\tTue, 12 Nov 2024 23:01: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=\"VMCSKB6v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1731481263; x=1732086063;\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=9kBiYua79kRlK/pk+m8UQmnK9jjMzam13G0JIap/GDg=;\n\tb=VMCSKB6vL8qvGo6/La1JVFypWQMzeDuDIamu4/QH7upHumBDoocoeIeTg2nnAAKxpl\n\tr0DPg96LmA81KQvhdXOe/Hu8mmrvi8PRcyttJy9QQFeWkJfXGUTKqul230R44+cCR1aK\n\tmWRwz+4d8IvwPmKbvFIs/cZ+WghxnmjDMI81U=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731481263; x=1732086063;\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=9kBiYua79kRlK/pk+m8UQmnK9jjMzam13G0JIap/GDg=;\n\tb=mWeaqgCjNuqCH+CdVJP4QLkXFaEqcBHHB5pBdWZSpIItTmvVIGMr9vhkOGDGKE3w8v\n\ttusmlniVepIQpE460FGkUgUDHWf2Ti9s07NbjKOyl5qDlfvs7u7utmKpMochlID/kQ1Q\n\tYUBrMk6NM2QFOLP3gnqG47riCKRDmW8wxcYCXzkDHbMXONG+JB1OgHCJvbdZuBWAGgjo\n\tUQYLeu/5vSqR3jsTq4/trAxf85HQH7ELbPrHlPE9NrqWxursX6kF2cO5gEeKkjIbOHAh\n\tM4JcO0ikoQvJ4gele9iz6zcn+jgUQyesAA4HdXmmJDnKYB7enmEBFjDX9r1SQSNe51qh\n\tyVrg==","X-Gm-Message-State":"AOJu0YwdVOkA5rECFasG+bzzwj0HCA6awLBLIiyqDprC0Yy0ja+f86xL\n\tLNnFpRMMe+Lhc6RHEKvWQhBrvpToLrE+ZfMA6WElvuVuSwp8UwoKPBX8C2OkwvlBj1MXi06UCAV\n\t3WjUZiMTcVcxaey5pngxjMIFh5dIMKIqsj7RWRa1qvk2Es8g=","X-Google-Smtp-Source":"AGHT+IHcj8OO5yQGfWbSVFMWWWRSwLh3baY1oVjUZNc26FJi3si8e+oz/Zge8DyGYOz2JmwqJ7TtiE7R6nsxstN0MDw=","X-Received":"by 2002:a2e:a99e:0:b0:2fb:4951:18a with SMTP id\n\t38308e7fff4ca-2ff203094dcmr105078341fa.37.1731481263257;\n\tTue, 12 Nov 2024 23:01:03 -0800 (PST)","MIME-Version":"1.0","References":"<20240912045703.3446748-1-chenghaoyang@google.com>\n\t<20240912045703.3446748-2-chenghaoyang@google.com>","In-Reply-To":"<20240912045703.3446748-2-chenghaoyang@google.com>","From":"Cheng-Hao Yang ","Date":"Wed, 13 Nov 2024 15:00:52 +0800","Message-ID":"","Subject":"Re: [PATCH 1/1] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","To":"libcamera-devel@lists.libcamera.org","Cc":"Han-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":32141,"web_url":"https://patchwork.libcamera.org/comment/32141/","msgid":"","date":"2024-11-13T10:46:58","subject":"Re: [PATCH 1/1] 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 sorry for the late feedback\n\nOn Thu, Sep 12, 2024 at 04:52:48AM +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 metadataCompleted;\n\nBikeshedding on the name a bit, as this is allowed to be called\nmultiple times, with partial metadata, I would call it\n'metadataAvailable'.\n\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/request.h | 5 +++\n> src/libcamera/camera.cpp | 6 +++\n> src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++\n> src/libcamera/request.cpp | 20 ++++++++++\n> 6 files changed, 70 insertions(+)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 94cee7bd..08c5c58c 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 metadataCompleted;\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 0d380803..1af63a23 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> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> \tvoid completeRequest(Request *request);\n>\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index e214a9d1..0200f4a1 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> #include \n> #include \n> @@ -64,6 +65,8 @@ public:\n>\n> \tstd::string toString() const;\n>\n> +\tControlList addCompletedMetadata(const ControlList &metadata);\n> +\n> private:\n> \tLIBCAMERA_DISABLE_COPY(Request)\n>\n> @@ -73,6 +76,8 @@ private:\n>\n> \tconst uint64_t cookie_;\n> \tStatus status_;\n> +\n> +\tstd::unordered_set completedMetadata_;\n> };\n>\n> std::ostream &operator<<(std::ostream &out, const Request &r);\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index a86f552a..5ffae23e 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::metadataCompleted\n> + * \\brief Signal emitted when some metadata for a request is completed as a\n> + * partial result\n\nCurrently, we return metadata as part of a Request signalled with\nCamera::requestCompleted. Existing applications do not handle the new\nsignal and I don't think we should make it mandatory to do so.\nAccording to the implementation of this patch, the same application\nrunning on a pipeline handler that calls\nPipelineHandler::completeMetadata and on a pipeline handler that\ndoesn't will receive different sets of metadata.\n\n- If a pipeline handler calls PipelineHandler::completeMetadata() it\n will send metadata in chunks and only the ones not previously\n signalled with be part of Request::metadata().\n\n- If a pipeline handler doesn't call PipelineHandler::completeMetadata()\n the full metadata buffer will be in Request::metadata().\n\nHow does an application know what to expect ?\n\nImo we should advertise if the camera supports partial metadata or not\n(maybe through a property). If it does applications should opt-in to\nreceive partial metadata. This could even be done per-configuration by\nadding a field to CameraConfiguration ?\n\nIf applications enable early metdata completion they agree to handle\nthe new signal, and the behaviour will be the one implemented in this\npatch. If they do not enable early metadata completion then\nPipelineHandler::completeMetadata() should simply merge the partial\nmetadata in Request::metadata() and not emit the new signal but return\nthe full metadata buffer as part of the Request in requestCompleted,\nas they currently do so that pipelines that use early completion and\npipelines that do not will behave identically from an application\nperspective.\n\nWhat do you think ?\n\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 e5940469..5d2999cb 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -535,6 +535,28 @@ 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 buffer belongs to\n\n * \\param[in] request The Request the metadata buffer belongs to\n\n\n> + * \\param[in] metadata The partial metadata that has completed\n> + *\n> + * This function could be called by pipeline handlers to signal completion of\n> + * the \\a metadata part of the \\a request. It notifies applications of metadata\n> + * completion.\n\nI think a little more details are needed.\n\n * This function could be called by pipeline handlers to signal\n * availability of \\a metadata before \\a request completes. Early\n * metadata completion allows to notify applications about the\n * availability of a partial metadata buffer before the associated\n * Request has completed.\n\n1) According to this implementation metadata can be completed\nout-of-order (partial metadata for Request X+1 can be signalled before\nRequest X completes). Is this desirable ?\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->addCompletedMetadata(metadata);\n\nRequest::addCompleteMetadata constructs and returns a ControlList,\nI'm surprised the compiler doesn't complain you're taking a reference\nto a temporary object (it's apparently legal for const references only).\n\nHowever, as a temporary will be constructed anyway this doesn't\nbuy you any gain, possibily the contrary, as this will disallow C++\ncopy elision from happening (afaiu). I think you should drop &\n\n\n> +\tif (!validMetadata.empty()) {\n> +\t\trequest->metadata().merge(validMetadata);\n> +\n> +\t\tCamera *camera = request->_d()->camera();\n> +\t\tcamera->metadataCompleted.emit(request, validMetadata);\n> +\t}\n> +}\n> +\n> /**\n> * \\brief Signal request completion\n> * \\param[in] request The request that has completed\n> @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n>\n> \tCamera::Private *data = camera->_d();\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> +\t */\n> +\tconst ControlList &validMetadata = request->addCompletedMetadata(\n> +\t\trequest->metadata());\n> +\tif (!validMetadata.empty())\n> +\t\tcamera->metadataCompleted.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 8c56ed30..3ce23a8e 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -598,6 +598,26 @@ std::string Request::toString() const\n> \treturn ss.str();\n> }\n>\n> +/**\n> + * \\brief Add completed metadata, as a partial result\n> + * \\param[in] metadata The metadata completed\n> + *\n> + * Request will record the entries that has been sent to the application, to\n> + * prevent duplicated controls.\n> + *\n> + * \\return ControlList that hasn't been completed before\n> + */\n> +ControlList Request::addCompletedMetadata(const ControlList &metadata)\n> +{\n> +\tControlList resultMetadata;\n> +\tfor (auto &[id, value] : metadata) {\n> +\t\tif (!completedMetadata_.count(id))\n\ncompletedMetadata_ should be cleared in Request::reset().\n\nI wonder if we should actually prevent the same metadata to be\nreturned multiple times by the pipeline handler or not.\n\n> +\t\t\tresultMetadata.set(id, value);\n> +\t}\n> +\n> +\treturn resultMetadata;\n> +}\n> +\n> /**\n> * \\brief Insert a text representation of a Request into an output stream\n> * \\param[in] out The output stream\n> --\n> 2.46.0.598.g6f2099f65c-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 45F21C324C\n\tfor ;\n\tWed, 13 Nov 2024 10:47:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 067C965809;\n\tWed, 13 Nov 2024 11:47:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B1BD6580A\n\tfor ;\n\tWed, 13 Nov 2024 11:47:03 +0100 (CET)","from ideasonboard.com (net-93-150-226-204.cust.dsl.teletu.it\n\t[93.150.226.204])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CBD78752;\n\tWed, 13 Nov 2024 11:46:49 +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=\"wfIt+r3i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731494810;\n\tbh=MW6zI8U+HPs4R9H4cp28LzDwYRiGOYEOIGqgYenk3OU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wfIt+r3igL7vU+EJ4/PvZFf0IZLQlLd1eMpB3Whd1dVWu03rmLTtjcTKpId/tdP5q\n\t2wKwW7amg4Z8ue4OYPszvjn3lc/NkQbLDfGmDhJPk63fQZiHL8NBI4KiMqtTXPr5KN\n\t8eiIDzS7iL5beT6CNuppEJznxuH23GjA/Ur8pzjM=","Date":"Wed, 13 Nov 2024 11:46:58 +0100","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Subject":"Re: [PATCH 1/1] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","Message-ID":"","References":"<20240912045703.3446748-1-chenghaoyang@google.com>\n\t<20240912045703.3446748-2-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240912045703.3446748-2-chenghaoyang@google.com>","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":32158,"web_url":"https://patchwork.libcamera.org/comment/32158/","msgid":"","date":"2024-11-14T06:29:20","subject":"Re: [PATCH 1/1] 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\nI'll upload a new version when all discussion is done.\n\nOn Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey,\n> sorry for the late feedback\n>\n> On Thu, Sep 12, 2024 at 04:52:48AM +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 metadataCompleted;\n>\n> Bikeshedding on the name a bit, as this is allowed to be called\n> multiple times, with partial metadata, I would call it\n> 'metadataAvailable'.\n\nSure, 'metadataAvailable' does make more sense.\nThanks!\n\n>\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/request.h | 5 +++\n> > src/libcamera/camera.cpp | 6 +++\n> > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++\n> > src/libcamera/request.cpp | 20 ++++++++++\n> > 6 files changed, 70 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 94cee7bd..08c5c58c 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 metadataCompleted;\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 0d380803..1af63a23 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> > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > void completeRequest(Request *request);\n> >\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index e214a9d1..0200f4a1 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> > #include \n> > #include \n> > @@ -64,6 +65,8 @@ public:\n> >\n> > std::string toString() const;\n> >\n> > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > +\n> > private:\n> > LIBCAMERA_DISABLE_COPY(Request)\n> >\n> > @@ -73,6 +76,8 @@ private:\n> >\n> > const uint64_t cookie_;\n> > Status status_;\n> > +\n> > + std::unordered_set completedMetadata_;\n> > };\n> >\n> > std::ostream &operator<<(std::ostream &out, const Request &r);\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index a86f552a..5ffae23e 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::metadataCompleted\n> > + * \\brief Signal emitted when some metadata for a request is completed as a\n> > + * partial result\n>\n> Currently, we return metadata as part of a Request signalled with\n> Camera::requestCompleted. Existing applications do not handle the new\n> signal and I don't think we should make it mandatory to do so.\n> According to the implementation of this patch, the same application\n> running on a pipeline handler that calls\n> PipelineHandler::completeMetadata and on a pipeline handler that\n> doesn't will receive different sets of metadata.\n>\n> - If a pipeline handler calls PipelineHandler::completeMetadata() it\n> will send metadata in chunks and only the ones not previously\n> signalled with be part of Request::metadata().\n\nActually, it's not true.\nIn this patch, `PipelineHandler::completeMetadata` will merge the\npartially available metadata into Request:\n`request->metadata().merge(validMetadata);`\n\nTherefore, whether an application handles the new signal, it can\nstill get access to all the metadata in a request via\n`Request.metadata()`. This should ease your concern of the old\nand new implementations of applications.\n\n>\n> - If a pipeline handler doesn't call PipelineHandler::completeMetadata()\n> the full metadata buffer will be in Request::metadata().\n>\n> How does an application know what to expect ?\n>\n> Imo we should advertise if the camera supports partial metadata or not\n> (maybe through a property). If it does applications should opt-in to\n> receive partial metadata. This could even be done per-configuration by\n> adding a field to CameraConfiguration ?\n>\n> If applications enable early metdata completion they agree to handle\n> the new signal, and the behaviour will be the one implemented in this\n> patch. If they do not enable early metadata completion then\n> PipelineHandler::completeMetadata() should simply merge the partial\n> metadata in Request::metadata() and not emit the new signal but return\n> the full metadata buffer as part of the Request in requestCompleted,\n> as they currently do so that pipelines that use early completion and\n> pipelines that do not will behave identically from an application\n> perspective.\n>\n> What do you think ?\n>\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 e5940469..5d2999cb 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -535,6 +535,28 @@ 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 buffer belongs to\n>\n> * \\param[in] request The Request the metadata buffer belongs to\n\nPerhaps:\n`* \\param[in] request The Request the metadata belongs to`\n?\n\n>\n>\n> > + * \\param[in] metadata The partial metadata that has completed\n> > + *\n> > + * This function could be called by pipeline handlers to signal completion of\n> > + * the \\a metadata part of the \\a request. It notifies applications of metadata\n> > + * completion.\n>\n> I think a little more details are needed.\n>\n> * This function could be called by pipeline handlers to signal\n> * availability of \\a metadata before \\a request completes. Early\n> * metadata completion allows to notify applications about the\n> * availability of a partial metadata buffer before the associated\n> * Request has completed.\n\nThanks! Updated.\n\n>\n> 1) According to this implementation metadata can be completed\n> out-of-order (partial metadata for Request X+1 can be signalled before\n> Request X completes). Is this desirable ?\n\nYes, this is what we expect, and Android also allows that.\nWDYT?\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->addCompletedMetadata(metadata);\n>\n> Request::addCompleteMetadata constructs and returns a ControlList,\n> I'm surprised the compiler doesn't complain you're taking a reference\n> to a temporary object (it's apparently legal for const references only).\n>\n> However, as a temporary will be constructed anyway this doesn't\n> buy you any gain, possibily the contrary, as this will disallow C++\n> copy elision from happening (afaiu). I think you should drop &\n\nRight, it's definitely a mistake. Removed.\n\n>\n>\n> > + if (!validMetadata.empty()) {\n> > + request->metadata().merge(validMetadata);\n> > +\n> > + Camera *camera = request->_d()->camera();\n> > + camera->metadataCompleted.emit(request, validMetadata);\n> > + }\n> > +}\n> > +\n> > /**\n> > * \\brief Signal request completion\n> > * \\param[in] request The request that has completed\n> > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> >\n> > Camera::Private *data = camera->_d();\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> > + const ControlList &validMetadata = request->addCompletedMetadata(\n> > + request->metadata());\n> > + if (!validMetadata.empty())\n> > + camera->metadataCompleted.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 8c56ed30..3ce23a8e 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -598,6 +598,26 @@ std::string Request::toString() const\n> > return ss.str();\n> > }\n> >\n> > +/**\n> > + * \\brief Add completed metadata, as a partial result\n> > + * \\param[in] metadata The metadata completed\n> > + *\n> > + * Request will record the entries that has been sent to the application, to\n> > + * prevent duplicated controls.\n> > + *\n> > + * \\return ControlList that hasn't been completed before\n> > + */\n> > +ControlList Request::addCompletedMetadata(const ControlList &metadata)\n> > +{\n> > + ControlList resultMetadata;\n> > + for (auto &[id, value] : metadata) {\n> > + if (!completedMetadata_.count(id))\n>\n> completedMetadata_ should be cleared in Request::reset().\n\nNice catch! Then how about we move the member variable and the member\nfunction to `Request::Private` instead?\n\n>\n> I wonder if we should actually prevent the same metadata to be\n> returned multiple times by the pipeline handler or not.\n\nThis is actually prohibited by Android camera hal [1]:\n`Partial metadata submitted should not include any metadata key\nreturned in a previous partial result for a given frame.`\n\nWe can certainly prevent that in Android Adapter only, while I think\nhaving this restriction in libcamera core library also makes sense.\nWDYT? I can also update the description of the new signal.\n\n[1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL.\n\nBR,\nHarvey\n>\n> > + resultMetadata.set(id, value);\n> > + }\n> > +\n> > + return resultMetadata;\n> > +}\n> > +\n> > /**\n> > * \\brief Insert a text representation of a Request into an output stream\n> > * \\param[in] out The output stream\n> > --\n> > 2.46.0.598.g6f2099f65c-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 0C2B2C324C\n\tfor ;\n\tThu, 14 Nov 2024 06:29:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB6656582F;\n\tThu, 14 Nov 2024 07:29:34 +0100 (CET)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 72F92618BC\n\tfor ;\n\tThu, 14 Nov 2024 07:29:32 +0100 (CET)","by mail-lj1-x232.google.com with SMTP id\n\t38308e7fff4ca-2f75c56f16aso2057971fa.0\n\tfor ;\n\tWed, 13 Nov 2024 22:29:32 -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=\"Xm6zlhwP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1731565771; x=1732170571;\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=1gn8PEYZKC88ntlzUI7VofirsqTkLkUCVL0i6Wlszj0=;\n\tb=Xm6zlhwPoV4PX37eI8x3NGEPyC1AaC8mK3auorYsunu5XqfsiuTr7nroSTcPNpX1+M\n\tMSejWk1X7cH+TV0yPhOfJ6n65rBcXdUI15u8znXUEe008rSirqDaT+WTF1aiLFVICfPJ\n\tYdj1fIGNW9aay3vGw9JekSRHVZ9JURC9G55LY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731565771; x=1732170571;\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=1gn8PEYZKC88ntlzUI7VofirsqTkLkUCVL0i6Wlszj0=;\n\tb=Cvgoo7oIlyGzpyX8UWPzpsQlav2vQ2MrvJClSvryh5zQy2p2bcJhRZRXpm3Uyexm4m\n\tVc21YGspN02KfGdAGXgpslVJ+czBnyj2BDZj4wJ2aVlW9HzPvMTRUMbJS5PHEfClKrSK\n\tvWD8ZdLeRo+dKQo9s+VJTUShnvaF3C6l9IuO/CgKhXDePVb0p1wh9hiiVQ+jd0jTZoUy\n\tGBGlQL4ZmlDXVPCv80mFlqDXT0HzS7AnmrGsdfKchowde6AYsl4lDtTP2lSRw+RzxHfx\n\te0axzRzF0JZ+04BXxXC1q/NhQOo9Dux8Zm5Vog7+iLusSOLU/viPbExpcECyMuM9zYXZ\n\tM0gg==","X-Gm-Message-State":"AOJu0YyWMKgn6iq7mkoCVg4Zg9JVaFpi2gKu5qHn9SlYqQ5Y9Cqjcz0P\n\trgPJYMQ/onxEMpir4U0GXJndQZsFU+sH6EvcabpZLypLnas35Q9Rz0uS0mpOYqm7FcG/CHc1tJ4\n\teH8aypOEHna5GFw6VOEZwSkOPpLxFpTImCSEa","X-Google-Smtp-Source":"AGHT+IEDTs6NWJKO4gszXQfFp7CfpbCB8PcjNHLaj3bUJmgSzVFUSuqGxBK2cp3BIvJtamGF2qSHxb3RuQjQDxrNqtY=","X-Received":"by 2002:a05:651c:4118:b0:2ff:4ce0:d233 with SMTP id\n\t38308e7fff4ca-2ff4ce0d3ffmr23643871fa.28.1731565771364;\n\tWed, 13 Nov 2024 22:29:31 -0800 (PST)","MIME-Version":"1.0","References":"<20240912045703.3446748-1-chenghaoyang@google.com>\n\t<20240912045703.3446748-2-chenghaoyang@google.com>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 14 Nov 2024 14:29:20 +0800","Message-ID":"","Subject":"Re: [PATCH 1/1] 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":32160,"web_url":"https://patchwork.libcamera.org/comment/32160/","msgid":"<6mh4oluzbek2izgwbfxmvkcoklfgbxejzgk26olzmhmoukkki2@myvfc2adwjhf>","date":"2024-11-14T08:36:35","subject":"Re: [PATCH 1/1] 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, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> I'll upload a new version when all discussion is done.\n>\n> On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Harvey,\n> > sorry for the late feedback\n> >\n> > On Thu, Sep 12, 2024 at 04:52:48AM +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 metadataCompleted;\n> >\n> > Bikeshedding on the name a bit, as this is allowed to be called\n> > multiple times, with partial metadata, I would call it\n> > 'metadataAvailable'.\n>\n> Sure, 'metadataAvailable' does make more sense.\n> Thanks!\n>\n> >\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/request.h | 5 +++\n> > > src/libcamera/camera.cpp | 6 +++\n> > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++\n> > > src/libcamera/request.cpp | 20 ++++++++++\n> > > 6 files changed, 70 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 94cee7bd..08c5c58c 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 metadataCompleted;\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 0d380803..1af63a23 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> > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > void completeRequest(Request *request);\n> > >\n> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index e214a9d1..0200f4a1 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> > > #include \n> > > #include \n> > > @@ -64,6 +65,8 @@ public:\n> > >\n> > > std::string toString() const;\n> > >\n> > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > +\n> > > private:\n> > > LIBCAMERA_DISABLE_COPY(Request)\n> > >\n> > > @@ -73,6 +76,8 @@ private:\n> > >\n> > > const uint64_t cookie_;\n> > > Status status_;\n> > > +\n> > > + std::unordered_set completedMetadata_;\n> > > };\n> > >\n> > > std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index a86f552a..5ffae23e 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::metadataCompleted\n> > > + * \\brief Signal emitted when some metadata for a request is completed as a\n> > > + * partial result\n> >\n> > Currently, we return metadata as part of a Request signalled with\n> > Camera::requestCompleted. Existing applications do not handle the new\n> > signal and I don't think we should make it mandatory to do so.\n> > According to the implementation of this patch, the same application\n> > running on a pipeline handler that calls\n> > PipelineHandler::completeMetadata and on a pipeline handler that\n> > doesn't will receive different sets of metadata.\n> >\n> > - If a pipeline handler calls PipelineHandler::completeMetadata() it\n> > will send metadata in chunks and only the ones not previously\n> > signalled with be part of Request::metadata().\n>\n> Actually, it's not true.\n> In this patch, `PipelineHandler::completeMetadata` will merge the\n> partially available metadata into Request:\n> `request->metadata().merge(validMetadata);`\n>\n\nUps, sorry I missed it!\n\n> Therefore, whether an application handles the new signal, it can\n> still get access to all the metadata in a request via\n> `Request.metadata()`. This should ease your concern of the old\n> and new implementations of applications.\n>\n\nYes, if this time I got this right, the full metadata pack will be\navailable in Request::metadata() at requestCompleted time, but it will\nbe completed in chunks if the pipeline handler calls\ncompleteMetadata() earlier.\n\nThere's only one thing left that I'm not sure about: in\nrequestCompleted()\n\n\tconst ControlList &validMetadata = request->addCompletedMetadata(\n\t\trequest->metadata());\n\tif (!validMetadata.empty())\n\t\tcamera->metadataCompleted.emit(request, validMetadata);\n\nIf the pipeline handler never calls completeMetadata() then\nvalidMetadata == request->metadata(). So that applications will\nreceive the same metadata buffer in metadataCompleted and\nrequestCompleted. I wonder if we should emit the metadataCompleted\nsignal at all if the pipeline handler never calls completeMetadata()\n(it would be easy to check, just make sure that Request:completedMetadata_\nis empty with an helper function).\n\n> >\n> > - If a pipeline handler doesn't call PipelineHandler::completeMetadata()\n> > the full metadata buffer will be in Request::metadata().\n> >\n> > How does an application know what to expect ?\n> >\n> > Imo we should advertise if the camera supports partial metadata or not\n> > (maybe through a property). If it does applications should opt-in to\n> > receive partial metadata. This could even be done per-configuration by\n> > adding a field to CameraConfiguration ?\n> >\n> > If applications enable early metdata completion they agree to handle\n> > the new signal, and the behaviour will be the one implemented in this\n> > patch. If they do not enable early metadata completion then\n> > PipelineHandler::completeMetadata() should simply merge the partial\n> > metadata in Request::metadata() and not emit the new signal but return\n> > the full metadata buffer as part of the Request in requestCompleted,\n> > as they currently do so that pipelines that use early completion and\n> > pipelines that do not will behave identically from an application\n> > perspective.\n> >\n> > What do you think ?\n> >\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 e5940469..5d2999cb 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -535,6 +535,28 @@ 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 buffer belongs to\n> >\n> > * \\param[in] request The Request the metadata buffer belongs to\n>\n> Perhaps:\n> `* \\param[in] request The Request the metadata belongs to`\n> ?\n>\n\nack\n\n> >\n> >\n> > > + * \\param[in] metadata The partial metadata that has completed\n> > > + *\n> > > + * This function could be called by pipeline handlers to signal completion of\n> > > + * the \\a metadata part of the \\a request. It notifies applications of metadata\n> > > + * completion.\n> >\n> > I think a little more details are needed.\n> >\n> > * This function could be called by pipeline handlers to signal\n> > * availability of \\a metadata before \\a request completes. Early\n> > * metadata completion allows to notify applications about the\n> > * availability of a partial metadata buffer before the associated\n> > * Request has completed.\n>\n> Thanks! Updated.\n>\n> >\n> > 1) According to this implementation metadata can be completed\n> > out-of-order (partial metadata for Request X+1 can be signalled before\n> > Request X completes). Is this desirable ?\n>\n> Yes, this is what we expect, and Android also allows that.\n> WDYT?\n>\n\nI'll ask others what to they think\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->addCompletedMetadata(metadata);\n> >\n> > Request::addCompleteMetadata constructs and returns a ControlList,\n> > I'm surprised the compiler doesn't complain you're taking a reference\n> > to a temporary object (it's apparently legal for const references only).\n> >\n> > However, as a temporary will be constructed anyway this doesn't\n> > buy you any gain, possibily the contrary, as this will disallow C++\n> > copy elision from happening (afaiu). I think you should drop &\n>\n> Right, it's definitely a mistake. Removed.\n>\n> >\n> >\n> > > + if (!validMetadata.empty()) {\n> > > + request->metadata().merge(validMetadata);\n> > > +\n> > > + Camera *camera = request->_d()->camera();\n> > > + camera->metadataCompleted.emit(request, validMetadata);\n> > > + }\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Signal request completion\n> > > * \\param[in] request The request that has completed\n> > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > >\n> > > Camera::Private *data = camera->_d();\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\nCould you please clarify me what this \\todo means ?\n\n> > > + const ControlList &validMetadata = request->addCompletedMetadata(\n> > > + request->metadata());\n> > > + if (!validMetadata.empty())\n> > > + camera->metadataCompleted.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 8c56ed30..3ce23a8e 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -598,6 +598,26 @@ std::string Request::toString() const\n> > > return ss.str();\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Add completed metadata, as a partial result\n> > > + * \\param[in] metadata The metadata completed\n> > > + *\n> > > + * Request will record the entries that has been sent to the application, to\n> > > + * prevent duplicated controls.\n> > > + *\n> > > + * \\return ControlList that hasn't been completed before\n> > > + */\n> > > +ControlList Request::addCompletedMetadata(const ControlList &metadata)\n> > > +{\n> > > + ControlList resultMetadata;\n> > > + for (auto &[id, value] : metadata) {\n> > > + if (!completedMetadata_.count(id))\n> >\n> > completedMetadata_ should be cleared in Request::reset().\n>\n> Nice catch! Then how about we move the member variable and the member\n> function to `Request::Private` instead?\n>\n\nYes, they both do not need to be exposed to applications indeed.\n\n> >\n> > I wonder if we should actually prevent the same metadata to be\n> > returned multiple times by the pipeline handler or not.\n>\n> This is actually prohibited by Android camera hal [1]:\n> `Partial metadata submitted should not include any metadata key\n> returned in a previous partial result for a given frame.`\n>\n> We can certainly prevent that in Android Adapter only, while I think\n> having this restriction in libcamera core library also makes sense.\n> WDYT? I can also update the description of the new signal.\n>\n> [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL.\n\nI can't find in the text where this is forbidden, however I'm not\npushing for the other option, I think it's actually saner to only\nsignal a metadata once per Request. I wonder however if this shouldn't\nbe a WARN as it means the pipeline handler is doing something\nunexpected. Also, the expectations that a metadata is returned once\nper Request has to be documented in the documentation of\nPipelineHandler::completeMetadata()\n\nThanks\n j\n\n>\n> BR,\n> Harvey\n> >\n> > > + resultMetadata.set(id, value);\n> > > + }\n> > > +\n> > > + return resultMetadata;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Insert a text representation of a Request into an output stream\n> > > * \\param[in] out The output stream\n> > > --\n> > > 2.46.0.598.g6f2099f65c-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 C8502C324C\n\tfor ;\n\tThu, 14 Nov 2024 08:36:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C545A65833;\n\tThu, 14 Nov 2024 09:36: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 7740A657DA\n\tfor ;\n\tThu, 14 Nov 2024 09:36: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 602C5827;\n\tThu, 14 Nov 2024 09:36:25 +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=\"XYMX2qAU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731573385;\n\tbh=IODrG2W+JuwTiV8MLLOdvo1QhcVhGU6Y7+NQC5ZaJuE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XYMX2qAUCWIvklqZ+tkfgDlW0T2uVK94rY2jqJ5C79BNywLJ1sqhK61hAOBbB1Smc\n\tOFWFsQBEEaSSR89j9gjifPFXhdybkwZ0C2/pNmTsKXZUYgj/IAl6Akx0URaFrXBonm\n\tNz0fxItJRQa7kTTKfDAk2f0qxKXtcLIlLhP2lVHE=","Date":"Thu, 14 Nov 2024 09:36: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 1/1] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","Message-ID":"<6mh4oluzbek2izgwbfxmvkcoklfgbxejzgk26olzmhmoukkki2@myvfc2adwjhf>","References":"<20240912045703.3446748-1-chenghaoyang@google.com>\n\t<20240912045703.3446748-2-chenghaoyang@google.com>\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":32163,"web_url":"https://patchwork.libcamera.org/comment/32163/","msgid":"","date":"2024-11-14T09:11:42","subject":"Re: [PATCH 1/1] 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, Nov 14, 2024 at 4:36 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Thu, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > I'll upload a new version when all discussion is done.\n> >\n> > On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Harvey,\n> > > sorry for the late feedback\n> > >\n> > > On Thu, Sep 12, 2024 at 04:52:48AM +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 metadataCompleted;\n> > >\n> > > Bikeshedding on the name a bit, as this is allowed to be called\n> > > multiple times, with partial metadata, I would call it\n> > > 'metadataAvailable'.\n> >\n> > Sure, 'metadataAvailable' does make more sense.\n> > Thanks!\n> >\n> > >\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/request.h | 5 +++\n> > > > src/libcamera/camera.cpp | 6 +++\n> > > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++\n> > > > src/libcamera/request.cpp | 20 ++++++++++\n> > > > 6 files changed, 70 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > index 94cee7bd..08c5c58c 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 metadataCompleted;\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 0d380803..1af63a23 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> > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > void completeRequest(Request *request);\n> > > >\n> > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > index e214a9d1..0200f4a1 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> > > > #include \n> > > > #include \n> > > > @@ -64,6 +65,8 @@ public:\n> > > >\n> > > > std::string toString() const;\n> > > >\n> > > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > > +\n> > > > private:\n> > > > LIBCAMERA_DISABLE_COPY(Request)\n> > > >\n> > > > @@ -73,6 +76,8 @@ private:\n> > > >\n> > > > const uint64_t cookie_;\n> > > > Status status_;\n> > > > +\n> > > > + std::unordered_set completedMetadata_;\n> > > > };\n> > > >\n> > > > std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index a86f552a..5ffae23e 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::metadataCompleted\n> > > > + * \\brief Signal emitted when some metadata for a request is completed as a\n> > > > + * partial result\n> > >\n> > > Currently, we return metadata as part of a Request signalled with\n> > > Camera::requestCompleted. Existing applications do not handle the new\n> > > signal and I don't think we should make it mandatory to do so.\n> > > According to the implementation of this patch, the same application\n> > > running on a pipeline handler that calls\n> > > PipelineHandler::completeMetadata and on a pipeline handler that\n> > > doesn't will receive different sets of metadata.\n> > >\n> > > - If a pipeline handler calls PipelineHandler::completeMetadata() it\n> > > will send metadata in chunks and only the ones not previously\n> > > signalled with be part of Request::metadata().\n> >\n> > Actually, it's not true.\n> > In this patch, `PipelineHandler::completeMetadata` will merge the\n> > partially available metadata into Request:\n> > `request->metadata().merge(validMetadata);`\n> >\n>\n> Ups, sorry I missed it!\n>\n> > Therefore, whether an application handles the new signal, it can\n> > still get access to all the metadata in a request via\n> > `Request.metadata()`. This should ease your concern of the old\n> > and new implementations of applications.\n> >\n>\n> Yes, if this time I got this right, the full metadata pack will be\n> available in Request::metadata() at requestCompleted time, but it will\n> be completed in chunks if the pipeline handler calls\n> completeMetadata() earlier.\n>\n> There's only one thing left that I'm not sure about: in\n> requestCompleted()\n>\n> const ControlList &validMetadata = request->addCompletedMetadata(\n> request->metadata());\n> if (!validMetadata.empty())\n> camera->metadataCompleted.emit(request, validMetadata);\n>\n> If the pipeline handler never calls completeMetadata() then\n> validMetadata == request->metadata(). So that applications will\n> receive the same metadata buffer in metadataCompleted and\n> requestCompleted. I wonder if we should emit the metadataCompleted\n> signal at all if the pipeline handler never calls completeMetadata()\n> (it would be easy to check, just make sure that Request:completedMetadata_\n> is empty with an helper function).\n\nI think this is how we expect a pipeline handler to behave and how an\napplication to handle signals.\n\nIn the current implementation, it's safer for applications that whether\na pipeline handler calls completeMetadata, applications still get full\nmetadata set with the new signal, and they don't need to rely on\nsignal requestCompleted. In other words, I want to guarantee that\napplications can choose to use either metadataAvailable or\nrequestCompleted.\nTriggering the new signal if there's any metadata left is just a safer\noption to support all pipeline handlers.\n\nOf course we can choose to check if\n`Request::Private::completedMetadata_` is empty, and only handle\nthis case, and expect the proper implementations of pipeline\nhandlers call completeMetadata properly without missing any\nmetadata. I just feel that it's unnecessary.\nAlso, I'm not sure if there's a case that pipeline handler doesn't\nwant to report some metadata earlier, and would rather send\nthem altogether when a request is completed. In this case,\nyour approach would require a pipeline handler to record\nthose unsent metadata on its own.\n\nWDYT?\n\n>\n> > >\n> > > - If a pipeline handler doesn't call PipelineHandler::completeMetadata()\n> > > the full metadata buffer will be in Request::metadata().\n> > >\n> > > How does an application know what to expect ?\n> > >\n> > > Imo we should advertise if the camera supports partial metadata or not\n> > > (maybe through a property). If it does applications should opt-in to\n> > > receive partial metadata. This could even be done per-configuration by\n> > > adding a field to CameraConfiguration ?\n> > >\n> > > If applications enable early metdata completion they agree to handle\n> > > the new signal, and the behaviour will be the one implemented in this\n> > > patch. If they do not enable early metadata completion then\n> > > PipelineHandler::completeMetadata() should simply merge the partial\n> > > metadata in Request::metadata() and not emit the new signal but return\n> > > the full metadata buffer as part of the Request in requestCompleted,\n> > > as they currently do so that pipelines that use early completion and\n> > > pipelines that do not will behave identically from an application\n> > > perspective.\n> > >\n> > > What do you think ?\n> > >\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 e5940469..5d2999cb 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -535,6 +535,28 @@ 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 buffer belongs to\n> > >\n> > > * \\param[in] request The Request the metadata buffer belongs to\n> >\n> > Perhaps:\n> > `* \\param[in] request The Request the metadata belongs to`\n> > ?\n> >\n>\n> ack\n>\n> > >\n> > >\n> > > > + * \\param[in] metadata The partial metadata that has completed\n> > > > + *\n> > > > + * This function could be called by pipeline handlers to signal completion of\n> > > > + * the \\a metadata part of the \\a request. It notifies applications of metadata\n> > > > + * completion.\n> > >\n> > > I think a little more details are needed.\n> > >\n> > > * This function could be called by pipeline handlers to signal\n> > > * availability of \\a metadata before \\a request completes. Early\n> > > * metadata completion allows to notify applications about the\n> > > * availability of a partial metadata buffer before the associated\n> > > * Request has completed.\n> >\n> > Thanks! Updated.\n> >\n> > >\n> > > 1) According to this implementation metadata can be completed\n> > > out-of-order (partial metadata for Request X+1 can be signalled before\n> > > Request X completes). Is this desirable ?\n> >\n> > Yes, this is what we expect, and Android also allows that.\n> > WDYT?\n> >\n>\n> I'll ask others what to they think\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->addCompletedMetadata(metadata);\n> > >\n> > > Request::addCompleteMetadata constructs and returns a ControlList,\n> > > I'm surprised the compiler doesn't complain you're taking a reference\n> > > to a temporary object (it's apparently legal for const references only).\n> > >\n> > > However, as a temporary will be constructed anyway this doesn't\n> > > buy you any gain, possibily the contrary, as this will disallow C++\n> > > copy elision from happening (afaiu). I think you should drop &\n> >\n> > Right, it's definitely a mistake. Removed.\n> >\n> > >\n> > >\n> > > > + if (!validMetadata.empty()) {\n> > > > + request->metadata().merge(validMetadata);\n> > > > +\n> > > > + Camera *camera = request->_d()->camera();\n> > > > + camera->metadataCompleted.emit(request, validMetadata);\n> > > > + }\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Signal request completion\n> > > > * \\param[in] request The request that has completed\n> > > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > > >\n> > > > Camera::Private *data = camera->_d();\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>\n> Could you please clarify me what this \\todo means ?\n\nSimilar to the above reasons:\nCurrently all pipeline handlers merge metadata into request.metadata()\ndirectly, except for the upcoming mtkisp7. We were expecting every\npipeline handler to migrate to `PipelineHandler::completeMetadata`\neventually.\n\nLet me know if you think this todo doesn't make sense, as we should\nstill allow the previous way to merge into request.metadata() directly,\nor there's a case that a pipeline handler doesn't want to report some\nmetadata earlier.\n\n>\n> > > > + const ControlList &validMetadata = request->addCompletedMetadata(\n> > > > + request->metadata());\n> > > > + if (!validMetadata.empty())\n> > > > + camera->metadataCompleted.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 8c56ed30..3ce23a8e 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -598,6 +598,26 @@ std::string Request::toString() const\n> > > > return ss.str();\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Add completed metadata, as a partial result\n> > > > + * \\param[in] metadata The metadata completed\n> > > > + *\n> > > > + * Request will record the entries that has been sent to the application, to\n> > > > + * prevent duplicated controls.\n> > > > + *\n> > > > + * \\return ControlList that hasn't been completed before\n> > > > + */\n> > > > +ControlList Request::addCompletedMetadata(const ControlList &metadata)\n> > > > +{\n> > > > + ControlList resultMetadata;\n> > > > + for (auto &[id, value] : metadata) {\n> > > > + if (!completedMetadata_.count(id))\n> > >\n> > > completedMetadata_ should be cleared in Request::reset().\n> >\n> > Nice catch! Then how about we move the member variable and the member\n> > function to `Request::Private` instead?\n> >\n>\n> Yes, they both do not need to be exposed to applications indeed.\n>\n> > >\n> > > I wonder if we should actually prevent the same metadata to be\n> > > returned multiple times by the pipeline handler or not.\n> >\n> > This is actually prohibited by Android camera hal [1]:\n> > `Partial metadata submitted should not include any metadata key\n> > returned in a previous partial result for a given frame.`\n> >\n> > We can certainly prevent that in Android Adapter only, while I think\n> > having this restriction in libcamera core library also makes sense.\n> > WDYT? I can also update the description of the new signal.\n> >\n> > [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL.\n>\n> I can't find in the text where this is forbidden, however I'm not\n> pushing for the other option, I think it's actually saner to only\n> signal a metadata once per Request. I wonder however if this shouldn't\n> be a WARN as it means the pipeline handler is doing something\n> unexpected.\n\nTrue that a WARN is an option.\nI would prefer the current approach though, otherwise I need to\nimplement the logic in Android Adapter instead.\n\nPlease let me know if anyone insists on the other approach.\n\n> Also, the expectations that a metadata is returned once\n> per Request has to be documented in the documentation of\n> PipelineHandler::completeMetadata()\n\nI'll update the doc in the next version :)\n\nBR,\nHarvey\n\n>\n> Thanks\n> j\n>\n> >\n> > BR,\n> > Harvey\n> > >\n> > > > + resultMetadata.set(id, value);\n> > > > + }\n> > > > +\n> > > > + return resultMetadata;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Insert a text representation of a Request into an output stream\n> > > > * \\param[in] out The output stream\n> > > > --\n> > > > 2.46.0.598.g6f2099f65c-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 74571C0F1B\n\tfor ;\n\tThu, 14 Nov 2024 09:11:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A176F65831;\n\tThu, 14 Nov 2024 10:11:56 +0100 (CET)","from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6218765831\n\tfor ;\n\tThu, 14 Nov 2024 10:11:54 +0100 (CET)","by mail-lj1-x22a.google.com with SMTP id\n\t38308e7fff4ca-2ff57619835so4693761fa.0\n\tfor ;\n\tThu, 14 Nov 2024 01:11:54 -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=\"UtZYMChH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1731575513; x=1732180313;\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=+iFba21av+Az96+oDkeuwQoKI+vpOldOk/AX9/uEMkI=;\n\tb=UtZYMChHM3Tti+QwH0T1i0QN0EJNvKaodWJZC34ssbPg4/IE/6KrzbJJ7F2R729i6H\n\thE/dkftzZCSTiAc6Sit5LggfIDkPvkSD9/HQMrSIpRwlyaihkQ+jZbjkehwmTUR0N77o\n\tJISlOTUBvesfMlkxIM/Ll5Smu2Bak5aCD9Oyc=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731575513; x=1732180313;\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=+iFba21av+Az96+oDkeuwQoKI+vpOldOk/AX9/uEMkI=;\n\tb=GZOCuPXsSLpKebFft2Rfzk8XqtZnmuLsMY0+4h+j2zr5O6rH3EVWoBT7+hy31jQn+v\n\tQ1k06a9HH1WsQ0HOzOUOWhBicmhRGvDGFA8adc+j3tpWk6dg9wcZjy5cqQzXkjsPpSPm\n\tKyULyscszFM23BO2zMim+s33QO7NcFn3DFJiz2NjMrqN3VNACj3G9uQNW7HWJjoFQhnK\n\t0XJwI7O3qFTeXqKhpWUyRFHBy8mxMT1nM4AKNIApjCYUVBXw9hg9TCmjarzhkFAXw5Qp\n\tmaTIa3nU/FPYHB4Uvuia6NcAnM4QpW4SXbkgA5N7jsUF5SgyorYdtubzspODHSIuv/8r\n\tTrSw==","X-Gm-Message-State":"AOJu0Yz81/LESl8ZLZU92DHcBXBC4RtG3olykWl11xQRGUx3wq3SBc1P\n\tSzcXnMtdYojim56dqHrAp3Ybsz0a4bHdRoS7M53j6IAevOyr/2iJyk81Un2nd/VCtD9cKylFMIi\n\tlbiZbs7nImB04WPycF60dZO1hWCxd45mKX53W","X-Google-Smtp-Source":"AGHT+IH2kWpK++thUwE20YItS3SN/WYKV4v1+pFI7hVmrUX/pTZuzQbuLSaz8VEKaVuBMYuHTKWyVgCOQh51em7tTQo=","X-Received":"by 2002:a2e:a555:0:b0:2f7:6062:a0a9 with SMTP id\n\t38308e7fff4ca-2ff5662cb00mr8797241fa.7.1731575513242; Thu, 14 Nov 2024\n\t01:11:53 -0800 (PST)","MIME-Version":"1.0","References":"<20240912045703.3446748-1-chenghaoyang@google.com>\n\t<20240912045703.3446748-2-chenghaoyang@google.com>\n\t\n\t\n\t<6mh4oluzbek2izgwbfxmvkcoklfgbxejzgk26olzmhmoukkki2@myvfc2adwjhf>","In-Reply-To":"<6mh4oluzbek2izgwbfxmvkcoklfgbxejzgk26olzmhmoukkki2@myvfc2adwjhf>","From":"Cheng-Hao Yang ","Date":"Thu, 14 Nov 2024 17:11:42 +0800","Message-ID":"","Subject":"Re: [PATCH 1/1] 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":32166,"web_url":"https://patchwork.libcamera.org/comment/32166/","msgid":"","date":"2024-11-14T10:17:06","subject":"Re: [PATCH 1/1] 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, Nov 14, 2024 at 05:11:42PM +0800, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> On Thu, Nov 14, 2024 at 4:36 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Harvey\n> >\n> > On Thu, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote:\n> > > Hi Jacopo,\n> > >\n> > > I'll upload a new version when all discussion is done.\n> > >\n> > > On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi Harvey,\n> > > > sorry for the late feedback\n> > > >\n> > > > On Thu, Sep 12, 2024 at 04:52:48AM +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 metadataCompleted;\n> > > >\n> > > > Bikeshedding on the name a bit, as this is allowed to be called\n> > > > multiple times, with partial metadata, I would call it\n> > > > 'metadataAvailable'.\n> > >\n> > > Sure, 'metadataAvailable' does make more sense.\n> > > Thanks!\n> > >\n> > > >\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/request.h | 5 +++\n> > > > > src/libcamera/camera.cpp | 6 +++\n> > > > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++\n> > > > > src/libcamera/request.cpp | 20 ++++++++++\n> > > > > 6 files changed, 70 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > > index 94cee7bd..08c5c58c 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 metadataCompleted;\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 0d380803..1af63a23 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> > > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > > void completeRequest(Request *request);\n> > > > >\n> > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > > index e214a9d1..0200f4a1 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> > > > > #include \n> > > > > #include \n> > > > > @@ -64,6 +65,8 @@ public:\n> > > > >\n> > > > > std::string toString() const;\n> > > > >\n> > > > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > > > +\n> > > > > private:\n> > > > > LIBCAMERA_DISABLE_COPY(Request)\n> > > > >\n> > > > > @@ -73,6 +76,8 @@ private:\n> > > > >\n> > > > > const uint64_t cookie_;\n> > > > > Status status_;\n> > > > > +\n> > > > > + std::unordered_set completedMetadata_;\n> > > > > };\n> > > > >\n> > > > > std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index a86f552a..5ffae23e 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::metadataCompleted\n> > > > > + * \\brief Signal emitted when some metadata for a request is completed as a\n> > > > > + * partial result\n> > > >\n> > > > Currently, we return metadata as part of a Request signalled with\n> > > > Camera::requestCompleted. Existing applications do not handle the new\n> > > > signal and I don't think we should make it mandatory to do so.\n> > > > According to the implementation of this patch, the same application\n> > > > running on a pipeline handler that calls\n> > > > PipelineHandler::completeMetadata and on a pipeline handler that\n> > > > doesn't will receive different sets of metadata.\n> > > >\n> > > > - If a pipeline handler calls PipelineHandler::completeMetadata() it\n> > > > will send metadata in chunks and only the ones not previously\n> > > > signalled with be part of Request::metadata().\n> > >\n> > > Actually, it's not true.\n> > > In this patch, `PipelineHandler::completeMetadata` will merge the\n> > > partially available metadata into Request:\n> > > `request->metadata().merge(validMetadata);`\n> > >\n> >\n> > Ups, sorry I missed it!\n> >\n> > > Therefore, whether an application handles the new signal, it can\n> > > still get access to all the metadata in a request via\n> > > `Request.metadata()`. This should ease your concern of the old\n> > > and new implementations of applications.\n> > >\n> >\n> > Yes, if this time I got this right, the full metadata pack will be\n> > available in Request::metadata() at requestCompleted time, but it will\n> > be completed in chunks if the pipeline handler calls\n> > completeMetadata() earlier.\n> >\n> > There's only one thing left that I'm not sure about: in\n> > requestCompleted()\n> >\n> > const ControlList &validMetadata = request->addCompletedMetadata(\n> > request->metadata());\n> > if (!validMetadata.empty())\n> > camera->metadataCompleted.emit(request, validMetadata);\n> >\n> > If the pipeline handler never calls completeMetadata() then\n> > validMetadata == request->metadata(). So that applications will\n> > receive the same metadata buffer in metadataCompleted and\n> > requestCompleted. I wonder if we should emit the metadataCompleted\n> > signal at all if the pipeline handler never calls completeMetadata()\n> > (it would be easy to check, just make sure that Request:completedMetadata_\n> > is empty with an helper function).\n>\n> I think this is how we expect a pipeline handler to behave and how an\n> application to handle signals.\n\nDo you expect all pipelines to use the new signal ?\n\nI quickly checked rkisp1 where most metadata are computed by the IPA\nin one go after parsing statistics. Only the frame timestamp and the\nscaler crop are available at different times (the buffer completion,\nso possibily later than the IPA computed ones).\n\n>\n> In the current implementation, it's safer for applications that whether\n> a pipeline handler calls completeMetadata, applications still get full\n> metadata set with the new signal, and they don't need to rely on\n> signal requestCompleted. In other words, I want to guarantee that\n> applications can choose to use either metadataAvailable or\n> requestCompleted.\n> Triggering the new signal if there's any metadata left is just a safer\n> option to support all pipeline handlers.\n\nMy concern is about sending over a signal a possibly long list of\ncontrols two times in the case all metadata are sent in a single chunk\nat requestComplete() time.\n\n>\n> Of course we can choose to check if\n> `Request::Private::completedMetadata_` is empty, and only handle\n> this case, and expect the proper implementations of pipeline\n> handlers call completeMetadata properly without missing any\n> metadata. I just feel that it's unnecessary.\n> Also, I'm not sure if there's a case that pipeline handler doesn't\n> want to report some metadata earlier, and would rather send\n> them altogether when a request is completed. In this case,\n> your approach would require a pipeline handler to record\n> those unsent metadata on its own.\n>\n> WDYT?\n>\n> >\n> > > >\n> > > > - If a pipeline handler doesn't call PipelineHandler::completeMetadata()\n> > > > the full metadata buffer will be in Request::metadata().\n> > > >\n> > > > How does an application know what to expect ?\n> > > >\n> > > > Imo we should advertise if the camera supports partial metadata or not\n> > > > (maybe through a property). If it does applications should opt-in to\n> > > > receive partial metadata. This could even be done per-configuration by\n> > > > adding a field to CameraConfiguration ?\n> > > >\n> > > > If applications enable early metdata completion they agree to handle\n> > > > the new signal, and the behaviour will be the one implemented in this\n> > > > patch. If they do not enable early metadata completion then\n> > > > PipelineHandler::completeMetadata() should simply merge the partial\n> > > > metadata in Request::metadata() and not emit the new signal but return\n> > > > the full metadata buffer as part of the Request in requestCompleted,\n> > > > as they currently do so that pipelines that use early completion and\n> > > > pipelines that do not will behave identically from an application\n> > > > perspective.\n> > > >\n> > > > What do you think ?\n> > > >\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 e5940469..5d2999cb 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -535,6 +535,28 @@ 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 buffer belongs to\n> > > >\n> > > > * \\param[in] request The Request the metadata buffer belongs to\n> > >\n> > > Perhaps:\n> > > `* \\param[in] request The Request the metadata belongs to`\n> > > ?\n> > >\n> >\n> > ack\n> >\n> > > >\n> > > >\n> > > > > + * \\param[in] metadata The partial metadata that has completed\n> > > > > + *\n> > > > > + * This function could be called by pipeline handlers to signal completion of\n> > > > > + * the \\a metadata part of the \\a request. It notifies applications of metadata\n> > > > > + * completion.\n> > > >\n> > > > I think a little more details are needed.\n> > > >\n> > > > * This function could be called by pipeline handlers to signal\n> > > > * availability of \\a metadata before \\a request completes. Early\n> > > > * metadata completion allows to notify applications about the\n> > > > * availability of a partial metadata buffer before the associated\n> > > > * Request has completed.\n> > >\n> > > Thanks! Updated.\n> > >\n> > > >\n> > > > 1) According to this implementation metadata can be completed\n> > > > out-of-order (partial metadata for Request X+1 can be signalled before\n> > > > Request X completes). Is this desirable ?\n> > >\n> > > Yes, this is what we expect, and Android also allows that.\n> > > WDYT?\n> > >\n> >\n> > I'll ask others what to they think\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->addCompletedMetadata(metadata);\n> > > >\n> > > > Request::addCompleteMetadata constructs and returns a ControlList,\n> > > > I'm surprised the compiler doesn't complain you're taking a reference\n> > > > to a temporary object (it's apparently legal for const references only).\n> > > >\n> > > > However, as a temporary will be constructed anyway this doesn't\n> > > > buy you any gain, possibily the contrary, as this will disallow C++\n> > > > copy elision from happening (afaiu). I think you should drop &\n> > >\n> > > Right, it's definitely a mistake. Removed.\n> > >\n> > > >\n> > > >\n> > > > > + if (!validMetadata.empty()) {\n> > > > > + request->metadata().merge(validMetadata);\n> > > > > +\n> > > > > + Camera *camera = request->_d()->camera();\n> > > > > + camera->metadataCompleted.emit(request, validMetadata);\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Signal request completion\n> > > > > * \\param[in] request The request that has completed\n> > > > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > >\n> > > > > Camera::Private *data = camera->_d();\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> >\n> > Could you please clarify me what this \\todo means ?\n>\n> Similar to the above reasons:\n> Currently all pipeline handlers merge metadata into request.metadata()\n> directly, except for the upcoming mtkisp7. We were expecting every\n> pipeline handler to migrate to `PipelineHandler::completeMetadata`\n> eventually.\n>\n> Let me know if you think this todo doesn't make sense, as we should\n> still allow the previous way to merge into request.metadata() directly,\n> or there's a case that a pipeline handler doesn't want to report some\n> metadata earlier.\n>\n\nI'm just not 100% sure all pipelines will be capable of actually send\nmetadata earlier. True, in the RkISP1 case I used as an example as\nsoon as the IPA has processed stats and computed metadata the pipeline\ncan call metadataComplete() but we have no guarantee the application\nhandles the signal, so we end up sending the same metadata list twice\n(one in chunks, the other as Request::metadata() at requestComplete()\ntime.\n\nI'll check with others what are the implications in terms of\nperformances of this.\n\nThanks\n j\n\n> >\n> > > > > + const ControlList &validMetadata = request->addCompletedMetadata(\n> > > > > + request->metadata());\n> > > > > + if (!validMetadata.empty())\n> > > > > + camera->metadataCompleted.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 8c56ed30..3ce23a8e 100644\n> > > > > --- a/src/libcamera/request.cpp\n> > > > > +++ b/src/libcamera/request.cpp\n> > > > > @@ -598,6 +598,26 @@ std::string Request::toString() const\n> > > > > return ss.str();\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Add completed metadata, as a partial result\n> > > > > + * \\param[in] metadata The metadata completed\n> > > > > + *\n> > > > > + * Request will record the entries that has been sent to the application, to\n> > > > > + * prevent duplicated controls.\n> > > > > + *\n> > > > > + * \\return ControlList that hasn't been completed before\n> > > > > + */\n> > > > > +ControlList Request::addCompletedMetadata(const ControlList &metadata)\n> > > > > +{\n> > > > > + ControlList resultMetadata;\n> > > > > + for (auto &[id, value] : metadata) {\n> > > > > + if (!completedMetadata_.count(id))\n> > > >\n> > > > completedMetadata_ should be cleared in Request::reset().\n> > >\n> > > Nice catch! Then how about we move the member variable and the member\n> > > function to `Request::Private` instead?\n> > >\n> >\n> > Yes, they both do not need to be exposed to applications indeed.\n> >\n> > > >\n> > > > I wonder if we should actually prevent the same metadata to be\n> > > > returned multiple times by the pipeline handler or not.\n> > >\n> > > This is actually prohibited by Android camera hal [1]:\n> > > `Partial metadata submitted should not include any metadata key\n> > > returned in a previous partial result for a given frame.`\n> > >\n> > > We can certainly prevent that in Android Adapter only, while I think\n> > > having this restriction in libcamera core library also makes sense.\n> > > WDYT? I can also update the description of the new signal.\n> > >\n> > > [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL.\n> >\n> > I can't find in the text where this is forbidden, however I'm not\n> > pushing for the other option, I think it's actually saner to only\n> > signal a metadata once per Request. I wonder however if this shouldn't\n> > be a WARN as it means the pipeline handler is doing something\n> > unexpected.\n>\n> True that a WARN is an option.\n> I would prefer the current approach though, otherwise I need to\n> implement the logic in Android Adapter instead.\n>\n> Please let me know if anyone insists on the other approach.\n>\n> > Also, the expectations that a metadata is returned once\n> > per Request has to be documented in the documentation of\n> > PipelineHandler::completeMetadata()\n>\n> I'll update the doc in the next version :)\n>\n> BR,\n> Harvey\n>\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > BR,\n> > > Harvey\n> > > >\n> > > > > + resultMetadata.set(id, value);\n> > > > > + }\n> > > > > +\n> > > > > + return resultMetadata;\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Insert a text representation of a Request into an output stream\n> > > > > * \\param[in] out The output stream\n> > > > > --\n> > > > > 2.46.0.598.g6f2099f65c-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 47BCDC0F1B\n\tfor ;\n\tThu, 14 Nov 2024 10:17:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A8AD965834;\n\tThu, 14 Nov 2024 11:17:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AEB4065834\n\tfor ;\n\tThu, 14 Nov 2024 11:17:10 +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 78D0F502;\n\tThu, 14 Nov 2024 11:16:56 +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=\"LyG72zv0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731579416;\n\tbh=06u8c5zPiEQqa6j9kflJkJ/Fbuht0KKWVsWXiVE7D5E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LyG72zv0jJ797Mht8Kervu3p74M6TewcC4C0jrniQMu/6UFgrXOg9fJQFWfpqC/gi\n\tjENvAUJ3obO1MMIxF8WneTUvJC92Y+2q1aF/fRVerW4Pbucb6crnvQopXfxfIf2lEX\n\tPf51/rYXix6ERLekDjpRZjfdl6vA28uCvbqr9nww=","Date":"Thu, 14 Nov 2024 11:17:06 +0100","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH 1/1] libcamera: Camera: Add signals for completion of\n\tmetadata as a partial result","Message-ID":"","References":"<20240912045703.3446748-1-chenghaoyang@google.com>\n\t<20240912045703.3446748-2-chenghaoyang@google.com>\n\t\n\t\n\t<6mh4oluzbek2izgwbfxmvkcoklfgbxejzgk26olzmhmoukkki2@myvfc2adwjhf>\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":32169,"web_url":"https://patchwork.libcamera.org/comment/32169/","msgid":"","date":"2024-11-14T11:06:15","subject":"Re: [PATCH 1/1] 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, Nov 14, 2024 at 6:17 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Thu, Nov 14, 2024 at 05:11:42PM +0800, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > On Thu, Nov 14, 2024 at 4:36 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Harvey\n> > >\n> > > On Thu, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > I'll upload a new version when all discussion is done.\n> > > >\n> > > > On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi\n> > > > wrote:\n> > > > >\n> > > > > Hi Harvey,\n> > > > > sorry for the late feedback\n> > > > >\n> > > > > On Thu, Sep 12, 2024 at 04:52:48AM +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 metadataCompleted;\n> > > > >\n> > > > > Bikeshedding on the name a bit, as this is allowed to be called\n> > > > > multiple times, with partial metadata, I would call it\n> > > > > 'metadataAvailable'.\n> > > >\n> > > > Sure, 'metadataAvailable' does make more sense.\n> > > > Thanks!\n> > > >\n> > > > >\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/request.h | 5 +++\n> > > > > > src/libcamera/camera.cpp | 6 +++\n> > > > > > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++\n> > > > > > src/libcamera/request.cpp | 20 ++++++++++\n> > > > > > 6 files changed, 70 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > > > index 94cee7bd..08c5c58c 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 metadataCompleted;\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 0d380803..1af63a23 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> > > > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > > > void completeRequest(Request *request);\n> > > > > >\n> > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > > > index e214a9d1..0200f4a1 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> > > > > > #include \n> > > > > > #include \n> > > > > > @@ -64,6 +65,8 @@ public:\n> > > > > >\n> > > > > > std::string toString() const;\n> > > > > >\n> > > > > > + ControlList addCompletedMetadata(const ControlList &metadata);\n> > > > > > +\n> > > > > > private:\n> > > > > > LIBCAMERA_DISABLE_COPY(Request)\n> > > > > >\n> > > > > > @@ -73,6 +76,8 @@ private:\n> > > > > >\n> > > > > > const uint64_t cookie_;\n> > > > > > Status status_;\n> > > > > > +\n> > > > > > + std::unordered_set completedMetadata_;\n> > > > > > };\n> > > > > >\n> > > > > > std::ostream &operator<<(std::ostream &out, const Request &r);\n> > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > > index a86f552a..5ffae23e 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::metadataCompleted\n> > > > > > + * \\brief Signal emitted when some metadata for a request is completed as a\n> > > > > > + * partial result\n> > > > >\n> > > > > Currently, we return metadata as part of a Request signalled with\n> > > > > Camera::requestCompleted. Existing applications do not handle the new\n> > > > > signal and I don't think we should make it mandatory to do so.\n> > > > > According to the implementation of this patch, the same application\n> > > > > running on a pipeline handler that calls\n> > > > > PipelineHandler::completeMetadata and on a pipeline handler that\n> > > > > doesn't will receive different sets of metadata.\n> > > > >\n> > > > > - If a pipeline handler calls PipelineHandler::completeMetadata() it\n> > > > > will send metadata in chunks and only the ones not previously\n> > > > > signalled with be part of Request::metadata().\n> > > >\n> > > > Actually, it's not true.\n> > > > In this patch, `PipelineHandler::completeMetadata` will merge the\n> > > > partially available metadata into Request:\n> > > > `request->metadata().merge(validMetadata);`\n> > > >\n> > >\n> > > Ups, sorry I missed it!\n> > >\n> > > > Therefore, whether an application handles the new signal, it can\n> > > > still get access to all the metadata in a request via\n> > > > `Request.metadata()`. This should ease your concern of the old\n> > > > and new implementations of applications.\n> > > >\n> > >\n> > > Yes, if this time I got this right, the full metadata pack will be\n> > > available in Request::metadata() at requestCompleted time, but it will\n> > > be completed in chunks if the pipeline handler calls\n> > > completeMetadata() earlier.\n> > >\n> > > There's only one thing left that I'm not sure about: in\n> > > requestCompleted()\n> > >\n> > > const ControlList &validMetadata = request->addCompletedMetadata(\n> > > request->metadata());\n> > > if (!validMetadata.empty())\n> > > camera->metadataCompleted.emit(request, validMetadata);\n> > >\n> > > If the pipeline handler never calls completeMetadata() then\n> > > validMetadata == request->metadata(). So that applications will\n> > > receive the same metadata buffer in metadataCompleted and\n> > > requestCompleted. I wonder if we should emit the metadataCompleted\n> > > signal at all if the pipeline handler never calls completeMetadata()\n> > > (it would be easy to check, just make sure that Request:completedMetadata_\n> > > is empty with an helper function).\n> >\n> > I think this is how we expect a pipeline handler to behave and how an\n> > application to handle signals.\n>\n> Do you expect all pipelines to use the new signal ?\n\nNot really, while I still think that we need to ensure the new signal sends\na complete set of metadata keys.\n\n>\n> I quickly checked rkisp1 where most metadata are computed by the IPA\n> in one go after parsing statistics. Only the frame timestamp and the\n> scaler crop are available at different times (the buffer completion,\n> so possibily later than the IPA computed ones).\n>\n> >\n> > In the current implementation, it's safer for applications that whether\n> > a pipeline handler calls completeMetadata, applications still get full\n> > metadata set with the new signal, and they don't need to rely on\n> > signal requestCompleted. In other words, I want to guarantee that\n> > applications can choose to use either metadataAvailable or\n> > requestCompleted.\n> > Triggering the new signal if there's any metadata left is just a safer\n> > option to support all pipeline handlers.\n>\n> My concern is about sending over a signal a possibly long list of\n> controls two times in the case all metadata are sent in a single chunk\n> at requestComplete() time.\n>\n> >\n> > Of course we can choose to check if\n> > `Request::Private::completedMetadata_` is empty, and only handle\n> > this case, and expect the proper implementations of pipeline\n> > handlers call completeMetadata properly without missing any\n> > metadata. I just feel that it's unnecessary.\n> > Also, I'm not sure if there's a case that pipeline handler doesn't\n> > want to report some metadata earlier, and would rather send\n> > them altogether when a request is completed. In this case,\n> > your approach would require a pipeline handler to record\n> > those unsent metadata on its own.\n> >\n> > WDYT?\n> >\n> > >\n> > > > >\n> > > > > - If a pipeline handler doesn't call PipelineHandler::completeMetadata()\n> > > > > the full metadata buffer will be in Request::metadata().\n> > > > >\n> > > > > How does an application know what to expect ?\n> > > > >\n> > > > > Imo we should advertise if the camera supports partial metadata or not\n> > > > > (maybe through a property). If it does applications should opt-in to\n> > > > > receive partial metadata. This could even be done per-configuration by\n> > > > > adding a field to CameraConfiguration ?\n> > > > >\n> > > > > If applications enable early metdata completion they agree to handle\n> > > > > the new signal, and the behaviour will be the one implemented in this\n> > > > > patch. If they do not enable early metadata completion then\n> > > > > PipelineHandler::completeMetadata() should simply merge the partial\n> > > > > metadata in Request::metadata() and not emit the new signal but return\n> > > > > the full metadata buffer as part of the Request in requestCompleted,\n> > > > > as they currently do so that pipelines that use early completion and\n> > > > > pipelines that do not will behave identically from an application\n> > > > > perspective.\n> > > > >\n> > > > > What do you think ?\n> > > > >\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 e5940469..5d2999cb 100644\n> > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > @@ -535,6 +535,28 @@ 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 buffer belongs to\n> > > > >\n> > > > > * \\param[in] request The Request the metadata buffer belongs to\n> > > >\n> > > > Perhaps:\n> > > > `* \\param[in] request The Request the metadata belongs to`\n> > > > ?\n> > > >\n> > >\n> > > ack\n> > >\n> > > > >\n> > > > >\n> > > > > > + * \\param[in] metadata The partial metadata that has completed\n> > > > > > + *\n> > > > > > + * This function could be called by pipeline handlers to signal completion of\n> > > > > > + * the \\a metadata part of the \\a request. It notifies applications of metadata\n> > > > > > + * completion.\n> > > > >\n> > > > > I think a little more details are needed.\n> > > > >\n> > > > > * This function could be called by pipeline handlers to signal\n> > > > > * availability of \\a metadata before \\a request completes. Early\n> > > > > * metadata completion allows to notify applications about the\n> > > > > * availability of a partial metadata buffer before the associated\n> > > > > * Request has completed.\n> > > >\n> > > > Thanks! Updated.\n> > > >\n> > > > >\n> > > > > 1) According to this implementation metadata can be completed\n> > > > > out-of-order (partial metadata for Request X+1 can be signalled before\n> > > > > Request X completes). Is this desirable ?\n> > > >\n> > > > Yes, this is what we expect, and Android also allows that.\n> > > > WDYT?\n> > > >\n> > >\n> > > I'll ask others what to they think\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->addCompletedMetadata(metadata);\n> > > > >\n> > > > > Request::addCompleteMetadata constructs and returns a ControlList,\n> > > > > I'm surprised the compiler doesn't complain you're taking a reference\n> > > > > to a temporary object (it's apparently legal for const references only).\n> > > > >\n> > > > > However, as a temporary will be constructed anyway this doesn't\n> > > > > buy you any gain, possibily the contrary, as this will disallow C++\n> > > > > copy elision from happening (afaiu). I think you should drop &\n> > > >\n> > > > Right, it's definitely a mistake. Removed.\n> > > >\n> > > > >\n> > > > >\n> > > > > > + if (!validMetadata.empty()) {\n> > > > > > + request->metadata().merge(validMetadata);\n> > > > > > +\n> > > > > > + Camera *camera = request->_d()->camera();\n> > > > > > + camera->metadataCompleted.emit(request, validMetadata);\n> > > > > > + }\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\brief Signal request completion\n> > > > > > * \\param[in] request The request that has completed\n> > > > > > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > > >\n> > > > > > Camera::Private *data = camera->_d();\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> > >\n> > > Could you please clarify me what this \\todo means ?\n> >\n> > Similar to the above reasons:\n> > Currently all pipeline handlers merge metadata into request.metadata()\n> > directly, except for the upcoming mtkisp7. We were expecting every\n> > pipeline handler to migrate to `PipelineHandler::completeMetadata`\n> > eventually.\n> >\n> > Let me know if you think this todo doesn't make sense, as we should\n> > still allow the previous way to merge into request.metadata() directly,\n> > or there's a case that a pipeline handler doesn't want to report some\n> > metadata earlier.\n> >\n>\n> I'm just not 100% sure all pipelines will be capable of actually send\n> metadata earlier. True, in the RkISP1 case I used as an example as\n> soon as the IPA has processed stats and computed metadata the pipeline\n> can call metadataComplete() but we have no guarantee the application\n> handles the signal, so we end up sending the same metadata list twice\n> (one in chunks, the other as Request::metadata() at requestComplete()\n> time.\n\nI understand your concern, while `libcamera::Signal` doesn't do anything\nif there's no slot connected. We still prepare the data chunks though.\nWe can also check if `libcamera::Signal::slots()` is empty before preparing\nthe data to be sent. WDYT?\n\nBR,\nHarvey\n\n>\n> I'll check with others what are the implications in terms of\n> performances of this.\n>\n> Thanks\n> j\n>\n> > >\n> > > > > > + const ControlList &validMetadata = request->addCompletedMetadata(\n> > > > > > + request->metadata());\n> > > > > > + if (!validMetadata.empty())\n> > > > > > + camera->metadataCompleted.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 8c56ed30..3ce23a8e 100644\n> > > > > > --- a/src/libcamera/request.cpp\n> > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > @@ -598,6 +598,26 @@ std::string Request::toString() const\n> > > > > > return ss.str();\n> > > > > > }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Add completed metadata, as a partial result\n> > > > > > + * \\param[in] metadata The metadata completed\n> > > > > > + *\n> > > > > > + * Request will record the entries that has been sent to the application, to\n> > > > > > + * prevent duplicated controls.\n> > > > > > + *\n> > > > > > + * \\return ControlList that hasn't been completed before\n> > > > > > + */\n> > > > > > +ControlList Request::addCompletedMetadata(const ControlList &metadata)\n> > > > > > +{\n> > > > > > + ControlList resultMetadata;\n> > > > > > + for (auto &[id, value] : metadata) {\n> > > > > > + if (!completedMetadata_.count(id))\n> > > > >\n> > > > > completedMetadata_ should be cleared in Request::reset().\n> > > >\n> > > > Nice catch! Then how about we move the member variable and the member\n> > > > function to `Request::Private` instead?\n> > > >\n> > >\n> > > Yes, they both do not need to be exposed to applications indeed.\n> > >\n> > > > >\n> > > > > I wonder if we should actually prevent the same metadata to be\n> > > > > returned multiple times by the pipeline handler or not.\n> > > >\n> > > > This is actually prohibited by Android camera hal [1]:\n> > > > `Partial metadata submitted should not include any metadata key\n> > > > returned in a previous partial result for a given frame.`\n> > > >\n> > > > We can certainly prevent that in Android Adapter only, while I think\n> > > > having this restriction in libcamera core library also makes sense.\n> > > > WDYT? I can also update the description of the new signal.\n> > > >\n> > > > [1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL.\n> > >\n> > > I can't find in the text where this is forbidden, however I'm not\n> > > pushing for the other option, I think it's actually saner to only\n> > > signal a metadata once per Request. I wonder however if this shouldn't\n> > > be a WARN as it means the pipeline handler is doing something\n> > > unexpected.\n> >\n> > True that a WARN is an option.\n> > I would prefer the current approach though, otherwise I need to\n> > implement the logic in Android Adapter instead.\n> >\n> > Please let me know if anyone insists on the other approach.\n> >\n> > > Also, the expectations that a metadata is returned once\n> > > per Request has to be documented in the documentation of\n> > > PipelineHandler::completeMetadata()\n> >\n> > I'll update the doc in the next version :)\n> >\n> > BR,\n> > Harvey\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > BR,\n> > > > Harvey\n> > > > >\n> > > > > > + resultMetadata.set(id, value);\n> > > > > > + }\n> > > > > > +\n> > > > > > + return resultMetadata;\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\brief Insert a text representation of a Request into an output stream\n> > > > > > * \\param[in] out The output stream\n> > > > > > --\n> > > > > > 2.46.0.598.g6f2099f65c-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 1AFAEC324C\n\tfor ;\n\tThu, 14 Nov 2024 11:06:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38C376583C;\n\tThu, 14 Nov 2024 12:06:30 +0100 (CET)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41B8C657E0\n\tfor ;\n\tThu, 14 Nov 2024 12:06:28 +0100 (CET)","by mail-lj1-x233.google.com with SMTP id\n\t38308e7fff4ca-2feeb1e8edfso6757621fa.1\n\tfor ;\n\tThu, 14 Nov 2024 03:06:28 -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=\"amQbD5EF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1731582387; x=1732187187;\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=Td0Ppzk/wlS9+NdbEIb4nt3okHQPkzGtczP0wSyR8ns=;\n\tb=amQbD5EF7S82ePi5pUfDwYDpIwBKRnb4NOi2Kc9tCOxpbI27LUuXxTEWYmQkRF8W3s\n\tL54T0+i/IJwnSuLisVqFQsLjiB6Syhodu8hElcPP2EyIkVqSqBDvfso+vg6/XU2aj2Q4\n\tH/4vV7D75oxw6AC0pff8qeGmXkZrwq3o7kOyw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731582387; x=1732187187;\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=Td0Ppzk/wlS9+NdbEIb4nt3okHQPkzGtczP0wSyR8ns=;\n\tb=aY4upEXRHWNrEp6twzFJEZnMDBczPcVgFhLXy1IYJCPDnF/tnxue14YM1/HiU+DQsv\n\t7OOBkLDVDOkZxTUiHtty8GwdHqWduZ6DwNyvvJ48fWWrKg5c2t2XnbfafqYozGdEyvt1\n\tK1KzUJeZu4+DVinomXIrj/TMgBiHzqVUjEV2jAW7b2DttM2nmU3Kl6JVsi+E2xDOqgnJ\n\tbMDyDgHiQ9u2tZFd6vjJK3Tcqp+rECjF0EEkqvdCJH8dp8jaorLlj1RfrEKyTvGjlMWc\n\tsESC3keJiwoC1ace/x3LLOgwEYKFN2b24MmPHcDBoEImfLIyl34V7ImBXRhq6oeTp+/h\n\t0MNg==","X-Gm-Message-State":"AOJu0Yw4iLBRgwcqGZsr7mxuBK6Bg//mBvicGkXHWAbPqEj2ByoeQt83\n\tElUhv3cWhmfrg6zFPRUtuviws5dHDUxpDBjStSG/rB+YTQxGAPczil7xhIU4Shj59bcd6Mdr4W7\n\tptlSylzcFuUwPX7gXL0M1XgBYEo5Mjtoq5mpj","X-Google-Smtp-Source":"AGHT+IGXMuwoNUCqwPbHt/sY6KbZZicy0rs1g/WfozGIuuW/u+o5OTgYYHWZ8mdjw1Xgiuxv50oB+ecM9k60x8g7ekw=","X-Received":"by 2002:a2e:9fc2:0:b0:2fb:4f8e:efd with SMTP id\n\t38308e7fff4ca-2ff5907d9b1mr15259061fa.32.1731582387080;\n\tThu, 14 Nov 2024 03:06:27 -0800 (PST)","MIME-Version":"1.0","References":"<20240912045703.3446748-1-chenghaoyang@google.com>\n\t<20240912045703.3446748-2-chenghaoyang@google.com>\n\t\n\t\n\t<6mh4oluzbek2izgwbfxmvkcoklfgbxejzgk26olzmhmoukkki2@myvfc2adwjhf>\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 14 Nov 2024 19:06:15 +0800","Message-ID":"","Subject":"Re: [PATCH 1/1] 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\" "}}]