[PATCH v1 7/8] [WIP] ipa: Add debug controls to the agc algorithm

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Wed, Oct 02, 2024 at 06:19:25PM +0200, Stefan Klug wrote:
> Add a few (arbitrary) debug metadata to the agc algorithm. This shows
> how easy it is to add debug metadata to an ipa even for classes that
> don't have direct access to the metadata list or the context like
> AgcMeanLuminance. The control_ids_debug.yaml was autogenerated by
> calling utils/gen-debug-controls.py
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp |  8 ++++++++
>  src/ipa/libipa/agc_mean_luminance.h   |  4 ++++
>  src/ipa/rkisp1/algorithms/agc.cpp     |  8 ++++++++
>  src/libcamera/control_ids_debug.yaml  | 17 ++++++++++++++++-
>  4 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index f97ef11771c4..bd0f85afcd2e 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -133,6 +133,11 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>   * values.
>   */
>  
> +/**
> + * \var AgcMeanLuminance::debugMeta_
> + * \brief DebugMetadata helper
> + */
> +
>  AgcMeanLuminance::AgcMeanLuminance()
>  	: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
>  {
> @@ -541,8 +546,11 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>  		exposureModeHelpers_.at(exposureModeIndex);
>  
>  	double gain = estimateInitialGain();
> +	debugMeta_.set<float>(controls::debug::AgcInitialGain, static_cast<float>(gain));
>  	gain = constraintClampGain(constraintModeIndex, yHist, gain);
>  
> +	debugMeta_.set<float>(controls::debug::AgcNewGain, static_cast<float>(gain));
> +
>  	/*
>  	 * We don't check whether we're already close to the target, because
>  	 * even if the effective exposure value is the same as the last frame's
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index 576d28be8eb0..428465a11a36 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -16,6 +16,7 @@
>  
>  #include <libcamera/controls.h>
>  
> +#include "libcamera/internal/debug_controls.h"
>  #include "libcamera/internal/yaml_parser.h"
>  
>  #include "exposure_mode_helper.h"
> @@ -71,6 +72,9 @@ public:
>  		frameCount_ = 0;
>  	}
>  
> +protected:
> +	DebugMetadata debugMeta_;
> +
>  private:
>  	virtual double estimateLuminance(const double gain) const = 0;
>  
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 17d074d9c03e..2dcee0ceaccf 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -139,6 +139,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>  {
>  	int ret;
>  
> +	debugMeta_.assignUpstream(&context.debugMetadata);
> +
>  	ret = parseTuningData(tuningData);
>  	if (ret)
>  		return ret;
> @@ -444,6 +446,12 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  			       frameContext.agc.exposureMode,
>  			       hist, effectiveExposureValue);
>  
> +	debugMeta_.set<float>(controls::debug::AgcAnalogGain, aGain);
> +	debugMeta_.set<float>(controls::debug::AgcDigitalGain, dGain);
> +
> +	const auto &data = hist.data();
> +	debugMeta_.set<Span<const int64_t>>(controls::debug::AgcHistogram, Span(reinterpret_cast<const int64_t *>(&data[0]), data.size()));

I'd ask you to split this line, but I think you won't like that :-)

I started checking if the length could be reduced, and wondered why you
specify the type explicitly as a template argument, as the whole point
of the control classes is to deduce the type automatically. Writing

	debugMeta_.set(controls::debug::AgcAnalogGain, aGain);
	debugMeta_.set(controls::debug::AgcDigitalGain, dGain);

compiles just fine. Then I realized you need this for the python script
that generates the yaml file...

> +
>  	LOG(RkISP1Agc, Debug)
>  		<< "Divided up shutter, analogue gain and digital gain are "
>  		<< shutterTime << ", " << aGain << " and " << dGain;
> diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml
> index 11ddfd7cc8d8..d1da5cca2364 100644
> --- a/src/libcamera/control_ids_debug.yaml
> +++ b/src/libcamera/control_ids_debug.yaml
> @@ -8,4 +8,19 @@
>  # set through Request::controls() and returned out through Request::metadata().
>  vendor: debug
>  controls:
> -
> +- AgcAnalogGain:
> +    type: float
> +    description: Debug control AgcAnalogGain found in src/ipa/rkisp1/algorithms/agc.cpp:448
> +- AgcDigitalGain:
> +    type: float
> +    description: Debug control AgcDigitalGain found in src/ipa/rkisp1/algorithms/agc.cpp:449
> +- AgcHistogram:
> +    type: int64_t
> +    description: Debug control AgcHistogram found in src/ipa/rkisp1/algorithms/agc.cpp:452
> +    size: '[n]'

Are the quotes needed ?

> +- AgcInitialGain:
> +    type: float
> +    description: Debug control AgcInitialGain found in src/ipa/libipa/agc_mean_luminance.cpp:543
> +- AgcNewGain:
> +    type: float
> +    description: Debug control AgcNewGain found in src/ipa/libipa/agc_mean_luminance.cpp:546

-- 
Regards,

Laurent Pinchart