[{"id":30592,"web_url":"https://patchwork.libcamera.org/comment/30592/","msgid":"<172286556043.1687952.43089816506639556@ping.linuxembedded.co.uk>","date":"2024-08-05T13:46:00","subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-08-05 13:05:04)\n> There are many use-cases (tuning-validation, working in static\n> environments) where a manual ColourTemperature control is helpful.\n> Implement that by interpolating and applying the white balance gains\n> from the tuning file according to the requested colour temperature. If\n> colour gains are provided on the same request, they take precedence. As\n> the colour temperature reported in the metadata is always based on the\n> measurements, we don't have to touch that.\n> \n> Note that in the automatic case, the colour gains are still based on the\n> gray world model and the ones from the tuning file get ignored.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 957d24fe3425..d482eda5b541 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms {\n> \n> LOG_DEFINE_CATEGORY(RkISP1Awb)\n> \n> +constexpr int32_t kMinColourTemperature = 2500;\n> +constexpr int32_t kMaxColourTemperature = 10000;\n> +constexpr int32_t kDefaultColourTemperature = 6500;\n> +\n> /* Minimum mean value below which AWB can't operate. */\n> constexpr double kMeanMinThreshold = 2.0;\n> \n> @@ -44,6 +48,11 @@ Awb::Awb()\n> */\n> int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> {\n> + auto &cmap = context.ctrlMap;\n> + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,\n> + kMaxColourTemperature,\n> + kDefaultColourTemperature);\n> +\n\nShould this control only be exposed if there are gains available in the\ntuning file?\n\nCan/Should the minColourTemperature/maxColourTemperature be based on the\nvalues that are available in the Tuning file? Or does the interpolator\nmake this possible to extend beyond the limits calibrated if it can\nextrapolate for wider values?\n\n\n\n> MatrixInterpolator gains;\n> int ret = gains.readYaml(tuningData[\"gains\"], \"ct\", \"gains\");\n> if (ret < 0)\n> @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context,\n> << \", blue: \" << awb.gains.manual.blue;\n> }\n> \n> + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) {\n> + Matrix gains = gains_->get(*colourTemperature);\n> + awb.gains.manual.red = gains[0][0];\n> + awb.gains.manual.blue = gains[1][0];\n> +\n> + LOG(RkISP1Awb, Debug)\n> + << \"Set colour gains to red: \" << awb.gains.manual.red\n> + << \", blue: \" << awb.gains.manual.blue;\n> + }\n> +\n> frameContext.awb.autoEnabled = awb.autoEnabled;\n> \n> if (!awb.autoEnabled) {\n> -- \n> 2.43.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 0F626C323E\n\tfor ;\n\tMon, 5 Aug 2024 13:46:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A78363382;\n\tMon, 5 Aug 2024 15:46:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89E5063369\n\tfor ;\n\tMon, 5 Aug 2024 15:46:03 +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 071E818D;\n\tMon, 5 Aug 2024 15:45:11 +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=\"gudkraiv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722865512;\n\tbh=fpBBynt8IMTgyQfNmIzy1y9vrkGsagXuRmgafLk94cI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=gudkraivNxQNqLxNerZzLiN/w5nh7PoL40Yl99BlR5SBXWobU29DzQn7bDRbWPDQl\n\tJz6Zq10kurGfZcod3rO6t0ahbp/lA0anz2SSPDxAT0rO7JfJSSU3GPOkzsN308Soqq\n\tu3HN7slLjYQgcwUbzIX873lboyC9VS7XaN4kIy/A=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240805120522.1613342-4-stefan.klug@ideasonboard.com>","References":"<20240805120522.1613342-1-stefan.klug@ideasonboard.com>\n\t<20240805120522.1613342-4-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","From":"Kieran Bingham ","Cc":"David Plowman ,\n\tStefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 05 Aug 2024 14:46:00 +0100","Message-ID":"<172286556043.1687952.43089816506639556@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":30607,"web_url":"https://patchwork.libcamera.org/comment/30607/","msgid":"","date":"2024-08-06T06:37:54","subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nThanks for the review.\n\nOn Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-08-05 13:05:04)\n> > There are many use-cases (tuning-validation, working in static\n> > environments) where a manual ColourTemperature control is helpful.\n> > Implement that by interpolating and applying the white balance gains\n> > from the tuning file according to the requested colour temperature. If\n> > colour gains are provided on the same request, they take precedence. As\n> > the colour temperature reported in the metadata is always based on the\n> > measurements, we don't have to touch that.\n> > \n> > Note that in the automatic case, the colour gains are still based on the\n> > gray world model and the ones from the tuning file get ignored.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++\n> > 1 file changed, 20 insertions(+)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index 957d24fe3425..d482eda5b541 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms {\n> > \n> > LOG_DEFINE_CATEGORY(RkISP1Awb)\n> > \n> > +constexpr int32_t kMinColourTemperature = 2500;\n> > +constexpr int32_t kMaxColourTemperature = 10000;\n> > +constexpr int32_t kDefaultColourTemperature = 6500;\n> > +\n> > /* Minimum mean value below which AWB can't operate. */\n> > constexpr double kMeanMinThreshold = 2.0;\n> > \n> > @@ -44,6 +48,11 @@ Awb::Awb()\n> > */\n> > int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> > {\n> > + auto &cmap = context.ctrlMap;\n> > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,\n> > + kMaxColourTemperature,\n> > + kDefaultColourTemperature);\n> > +\n> \n> Should this control only be exposed if there are gains available in the\n> tuning file?\n\nAs noted in the previous mail I think unconditional is ok as it also\napplies to the ccms which should be available.\n\n> \n> Can/Should the minColourTemperature/maxColourTemperature be based on the\n> values that are available in the Tuning file? Or does the interpolator\n> make this possible to extend beyond the limits calibrated if it can\n> extrapolate for wider values?\n\nThe interpolator extends the values if the temperature out of range.\nHere we would also need to check the range of ccms and gains. I don't\nknow if it's worth the effort. On the other hand the min/max values are\narbitrary. A quick google indicates that we could even lower the min to\n1000.\n\n> \n> \n> \n> > MatrixInterpolator gains;\n> > int ret = gains.readYaml(tuningData[\"gains\"], \"ct\", \"gains\");\n> > if (ret < 0)\n> > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context,\n> > << \", blue: \" << awb.gains.manual.blue;\n> > }\n> > \n> > + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) {\n> > + Matrix gains = gains_->get(*colourTemperature);\n> > + awb.gains.manual.red = gains[0][0];\n> > + awb.gains.manual.blue = gains[1][0];\n> > +\n> > + LOG(RkISP1Awb, Debug)\n> > + << \"Set colour gains to red: \" << awb.gains.manual.red\n> > + << \", blue: \" << awb.gains.manual.blue;\n> > + }\n> > +\n> > frameContext.awb.autoEnabled = awb.autoEnabled;\n> > \n> > if (!awb.autoEnabled) {\n> > -- \n> > 2.43.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 F0D0BC323E\n\tfor ;\n\tTue, 6 Aug 2024 06:37:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9E9363394;\n\tTue, 6 Aug 2024 08:37:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F320863369\n\tfor ;\n\tTue, 6 Aug 2024 08:37:57 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:7a68:2b6f:8265:aa72])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F0B892C5;\n\tTue, 6 Aug 2024 08:37:05 +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=\"s4LNd2SC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722926226;\n\tbh=LPVKbma6+PkkXhToRzyMpUDbR+sDFH/5C5KSAhP0ZLE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s4LNd2SCWdSAVwf9p2XnEW4FFB45s7ZJs2RxncXs9y4rvE1qakc+qvVITNwlekqp9\n\tezsPvsv9FtxO+bQ8ClU9IbWz/8QlXUJat+lJq5Y/D1mRvKyCc/V7Myy510ePsXTERq\n\tP6RABnVloUpb9qGsBvQ1+w+YvvV/vrytEOYaiye0=","Date":"Tue, 6 Aug 2024 08:37:54 +0200","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDavid Plowman ","Subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","Message-ID":"","References":"<20240805120522.1613342-1-stefan.klug@ideasonboard.com>\n\t<20240805120522.1613342-4-stefan.klug@ideasonboard.com>\n\t<172286556043.1687952.43089816506639556@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172286556043.1687952.43089816506639556@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":30619,"web_url":"https://patchwork.libcamera.org/comment/30619/","msgid":"<172293448173.1687952.13530232797230268069@ping.linuxembedded.co.uk>","date":"2024-08-06T08:54:41","subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-08-06 07:37:54)\n> Hi Kieran,\n> \n> Thanks for the review.\n> \n> On Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-08-05 13:05:04)\n> > > There are many use-cases (tuning-validation, working in static\n> > > environments) where a manual ColourTemperature control is helpful.\n> > > Implement that by interpolating and applying the white balance gains\n> > > from the tuning file according to the requested colour temperature. If\n> > > colour gains are provided on the same request, they take precedence. As\n> > > the colour temperature reported in the metadata is always based on the\n> > > measurements, we don't have to touch that.\n> > > \n> > > Note that in the automatic case, the colour gains are still based on the\n> > > gray world model and the ones from the tuning file get ignored.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++\n> > > 1 file changed, 20 insertions(+)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > index 957d24fe3425..d482eda5b541 100644\n> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms {\n> > > \n> > > LOG_DEFINE_CATEGORY(RkISP1Awb)\n> > > \n> > > +constexpr int32_t kMinColourTemperature = 2500;\n> > > +constexpr int32_t kMaxColourTemperature = 10000;\n> > > +constexpr int32_t kDefaultColourTemperature = 6500;\n> > > +\n> > > /* Minimum mean value below which AWB can't operate. */\n> > > constexpr double kMeanMinThreshold = 2.0;\n> > > \n> > > @@ -44,6 +48,11 @@ Awb::Awb()\n> > > */\n> > > int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> > > {\n> > > + auto &cmap = context.ctrlMap;\n> > > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,\n> > > + kMaxColourTemperature,\n> > > + kDefaultColourTemperature);\n> > > +\n> > \n> > Should this control only be exposed if there are gains available in the\n> > tuning file?\n> \n> As noted in the previous mail I think unconditional is ok as it also\n> applies to the ccms which should be available.\n\nSo - perhaps we should be instantiating this control at the top level\nand not inside the awb. If someone disables the awb module\nin the tuning file, it would impact CCM ?\n\n\n> > Can/Should the minColourTemperature/maxColourTemperature be based on the\n> > values that are available in the Tuning file? Or does the interpolator\n> > make this possible to extend beyond the limits calibrated if it can\n> > extrapolate for wider values?\n> \n> The interpolator extends the values if the temperature out of range.\n> Here we would also need to check the range of ccms and gains. I don't\n> know if it's worth the effort. On the other hand the min/max values are\n> arbitrary. A quick google indicates that we could even lower the min to\n> 1000.\n> \n> > > MatrixInterpolator gains;\n> > > int ret = gains.readYaml(tuningData[\"gains\"], \"ct\", \"gains\");\n> > > if (ret < 0)\n> > > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context,\n> > > << \", blue: \" << awb.gains.manual.blue;\n> > > }\n> > > \n> > > + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> > > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) {\n> > > + Matrix gains = gains_->get(*colourTemperature);\n> > > + awb.gains.manual.red = gains[0][0];\n> > > + awb.gains.manual.blue = gains[1][0];\n> > > +\n> > > + LOG(RkISP1Awb, Debug)\n> > > + << \"Set colour gains to red: \" << awb.gains.manual.red\n> > > + << \", blue: \" << awb.gains.manual.blue;\n> > > + }\n> > > +\n> > > frameContext.awb.autoEnabled = awb.autoEnabled;\n> > > \n> > > if (!awb.autoEnabled) {\n> > > -- \n> > > 2.43.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 C4E1CC323E\n\tfor ;\n\tTue, 6 Aug 2024 08:54:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB3F763398;\n\tTue, 6 Aug 2024 10:54:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3804F61955\n\tfor ;\n\tTue, 6 Aug 2024 10:54:45 +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 173E44CD;\n\tTue, 6 Aug 2024 10:53:53 +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=\"P/CO6KUj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722934433;\n\tbh=Ofn6DHtmRfuCuvZe//Ax93T71o6AXLULZDY7RbRBgDs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=P/CO6KUj+wsOvLJMeF4dg/OpBj9qijnmVt3U6USCw+C46UUOOHJ8zS95ALt5vkZ1t\n\teckskxz+4Hc/CIPOwvkrk5gZPqcPgRMVJEct+WnVwDUi7B+9qXRz8Mk5o7A8x/53oW\n\t4T4KzMPscoW1xKZ4GWzj6MTxQCBbfDtPXc2FfwME=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240805120522.1613342-1-stefan.klug@ideasonboard.com>\n\t<20240805120522.1613342-4-stefan.klug@ideasonboard.com>\n\t<172286556043.1687952.43089816506639556@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tDavid Plowman ","To":"Stefan Klug ","Date":"Tue, 06 Aug 2024 09:54:41 +0100","Message-ID":"<172293448173.1687952.13530232797230268069@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":30770,"web_url":"https://patchwork.libcamera.org/comment/30770/","msgid":"<5qsitqfs4fvn5i325ugnsspyqlibs4qimwyxcct2aqoi73zrx5@z6e6s4ink2eu>","date":"2024-08-13T07:36:58","subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Aug 06, 2024 at 09:54:41AM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-08-06 07:37:54)\n> > Hi Kieran,\n> > \n> > Thanks for the review.\n> > \n> > On Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote:\n> > > Quoting Stefan Klug (2024-08-05 13:05:04)\n> > > > There are many use-cases (tuning-validation, working in static\n> > > > environments) where a manual ColourTemperature control is helpful.\n> > > > Implement that by interpolating and applying the white balance gains\n> > > > from the tuning file according to the requested colour temperature. If\n> > > > colour gains are provided on the same request, they take precedence. As\n> > > > the colour temperature reported in the metadata is always based on the\n> > > > measurements, we don't have to touch that.\n> > > > \n> > > > Note that in the automatic case, the colour gains are still based on the\n> > > > gray world model and the ones from the tuning file get ignored.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++\n> > > > 1 file changed, 20 insertions(+)\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > index 957d24fe3425..d482eda5b541 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms {\n> > > > \n> > > > LOG_DEFINE_CATEGORY(RkISP1Awb)\n> > > > \n> > > > +constexpr int32_t kMinColourTemperature = 2500;\n> > > > +constexpr int32_t kMaxColourTemperature = 10000;\n> > > > +constexpr int32_t kDefaultColourTemperature = 6500;\n> > > > +\n> > > > /* Minimum mean value below which AWB can't operate. */\n> > > > constexpr double kMeanMinThreshold = 2.0;\n> > > > \n> > > > @@ -44,6 +48,11 @@ Awb::Awb()\n> > > > */\n> > > > int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> > > > {\n> > > > + auto &cmap = context.ctrlMap;\n> > > > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,\n> > > > + kMaxColourTemperature,\n> > > > + kDefaultColourTemperature);\n> > > > +\n> > > \n> > > Should this control only be exposed if there are gains available in the\n> > > tuning file?\n> > \n> > As noted in the previous mail I think unconditional is ok as it also\n> > applies to the ccms which should be available.\n> \n> So - perhaps we should be instantiating this control at the top level\n> and not inside the awb. If someone disables the awb module\n> in the tuning file, it would impact CCM ?\n>\n\nI thought about that a bit. In automatic mode, the colour temperature\ngets estimated in the awb module. So we would also need to move the\nAwbEnable handling to the top level. So code wise, the colour\ntemperature would be set from two different places and the Awb handling\nwould also be spread across two places. I don't think that's worth it.\n\nCheers,\nStefan\n\n> \n> > > Can/Should the minColourTemperature/maxColourTemperature be based on the\n> > > values that are available in the Tuning file? Or does the interpolator\n> > > make this possible to extend beyond the limits calibrated if it can\n> > > extrapolate for wider values?\n> > \n> > The interpolator extends the values if the temperature out of range.\n> > Here we would also need to check the range of ccms and gains. I don't\n> > know if it's worth the effort. On the other hand the min/max values are\n> > arbitrary. A quick google indicates that we could even lower the min to\n> > 1000.\n> > \n> > > > MatrixInterpolator gains;\n> > > > int ret = gains.readYaml(tuningData[\"gains\"], \"ct\", \"gains\");\n> > > > if (ret < 0)\n> > > > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context,\n> > > > << \", blue: \" << awb.gains.manual.blue;\n> > > > }\n> > > > \n> > > > + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> > > > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) {\n> > > > + Matrix gains = gains_->get(*colourTemperature);\n> > > > + awb.gains.manual.red = gains[0][0];\n> > > > + awb.gains.manual.blue = gains[1][0];\n> > > > +\n> > > > + LOG(RkISP1Awb, Debug)\n> > > > + << \"Set colour gains to red: \" << awb.gains.manual.red\n> > > > + << \", blue: \" << awb.gains.manual.blue;\n> > > > + }\n> > > > +\n> > > > frameContext.awb.autoEnabled = awb.autoEnabled;\n> > > > \n> > > > if (!awb.autoEnabled) {\n> > > > -- \n> > > > 2.43.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 5DFCAC323E\n\tfor ;\n\tTue, 13 Aug 2024 07:37:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 57F0E633B5;\n\tTue, 13 Aug 2024 09:37:03 +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 ECD576337E\n\tfor ;\n\tTue, 13 Aug 2024 09:37:00 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:7025:8d00:1ffd:751a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DD29A827;\n\tTue, 13 Aug 2024 09:36:03 +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=\"RR7lBVs2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1723534563;\n\tbh=oK5DkaK5PzpagmWpGeCqEWiBC/Mhc6B7+3eYPVD9VSc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RR7lBVs29dJNpnr+WWfk+94xdnsiNm1yB4sw6QmrRsggRdUMUJ/NNCk2UxVFUFq2O\n\tLiCwUADxiTOYqFJiwarASScFpfKAVGk3W6H2KRg99kLmeg/qaz651eBBEcI6M0cZ3O\n\tsS7LOZDseKUsVFXmuLRHz2LjH8lV9SxpK63HhSrQ=","Date":"Tue, 13 Aug 2024 09:36:58 +0200","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDavid Plowman ","Subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","Message-ID":"<5qsitqfs4fvn5i325ugnsspyqlibs4qimwyxcct2aqoi73zrx5@z6e6s4ink2eu>","References":"<20240805120522.1613342-1-stefan.klug@ideasonboard.com>\n\t<20240805120522.1613342-4-stefan.klug@ideasonboard.com>\n\t<172286556043.1687952.43089816506639556@ping.linuxembedded.co.uk>\n\t\n\t<172293448173.1687952.13530232797230268069@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172293448173.1687952.13530232797230268069@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":30771,"web_url":"https://patchwork.libcamera.org/comment/30771/","msgid":"","date":"2024-08-13T07:59:06","subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Stefan\n\nThanks for resurrecting this topic. I seem to remember submitting\nsomething like this a looong time ago. Don't recall what, if anything,\never happened to it. But indeed, I think it's useful, so glad to see\nit's not been forgotten!\n\nOn Tue, 13 Aug 2024 at 08:37, Stefan Klug wrote:\n>\n> Hi Kieran,\n>\n> On Tue, Aug 06, 2024 at 09:54:41AM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-08-06 07:37:54)\n> > > Hi Kieran,\n> > >\n> > > Thanks for the review.\n> > >\n> > > On Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote:\n> > > > Quoting Stefan Klug (2024-08-05 13:05:04)\n> > > > > There are many use-cases (tuning-validation, working in static\n> > > > > environments) where a manual ColourTemperature control is helpful.\n> > > > > Implement that by interpolating and applying the white balance gains\n> > > > > from the tuning file according to the requested colour temperature. If\n> > > > > colour gains are provided on the same request, they take precedence. As\n> > > > > the colour temperature reported in the metadata is always based on the\n> > > > > measurements, we don't have to touch that.\n> > > > >\n> > > > > Note that in the automatic case, the colour gains are still based on the\n> > > > > gray world model and the ones from the tuning file get ignored.\n> > > > >\n> > > > > Signed-off-by: Stefan Klug \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++\n> > > > > 1 file changed, 20 insertions(+)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > > index 957d24fe3425..d482eda5b541 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms {\n> > > > >\n> > > > > LOG_DEFINE_CATEGORY(RkISP1Awb)\n> > > > >\n> > > > > +constexpr int32_t kMinColourTemperature = 2500;\n> > > > > +constexpr int32_t kMaxColourTemperature = 10000;\n> > > > > +constexpr int32_t kDefaultColourTemperature = 6500;\n> > > > > +\n> > > > > /* Minimum mean value below which AWB can't operate. */\n> > > > > constexpr double kMeanMinThreshold = 2.0;\n> > > > >\n> > > > > @@ -44,6 +48,11 @@ Awb::Awb()\n> > > > > */\n> > > > > int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> > > > > {\n> > > > > + auto &cmap = context.ctrlMap;\n> > > > > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,\n> > > > > + kMaxColourTemperature,\n> > > > > + kDefaultColourTemperature);\n> > > > > +\n> > > >\n> > > > Should this control only be exposed if there are gains available in the\n> > > > tuning file?\n> > >\n> > > As noted in the previous mail I think unconditional is ok as it also\n> > > applies to the ccms which should be available.\n> >\n> > So - perhaps we should be instantiating this control at the top level\n> > and not inside the awb. If someone disables the awb module\n> > in the tuning file, it would impact CCM ?\n> >\n>\n> I thought about that a bit. In automatic mode, the colour temperature\n> gets estimated in the awb module. So we would also need to move the\n> AwbEnable handling to the top level. So code wise, the colour\n> temperature would be set from two different places and the Awb handling\n> would also be spread across two places. I don't think that's worth it.\n\nI think it suits us (RPi) better like this too. It's also like our\nAEC/AGC, where fixed values are actually handled by the algorithm even\nwhen you're in manual mode.\n\nThanks\nDavid\n\n>\n> Cheers,\n> Stefan\n>\n> >\n> > > > Can/Should the minColourTemperature/maxColourTemperature be based on the\n> > > > values that are available in the Tuning file? Or does the interpolator\n> > > > make this possible to extend beyond the limits calibrated if it can\n> > > > extrapolate for wider values?\n> > >\n> > > The interpolator extends the values if the temperature out of range.\n> > > Here we would also need to check the range of ccms and gains. I don't\n> > > know if it's worth the effort. On the other hand the min/max values are\n> > > arbitrary. A quick google indicates that we could even lower the min to\n> > > 1000.\n> > >\n> > > > > MatrixInterpolator gains;\n> > > > > int ret = gains.readYaml(tuningData[\"gains\"], \"ct\", \"gains\");\n> > > > > if (ret < 0)\n> > > > > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context,\n> > > > > << \", blue: \" << awb.gains.manual.blue;\n> > > > > }\n> > > > >\n> > > > > + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> > > > > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) {\n> > > > > + Matrix gains = gains_->get(*colourTemperature);\n> > > > > + awb.gains.manual.red = gains[0][0];\n> > > > > + awb.gains.manual.blue = gains[1][0];\n> > > > > +\n> > > > > + LOG(RkISP1Awb, Debug)\n> > > > > + << \"Set colour gains to red: \" << awb.gains.manual.red\n> > > > > + << \", blue: \" << awb.gains.manual.blue;\n> > > > > + }\n> > > > > +\n> > > > > frameContext.awb.autoEnabled = awb.autoEnabled;\n> > > > >\n> > > > > if (!awb.autoEnabled) {\n> > > > > --\n> > > > > 2.43.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 CE6C2BDB13\n\tfor ;\n\tTue, 13 Aug 2024 07:59:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E896B633B5;\n\tTue, 13 Aug 2024 09:59:19 +0200 (CEST)","from mail-qk1-x736.google.com (mail-qk1-x736.google.com\n\t[IPv6:2607:f8b0:4864:20::736])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8420D6337E\n\tfor ;\n\tTue, 13 Aug 2024 09:59:18 +0200 (CEST)","by mail-qk1-x736.google.com with SMTP id\n\taf79cd13be357-7a1d7bc07b7so349242685a.0\n\tfor ;\n\tTue, 13 Aug 2024 00:59:18 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"KoqcKdGj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1723535957; x=1724140757;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=ea3F2NgALIOH1wMCWwvVAiU3cYuHVMBQefqISzPX2Kg=;\n\tb=KoqcKdGjHpUrKpIRSf4Hjk554xdEvbVp3uK/9FArOGYT0rUk8bF/DTN6w+shQ6X+xb\n\teBlRU3Xr232EdgRpQf4471sswqRSuFPhZ+kavqNCfIPT/J399hJR4TU2/iqQkDpYp08n\n\t4Ouovaf1jd4m2LFGKRpBK/31i6ZAqDgz6QktGJJG2d63LWykDNpY3xmcOCtzggdVms5s\n\tNSgWZbaZWbbrG9lGo4FirHQwIHVPgFOe+lVENqblnfZ1OtiSNhmaK1VsC2a7/9DBGeKF\n\t0AfrWiX4RkHFM8JcnYdx9F7Ao73ZW9qp2tzmKvnXhg1Yv05X0eg35E/gQ8bbC2mIN7g9\n\tqtXA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1723535957; x=1724140757;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=ea3F2NgALIOH1wMCWwvVAiU3cYuHVMBQefqISzPX2Kg=;\n\tb=TqDwvilMZNoCfmYsrErGNtG/Y2zpPs6iDsooQIg1DobOtfrdOIXpDNp2NYhm/oo1mt\n\tYoc9E4Y0+O8Ye6DfOq92SYWdACbfg2hL9WU7Y+TXS7MUyQufBPwaexRCdShlqRAVENmJ\n\tCHcSGVTWj//Ai0ZXGSN5CQphvhGHibSTdDPSuFLZ+32m8WcciGBGxu29h2kraqs/Ho8D\n\tX+FyRb57szBIHjqYr+qBMRs5iP7MSCDeauJtoAON8DIIyj11D9aR0UlpO+0TSr+FefLp\n\twbkrP/B8pwJKjUJUBfiJAAf6uRRT7td3d5J58l4tVmqMAHaHww++AVi5phRgULhaFnHX\n\tz1YQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWDnXNjg4t625BYxHwvodg/3bb0vrSqrdH3OKOcGn+DdVowL9M3c6lierrPj7JPt3vzw9oTpyXtHsms3kYlAHY=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxDxU9SzPt8RAYJflxxoe4OuStZBpPky1RdN+telJocFYG9VfBU\n\tXYu22L0YSBd+NtwjqvxGsztasYUqp7FUFOAUaP5ZsFkbxYL0Wg8ccmye94oqOpa7j+HBsZOIIPR\n\tnkDAeLzx4BT15vbsdPL1aTrWXwNqJ5HCLHPCGFjSgW1CU77iZ","X-Google-Smtp-Source":"AGHT+IG1qU3BEzEiqpSz88M217MbnQQa0DM17NT4phvgWRjfBNYnpBx7TBCOhvGhTeC5OGw1pXBQJWtDsq7Y5Qd/fAE=","X-Received":"by 2002:a05:620a:f13:b0:79f:17d9:d86b with SMTP id\n\taf79cd13be357-7a4e150427amr268722885a.12.1723535957261;\n\tTue, 13 Aug 2024 00:59:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20240805120522.1613342-1-stefan.klug@ideasonboard.com>\n\t<20240805120522.1613342-4-stefan.klug@ideasonboard.com>\n\t<172286556043.1687952.43089816506639556@ping.linuxembedded.co.uk>\n\t\n\t<172293448173.1687952.13530232797230268069@ping.linuxembedded.co.uk>\n\t<5qsitqfs4fvn5i325ugnsspyqlibs4qimwyxcct2aqoi73zrx5@z6e6s4ink2eu>","In-Reply-To":"<5qsitqfs4fvn5i325ugnsspyqlibs4qimwyxcct2aqoi73zrx5@z6e6s4ink2eu>","From":"David Plowman ","Date":"Tue, 13 Aug 2024 08:59:06 +0100","Message-ID":"","Subject":"Re: [PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","To":"Stefan Klug ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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\" "}}]