[{"id":32590,"web_url":"https://patchwork.libcamera.org/comment/32590/","msgid":"<173349783713.312267.1150210435925103159@ping.linuxembedded.co.uk>","date":"2024-12-06T15:10:37","subject":"Re: [PATCH v5 3/8] 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-12-06 14:52:23)\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> Reviewed-by: Paul Elder \n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 35 ++++++++++++++++++++++++-------\n> 1 file changed, 28 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 23a81e75d3d3..41f260e7089c 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -33,6 +33,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,8 +48,13 @@ Awb::Awb()\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::init\n> */\n> -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\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> Interpolator> gains;\n> int ret = gains.readYaml(tuningData[\"gains\"], \"ct\", \"gains\");\n> if (ret < 0)\n> @@ -101,19 +110,31 @@ void Awb::queueRequest(IPAContext &context,\n> << (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n> }\n> \n> + frameContext.awb.autoEnabled = awb.autoEnabled;\n> +\n> const auto &colourGains = controls.get(controls::ColourGains);\n> - if (colourGains && !awb.autoEnabled) {\n> + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n\nWhy do you get the colourTemperature here\n\n> +\n> + if (awb.autoEnabled)\n\nAnd then potentially return without using it here ?\n\n> + return;\n\nPerhaps we should only get it here ?\n\n> +\n> + bool update = false;\n> + if (colourGains) {\n> awb.gains.manual.r() = (*colourGains)[0];\n> awb.gains.manual.b() = (*colourGains)[1];\n> + update = true;\n> + } else if (colourTemperature && gains_) {\n> + auto gains = gains_->getInterpolated(*colourTemperature);\n> + awb.gains.manual.r() = gains[0];\n> + awb.gains.manual.b() = gains[1];\n> + update = true;\n> + }\n\nI'm getting more convinced that queueRequest for each algorithm that\nuses libipa should be using a common path... This would need to be\nimplemented in the same way for Mali, IPU3, RKISP1 ... (and Pi\nseparately)\n\n(I had the same comment earlier on AGC for Mali ... but I don't think it\nblocks getting these merged - but I wouldn't want to see all the libipa\nimplementations having a different response to control handling in the\nend).\n\nWith the adjustment for when you read the\ncontrols.get(controls::ColourTemperature); to be after the\nawb.autoEnabled()\n\n\nReviewed-by: Kieran Bingham \n\n\n> \n> + if (update)\n> LOG(RkISP1Awb, Debug)\n> << \"Set colour gains to \" << awb.gains.manual;\n> - }\n> -\n> - frameContext.awb.autoEnabled = awb.autoEnabled;\n> \n> - if (!awb.autoEnabled)\n> - frameContext.awb.gains = awb.gains.manual;\n> + frameContext.awb.gains = awb.gains.manual;\n> }\n> \n> /**\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 43528BF415\n\tfor ;\n\tFri, 6 Dec 2024 15:10:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 83AE266176;\n\tFri, 6 Dec 2024 16:10:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 346436615F\n\tfor ;\n\tFri, 6 Dec 2024 16:10:40 +0100 (CET)","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 68D4274C;\n\tFri, 6 Dec 2024 16:10:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"L2j1GtuW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733497810;\n\tbh=VJeChYKnRQ3aeoaJJ6P23uZbVe8Ip1PauvvBfY6u0rM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=L2j1GtuWV6wlbjcR/URqgbs5KbyscScYpp0wEJ3NlyYeorDvelsaZpuZ0HakuoIM9\n\tojMU6KMWRPv/Nji7JBLnb0txGLGYaN1FUdQBVV/0yYk8NmGGYtOOUgnnHeN7giqM+1\n\tH14XG1Oi3+JegoxdHVZoLEsTBdUXLQNcxj4nkiiQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241206145242.827886-4-stefan.klug@ideasonboard.com>","References":"<20241206145242.827886-1-stefan.klug@ideasonboard.com>\n\t<20241206145242.827886-4-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v5 3/8] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","From":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tPaul Elder ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 06 Dec 2024 15:10:37 +0000","Message-ID":"<173349783713.312267.1150210435925103159@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":32615,"web_url":"https://patchwork.libcamera.org/comment/32615/","msgid":"<20241209020233.GM17570@pendragon.ideasonboard.com>","date":"2024-12-09T02:02:33","subject":"Re: [PATCH v5 3/8] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Dec 06, 2024 at 03:10:37PM +0000, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-12-06 14:52:23)\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\nThis last sentence may need to be revisited based on the outcome of the\ndiscussion on the control definition.\n\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\ns/the ones/the CT curve/ if you agree with my comment on 2/8.\n\n> > \n> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Paul Elder \n> > ---\n> > src/ipa/rkisp1/algorithms/awb.cpp | 35 ++++++++++++++++++++++++-------\n> > 1 file changed, 28 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index 23a81e75d3d3..41f260e7089c 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -33,6 +33,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,8 +48,13 @@ Awb::Awb()\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::init\n> > */\n> > -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\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> > Interpolator> gains;\n> > int ret = gains.readYaml(tuningData[\"gains\"], \"ct\", \"gains\");\n> > if (ret < 0)\n> > @@ -101,19 +110,31 @@ void Awb::queueRequest(IPAContext &context,\n> > << (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n> > }\n> > \n> > + frameContext.awb.autoEnabled = awb.autoEnabled;\n> > +\n> > const auto &colourGains = controls.get(controls::ColourGains);\n> > - if (colourGains && !awb.autoEnabled) {\n> > + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> \n> Why do you get the colourTemperature here\n> \n> > +\n> > + if (awb.autoEnabled)\n> \n> And then potentially return without using it here ?\n> \n> > + return;\n> \n> Perhaps we should only get it here ?\n\nRetrieval of colourGains should also be moved here.\n\n> \n> > +\n> > + bool update = false;\n> > + if (colourGains) {\n> > awb.gains.manual.r() = (*colourGains)[0];\n> > awb.gains.manual.b() = (*colourGains)[1];\n> > + update = true;\n> > + } else if (colourTemperature && gains_) {\n\nIf we have no CT curve, should we expose the ColourTemperature control\nas output only ? And now that I'm writing that, I don't think we have a\nway to indicate if a control is inout or out only in a dynamic way,\nPaul's series only handles the control definition.\n\nWill we need to extend's Paul's series to differentiate \"optionally\ninput\" vs. \"mandatory input\", and also add the direction to ControlInfo\nin addition to ControlId ?\n\n> > + auto gains = gains_->getInterpolated(*colourTemperature);\n\nconst auto &gains\n\n> > + awb.gains.manual.r() = gains[0];\n> > + awb.gains.manual.b() = gains[1];\n> > + update = true;\n> > + }\n> \n> I'm getting more convinced that queueRequest for each algorithm that\n> uses libipa should be using a common path... This would need to be\n> implemented in the same way for Mali, IPU3, RKISP1 ... (and Pi\n> separately)\n> \n> (I had the same comment earlier on AGC for Mali ... but I don't think it\n> blocks getting these merged - but I wouldn't want to see all the libipa\n> implementations having a different response to control handling in the\n> end).\n> \n> With the adjustment for when you read the\n> controls.get(controls::ColourTemperature); to be after the\n> awb.autoEnabled()\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> > \n> > + if (update)\n> > LOG(RkISP1Awb, Debug)\n> > << \"Set colour gains to \" << awb.gains.manual;\n> > - }\n> > -\n> > - frameContext.awb.autoEnabled = awb.autoEnabled;\n> > \n> > - if (!awb.autoEnabled)\n> > - frameContext.awb.gains = awb.gains.manual;\n> > + frameContext.awb.gains = awb.gains.manual;\n> > }\n> > \n> > /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 47043BD80A\n\tfor ;\n\tMon, 9 Dec 2024 02:02:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1EA2467E49;\n\tMon, 9 Dec 2024 03:02:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07D5267E41\n\tfor ;\n\tMon, 9 Dec 2024 03:02:48 +0100 (CET)","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 7616C502;\n\tMon, 9 Dec 2024 03:02:17 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HNViw/er\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733709737;\n\tbh=tAsBgIkuCbx3UX6T3iNdLGdgAO8w7l//k/hu7ZwWSk0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HNViw/er97ZOIlm9LHJ+8WG3AKlnZOcrL32/ZgsYWsluSdvaeO8K2209ex6eGUTjf\n\tRndm44b6eyMHkWniaj2pXKaHwqAa4WLNx4Iuw8rPsFJCCcqldF2NoEM5o1TVgXHQ8b\n\tpvHoY/4aRWZ3/MeQ6ZbtR6hu1m/h/zIAJfem7qOY=","Date":"Mon, 9 Dec 2024 04:02:33 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v5 3/8] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","Message-ID":"<20241209020233.GM17570@pendragon.ideasonboard.com>","References":"<20241206145242.827886-1-stefan.klug@ideasonboard.com>\n\t<20241206145242.827886-4-stefan.klug@ideasonboard.com>\n\t<173349783713.312267.1150210435925103159@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173349783713.312267.1150210435925103159@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\" "}}]