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

21478 diff mbox series

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@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(-)

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@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