[{"id":35468,"web_url":"https://patchwork.libcamera.org/comment/35468/","msgid":"<175550903057.1721288.16662952299252537059@ping.linuxembedded.co.uk>","date":"2025-08-18T09:23:50","subject":"Re: [RFC PATCH 2/4] ipa: rkisp1: agc: Do not derive from\n\tAgcMeanLuminance","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-08-18 09:28:40)\n> There is no need to derive from AgcMeanLuminance anymore. Use\n> composition for easier understanding.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 24 ++++++++++++------------\n> src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> 2 files changed, 14 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 3a078f9753f6..f7dc025ee050 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -138,7 +138,7 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> {\n> int ret;\n> \n> - ret = parseTuningData(tuningData);\n> + ret = agc_.parseTuningData(tuningData);\n> if (ret)\n> return ret;\n> \n> @@ -158,7 +158,7 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> /* \\todo Move this to the Camera class */\n> context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);\n> context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n> - context.ctrlMap.merge(controls());\n> + context.ctrlMap.merge(agc_.controls());\n\nHaving worked through similar things - this is absolutely where it's\nbeneficial.\n\nI was confused last week - not realising controls() was actually coming\nfrom AgcMeanLuiminance.\n\n\nIn my experimental branch - I think this entire function can be a single\ncall itno AgcMeanLuminance.\n\n\tagc_->init(....)\n\nThen the AgcMeanLuminance can parse it's tuning data and register the\ncontrols it provides, letting the platform specific IPA handle anything\nextra if it needs. Which I don't think is much if ever.\n\nI think\nhttps://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18/files#diff-eb66f1d79de37124aabcc706393ce9516891401a\nshows you my current diff (all very much still WIP).\n\nI also think handling FrameDurationLimits needs to move *out* of AGC and\ninto a sensor specific handler.\n\n\n> \n> return 0;\n> }\n> @@ -183,9 +183,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> context.activeState.agc.exposureValue = 0.0;\n> \n> context.activeState.agc.constraintMode =\n> - static_cast(constraintModes().begin()->first);\n> + static_cast(agc_.constraintModes().begin()->first);\n\nSo this is just validating my theory that all of this handling should\nmove inside the AgcMeanLuminance class itself !\n\n\n> context.activeState.agc.exposureMode =\n> - static_cast(exposureModeHelpers().begin()->first);\n> + static_cast(agc_.exposureModeHelpers().begin()->first);\n> context.activeState.agc.meteringMode =\n> static_cast(meteringModes_.begin()->first);\n> \n> @@ -199,12 +199,12 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> \n> - setLimits(context.configuration.sensor.minExposureTime,\n> + agc_.setLimits(context.configuration.sensor.minExposureTime,\n> context.configuration.sensor.maxExposureTime,\n> context.configuration.sensor.minAnalogueGain,\n> context.configuration.sensor.maxAnalogueGain);\n> \n> - resetFrameCount();\n> + agc_.resetFrameCount();\n> \n> return 0;\n> }\n> @@ -553,7 +553,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> maxAnalogueGain = frameContext.agc.gain;\n> }\n> \n> - setLimits(minExposureTime, maxExposureTime, minAnalogueGain, maxAnalogueGain);\n> + agc_.setLimits(minExposureTime, maxExposureTime, minAnalogueGain, maxAnalogueGain);\n> \n> /*\n> * The Agc algorithm needs to know the effective exposure value that was\n> @@ -563,7 +563,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> double analogueGain = frameContext.sensor.gain;\n> utils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> \n> - setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> + agc_.setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> \n> AgcMeanLuminance::EstimateLuminanceFn estimateLuminanceFn = std::bind(\n> &Agc::estimateLuminance, this,\n> @@ -574,10 +574,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> utils::Duration newExposureTime;\n> double aGain, dGain;\n> std::tie(newExposureTime, aGain, dGain) =\n> - calculateNewEv(estimateLuminanceFn,\n> - frameContext.agc.constraintMode,\n> - frameContext.agc.exposureMode,\n> - hist, effectiveExposureValue);\n> + agc_.calculateNewEv(estimateLuminanceFn,\n> + frameContext.agc.constraintMode,\n> + frameContext.agc.exposureMode,\n> + hist, effectiveExposureValue);\n> \n> LOG(RkISP1Agc, Debug)\n> << \"Divided up exposure time, analogue gain and digital gain are \"\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index 742c8f7371f3..02ba04bd2641 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -22,7 +22,7 @@ namespace libcamera {\n> \n> namespace ipa::rkisp1::algorithms {\n> \n> -class Agc : public Algorithm, public AgcMeanLuminance\n> +class Agc : public Algorithm\n> {\n> public:\n> Agc();\n> @@ -55,6 +55,7 @@ private:\n> IPAFrameContext &frameContext,\n> utils::Duration frameDuration);\n> \n> + AgcMeanLuminance agc_;\n\nYes ;-) ok - so that's what I thought I'd see before the previous patch\n- but I understand the ordering.\n\n\nThere is /so/ much overlap with what I've been trying to work on in my\nspare time lately here - that I think I should offload my spare time\ntasks to you ;-)\n\n\n\n> \n> std::map> meteringModes_;\n> };\n> -- \n> 2.48.1\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 28BA5BEFBE\n\tfor ;\n\tMon, 18 Aug 2025 09:23:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A79969259;\n\tMon, 18 Aug 2025 11:23:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E38626924E\n\tfor ;\n\tMon, 18 Aug 2025 11:23:53 +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 C13EB17D1;\n\tMon, 18 Aug 2025 11:22:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Uju30JGM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755508976;\n\tbh=RO2TuRgPSp3NehRgkjFWp7tOXe/YXlwdNlyOsVhLfyw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Uju30JGMadFGq7sH+NBNInTc8LlVUSCEgMpT21j85GKhK0e3y2syiF4n1Tpw2ix9J\n\tLKQUV+dh8aomxyFFEE2TCiQJhuKX0hgUafsFVINxTbNdJ261RTM6fEzZAYA7KHf7K+\n\tBjyu/wNUsNtWsn5ySyzM7FGc2ZsbC6b/CAgQxBhA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250818082909.2001635-3-stefan.klug@ideasonboard.com>","References":"<20250818082909.2001635-1-stefan.klug@ideasonboard.com>\n\t<20250818082909.2001635-3-stefan.klug@ideasonboard.com>","Subject":"Re: [RFC PATCH 2/4] ipa: rkisp1: agc: Do not derive from\n\tAgcMeanLuminance","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 18 Aug 2025 10:23:50 +0100","Message-ID":"<175550903057.1721288.16662952299252537059@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]