[{"id":29228,"web_url":"https://patchwork.libcamera.org/comment/29228/","msgid":"<20240415132251.zd3zisoikjfbaiyt@jasper>","date":"2024-04-15T13:22:51","subject":"Re: [PATCH 1/5] ipa: rkisp1: agc: Read histogram weights from tuning\n\tfile","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nthanks for the patch.\n\nOn Fri, Apr 05, 2024 at 11:47:25PM +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> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 80 +++++++++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/agc.h | 6 +++\n> 2 files changed, 83 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 1cd977a0..fd47ba4e 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> @@ -42,6 +44,62 @@ static constexpr double kMinAnalogueGain = 1.0;\n> /* \\todo Honour the FrameDurationLimits control instead of hardcoding a limit */\n> static constexpr utils::Duration kMaxShutterSpeed = 60ms;\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> +\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\tstd::vector weights =\n> +\t\t\tvalue.getList().value_or(std::vector{});\n> +\t\tif (weights.size() != context.hw->numHistogramWeights) {\n> +\t\t\tLOG(RkISP1Agc, Error)\n> +\t\t\t\t<< \"Invalid '\" << key << \"' values: expected \"\n> +\t\t\t\t<< context.hw->numHistogramWeights\n> +\t\t\t\t<< \" elements, got \" << weights.size();\n> +\t\t\treturn -ENODATA;\n> +\t\t}\n> +\n> +\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> +\t}\n\nKieran already mentioned that. What about adding the matrix metering\nmode (+ warning) in case meteringModes_ is still empty.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +uint8_t Agc::predivider(Size &size)\n> +{\n> +\tstatic const std::vector>\n> +\t\tpixelCountThresholds = {\n> +\t\t\t{ 640 * 480, 3 },\n> +\t\t\t{ 800 * 600, 4 },\n> +\t\t\t{ 1024 * 768, 5 },\n> +\t\t\t{ 1280 * 960, 6 },\n> +\t\t\t{ 1600 * 1200, 8 },\n> +\t\t\t{ 2048 * 1536, 10 },\n> +\t\t\t{ 2600 * 1950, 12 },\n> +\t\t\t{ 4096 * 3072, 16 },\n> +\t\t};\n\nWhere do these values come from? What would happen if we keep the predivider\nof 4?\n\n> +\n> +\tunsigned long pixels = size.width * size.height;\n> +\tfor (auto &pair : pixelCountThresholds)\n> +\t\tif (pixels < pair.first)\n> +\t\t\treturn pair.second;\n> +\n> +\treturn 24;\n> +}\n> +\n> Agc::Agc()\n> {\n> \tsupportsRaw_ = true;\n> @@ -72,6 +130,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> @@ -177,6 +239,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> \tif (frame > 0)\n> \t\treturn;\n> \n> @@ -195,14 +258,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> +\t/* \\todo Add a control for this? */\n> +\tparams->meas.hst_config.histogram_predivider =\n> +\t\tpredivider(context.configuration.sensor.size);\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 311d4e94..43e3d5b2 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -44,6 +44,10 @@ 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> \tvoid parseStatistics(const rkisp1_stat_buffer *stats,\n> @@ -52,6 +56,8 @@ private:\n> \n> \tHistogram hist_;\n> \tSpan expMeans_;\n> +\n> +\tstd::map> meteringModes_;\n> };\n> \n> } /* namespace ipa::rkisp1::algorithms */\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 81E6EBE08B\n\tfor ;\n\tMon, 15 Apr 2024 13:22:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8795963352;\n\tMon, 15 Apr 2024 15:22:56 +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 A511563339\n\tfor ;\n\tMon, 15 Apr 2024 15:22:54 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:7a0d:dd2e:881a:db83])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C79F79C1;\n\tMon, 15 Apr 2024 15:22:08 +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=\"voC3Z/uB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713187328;\n\tbh=jAvndHy1tcFl8CZgfareI9N/Jd0tvNM/xG6J7o2Kvdc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=voC3Z/uBdPoeFwkNu1gfrpxb9aPkt0Z08ZBhIE9fg9tIgd0LWSx7PJ2D+LMfnbC/p\n\tCv2sFjmTUWUpHv0rD/vuASl6089n1x5Bs3/GXdwiZU+jDbuh0NuiiC4/SMdeqNGb1A\n\t/tgK8x4N4kmyHsNydExw0KnQ7tYmTxQDkLl7z3rI=","Date":"Mon, 15 Apr 2024 15:22:51 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: rkisp1: agc: Read histogram weights from tuning\n\tfile","Message-ID":"<20240415132251.zd3zisoikjfbaiyt@jasper>","References":"<20240405144729.2992219-1-paul.elder@ideasonboard.com>\n\t<20240405144729.2992219-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240405144729.2992219-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":29481,"web_url":"https://patchwork.libcamera.org/comment/29481/","msgid":"","date":"2024-05-09T11:08:14","subject":"Re: [PATCH 1/5] ipa: rkisp1: agc: Read histogram weights from tuning\n\tfile","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Apr 15, 2024 at 03:22:51PM +0200, Stefan Klug wrote:\n> Hi Paul,\n> \n> thanks for the patch.\n> \n> On Fri, Apr 05, 2024 at 11:47:25PM +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> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 80 +++++++++++++++++++++++++++++--\n> > src/ipa/rkisp1/algorithms/agc.h | 6 +++\n> > 2 files changed, 83 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 1cd977a0..fd47ba4e 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> > @@ -42,6 +44,62 @@ static constexpr double kMinAnalogueGain = 1.0;\n> > /* \\todo Honour the FrameDurationLimits control instead of hardcoding a limit */\n> > static constexpr utils::Duration kMaxShutterSpeed = 60ms;\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> > +\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\tstd::vector weights =\n> > +\t\t\tvalue.getList().value_or(std::vector{});\n> > +\t\tif (weights.size() != context.hw->numHistogramWeights) {\n> > +\t\t\tLOG(RkISP1Agc, Error)\n> > +\t\t\t\t<< \"Invalid '\" << key << \"' values: expected \"\n> > +\t\t\t\t<< context.hw->numHistogramWeights\n> > +\t\t\t\t<< \" elements, got \" << weights.size();\n> > +\t\t\treturn -ENODATA;\n> > +\t\t}\n> > +\n> > +\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> > +\t}\n> \n> Kieran already mentioned that. What about adding the matrix metering\n> mode (+ warning) in case meteringModes_ is still empty.\n> \n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +uint8_t Agc::predivider(Size &size)\n> > +{\n> > +\tstatic const std::vector>\n> > +\t\tpixelCountThresholds = {\n> > +\t\t\t{ 640 * 480, 3 },\n> > +\t\t\t{ 800 * 600, 4 },\n> > +\t\t\t{ 1024 * 768, 5 },\n> > +\t\t\t{ 1280 * 960, 6 },\n> > +\t\t\t{ 1600 * 1200, 8 },\n> > +\t\t\t{ 2048 * 1536, 10 },\n> > +\t\t\t{ 2600 * 1950, 12 },\n> > +\t\t\t{ 4096 * 3072, 16 },\n> > +\t\t};\n> \n> Where do these values come from? What would happen if we keep the predivider\n> of 4?\n\nIt comes from the programming guide. If the predivider is small and the\nhistogram is spikey then the counter will wraparound for the spikey bin.\n\n\nPaul\n\n> \n> > +\n> > +\tunsigned long pixels = size.width * size.height;\n> > +\tfor (auto &pair : pixelCountThresholds)\n> > +\t\tif (pixels < pair.first)\n> > +\t\t\treturn pair.second;\n> > +\n> > +\treturn 24;\n> > +}\n> > +\n> > Agc::Agc()\n> > {\n> > \tsupportsRaw_ = true;\n> > @@ -72,6 +130,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> > @@ -177,6 +239,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> > \tif (frame > 0)\n> > \t\treturn;\n> > \n> > @@ -195,14 +258,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> > +\t/* \\todo Add a control for this? */\n> > +\tparams->meas.hst_config.histogram_predivider =\n> > +\t\tpredivider(context.configuration.sensor.size);\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 311d4e94..43e3d5b2 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -44,6 +44,10 @@ 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> > \tvoid parseStatistics(const rkisp1_stat_buffer *stats,\n> > @@ -52,6 +56,8 @@ private:\n> > \n> > \tHistogram hist_;\n> > \tSpan expMeans_;\n> > +\n> > +\tstd::map> meteringModes_;\n> > };\n> > \n> > } /* namespace ipa::rkisp1::algorithms */\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 9674EBDE6B\n\tfor ;\n\tThu, 9 May 2024 11:08:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97EA863460;\n\tThu, 9 May 2024 13:08:24 +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 531CC633FA\n\tfor ;\n\tThu, 9 May 2024 13:08:23 +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 E4E29904;\n\tThu, 9 May 2024 13:08:18 +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=\"UkuNgWey\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715252899;\n\tbh=v+KJtzReU2Ah0U2QQ7NpyWTramLnhRvfegEzFRRFLS4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UkuNgWeyWUx7f242qJ+E8QyW7Ow/sL7O6pvekCwkXQRF4veBxg9outQaKeBFRoQ8O\n\t6PHzm2kTB+MsSG4e6njtKqvZUrQ++9GZwdKjhGCiDMyPgUuW37DG9ISWKSy8QilDz0\n\t/GUZz/tHh+KPSBozL8mHT27uaapBSEJap8rW2C84=","Date":"Thu, 9 May 2024 20:08:14 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: rkisp1: agc: Read histogram weights from tuning\n\tfile","Message-ID":"","References":"<20240405144729.2992219-1-paul.elder@ideasonboard.com>\n\t<20240405144729.2992219-2-paul.elder@ideasonboard.com>\n\t<20240415132251.zd3zisoikjfbaiyt@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240415132251.zd3zisoikjfbaiyt@jasper>","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":29484,"web_url":"https://patchwork.libcamera.org/comment/29484/","msgid":"<20240509111153.GE14520@pendragon.ideasonboard.com>","date":"2024-05-09T11:11:53","subject":"Re: [PATCH 1/5] ipa: rkisp1: agc: Read histogram weights from tuning\n\tfile","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Apr 15, 2024 at 03:22:51PM +0200, Stefan Klug wrote:\n> Hi Paul,\n> \n> thanks for the patch.\n> \n> On Fri, Apr 05, 2024 at 11:47:25PM +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> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 80 +++++++++++++++++++++++++++++--\n> > src/ipa/rkisp1/algorithms/agc.h | 6 +++\n> > 2 files changed, 83 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 1cd977a0..fd47ba4e 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> > @@ -42,6 +44,62 @@ static constexpr double kMinAnalogueGain = 1.0;\n> > /* \\todo Honour the FrameDurationLimits control instead of hardcoding a limit */\n> > static constexpr utils::Duration kMaxShutterSpeed = 60ms;\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> > +\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\tstd::vector weights =\n> > +\t\t\tvalue.getList().value_or(std::vector{});\n> > +\t\tif (weights.size() != context.hw->numHistogramWeights) {\n> > +\t\t\tLOG(RkISP1Agc, Error)\n> > +\t\t\t\t<< \"Invalid '\" << key << \"' values: expected \"\n> > +\t\t\t\t<< context.hw->numHistogramWeights\n> > +\t\t\t\t<< \" elements, got \" << weights.size();\n> > +\t\t\treturn -ENODATA;\n> > +\t\t}\n> > +\n> > +\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> > +\t}\n> \n> Kieran already mentioned that. What about adding the matrix metering\n> mode (+ warning) in case meteringModes_ is still empty.\n> \n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +uint8_t Agc::predivider(Size &size)\n> > +{\n> > +\tstatic const std::vector>\n> > +\t\tpixelCountThresholds = {\n> > +\t\t\t{ 640 * 480, 3 },\n> > +\t\t\t{ 800 * 600, 4 },\n> > +\t\t\t{ 1024 * 768, 5 },\n> > +\t\t\t{ 1280 * 960, 6 },\n> > +\t\t\t{ 1600 * 1200, 8 },\n> > +\t\t\t{ 2048 * 1536, 10 },\n> > +\t\t\t{ 2600 * 1950, 12 },\n> > +\t\t\t{ 4096 * 3072, 16 },\n> > +\t\t};\n> \n> Where do these values come from? What would happen if we keep the predivider\n> of 4?\n\nThe predivider is meant to avoid overflows in the worst case situation.\nI'd rather compute them in code instead of using a table.\n\n> > +\n> > +\tunsigned long pixels = size.width * size.height;\n> > +\tfor (auto &pair : pixelCountThresholds)\n> > +\t\tif (pixels < pair.first)\n> > +\t\t\treturn pair.second;\n> > +\n> > +\treturn 24;\n> > +}\n> > +\n> > Agc::Agc()\n> > {\n> > \tsupportsRaw_ = true;\n> > @@ -72,6 +130,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> > @@ -177,6 +239,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> > \tif (frame > 0)\n> > \t\treturn;\n> > \n> > @@ -195,14 +258,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> > +\t/* \\todo Add a control for this? */\n> > +\tparams->meas.hst_config.histogram_predivider =\n> > +\t\tpredivider(context.configuration.sensor.size);\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 311d4e94..43e3d5b2 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -44,6 +44,10 @@ 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> > \tvoid parseStatistics(const rkisp1_stat_buffer *stats,\n> > @@ -52,6 +56,8 @@ private:\n> > \n> > \tHistogram hist_;\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 43DA9C3226\n\tfor ;\n\tThu, 9 May 2024 11:12:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4167B63469;\n\tThu, 9 May 2024 13:12:04 +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 AD6D363460\n\tfor ;\n\tThu, 9 May 2024 13:12:02 +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 EE3A7904;\n\tThu, 9 May 2024 13:11:58 +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=\"t5bABtwI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715253119;\n\tbh=OXQYMKQaIsYe+CT7uu99lB99xyksLf6dEJpNJBQQreU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=t5bABtwIk4J8e9ESjRVR41MPVdBz5yEoPOAwuh9f5byxsxiyOMg/TEP/Y+35zfXC8\n\tX8J2lK2BQxjipEZKLkf8LUp97fWxIX99KtMuswvr8yaL0kP3SYhNCES9EQZNf21oug\n\tzZbXHUx410g6htSjvdu4MCOuqRYl+45Rgi6XCpPA=","Date":"Thu, 9 May 2024 14:11:53 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: rkisp1: agc: Read histogram weights from tuning\n\tfile","Message-ID":"<20240509111153.GE14520@pendragon.ideasonboard.com>","References":"<20240405144729.2992219-1-paul.elder@ideasonboard.com>\n\t<20240405144729.2992219-2-paul.elder@ideasonboard.com>\n\t<20240415132251.zd3zisoikjfbaiyt@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240415132251.zd3zisoikjfbaiyt@jasper>","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\" "}}]