[{"id":29839,"web_url":"https://patchwork.libcamera.org/comment/29839/","msgid":"<20240611160818.GC10397@pendragon.ideasonboard.com>","date":"2024-06-11T16:08:18","subject":"Re: [PATCH v5 1/3] ipa: rkisp1: agc: Read histogram weights from\n\ttuning file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jun 07, 2024 at 05:03:28PM +0900, Paul Elder wrote:\n> Add support to the rkisp1 AGC to read histogram weights from the tuning\n> file. As controls for selecting the metering mode are not yet supported,\n> for now hardcode the matrix metering mode, which is the same as what the\n> AGC previously hardcoded.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Stefan Klug \n> Reviewed-by: Daniel Scally \n> \n> ---\n> No change in v5\n> \n> Changes in v4:\n> - add debug print when parsing the yaml file\n> \n> Changes in v3:\n> - support the new tuning file layout for interpolated matrices\n> - support both v10 and v12\n> \n> Changes in v2:\n> - add default metering mode if none are read successfully from the\n> tuning file\n> - compute the predivider instead of using a table\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 94 ++++++++++++++++++++++++++++++-\n> src/ipa/rkisp1/algorithms/agc.h | 6 ++\n> 2 files changed, 97 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 50e0690fe..3bbafd978 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -17,6 +17,8 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> #include \"libipa/histogram.h\"\n> \n> /**\n> @@ -36,6 +38,76 @@ namespace ipa::rkisp1::algorithms {\n> \n> LOG_DEFINE_CATEGORY(RkISP1Agc)\n> \n> +int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> +\t\t\t const char *prop)\n> +{\n> +\tconst YamlObject &yamlMeteringModes = tuningData[prop];\n\nI think this belongs to the caller.\n\n> +\tif (!yamlMeteringModes.isDictionary()) {\n> +\t\tLOG(RkISP1Agc, Error)\n> +\t\t\t<< \"'\" << prop << \"' parameter not found in tuning file\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tfor (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> +\t\tif (controls::AeMeteringModeNameValueMap.find(key) ==\n> +\t\t controls::AeMeteringModeNameValueMap.end()) {\n> +\t\t\tLOG(RkISP1Agc, Warning)\n> +\t\t\t\t<< \"Skipping unknown metering mode '\" << key << \"'\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tfor (const auto &[version, matrix] : value.asDict()) {\n\nWhat is the version ? It isn't used below except in a debugging message.\n\nMore generally, where do we document the format of the tuning data ? If\nthe answer is '\\todo' then could we have at least one example yaml file\nin the source tree that includes all tuning blocks ? This can be done on\ntop of this series, but please sooner than later.\n\n> +\t\t\tstd::vector weights =\n> +\t\t\t\tmatrix.getList().value_or(std::vector{});\n> +\t\t\tif (weights.size() != context.hw->numHistogramWeights)\n> +\t\t\t\tcontinue;\n\nI'd be very vocal about this, or even fail parsing completely.\n\n> +\n> +\t\t\tLOG(RkISP1Agc, Debug)\n> +\t\t\t\t<< \"Matched metering matrix mode \"\n> +\t\t\t\t<< key << \", version \" << version;\n> +\n> +\t\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n\nIf there are multiple \"versions\", you'll overwrite the metering modes.\nIs that really intended ?\n\n> +\t\t}\n> +\t}\n> +\n> +\tif (meteringModes_.empty()) {\n> +\t\tint32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> +\t\tstd::vector weights(context.hw->numHistogramWeights, 1);\n> +\n> +\t\tmeteringModes_[meteringModeId] = weights;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +uint8_t Agc::predivider(Size &size)\n\nAgc::computeHistogramPredivider()\n\n(or s/compute/calculate/)\n\n> +{\n> +\t/*\n> +\t * The maximum number of pixels that could potentially be in one bin is\n> +\t * if all the pixels of the image are in it, multiplied by 3 for the\n\nI think you should pass the histogram mode to this function, to pick the\nright multiplier. We currently hardcode usage of the Y histogram, so the\n*3 factor below will result in higher predividers and loss of accuracy.\n\nI think we should also take the weights into account.\n\n> +\t * three color channels. The counter for each bin is 16 bits wide, so\n> +\t * `factor` thus contains the number of times we'd wrap around. This is\n> +\t * obviously the number of pixels that we need to skip to make sure\n> +\t * that we don't wrap around, but we compute the square root of it\n> +\t * instead, as the skip that we need to program is for both the x and y\n> +\t * directions.\n> +\t *\n> +\t * There's a bit of extra rounding math to make sure the rounding goes\n> +\t * the correct direction so that the square of the step is big enough\n> +\t * to encompass the `factor` number of pixels that we need to skip.\n> +\t */\n> +\tdouble factor = size.width * size.height * 3 / 65535.0;\n> +\tdouble root = std::sqrt(factor);\n> +\tuint8_t ret;\n\ns/ret/predivider/\n\n> +\n> +\tif (std::pow(std::floor(root), 2) + 0.01 < factor)\n> +\t\tret = static_cast(std::ceil(root));\n> +\telse\n> +\t\tret = static_cast(std::floor(root));\n\nI have trouble validating the math here. Let's assume we have an image\nsize of 760x460 pixels. This leads to\n\n\tfactor = 760 * 460* 3 / 65535.0 = 16.003662165255207\n\n(I'm using Python to do the math, I don't think the differences in\nrounding with C++ double, if any, will make a difference here)\n\n\troot = 4.000457744465652\n\tstd::pow(std::floor(root), 2) + 0.01 = 16.01\n\nThis is higher than factor, so\n\n\tpredivider = static_cast(std::floor(root)) = 4\n\nSkipping by a factor of 4 in both directions gives us 190x115 pixels,\nand multiplied by 3, that's 65550, which overflows. It may not be the\nbiggest deal, as the hardware clamps the histogram values instead of\nrolling over, and the risk of *all* pixels being in the same bin is\nvirtually 0, but it's still not correct.\n\n> +\n> +\treturn std::clamp(ret, 3, 127);\n> +}\n> +\n> Agc::Agc()\n> {\n> \tsupportsRaw_ = true;\n> @@ -59,6 +131,10 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tret = parseMeteringModes(context, tuningData, \"AeMeteringMode\");\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tcontext.ctrlMap.merge(controls());\n> \n> \treturn 0;\n> @@ -160,6 +236,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> \t}\n> \n> +\t/* \\todo Remove this when we can set the below with controls */\n\nPatch 3/3 enables setting the metering mode through controls, but\ndoesn't remove this. Is that intended ? Note that we should reprogram\nthe histogram engine only in the case where the metering mode has\nactually changed, we shouldn't do it on every frame.\n\n> \tif (frame > 0)\n> \t\treturn;\n> \n> @@ -178,14 +255,25 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \tparams->meas.hst_config.meas_window = context.configuration.agc.measureWindow;\n> \t/* Produce the luminance histogram. */\n> \tparams->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;\n> +\n> \t/* Set an average weighted histogram. */\n> \tSpan weights{\n> \t\tparams->meas.hst_config.hist_weight,\n> \t\tcontext.hw->numHistogramWeights\n> \t};\n> -\tstd::fill(weights.begin(), weights.end(), 1);\n> -\t/* Step size can't be less than 3. */\n> -\tparams->meas.hst_config.histogram_predivider = 4;\n> +\t/* \\todo Get this from control */\n> +\tstd::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> +\tstd::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> +\n> +\tstd::stringstream str;\n> +\tstr << \"Histogram weights : \";\n> +\tfor (size_t i = 0; i < context.hw->numHistogramWeights; i++)\n> +\t\tstr << (int)params->meas.hst_config.hist_weight[i] << \" \";\n> +\tLOG(RkISP1Agc, Debug) << str.str();\n\nYou will construct the string unconditionally, which is not very nice.\nI'd drop this. If you want to log the weight I would do it at init()\ntime, and only log the metering mode here.\n\n> +\n> +\t/* \\todo Add a control for this? */\n\nI don't think that's needed, but I may be missing something. What would\nbe the rationale ? It should be captured in the todo comment if we think\nit can be useful.\n\n> +\tparams->meas.hst_config.histogram_predivider =\n> +\t\tpredivider(context.configuration.sensor.size);\n\nShouldn't you pass the measurement window size here, not the sensor size\n?\n\n> \n> \t/* Update the configuration for histogram. */\n> \tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_HST;\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index 04b3247e1..4ab56ead5 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -44,11 +44,17 @@ public:\n> \t\t ControlList &metadata) override;\n> \n> private:\n> +\tint parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> +\t\t\t const char *prop);\n> +\tuint8_t predivider(Size &size);\n> +\n> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t ControlList &metadata);\n> \tdouble estimateLuminance(double gain) const override;\n> \n> \tSpan expMeans_;\n> +\n> +\tstd::map> meteringModes_;\n> };\n> \n> } /* namespace ipa::rkisp1::algorithms */","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 259C5C31E9\n\tfor ;\n\tTue, 11 Jun 2024 16:08:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B18E265456;\n\tTue, 11 Jun 2024 18:08:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD61261A26\n\tfor ;\n\tTue, 11 Jun 2024 18:08:37 +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 0A5CCA9A;\n\tTue, 11 Jun 2024 18:08: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=\"uUXHe70K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718122105;\n\tbh=APFc4IpiujhcTs7vcrVk88l5PhmrybrTT3PQoLdnE0U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uUXHe70KhXH6EZwmsBYl/OJdCbzdKMRtrbR1ASEIODr3pFzhO3ijrlte+bBiB7Nwk\n\tqJeAekXLkgE8lQTjNT38zuKimAaOk+sP6UEBKyRotUWl7Kkx1H3rZkrf2uGr9ZyoDx\n\tIWFi7eyBxDi3qLfegGy250+QLLuBAvmyA5YjchyU=","Date":"Tue, 11 Jun 2024 19:08:18 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 1/3] ipa: rkisp1: agc: Read histogram weights from\n\ttuning file","Message-ID":"<20240611160818.GC10397@pendragon.ideasonboard.com>","References":"<20240607080330.2667994-1-paul.elder@ideasonboard.com>\n\t<20240607080330.2667994-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240607080330.2667994-2-paul.elder@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":29873,"web_url":"https://patchwork.libcamera.org/comment/29873/","msgid":"","date":"2024-06-12T10:50:45","subject":"Re: [PATCH v5 1/3] ipa: rkisp1: agc: Read histogram weights from\n\ttuning file","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Jun 11, 2024 at 07:08:18PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Jun 07, 2024 at 05:03:28PM +0900, Paul Elder wrote:\n> > Add support to the rkisp1 AGC to read histogram weights from the tuning\n> > file. As controls for selecting the metering mode are not yet supported,\n> > for now hardcode the matrix metering mode, which is the same as what the\n> > AGC previously hardcoded.\n> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Stefan Klug \n> > Reviewed-by: Daniel Scally \n> > \n> > ---\n> > No change in v5\n> > \n> > Changes in v4:\n> > - add debug print when parsing the yaml file\n> > \n> > Changes in v3:\n> > - support the new tuning file layout for interpolated matrices\n> > - support both v10 and v12\n> > \n> > Changes in v2:\n> > - add default metering mode if none are read successfully from the\n> > tuning file\n> > - compute the predivider instead of using a table\n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 94 ++++++++++++++++++++++++++++++-\n> > src/ipa/rkisp1/algorithms/agc.h | 6 ++\n> > 2 files changed, 97 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 50e0690fe..3bbafd978 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -17,6 +17,8 @@\n> > #include \n> > #include \n> > \n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > #include \"libipa/histogram.h\"\n> > \n> > /**\n> > @@ -36,6 +38,76 @@ namespace ipa::rkisp1::algorithms {\n> > \n> > LOG_DEFINE_CATEGORY(RkISP1Agc)\n> > \n> > +int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> > +\t\t\t const char *prop)\n> > +{\n> > +\tconst YamlObject &yamlMeteringModes = tuningData[prop];\n> \n> I think this belongs to the caller.\n> \n> > +\tif (!yamlMeteringModes.isDictionary()) {\n> > +\t\tLOG(RkISP1Agc, Error)\n> > +\t\t\t<< \"'\" << prop << \"' parameter not found in tuning file\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tfor (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> > +\t\tif (controls::AeMeteringModeNameValueMap.find(key) ==\n> > +\t\t controls::AeMeteringModeNameValueMap.end()) {\n> > +\t\t\tLOG(RkISP1Agc, Warning)\n> > +\t\t\t\t<< \"Skipping unknown metering mode '\" << key << \"'\";\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tfor (const auto &[version, matrix] : value.asDict()) {\n> \n> What is the version ? It isn't used below except in a debugging message.\n\nIt's \"v10\" or \"v12\". It is indeed not used because...\n\n> \n> More generally, where do we document the format of the tuning data ? If\n> the answer is '\\todo' then could we have at least one example yaml file\n> in the source tree that includes all tuning blocks ? This can be done on\n> top of this series, but please sooner than later.\n\n(there's a schema patch from Stefan)\n\n> \n> > +\t\t\tstd::vector weights =\n> > +\t\t\t\tmatrix.getList().value_or(std::vector{});\n> > +\t\t\tif (weights.size() != context.hw->numHistogramWeights)\n\n...this is the \"real\" version checker.\n\n> > +\t\t\t\tcontinue;\n> \n> I'd be very vocal about this, or even fail parsing completely.\n\nThe tuning file will have something like:\n- v10: \n- v12: \n\nSo it'll skip the non-matching version and only keep the matching. We\nonly have to be vocal if none are found... in which case a default one\nis used. So I guess I should add a message when using a default one\nbelow then.\n\n> \n> > +\n> > +\t\t\tLOG(RkISP1Agc, Debug)\n> > +\t\t\t\t<< \"Matched metering matrix mode \"\n> > +\t\t\t\t<< key << \", version \" << version;\n> > +\n> > +\t\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> \n> If there are multiple \"versions\", you'll overwrite the metering modes.\n> Is that really intended ?\n\nIf there are multiple version then they'd get skipped because of the\nabove \"real\" version checker.\n\n> \n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (meteringModes_.empty()) {\n> > +\t\tint32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> > +\t\tstd::vector weights(context.hw->numHistogramWeights, 1);\n> > +\n> > +\t\tmeteringModes_[meteringModeId] = weights;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +uint8_t Agc::predivider(Size &size)\n> \n> Agc::computeHistogramPredivider()\n> \n> (or s/compute/calculate/)\n> \n> > +{\n> > +\t/*\n> > +\t * The maximum number of pixels that could potentially be in one bin is\n> > +\t * if all the pixels of the image are in it, multiplied by 3 for the\n> \n> I think you should pass the histogram mode to this function, to pick the\n> right multiplier. We currently hardcode usage of the Y histogram, so the\n> *3 factor below will result in higher predividers and loss of accuracy.\n\nAh, indeed.\n\n> \n> I think we should also take the weights into account.\n\nHow so? The documentation doesn't mention having to do so.\n\n> \n> > +\t * three color channels. The counter for each bin is 16 bits wide, so\n> > +\t * `factor` thus contains the number of times we'd wrap around. This is\n> > +\t * obviously the number of pixels that we need to skip to make sure\n> > +\t * that we don't wrap around, but we compute the square root of it\n> > +\t * instead, as the skip that we need to program is for both the x and y\n> > +\t * directions.\n> > +\t *\n> > +\t * There's a bit of extra rounding math to make sure the rounding goes\n> > +\t * the correct direction so that the square of the step is big enough\n> > +\t * to encompass the `factor` number of pixels that we need to skip.\n> > +\t */\n> > +\tdouble factor = size.width * size.height * 3 / 65535.0;\n> > +\tdouble root = std::sqrt(factor);\n> > +\tuint8_t ret;\n> \n> s/ret/predivider/\n> \n> > +\n> > +\tif (std::pow(std::floor(root), 2) + 0.01 < factor)\n> > +\t\tret = static_cast(std::ceil(root));\n> > +\telse\n> > +\t\tret = static_cast(std::floor(root));\n> \n> I have trouble validating the math here. Let's assume we have an image\n> size of 760x460 pixels. This leads to\n> \n> \tfactor = 760 * 460* 3 / 65535.0 = 16.003662165255207\n> \n> (I'm using Python to do the math, I don't think the differences in\n> rounding with C++ double, if any, will make a difference here)\n> \n> \troot = 4.000457744465652\n> \tstd::pow(std::floor(root), 2) + 0.01 = 16.01\n> \n> This is higher than factor, so\n> \n> \tpredivider = static_cast(std::floor(root)) = 4\n> \n> Skipping by a factor of 4 in both directions gives us 190x115 pixels,\n> and multiplied by 3, that's 65550, which overflows. It may not be the\n> biggest deal, as the hardware clamps the histogram values instead of\n> rolling over, and the risk of *all* pixels being in the same bin is\n> virtually 0, but it's still not correct.\n\nTurns out I didn't need the +0.01.\n\n\nPaul\n\n> \n> > +\n> > +\treturn std::clamp(ret, 3, 127);\n> > +}\n> > +\n> > Agc::Agc()\n> > {\n> > \tsupportsRaw_ = true;\n> > @@ -59,6 +131,10 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > +\tret = parseMeteringModes(context, tuningData, \"AeMeteringMode\");\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > \tcontext.ctrlMap.merge(controls());\n> > \n> > \treturn 0;\n> > @@ -160,6 +236,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > \t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > \t}\n> > \n> > +\t/* \\todo Remove this when we can set the below with controls */\n> \n> Patch 3/3 enables setting the metering mode through controls, but\n> doesn't remove this. Is that intended ? Note that we should reprogram\n> the histogram engine only in the case where the metering mode has\n> actually changed, we shouldn't do it on every frame.\n> \n> > \tif (frame > 0)\n> > \t\treturn;\n> > \n> > @@ -178,14 +255,25 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > \tparams->meas.hst_config.meas_window = context.configuration.agc.measureWindow;\n> > \t/* Produce the luminance histogram. */\n> > \tparams->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;\n> > +\n> > \t/* Set an average weighted histogram. */\n> > \tSpan weights{\n> > \t\tparams->meas.hst_config.hist_weight,\n> > \t\tcontext.hw->numHistogramWeights\n> > \t};\n> > -\tstd::fill(weights.begin(), weights.end(), 1);\n> > -\t/* Step size can't be less than 3. */\n> > -\tparams->meas.hst_config.histogram_predivider = 4;\n> > +\t/* \\todo Get this from control */\n> > +\tstd::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> > +\tstd::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> > +\n> > +\tstd::stringstream str;\n> > +\tstr << \"Histogram weights : \";\n> > +\tfor (size_t i = 0; i < context.hw->numHistogramWeights; i++)\n> > +\t\tstr << (int)params->meas.hst_config.hist_weight[i] << \" \";\n> > +\tLOG(RkISP1Agc, Debug) << str.str();\n> \n> You will construct the string unconditionally, which is not very nice.\n> I'd drop this. If you want to log the weight I would do it at init()\n> time, and only log the metering mode here.\n> \n> > +\n> > +\t/* \\todo Add a control for this? */\n> \n> I don't think that's needed, but I may be missing something. What would\n> be the rationale ? It should be captured in the todo comment if we think\n> it can be useful.\n> \n> > +\tparams->meas.hst_config.histogram_predivider =\n> > +\t\tpredivider(context.configuration.sensor.size);\n> \n> Shouldn't you pass the measurement window size here, not the sensor size\n> ?\n> \n> > \n> > \t/* Update the configuration for histogram. */\n> > \tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_HST;\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index 04b3247e1..4ab56ead5 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -44,11 +44,17 @@ public:\n> > \t\t ControlList &metadata) override;\n> > \n> > private:\n> > +\tint parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> > +\t\t\t const char *prop);\n> > +\tuint8_t predivider(Size &size);\n> > +\n> > \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > \t\t\t ControlList &metadata);\n> > \tdouble estimateLuminance(double gain) const override;\n> > \n> > \tSpan expMeans_;\n> > +\n> > +\tstd::map> meteringModes_;\n> > };\n> > \n> > } /* namespace ipa::rkisp1::algorithms */","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 59D82BD87C\n\tfor ;\n\tWed, 12 Jun 2024 10:50:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 353AB6545D;\n\tWed, 12 Jun 2024 12:50:54 +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 133A56545A\n\tfor ;\n\tWed, 12 Jun 2024 12:50:52 +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 3FFC64AB;\n\tWed, 12 Jun 2024 12:50:37 +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=\"M+dGmf0k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718189438;\n\tbh=6dAeUCjNtyn111jhvAke9c1fr25ZeWG0oWti9m+oBBA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M+dGmf0kA9VVpiAcr4X4+tJv0GWTIwbvSsUkj+IdQll4MuafbbOQlqVaipAiVOcVt\n\t9+KvUVmhsK7Cro8bJ1syhU9zHzTllpTTIhLo1/v6yUOFQNpZYIY7vHlJRZ2uHNSgv1\n\tvyrOPlEJFBD5C7me1JPDJHooLx216Y53nSrYyC0g=","Date":"Wed, 12 Jun 2024 19:50:45 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 1/3] ipa: rkisp1: agc: Read histogram weights from\n\ttuning file","Message-ID":"","References":"<20240607080330.2667994-1-paul.elder@ideasonboard.com>\n\t<20240607080330.2667994-2-paul.elder@ideasonboard.com>\n\t<20240611160818.GC10397@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240611160818.GC10397@pendragon.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":29874,"web_url":"https://patchwork.libcamera.org/comment/29874/","msgid":"<20240612110157.GK28989@pendragon.ideasonboard.com>","date":"2024-06-12T11:01:57","subject":"Re: [PATCH v5 1/3] ipa: rkisp1: agc: Read histogram weights from\n\ttuning file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jun 12, 2024 at 07:50:45PM +0900, Paul Elder wrote:\n> On Tue, Jun 11, 2024 at 07:08:18PM +0300, Laurent Pinchart wrote:\n> > On Fri, Jun 07, 2024 at 05:03:28PM +0900, Paul Elder wrote:\n> > > Add support to the rkisp1 AGC to read histogram weights from the tuning\n> > > file. As controls for selecting the metering mode are not yet supported,\n> > > for now hardcode the matrix metering mode, which is the same as what the\n> > > AGC previously hardcoded.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > Reviewed-by: Stefan Klug \n> > > Reviewed-by: Daniel Scally \n> > > \n> > > ---\n> > > No change in v5\n> > > \n> > > Changes in v4:\n> > > - add debug print when parsing the yaml file\n> > > \n> > > Changes in v3:\n> > > - support the new tuning file layout for interpolated matrices\n> > > - support both v10 and v12\n> > > \n> > > Changes in v2:\n> > > - add default metering mode if none are read successfully from the\n> > > tuning file\n> > > - compute the predivider instead of using a table\n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 94 ++++++++++++++++++++++++++++++-\n> > > src/ipa/rkisp1/algorithms/agc.h | 6 ++\n> > > 2 files changed, 97 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 50e0690fe..3bbafd978 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -17,6 +17,8 @@\n> > > #include \n> > > #include \n> > > \n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > #include \"libipa/histogram.h\"\n> > > \n> > > /**\n> > > @@ -36,6 +38,76 @@ namespace ipa::rkisp1::algorithms {\n> > > \n> > > LOG_DEFINE_CATEGORY(RkISP1Agc)\n> > > \n> > > +int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> > > +\t\t\t const char *prop)\n> > > +{\n> > > +\tconst YamlObject &yamlMeteringModes = tuningData[prop];\n> > \n> > I think this belongs to the caller.\n> > \n> > > +\tif (!yamlMeteringModes.isDictionary()) {\n> > > +\t\tLOG(RkISP1Agc, Error)\n> > > +\t\t\t<< \"'\" << prop << \"' parameter not found in tuning file\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tfor (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> > > +\t\tif (controls::AeMeteringModeNameValueMap.find(key) ==\n> > > +\t\t controls::AeMeteringModeNameValueMap.end()) {\n> > > +\t\t\tLOG(RkISP1Agc, Warning)\n> > > +\t\t\t\t<< \"Skipping unknown metering mode '\" << key << \"'\";\n> > > +\t\t\tcontinue;\n> > > +\t\t}\n> > > +\n> > > +\t\tfor (const auto &[version, matrix] : value.asDict()) {\n> > \n> > What is the version ? It isn't used below except in a debugging message.\n> \n> It's \"v10\" or \"v12\". It is indeed not used because...\n> \n> > More generally, where do we document the format of the tuning data ? If\n> > the answer is '\\todo' then could we have at least one example yaml file\n> > in the source tree that includes all tuning blocks ? This can be done on\n> > top of this series, but please sooner than later.\n> \n> (there's a schema patch from Stefan)\n\nOne step in the right direction :-)\n\n> > > +\t\t\tstd::vector weights =\n> > > +\t\t\t\tmatrix.getList().value_or(std::vector{});\n> > > +\t\t\tif (weights.size() != context.hw->numHistogramWeights)\n> \n> ...this is the \"real\" version checker.\n> \n> > > +\t\t\t\tcontinue;\n> > \n> > I'd be very vocal about this, or even fail parsing completely.\n> \n> The tuning file will have something like:\n> - v10: \n> - v12: \n> \n> So it'll skip the non-matching version and only keep the matching. We\n> only have to be vocal if none are found... in which case a default one\n> is used. So I guess I should add a message when using a default one\n> below then.\n\nI don't like this much. Could we have a single table that we adapt\nbetween the versions ?\n\nOn a more general note, we will have algorithms that will differ on\ni.MX8MP compared to v10 and v12, as the ISP8000Nano has additional\nhardware blocks. I think this will require different tuning files on\ndifferent platforms.\n\n> > > +\n> > > +\t\t\tLOG(RkISP1Agc, Debug)\n> > > +\t\t\t\t<< \"Matched metering matrix mode \"\n> > > +\t\t\t\t<< key << \", version \" << version;\n> > > +\n> > > +\t\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> > \n> > If there are multiple \"versions\", you'll overwrite the metering modes.\n> > Is that really intended ?\n> \n> If there are multiple version then they'd get skipped because of the\n> above \"real\" version checker.\n> \n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tif (meteringModes_.empty()) {\n> > > +\t\tint32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> > > +\t\tstd::vector weights(context.hw->numHistogramWeights, 1);\n> > > +\n> > > +\t\tmeteringModes_[meteringModeId] = weights;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +uint8_t Agc::predivider(Size &size)\n> > \n> > Agc::computeHistogramPredivider()\n> > \n> > (or s/compute/calculate/)\n> > \n> > > +{\n> > > +\t/*\n> > > +\t * The maximum number of pixels that could potentially be in one bin is\n> > > +\t * if all the pixels of the image are in it, multiplied by 3 for the\n> > \n> > I think you should pass the histogram mode to this function, to pick the\n> > right multiplier. We currently hardcode usage of the Y histogram, so the\n> > *3 factor below will result in higher predividers and loss of accuracy.\n> \n> Ah, indeed.\n> \n> > I think we should also take the weights into account.\n> \n> How so? The documentation doesn't mention having to do so.\n\nThe weights are specified as an integer in the [0, 16] range, which maps\nto [0.0, 1.0] scaling factors. The histogram accumulates weighted\npixels. If all zones use a 1/16 weight for instance, then the histogram\nvalues will effectively be divided by 16, which influences the need for\na predivider.\n\n> > > +\t * three color channels. The counter for each bin is 16 bits wide, so\n> > > +\t * `factor` thus contains the number of times we'd wrap around. This is\n> > > +\t * obviously the number of pixels that we need to skip to make sure\n> > > +\t * that we don't wrap around, but we compute the square root of it\n> > > +\t * instead, as the skip that we need to program is for both the x and y\n> > > +\t * directions.\n> > > +\t *\n> > > +\t * There's a bit of extra rounding math to make sure the rounding goes\n> > > +\t * the correct direction so that the square of the step is big enough\n> > > +\t * to encompass the `factor` number of pixels that we need to skip.\n> > > +\t */\n> > > +\tdouble factor = size.width * size.height * 3 / 65535.0;\n> > > +\tdouble root = std::sqrt(factor);\n> > > +\tuint8_t ret;\n> > \n> > s/ret/predivider/\n> > \n> > > +\n> > > +\tif (std::pow(std::floor(root), 2) + 0.01 < factor)\n> > > +\t\tret = static_cast(std::ceil(root));\n> > > +\telse\n> > > +\t\tret = static_cast(std::floor(root));\n> > \n> > I have trouble validating the math here. Let's assume we have an image\n> > size of 760x460 pixels. This leads to\n> > \n> > \tfactor = 760 * 460* 3 / 65535.0 = 16.003662165255207\n> > \n> > (I'm using Python to do the math, I don't think the differences in\n> > rounding with C++ double, if any, will make a difference here)\n> > \n> > \troot = 4.000457744465652\n> > \tstd::pow(std::floor(root), 2) + 0.01 = 16.01\n> > \n> > This is higher than factor, so\n> > \n> > \tpredivider = static_cast(std::floor(root)) = 4\n> > \n> > Skipping by a factor of 4 in both directions gives us 190x115 pixels,\n> > and multiplied by 3, that's 65550, which overflows. It may not be the\n> > biggest deal, as the hardware clamps the histogram values instead of\n> > rolling over, and the risk of *all* pixels being in the same bin is\n> > virtually 0, but it's still not correct.\n> \n> Turns out I didn't need the +0.01.\n\nIf you drop the +0.01, can the condition ever be false ?\n\n> > > +\n> > > +\treturn std::clamp(ret, 3, 127);\n> > > +}\n> > > +\n> > > Agc::Agc()\n> > > {\n> > > \tsupportsRaw_ = true;\n> > > @@ -59,6 +131,10 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > \n> > > +\tret = parseMeteringModes(context, tuningData, \"AeMeteringMode\");\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > \tcontext.ctrlMap.merge(controls());\n> > > \n> > > \treturn 0;\n> > > @@ -160,6 +236,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > \t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > > \t}\n> > > \n> > > +\t/* \\todo Remove this when we can set the below with controls */\n> > \n> > Patch 3/3 enables setting the metering mode through controls, but\n> > doesn't remove this. Is that intended ? Note that we should reprogram\n> > the histogram engine only in the case where the metering mode has\n> > actually changed, we shouldn't do it on every frame.\n> > \n> > > \tif (frame > 0)\n> > > \t\treturn;\n> > > \n> > > @@ -178,14 +255,25 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > \tparams->meas.hst_config.meas_window = context.configuration.agc.measureWindow;\n> > > \t/* Produce the luminance histogram. */\n> > > \tparams->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;\n> > > +\n> > > \t/* Set an average weighted histogram. */\n> > > \tSpan weights{\n> > > \t\tparams->meas.hst_config.hist_weight,\n> > > \t\tcontext.hw->numHistogramWeights\n> > > \t};\n> > > -\tstd::fill(weights.begin(), weights.end(), 1);\n> > > -\t/* Step size can't be less than 3. */\n> > > -\tparams->meas.hst_config.histogram_predivider = 4;\n> > > +\t/* \\todo Get this from control */\n> > > +\tstd::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> > > +\tstd::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> > > +\n> > > +\tstd::stringstream str;\n> > > +\tstr << \"Histogram weights : \";\n> > > +\tfor (size_t i = 0; i < context.hw->numHistogramWeights; i++)\n> > > +\t\tstr << (int)params->meas.hst_config.hist_weight[i] << \" \";\n> > > +\tLOG(RkISP1Agc, Debug) << str.str();\n> > \n> > You will construct the string unconditionally, which is not very nice.\n> > I'd drop this. If you want to log the weight I would do it at init()\n> > time, and only log the metering mode here.\n> > \n> > > +\n> > > +\t/* \\todo Add a control for this? */\n> > \n> > I don't think that's needed, but I may be missing something. What would\n> > be the rationale ? It should be captured in the todo comment if we think\n> > it can be useful.\n> > \n> > > +\tparams->meas.hst_config.histogram_predivider =\n> > > +\t\tpredivider(context.configuration.sensor.size);\n> > \n> > Shouldn't you pass the measurement window size here, not the sensor size\n> > ?\n> > \n> > > \n> > > \t/* Update the configuration for histogram. */\n> > > \tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_HST;\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > index 04b3247e1..4ab56ead5 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > @@ -44,11 +44,17 @@ public:\n> > > \t\t ControlList &metadata) override;\n> > > \n> > > private:\n> > > +\tint parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> > > +\t\t\t const char *prop);\n> > > +\tuint8_t predivider(Size &size);\n> > > +\n> > > \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > \t\t\t ControlList &metadata);\n> > > \tdouble estimateLuminance(double gain) const override;\n> > > \n> > > \tSpan expMeans_;\n> > > +\n> > > +\tstd::map> meteringModes_;\n> > > };\n> > > \n> > > } /* namespace ipa::rkisp1::algorithms */","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 F28E2C3237\n\tfor ;\n\tWed, 12 Jun 2024 11:02:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDFB065463;\n\tWed, 12 Jun 2024 13:02:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80B8E6545A\n\tfor ;\n\tWed, 12 Jun 2024 13:02:19 +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 C4860EA2;\n\tWed, 12 Jun 2024 13:02: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=\"c91E3btR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718190126;\n\tbh=w/qXwdmlYJ7716aUu+rPgzQXsQ8/KU2gSR0o0an48O8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c91E3btRWBMr4lmPtlPTi9pylp48Wn1sCA4Qj3K5ZM4A/m9pPXgsY81MAgyuhSk6J\n\tLZUnvK9qy84PiMBP1b2RZvS3bb3DjklPxa+Ht2Upy4+7BpRSJo0XDAlU7plWZIM0fw\n\tD0C6h8kb3igDTZMsrpDk3VflQHUj7KxwHiyiAmBQ=","Date":"Wed, 12 Jun 2024 14:01:57 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 1/3] ipa: rkisp1: agc: Read histogram weights from\n\ttuning file","Message-ID":"<20240612110157.GK28989@pendragon.ideasonboard.com>","References":"<20240607080330.2667994-1-paul.elder@ideasonboard.com>\n\t<20240607080330.2667994-2-paul.elder@ideasonboard.com>\n\t<20240611160818.GC10397@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]