[{"id":25037,"web_url":"https://patchwork.libcamera.org/comment/25037/","msgid":"<166371565190.18961.16342233382714806161@Monstersaurus>","date":"2022-09-20T23:14:11","subject":"Re: [libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the\n\tactive state and frame context","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:52)\n> Now that data used by algorithms has been partitioned between the active\n> state and frame context, we have a better view of the role of each of\n> those structures. Document them appropriately.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/ipa_context.cpp | 55 ++++++++++++++++++++++++++++------\n> 1 file changed, 46 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index b2628ef73d49..335cb32c538d 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -88,16 +88,28 @@ namespace libcamera::ipa::rkisp1 {\n> * \\struct IPAActiveState\n> * \\brief Active state for algorithms\n> *\n> - * The active state stores algorithm-specific data that needs to be shared\n> - * between multiple algorithms and the IPA module. It is accessible through the\n> - * IPAContext structure.\n> + * The active state contains all algorithm-specific data that need to be\n\n/need/needs/\n\n> + * maintained by algorithms across frames. Unlike the session configuration,\n> + * the active state is mutable and constantly updated by algorithms. The active\n> + * state is accessible through the IPAContext structure.\n> *\n> - * \\todo Split the data contained in this structure between the active state\n> - * and the frame contexts.\n> + * The active state stores two distinct categories of information:\n> *\n> - * Each of the fields in the active state belongs to either a specific\n> - * algorithm, or to the top-level IPA module. A field may be read by any\n> - * algorithm, but should only be written by its owner.\n> + * - The consolidated value of all algorithm controls. Requests passed to\n> + * the queueRequest() function store values for controls that the\n> + * application wants to modify for that particular frame, and the\n> + * queueRequest() function updates the active state with those values.\n> + * The active state thus contains a consolidated view of the value of all\n> + * controls handled by the algorithm.\n> + *\n> + * - The value of parameters computed by the algorithm when running in auto\n> + * mode. Algorithms running in auto mode compute new parameters every\n> + * time statistics buffers are received (either synchronously, or\n> + * possibly in a background thread). The latest computed value of those\n> + * parameters is stored in the active state in the process() function.\n> + *\n> + * Each of the members in the active state belongs to a specific algorithm. A\n> + * member may be read by any algorithm, but shall only be written by its owner.\n> */\n> \n> /**\n> @@ -185,7 +197,32 @@ namespace libcamera::ipa::rkisp1 {\n> * \\struct RkISP1FrameContext\n> * \\brief Per-frame context for algorithms\n> *\n> - * \\todo Populate the frame context for all algorithms\n> + * The frame context stores two distinct categories of information:\n> + *\n> + * - The value of the controls to be applied to the frame. These values are\n> + * typically set in the queueRequest() function, from the consolidated\n> + * control values stored in the active state. The frame context thus stores\n> + * values for all controls related to the algorithm, not limited to the\n> + * controls specified in the corresponding request, but consolidated from all\n> + * requests that have been queued so far.\n> + *\n> + * For controls that can be set manually or computed by an algorithm\n> + * (depending on the algorithm operation mode), such as for instance the\n> + * colour gains for the AWB algorithm, the control value will be stored in\n> + * the frame context in the queueRequest() function only when operating in\n> + * manual mode. When operating in auto mode, the values are computed by the\n> + * algorithm in process(), stored in the active state, and copied to the\n> + * frame context in prepare(), just before being stored in the ISP parameters\n> + * buffer.\n> + *\n> + * The queueRequest() function can also store ancillary data in the frame\n> + * context, such as flags to indicate if (and what) control values have\n> + * changed compared to the previous request.\n> + *\n> + * - Status information computed by the algorithm for a frame. For instance,\n> + * the colour temperature estimated by the AWB algorithm from ISP statistics\n> + * calculated on a frame is stored in the frame context for that frame in\n> + * the process() function.\n\nThat all sounds fine. I wonder if some of the examples might need\ntweaking later, as I think for instance the colour temperature for a\nframe might go directlty into the metadata, as mentioned earlier. The\nFrameContext is really only storing information about a frame that is\nrequired at multiple processing steps/calls. And the ColourTemperature\nis likely not needed (for a specific frame) after it's calculated,\nexcept to put it into the metadata for that frame.\n\n\nReviewed-by: Kieran Bingham \n\n> */\n> \n> /**\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 F4162C0DA4\n\tfor ;\n\tTue, 20 Sep 2022 23:14:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AFDD621D1;\n\tWed, 21 Sep 2022 01:14:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 05E7961F7D\n\tfor ;\n\tWed, 21 Sep 2022 01:14:15 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6FCAC415;\n\tWed, 21 Sep 2022 01:14:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663715656;\n\tbh=L65WbnDS3hFpYhhIAs/loZd2Y5a51urvKviUKfTQZjM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=hd7v7QVXNTRZK1UDZiazCqKOlga3EbPBgyhgqqZtLCI1PShBId+qynVgwvFJwjJAf\n\tSFh7y5kSDDJBGfMH8EG1uKb08KRKiTv/gOe095zlLKohaBfl4FVUwfFuxI78oHge4l\n\tcYO4cbcvCmDt2Vf2w2r8GfoTYHTk8AZCLzmAzjEC2FD1/K54LSSIIV6s1EnU3XXNH2\n\tiEcjTS7oB+6NXdLSAQU3qpxNd0yJKaPtup9MkT5dMmFzLGP0aq2Vp1s6sVc+dOA/t6\n\tFQjGshRQNOrsswmbVUfTBphhxDdIpth9035SjaJWtX1l+eve2+ynqNA6DYcjVFKhwO\n\thnu4B8zYAeKgA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663715654;\n\tbh=L65WbnDS3hFpYhhIAs/loZd2Y5a51urvKviUKfTQZjM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Um140LRzWVb4i1XTKfT6HqDUSiyC6C/H04NDfSf21qTRVmc9FZc8wqUl8+3OwcEyg\n\tNXB7Lg7fI6Idhc1HWu4EBlnEBbE+GoH11CE4nWuHYZulp9eKHYkfZ9nFL8mjJ12L/b\n\t5BXoY1rvza7w48kXtaWTVP+aAkVMthYu3BbVg+xA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Um140LRz\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220908014200.28728-25-laurent.pinchart@ideasonboard.com>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-25-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 21 Sep 2022 00:14:11 +0100","Message-ID":"<166371565190.18961.16342233382714806161@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the\n\tactive state and frame context","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25070,"web_url":"https://patchwork.libcamera.org/comment/25070/","msgid":"<20220921205625.vxnzq4u4bkmfe2ti@uno.localdomain>","date":"2022-09-21T20:56:25","subject":"Re: [libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the\n\tactive state and frame context","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Wed, Sep 21, 2022 at 12:14:11AM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:52)\n> > Now that data used by algorithms has been partitioned between the active\n> > state and frame context, we have a better view of the role of each of\n> > those structures. Document them appropriately.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rkisp1/ipa_context.cpp | 55 ++++++++++++++++++++++++++++------\n> > 1 file changed, 46 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index b2628ef73d49..335cb32c538d 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -88,16 +88,28 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\struct IPAActiveState\n> > * \\brief Active state for algorithms\n> > *\n> > - * The active state stores algorithm-specific data that needs to be shared\n> > - * between multiple algorithms and the IPA module. It is accessible through the\n> > - * IPAContext structure.\n> > + * The active state contains all algorithm-specific data that need to be\n>\n> /need/needs/\n>\n> > + * maintained by algorithms across frames. Unlike the session configuration,\n> > + * the active state is mutable and constantly updated by algorithms. The active\n> > + * state is accessible through the IPAContext structure.\n\nOnce IPA will be threaded, if ever, it will be sweet to implement and\ndebug concurrent accesses to the active state.\n\n> > *\n> > - * \\todo Split the data contained in this structure between the active state\n> > - * and the frame contexts.\n> > + * The active state stores two distinct categories of information:\n> > *\n> > - * Each of the fields in the active state belongs to either a specific\n> > - * algorithm, or to the top-level IPA module. A field may be read by any\n> > - * algorithm, but should only be written by its owner.\n> > + * - The consolidated value of all algorithm controls. Requests passed to\n> > + * the queueRequest() function store values for controls that the\n> > + * application wants to modify for that particular frame, and the\n> > + * queueRequest() function updates the active state with those values.\n> > + * The active state thus contains a consolidated view of the value of all\n> > + * controls handled by the algorithm.\n\nI think what we actually store are only controls that change the\nalgorithm state (enable/disable). What are the examples of\n\"consolidate control lists\" in the active state ?\n\n> > + *\n> > + * - The value of parameters computed by the algorithm when running in auto\n> > + * mode. Algorithms running in auto mode compute new parameters every\n> > + * time statistics buffers are received (either synchronously, or\n> > + * possibly in a background thread). The latest computed value of those\n> > + * parameters is stored in the active state in the process() function.\n\nAWB stores manual controls, and I understand it might be an outliner,\nbut doesn't the active state actually store the last values -applied-\nby an algorithm, being it from the manual or the auto mode ?\n\n> > + *\n> > + * Each of the members in the active state belongs to a specific algorithm. A\n> > + * member may be read by any algorithm, but shall only be written by its owner.\n> > */\n> >\n> > /**\n> > @@ -185,7 +197,32 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\struct RkISP1FrameContext\n> > * \\brief Per-frame context for algorithms\n> > *\n> > - * \\todo Populate the frame context for all algorithms\n> > + * The frame context stores two distinct categories of information:\n> > + *\n> > + * - The value of the controls to be applied to the frame. These values are\n> > + * typically set in the queueRequest() function, from the consolidated\n> > + * control values stored in the active state. The frame context thus stores\n> > + * values for all controls related to the algorithm, not limited to the\n> > + * controls specified in the corresponding request, but consolidated from all\n> > + * requests that have been queued so far.\n> > + *\n> > + * For controls that can be set manually or computed by an algorithm\n> > + * (depending on the algorithm operation mode), such as for instance the\n> > + * colour gains for the AWB algorithm, the control value will be stored in\n> > + * the frame context in the queueRequest() function only when operating in\n> > + * manual mode. When operating in auto mode, the values are computed by the\n> > + * algorithm in process(), stored in the active state, and copied to the\n> > + * frame context in prepare(), just before being stored in the ISP parameters\n> > + * buffer.\n\nI presume AEGC will behave the same, so this doesn't make AWB an\noutliner, but the above will become a pattern.\n\nReviewed-by: Jacopo Mondi \n\n\n\n> > + *\n> > + * The queueRequest() function can also store ancillary data in the frame\n> > + * context, such as flags to indicate if (and what) control values have\n> > + * changed compared to the previous request.\n> > + *\n> > + * - Status information computed by the algorithm for a frame. For instance,\n> > + * the colour temperature estimated by the AWB algorithm from ISP statistics\n> > + * calculated on a frame is stored in the frame context for that frame in\n> > + * the process() function.\n>\n> That all sounds fine. I wonder if some of the examples might need\n> tweaking later, as I think for instance the colour temperature for a\n> frame might go directlty into the metadata, as mentioned earlier. The\n> FrameContext is really only storing information about a frame that is\n> required at multiple processing steps/calls. And the ColourTemperature\n> is likely not needed (for a specific frame) after it's calculated,\n> except to put it into the metadata for that frame.\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n> > */\n> >\n> > /**\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 3B2E1C3272\n\tfor ;\n\tWed, 21 Sep 2022 20:56:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C832621FE;\n\tWed, 21 Sep 2022 22:56:29 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A57F600AA\n\tfor ;\n\tWed, 21 Sep 2022 22:56:28 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 50DE540002;\n\tWed, 21 Sep 2022 20:56:27 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663793789;\n\tbh=jDpxtkz6OvovHaj6ajh0gyvh0r1/d/AJCsq4gJsTofA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ZlMwOYTRuFozztD3TSFHyu5v7Kva+2gW8gkzwtc1T+lDGExO37SFL6qw1uqiCyHxA\n\tMZFXD+uwKxvl/s6xz9WXAWikb8jNkq4PtuOb764gYK0QbM2PEdHY4ifQuGvVoPkX2H\n\t5sWXvzjo3aZ7C5Tpy2MmeEP7NriPW/YNGebOfYBYPgj9kD2XOCFtgsfnHqPjsb+6Iw\n\tO5zAs7Wm/JRvINAjdkPwvCWkpbeldZallj2nnUpxQ3tINxXAgcBZuNFrNvJyelpuww\n\tCh295+eIep6vNY+HNIALOaZR5VTJn1zu0g80W6kUa9b2qA/kfSTiw1hSInof1Cd9m1\n\tVJq/V52i1R9Hg==","Date":"Wed, 21 Sep 2022 22:56:25 +0200","To":"Kieran Bingham ","Message-ID":"<20220921205625.vxnzq4u4bkmfe2ti@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-25-laurent.pinchart@ideasonboard.com>\n\t<166371565190.18961.16342233382714806161@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166371565190.18961.16342233382714806161@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the\n\tactive state and frame context","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25095,"web_url":"https://patchwork.libcamera.org/comment/25095/","msgid":"","date":"2022-09-22T20:35:48","subject":"Re: [libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the\n\tactive state and frame context","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Sep 21, 2022 at 12:14:11AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:52)\n> > Now that data used by algorithms has been partitioned between the active\n> > state and frame context, we have a better view of the role of each of\n> > those structures. Document them appropriately.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rkisp1/ipa_context.cpp | 55 ++++++++++++++++++++++++++++------\n> > 1 file changed, 46 insertions(+), 9 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index b2628ef73d49..335cb32c538d 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -88,16 +88,28 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\struct IPAActiveState\n> > * \\brief Active state for algorithms\n> > *\n> > - * The active state stores algorithm-specific data that needs to be shared\n> > - * between multiple algorithms and the IPA module. It is accessible through the\n> > - * IPAContext structure.\n> > + * The active state contains all algorithm-specific data that need to be\n> \n> /need/needs/\n> \n> > + * maintained by algorithms across frames. Unlike the session configuration,\n> > + * the active state is mutable and constantly updated by algorithms. The active\n> > + * state is accessible through the IPAContext structure.\n> > *\n> > - * \\todo Split the data contained in this structure between the active state\n> > - * and the frame contexts.\n> > + * The active state stores two distinct categories of information:\n> > *\n> > - * Each of the fields in the active state belongs to either a specific\n> > - * algorithm, or to the top-level IPA module. A field may be read by any\n> > - * algorithm, but should only be written by its owner.\n> > + * - The consolidated value of all algorithm controls. Requests passed to\n> > + * the queueRequest() function store values for controls that the\n> > + * application wants to modify for that particular frame, and the\n> > + * queueRequest() function updates the active state with those values.\n> > + * The active state thus contains a consolidated view of the value of all\n> > + * controls handled by the algorithm.\n> > + *\n> > + * - The value of parameters computed by the algorithm when running in auto\n> > + * mode. Algorithms running in auto mode compute new parameters every\n> > + * time statistics buffers are received (either synchronously, or\n> > + * possibly in a background thread). The latest computed value of those\n> > + * parameters is stored in the active state in the process() function.\n> > + *\n> > + * Each of the members in the active state belongs to a specific algorithm. A\n> > + * member may be read by any algorithm, but shall only be written by its owner.\n> > */\n> > \n> > /**\n> > @@ -185,7 +197,32 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\struct RkISP1FrameContext\n> > * \\brief Per-frame context for algorithms\n> > *\n> > - * \\todo Populate the frame context for all algorithms\n> > + * The frame context stores two distinct categories of information:\n> > + *\n> > + * - The value of the controls to be applied to the frame. These values are\n> > + * typically set in the queueRequest() function, from the consolidated\n> > + * control values stored in the active state. The frame context thus stores\n> > + * values for all controls related to the algorithm, not limited to the\n> > + * controls specified in the corresponding request, but consolidated from all\n> > + * requests that have been queued so far.\n> > + *\n> > + * For controls that can be set manually or computed by an algorithm\n> > + * (depending on the algorithm operation mode), such as for instance the\n> > + * colour gains for the AWB algorithm, the control value will be stored in\n> > + * the frame context in the queueRequest() function only when operating in\n> > + * manual mode. When operating in auto mode, the values are computed by the\n> > + * algorithm in process(), stored in the active state, and copied to the\n> > + * frame context in prepare(), just before being stored in the ISP parameters\n> > + * buffer.\n> > + *\n> > + * The queueRequest() function can also store ancillary data in the frame\n> > + * context, such as flags to indicate if (and what) control values have\n> > + * changed compared to the previous request.\n> > + *\n> > + * - Status information computed by the algorithm for a frame. For instance,\n> > + * the colour temperature estimated by the AWB algorithm from ISP statistics\n> > + * calculated on a frame is stored in the frame context for that frame in\n> > + * the process() function.\n> \n> That all sounds fine. I wonder if some of the examples might need\n> tweaking later, as I think for instance the colour temperature for a\n> frame might go directlty into the metadata, as mentioned earlier. The\n\nI agree. I'll probably rewrite this section to mention metadata instead\nof colour temperature then.\n\n> FrameContext is really only storing information about a frame that is\n> required at multiple processing steps/calls. And the ColourTemperature\n> is likely not needed (for a specific frame) after it's calculated,\n> except to put it into the metadata for that frame.\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> > */\n> > \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 DD741BD16B\n\tfor ;\n\tThu, 22 Sep 2022 20:36:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 25A6E6221B;\n\tThu, 22 Sep 2022 22:36:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E56E66219A\n\tfor ;\n\tThu, 22 Sep 2022 22:36:03 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4285F6BE;\n\tThu, 22 Sep 2022 22:36:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663878965;\n\tbh=/ewHLj90vlS2DEGS6OteC4l4XpyUs34LbLCmFpU6BSg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=jJGRv8A5Lj0IHc7lA+Ce+lX1wi5NPsUEwYbhEdMm4bMNTFTTZSaHANZ1njAEiF7cY\n\tIvkltOiOMxUabgJuhKH692W4RkusskVVXA/JSDzRNQkjBKhD0Tdd12mnObKT7+GGuL\n\tNify9MzQSK10YcO6lzSggK0OqccyHeJPi2eBVjcDHy1cxPHyofjBMKap/jgzfRZl89\n\thoIiOif8XUpCKfIg9wX+PQnT5DRFzqZmJ0H15UQm9upwOHYttQTq57RwTNhBJZJ4OD\n\tU4IzAWFWkg9NsGmcNxlDJNdqcVliq8T+CIInktasCDFSvSQxY70d5bKgkHmJQUyrvr\n\tndn6Rxgr192/A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663878963;\n\tbh=/ewHLj90vlS2DEGS6OteC4l4XpyUs34LbLCmFpU6BSg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lfwZIkoCkVuRHIV4S56sFR34KNEgISvYCcbGLUARwnMfBWLoQApEkjW52LtO8H8Nc\n\tT4HjZK3lQRzoU1dVKzYxyrWDawrTufu01v7EyRTxNb3/oXUMc0Czc4jN2/y3doNNh3\n\tljMeA0rvIC+OIiF+t7/8zpZt7iU1NXvwb9dkhlkQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lfwZIkoC\"; dkim-atps=neutral","Date":"Thu, 22 Sep 2022 23:35:48 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-25-laurent.pinchart@ideasonboard.com>\n\t<166371565190.18961.16342233382714806161@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166371565190.18961.16342233382714806161@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the\n\tactive state and frame context","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25096,"web_url":"https://patchwork.libcamera.org/comment/25096/","msgid":"","date":"2022-09-22T20:41:01","subject":"Re: [libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the\n\tactive state and frame context","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 21, 2022 at 10:56:25PM +0200, Jacopo Mondi wrote:\n> On Wed, Sep 21, 2022 at 12:14:11AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:52)\n> > > Now that data used by algorithms has been partitioned between the active\n> > > state and frame context, we have a better view of the role of each of\n> > > those structures. Document them appropriately.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/ipa/rkisp1/ipa_context.cpp | 55 ++++++++++++++++++++++++++++------\n> > > 1 file changed, 46 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > index b2628ef73d49..335cb32c538d 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > @@ -88,16 +88,28 @@ namespace libcamera::ipa::rkisp1 {\n> > > * \\struct IPAActiveState\n> > > * \\brief Active state for algorithms\n> > > *\n> > > - * The active state stores algorithm-specific data that needs to be shared\n> > > - * between multiple algorithms and the IPA module. It is accessible through the\n> > > - * IPAContext structure.\n> > > + * The active state contains all algorithm-specific data that need to be\n> >\n> > /need/needs/\n> >\n> > > + * maintained by algorithms across frames. Unlike the session configuration,\n> > > + * the active state is mutable and constantly updated by algorithms. The active\n> > > + * state is accessible through the IPAContext structure.\n> \n> Once IPA will be threaded, if ever, it will be sweet to implement and\n> debug concurrent accesses to the active state.\n\nI think the active state should then only be accessed from the main IPA\nthread, with all data that needs to be accessed from multiple threads\nbeing stored inside individual algorithms.\n\n> > > *\n> > > - * \\todo Split the data contained in this structure between the active state\n> > > - * and the frame contexts.\n> > > + * The active state stores two distinct categories of information:\n> > > *\n> > > - * Each of the fields in the active state belongs to either a specific\n> > > - * algorithm, or to the top-level IPA module. A field may be read by any\n> > > - * algorithm, but should only be written by its owner.\n> > > + * - The consolidated value of all algorithm controls. Requests passed to\n> > > + * the queueRequest() function store values for controls that the\n> > > + * application wants to modify for that particular frame, and the\n> > > + * queueRequest() function updates the active state with those values.\n> > > + * The active state thus contains a consolidated view of the value of all\n> > > + * controls handled by the algorithm.\n> \n> I think what we actually store are only controls that change the\n> algorithm state (enable/disable). What are the examples of\n> \"consolidate control lists\" in the active state ?\n\nNote that I intentionally didn't mention control list here.\n\nWe store the value of all controls in the active state. Looking at the\nfilter algorithm for instance, the active state stores the sharpness\nvalue, which comes directly from the request controls (after clamping to\nthe valid range and converting to an integer).\n\n> > > + *\n> > > + * - The value of parameters computed by the algorithm when running in auto\n> > > + * mode. Algorithms running in auto mode compute new parameters every\n> > > + * time statistics buffers are received (either synchronously, or\n> > > + * possibly in a background thread). The latest computed value of those\n> > > + * parameters is stored in the active state in the process() function.\n> \n> AWB stores manual controls, and I understand it might be an outliner,\n> but doesn't the active state actually store the last values -applied-\n> by an algorithm, being it from the manual or the auto mode ?\n\nThe AWB algorithm stores both the manual and auto values in the active\nstate.\n\n> > > + *\n> > > + * Each of the members in the active state belongs to a specific algorithm. A\n> > > + * member may be read by any algorithm, but shall only be written by its owner.\n> > > */\n> > >\n> > > /**\n> > > @@ -185,7 +197,32 @@ namespace libcamera::ipa::rkisp1 {\n> > > * \\struct RkISP1FrameContext\n> > > * \\brief Per-frame context for algorithms\n> > > *\n> > > - * \\todo Populate the frame context for all algorithms\n> > > + * The frame context stores two distinct categories of information:\n> > > + *\n> > > + * - The value of the controls to be applied to the frame. These values are\n> > > + * typically set in the queueRequest() function, from the consolidated\n> > > + * control values stored in the active state. The frame context thus stores\n> > > + * values for all controls related to the algorithm, not limited to the\n> > > + * controls specified in the corresponding request, but consolidated from all\n> > > + * requests that have been queued so far.\n> > > + *\n> > > + * For controls that can be set manually or computed by an algorithm\n> > > + * (depending on the algorithm operation mode), such as for instance the\n> > > + * colour gains for the AWB algorithm, the control value will be stored in\n> > > + * the frame context in the queueRequest() function only when operating in\n> > > + * manual mode. When operating in auto mode, the values are computed by the\n> > > + * algorithm in process(), stored in the active state, and copied to the\n> > > + * frame context in prepare(), just before being stored in the ISP parameters\n> > > + * buffer.\n> \n> I presume AEGC will behave the same, so this doesn't make AWB an\n> outliner, but the above will become a pattern.\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > > + *\n> > > + * The queueRequest() function can also store ancillary data in the frame\n> > > + * context, such as flags to indicate if (and what) control values have\n> > > + * changed compared to the previous request.\n> > > + *\n> > > + * - Status information computed by the algorithm for a frame. For instance,\n> > > + * the colour temperature estimated by the AWB algorithm from ISP statistics\n> > > + * calculated on a frame is stored in the frame context for that frame in\n> > > + * the process() function.\n> >\n> > That all sounds fine. I wonder if some of the examples might need\n> > tweaking later, as I think for instance the colour temperature for a\n> > frame might go directlty into the metadata, as mentioned earlier. The\n> > FrameContext is really only storing information about a frame that is\n> > required at multiple processing steps/calls. And the ColourTemperature\n> > is likely not needed (for a specific frame) after it's calculated,\n> > except to put it into the metadata for that frame.\n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > */\n> > >\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 A99F2C0DA4\n\tfor ;\n\tThu, 22 Sep 2022 20:41:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18CEF6221D;\n\tThu, 22 Sep 2022 22:41:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC2A16219A\n\tfor ;\n\tThu, 22 Sep 2022 22:41:16 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F37E06BE;\n\tThu, 22 Sep 2022 22:41:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663879278;\n\tbh=/y/ISiQweRVz0uHgSDbLGjGbAkMYjNl+OpZD84LM9gg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=TRWs11Off+qOsZ14jsIxtK4o1mVt5pcMUNb1IWUSiEIz1h7h2Zn9j7NObsXsMrENz\n\th7u3NYGgw4OZEZeXUlsj97w1doohmTAILByl/3Tcek8xsGy1MZ1GQ1AQJvoXJ5yFLc\n\tA3t3O6lCQDXtQu/AXFvot11V7JQDpibZBnlLoW+lCboFCnClXbYAbS/31MN94L9KEP\n\tdFZwHEixrA6nljATeKxamzWZn6xTaKo9fICWoh+F3HCZX2jJwknm64/X4DKsVxxgHL\n\tw7lQBjVW7XrOr3/Qcq26CJ9p9sSHxTJMs6SVLigu/L/C546MwNuQ7P07rEn4SZfnDY\n\twHorPz5hPDrFw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663879276;\n\tbh=/y/ISiQweRVz0uHgSDbLGjGbAkMYjNl+OpZD84LM9gg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zngr/+nLSH5gWrMG0EJ7N8PhLyDVXVZZPkQCBgRXLIjO6arQkbw4q9sUBum8PxMOI\n\tI+uFDLaG/ucVhATUTEEtLaV8qHKTGPoGdAmH8uMUUcpA03C/F3asbuka/dS7eJ3NQ2\n\tDXirmIYJ/l4/dOhrdR/7iEsa59usruGn3SbvPUzY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Zngr/+nL\"; dkim-atps=neutral","Date":"Thu, 22 Sep 2022 23:41:01 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-25-laurent.pinchart@ideasonboard.com>\n\t<166371565190.18961.16342233382714806161@Monstersaurus>\n\t<20220921205625.vxnzq4u4bkmfe2ti@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220921205625.vxnzq4u4bkmfe2ti@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the\n\tactive state and frame context","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]