[{"id":31842,"web_url":"https://patchwork.libcamera.org/comment/31842/","msgid":"","date":"2024-10-21T03:25:14","subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote:\n> The RkISP1 AGC algorithms assumes the metering mode to be\n> \"MeteringMatrix\" and pre-fill an array of weights associated\n> with it stored in meteringModes_[MeteringMatrix] when intializing\n> the algorithm in parseMeteringModes().\n> \n> It laters fetches the weights when computing parameters using the\n> FrameContext.agc.meteringMode as index of the meteringModes_ array.\n> \n> When a Request gets queued to the algorithm, the\n> FrameContext.agc.meteringMode index is populated from the algorithm's\n> active state, and optionally updated if the application has specified\n> a different metering mode.\n> \n> In case of Request underrun however, a non-initialized FrameContext\n> gets provided to the algorithm, causing an (unhandled) exception when\n> accessing the meteringModes_ array.\n> \n> Fix this by intializing the AGC metering mode to a supported value\n> coming from the ActiveState in IPAFrameContext::init().\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 2 ++\n> src/ipa/rkisp1/ipa_context.cpp | 5 +++++\n> 2 files changed, 7 insertions(+)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 17d074d9c03e..dd7e9468741e 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t\tstatic_cast(constraintModes().begin()->first);\n> \tcontext.activeState.agc.exposureMode =\n> \t\tstatic_cast(exposureModeHelpers().begin()->first);\n> +\n> +\t/* Use the metering matrix mode by default. */\n\nStrictly speaking, this isn't where the metering mode is defaulted to\nmatrix; it's in parseMeteringModes().\n\n> \tcontext.activeState.agc.meteringMode =\n> \t\tstatic_cast(meteringModes_.begin()->first);\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 2dad42b3154f..e88609137345 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum,\n> \t\t\t const ActiveState &activeState)\n> {\n> \tFrameContext::init(frameNum, activeState);\n> +\n> +\tconst IPAActiveState *rkisp1ActiveState =\n> +\t\treinterpret_cast(&activeState);\n> +\n> +\tagc.meteringMode = rkisp1ActiveState->agc.meteringMode;\n\nBut yes this looks like the right solution.\n\nReviewed-by: Paul Elder \n\n> }\n> \n> /**\n> -- \n> 2.47.0\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 004A7C32A3\n\tfor ;\n\tMon, 21 Oct 2024 03:25:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49E6865391;\n\tMon, 21 Oct 2024 05:25:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF69760395\n\tfor ;\n\tMon, 21 Oct 2024 05:25:21 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6758:2ada:48a8:78df])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1DF2E74A;\n\tMon, 21 Oct 2024 05:23:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cYJVV25e\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729481016;\n\tbh=LkPdTfDwzsZ48d+zwAimpXoULu8EL2gdXDhfRXXMKEs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cYJVV25ezxlwihaKaXadh+Hwb/Jn/lYETXRZ3mRAn3DQBDNu89QgFxQN1Q1Zj3esc\n\tKO63Y3IPD2/lwtweeYD9XB/cEk2EOMKEF75LPJyh8uV4wLe4BQTptif/k7kXFC0/J0\n\tnxjQSELN4BQyfJg1LFJQ+yfsV6uySiyMdxFCTQIA=","Date":"Mon, 21 Oct 2024 12:25:14 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-5-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241016170348.715993-5-jacopo.mondi@ideasonboard.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":31944,"web_url":"https://patchwork.libcamera.org/comment/31944/","msgid":"","date":"2024-10-28T16:47:28","subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch. \n\nOn Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote:\n> The RkISP1 AGC algorithms assumes the metering mode to be\n> \"MeteringMatrix\" and pre-fill an array of weights associated\n> with it stored in meteringModes_[MeteringMatrix] when intializing\n> the algorithm in parseMeteringModes().\n> \n> It laters fetches the weights when computing parameters using the\n> FrameContext.agc.meteringMode as index of the meteringModes_ array.\n> \n> When a Request gets queued to the algorithm, the\n> FrameContext.agc.meteringMode index is populated from the algorithm's\n> active state, and optionally updated if the application has specified\n> a different metering mode.\n> \n> In case of Request underrun however, a non-initialized FrameContext\n> gets provided to the algorithm, causing an (unhandled) exception when\n> accessing the meteringModes_ array.\n> \n> Fix this by intializing the AGC metering mode to a supported value\n> coming from the ActiveState in IPAFrameContext::init().\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 2 ++\n> src/ipa/rkisp1/ipa_context.cpp | 5 +++++\n> 2 files changed, 7 insertions(+)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 17d074d9c03e..dd7e9468741e 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t\tstatic_cast(constraintModes().begin()->first);\n> \tcontext.activeState.agc.exposureMode =\n> \t\tstatic_cast(exposureModeHelpers().begin()->first);\n> +\n> +\t/* Use the metering matrix mode by default. */\n> \tcontext.activeState.agc.meteringMode =\n> \t\tstatic_cast(meteringModes_.begin()->first);\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 2dad42b3154f..e88609137345 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum,\n> \t\t\t const ActiveState &activeState)\n> {\n> \tFrameContext::init(frameNum, activeState);\n> +\n> +\tconst IPAActiveState *rkisp1ActiveState =\n> +\t\treinterpret_cast(&activeState);\n> +\n> +\tagc.meteringMode = rkisp1ActiveState->agc.meteringMode;\n\nI don't particularly like that the IPAFrameContext needs that knowledge\nwhich should be internal to the agc algorithm. We could add a\ninitContext() function to the algorithm base class. But then we need to\nloop over all algorithms here, which doesn't feel efficient for a single\ncase. But on the other hand if we assume that request underruns become\nmore common, then the other algorithms need to be able to continue with\ncurrent data without a prior queueRequest() call. While writing this,\nyes I think we should forward that to every algorithm. Otherwise we\nmight end up with strange effects because the algorithms use the data\nfrom the frame context that was reused without proper initialization.\n\nI hope it is somehow understandable.\n\nCheers,\nStefan\n\n> }\n> \n> /**\n> -- \n> 2.47.0\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 88477BD78E\n\tfor ;\n\tMon, 28 Oct 2024 16:47:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B27E16539E;\n\tMon, 28 Oct 2024 17:47:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 55B3F60367\n\tfor ;\n\tMon, 28 Oct 2024 17:47:32 +0100 (CET)","from ideasonboard.com (tmo-120-11.customers.d1-online.com\n\t[80.187.120.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D7D7641;\n\tMon, 28 Oct 2024 17:47:30 +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=\"rBNFAl9r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730134050;\n\tbh=Dcw6qoXR9ifR67YjZPL9z1RTx6UWbPy5v8fbIufN8ko=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rBNFAl9r7/qY4bJ/Cn2YvX8THuNtzTvLoEKRMV1T8EZLUZcgsD39KfrMiz+tkbm74\n\tAgbtpqOqF0DOzUqSgLKifKcXJDjfbiKbljWmbbTUiXklEQlr8pTrrIKUrNLNhv93SR\n\tITIw9UBvnnwGeM65Q7LfNyg/vBsgYKJbJzkYSGEA=","Date":"Mon, 28 Oct 2024 17:47:28 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-5-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241016170348.715993-5-jacopo.mondi@ideasonboard.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":31945,"web_url":"https://patchwork.libcamera.org/comment/31945/","msgid":"","date":"2024-10-28T17:14:42","subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote:\n> > The RkISP1 AGC algorithms assumes the metering mode to be\n> > \"MeteringMatrix\" and pre-fill an array of weights associated\n> > with it stored in meteringModes_[MeteringMatrix] when intializing\n> > the algorithm in parseMeteringModes().\n> >\n> > It laters fetches the weights when computing parameters using the\n> > FrameContext.agc.meteringMode as index of the meteringModes_ array.\n> >\n> > When a Request gets queued to the algorithm, the\n> > FrameContext.agc.meteringMode index is populated from the algorithm's\n> > active state, and optionally updated if the application has specified\n> > a different metering mode.\n> >\n> > In case of Request underrun however, a non-initialized FrameContext\n> > gets provided to the algorithm, causing an (unhandled) exception when\n> > accessing the meteringModes_ array.\n> >\n> > Fix this by intializing the AGC metering mode to a supported value\n> > coming from the ActiveState in IPAFrameContext::init().\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++\n> > src/ipa/rkisp1/ipa_context.cpp | 5 +++++\n> > 2 files changed, 7 insertions(+)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 17d074d9c03e..dd7e9468741e 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \t\tstatic_cast(constraintModes().begin()->first);\n> > \tcontext.activeState.agc.exposureMode =\n> > \t\tstatic_cast(exposureModeHelpers().begin()->first);\n> > +\n> > +\t/* Use the metering matrix mode by default. */\n> > \tcontext.activeState.agc.meteringMode =\n> > \t\tstatic_cast(meteringModes_.begin()->first);\n> >\n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 2dad42b3154f..e88609137345 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum,\n> > \t\t\t const ActiveState &activeState)\n> > {\n> > \tFrameContext::init(frameNum, activeState);\n> > +\n> > +\tconst IPAActiveState *rkisp1ActiveState =\n> > +\t\treinterpret_cast(&activeState);\n> > +\n> > +\tagc.meteringMode = rkisp1ActiveState->agc.meteringMode;\n>\n> I don't particularly like that the IPAFrameContext needs that knowledge\n> which should be internal to the agc algorithm. We could add a\n\nYou know, my feeling was the opposite. Algorithms should make less\nassumptions about their internals implementation details but rely on a\nmore globally shared state, kept in the FrameContext and the\nActiveState. This would make easy to \"swap\" algoroithms.\n\nAfter all, this already shows the AGC algo depends on a precise\nordering of operations which if not respected results in a run-time\nexception.\n\n> initContext() function to the algorithm base class. But then we need to\n> loop over all algorithms here, which doesn't feel efficient for a single\n> case. But on the other hand if we assume that request underruns become\n\nI considered that but it seemed rather non-efficient.\n\n> more common, then the other algorithms need to be able to continue with\n> current data without a prior queueRequest() call. While writing this,\n\nIsn't the \"current data\" exactly the active state ?\n\n> yes I think we should forward that to every algorithm. Otherwise we\n> might end up with strange effects because the algorithms use the data\n> from the frame context that was reused without proper initialization.\n\nI might be missing how a FrameContext could be \"reused\" and \"without\nproper initialization\". I understand a FrameContext might not\ninitialized, or not correctly re-initalized but if that happens is an\nimplementation issue, isn't it ?\n\n>\n> I hope it is somehow understandable.\n\nThank you!\n j\n\n>\n> Cheers,\n> Stefan\n>\n> > }\n> >\n> > /**\n> > --\n> > 2.47.0\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 A9819C3220\n\tfor ;\n\tMon, 28 Oct 2024 17:15:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23F706539E;\n\tMon, 28 Oct 2024 18:15:03 +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 DF57C60367\n\tfor ;\n\tMon, 28 Oct 2024 18:15:01 +0100 (CET)","from ideasonboard.com (mob-5-90-59-111.net.vodafone.it\n\t[5.90.59.111])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFF6D43;\n\tMon, 28 Oct 2024 18:14:58 +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=\"oNLIJIUP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730135699;\n\tbh=qBAjYpp22RNOUDMfyDV5J3pGI2KM4kc5a2j77Bt+eLs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oNLIJIUPfBVAiTtKjl+RTKIOY/RjoCminRM/H/cBidKxpXwUbZFMuNy7hKCPYWWHU\n\tKQvdy9JHFiTCv09/HZrDalgUsInD6jO1lAy8OVshNeQFtoiURMPJfYh0kO8+IR7dxO\n\tKz7mv1+t3xoJSqcRgvmWVKhv3CSLlTvAEWXKNT5w=","Date":"Mon, 28 Oct 2024 18:14:42 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-5-jacopo.mondi@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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":31947,"web_url":"https://patchwork.libcamera.org/comment/31947/","msgid":"<7e2qocsrys7jszmk6vibmbfzlff2coskcmghtx4wyqv7odmnue@h7g3hlgse5e2>","date":"2024-10-28T18:12:57","subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Oct 28, 2024 at 06:14:42PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote:\n> > > The RkISP1 AGC algorithms assumes the metering mode to be\n> > > \"MeteringMatrix\" and pre-fill an array of weights associated\n> > > with it stored in meteringModes_[MeteringMatrix] when intializing\n> > > the algorithm in parseMeteringModes().\n> > >\n> > > It laters fetches the weights when computing parameters using the\n> > > FrameContext.agc.meteringMode as index of the meteringModes_ array.\n> > >\n> > > When a Request gets queued to the algorithm, the\n> > > FrameContext.agc.meteringMode index is populated from the algorithm's\n> > > active state, and optionally updated if the application has specified\n> > > a different metering mode.\n> > >\n> > > In case of Request underrun however, a non-initialized FrameContext\n> > > gets provided to the algorithm, causing an (unhandled) exception when\n> > > accessing the meteringModes_ array.\n> > >\n> > > Fix this by intializing the AGC metering mode to a supported value\n> > > coming from the ActiveState in IPAFrameContext::init().\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++\n> > > src/ipa/rkisp1/ipa_context.cpp | 5 +++++\n> > > 2 files changed, 7 insertions(+)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 17d074d9c03e..dd7e9468741e 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > \t\tstatic_cast(constraintModes().begin()->first);\n> > > \tcontext.activeState.agc.exposureMode =\n> > > \t\tstatic_cast(exposureModeHelpers().begin()->first);\n> > > +\n> > > +\t/* Use the metering matrix mode by default. */\n> > > \tcontext.activeState.agc.meteringMode =\n> > > \t\tstatic_cast(meteringModes_.begin()->first);\n> > >\n> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > index 2dad42b3154f..e88609137345 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum,\n> > > \t\t\t const ActiveState &activeState)\n> > > {\n> > > \tFrameContext::init(frameNum, activeState);\n> > > +\n> > > +\tconst IPAActiveState *rkisp1ActiveState =\n> > > +\t\treinterpret_cast(&activeState);\n> > > +\n> > > +\tagc.meteringMode = rkisp1ActiveState->agc.meteringMode;\n> >\n> > I don't particularly like that the IPAFrameContext needs that knowledge\n> > which should be internal to the agc algorithm. We could add a\n> \n> You know, my feeling was the opposite. Algorithms should make less\n> assumptions about their internals implementation details but rely on a\n> more globally shared state, kept in the FrameContext and the\n> ActiveState. This would make easy to \"swap\" algoroithms.\n> \n> After all, this already shows the AGC algo depends on a precise\n> ordering of operations which if not respected results in a run-time\n> exception.\n> \n> > initContext() function to the algorithm base class. But then we need to\n> > loop over all algorithms here, which doesn't feel efficient for a single\n> > case. But on the other hand if we assume that request underruns become\n> \n> I considered that but it seemed rather non-efficient.\n\nAgreed, but I'm not sure if we can completely avoid it. Maybe with some\nmore restructuring...\n\n> \n> > more common, then the other algorithms need to be able to continue with\n> > current data without a prior queueRequest() call. While writing this,\n> \n> Isn't the \"current data\" exactly the active state ?\n\nThe active state is the current measuered/stats data. But the current manual\n(user supplied) data is stored in the frame context.\n\n> \n> > yes I think we should forward that to every algorithm. Otherwise we\n> > might end up with strange effects because the algorithms use the data\n> > from the frame context that was reused without proper initialization.\n> \n> I might be missing how a FrameContext could be \"reused\" and \"without\n> proper initialization\". I understand a FrameContext might not\n> initialized, or not correctly re-initalized but if that happens is an\n> implementation issue, isn't it ?\n\nLet's see if I can get this straight. For reference I take the awb algo.\nLet's assume the following gets queued:\n\nFrame 1: awb.autoEnable = false, gain.blue = 4\nFrame 2: awb.autoEnable = true\n... no more requests get queued ...\n\nLet's further assume the FCQueue has a length of 2. Now no more requests\nget queued. But process() still get's called because the internal\nmachinery is now ticked by the sensor producing images. That results in\na cyclic reuse of the two frame comtexts. Now the blue gain gets\nconstantly toggled between 4 and the automatic value. As the stats\ncalculation depends on the gains there will be unexpected effects.\n\nI expect there are more cases like this and I'm not sure if we should\nhandle them inside FrameContext::init()\n\nMaybe I'm missing something obvious...\n\nCheers,\nStefan\n\n> \n> >\n> > I hope it is somehow understandable.\n> \n> Thank you!\n> j\n> \n> >\n> > Cheers,\n> > Stefan\n> >\n> > > }\n> > >\n> > > /**\n> > > --\n> > > 2.47.0\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 52DFBC3220\n\tfor ;\n\tMon, 28 Oct 2024 18:13:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27DC66539E;\n\tMon, 28 Oct 2024 19:13:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63E3B60367\n\tfor ;\n\tMon, 28 Oct 2024 19:13:02 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a01:599:708:840f:814a:6b8b:c97e:5da6])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4DBA3641;\n\tMon, 28 Oct 2024 19:13:00 +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=\"bMgOVSxC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730139180;\n\tbh=dNfQVVZykbGs9e6rhML95aeu0tdxUJPq0prA6cKdfb8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bMgOVSxCKWRsAfCt4A7nap+2DFE11S4dWMxaio4mD43/GN/TA3Zx/OwT39PuTMEq3\n\tEOxe2R0k/Liq1xBglvS4ERVy4OAw0knjsn/tLKbC+LF8XZzI28AbtbRYaZnt5lg967\n\tnvl3cDj3ss4AS6yMEcPo2Ou3fwMvKfdtXC8IyGyM=","Date":"Mon, 28 Oct 2024 19:12:57 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","Message-ID":"<7e2qocsrys7jszmk6vibmbfzlff2coskcmghtx4wyqv7odmnue@h7g3hlgse5e2>","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-5-jacopo.mondi@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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":31950,"web_url":"https://patchwork.libcamera.org/comment/31950/","msgid":"<5tnul6d3k2gggvrk52cepoczkhorpawzz2tzmhxnljf4lhqx7k@irem5a4u6yie>","date":"2024-10-29T07:49:26","subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Oct 28, 2024 at 07:12:57PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> On Mon, Oct 28, 2024 at 06:14:42PM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote:\n> > > > The RkISP1 AGC algorithms assumes the metering mode to be\n> > > > \"MeteringMatrix\" and pre-fill an array of weights associated\n> > > > with it stored in meteringModes_[MeteringMatrix] when intializing\n> > > > the algorithm in parseMeteringModes().\n> > > >\n> > > > It laters fetches the weights when computing parameters using the\n> > > > FrameContext.agc.meteringMode as index of the meteringModes_ array.\n> > > >\n> > > > When a Request gets queued to the algorithm, the\n> > > > FrameContext.agc.meteringMode index is populated from the algorithm's\n> > > > active state, and optionally updated if the application has specified\n> > > > a different metering mode.\n> > > >\n> > > > In case of Request underrun however, a non-initialized FrameContext\n> > > > gets provided to the algorithm, causing an (unhandled) exception when\n> > > > accessing the meteringModes_ array.\n> > > >\n> > > > Fix this by intializing the AGC metering mode to a supported value\n> > > > coming from the ActiveState in IPAFrameContext::init().\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++\n> > > > src/ipa/rkisp1/ipa_context.cpp | 5 +++++\n> > > > 2 files changed, 7 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index 17d074d9c03e..dd7e9468741e 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > > \t\tstatic_cast(constraintModes().begin()->first);\n> > > > \tcontext.activeState.agc.exposureMode =\n> > > > \t\tstatic_cast(exposureModeHelpers().begin()->first);\n> > > > +\n> > > > +\t/* Use the metering matrix mode by default. */\n> > > > \tcontext.activeState.agc.meteringMode =\n> > > > \t\tstatic_cast(meteringModes_.begin()->first);\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > index 2dad42b3154f..e88609137345 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum,\n> > > > \t\t\t const ActiveState &activeState)\n> > > > {\n> > > > \tFrameContext::init(frameNum, activeState);\n> > > > +\n> > > > +\tconst IPAActiveState *rkisp1ActiveState =\n> > > > +\t\treinterpret_cast(&activeState);\n> > > > +\n> > > > +\tagc.meteringMode = rkisp1ActiveState->agc.meteringMode;\n> > >\n> > > I don't particularly like that the IPAFrameContext needs that knowledge\n> > > which should be internal to the agc algorithm. We could add a\n> >\n> > You know, my feeling was the opposite. Algorithms should make less\n> > assumptions about their internals implementation details but rely on a\n> > more globally shared state, kept in the FrameContext and the\n> > ActiveState. This would make easy to \"swap\" algoroithms.\n> >\n> > After all, this already shows the AGC algo depends on a precise\n> > ordering of operations which if not respected results in a run-time\n> > exception.\n> >\n> > > initContext() function to the algorithm base class. But then we need to\n> > > loop over all algorithms here, which doesn't feel efficient for a single\n> > > case. But on the other hand if we assume that request underruns become\n> >\n> > I considered that but it seemed rather non-efficient.\n>\n> Agreed, but I'm not sure if we can completely avoid it. Maybe with some\n> more restructuring...\n>\n> >\n> > > more common, then the other algorithms need to be able to continue with\n> > > current data without a prior queueRequest() call. While writing this,\n> >\n> > Isn't the \"current data\" exactly the active state ?\n>\n> The active state is the current measuered/stats data. But the current manual\n> (user supplied) data is stored in the frame context.\n>\n\nAs the name suggests, per-frame settings are indeed stored in the\nframe context, but the active state keeps track of the algorithm's\nstate. In example, the AGC algo for RkISP1 stores in the active state\nthe auto/manual mode and the associated exposure time and gain.\n\nAs I see it, and might be my partial understanding of algos, the\nFrameContext can be initialized using the active state values, then:\n\n- when alloc()-ed during a queueRequest values coming from the application\n ControlList will override both the FrameContext and the ActiveState as\n it happens today\n\n- when get() without an alloc (Request underrun) the values it has\n been initialized to will continue to be in use and the algorithms\n can keep operating without a user Request\n\nWhat I'm not sure about is, in case of alloc(), if we need the\ninitialization or not, as the newly alloc()-ed FrameContext will be\npassed through Algo::queueRequest(), so values copied from the\nActiveState will be overwritten anyway.\n\n> >\n> > > yes I think we should forward that to every algorithm. Otherwise we\n> > > might end up with strange effects because the algorithms use the data\n> > > from the frame context that was reused without proper initialization.\n> >\n> > I might be missing how a FrameContext could be \"reused\" and \"without\n> > proper initialization\". I understand a FrameContext might not\n> > initialized, or not correctly re-initalized but if that happens is an\n> > implementation issue, isn't it ?\n>\n> Let's see if I can get this straight. For reference I take the awb algo.\n> Let's assume the following gets queued:\n>\n> Frame 1: awb.autoEnable = false, gain.blue = 4\n> Frame 2: awb.autoEnable = true\n> ... no more requests get queued ...\n>\n> Let's further assume the FCQueue has a length of 2. Now no more requests\n> get queued. But process() still get's called because the internal\n> machinery is now ticked by the sensor producing images. That results in\n> a cyclic reuse of the two frame comtexts. Now the blue gain gets\n\nThat shouldn't happen. What have I missed ?\n\nIn my understanding, following your example, we have\n\nFCQ:\n frame#0 frame#1\n +--------+--------+\n |auto = f|auto = t|\n |blue = 4| |\n +--------+--------+\n\nThey both get alloc()-ed and passed through Awb::queueRequest() in\nIPA::queueRequest(0) and IPA::queueRequest(1)\n\nThe call to Awb::queueRequest(1) will\n\n void Awb::queueRequest(IPAContext &context,\n [[maybe_unused]] const uint32_t frame,\n IPAFrameContext &frameContext,\n const ControlList &controls)\n {\n auto &awb = context.activeState.awb;\n\n const auto &awbEnable = controls.get(controls::AwbEnable);\n if (awbEnable && *awbEnable != awb.autoEnabled) {\n awb.autoEnabled = *awbEnable;\n\n LOG(RkISP1Awb, Debug)\n << (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n }\n\n ...\n\n }\n\nSo now ActiveContext contains\n\n awb.autoEnabled = true\n\nNow a new frame gets produced, and no Request has been queued, so we get(2)\nwithout a preceding alloc(2).\n\nWe call get(2) and hence we target FCQ[0] which has already been alloc()\nto store frame#0 settings\n\n\tFrameContext &get(uint32_t frame, const ActiveState &activeState)\n\t{\n\t\tFrameContext &frameContext = contexts_[frame % contexts_.size()];\n\n\t\tif (frame < frameContext.frame)\n /*\n * We don't get here, frame = 2,\n * frameContext.frame = 0\n */\n\n\n\t\tif (frame == 0 && !frameContext.initialised)\n /* We don't get here, frame = 2 */\n\n\t\tif (frame == frameContext.frame)\n /*\n * We don't get here, frame = 2,\n * frameContext.frame = 0\n */\n\n /*\n * So we get here, where we WARN and re-initialize the\n * frame context\n */\n\t\tLOG(FCQueue, Warning)\n\t\t\t<< \"Obtained an uninitialised FrameContext for \" << frame;\n\n\t\tframeContext.init(frame, activeState);\n\n\t\treturn frameContext;\n\t}\n\nThe frameContext is re-initialized with the current activeState values\nso with autoEnabled=true and passed to process().\n\nNow, how do we recover from a Request underrun is what we have to\ndesign, but assuming no futher Request gets queued, I don't see the\nalgo oscillating between auto and manual.gain.blue = 4.\n\nWhat am I missing ?\n\n> constantly toggled between 4 and the automatic value. As the stats\n> calculation depends on the gains there will be unexpected effects.\n>\n> I expect there are more cases like this and I'm not sure if we should\n> handle them inside FrameContext::init()\n>\n> Maybe I'm missing something obvious...\n\nMaybe I am.\n\nThe above is my interpretation of how thing should work. If we agree\nbut you see a different behaviour, then we might have a bug somewhere\n(which I'm not seeing).\n\n>\n> Cheers,\n> Stefan\n\nThanks!\n\n>\n> >\n> > >\n> > > I hope it is somehow understandable.\n> >\n> > Thank you!\n> > j\n> >\n> > >\n> > > Cheers,\n> > > Stefan\n> > >\n> > > > }\n> > > >\n> > > > /**\n> > > > --\n> > > > 2.47.0\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 CCC0CC31E9\n\tfor ;\n\tTue, 29 Oct 2024 07:49:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B28AF6539E;\n\tTue, 29 Oct 2024 08:49:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFA216035E\n\tfor ;\n\tTue, 29 Oct 2024 08:49:32 +0100 (CET)","from ideasonboard.com (mob-5-90-50-182.net.vodafone.it\n\t[5.90.50.182])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A569AF;\n\tTue, 29 Oct 2024 08:49:29 +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=\"ZFPBGyGP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730188170;\n\tbh=oQxVzoFA09MvlVUj513gTPq474nCWAT22KmLMmg6u+M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZFPBGyGPRGfs4UGlOjieJJUMM9h0EVrgM03WsPzjaESgt2NcYWoA+GHnx/7Gbcx83\n\tY6K3RKFnzgvqU36K49djR55sLfwoZUfWMNpFyDRR3TVT+AlhR2I8Bo19n5D/LAtkMq\n\tRDSnl9KnicJqC+6P+0PictdVyG+Ax3TTGyjB7yic=","Date":"Tue, 29 Oct 2024 08:49:26 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","Message-ID":"<5tnul6d3k2gggvrk52cepoczkhorpawzz2tzmhxnljf4lhqx7k@irem5a4u6yie>","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-5-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t<7e2qocsrys7jszmk6vibmbfzlff2coskcmghtx4wyqv7odmnue@h7g3hlgse5e2>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<7e2qocsrys7jszmk6vibmbfzlff2coskcmghtx4wyqv7odmnue@h7g3hlgse5e2>","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":31951,"web_url":"https://patchwork.libcamera.org/comment/31951/","msgid":"","date":"2024-10-29T08:55:18","subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Oct 29, 2024 at 08:49:26AM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Mon, Oct 28, 2024 at 07:12:57PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, Oct 28, 2024 at 06:14:42PM +0100, Jacopo Mondi wrote:\n> > > Hi Stefan\n> > >\n> > > On Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote:\n> > > > > The RkISP1 AGC algorithms assumes the metering mode to be\n> > > > > \"MeteringMatrix\" and pre-fill an array of weights associated\n> > > > > with it stored in meteringModes_[MeteringMatrix] when intializing\n> > > > > the algorithm in parseMeteringModes().\n> > > > >\n> > > > > It laters fetches the weights when computing parameters using the\n> > > > > FrameContext.agc.meteringMode as index of the meteringModes_ array.\n> > > > >\n> > > > > When a Request gets queued to the algorithm, the\n> > > > > FrameContext.agc.meteringMode index is populated from the algorithm's\n> > > > > active state, and optionally updated if the application has specified\n> > > > > a different metering mode.\n> > > > >\n> > > > > In case of Request underrun however, a non-initialized FrameContext\n> > > > > gets provided to the algorithm, causing an (unhandled) exception when\n> > > > > accessing the meteringModes_ array.\n> > > > >\n> > > > > Fix this by intializing the AGC metering mode to a supported value\n> > > > > coming from the ActiveState in IPAFrameContext::init().\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 ++\n> > > > > src/ipa/rkisp1/ipa_context.cpp | 5 +++++\n> > > > > 2 files changed, 7 insertions(+)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > index 17d074d9c03e..dd7e9468741e 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > > > \t\tstatic_cast(constraintModes().begin()->first);\n> > > > > \tcontext.activeState.agc.exposureMode =\n> > > > > \t\tstatic_cast(exposureModeHelpers().begin()->first);\n> > > > > +\n> > > > > +\t/* Use the metering matrix mode by default. */\n> > > > > \tcontext.activeState.agc.meteringMode =\n> > > > > \t\tstatic_cast(meteringModes_.begin()->first);\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > index 2dad42b3154f..e88609137345 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum,\n> > > > > \t\t\t const ActiveState &activeState)\n> > > > > {\n> > > > > \tFrameContext::init(frameNum, activeState);\n> > > > > +\n> > > > > +\tconst IPAActiveState *rkisp1ActiveState =\n> > > > > +\t\treinterpret_cast(&activeState);\n> > > > > +\n> > > > > +\tagc.meteringMode = rkisp1ActiveState->agc.meteringMode;\n> > > >\n> > > > I don't particularly like that the IPAFrameContext needs that knowledge\n> > > > which should be internal to the agc algorithm. We could add a\n> > >\n> > > You know, my feeling was the opposite. Algorithms should make less\n> > > assumptions about their internals implementation details but rely on a\n> > > more globally shared state, kept in the FrameContext and the\n> > > ActiveState. This would make easy to \"swap\" algoroithms.\n> > >\n> > > After all, this already shows the AGC algo depends on a precise\n> > > ordering of operations which if not respected results in a run-time\n> > > exception.\n> > >\n> > > > initContext() function to the algorithm base class. But then we need to\n> > > > loop over all algorithms here, which doesn't feel efficient for a single\n> > > > case. But on the other hand if we assume that request underruns become\n> > >\n> > > I considered that but it seemed rather non-efficient.\n> >\n> > Agreed, but I'm not sure if we can completely avoid it. Maybe with some\n> > more restructuring...\n> >\n> > >\n> > > > more common, then the other algorithms need to be able to continue with\n> > > > current data without a prior queueRequest() call. While writing this,\n> > >\n> > > Isn't the \"current data\" exactly the active state ?\n> >\n> > The active state is the current measuered/stats data. But the current manual\n> > (user supplied) data is stored in the frame context.\n> >\n> \n> As the name suggests, per-frame settings are indeed stored in the\n> frame context, but the active state keeps track of the algorithm's\n> state. In example, the AGC algo for RkISP1 stores in the active state\n> the auto/manual mode and the associated exposure time and gain.\n> \n> As I see it, and might be my partial understanding of algos, the\n> FrameContext can be initialized using the active state values, then:\n> \n> - when alloc()-ed during a queueRequest values coming from the application\n> ControlList will override both the FrameContext and the ActiveState as\n> it happens today\n> \n> - when get() without an alloc (Request underrun) the values it has\n> been initialized to will continue to be in use and the algorithms\n> can keep operating without a user Request\n> \n> What I'm not sure about is, in case of alloc(), if we need the\n> initialization or not, as the newly alloc()-ed FrameContext will be\n> passed through Algo::queueRequest(), so values copied from the\n> ActiveState will be overwritten anyway.\n\nI wrote the following paragraph last. Maybe you should read the rest\nfirst :-)\n\nWhat about renaming Algorithm::queueRequest to\nAlgorithm::initFrameContext which gets called once in both code paths\n(Either after/in alloc() or after/in get() without prior alloc()) For\nthe get() without alloc we just pass an empty ControlsList.\n\n> \n> > >\n> > > > yes I think we should forward that to every algorithm. Otherwise we\n> > > > might end up with strange effects because the algorithms use the data\n> > > > from the frame context that was reused without proper initialization.\n> > >\n> > > I might be missing how a FrameContext could be \"reused\" and \"without\n> > > proper initialization\". I understand a FrameContext might not\n> > > initialized, or not correctly re-initalized but if that happens is an\n> > > implementation issue, isn't it ?\n> >\n> > Let's see if I can get this straight. For reference I take the awb algo.\n> > Let's assume the following gets queued:\n> >\n> > Frame 1: awb.autoEnable = false, gain.blue = 4\n> > Frame 2: awb.autoEnable = true\n> > ... no more requests get queued ...\n> >\n> > Let's further assume the FCQueue has a length of 2. Now no more requests\n> > get queued. But process() still get's called because the internal\n> > machinery is now ticked by the sensor producing images. That results in\n> > a cyclic reuse of the two frame comtexts. Now the blue gain gets\n> \n> That shouldn't happen. What have I missed ?\n> \n> In my understanding, following your example, we have\n> \n> FCQ:\n> frame#0 frame#1\n> +--------+--------+\n> |auto = f|auto = t|\n> |blue = 4| |\n> +--------+--------+\n> \n> They both get alloc()-ed and passed through Awb::queueRequest() in\n> IPA::queueRequest(0) and IPA::queueRequest(1)\n> \n> The call to Awb::queueRequest(1) will\n> \n> void Awb::queueRequest(IPAContext &context,\n> [[maybe_unused]] const uint32_t frame,\n> IPAFrameContext &frameContext,\n> const ControlList &controls)\n> {\n> auto &awb = context.activeState.awb;\n> \n> const auto &awbEnable = controls.get(controls::AwbEnable);\n> if (awbEnable && *awbEnable != awb.autoEnabled) {\n> awb.autoEnabled = *awbEnable;\n> \n> LOG(RkISP1Awb, Debug)\n> << (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n> }\n> \n> ...\n> \n> }\n> \n> So now ActiveContext contains\n> \n> awb.autoEnabled = true\n> \n> Now a new frame gets produced, and no Request has been queued, so we get(2)\n> without a preceding alloc(2).\n> \n> We call get(2) and hence we target FCQ[0] which has already been alloc()\n> to store frame#0 settings\n> \n> \tFrameContext &get(uint32_t frame, const ActiveState &activeState)\n> \t{\n> \t\tFrameContext &frameContext = contexts_[frame % contexts_.size()];\n> \n> \t\tif (frame < frameContext.frame)\n> /*\n> * We don't get here, frame = 2,\n> * frameContext.frame = 0\n> */\n> \n> \n> \t\tif (frame == 0 && !frameContext.initialised)\n> /* We don't get here, frame = 2 */\n> \n> \t\tif (frame == frameContext.frame)\n> /*\n> * We don't get here, frame = 2,\n> * frameContext.frame = 0\n> */\n> \n> /*\n> * So we get here, where we WARN and re-initialize the\n> * frame context\n> */\n> \t\tLOG(FCQueue, Warning)\n> \t\t\t<< \"Obtained an uninitialised FrameContext for \" << frame;\n> \n> \t\tframeContext.init(frame, activeState);\n> \n> \t\treturn frameContext;\n> \t}\n> \n> The frameContext is re-initialized with the current activeState values\n> so with autoEnabled=true and passed to process().\n\nHere is the catch (or the piece I can't see). Who does the assignment\nframeContext.awb.autoEnable = activeState.awb.autoEnable?\n\nIt has to be inside the frameContext.init() function. Isn't that the\nplace where we started off with the discussion? That we could either\nhardcode it inside IPAFrameContext::init() if the frame context has\nenough information on what to initialize from activeState or forward\nthat to the corresponding algorithm and implement a\nAlgorithm::initFrameContext(fc, activeState).\n\n> \n> Now, how do we recover from a Request underrun is what we have to\n> design, but assuming no futher Request gets queued, I don't see the\n> algo oscillating between auto and manual.gain.blue = 4.\n> \n> What am I missing ?\n\nI think we are on the same page. The only difference I see is that I\ntend to move things into the agorithms to keep all that logic in one\nplace and I believe you see the algorithms more as a pluggable formula\nwhich operate on the state that is kept (and known) in frame context and\ntherefore frame context is able to initialize it.\n\n> \n> > constantly toggled between 4 and the automatic value. As the stats\n> > calculation depends on the gains there will be unexpected effects.\n> >\n> > I expect there are more cases like this and I'm not sure if we should\n> > handle them inside FrameContext::init()\n> >\n> > Maybe I'm missing something obvious...\n> \n> Maybe I am.\n> \n> The above is my interpretation of how thing should work. If we agree\n> but you see a different behaviour, then we might have a bug somewhere\n> (which I'm not seeing).\n\nI think we agree in the overall picture. Maybe one approach to solve\nthis is to further fill the FrameContext::init() to see how much\nalgorithm specific knowledge we need there. This boils down to copying\nall assignments to frameContext from the Algorihm::queueRequest()\nfunctions into the FrameContext::init() or try to rename\nAlgorithm::queueRequest as proposed above.\n\nBest regards,\nStefan\n\n> \n> >\n> > Cheers,\n> > Stefan\n> \n> Thanks!\n> \n> >\n> > >\n> > > >\n> > > > I hope it is somehow understandable.\n> > >\n> > > Thank you!\n> > > j\n> > >\n> > > >\n> > > > Cheers,\n> > > > Stefan\n> > > >\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > --\n> > > > > 2.47.0\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 B99DBC3220\n\tfor ;\n\tTue, 29 Oct 2024 08:55:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A19ED6539E;\n\tTue, 29 Oct 2024 09:55:23 +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 D702D60366\n\tfor ;\n\tTue, 29 Oct 2024 09:55:21 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:84fb:185d:159:46bc])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D0BB4D4;\n\tTue, 29 Oct 2024 09:55:19 +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=\"qZkNhjF9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730192119;\n\tbh=B7YMqs5IIXS2pqOsSNbpEVdXEIt1MdE3adJMQOu68zE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qZkNhjF9wkGgNkCWtf6vKol3mGbeVlxvu4vKulgkOSeH+caiYum+awkQOTlpSkei5\n\t8T8ZcWQSB+xMNuBRupB71QIBjE/mzfz8vkv7TVXCtVTVmMA6Mo0L4upg/Ol2glSGxG\n\tkV/RgvFQQ/47uNKAm9z9Bur1KBEPjxI2siCYI9I0=","Date":"Tue, 29 Oct 2024 09:55:18 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH 4/4] ipa: rkisp1: Initialize\n\tFrameContext.agc.meteringMode","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-5-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t<7e2qocsrys7jszmk6vibmbfzlff2coskcmghtx4wyqv7odmnue@h7g3hlgse5e2>\n\t<5tnul6d3k2gggvrk52cepoczkhorpawzz2tzmhxnljf4lhqx7k@irem5a4u6yie>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<5tnul6d3k2gggvrk52cepoczkhorpawzz2tzmhxnljf4lhqx7k@irem5a4u6yie>","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\" "}}]