[{"id":31577,"web_url":"https://patchwork.libcamera.org/comment/31577/","msgid":"<20241003212230.GA14035@pendragon.ideasonboard.com>","date":"2024-10-03T21:22:30","subject":"Re: [PATCH v1 7/8] [WIP] ipa: Add debug controls to the agc\n\talgorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Wed, Oct 02, 2024 at 06:19:25PM +0200, Stefan Klug wrote:\n> Add a few (arbitrary) debug metadata to the agc algorithm. This shows\n> how easy it is to add debug metadata to an ipa even for classes that\n> don't have direct access to the metadata list or the context like\n> AgcMeanLuminance. The control_ids_debug.yaml was autogenerated by\n> calling utils/gen-debug-controls.py\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 8 ++++++++\n> src/ipa/libipa/agc_mean_luminance.h | 4 ++++\n> src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++\n> src/libcamera/control_ids_debug.yaml | 17 ++++++++++++++++-\n> 4 files changed, 36 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index f97ef11771c4..bd0f85afcd2e 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -133,6 +133,11 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> * values.\n> */\n> \n> +/**\n> + * \\var AgcMeanLuminance::debugMeta_\n> + * \\brief DebugMetadata helper\n> + */\n> +\n> AgcMeanLuminance::AgcMeanLuminance()\n> \t: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> {\n> @@ -541,8 +546,11 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> \t\texposureModeHelpers_.at(exposureModeIndex);\n> \n> \tdouble gain = estimateInitialGain();\n> +\tdebugMeta_.set(controls::debug::AgcInitialGain, static_cast(gain));\n> \tgain = constraintClampGain(constraintModeIndex, yHist, gain);\n> \n> +\tdebugMeta_.set(controls::debug::AgcNewGain, static_cast(gain));\n> +\n> \t/*\n> \t * We don't check whether we're already close to the target, because\n> \t * even if the effective exposure value is the same as the last frame's\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index 576d28be8eb0..428465a11a36 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -16,6 +16,7 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/debug_controls.h\"\n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> #include \"exposure_mode_helper.h\"\n> @@ -71,6 +72,9 @@ public:\n> \t\tframeCount_ = 0;\n> \t}\n> \n> +protected:\n> +\tDebugMetadata debugMeta_;\n> +\n> private:\n> \tvirtual double estimateLuminance(const double gain) const = 0;\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 17d074d9c03e..2dcee0ceaccf 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -139,6 +139,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> {\n> \tint ret;\n> \n> +\tdebugMeta_.assignUpstream(&context.debugMetadata);\n> +\n> \tret = parseTuningData(tuningData);\n> \tif (ret)\n> \t\treturn ret;\n> @@ -444,6 +446,12 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t\t frameContext.agc.exposureMode,\n> \t\t\t hist, effectiveExposureValue);\n> \n> +\tdebugMeta_.set(controls::debug::AgcAnalogGain, aGain);\n> +\tdebugMeta_.set(controls::debug::AgcDigitalGain, dGain);\n> +\n> +\tconst auto &data = hist.data();\n> +\tdebugMeta_.set>(controls::debug::AgcHistogram, Span(reinterpret_cast(&data[0]), data.size()));\n\nI'd ask you to split this line, but I think you won't like that :-)\n\nI started checking if the length could be reduced, and wondered why you\nspecify the type explicitly as a template argument, as the whole point\nof the control classes is to deduce the type automatically. Writing\n\n\tdebugMeta_.set(controls::debug::AgcAnalogGain, aGain);\n\tdebugMeta_.set(controls::debug::AgcDigitalGain, dGain);\n\ncompiles just fine. Then I realized you need this for the python script\nthat generates the yaml file...\n\n> +\n> \tLOG(RkISP1Agc, Debug)\n> \t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n> \t\t<< shutterTime << \", \" << aGain << \" and \" << dGain;\n> diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n> index 11ddfd7cc8d8..d1da5cca2364 100644\n> --- a/src/libcamera/control_ids_debug.yaml\n> +++ b/src/libcamera/control_ids_debug.yaml\n> @@ -8,4 +8,19 @@\n> # set through Request::controls() and returned out through Request::metadata().\n> vendor: debug\n> controls:\n> -\n> +- AgcAnalogGain:\n> + type: float\n> + description: Debug control AgcAnalogGain found in src/ipa/rkisp1/algorithms/agc.cpp:448\n> +- AgcDigitalGain:\n> + type: float\n> + description: Debug control AgcDigitalGain found in src/ipa/rkisp1/algorithms/agc.cpp:449\n> +- AgcHistogram:\n> + type: int64_t\n> + description: Debug control AgcHistogram found in src/ipa/rkisp1/algorithms/agc.cpp:452\n> + size: '[n]'\n\nAre the quotes needed ?\n\n> +- AgcInitialGain:\n> + type: float\n> + description: Debug control AgcInitialGain found in src/ipa/libipa/agc_mean_luminance.cpp:543\n> +- AgcNewGain:\n> + type: float\n> + description: Debug control AgcNewGain found in src/ipa/libipa/agc_mean_luminance.cpp:546","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 8A300BD80A\n\tfor ;\n\tThu, 3 Oct 2024 21:22:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56A8A63526;\n\tThu, 3 Oct 2024 23:22:35 +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 5BB8760534\n\tfor ;\n\tThu, 3 Oct 2024 23:22:34 +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 C4B6C55;\n\tThu, 3 Oct 2024 23:21:00 +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=\"GWxo/6gg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727990461;\n\tbh=mAa4AKzMjXTRfwR+0HsscPbMGWZTkoHVS8rrP1a4bjU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GWxo/6gghu2YpyDmmMogm9hZ+bb2oXaaiRgHWEo2AmaNOp260KSE3SVQCfbKmQq8G\n\tPBQ9zaw9NgwcULurjuW4Gb+ZtYT1vVh4IA6AI0f8L64Oj86fcW3XHRG0E4n53NDu9U\n\t8Slq9f6TAMwz4CrsNBKuOg1cyQPKIBOp7SwaN96Y=","Date":"Fri, 4 Oct 2024 00:22:30 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 7/8] [WIP] ipa: Add debug controls to the agc\n\talgorithm","Message-ID":"<20241003212230.GA14035@pendragon.ideasonboard.com>","References":"<20241002161933.247091-1-stefan.klug@ideasonboard.com>\n\t<20241002161933.247091-8-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241002161933.247091-8-stefan.klug@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\" "}}]