[{"id":32919,"web_url":"https://patchwork.libcamera.org/comment/32919/","msgid":"","date":"2025-01-02T16:13:36","subject":"Re: [PATCH 3/8] libcamera: pipeline_handler: Add metadataAvailable()\n\tfunction","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n wrote:\n>\n> Currently the way pipeline handlers have to store metadata results\n> for a Request is to access the Request::metadata_ list directly and\n> either merge a ControlList or set a single control value there.\n>\n> Direct access to Request::metadata_ is however problematic as even if\n> metadata would be available earlier, pipeline handlers can only\n> accumulate the results in Request::metadata_ and they're only available\n> for applications at Request complete time.\n>\n> Instead of letting pipeline handlers access Request::metadata_ directly\n> provide two helper functions, similar in spirit to\n> PipelineHandler::completeBuffer() and\n> PipelineHandler::completeRequest(), to allow pipeline handlers to\n> notify early availability of metadata.\n>\n> Provide two overloads, one that accepts a ControlList and merges it into\n> Request::metadata_ and one that allows to set a single metadata result\n> there without going through an intermediate copy.\n>\n> The newly provided helpers trigger the Camera::availableMetadata signal\n> from where applications can retrieve the list of ControlIds that have\n> just been made available by the pipeline handler.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/pipeline_handler.h | 41 ++++++++++\n> src/libcamera/pipeline_handler.cpp | 74 +++++++++++++++++++\n> 2 files changed, 115 insertions(+)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index fb28a18d0f46..3ca6a0290b2b 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -11,13 +11,17 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \n>\n> +#include \n> #include \n> #include \n>\n> +#include \"libcamera/internal/request.h\"\n> +\n> namespace libcamera {\n>\n> class Camera;\n> @@ -58,6 +62,43 @@ public:\n> void registerRequest(Request *request);\n> void queueRequest(Request *request);\n>\n> + void metadataAvailable(Request *request, const ControlList &metadata);\n> +\n> + template\n> + void metadataAvailable(Request *request, const Control &ctrl,\n> + const T &value)\n> + {\n> + if (request->metadata().contains(ctrl.id()))\n> + return;\n> +\n> + std::unordered_set ids;\n> + ids.insert(&ctrl);\n> +\n> + request->metadata().set(ctrl, value);\n> +\n> + Camera *camera = request->_d()->camera();\n> + camera->metadataAvailable.emit(request, ids);\n> + }\n\nOne question: Have you considered wrapping `ctrls` and `value` into a\nControlList, and reuse `void metadataAvailable(Request *request, const\nControlList &metadata)` above?\nI think this function doesn't check `request->metadata()->idMap()`\nwhile the above function does.\n\nSame for the function below.\n\nBR,\nHarvey\n\n\n> +\n> +#ifndef __DOXYGEN__\n> + template\n> + void metadataAvailable(Request *request,\n> + const Control> &ctrl,\n> + const Span &value)\n> + {\n> + if (request->metadata().contains(ctrl.id()))\n> + return;\n> +\n> + std::unordered_set ids;\n> + ids.insert(&ctrl);\n> +\n> + request->metadata().set(ctrl, value);\n> +\n> + Camera *camera = request->_d()->camera();\n> + camera->metadataAvailable.emit(request, ids);\n> + }\n> +#endif\n> +\n> bool completeBuffer(Request *request, FrameBuffer *buffer);\n> void completeRequest(Request *request);\n> void cancelRequest(Request *request);\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index caa5c20e7483..a69d789116ad 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -507,6 +507,80 @@ void PipelineHandler::doQueueRequests()\n> * \\return 0 on success or a negative error code otherwise\n> */\n>\n> +/**\n> + * \\brief Notify the availability of a list of metadata for \\a request\n> + * \\param[in] request The request the metadata belongs to\n> + * \\param[in] metadata The metadata list\n> + *\n> + * This function should be called multiple times by pipeline handlers to signal\n> + * the availability of a list of metadata results. It notifies applications\n> + * by triggering the Camera::availableMetadata signal and accumulates the\n> + * metadata results in Request::metadata().\n> + *\n> + * Early metadata completion allows pipeline handlers to fast track delivery of\n> + * metadata results as soon as they are available before the completion of \\a\n> + * request. The full list of metadata results of a Request is available at\n> + * Request completion time in Request::metadata().\n> + *\n> + * A metadata key is expected to be notified at most once. Metadata keys\n> + * notified multiple times are ignored.\n> + *\n> + * This overload allows to signal the availability of a list of metadata and\n> + * merges them in the Request::metadata() list. This operations is expensive\n> + * as controls are copied from \\a metadata to Request::metadata().\n> + *\n> + * \\context This function shall be called from the CameraManager thread.\n> + */\n> +void PipelineHandler::metadataAvailable(Request *request, const ControlList &metadata)\n> +{\n> + std::unordered_set ids;\n> + const ControlIdMap *idmap = request->metadata().idMap();\n> +\n> + for (const auto &ctrl : metadata) {\n> + if (request->metadata().contains(ctrl.first))\n> + continue;\n> +\n> + ASSERT(idmap->count(ctrl.first));\n> +\n> + ids.insert(idmap->at(ctrl.first));\n> + }\n> +\n> + if (ids.empty())\n> + return;\n> +\n> + request->metadata().merge(metadata);\n> +\n> + Camera *camera = request->_d()->camera();\n> + camera->metadataAvailable.emit(request, ids);\n> +}\n> +\n> +/**\n> + * \\fn void PipelineHandler::metadataAvailable(Request *request, const Control &ctrl, const T &value)\n> + * \\brief Notify the availability of a metadata result for \\a request\n> + * \\param[in] request The request the metadata belongs to\n> + * \\param[in] ctrl The control id to notify\n> + * \\param[in] value the control value\n> + *\n> + * This function should be called multiple times by pipeline handlers to signal\n> + * the availability of a metadata result. It notifies applications\n> + * by triggering the Camera::availableMetadata signal and accumulates the\n> + * metadata result in Request::metadata().\n> + *\n> + * Early metadata completion allows pipeline handlers to fast track delivery of\n> + * metadata results as soon as they are available before the completion of \\a\n> + * request. The full list of metadata results of a Request is available at\n> + * Request completion time in Request::metadata().\n> + *\n> + * A metadata key is expected to be notified at most once. Metadata keys\n> + * notified multiple times are ignored.\n> + *\n> + * This overload allows to signal the availability of a single metadata and\n> + * merge \\a value in the Request::metadata() list. This operations copies \\a\n> + * value in the Request::metadata() list without creating intermediate copies.\n> + *\n> + * \\context This function shall be called from the CameraManager thread.\n> + */\n> +\n> /**\n> * \\brief Complete a buffer for a request\n> * \\param[in] request The request the buffer belongs to\n> --\n> 2.47.1\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 2040CBDB1C\n\tfor ;\n\tThu, 2 Jan 2025 16:13:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C719F684CF;\n\tThu, 2 Jan 2025 17:13:49 +0100 (CET)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4992C684CF\n\tfor ;\n\tThu, 2 Jan 2025 17:13:48 +0100 (CET)","by mail-lf1-x136.google.com with SMTP id\n\t2adb3069b0e04-54020b0dcd2so13710825e87.1\n\tfor ;\n\tThu, 02 Jan 2025 08:13:48 -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=\"JV5Rqpyl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1735834427; x=1736439227;\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=UWiWlA634JEKWQmwa44IA1lefdaaJbVSqillKkL4E2Y=;\n\tb=JV5RqpylDP0HoBqPPDwVOTDL1dYFw76rTpDP4YIgjz7U8L2JSsXn3uB3zAYWpLZZVw\n\tq3co+u0UVVTSDGgmDflCKgGMzjYSkZgRlwjXogD4ijfb1v4eWdkEgMiq7OpdcWCtiXB0\n\thXb3DoAqhBN8Zh0DynenJfTR5rySw8ftLAidE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1735834427; x=1736439227;\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=UWiWlA634JEKWQmwa44IA1lefdaaJbVSqillKkL4E2Y=;\n\tb=TzMxHrTscIfTgEy22XwnbNLg4ymshZwERruD0Nr3zphZXfpJmUvWmQqHyI9n9ykNAr\n\tD1suoe8liyYFIXBuVed2nIk8Kn/ec2lb21sfyF7GlPxn8KtaRBAPmJbsAJonfy95yGgU\n\tvKky5dGaFJBtqsBtfegmEnhLtRWjBOUw12zr2mtGkwA0MLSBT7MvstW/h+vaEUOZxRT6\n\t714tO1bXphRvCj0UgNHlojy2Qf2aGbvhapiLLse99lZ3FSQdsZI/X2MW7w6wIvC2Sif8\n\tXNzKcl5MjGpr2HPo2/28CzumEAfrNVpNSSrH6vIAIqvxPLocXX9GREy/CNEdOxSMLp9s\n\tXjFg==","X-Gm-Message-State":"AOJu0YzYftmW4jhR7PVH+h5SVEzrGtHsqbUlNfRNIAW1GCA5EtrFPs+1\n\tcpgv1m0R3IecjbYg9qraGNJDjMeGgo8H1cL2IFvAmuUpWUsZYijmu452EYgMGQxsAWy7y1sfAL1\n\tchGnDWVAGX33GHdd7nmHNkPGyaI5SajUElXXQ","X-Gm-Gg":"ASbGncuxWkBT6Kt3sRGVmq2JVNVKnH7YsxyokmVMtjGSY6A7RXJywGgQ69NE/f0ipKQ\n\tYJOROcYAm9qN1HvlNIIP+MDfphHQcEUBolUCa","X-Google-Smtp-Source":"AGHT+IFbnjDLWfJmUbvV/drJGHA+7U2N13nC2xtxzf/wBfspa7er1fqzQmJQmo2dpCea/BDLGshpLUioNZ+hOqkjO0w=","X-Received":"by 2002:a05:651c:551:b0:302:23a7:1ee8 with SMTP id\n\t38308e7fff4ca-3045833755bmr169928621fa.3.1735834427495;\n\tThu, 02 Jan 2025 08:13:47 -0800 (PST)","MIME-Version":"1.0","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-4-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20241206160747.97176-4-jacopo.mondi@ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Thu, 2 Jan 2025 17:13:36 +0100","Message-ID":"","Subject":"Re: [PATCH 3/8] libcamera: pipeline_handler: Add metadataAvailable()\n\tfunction","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":32928,"web_url":"https://patchwork.libcamera.org/comment/32928/","msgid":"","date":"2025-01-03T17:08:07","subject":"Re: [PATCH 3/8] libcamera: pipeline_handler: Add metadataAvailable()\n\tfunction","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, Jan 02, 2025 at 05:13:36PM +0100, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n> wrote:\n> >\n> > Currently the way pipeline handlers have to store metadata results\n> > for a Request is to access the Request::metadata_ list directly and\n> > either merge a ControlList or set a single control value there.\n> >\n> > Direct access to Request::metadata_ is however problematic as even if\n> > metadata would be available earlier, pipeline handlers can only\n> > accumulate the results in Request::metadata_ and they're only available\n> > for applications at Request complete time.\n> >\n> > Instead of letting pipeline handlers access Request::metadata_ directly\n> > provide two helper functions, similar in spirit to\n> > PipelineHandler::completeBuffer() and\n> > PipelineHandler::completeRequest(), to allow pipeline handlers to\n> > notify early availability of metadata.\n> >\n> > Provide two overloads, one that accepts a ControlList and merges it into\n> > Request::metadata_ and one that allows to set a single metadata result\n> > there without going through an intermediate copy.\n> >\n> > The newly provided helpers trigger the Camera::availableMetadata signal\n> > from where applications can retrieve the list of ControlIds that have\n> > just been made available by the pipeline handler.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 41 ++++++++++\n> > src/libcamera/pipeline_handler.cpp | 74 +++++++++++++++++++\n> > 2 files changed, 115 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index fb28a18d0f46..3ca6a0290b2b 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -11,13 +11,17 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> >\n> > +#include \n> > #include \n> > #include \n> >\n> > +#include \"libcamera/internal/request.h\"\n> > +\n> > namespace libcamera {\n> >\n> > class Camera;\n> > @@ -58,6 +62,43 @@ public:\n> > void registerRequest(Request *request);\n> > void queueRequest(Request *request);\n> >\n> > + void metadataAvailable(Request *request, const ControlList &metadata);\n> > +\n> > + template\n> > + void metadataAvailable(Request *request, const Control &ctrl,\n> > + const T &value)\n> > + {\n> > + if (request->metadata().contains(ctrl.id()))\n> > + return;\n> > +\n> > + std::unordered_set ids;\n> > + ids.insert(&ctrl);\n> > +\n> > + request->metadata().set(ctrl, value);\n> > +\n> > + Camera *camera = request->_d()->camera();\n> > + camera->metadataAvailable.emit(request, ids);\n> > + }\n>\n> One question: Have you considered wrapping `ctrls` and `value` into a\n> ControlList, and reuse `void metadataAvailable(Request *request, const\n> ControlList &metadata)` above?\n\nIf memory doesn't fail me, to wrap them I should create a ControlList\nand set(), going through a copy, doesn't I ?\n\n> I think this function doesn't check `request->metadata()->idMap()`\n> while the above function does.\n\nI think it's fine. In the templated versions ctrl is a Control,\nwhile in\n\nvoid PipelineHandler::metadataAvailable(Request *request, const ControlList &metadata)\n\nctrl is an int, that's why I need to go through an id map.\n\nDo you agree ?\n\nThanks\n j\n\n>\n> Same for the function below.\n>\n> BR,\n> Harvey\n>\n>\n> > +\n> > +#ifndef __DOXYGEN__\n> > + template\n> > + void metadataAvailable(Request *request,\n> > + const Control> &ctrl,\n> > + const Span &value)\n> > + {\n> > + if (request->metadata().contains(ctrl.id()))\n> > + return;\n> > +\n> > + std::unordered_set ids;\n> > + ids.insert(&ctrl);\n> > +\n> > + request->metadata().set(ctrl, value);\n> > +\n> > + Camera *camera = request->_d()->camera();\n> > + camera->metadataAvailable.emit(request, ids);\n> > + }\n> > +#endif\n> > +\n> > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > void completeRequest(Request *request);\n> > void cancelRequest(Request *request);\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index caa5c20e7483..a69d789116ad 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -507,6 +507,80 @@ void PipelineHandler::doQueueRequests()\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> >\n> > +/**\n> > + * \\brief Notify the availability of a list of metadata for \\a request\n> > + * \\param[in] request The request the metadata belongs to\n> > + * \\param[in] metadata The metadata list\n> > + *\n> > + * This function should be called multiple times by pipeline handlers to signal\n> > + * the availability of a list of metadata results. It notifies applications\n> > + * by triggering the Camera::availableMetadata signal and accumulates the\n> > + * metadata results in Request::metadata().\n> > + *\n> > + * Early metadata completion allows pipeline handlers to fast track delivery of\n> > + * metadata results as soon as they are available before the completion of \\a\n> > + * request. The full list of metadata results of a Request is available at\n> > + * Request completion time in Request::metadata().\n> > + *\n> > + * A metadata key is expected to be notified at most once. Metadata keys\n> > + * notified multiple times are ignored.\n> > + *\n> > + * This overload allows to signal the availability of a list of metadata and\n> > + * merges them in the Request::metadata() list. This operations is expensive\n> > + * as controls are copied from \\a metadata to Request::metadata().\n> > + *\n> > + * \\context This function shall be called from the CameraManager thread.\n> > + */\n> > +void PipelineHandler::metadataAvailable(Request *request, const ControlList &metadata)\n> > +{\n> > + std::unordered_set ids;\n> > + const ControlIdMap *idmap = request->metadata().idMap();\n> > +\n> > + for (const auto &ctrl : metadata) {\n> > + if (request->metadata().contains(ctrl.first))\n> > + continue;\n> > +\n> > + ASSERT(idmap->count(ctrl.first));\n> > +\n> > + ids.insert(idmap->at(ctrl.first));\n> > + }\n> > +\n> > + if (ids.empty())\n> > + return;\n> > +\n> > + request->metadata().merge(metadata);\n> > +\n> > + Camera *camera = request->_d()->camera();\n> > + camera->metadataAvailable.emit(request, ids);\n> > +}\n> > +\n> > +/**\n> > + * \\fn void PipelineHandler::metadataAvailable(Request *request, const Control &ctrl, const T &value)\n> > + * \\brief Notify the availability of a metadata result for \\a request\n> > + * \\param[in] request The request the metadata belongs to\n> > + * \\param[in] ctrl The control id to notify\n> > + * \\param[in] value the control value\n> > + *\n> > + * This function should be called multiple times by pipeline handlers to signal\n> > + * the availability of a metadata result. It notifies applications\n> > + * by triggering the Camera::availableMetadata signal and accumulates the\n> > + * metadata result in Request::metadata().\n> > + *\n> > + * Early metadata completion allows pipeline handlers to fast track delivery of\n> > + * metadata results as soon as they are available before the completion of \\a\n> > + * request. The full list of metadata results of a Request is available at\n> > + * Request completion time in Request::metadata().\n> > + *\n> > + * A metadata key is expected to be notified at most once. Metadata keys\n> > + * notified multiple times are ignored.\n> > + *\n> > + * This overload allows to signal the availability of a single metadata and\n> > + * merge \\a value in the Request::metadata() list. This operations copies \\a\n> > + * value in the Request::metadata() list without creating intermediate copies.\n> > + *\n> > + * \\context This function shall be called from the CameraManager thread.\n> > + */\n> > +\n> > /**\n> > * \\brief Complete a buffer for a request\n> > * \\param[in] request The request the buffer belongs to\n> > --\n> > 2.47.1\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 DCAD9C0F1B\n\tfor ;\n\tFri, 3 Jan 2025 17:08:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3187684E7;\n\tFri, 3 Jan 2025 18:08:12 +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 D7F27684CC\n\tfor ;\n\tFri, 3 Jan 2025 18:08:10 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 332C6316;\n\tFri, 3 Jan 2025 18:07:21 +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=\"q/zIkWlz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1735924041;\n\tbh=YZ+ygNEb36A+cirwwCH3vpI7xCSSLBgQgCy7CpQ710E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=q/zIkWlzyT0wGQPoDFZdOmR93+9yoiR2j46TuT4ctdDzFNcwcJkSuZhrZJ7siMc2i\n\tfzufGKRmrZVrpahyjIqrhEii+HviowMsWpBNs26/nHhdbEPX/yhI2p0VKo5+o17fIg\n\t1TDcxaoVu+U5yYt9UsfCNX1A60JmXkGp/BzOTxgs=","Date":"Fri, 3 Jan 2025 18:08:07 +0100","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH 3/8] libcamera: pipeline_handler: Add metadataAvailable()\n\tfunction","Message-ID":"","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-4-jacopo.mondi@ideasonboard.com>\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":32932,"web_url":"https://patchwork.libcamera.org/comment/32932/","msgid":"","date":"2025-01-03T21:26:15","subject":"Re: [PATCH 3/8] libcamera: pipeline_handler: Add metadataAvailable()\n\tfunction","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Fri, Jan 3, 2025 at 6:08 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Thu, Jan 02, 2025 at 05:13:36PM +0100, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Currently the way pipeline handlers have to store metadata results\n> > > for a Request is to access the Request::metadata_ list directly and\n> > > either merge a ControlList or set a single control value there.\n> > >\n> > > Direct access to Request::metadata_ is however problematic as even if\n> > > metadata would be available earlier, pipeline handlers can only\n> > > accumulate the results in Request::metadata_ and they're only available\n> > > for applications at Request complete time.\n> > >\n> > > Instead of letting pipeline handlers access Request::metadata_ directly\n> > > provide two helper functions, similar in spirit to\n> > > PipelineHandler::completeBuffer() and\n> > > PipelineHandler::completeRequest(), to allow pipeline handlers to\n> > > notify early availability of metadata.\n> > >\n> > > Provide two overloads, one that accepts a ControlList and merges it into\n> > > Request::metadata_ and one that allows to set a single metadata result\n> > > there without going through an intermediate copy.\n> > >\n> > > The newly provided helpers trigger the Camera::availableMetadata signal\n> > > from where applications can retrieve the list of ControlIds that have\n> > > just been made available by the pipeline handler.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/pipeline_handler.h | 41 ++++++++++\n> > > src/libcamera/pipeline_handler.cpp | 74 +++++++++++++++++++\n> > > 2 files changed, 115 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index fb28a18d0f46..3ca6a0290b2b 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -11,13 +11,17 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > +#include \"libcamera/internal/request.h\"\n> > > +\n> > > namespace libcamera {\n> > >\n> > > class Camera;\n> > > @@ -58,6 +62,43 @@ public:\n> > > void registerRequest(Request *request);\n> > > void queueRequest(Request *request);\n> > >\n> > > + void metadataAvailable(Request *request, const ControlList &metadata);\n> > > +\n> > > + template\n> > > + void metadataAvailable(Request *request, const Control &ctrl,\n> > > + const T &value)\n> > > + {\n> > > + if (request->metadata().contains(ctrl.id()))\n> > > + return;\n> > > +\n> > > + std::unordered_set ids;\n> > > + ids.insert(&ctrl);\n> > > +\n> > > + request->metadata().set(ctrl, value);\n> > > +\n> > > + Camera *camera = request->_d()->camera();\n> > > + camera->metadataAvailable.emit(request, ids);\n> > > + }\n> >\n> > One question: Have you considered wrapping `ctrls` and `value` into a\n> > ControlList, and reuse `void metadataAvailable(Request *request, const\n> > ControlList &metadata)` above?\n>\n> If memory doesn't fail me, to wrap them I should create a ControlList\n> and set(), going through a copy, doesn't I ?\n\nAh got it, thanks for the explanation. I guess using rvalue reference\nhere (Control &&ctrl) and expecting move semantics of users is an\noverkill? Fine with the current way though.\n\n>\n> > I think this function doesn't check `request->metadata()->idMap()`\n> > while the above function does.\n>\n> I think it's fine. In the templated versions ctrl is a Control,\n> while in\n>\n> void PipelineHandler::metadataAvailable(Request *request, const ControlList &metadata)\n>\n> ctrl is an int, that's why I need to go through an id map.\n>\n> Do you agree ?\n\nHmm, Control classes in libcamera have always been a headache to me,\nwhile I thought that ControlList is just a list of `pair,\nT>`...? (Observed as `void ControlList::set(const Control &ctrl,\nconst V &value)`)\n\n\n\n>\n> Thanks\n> j\n>\n> >\n> > Same for the function below.\n> >\n> > BR,\n> > Harvey\n> >\n> >\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > + template\n> > > + void metadataAvailable(Request *request,\n> > > + const Control> &ctrl,\n> > > + const Span &value)\n> > > + {\n> > > + if (request->metadata().contains(ctrl.id()))\n> > > + return;\n> > > +\n> > > + std::unordered_set ids;\n> > > + ids.insert(&ctrl);\n> > > +\n> > > + request->metadata().set(ctrl, value);\n> > > +\n> > > + Camera *camera = request->_d()->camera();\n> > > + camera->metadataAvailable.emit(request, ids);\n> > > + }\n> > > +#endif\n> > > +\n> > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > void completeRequest(Request *request);\n> > > void cancelRequest(Request *request);\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index caa5c20e7483..a69d789116ad 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -507,6 +507,80 @@ void PipelineHandler::doQueueRequests()\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Notify the availability of a list of metadata for \\a request\n> > > + * \\param[in] request The request the metadata belongs to\n> > > + * \\param[in] metadata The metadata list\n> > > + *\n> > > + * This function should be called multiple times by pipeline handlers to signal\n\nnit: I think `should` is a relatively strong term. Some pipeline\nhandlers might still be calling it once at the end of a Request?\n\n> > > + * the availability of a list of metadata results. It notifies applications\n> > > + * by triggering the Camera::availableMetadata signal and accumulates the\n> > > + * metadata results in Request::metadata().\n> > > + *\n> > > + * Early metadata completion allows pipeline handlers to fast track delivery of\n> > > + * metadata results as soon as they are available before the completion of \\a\n> > > + * request. The full list of metadata results of a Request is available at\n> > > + * Request completion time in Request::metadata().\n> > > + *\n> > > + * A metadata key is expected to be notified at most once. Metadata keys\n> > > + * notified multiple times are ignored.\n> > > + *\n> > > + * This overload allows to signal the availability of a list of metadata and\n> > > + * merges them in the Request::metadata() list. This operations is expensive\n\nnit: s/operations/operation\n\n> > > + * as controls are copied from \\a metadata to Request::metadata().\n> > > + *\n> > > + * \\context This function shall be called from the CameraManager thread.\n> > > + */\n> > > +void PipelineHandler::metadataAvailable(Request *request, const ControlList &metadata)\n> > > +{\n> > > + std::unordered_set ids;\n> > > + const ControlIdMap *idmap = request->metadata().idMap();\n> > > +\n> > > + for (const auto &ctrl : metadata) {\n> > > + if (request->metadata().contains(ctrl.first))\n> > > + continue;\n> > > +\n> > > + ASSERT(idmap->count(ctrl.first));\n> > > +\n> > > + ids.insert(idmap->at(ctrl.first));\n> > > + }\n> > > +\n> > > + if (ids.empty())\n> > > + return;\n> > > +\n> > > + request->metadata().merge(metadata);\n\nIf we're ignoring the whole `metadata` above when `ids.empty() ==\ntrue`, why do we still want to merge the ids that are not listed in\n`idmap`?\nI suggest setting the valid id & value pairs in the for loop instead. WDYT?\n\nBR,\nHarvey\n\n> > > +\n> > > + Camera *camera = request->_d()->camera();\n> > > + camera->metadataAvailable.emit(request, ids);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn void PipelineHandler::metadataAvailable(Request *request, const Control &ctrl, const T &value)\n> > > + * \\brief Notify the availability of a metadata result for \\a request\n> > > + * \\param[in] request The request the metadata belongs to\n> > > + * \\param[in] ctrl The control id to notify\n> > > + * \\param[in] value the control value\n> > > + *\n> > > + * This function should be called multiple times by pipeline handlers to signal\n> > > + * the availability of a metadata result. It notifies applications\n> > > + * by triggering the Camera::availableMetadata signal and accumulates the\n> > > + * metadata result in Request::metadata().\n> > > + *\n> > > + * Early metadata completion allows pipeline handlers to fast track delivery of\n> > > + * metadata results as soon as they are available before the completion of \\a\n> > > + * request. The full list of metadata results of a Request is available at\n> > > + * Request completion time in Request::metadata().\n> > > + *\n> > > + * A metadata key is expected to be notified at most once. Metadata keys\n> > > + * notified multiple times are ignored.\n> > > + *\n> > > + * This overload allows to signal the availability of a single metadata and\n> > > + * merge \\a value in the Request::metadata() list. This operations copies \\a\n> > > + * value in the Request::metadata() list without creating intermediate copies.\n> > > + *\n> > > + * \\context This function shall be called from the CameraManager thread.\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Complete a buffer for a request\n> > > * \\param[in] request The request the buffer belongs to\n> > > --\n> > > 2.47.1\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 E0A7CC0F1B\n\tfor ;\n\tFri, 3 Jan 2025 21:26:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 131D3684CC;\n\tFri, 3 Jan 2025 22:26:30 +0100 (CET)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 259DB61886\n\tfor ;\n\tFri, 3 Jan 2025 22:26:28 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id\n\t38308e7fff4ca-3003c82c95cso106712391fa.3\n\tfor ;\n\tFri, 03 Jan 2025 13:26: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=\"nc03FTL9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1735939587; x=1736544387;\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=r54oe/yZGGEtiQ4i+86vR0WHQ4haFiN9EmrjQDf13IU=;\n\tb=nc03FTL9pXpPjkL2bf6nEOtRvta1DQgrVvsvSEwsf/jw4XWisgMyZnr2D/2mbG62fH\n\t6xXsWyx9V+uOk5EvU5n/T9M9jjNY7bzF+0Gs3PoIoVVaYBaSOdD8Ajal/H5PlWnQOj4D\n\trnybETcr6qhYNyhqs4OWsnre8jIUgr9+ou1kE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1735939587; x=1736544387;\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=r54oe/yZGGEtiQ4i+86vR0WHQ4haFiN9EmrjQDf13IU=;\n\tb=TnPJyPPTFVcuh0aEiiJUWzKkj+obcGM9wi9V+76sGD27glSrqadVrQe1EqV2mSCBjD\n\tlwCDnMt6KNYPoI7367ie4GG0oB58XDL0Iy+FBC2UNOJ1DbN1WN8KCrf+QmL01FMTRF4e\n\tbO4NDkch4ZIPuyIyiHH1fo8sQp2r/Ei+hxTXU2DZUR7F7xzbC5DivlaRPz+PTCjub2Aa\n\tprYk3HMeUkNRhr83PGtYuQ7exLIzQmDtak9FiBdLHE3YQsQhsZVaZknMtKWyQCFbMfgA\n\tMrknlEtRycHsctBy1XCehclODhHlM9r9pu48u3kX7DXWY3IXFILrERGJOF/kTTCIwLAO\n\tK3/g==","X-Gm-Message-State":"AOJu0YyX2gp6wWuxAtI/zdxWP7jqf9Kwo/GG+FXfWXa7Lqp+7Drlp4n+\n\t0+ejn00onsvYoVpQNTty7yC4nkbOj8Y/772s1ADG9F73A2XAJkmw0PhjHfTvygoDvm/G9H5HQtM\n\tYWSsBgTPPpFioxH3YMmpCQwfoqBhofVoZUR6C","X-Gm-Gg":"ASbGncvaWkP61cbQx8FPxSOWBJpyHLDs7LJbUtueTrHFKujiQCgRBRZof2tSYJkaAUd\n\tlqeS/8rPrSI5BWoVsbgEFLXv+287zLIhNjhJ2WWw=","X-Google-Smtp-Source":"AGHT+IEdR4+ntchX0iI3jwLnRyMkD4hoqTvdXh+/EyQMJ6ZRIaCLi5aMeKORDJxVieUCG0WnFNcfYREwYGnpX7OTWLQ=","X-Received":"by 2002:a05:651c:2224:b0:302:497a:9e5b with SMTP id\n\t38308e7fff4ca-30468548c0bmr144916291fa.2.1735939587115;\n\tFri, 03 Jan 2025 13:26:27 -0800 (PST)","MIME-Version":"1.0","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-4-jacopo.mondi@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Fri, 3 Jan 2025 22:26:15 +0100","Message-ID":"","Subject":"Re: [PATCH 3/8] libcamera: pipeline_handler: Add metadataAvailable()\n\tfunction","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\" "}}]