[{"id":32125,"web_url":"https://patchwork.libcamera.org/comment/32125/","msgid":"<52fqvjfldli7qxxiwb44ukid4hfrjc6bk6s2qpvs7kxzy2iwky@2tuxylav3uaq>","date":"2024-11-12T16:35:52","subject":"Re: [PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext","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 30, 2024 at 05:48:52PM +0100, 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> To handle the situation when a Request underrun occours and let\n> algorithms initialize a FrameContext to safe defaults:\n> \n> - Add an 'underrun' flag to FrameContext\n> - Add an 'initFrameContext()' function to RkISP1 algorithms\n> - If a FrameContext is get() before being allocated, pass it through\n> the algorithms' initFrameContext() to safely initialize it\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/libipa/fc_queue.cpp | 6 ++++++\n> src/ipa/libipa/fc_queue.h | 5 +++++\n> src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++\n> src/ipa/rkisp1/algorithms/agc.h | 4 ++++\n> src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++\n> src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++\n> 6 files changed, 37 insertions(+)\n> \n> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> index 0365e9197748..44f9d866ad1b 100644\n> --- a/src/ipa/libipa/fc_queue.cpp\n> +++ b/src/ipa/libipa/fc_queue.cpp\n> @@ -36,6 +36,12 @@ namespace ipa {\n> *\n> * \\var FrameContext::frame\n> * \\brief The frame number\n> + *\n> + * \\var FrameContext::underrun\n> + * \\brief Boolean flag that reports if the FrameContext has been accessed with\n> + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set\n> + * to True then the frame context needs to be initialized by algorithms to safe\n> + * defaults as it won't be initialized with any non-user provided control.\n> */\n> \n> /**\n> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> index a1d136521107..d70ca9601bd7 100644\n> --- a/src/ipa/libipa/fc_queue.h\n> +++ b/src/ipa/libipa/fc_queue.h\n> @@ -22,6 +22,9 @@ template\n> class FCQueue;\n> \n> struct FrameContext {\n> +public:\n> +\tbool underrun = false;\n> +\n> private:\n> \ttemplate friend class FCQueue;\n> \tuint32_t frame;\n> @@ -97,6 +100,7 @@ public:\n> \t\t\t * is called before alloc() by the IPA for frame#0.\n> \t\t\t */\n> \t\t\tinit(frameContext, frame);\n> +\t\t\tframeContext.underrun = true;\n\nI can't find the place where the flag is reset to false (In case there\nare temporary underruns).\n\n> \n> \t\t\treturn frameContext;\n> \t\t}\n> @@ -117,6 +121,7 @@ public:\n> \t\t\t<< \"Obtained an uninitialised FrameContext for \" << frame;\n> \n> \t\tinit(frameContext, frame);\n> +\t\tframeContext.underrun = true;\n> \n> \t\treturn frameContext;\n> \t}\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 301b7ec26508..4122f665b3ee 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \treturn 0;\n> }\n> \n> +void Agc::initFrameContext(IPAContext &context,\n> +\t\t\t IPAFrameContext &frameContext)\n> +{\n> +\tauto &agc = context.activeState.agc;\n> +\n> +\tframeContext.agc.meteringMode = agc.meteringMode;\n> +}\n> +\n\nI'm unsure if we really need the separate function.\n\nFrom an algorithm point of view, I can't see the difference between\ncalling\n\ninitFrameContext(ipaContext, frameContext)\n\nand\n\nqueueRequest(ipaContext, frameContext.frame, frameContext, {})\n\nCouldn't we just rename queueRequest to initFrameContext, so that we\ndon't have to implement two separate functions?\n\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> */\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index aa86f2c5bc21..c1adf91bbc4e 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -30,6 +30,10 @@ public:\n> \n> \tint init(IPAContext &context, const YamlObject &tuningData) override;\n> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> +\n> +\tvoid initFrameContext(IPAContext &context,\n> +\t\t\t IPAFrameContext &frameContext) override;\n> +\n> \tvoid queueRequest(IPAContext &context,\n> \t\t\t const uint32_t frame,\n> \t\t\t IPAFrameContext &frameContext,\n> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> index 715cfcd8298b..82603b7b372d 100644\n> --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> @@ -23,6 +23,11 @@ public:\n> \t{\n> \t}\n> \n> +\tvirtual void initFrameContext([[maybe_unused]] IPAContext &context,\n> +\t\t\t\t [[maybe_unused]] IPAFrameContext &frameContext)\n> +\t{\n> +\t}\n> +\n> \tbool disabled_;\n> \tbool supportsRaw_;\n> };\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 9e161cabdea4..b743de9ff6af 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> {\n> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \n> +\tif (frameContext.underrun) {\n> +\t\tfor (auto const &a : algorithms()) {\n> +\t\t\tAlgorithm *algo = static_cast(a.get());\n> +\t\t\tif (algo->disabled_)\n> +\t\t\t\tcontinue;\n> +\t\t\talgo->initFrameContext(context_, frameContext);\n> +\t\t}\n\nMaybe that would be the place for frameContext.underrun = false?\n\nBest regards,\nStefan\n\n> +\t}\n> +\n> \t/*\n> \t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> \t * provided.\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 2246DBE173\n\tfor ;\n\tTue, 12 Nov 2024 16:35:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BFB4657F4;\n\tTue, 12 Nov 2024 17:35:58 +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 52CE7657B0\n\tfor ;\n\tTue, 12 Nov 2024 17:35:56 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c176:4769:9bb6:c5be])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B3C2512;\n\tTue, 12 Nov 2024 17:35:43 +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=\"RjGhIywm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731429343;\n\tbh=rvvNAcQ9UuqotS1d2KTKgTE+McxIHgGDz3ojokiEqag=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RjGhIywm31VQ+7NwdZxm/YBhuscZIvn8YWTHW97uZMJod5ksddJx+zkXu3bQUyt3o\n\tsry6pnNUKFaAsYm1U3KitzokTItNVpyf9ztYpe6IPsuU8VJAcBMkuC0j6uzISQfJwO\n\tpEBSW4+YaOcmc8vUdx5iSsMkZLwcAdZusKy3qpvU=","Date":"Tue, 12 Nov 2024 17:35:52 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDan Scally ","Subject":"Re: [PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext","Message-ID":"<52fqvjfldli7qxxiwb44ukid4hfrjc6bk6s2qpvs7kxzy2iwky@2tuxylav3uaq>","References":"<20241030164853.87586-1-jacopo.mondi@ideasonboard.com>\n\t<20241030164853.87586-3-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241030164853.87586-3-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":32133,"web_url":"https://patchwork.libcamera.org/comment/32133/","msgid":"","date":"2024-11-13T07:35:24","subject":"Re: [PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n thanks for the review\n\nOn Tue, Nov 12, 2024 at 05:35:52PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Oct 30, 2024 at 05:48:52PM +0100, 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> > To handle the situation when a Request underrun occours and let\n> > algorithms initialize a FrameContext to safe defaults:\n> >\n> > - Add an 'underrun' flag to FrameContext\n> > - Add an 'initFrameContext()' function to RkISP1 algorithms\n> > - If a FrameContext is get() before being allocated, pass it through\n> > the algorithms' initFrameContext() to safely initialize it\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/libipa/fc_queue.cpp | 6 ++++++\n> > src/ipa/libipa/fc_queue.h | 5 +++++\n> > src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++\n> > src/ipa/rkisp1/algorithms/agc.h | 4 ++++\n> > src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++\n> > src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++\n> > 6 files changed, 37 insertions(+)\n> >\n> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> > index 0365e9197748..44f9d866ad1b 100644\n> > --- a/src/ipa/libipa/fc_queue.cpp\n> > +++ b/src/ipa/libipa/fc_queue.cpp\n> > @@ -36,6 +36,12 @@ namespace ipa {\n> > *\n> > * \\var FrameContext::frame\n> > * \\brief The frame number\n> > + *\n> > + * \\var FrameContext::underrun\n> > + * \\brief Boolean flag that reports if the FrameContext has been accessed with\n> > + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set\n> > + * to True then the frame context needs to be initialized by algorithms to safe\n> > + * defaults as it won't be initialized with any non-user provided control.\n> > */\n> >\n> > /**\n> > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > index a1d136521107..d70ca9601bd7 100644\n> > --- a/src/ipa/libipa/fc_queue.h\n> > +++ b/src/ipa/libipa/fc_queue.h\n> > @@ -22,6 +22,9 @@ template\n> > class FCQueue;\n> >\n> > struct FrameContext {\n> > +public:\n> > +\tbool underrun = false;\n> > +\n> > private:\n> > \ttemplate friend class FCQueue;\n> > \tuint32_t frame;\n> > @@ -97,6 +100,7 @@ public:\n> > \t\t\t * is called before alloc() by the IPA for frame#0.\n> > \t\t\t */\n> > \t\t\tinit(frameContext, frame);\n> > +\t\t\tframeContext.underrun = true;\n>\n> I can't find the place where the flag is reset to false (In case there\n> are temporary underruns).\n\nIt happens in init()\n\n\tvoid init(FrameContext &frameContext, const uint32_t frame)\n\t{\n\t\tframeContext = {};\n\nand 'underrun' gets set to true only in the case get(x) is called\nbefore alloc(x)\n\nI should probably reset it in clear() too though\n\n>\n> >\n> > \t\t\treturn frameContext;\n> > \t\t}\n> > @@ -117,6 +121,7 @@ public:\n> > \t\t\t<< \"Obtained an uninitialised FrameContext for \" << frame;\n> >\n> > \t\tinit(frameContext, frame);\n> > +\t\tframeContext.underrun = true;\n> >\n> > \t\treturn frameContext;\n> > \t}\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 301b7ec26508..4122f665b3ee 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \treturn 0;\n> > }\n> >\n> > +void Agc::initFrameContext(IPAContext &context,\n> > +\t\t\t IPAFrameContext &frameContext)\n> > +{\n> > +\tauto &agc = context.activeState.agc;\n> > +\n> > +\tframeContext.agc.meteringMode = agc.meteringMode;\n> > +}\n> > +\n>\n> I'm unsure if we really need the separate function.\n>\n> From an algorithm point of view, I can't see the difference between\n> calling\n>\n> initFrameContext(ipaContext, frameContext)\n>\n> and\n>\n> queueRequest(ipaContext, frameContext.frame, frameContext, {})\n\nI like this way of thinking about it\n\n>\n> Couldn't we just rename queueRequest to initFrameContext, so that we\n> don't have to implement two separate functions?\n>\n\nI like this ideas as well\n\nLet's bikeshed the names a bit\n\nwe have\n\ninit()\nconfigure()\nqueueRequest()\nprepare()\nprocess()\n\nwith the last three operations being called repetidly in a streaming\nsession.\n\nI was never in love with the \"prepare()\" and \"process()\" names, and I\nwould rather use \"prepare()\" for queueRequest(), but that's what we\nhave right now.\n\nThe idea is that queueRequest actually initializes a frame context,\noptionally with user provided controls. So I think that using\ninitFrameContext() is actually a good suggestion.\n\n\n\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > */\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index aa86f2c5bc21..c1adf91bbc4e 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -30,6 +30,10 @@ public:\n> >\n> > \tint init(IPAContext &context, const YamlObject &tuningData) override;\n> > \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > +\n> > +\tvoid initFrameContext(IPAContext &context,\n> > +\t\t\t IPAFrameContext &frameContext) override;\n> > +\n> > \tvoid queueRequest(IPAContext &context,\n> > \t\t\t const uint32_t frame,\n> > \t\t\t IPAFrameContext &frameContext,\n> > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> > index 715cfcd8298b..82603b7b372d 100644\n> > --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> > +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> > @@ -23,6 +23,11 @@ public:\n> > \t{\n> > \t}\n> >\n> > +\tvirtual void initFrameContext([[maybe_unused]] IPAContext &context,\n> > +\t\t\t\t [[maybe_unused]] IPAFrameContext &frameContext)\n> > +\t{\n> > +\t}\n> > +\n> > \tbool disabled_;\n> > \tbool supportsRaw_;\n> > };\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 9e161cabdea4..b743de9ff6af 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > {\n> > \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >\n> > +\tif (frameContext.underrun) {\n> > +\t\tfor (auto const &a : algorithms()) {\n> > +\t\t\tAlgorithm *algo = static_cast(a.get());\n> > +\t\t\tif (algo->disabled_)\n> > +\t\t\t\tcontinue;\n> > +\t\t\talgo->initFrameContext(context_, frameContext);\n> > +\t\t}\n>\n> Maybe that would be the place for frameContext.underrun = false?\n\nI don't think the FrameContext status flags should be handled outside\nof the FCQ implementation, otherwise each algo implementation would\nhave to do that correctly by its own.\n\nThanks\n j\n\n>\n> Best regards,\n> Stefan\n>\n> > +\t}\n> > +\n> > \t/*\n> > \t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> > \t * provided.\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 C578DC324C\n\tfor ;\n\tWed, 13 Nov 2024 07:35:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 759B765803;\n\tWed, 13 Nov 2024 08:35:30 +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 46EB965800\n\tfor ;\n\tWed, 13 Nov 2024 08:35:28 +0100 (CET)","from ideasonboard.com (net-93-150-226-204.cust.vodafonedsl.it\n\t[93.150.226.204])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 16473594;\n\tWed, 13 Nov 2024 08:35:15 +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=\"lbwWEjUb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731483315;\n\tbh=/2U5sFjDsBNDQEeqdKC5gpjGp7B0c3n7WtA1Pk0T1Eo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lbwWEjUbc+KtHIRL+Tvsoe+m3wO+9llARa5S+r4QSLBgH9BZLGqE8j3JsyHINujlb\n\tAyqgJsO20myyNA2bRSHCs0rS/YEzeZas6i363OUhfiWbOmzrsw7+nhOCsE8Bg0uvtk\n\tkqpYgq5Pw3WzfIQFJl3AqYew3AwAC5tFfWC/AR+g=","Date":"Wed, 13 Nov 2024 08:35:24 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tDan Scally ","Subject":"Re: [PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext","Message-ID":"","References":"<20241030164853.87586-1-jacopo.mondi@ideasonboard.com>\n\t<20241030164853.87586-3-jacopo.mondi@ideasonboard.com>\n\t<52fqvjfldli7qxxiwb44ukid4hfrjc6bk6s2qpvs7kxzy2iwky@2tuxylav3uaq>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<52fqvjfldli7qxxiwb44ukid4hfrjc6bk6s2qpvs7kxzy2iwky@2tuxylav3uaq>","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":32134,"web_url":"https://patchwork.libcamera.org/comment/32134/","msgid":"","date":"2024-11-13T08:43:55","subject":"Re: [PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Nov 13, 2024 at 08:35:24AM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> thanks for the review\n> \n> On Tue, Nov 12, 2024 at 05:35:52PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Oct 30, 2024 at 05:48:52PM +0100, 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> > > To handle the situation when a Request underrun occours and let\n> > > algorithms initialize a FrameContext to safe defaults:\n> > >\n> > > - Add an 'underrun' flag to FrameContext\n> > > - Add an 'initFrameContext()' function to RkISP1 algorithms\n> > > - If a FrameContext is get() before being allocated, pass it through\n> > > the algorithms' initFrameContext() to safely initialize it\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/ipa/libipa/fc_queue.cpp | 6 ++++++\n> > > src/ipa/libipa/fc_queue.h | 5 +++++\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++\n> > > src/ipa/rkisp1/algorithms/agc.h | 4 ++++\n> > > src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++\n> > > src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++\n> > > 6 files changed, 37 insertions(+)\n> > >\n> > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> > > index 0365e9197748..44f9d866ad1b 100644\n> > > --- a/src/ipa/libipa/fc_queue.cpp\n> > > +++ b/src/ipa/libipa/fc_queue.cpp\n> > > @@ -36,6 +36,12 @@ namespace ipa {\n> > > *\n> > > * \\var FrameContext::frame\n> > > * \\brief The frame number\n> > > + *\n> > > + * \\var FrameContext::underrun\n> > > + * \\brief Boolean flag that reports if the FrameContext has been accessed with\n> > > + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set\n> > > + * to True then the frame context needs to be initialized by algorithms to safe\n> > > + * defaults as it won't be initialized with any non-user provided control.\n> > > */\n> > >\n> > > /**\n> > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > > index a1d136521107..d70ca9601bd7 100644\n> > > --- a/src/ipa/libipa/fc_queue.h\n> > > +++ b/src/ipa/libipa/fc_queue.h\n> > > @@ -22,6 +22,9 @@ template\n> > > class FCQueue;\n> > >\n> > > struct FrameContext {\n> > > +public:\n> > > +\tbool underrun = false;\n> > > +\n> > > private:\n> > > \ttemplate friend class FCQueue;\n> > > \tuint32_t frame;\n> > > @@ -97,6 +100,7 @@ public:\n> > > \t\t\t * is called before alloc() by the IPA for frame#0.\n> > > \t\t\t */\n> > > \t\t\tinit(frameContext, frame);\n> > > +\t\t\tframeContext.underrun = true;\n> >\n> > I can't find the place where the flag is reset to false (In case there\n> > are temporary underruns).\n> \n> It happens in init()\n> \n> \tvoid init(FrameContext &frameContext, const uint32_t frame)\n> \t{\n> \t\tframeContext = {};\n> \n> and 'underrun' gets set to true only in the case get(x) is called\n> before alloc(x)\n> \n> I should probably reset it in clear() too though\n> \n\nOh right, sorry for the noise.\n\n> >\n> > >\n> > > \t\t\treturn frameContext;\n> > > \t\t}\n> > > @@ -117,6 +121,7 @@ public:\n> > > \t\t\t<< \"Obtained an uninitialised FrameContext for \" << frame;\n> > >\n> > > \t\tinit(frameContext, frame);\n> > > +\t\tframeContext.underrun = true;\n> > >\n> > > \t\treturn frameContext;\n> > > \t}\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 301b7ec26508..4122f665b3ee 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > \treturn 0;\n> > > }\n> > >\n> > > +void Agc::initFrameContext(IPAContext &context,\n> > > +\t\t\t IPAFrameContext &frameContext)\n> > > +{\n> > > +\tauto &agc = context.activeState.agc;\n> > > +\n> > > +\tframeContext.agc.meteringMode = agc.meteringMode;\n> > > +}\n> > > +\n> >\n> > I'm unsure if we really need the separate function.\n> >\n> > From an algorithm point of view, I can't see the difference between\n> > calling\n> >\n> > initFrameContext(ipaContext, frameContext)\n> >\n> > and\n> >\n> > queueRequest(ipaContext, frameContext.frame, frameContext, {})\n> \n> I like this way of thinking about it\n> \n> >\n> > Couldn't we just rename queueRequest to initFrameContext, so that we\n> > don't have to implement two separate functions?\n> >\n> \n> I like this ideas as well\n> \n> Let's bikeshed the names a bit\n> \n> we have\n> \n> init()\n> configure()\n> queueRequest()\n> prepare()\n> process()\n> \n> with the last three operations being called repetidly in a streaming\n> session.\n> \n> I was never in love with the \"prepare()\" and \"process()\" names, and I\n> would rather use \"prepare()\" for queueRequest(), but that's what we\n> have right now.\n> \n> The idea is that queueRequest actually initializes a frame context,\n> optionally with user provided controls. So I think that using\n> initFrameContext() is actually a good suggestion.\n> \n> \n\nGlad you like it. The only thing we should then clarify is the\ndefinition of frame. In the docs it says \"The frame number is the\nRequest sequence number and identifies the desired corresponding frame\nto target for the controls to take effect.\" I guess in the new pfc\nscheme that will become the internal/sensor frame number, and\ntranslation to request sequence happens outside. From an algorithm point\nof view that seems sensible.\n\nBest regards,\nStefan\n\n> \n> > > /**\n> > > * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > > */\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > index aa86f2c5bc21..c1adf91bbc4e 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > @@ -30,6 +30,10 @@ public:\n> > >\n> > > \tint init(IPAContext &context, const YamlObject &tuningData) override;\n> > > \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > > +\n> > > +\tvoid initFrameContext(IPAContext &context,\n> > > +\t\t\t IPAFrameContext &frameContext) override;\n> > > +\n> > > \tvoid queueRequest(IPAContext &context,\n> > > \t\t\t const uint32_t frame,\n> > > \t\t\t IPAFrameContext &frameContext,\n> > > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> > > index 715cfcd8298b..82603b7b372d 100644\n> > > --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> > > +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> > > @@ -23,6 +23,11 @@ public:\n> > > \t{\n> > > \t}\n> > >\n> > > +\tvirtual void initFrameContext([[maybe_unused]] IPAContext &context,\n> > > +\t\t\t\t [[maybe_unused]] IPAFrameContext &frameContext)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > \tbool disabled_;\n> > > \tbool supportsRaw_;\n> > > };\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 9e161cabdea4..b743de9ff6af 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > {\n> > > \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > >\n> > > +\tif (frameContext.underrun) {\n> > > +\t\tfor (auto const &a : algorithms()) {\n> > > +\t\t\tAlgorithm *algo = static_cast(a.get());\n> > > +\t\t\tif (algo->disabled_)\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\talgo->initFrameContext(context_, frameContext);\n> > > +\t\t}\n> >\n> > Maybe that would be the place for frameContext.underrun = false?\n> \n> I don't think the FrameContext status flags should be handled outside\n> of the FCQ implementation, otherwise each algo implementation would\n> have to do that correctly by its own.\n> \n> Thanks\n> j\n> \n> >\n> > Best regards,\n> > Stefan\n> >\n> > > +\t}\n> > > +\n> > > \t/*\n> > > \t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> > > \t * provided.\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 9A82DBE173\n\tfor ;\n\tWed, 13 Nov 2024 08:44:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B7FB65802;\n\tWed, 13 Nov 2024 09:44:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8156F605B5\n\tfor ;\n\tWed, 13 Nov 2024 09:43:58 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:f0b7:dedf:7ae5:f332])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 320AE752;\n\tWed, 13 Nov 2024 09:43:45 +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=\"ShZIueZs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731487425;\n\tbh=gMOZlyKxRaWiTvPTUbzviKLkSUfSB/A1T5elXiWIzVo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ShZIueZsESiew+36Ldm43ZdBSBRwBZEAMN1rvL2a2aVBpyaDDwR73vLcJNJ+yXYmS\n\tD+v5P4EjRZKyrFmRC/krkBOX9CKCarFU0OK5N79GOvbkJ/R8MU2mpiWCTpZxnsfbkQ\n\tMFnFWdhLFzpg1Jni58VEFX+KfnnQQc8VdG6QRJyE=","Date":"Wed, 13 Nov 2024 09:43:55 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDan Scally ","Subject":"Re: [PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext","Message-ID":"","References":"<20241030164853.87586-1-jacopo.mondi@ideasonboard.com>\n\t<20241030164853.87586-3-jacopo.mondi@ideasonboard.com>\n\t<52fqvjfldli7qxxiwb44ukid4hfrjc6bk6s2qpvs7kxzy2iwky@2tuxylav3uaq>\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":32135,"web_url":"https://patchwork.libcamera.org/comment/32135/","msgid":"","date":"2024-11-13T08:57:02","subject":"Re: [PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Wed, Nov 13, 2024 at 09:43:55AM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> On Wed, Nov 13, 2024 at 08:35:24AM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> > thanks for the review\n> >\n> > On Tue, Nov 12, 2024 at 05:35:52PM +0100, Stefan Klug wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Wed, Oct 30, 2024 at 05:48:52PM +0100, 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> > > > To handle the situation when a Request underrun occours and let\n> > > > algorithms initialize a FrameContext to safe defaults:\n> > > >\n> > > > - Add an 'underrun' flag to FrameContext\n> > > > - Add an 'initFrameContext()' function to RkISP1 algorithms\n> > > > - If a FrameContext is get() before being allocated, pass it through\n> > > > the algorithms' initFrameContext() to safely initialize it\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/ipa/libipa/fc_queue.cpp | 6 ++++++\n> > > > src/ipa/libipa/fc_queue.h | 5 +++++\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++\n> > > > src/ipa/rkisp1/algorithms/agc.h | 4 ++++\n> > > > src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++\n> > > > src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++\n> > > > 6 files changed, 37 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> > > > index 0365e9197748..44f9d866ad1b 100644\n> > > > --- a/src/ipa/libipa/fc_queue.cpp\n> > > > +++ b/src/ipa/libipa/fc_queue.cpp\n> > > > @@ -36,6 +36,12 @@ namespace ipa {\n> > > > *\n> > > > * \\var FrameContext::frame\n> > > > * \\brief The frame number\n> > > > + *\n> > > > + * \\var FrameContext::underrun\n> > > > + * \\brief Boolean flag that reports if the FrameContext has been accessed with\n> > > > + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set\n> > > > + * to True then the frame context needs to be initialized by algorithms to safe\n> > > > + * defaults as it won't be initialized with any non-user provided control.\n> > > > */\n> > > >\n> > > > /**\n> > > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > > > index a1d136521107..d70ca9601bd7 100644\n> > > > --- a/src/ipa/libipa/fc_queue.h\n> > > > +++ b/src/ipa/libipa/fc_queue.h\n> > > > @@ -22,6 +22,9 @@ template\n> > > > class FCQueue;\n> > > >\n> > > > struct FrameContext {\n> > > > +public:\n> > > > +\tbool underrun = false;\n> > > > +\n> > > > private:\n> > > > \ttemplate friend class FCQueue;\n> > > > \tuint32_t frame;\n> > > > @@ -97,6 +100,7 @@ public:\n> > > > \t\t\t * is called before alloc() by the IPA for frame#0.\n> > > > \t\t\t */\n> > > > \t\t\tinit(frameContext, frame);\n> > > > +\t\t\tframeContext.underrun = true;\n> > >\n> > > I can't find the place where the flag is reset to false (In case there\n> > > are temporary underruns).\n> >\n> > It happens in init()\n> >\n> > \tvoid init(FrameContext &frameContext, const uint32_t frame)\n> > \t{\n> > \t\tframeContext = {};\n> >\n> > and 'underrun' gets set to true only in the case get(x) is called\n> > before alloc(x)\n> >\n> > I should probably reset it in clear() too though\n> >\n>\n> Oh right, sorry for the noise.\n>\n> > >\n> > > >\n> > > > \t\t\treturn frameContext;\n> > > > \t\t}\n> > > > @@ -117,6 +121,7 @@ public:\n> > > > \t\t\t<< \"Obtained an uninitialised FrameContext for \" << frame;\n> > > >\n> > > > \t\tinit(frameContext, frame);\n> > > > +\t\tframeContext.underrun = true;\n> > > >\n> > > > \t\treturn frameContext;\n> > > > \t}\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index 301b7ec26508..4122f665b3ee 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > +void Agc::initFrameContext(IPAContext &context,\n> > > > +\t\t\t IPAFrameContext &frameContext)\n> > > > +{\n> > > > +\tauto &agc = context.activeState.agc;\n> > > > +\n> > > > +\tframeContext.agc.meteringMode = agc.meteringMode;\n> > > > +}\n> > > > +\n> > >\n> > > I'm unsure if we really need the separate function.\n> > >\n> > > From an algorithm point of view, I can't see the difference between\n> > > calling\n> > >\n> > > initFrameContext(ipaContext, frameContext)\n> > >\n> > > and\n> > >\n> > > queueRequest(ipaContext, frameContext.frame, frameContext, {})\n> >\n> > I like this way of thinking about it\n> >\n> > >\n> > > Couldn't we just rename queueRequest to initFrameContext, so that we\n> > > don't have to implement two separate functions?\n> > >\n> >\n> > I like this ideas as well\n> >\n> > Let's bikeshed the names a bit\n> >\n> > we have\n> >\n> > init()\n> > configure()\n> > queueRequest()\n> > prepare()\n> > process()\n> >\n> > with the last three operations being called repetidly in a streaming\n> > session.\n> >\n> > I was never in love with the \"prepare()\" and \"process()\" names, and I\n> > would rather use \"prepare()\" for queueRequest(), but that's what we\n> > have right now.\n> >\n> > The idea is that queueRequest actually initializes a frame context,\n> > optionally with user provided controls. So I think that using\n> > initFrameContext() is actually a good suggestion.\n> >\n> > \n>\n> Glad you like it. The only thing we should then clarify is the\n> definition of frame. In the docs it says \"The frame number is the\n> Request sequence number and identifies the desired corresponding frame\n> to target for the controls to take effect.\" I guess in the new pfc\n> scheme that will become the internal/sensor frame number, and\n> translation to request sequence happens outside. From an algorithm point\n> of view that seems sensible.\n\nWe have a mix of Request::sequence and HW frame number in the doc and\nimplementation. We should aim to clearly separate the two. We'll get\nthere ;)\n\n>\n> Best regards,\n> Stefan\n>\n> >\n> > > > /**\n> > > > * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > > > */\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > > index aa86f2c5bc21..c1adf91bbc4e 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > > @@ -30,6 +30,10 @@ public:\n> > > >\n> > > > \tint init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > > > +\n> > > > +\tvoid initFrameContext(IPAContext &context,\n> > > > +\t\t\t IPAFrameContext &frameContext) override;\n> > > > +\n> > > > \tvoid queueRequest(IPAContext &context,\n> > > > \t\t\t const uint32_t frame,\n> > > > \t\t\t IPAFrameContext &frameContext,\n> > > > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> > > > index 715cfcd8298b..82603b7b372d 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> > > > +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> > > > @@ -23,6 +23,11 @@ public:\n> > > > \t{\n> > > > \t}\n> > > >\n> > > > +\tvirtual void initFrameContext([[maybe_unused]] IPAContext &context,\n> > > > +\t\t\t\t [[maybe_unused]] IPAFrameContext &frameContext)\n> > > > +\t{\n> > > > +\t}\n> > > > +\n> > > > \tbool disabled_;\n> > > > \tbool supportsRaw_;\n> > > > };\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 9e161cabdea4..b743de9ff6af 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > > {\n> > > > \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > >\n> > > > +\tif (frameContext.underrun) {\n> > > > +\t\tfor (auto const &a : algorithms()) {\n> > > > +\t\t\tAlgorithm *algo = static_cast(a.get());\n> > > > +\t\t\tif (algo->disabled_)\n> > > > +\t\t\t\tcontinue;\n> > > > +\t\t\talgo->initFrameContext(context_, frameContext);\n> > > > +\t\t}\n> > >\n> > > Maybe that would be the place for frameContext.underrun = false?\n> >\n> > I don't think the FrameContext status flags should be handled outside\n> > of the FCQ implementation, otherwise each algo implementation would\n> > have to do that correctly by its own.\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > Best regards,\n> > > Stefan\n> > >\n> > > > +\t}\n> > > > +\n> > > > \t/*\n> > > > \t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> > > > \t * provided.\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 93BB3C324C\n\tfor ;\n\tWed, 13 Nov 2024 08:57:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C33965802;\n\tWed, 13 Nov 2024 09:57:08 +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 EC123605B5\n\tfor ;\n\tWed, 13 Nov 2024 09:57:06 +0100 (CET)","from ideasonboard.com (net-93-150-226-204.cust.vodafonedsl.it\n\t[93.150.226.204])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84277752;\n\tWed, 13 Nov 2024 09:56:53 +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=\"WbNkEa/y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731488213;\n\tbh=Miub7P9PQZ9b4PMwPiU01HSN47K5QxxJLDRv2kQe4oo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WbNkEa/yfCMEBOPfl2HlrO+mcmU34J0AuIGCB6MEVfPohsK2mDegi/vWoLWQPcsgv\n\trOU9KzyKunnR/c7NoF4WUsbtv082nS/phf9lUzEwf2UVnHgv1l5LRuLjlqVmdTyUSQ\n\tS4aN+UvUhF9o+Mtij8QsRwNGOBS8YQDBy4pWroV8=","Date":"Wed, 13 Nov 2024 09:57:02 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tDan Scally ","Subject":"Re: [PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext","Message-ID":"","References":"<20241030164853.87586-1-jacopo.mondi@ideasonboard.com>\n\t<20241030164853.87586-3-jacopo.mondi@ideasonboard.com>\n\t<52fqvjfldli7qxxiwb44ukid4hfrjc6bk6s2qpvs7kxzy2iwky@2tuxylav3uaq>\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\" "}}]