[{"id":29798,"web_url":"https://patchwork.libcamera.org/comment/29798/","msgid":"<171801549271.1489778.7026809108168691929@ping.linuxembedded.co.uk>","date":"2024-06-10T10:31:32","subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-06-07 08:59:14)\n> Change the constraint luminance target from a scalar value to a\n> piecewise linear function that needs to be sampled by the estimated lux\n> value.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> This patch depends on v5 of \"ipa:: Move Pwl from Raspberry Pi\"\n> \n> Changes in v3:\n> - use new PointF that's a typedef of Vector\n> \n> Changes in v2:\n> - s/FPoint/PointF/\n> - construct default Pwl with *two* points so that it actually constructs\n> properly\n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------\n> src/ipa/libipa/agc_mean_luminance.h | 4 ++--\n> 2 files changed, 11 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 10c16850d..575143610 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> AgcConstraint::Bound bound = static_cast(idx);\n> double qLo = content[\"qLo\"].get().value_or(0.98);\n> double qHi = content[\"qHi\"].get().value_or(1.0);\n> - double yTarget =\n> - content[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n> + Pwl yTarget;\n> + int ret = yTarget.readYaml(content[\"yTarget\"]);\n> + if (ret < 0)\n> + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) });\n> \n> AgcConstraint constraint = { bound, qLo, qHi, yTarget };\n> \n> @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> AgcConstraint::Bound::lower,\n> 0.98,\n> 1.0,\n> - 0.5\n> + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) })\n\nIn V2, I asked:\n\n> Should this be some pre-constructed global const if it's going to be\n> used in multiple places?\n \nAnd you replied:\n\nProbably? It's used three times; twice here and once more for\nrelativeLuminance.\n\nBut I don't see that action taken for v3?\n\nIf you do so, it would be a good opportunity to define and document the\nvalue choices for 0,0.5 -> 1000,0.5 and state clearly that this is a\nsingle horizontal linear default Pwl used to express a mid-point\nbrightness level regardless of the input colour temperature?\n\n\n\n\n> };\n> \n> constraintModes_[controls::ConstraintNormal].insert(\n> @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const\n> * \\param[in] constraintModeIndex The index of the constraint to adhere to\n> * \\param[in] hist A histogram over which to calculate inter-quantile means\n> * \\param[in] gain The gain to clamp\n> + * \\param[in] lux The lux value at which to sample the constraint luminance target pwl\n> *\n> * \\return The gain clamped within the constraint bounds\n> */\n> double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> const Histogram &hist,\n> - double gain)\n> + double gain, double lux)\n> {\n> std::vector &constraints = constraintModes_[constraintModeIndex];\n> for (const AgcConstraint &constraint : constraints) {\n> - double newGain = constraint.yTarget * hist.bins() /\n> + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() /\n> hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> \n> if (constraint.bound == AgcConstraint::Bound::lower &&\n> @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> exposureModeHelpers_.at(exposureModeIndex);\n> \n> double gain = estimateInitialGain(lux);\n> - gain = constraintClampGain(constraintModeIndex, yHist, gain);\n> + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux);\n> \n> /*\n> * We don't check whether we're already close to the target, because\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index 6ec2a0dc9..d43f673dd 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -38,7 +38,7 @@ public:\n> Bound bound;\n> double qLo;\n> double qHi;\n> - double yTarget;\n> + Pwl yTarget;\n> };\n> \n> int parseTuningData(const YamlObject &tuningData);\n> @@ -80,7 +80,7 @@ private:\n> double estimateInitialGain(double lux) const;\n> double constraintClampGain(uint32_t constraintModeIndex,\n> const Histogram &hist,\n> - double gain);\n> + double gain, double lux);\n> utils::Duration filterExposure(utils::Duration exposureValue);\n> \n> uint64_t frameCount_;\n> -- \n> 2.39.2\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 2070FC31E9\n\tfor ;\n\tMon, 10 Jun 2024 10:31:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CFF165462;\n\tMon, 10 Jun 2024 12:31:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1030A65446\n\tfor ;\n\tMon, 10 Jun 2024 12:31:36 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 247F4512;\n\tMon, 10 Jun 2024 12:31:24 +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=\"N74JgoK9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718015484;\n\tbh=OVtJQrNwNUAfMylRjS+cBSOJBO4WzkxQHNB9td6cmSo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=N74JgoK92djnwclKqn1edxivQdr/csBxA363wfz0KT18C+b7rrGFH0OtFIH3WN7CG\n\tst03zLyvefFNa+kzwqpCMfJtt9U8Un+A485vfiHQqmRN7XpDVAJFQXKZZbKXKBtZbH\n\t1GUbLbtumhkZm/uFiqNH2J2lQFK06BLKqzjNQJHw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>","References":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","From":"Kieran Bingham ","Cc":"Paul Elder ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 10 Jun 2024 11:31:32 +0100","Message-ID":"<171801549271.1489778.7026809108168691929@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29810,"web_url":"https://patchwork.libcamera.org/comment/29810/","msgid":"","date":"2024-06-10T14:22:04","subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-06-07 08:59:14)\n> > Change the constraint luminance target from a scalar value to a\n> > piecewise linear function that needs to be sampled by the estimated lux\n> > value.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > This patch depends on v5 of \"ipa:: Move Pwl from Raspberry Pi\"\n> > \n> > Changes in v3:\n> > - use new PointF that's a typedef of Vector\n> > \n> > Changes in v2:\n> > - s/FPoint/PointF/\n> > - construct default Pwl with *two* points so that it actually constructs\n> > properly\n> > ---\n> > src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------\n> > src/ipa/libipa/agc_mean_luminance.h | 4 ++--\n> > 2 files changed, 11 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 10c16850d..575143610 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > AgcConstraint::Bound bound = static_cast(idx);\n> > double qLo = content[\"qLo\"].get().value_or(0.98);\n> > double qHi = content[\"qHi\"].get().value_or(1.0);\n> > - double yTarget =\n> > - content[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n> > + Pwl yTarget;\n> > + int ret = yTarget.readYaml(content[\"yTarget\"]);\n> > + if (ret < 0)\n> > + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) });\n> > \n> > AgcConstraint constraint = { bound, qLo, qHi, yTarget };\n> > \n> > @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> > AgcConstraint::Bound::lower,\n> > 0.98,\n> > 1.0,\n> > - 0.5\n> > + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) })\n> \n> In V2, I asked:\n> \n> > Should this be some pre-constructed global const if it's going to be\n> > used in multiple places?\n> \n> And you replied:\n> \n> Probably? It's used three times; twice here and once more for\n> relativeLuminance.\n> \n> But I don't see that action taken for v3?\n\nIt's split between two patches and I don't really want to introduce\nanother depencency at this stage...\n\n\nPaul\n\n> \n> If you do so, it would be a good opportunity to define and document the\n> value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a\n> single horizontal linear default Pwl used to express a mid-point\n> brightness level regardless of the input colour temperature?\n> \n> \n> \n> \n> > };\n> > \n> > constraintModes_[controls::ConstraintNormal].insert(\n> > @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const\n> > * \\param[in] constraintModeIndex The index of the constraint to adhere to\n> > * \\param[in] hist A histogram over which to calculate inter-quantile means\n> > * \\param[in] gain The gain to clamp\n> > + * \\param[in] lux The lux value at which to sample the constraint luminance target pwl\n> > *\n> > * \\return The gain clamped within the constraint bounds\n> > */\n> > double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > const Histogram &hist,\n> > - double gain)\n> > + double gain, double lux)\n> > {\n> > std::vector &constraints = constraintModes_[constraintModeIndex];\n> > for (const AgcConstraint &constraint : constraints) {\n> > - double newGain = constraint.yTarget * hist.bins() /\n> > + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() /\n> > hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> > \n> > if (constraint.bound == AgcConstraint::Bound::lower &&\n> > @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> > exposureModeHelpers_.at(exposureModeIndex);\n> > \n> > double gain = estimateInitialGain(lux);\n> > - gain = constraintClampGain(constraintModeIndex, yHist, gain);\n> > + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux);\n> > \n> > /*\n> > * We don't check whether we're already close to the target, because\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index 6ec2a0dc9..d43f673dd 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -38,7 +38,7 @@ public:\n> > Bound bound;\n> > double qLo;\n> > double qHi;\n> > - double yTarget;\n> > + Pwl yTarget;\n> > };\n> > \n> > int parseTuningData(const YamlObject &tuningData);\n> > @@ -80,7 +80,7 @@ private:\n> > double estimateInitialGain(double lux) const;\n> > double constraintClampGain(uint32_t constraintModeIndex,\n> > const Histogram &hist,\n> > - double gain);\n> > + double gain, double lux);\n> > utils::Duration filterExposure(utils::Duration exposureValue);\n> > \n> > uint64_t frameCount_;\n> > -- \n> > 2.39.2\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 82A80BD87C\n\tfor ;\n\tMon, 10 Jun 2024 14:22:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9062965462;\n\tMon, 10 Jun 2024 16:22:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 250BC634D5\n\tfor ;\n\tMon, 10 Jun 2024 16:22:12 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40912397;\n\tMon, 10 Jun 2024 16:21:59 +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=\"fzNpuBVp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718029320;\n\tbh=ytBbPVr+mLK//jkzN4xCVGFqp+s12OstcrgaveNazpQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fzNpuBVpTKrnxGEhi2oyda7Eqr1cQimqQhIkVpjhtVl0gCuHPERXjwK6eQvKjM3n7\n\t50R3nzeYChNkW5oi0J7GkNXlYROVMh34UzVl/NUt/WTtPPd/YmjZ4lTXd3dwEYrJ2w\n\ti+xHv6yyAwSxRBp7vgjO8tt03BTw1O8aVHbn099M=","Date":"Mon, 10 Jun 2024 23:22:04 +0900","From":"Paul Elder ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","Message-ID":"","References":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>\n\t<171801549271.1489778.7026809108168691929@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<171801549271.1489778.7026809108168691929@ping.linuxembedded.co.uk>","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":29814,"web_url":"https://patchwork.libcamera.org/comment/29814/","msgid":"<171803084398.2248009.17947778596567447771@ping.linuxembedded.co.uk>","date":"2024-06-10T14:47:23","subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-06-10 15:22:04)\n> On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote:\n> > Quoting Paul Elder (2024-06-07 08:59:14)\n> > > Change the constraint luminance target from a scalar value to a\n> > > piecewise linear function that needs to be sampled by the estimated lux\n> > > value.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \n> > > ---\n> > > This patch depends on v5 of \"ipa:: Move Pwl from Raspberry Pi\"\n> > > \n> > > Changes in v3:\n> > > - use new PointF that's a typedef of Vector\n> > > \n> > > Changes in v2:\n> > > - s/FPoint/PointF/\n> > > - construct default Pwl with *two* points so that it actually constructs\n> > > properly\n> > > ---\n> > > src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------\n> > > src/ipa/libipa/agc_mean_luminance.h | 4 ++--\n> > > 2 files changed, 11 insertions(+), 8 deletions(-)\n> > > \n> > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > index 10c16850d..575143610 100644\n> > > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > > AgcConstraint::Bound bound = static_cast(idx);\n> > > double qLo = content[\"qLo\"].get().value_or(0.98);\n> > > double qHi = content[\"qHi\"].get().value_or(1.0);\n> > > - double yTarget =\n> > > - content[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n> > > + Pwl yTarget;\n> > > + int ret = yTarget.readYaml(content[\"yTarget\"]);\n> > > + if (ret < 0)\n> > > + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) });\n> > > \n> > > AgcConstraint constraint = { bound, qLo, qHi, yTarget };\n> > > \n> > > @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> > > AgcConstraint::Bound::lower,\n> > > 0.98,\n> > > 1.0,\n> > > - 0.5\n> > > + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) })\n> > \n> > In V2, I asked:\n> > \n> > > Should this be some pre-constructed global const if it's going to be\n> > > used in multiple places?\n> > \n> > And you replied:\n> > \n> > Probably? It's used three times; twice here and once more for\n> > relativeLuminance.\n> > \n> > But I don't see that action taken for v3?\n> \n> It's split between two patches and I don't really want to introduce\n> another depencency at this stage...\n\nSo - what action can we take to earn the RB tag?\n\nPwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) is opaque,\nunderdocumented, and used in multiple places.\n\nNot documenting the meaning behind \"0, 0.5\"->\"1000, 0.5\" is currently\nblocking me giving a tag...\n\n\nFurthermore I see \"ipa: libipa: agc: Change luminance target to\npiecewise linear function\" does this:\n\n\n+\tstd::vector points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }),\n+\t\t\t\t\t Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) };\n\nwhich is suddenly 0->10000.\n\nIs that the one you mean ?\n\nkDefaultRelativeLuminanceTarget is set to 0.16; ? So they're not the\nsame. Why aren't they the same? Should they be?\n\n\n\n--\nKieran\n\n\n> \n> \n> Paul\n> \n> > \n> > If you do so, it would be a good opportunity to define and document the\n> > value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a\n> > single horizontal linear default Pwl used to express a mid-point\n> > brightness level regardless of the input colour temperature?\n> > \n> > \n> > \n> > \n> > > };\n> > > \n> > > constraintModes_[controls::ConstraintNormal].insert(\n> > > @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const\n> > > * \\param[in] constraintModeIndex The index of the constraint to adhere to\n> > > * \\param[in] hist A histogram over which to calculate inter-quantile means\n> > > * \\param[in] gain The gain to clamp\n> > > + * \\param[in] lux The lux value at which to sample the constraint luminance target pwl\n> > > *\n> > > * \\return The gain clamped within the constraint bounds\n> > > */\n> > > double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > > const Histogram &hist,\n> > > - double gain)\n> > > + double gain, double lux)\n> > > {\n> > > std::vector &constraints = constraintModes_[constraintModeIndex];\n> > > for (const AgcConstraint &constraint : constraints) {\n> > > - double newGain = constraint.yTarget * hist.bins() /\n> > > + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() /\n> > > hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> > > \n> > > if (constraint.bound == AgcConstraint::Bound::lower &&\n> > > @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> > > exposureModeHelpers_.at(exposureModeIndex);\n> > > \n> > > double gain = estimateInitialGain(lux);\n> > > - gain = constraintClampGain(constraintModeIndex, yHist, gain);\n> > > + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux);\n> > > \n> > > /*\n> > > * We don't check whether we're already close to the target, because\n> > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > > index 6ec2a0dc9..d43f673dd 100644\n> > > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > > @@ -38,7 +38,7 @@ public:\n> > > Bound bound;\n> > > double qLo;\n> > > double qHi;\n> > > - double yTarget;\n> > > + Pwl yTarget;\n> > > };\n> > > \n> > > int parseTuningData(const YamlObject &tuningData);\n> > > @@ -80,7 +80,7 @@ private:\n> > > double estimateInitialGain(double lux) const;\n> > > double constraintClampGain(uint32_t constraintModeIndex,\n> > > const Histogram &hist,\n> > > - double gain);\n> > > + double gain, double lux);\n> > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > > \n> > > uint64_t frameCount_;\n> > > -- \n> > > 2.39.2\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 B6706BD87C\n\tfor ;\n\tMon, 10 Jun 2024 14:47:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF55565466;\n\tMon, 10 Jun 2024 16:47:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D78F6545E\n\tfor ;\n\tMon, 10 Jun 2024 16:47:27 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B9A4397;\n\tMon, 10 Jun 2024 16:47:15 +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=\"X8GtYpzS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718030835;\n\tbh=0b6mE5jQT5jaDyn5DvMZH8orLJY+Whlznza9rNGbKY4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=X8GtYpzSiFWLag8BzEWefE4MVWxVvIqvjBbUvyV1imZybdRwXlBwzl1uAd7gm6b6G\n\tc7U3mNvf6UBpstQKU5m3d/vDb8e1XaTPkPCsaQL+2ksIwBiUYttiFaHVrHXEzTd2eG\n\t0W47olyTYAAsGY5/deJpd3jMt5cdLyNCeM7DgRCM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>\n\t<171801549271.1489778.7026809108168691929@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Paul Elder ","Date":"Mon, 10 Jun 2024 15:47:23 +0100","Message-ID":"<171803084398.2248009.17947778596567447771@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29852,"web_url":"https://patchwork.libcamera.org/comment/29852/","msgid":"<20240611224810.GC15006@pendragon.ideasonboard.com>","date":"2024-06-11T22:48:10","subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 10, 2024 at 03:47:23PM +0100, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-06-10 15:22:04)\n> > On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote:\n> > > Quoting Paul Elder (2024-06-07 08:59:14)\n> > > > Change the constraint luminance target from a scalar value to a\n> > > > piecewise linear function that needs to be sampled by the estimated lux\n> > > > value.\n> > > > \n> > > > Signed-off-by: Paul Elder \n> > > > \n> > > > ---\n> > > > This patch depends on v5 of \"ipa:: Move Pwl from Raspberry Pi\"\n> > > > \n> > > > Changes in v3:\n> > > > - use new PointF that's a typedef of Vector\n> > > > \n> > > > Changes in v2:\n> > > > - s/FPoint/PointF/\n> > > > - construct default Pwl with *two* points so that it actually constructs\n> > > > properly\n> > > > ---\n> > > > src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------\n> > > > src/ipa/libipa/agc_mean_luminance.h | 4 ++--\n> > > > 2 files changed, 11 insertions(+), 8 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > index 10c16850d..575143610 100644\n> > > > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > > > AgcConstraint::Bound bound = static_cast(idx);\n> > > > double qLo = content[\"qLo\"].get().value_or(0.98);\n> > > > double qHi = content[\"qHi\"].get().value_or(1.0);\n> > > > - double yTarget =\n> > > > - content[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n> > > > + Pwl yTarget;\n> > > > + int ret = yTarget.readYaml(content[\"yTarget\"]);\n> > > > + if (ret < 0)\n> > > > + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) });\n> > > > \n> > > > AgcConstraint constraint = { bound, qLo, qHi, yTarget };\n> > > > \n> > > > @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> > > > AgcConstraint::Bound::lower,\n> > > > 0.98,\n> > > > 1.0,\n> > > > - 0.5\n> > > > + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) })\n> > > \n> > > In V2, I asked:\n> > > \n> > > > Should this be some pre-constructed global const if it's going to be\n> > > > used in multiple places?\n> > > \n> > > And you replied:\n> > > \n> > > Probably? It's used three times; twice here and once more for\n> > > relativeLuminance.\n> > > \n> > > But I don't see that action taken for v3?\n> > \n> > It's split between two patches and I don't really want to introduce\n> > another depencency at this stage...\n\nLet's get the dependencies merged, we're nearly there.\n\n> So - what action can we take to earn the RB tag?\n> \n> Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) is opaque,\n> underdocumented, and used in multiple places.\n> \n> Not documenting the meaning behind \"0, 0.5\"->\"1000, 0.5\" is currently\n> blocking me giving a tag...\n> \n> \n> Furthermore I see \"ipa: libipa: agc: Change luminance target to\n> piecewise linear function\" does this:\n> \n> +\tstd::vector points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }),\n> +\t\t\t\t\t Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) };\n> \n> which is suddenly 0->10000.\n\nThat won't make a difference in practice, as the function is flat, but I\nagree it would be nice to be consistent. For a default curve, I would\nuse typical lux levels. 0 is complete darkness, I think that's as good\nas anything, even if we could replace it with 1 (full moonlight) or 10\n(street lighting at night). 10000 is the lower end of ambient daylight,\nwhile 1000 is the lower end of overcast daylight (or a really well lit\nroom).\n\n> Is that the one you mean ?\n> \n> kDefaultRelativeLuminanceTarget is set to 0.16; ? So they're not the\n> same. Why aren't they the same? Should they be?\n\nThat one is a bit more concerning.\n\n> > > If you do so, it would be a good opportunity to define and document the\n> > > value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a\n> > > single horizontal linear default Pwl used to express a mid-point\n> > > brightness level regardless of the input colour temperature?\n> > > \n> > > > };\n> > > > \n> > > > constraintModes_[controls::ConstraintNormal].insert(\n> > > > @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const\n> > > > * \\param[in] constraintModeIndex The index of the constraint to adhere to\n> > > > * \\param[in] hist A histogram over which to calculate inter-quantile means\n> > > > * \\param[in] gain The gain to clamp\n> > > > + * \\param[in] lux The lux value at which to sample the constraint luminance target pwl\n> > > > *\n> > > > * \\return The gain clamped within the constraint bounds\n> > > > */\n> > > > double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > > > const Histogram &hist,\n> > > > - double gain)\n> > > > + double gain, double lux)\n> > > > {\n> > > > std::vector &constraints = constraintModes_[constraintModeIndex];\n> > > > for (const AgcConstraint &constraint : constraints) {\n> > > > - double newGain = constraint.yTarget * hist.bins() /\n> > > > + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() /\n> > > > hist.interQuantileMean(constraint.qLo, constraint.qHi);\n\nLet's shorten that a bit:\n\n\t\t\tdouble newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux))\n\t\t\t\t * hist.bins()\n\t\t\t\t / hist.interQuantileMean(constraint.qLo, constraint.qHi);\n\nBut I recall you mentioned that the Pwl class models a function that is\nflat outside of the domain, so do you need to clamp ? If not,\n\n\t\t\tdouble newGain = constraint.yTarget.eval(lux) * hist.bins()\n\t\t\t\t / hist.interQuantileMean(constraint.qLo, constraint.qHi);\n\nlooks nicer.\n\n> > > > \n> > > > if (constraint.bound == AgcConstraint::Bound::lower &&\n> > > > @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> > > > exposureModeHelpers_.at(exposureModeIndex);\n> > > > \n> > > > double gain = estimateInitialGain(lux);\n> > > > - gain = constraintClampGain(constraintModeIndex, yHist, gain);\n> > > > + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux);\n> > > > \n> > > > /*\n> > > > * We don't check whether we're already close to the target, because\n> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > > > index 6ec2a0dc9..d43f673dd 100644\n> > > > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > > > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > > > @@ -38,7 +38,7 @@ public:\n> > > > Bound bound;\n> > > > double qLo;\n> > > > double qHi;\n> > > > - double yTarget;\n> > > > + Pwl yTarget;\n> > > > };\n> > > > \n> > > > int parseTuningData(const YamlObject &tuningData);\n> > > > @@ -80,7 +80,7 @@ private:\n> > > > double estimateInitialGain(double lux) const;\n> > > > double constraintClampGain(uint32_t constraintModeIndex,\n> > > > const Histogram &hist,\n> > > > - double gain);\n> > > > + double gain, double lux);\n> > > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > > > \n> > > > uint64_t frameCount_;","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 1A83FC3237\n\tfor ;\n\tTue, 11 Jun 2024 22:48:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17009634D5;\n\tWed, 12 Jun 2024 00:48:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92AD261A26\n\tfor ;\n\tWed, 12 Jun 2024 00:48:30 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5B57A230;\n\tWed, 12 Jun 2024 00:48:17 +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=\"PeqGE2qC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718146097;\n\tbh=8tH4b5uf6lXqUePx8hs2+X0/X0OFJkB7bc4Nq9OCXB0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PeqGE2qCz411h5aHJsTKlbh+d/wHRN2tIoTZS7MiN9SozPBJ036oL1CVNEPTTcfsg\n\ti4CO+Bn0yMDIyj3mX4Cf92n7KwbxyNExeouTfe8NaAr4nmTZzwQBnGgDCY37uNp9kd\n\tG7qADK/UF+q511i4zchEnlxKsxkrem2BUWaBGLfk=","Date":"Wed, 12 Jun 2024 01:48:10 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","Message-ID":"<20240611224810.GC15006@pendragon.ideasonboard.com>","References":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>\n\t<171801549271.1489778.7026809108168691929@ping.linuxembedded.co.uk>\n\t\n\t<171803084398.2248009.17947778596567447771@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171803084398.2248009.17947778596567447771@ping.linuxembedded.co.uk>","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":29940,"web_url":"https://patchwork.libcamera.org/comment/29940/","msgid":"<19e0e8e8-1ef8-4a72-bca8-0747498490f7@ideasonboard.com>","date":"2024-06-13T21:39:00","subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hello\n\nOn 11/06/2024 23:48, Laurent Pinchart wrote:\n> On Mon, Jun 10, 2024 at 03:47:23PM +0100, Kieran Bingham wrote:\n>> Quoting Paul Elder (2024-06-10 15:22:04)\n>>> On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote:\n>>>> Quoting Paul Elder (2024-06-07 08:59:14)\n>>>>> Change the constraint luminance target from a scalar value to a\n>>>>> piecewise linear function that needs to be sampled by the estimated lux\n>>>>> value.\n>>>>>\n>>>>> Signed-off-by: Paul Elder \n>>>>>\n>>>>> ---\n>>>>> This patch depends on v5 of \"ipa:: Move Pwl from Raspberry Pi\"\n>>>>>\n>>>>> Changes in v3:\n>>>>> - use new PointF that's a typedef of Vector\n>>>>>\n>>>>> Changes in v2:\n>>>>> - s/FPoint/PointF/\n>>>>> - construct default Pwl with *two* points so that it actually constructs\n>>>>> properly\n>>>>> ---\n>>>>> src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------\n>>>>> src/ipa/libipa/agc_mean_luminance.h | 4 ++--\n>>>>> 2 files changed, 11 insertions(+), 8 deletions(-)\n>>>>>\n>>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n>>>>> index 10c16850d..575143610 100644\n>>>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n>>>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n>>>>> @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n>>>>> AgcConstraint::Bound bound = static_cast(idx);\n>>>>> double qLo = content[\"qLo\"].get().value_or(0.98);\n>>>>> double qHi = content[\"qHi\"].get().value_or(1.0);\n>>>>> - double yTarget =\n>>>>> - content[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n>>>>> + Pwl yTarget;\n>>>>> + int ret = yTarget.readYaml(content[\"yTarget\"]);\n>>>>> + if (ret < 0)\n>>>>> + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) });\n>>>>> \n>>>>> AgcConstraint constraint = { bound, qLo, qHi, yTarget };\n>>>>> \n>>>>> @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n>>>>> AgcConstraint::Bound::lower,\n>>>>> 0.98,\n>>>>> 1.0,\n>>>>> - 0.5\n>>>>> + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) })\n>>>> In V2, I asked:\n>>>>\n>>>>> Should this be some pre-constructed global const if it's going to be\n>>>>> used in multiple places?\n>>>> \n>>>> And you replied:\n>>>>\n>>>> Probably? It's used three times; twice here and once more for\n>>>> relativeLuminance.\n>>>>\n>>>> But I don't see that action taken for v3?\n>>> It's split between two patches and I don't really want to introduce\n>>> another depencency at this stage...\n> Let's get the dependencies merged, we're nearly there.\n>\n>> So - what action can we take to earn the RB tag?\n>>\n>> Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) is opaque,\n>> underdocumented, and used in multiple places.\n>>\n>> Not documenting the meaning behind \"0, 0.5\"->\"1000, 0.5\" is currently\n>> blocking me giving a tag...\n>>\n>>\n>> Furthermore I see \"ipa: libipa: agc: Change luminance target to\n>> piecewise linear function\" does this:\n>>\n>> +\tstd::vector points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }),\n>> +\t\t\t\t\t Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) };\n>>\n>> which is suddenly 0->10000.\n> That won't make a difference in practice, as the function is flat, but I\n> agree it would be nice to be consistent. For a default curve, I would\n> use typical lux levels. 0 is complete darkness, I think that's as good\n> as anything, even if we could replace it with 1 (full moonlight) or 10\n> (street lighting at night). 10000 is the lower end of ambient daylight,\n> while 1000 is the lower end of overcast daylight (or a really well lit\n> room).\n>\n>> Is that the one you mean ?\n>>\n>> kDefaultRelativeLuminanceTarget is set to 0.16; ? So they're not the\n>> same. Why aren't they the same? Should they be?\n> That one is a bit more concerning.\n\n\nThey're targets for different things; they shouldn't be the same. kDefaultRelativeLuminanceTarget is \nthe default target we chose for the image as a whole - the mean luminance agc method tries to drive \nthe mean luminance of the entire image towards that target. The yTarget for the constraints here are \njust to clamp the mean luminance for a particular portion of the histogram to a value. The effect of \nthe defaults is to drive the mean luminance of the entire image to 0.16, but to clamp that of the \ntop 2% of the histogram to at least 0.5 - that's how it currently behaves too, though the default is \nexpressed as a single value rather than a flat piecewise linear function.\n\n\n>\n>>>> If you do so, it would be a good opportunity to define and document the\n>>>> value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a\n>>>> single horizontal linear default Pwl used to express a mid-point\n>>>> brightness level regardless of the input colour temperature?\n>>>>\n>>>>> };\n>>>>> \n>>>>> constraintModes_[controls::ConstraintNormal].insert(\n>>>>> @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const\n>>>>> * \\param[in] constraintModeIndex The index of the constraint to adhere to\n>>>>> * \\param[in] hist A histogram over which to calculate inter-quantile means\n>>>>> * \\param[in] gain The gain to clamp\n>>>>> + * \\param[in] lux The lux value at which to sample the constraint luminance target pwl\n>>>>> *\n>>>>> * \\return The gain clamped within the constraint bounds\n>>>>> */\n>>>>> double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n>>>>> const Histogram &hist,\n>>>>> - double gain)\n>>>>> + double gain, double lux)\n>>>>> {\n>>>>> std::vector &constraints = constraintModes_[constraintModeIndex];\n>>>>> for (const AgcConstraint &constraint : constraints) {\n>>>>> - double newGain = constraint.yTarget * hist.bins() /\n>>>>> + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() /\n>>>>> hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> Let's shorten that a bit:\n>\n> \t\t\tdouble newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux))\n> \t\t\t\t * hist.bins()\n> \t\t\t\t / hist.interQuantileMean(constraint.qLo, constraint.qHi);\n>\n> But I recall you mentioned that the Pwl class models a function that is\n> flat outside of the domain, so do you need to clamp ? If not,\n>\n> \t\t\tdouble newGain = constraint.yTarget.eval(lux) * hist.bins()\n> \t\t\t\t / hist.interQuantileMean(constraint.qLo, constraint.qHi);\n>\n> looks nicer.\n>\n>>>>> \n>>>>> if (constraint.bound == AgcConstraint::Bound::lower &&\n>>>>> @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n>>>>> exposureModeHelpers_.at(exposureModeIndex);\n>>>>> \n>>>>> double gain = estimateInitialGain(lux);\n>>>>> - gain = constraintClampGain(constraintModeIndex, yHist, gain);\n>>>>> + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux);\n>>>>> \n>>>>> /*\n>>>>> * We don't check whether we're already close to the target, because\n>>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n>>>>> index 6ec2a0dc9..d43f673dd 100644\n>>>>> --- a/src/ipa/libipa/agc_mean_luminance.h\n>>>>> +++ b/src/ipa/libipa/agc_mean_luminance.h\n>>>>> @@ -38,7 +38,7 @@ public:\n>>>>> Bound bound;\n>>>>> double qLo;\n>>>>> double qHi;\n>>>>> - double yTarget;\n>>>>> + Pwl yTarget;\n>>>>> };\n>>>>> \n>>>>> int parseTuningData(const YamlObject &tuningData);\n>>>>> @@ -80,7 +80,7 @@ private:\n>>>>> double estimateInitialGain(double lux) const;\n>>>>> double constraintClampGain(uint32_t constraintModeIndex,\n>>>>> const Histogram &hist,\n>>>>> - double gain);\n>>>>> + double gain, double lux);\n>>>>> utils::Duration filterExposure(utils::Duration exposureValue);\n>>>>> \n>>>>> uint64_t frameCount_;","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 A2E8ABD87C\n\tfor ;\n\tThu, 13 Jun 2024 21:39:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5BA626548F;\n\tThu, 13 Jun 2024 23:39:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41FF76548B\n\tfor ;\n\tThu, 13 Jun 2024 23:39:04 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D79FF4CC\n\tfor ;\n\tThu, 13 Jun 2024 23:38:49 +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=\"J8B/XKcn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718314729;\n\tbh=Ikqnmd1BPpWyvLWzkvrznSlB3rWWE4eLGKqYHlVh95I=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=J8B/XKcn2hHnlxvkGcacnkZRZXeZoi8searjFK+Dk0+0TKwdn9AyNXWoLeqUZ0a6t\n\tJjkcr2gqjLtmSvcNDeV9LFbBaLFriyMcGaFo/qY4cMeDaely6dJjX0+VRJr3FG+Uxe\n\tJ+LgH6cIv9CTaqVbAA02/IgVsqi8Mc5jmyFSzVK8=","Message-ID":"<19e0e8e8-1ef8-4a72-bca8-0747498490f7@ideasonboard.com>","Date":"Thu, 13 Jun 2024 22:39:00 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","To":"libcamera-devel@lists.libcamera.org","References":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>\n\t<171801549271.1489778.7026809108168691929@ping.linuxembedded.co.uk>\n\t\n\t<171803084398.2248009.17947778596567447771@ping.linuxembedded.co.uk>\n\t<20240611224810.GC15006@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240611224810.GC15006@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":29943,"web_url":"https://patchwork.libcamera.org/comment/29943/","msgid":"<20240613223450.GC6806@pendragon.ideasonboard.com>","date":"2024-06-13T22:34:50","subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dan,\n\nOn Thu, Jun 13, 2024 at 10:39:00PM +0100, Daniel Scally wrote:\n> On 11/06/2024 23:48, Laurent Pinchart wrote:\n> > On Mon, Jun 10, 2024 at 03:47:23PM +0100, Kieran Bingham wrote:\n> >> Quoting Paul Elder (2024-06-10 15:22:04)\n> >>> On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote:\n> >>>> Quoting Paul Elder (2024-06-07 08:59:14)\n> >>>>> Change the constraint luminance target from a scalar value to a\n> >>>>> piecewise linear function that needs to be sampled by the estimated lux\n> >>>>> value.\n> >>>>>\n> >>>>> Signed-off-by: Paul Elder \n> >>>>>\n> >>>>> ---\n> >>>>> This patch depends on v5 of \"ipa:: Move Pwl from Raspberry Pi\"\n> >>>>>\n> >>>>> Changes in v3:\n> >>>>> - use new PointF that's a typedef of Vector\n> >>>>>\n> >>>>> Changes in v2:\n> >>>>> - s/FPoint/PointF/\n> >>>>> - construct default Pwl with *two* points so that it actually constructs\n> >>>>> properly\n> >>>>> ---\n> >>>>> src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------\n> >>>>> src/ipa/libipa/agc_mean_luminance.h | 4 ++--\n> >>>>> 2 files changed, 11 insertions(+), 8 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> >>>>> index 10c16850d..575143610 100644\n> >>>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> >>>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> >>>>> @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> >>>>> AgcConstraint::Bound bound = static_cast(idx);\n> >>>>> double qLo = content[\"qLo\"].get().value_or(0.98);\n> >>>>> double qHi = content[\"qHi\"].get().value_or(1.0);\n> >>>>> - double yTarget =\n> >>>>> - content[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n> >>>>> + Pwl yTarget;\n> >>>>> + int ret = yTarget.readYaml(content[\"yTarget\"]);\n> >>>>> + if (ret < 0)\n> >>>>> + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) });\n> >>>>> \n> >>>>> AgcConstraint constraint = { bound, qLo, qHi, yTarget };\n> >>>>> \n> >>>>> @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> >>>>> AgcConstraint::Bound::lower,\n> >>>>> 0.98,\n> >>>>> 1.0,\n> >>>>> - 0.5\n> >>>>> + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) })\n> >>>>\n> >>>> In V2, I asked:\n> >>>>\n> >>>>> Should this be some pre-constructed global const if it's going to be\n> >>>>> used in multiple places?\n> >>>> \n> >>>> And you replied:\n> >>>>\n> >>>> Probably? It's used three times; twice here and once more for\n> >>>> relativeLuminance.\n> >>>>\n> >>>> But I don't see that action taken for v3?\n> >>>\n> >>> It's split between two patches and I don't really want to introduce\n> >>> another depencency at this stage...\n> >\n> > Let's get the dependencies merged, we're nearly there.\n> >\n> >> So - what action can we take to earn the RB tag?\n> >>\n> >> Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) is opaque,\n> >> underdocumented, and used in multiple places.\n> >>\n> >> Not documenting the meaning behind \"0, 0.5\"->\"1000, 0.5\" is currently\n> >> blocking me giving a tag...\n> >>\n> >>\n> >> Furthermore I see \"ipa: libipa: agc: Change luminance target to\n> >> piecewise linear function\" does this:\n> >>\n> >> +\tstd::vector points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }),\n> >> +\t\t\t\t\t Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) };\n> >>\n> >> which is suddenly 0->10000.\n> >\n> > That won't make a difference in practice, as the function is flat, but I\n> > agree it would be nice to be consistent. For a default curve, I would\n> > use typical lux levels. 0 is complete darkness, I think that's as good\n> > as anything, even if we could replace it with 1 (full moonlight) or 10\n> > (street lighting at night). 10000 is the lower end of ambient daylight,\n> > while 1000 is the lower end of overcast daylight (or a really well lit\n> > room).\n> >\n> >> Is that the one you mean ?\n> >>\n> >> kDefaultRelativeLuminanceTarget is set to 0.16; ? So they're not the\n> >> same. Why aren't they the same? Should they be?\n> > That one is a bit more concerning.\n> \n> They're targets for different things; they shouldn't be the same. kDefaultRelativeLuminanceTarget is \n> the default target we chose for the image as a whole - the mean luminance agc method tries to drive \n> the mean luminance of the entire image towards that target. The yTarget for the constraints here are \n> just to clamp the mean luminance for a particular portion of the histogram to a value. The effect of \n> the defaults is to drive the mean luminance of the entire image to 0.16, but to clamp that of the \n> top 2% of the histogram to at least 0.5 - that's how it currently behaves too, though the default is \n> expressed as a single value rather than a flat piecewise linear function.\n\nYou're right, my bad.\n\nOne day it would be very nice to document the AGC algorithm. I\nunderstand that we combine two different mechanisms, but I don't\nunderstand why that produces the desired result :-)\n\n> >>>> If you do so, it would be a good opportunity to define and document the\n> >>>> value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a\n> >>>> single horizontal linear default Pwl used to express a mid-point\n> >>>> brightness level regardless of the input colour temperature?\n> >>>>\n> >>>>> };\n> >>>>> \n> >>>>> constraintModes_[controls::ConstraintNormal].insert(\n> >>>>> @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const\n> >>>>> * \\param[in] constraintModeIndex The index of the constraint to adhere to\n> >>>>> * \\param[in] hist A histogram over which to calculate inter-quantile means\n> >>>>> * \\param[in] gain The gain to clamp\n> >>>>> + * \\param[in] lux The lux value at which to sample the constraint luminance target pwl\n> >>>>> *\n> >>>>> * \\return The gain clamped within the constraint bounds\n> >>>>> */\n> >>>>> double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> >>>>> const Histogram &hist,\n> >>>>> - double gain)\n> >>>>> + double gain, double lux)\n> >>>>> {\n> >>>>> std::vector &constraints = constraintModes_[constraintModeIndex];\n> >>>>> for (const AgcConstraint &constraint : constraints) {\n> >>>>> - double newGain = constraint.yTarget * hist.bins() /\n> >>>>> + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() /\n> >>>>> hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> >\n> > Let's shorten that a bit:\n> >\n> > \t\t\tdouble newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux))\n> > \t\t\t\t * hist.bins()\n> > \t\t\t\t / hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> >\n> > But I recall you mentioned that the Pwl class models a function that is\n> > flat outside of the domain, so do you need to clamp ? If not,\n> >\n> > \t\t\tdouble newGain = constraint.yTarget.eval(lux) * hist.bins()\n> > \t\t\t\t / hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> >\n> > looks nicer.\n> >\n> >>>>> \n> >>>>> if (constraint.bound == AgcConstraint::Bound::lower &&\n> >>>>> @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> >>>>> exposureModeHelpers_.at(exposureModeIndex);\n> >>>>> \n> >>>>> double gain = estimateInitialGain(lux);\n> >>>>> - gain = constraintClampGain(constraintModeIndex, yHist, gain);\n> >>>>> + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux);\n> >>>>> \n> >>>>> /*\n> >>>>> * We don't check whether we're already close to the target, because\n> >>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> >>>>> index 6ec2a0dc9..d43f673dd 100644\n> >>>>> --- a/src/ipa/libipa/agc_mean_luminance.h\n> >>>>> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> >>>>> @@ -38,7 +38,7 @@ public:\n> >>>>> Bound bound;\n> >>>>> double qLo;\n> >>>>> double qHi;\n> >>>>> - double yTarget;\n> >>>>> + Pwl yTarget;\n> >>>>> };\n> >>>>> \n> >>>>> int parseTuningData(const YamlObject &tuningData);\n> >>>>> @@ -80,7 +80,7 @@ private:\n> >>>>> double estimateInitialGain(double lux) const;\n> >>>>> double constraintClampGain(uint32_t constraintModeIndex,\n> >>>>> const Histogram &hist,\n> >>>>> - double gain);\n> >>>>> + double gain, double lux);\n> >>>>> utils::Duration filterExposure(utils::Duration exposureValue);\n> >>>>> \n> >>>>> uint64_t frameCount_;","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 19E8EC3237\n\tfor ;\n\tThu, 13 Jun 2024 22:35:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 04BE66548F;\n\tFri, 14 Jun 2024 00:35:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE5DD6545A\n\tfor ;\n\tFri, 14 Jun 2024 00:35:11 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EEDC23D5;\n\tFri, 14 Jun 2024 00:34:56 +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=\"almMeA01\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718318097;\n\tbh=775F045jItlte6onb3XrO5JDayGWEzZC3eWMnrF9AJg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=almMeA016SO2FmnesKmPaAxxadpDLXq4PmmpqoEfoxb8ZWLl1C/ChUC2HfJqApBZV\n\t0OC0rx9UZpsOXS4NQcbmfBXIU28IH35dNdI5RBUOF3e1ha9239UfXndglHIfFVmrxT\n\tz4JIeZRPQhQAZVACJezXEI4sN6RGIQI99NGk/I1c=","Date":"Fri, 14 Jun 2024 01:34:50 +0300","From":"Laurent Pinchart ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","Message-ID":"<20240613223450.GC6806@pendragon.ideasonboard.com>","References":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>\n\t<171801549271.1489778.7026809108168691929@ping.linuxembedded.co.uk>\n\t\n\t<171803084398.2248009.17947778596567447771@ping.linuxembedded.co.uk>\n\t<20240611224810.GC15006@pendragon.ideasonboard.com>\n\t<19e0e8e8-1ef8-4a72-bca8-0747498490f7@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<19e0e8e8-1ef8-4a72-bca8-0747498490f7@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":29946,"web_url":"https://patchwork.libcamera.org/comment/29946/","msgid":"","date":"2024-06-14T08:55:47","subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul\n\nOn 07/06/2024 08:59, Paul Elder wrote:\n> Change the constraint luminance target from a scalar value to a\n> piecewise linear function that needs to be sampled by the estimated lux\n> value.\n>\n> Signed-off-by: Paul Elder \nFor me, the only problem now is that the documentation for parseTuningData() now needs updating, as \nit still suggests a single value for the constraint mode yTarget rather than an array. The rest of \nthe patch I think is fine.\n> ---\n> This patch depends on v5 of \"ipa:: Move Pwl from Raspberry Pi\"\n>\n> Changes in v3:\n> - use new PointF that's a typedef of Vector\n>\n> Changes in v2:\n> - s/FPoint/PointF/\n> - construct default Pwl with *two* points so that it actually constructs\n> properly\n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------\n> src/ipa/libipa/agc_mean_luminance.h | 4 ++--\n> 2 files changed, 11 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 10c16850d..575143610 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> \t\tAgcConstraint::Bound bound = static_cast(idx);\n> \t\tdouble qLo = content[\"qLo\"].get().value_or(0.98);\n> \t\tdouble qHi = content[\"qHi\"].get().value_or(1.0);\n> -\t\tdouble yTarget =\n> -\t\t\tcontent[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n> +\t\tPwl yTarget;\n> +\t\tint ret = yTarget.readYaml(content[\"yTarget\"]);\n> +\t\tif (ret < 0)\n> +\t\t\tyTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) });\n> \n> \t\tAgcConstraint constraint = { bound, qLo, qHi, yTarget };\n> \n> @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> \t\t\tAgcConstraint::Bound::lower,\n> \t\t\t0.98,\n> \t\t\t1.0,\n> -\t\t\t0.5\n> +\t\t\tPwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) })\n> \t\t};\n> \n> \t\tconstraintModes_[controls::ConstraintNormal].insert(\n> @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const\n> * \\param[in] constraintModeIndex The index of the constraint to adhere to\n> * \\param[in] hist A histogram over which to calculate inter-quantile means\n> * \\param[in] gain The gain to clamp\n> + * \\param[in] lux The lux value at which to sample the constraint luminance target pwl\n> *\n> * \\return The gain clamped within the constraint bounds\n> */\n> double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> \t\t\t\t\t const Histogram &hist,\n> -\t\t\t\t\t double gain)\n> +\t\t\t\t\t double gain, double lux)\n> {\n> \tstd::vector &constraints = constraintModes_[constraintModeIndex];\n> \tfor (const AgcConstraint &constraint : constraints) {\n> -\t\tdouble newGain = constraint.yTarget * hist.bins() /\n> +\t\tdouble newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() /\n> \t\t\t\t hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> \n> \t\tif (constraint.bound == AgcConstraint::Bound::lower &&\n> @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> \t\texposureModeHelpers_.at(exposureModeIndex);\n> \n> \tdouble gain = estimateInitialGain(lux);\n> -\tgain = constraintClampGain(constraintModeIndex, yHist, gain);\n> +\tgain = constraintClampGain(constraintModeIndex, yHist, gain, lux);\n> \n> \t/*\n> \t * We don't check whether we're already close to the target, because\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index 6ec2a0dc9..d43f673dd 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -38,7 +38,7 @@ public:\n> \t\tBound bound;\n> \t\tdouble qLo;\n> \t\tdouble qHi;\n> -\t\tdouble yTarget;\n> +\t\tPwl yTarget;\n> \t};\n> \n> \tint parseTuningData(const YamlObject &tuningData);\n> @@ -80,7 +80,7 @@ private:\n> \tdouble estimateInitialGain(double lux) const;\n> \tdouble constraintClampGain(uint32_t constraintModeIndex,\n> \t\t\t\t const Histogram &hist,\n> -\t\t\t\t double gain);\n> +\t\t\t\t double gain, double lux);\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> \n> \tuint64_t frameCount_;","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 E9350C3237\n\tfor ;\n\tFri, 14 Jun 2024 08:55:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6AF56548D;\n\tFri, 14 Jun 2024 10:55:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6F1865456\n\tfor ;\n\tFri, 14 Jun 2024 10:55:50 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0FAF5183\n\tfor ;\n\tFri, 14 Jun 2024 10:55:36 +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=\"MUBI72vN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718355336;\n\tbh=GABO7JpDqahqt5eGeEalFTdsxzNN8cTZ1rKpFaU3TvY=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=MUBI72vNJicaROj8EF1nzRl29xoejSUmV8g5hWbzskzNSQBj3dEyYA2DMnAKiMV2c\n\tvauwNjSmx1VCVnrqQ/W0M5O7w6jqnpolGgPEStg5sUdbD71aUEkSNfJ/yaoHnvOxRm\n\tPQ1OjrTMFo6aNR8qx3C4gq9jQ15ldNGbzFGIayT0=","Message-ID":"","Date":"Fri, 14 Jun 2024 09:55:47 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3] ipa: libipa: Change constraint Y target to piecewise\n\tlinear function","To":"libcamera-devel@lists.libcamera.org","References":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240607075914.2655019-1-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]