[RFC] ipa: Reformat algorithms in a checkstyle compatible way

28218 diff mbox series

Reformat some code sections in a checkstyle compatible way while trying
to stay within the 80/100 column limit.

Most of the changes are just reformatting.

In libipa::AgcAlgorithm the visitor patterns were reformatted using
intermediate variables.

In rkisp1 Agc::process the code was reordered to spare one indentation
level.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Hi,

There have been several discussions around the question if checkstyle is
mandatory or not with a tendency towards *not mandatory*.

However during my daily work and many rebases it became even more
painful to get constant warnings from checkstyle which eventually
renders the tool useless. So I had a look at the code sections that
popped up recently and tried to reformat them in a checkstyle compatible
way, thereby keeping the indentation levels low. The result is this
patch.

I'd like to get some feedback from the group if these formattings could
be a viable option.

Best regards,
Stefan
---
 src/ipa/ipu3/algorithms/agc.cpp     |  67 ++++++-----
 src/ipa/libipa/agc.cpp              | 174 ++++++++++++++--------------
 src/ipa/mali-c55/algorithms/lsc.cpp |  14 ++-
 src/ipa/rkisp1/algorithms/agc.cpp   | 113 ++++++++++--------
 src/ipa/rkisp1/algorithms/lsc.cpp   |  14 ++-
 5 files changed, 203 insertions(+), 179 deletions(-)

2026. 09. 09. 11:54 keltezéssel, Stefan Klug írta:
> Reformat some code sections in a checkstyle compatible way while trying
> to stay within the 80/100 column limit.
> 
> Most of the changes are just reformatting.
> 
> In libipa::AgcAlgorithm the visitor patterns were reformatted using
> intermediate variables.

I cannot say that I like what I'm seeing, in fact, I really dislike the
separate variables. But I am still of the opinion that a code formatter
is either mandatory or useless.


> 
> In rkisp1 Agc::process the code was reordered to spare one indentation
> level.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Hi,
> 
> There have been several discussions around the question if checkstyle is
> mandatory or not with a tendency towards *not mandatory*.
> 
> However during my daily work and many rebases it became even more
> painful to get constant warnings from checkstyle which eventually
> renders the tool useless. So I had a look at the code sections that
> popped up recently and tried to reformat them in a checkstyle compatible
> way, thereby keeping the indentation levels low. The result is this
> patch.
> 
> I'd like to get some feedback from the group if these formattings could
> be a viable option.
> 
> Best regards,
> Stefan
> ---
>   src/ipa/ipu3/algorithms/agc.cpp     |  67 ++++++-----
>   src/ipa/libipa/agc.cpp              | 174 ++++++++++++++--------------
>   src/ipa/mali-c55/algorithms/lsc.cpp |  14 ++-
>   src/ipa/rkisp1/algorithms/agc.cpp   | 113 ++++++++++--------
>   src/ipa/rkisp1/algorithms/lsc.cpp   |  14 ++-
>   5 files changed, 203 insertions(+), 179 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 57cfb325f1fc..323021cb9a4b 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -67,11 +67,13 @@ Agc::Agc()
>    */
>   int Agc::init(IPAContext &context, const ValueNode &tuningData)
>   {
> -	return agc_.init(tuningData, context.camHelper.get(), {
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -	});
> +	return agc_.init(
> +		tuningData, context.camHelper.get(),
> +		{
> +			.sensorInfo = context.sensorInfo,
> +			.sensorControls = context.sensorControls,
> +			.ctrlMap = context.ctrlMap,
> +		});
>   }
>   
>   /**
> @@ -84,14 +86,16 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>   int Agc::configure(IPAContext &context,
>   		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>   {
> -	stride_ =  context.configuration.grid.stride;
> +	stride_ = context.configuration.grid.stride;
>   	bdsGrid_ = context.configuration.grid.bdsGrid;
>   
> -	return agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -	});
> +	return agc_.configure(
> +		context.configuration.agc, context.activeState.agc,
> +		{
> +			.sensorInfo = context.sensorInfo,
> +			.sensorControls = context.sensorControls,
> +			.ctrlMap = context.ctrlMap,
> +		});
>   }
>   
>   /**
> @@ -129,11 +133,10 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
>   				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>   					&stats->awb_raw_buffer.meta_data[cellPosition]);
>   
> -			rgbTriples_.push_back({
> -				cell->R_avg,
> -				(cell->Gr_avg + cell->Gb_avg) / 2,
> -				cell->B_avg
> -			});
> +			rgbTriples_.push_back(
> +				{ cell->R_avg,
> +				  (cell->Gr_avg + cell->Gb_avg) / 2,
> +				  cell->B_avg });
>   
>   			/*
>   			 * Store the average green value to estimate the
> @@ -219,20 +222,24 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   {
>   	Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid);
>   
> -	agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> -		.traits = AgcTraits{
> -			rgbTriples_,
> -			{{
> -				context.activeState.awb.gains.red,
> -				context.activeState.awb.gains.blue,
> -				context.activeState.awb.gains.green,
> -			}},
> -			bdsGrid_,
> -		},
> -		.yHist = hist,
> -		.exposure = frameContext.sensor.exposure,
> -		.gain = frameContext.sensor.gain,
> -	}}, metadata);
> +	agc_.process(
> +		context.configuration.agc, context.activeState.agc,
> +		frameContext.agc,
> +		{ {
> +			.traits = AgcTraits{
> +				rgbTriples_,
> +				{ {
> +					context.activeState.awb.gains.red,
> +					context.activeState.awb.gains.blue,
> +					context.activeState.awb.gains.green,
> +				} },
> +				bdsGrid_,
> +			},
> +			.yHist = hist,
> +			.exposure = frameContext.sensor.exposure,
> +			.gain = frameContext.sensor.gain,
> +		} },
> +		metadata);

Shouldn't the same thing be done in the mali-c55 ipa module? I imagine
the code there upsets clang-format equally.


>   }
>   
>   REGISTER_IPA_ALGORITHM(Agc, "Agc")
> [...]
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 1f1d6c96a3c6..a4a79cc792a7 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> [...]
> @@ -358,40 +363,48 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   		if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
>   			params = &stats->params;
>   		else
> -			LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> -	}
> -
> -	if (params) {
> -		std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> -		if (context.activeState.wdr.mode != controls::WdrOff)
> -			additionalConstraints.push_back(context.activeState.wdr.constraint);
> -
> -		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> -			.traits = AgcTraits{
> -				{ params->ae.exp_mean, context.hw.numAeCells },
> -				meteringModes_.at(frameContext.agc.meteringMode),
> -			},
> -			.yHist = {
> -				/* The lower 4 bits are fractional and meant to be discarded. */
> -				{ params->hist.hist_bins, context.hw.numHistogramBins },
> -				[](uint32_t x) { return x >> 4; },
> -			},
> -			.exposure = frameContext.sensor.exposure,
> -			/*
> -			 * Include the quantization gain if it was applied. Do not use
> -			 * compress.gain because it will include gains that shall not be
> -			 * reported to the user when HDR is implemented.
> -			 */
> -			.gain = frameContext.sensor.gain
> -			        * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> -			.additionalConstraints = std::move(additionalConstraints),
> -			.lux = frameContext.lux.lux,
> -		}}, metadata);
> -	} else {
> -		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> +			LOG(RkISP1Agc, Error)
> +				<< "AUTOEXP data is missing in statistics";
>   	}
>   
>   	metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> +
> +	if (!params) {
> +		agc_.process(context.configuration.agc, context.activeState.agc,
> +			     frameContext.agc, {}, metadata);
> +		return;
> +	}
> +
> +	std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> +	if (context.activeState.wdr.mode != controls::WdrOff)
> +		additionalConstraints.push_back(context.activeState.wdr.constraint);
> +
> +	double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
> +	std::optional<AgcAlgorithm::ProcessParams> processParams{ {
> +		.traits = AgcTraits{
> +			{ params->ae.exp_mean, context.hw.numAeCells },
> +			meteringModes_.at(frameContext.agc.meteringMode),
> +		},
> +		.yHist = {
> +			/*
> +			 * The lower 4 bits are fractional and meant to be
> +			 * discarded.
> +			 */
> +			{ params->hist.hist_bins, context.hw.numHistogramBins },
> +			[](uint32_t x) { return x >> 4; },
> +		},
> +		.exposure = frameContext.sensor.exposure,
> +		/*
> +		 * Include the quantization gain if it was applied. Do not use
> +		 * compress.gain because it will include gains that shall not be
> +		 * reported to the user when HDR is implemented.
> +		 */
> +		.gain = frameContext.sensor.gain * qgain,
> +		.additionalConstraints = std::move(additionalConstraints),
> +		.lux = frameContext.lux.lux,
> +	} };

This doesn't work like this because `traits` and `yHist` will be dangling references.



> +	agc_.process(context.configuration.agc, context.activeState.agc,
> +		     frameContext.agc, std::move(processParams), metadata);
>   }
>   
>   REGISTER_IPA_ALGORITHM(Agc, "Agc")
> [...]

Quoting Barnabás Pőcze (2026-09-09 12:23:30)
> 2026. 09. 09. 11:54 keltezéssel, Stefan Klug írta:
> > Reformat some code sections in a checkstyle compatible way while trying
> > to stay within the 80/100 column limit.
> > 
> > Most of the changes are just reformatting.
> > 
> > In libipa::AgcAlgorithm the visitor patterns were reformatted using
> > intermediate variables.
> 
> I cannot say that I like what I'm seeing, in fact, I really dislike the
> separate variables. But I am still of the opinion that a code formatter
> is either mandatory or useless.

Really? I actually found it easier to parse that way.
Performance/optimization wise it is no difference, is it?

> 
> 
> > 
> > In rkisp1 Agc::process the code was reordered to spare one indentation
> > level.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > 
> > Hi,
> > 
> > There have been several discussions around the question if checkstyle is
> > mandatory or not with a tendency towards *not mandatory*.
> > 
> > However during my daily work and many rebases it became even more
> > painful to get constant warnings from checkstyle which eventually
> > renders the tool useless. So I had a look at the code sections that
> > popped up recently and tried to reformat them in a checkstyle compatible
> > way, thereby keeping the indentation levels low. The result is this
> > patch.
> > 
> > I'd like to get some feedback from the group if these formattings could
> > be a viable option.
> > 
> > Best regards,
> > Stefan
> > ---
> >   src/ipa/ipu3/algorithms/agc.cpp     |  67 ++++++-----
> >   src/ipa/libipa/agc.cpp              | 174 ++++++++++++++--------------
> >   src/ipa/mali-c55/algorithms/lsc.cpp |  14 ++-
> >   src/ipa/rkisp1/algorithms/agc.cpp   | 113 ++++++++++--------
> >   src/ipa/rkisp1/algorithms/lsc.cpp   |  14 ++-
> >   5 files changed, 203 insertions(+), 179 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index 57cfb325f1fc..323021cb9a4b 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -67,11 +67,13 @@ Agc::Agc()
> >    */
> >   int Agc::init(IPAContext &context, const ValueNode &tuningData)
> >   {
> > -     return agc_.init(tuningData, context.camHelper.get(), {
> > -             .sensorInfo = context.sensorInfo,
> > -             .sensorControls = context.sensorControls,
> > -             .ctrlMap = context.ctrlMap,
> > -     });
> > +     return agc_.init(
> > +             tuningData, context.camHelper.get(),
> > +             {
> > +                     .sensorInfo = context.sensorInfo,
> > +                     .sensorControls = context.sensorControls,
> > +                     .ctrlMap = context.ctrlMap,
> > +             });
> >   }
> >   
> >   /**
> > @@ -84,14 +86,16 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
> >   int Agc::configure(IPAContext &context,
> >                  [[maybe_unused]] const IPAConfigInfo &configInfo)
> >   {
> > -     stride_ =  context.configuration.grid.stride;
> > +     stride_ = context.configuration.grid.stride;
> >       bdsGrid_ = context.configuration.grid.bdsGrid;
> >   
> > -     return agc_.configure(context.configuration.agc, context.activeState.agc, {
> > -             .sensorInfo = context.sensorInfo,
> > -             .sensorControls = context.sensorControls,
> > -             .ctrlMap = context.ctrlMap,
> > -     });
> > +     return agc_.configure(
> > +             context.configuration.agc, context.activeState.agc,
> > +             {
> > +                     .sensorInfo = context.sensorInfo,
> > +                     .sensorControls = context.sensorControls,
> > +                     .ctrlMap = context.ctrlMap,
> > +             });
> >   }
> >   
> >   /**
> > @@ -129,11 +133,10 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
> >                               reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> >                                       &stats->awb_raw_buffer.meta_data[cellPosition]);
> >   
> > -                     rgbTriples_.push_back({
> > -                             cell->R_avg,
> > -                             (cell->Gr_avg + cell->Gb_avg) / 2,
> > -                             cell->B_avg
> > -                     });
> > +                     rgbTriples_.push_back(
> > +                             { cell->R_avg,
> > +                               (cell->Gr_avg + cell->Gb_avg) / 2,
> > +                               cell->B_avg });
> >   
> >                       /*
> >                        * Store the average green value to estimate the
> > @@ -219,20 +222,24 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >   {
> >       Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid);
> >   
> > -     agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> > -             .traits = AgcTraits{
> > -                     rgbTriples_,
> > -                     {{
> > -                             context.activeState.awb.gains.red,
> > -                             context.activeState.awb.gains.blue,
> > -                             context.activeState.awb.gains.green,
> > -                     }},
> > -                     bdsGrid_,
> > -             },
> > -             .yHist = hist,
> > -             .exposure = frameContext.sensor.exposure,
> > -             .gain = frameContext.sensor.gain,
> > -     }}, metadata);
> > +     agc_.process(
> > +             context.configuration.agc, context.activeState.agc,
> > +             frameContext.agc,
> > +             { {
> > +                     .traits = AgcTraits{
> > +                             rgbTriples_,
> > +                             { {
> > +                                     context.activeState.awb.gains.red,
> > +                                     context.activeState.awb.gains.blue,
> > +                                     context.activeState.awb.gains.green,
> > +                             } },
> > +                             bdsGrid_,
> > +                     },
> > +                     .yHist = hist,
> > +                     .exposure = frameContext.sensor.exposure,
> > +                     .gain = frameContext.sensor.gain,
> > +             } },
> > +             metadata);
> 
> Shouldn't the same thing be done in the mali-c55 ipa module? I imagine
> the code there upsets clang-format equally.

Yes, I only picked out a few places. If we can agree an that formatting
I would rerun checkstyle on more places.

> 
> 
> >   }
> >   
> >   REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > [...]
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 1f1d6c96a3c6..a4a79cc792a7 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > [...]
> > @@ -358,40 +363,48 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >               if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
> >                       params = &stats->params;
> >               else
> > -                     LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> > -     }
> > -
> > -     if (params) {
> > -             std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> > -             if (context.activeState.wdr.mode != controls::WdrOff)
> > -                     additionalConstraints.push_back(context.activeState.wdr.constraint);
> > -
> > -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> > -                     .traits = AgcTraits{
> > -                             { params->ae.exp_mean, context.hw.numAeCells },
> > -                             meteringModes_.at(frameContext.agc.meteringMode),
> > -                     },
> > -                     .yHist = {
> > -                             /* The lower 4 bits are fractional and meant to be discarded. */
> > -                             { params->hist.hist_bins, context.hw.numHistogramBins },
> > -                             [](uint32_t x) { return x >> 4; },
> > -                     },
> > -                     .exposure = frameContext.sensor.exposure,
> > -                     /*
> > -                      * Include the quantization gain if it was applied. Do not use
> > -                      * compress.gain because it will include gains that shall not be
> > -                      * reported to the user when HDR is implemented.
> > -                      */
> > -                     .gain = frameContext.sensor.gain
> > -                             * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> > -                     .additionalConstraints = std::move(additionalConstraints),
> > -                     .lux = frameContext.lux.lux,
> > -             }}, metadata);
> > -     } else {
> > -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> > +                     LOG(RkISP1Agc, Error)
> > +                             << "AUTOEXP data is missing in statistics";
> >       }
> >   
> >       metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> > +
> > +     if (!params) {
> > +             agc_.process(context.configuration.agc, context.activeState.agc,
> > +                          frameContext.agc, {}, metadata);
> > +             return;
> > +     }
> > +
> > +     std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> > +     if (context.activeState.wdr.mode != controls::WdrOff)
> > +             additionalConstraints.push_back(context.activeState.wdr.constraint);
> > +
> > +     double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
> > +     std::optional<AgcAlgorithm::ProcessParams> processParams{ {
> > +             .traits = AgcTraits{
> > +                     { params->ae.exp_mean, context.hw.numAeCells },
> > +                     meteringModes_.at(frameContext.agc.meteringMode),
> > +             },
> > +             .yHist = {
> > +                     /*
> > +                      * The lower 4 bits are fractional and meant to be
> > +                      * discarded.
> > +                      */
> > +                     { params->hist.hist_bins, context.hw.numHistogramBins },
> > +                     [](uint32_t x) { return x >> 4; },
> > +             },
> > +             .exposure = frameContext.sensor.exposure,
> > +             /*
> > +              * Include the quantization gain if it was applied. Do not use
> > +              * compress.gain because it will include gains that shall not be
> > +              * reported to the user when HDR is implemented.
> > +              */
> > +             .gain = frameContext.sensor.gain * qgain,
> > +             .additionalConstraints = std::move(additionalConstraints),
> > +             .lux = frameContext.lux.lux,
> > +     } };
> 
> This doesn't work like this because `traits` and `yHist` will be dangling references.

Bummer. Well it compiles :-). This is a situation where I really dislike
C++ and am wondering if yHist and traits should just be normal values
as this has a high potential of misuse. And the Histogram instance is
created anyways. So waht is the benefit of having references here?

But aside from that it could also be formatted as

	std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
	if (context.activeState.wdr.mode != controls::WdrOff)
		additionalConstraints.push_back(context.activeState.wdr.constraint);

	double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
	agc_.process(
		context.configuration.agc, context.activeState.agc,
		frameContext.agc,
		{ {
			.traits = AgcTraits{
				{ params->ae.exp_mean, context.hw.numAeCells },
				meteringModes_.at(frameContext.agc.meteringMode),
			},
			.yHist = {
				/*
				 * The lower 4 bits are fractional and meant to
				 * be discarded.
				 */
				{ params->hist.hist_bins, context.hw.numHistogramBins },
				[](uint32_t x) { return x >> 4; },
			},
			.exposure = frameContext.sensor.exposure,
			/*
			 * Include the quantization gain if it was applied. Do
			 * not use compress.gain because it will include gains
			 * that shall not be reported to the user when HDR is
			 * implemented.
			 */
			.gain = frameContext.sensor.gain * qgain,
			.additionalConstraints = std::move(additionalConstraints),
			.lux = frameContext.lux.lux,
		} },
		metadata);


Best regards,
Stefan

> 
> 
> 
> > +     agc_.process(context.configuration.agc, context.activeState.agc,
> > +                  frameContext.agc, std::move(processParams), metadata);
> >   }
> >   
> >   REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > [...]

2026. 09. 09. 12:52 keltezéssel, Stefan Klug írta:
> Quoting Barnabás Pőcze (2026-09-09 12:23:30)
>> 2026. 09. 09. 11:54 keltezéssel, Stefan Klug írta:
>>> Reformat some code sections in a checkstyle compatible way while trying
>>> to stay within the 80/100 column limit.
>>>
>>> Most of the changes are just reformatting.
>>>
>>> In libipa::AgcAlgorithm the visitor patterns were reformatted using
>>> intermediate variables.
>>
>> I cannot say that I like what I'm seeing, in fact, I really dislike the
>> separate variables. But I am still of the opinion that a code formatter
>> is either mandatory or useless.
> 
> Really? I actually found it easier to parse that way.
> Performance/optimization wise it is no difference, is it?

I feel like I'm too used to how I usually write these. In any case, this is all
irrelevant if we can agree on an automatically enforced mandatory formatting.


> 
>>
>>
>>>
>>> In rkisp1 Agc::process the code was reordered to spare one indentation
>>> level.
>>>
>>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
>>>
>>> ---
>>>
>>> Hi,
>>>
>>> There have been several discussions around the question if checkstyle is
>>> mandatory or not with a tendency towards *not mandatory*.
>>>
>>> However during my daily work and many rebases it became even more
>>> painful to get constant warnings from checkstyle which eventually
>>> renders the tool useless. So I had a look at the code sections that
>>> popped up recently and tried to reformat them in a checkstyle compatible
>>> way, thereby keeping the indentation levels low. The result is this
>>> patch.
>>>
>>> I'd like to get some feedback from the group if these formattings could
>>> be a viable option.
>>>
>>> Best regards,
>>> Stefan
>>> ---
>>>    src/ipa/ipu3/algorithms/agc.cpp     |  67 ++++++-----
>>>    src/ipa/libipa/agc.cpp              | 174 ++++++++++++++--------------
>>>    src/ipa/mali-c55/algorithms/lsc.cpp |  14 ++-
>>>    src/ipa/rkisp1/algorithms/agc.cpp   | 113 ++++++++++--------
>>>    src/ipa/rkisp1/algorithms/lsc.cpp   |  14 ++-
>>>    5 files changed, 203 insertions(+), 179 deletions(-)
>>>
>>> [...]
>>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
>>> index 1f1d6c96a3c6..a4a79cc792a7 100644
>>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>>> [...]
>>> @@ -358,40 +363,48 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>>                if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
>>>                        params = &stats->params;
>>>                else
>>> -                     LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
>>> -     }
>>> -
>>> -     if (params) {
>>> -             std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
>>> -             if (context.activeState.wdr.mode != controls::WdrOff)
>>> -                     additionalConstraints.push_back(context.activeState.wdr.constraint);
>>> -
>>> -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
>>> -                     .traits = AgcTraits{
>>> -                             { params->ae.exp_mean, context.hw.numAeCells },
>>> -                             meteringModes_.at(frameContext.agc.meteringMode),
>>> -                     },
>>> -                     .yHist = {
>>> -                             /* The lower 4 bits are fractional and meant to be discarded. */
>>> -                             { params->hist.hist_bins, context.hw.numHistogramBins },
>>> -                             [](uint32_t x) { return x >> 4; },
>>> -                     },
>>> -                     .exposure = frameContext.sensor.exposure,
>>> -                     /*
>>> -                      * Include the quantization gain if it was applied. Do not use
>>> -                      * compress.gain because it will include gains that shall not be
>>> -                      * reported to the user when HDR is implemented.
>>> -                      */
>>> -                     .gain = frameContext.sensor.gain
>>> -                             * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
>>> -                     .additionalConstraints = std::move(additionalConstraints),
>>> -                     .lux = frameContext.lux.lux,
>>> -             }}, metadata);
>>> -     } else {
>>> -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
>>> +                     LOG(RkISP1Agc, Error)
>>> +                             << "AUTOEXP data is missing in statistics";
>>>        }
>>>    
>>>        metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
>>> +
>>> +     if (!params) {
>>> +             agc_.process(context.configuration.agc, context.activeState.agc,
>>> +                          frameContext.agc, {}, metadata);
>>> +             return;
>>> +     }
>>> +
>>> +     std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
>>> +     if (context.activeState.wdr.mode != controls::WdrOff)
>>> +             additionalConstraints.push_back(context.activeState.wdr.constraint);
>>> +
>>> +     double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
>>> +     std::optional<AgcAlgorithm::ProcessParams> processParams{ {
>>> +             .traits = AgcTraits{
>>> +                     { params->ae.exp_mean, context.hw.numAeCells },
>>> +                     meteringModes_.at(frameContext.agc.meteringMode),
>>> +             },
>>> +             .yHist = {
>>> +                     /*
>>> +                      * The lower 4 bits are fractional and meant to be
>>> +                      * discarded.
>>> +                      */
>>> +                     { params->hist.hist_bins, context.hw.numHistogramBins },
>>> +                     [](uint32_t x) { return x >> 4; },
>>> +             },
>>> +             .exposure = frameContext.sensor.exposure,
>>> +             /*
>>> +              * Include the quantization gain if it was applied. Do not use
>>> +              * compress.gain because it will include gains that shall not be
>>> +              * reported to the user when HDR is implemented.
>>> +              */
>>> +             .gain = frameContext.sensor.gain * qgain,
>>> +             .additionalConstraints = std::move(additionalConstraints),
>>> +             .lux = frameContext.lux.lux,
>>> +     } };
>>
>> This doesn't work like this because `traits` and `yHist` will be dangling references.
> 
> Bummer. Well it compiles :-). This is a situation where I really dislike
> C++ and am wondering if yHist and traits should just be normal values
> as this has a high potential of misuse. And the Histogram instance is
> created anyways. So waht is the benefit of having references here?

`traits` must either be a pointer or reference because of the polymorphism.
`yHist` could be a normal value, but I would try to avoid that because e.g.
in the softisp agc algorithm the same histogram is used in two places,
so it would be nice to be able to avoid having to construct the same thing twice.

They could be separate variables like `qgain`, though.


> 
> But aside from that it could also be formatted as

Given that this is most similar to the proposed formatting in the ipu3 agc
algorithm, I don't see any issues, long lines never bothered me in any case.


> 
> 	std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> 	if (context.activeState.wdr.mode != controls::WdrOff)
> 		additionalConstraints.push_back(context.activeState.wdr.constraint);
> 
> 	double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
> 	agc_.process(
> 		context.configuration.agc, context.activeState.agc,
> 		frameContext.agc,
> 		{ {
> 			.traits = AgcTraits{
> 				{ params->ae.exp_mean, context.hw.numAeCells },
> 				meteringModes_.at(frameContext.agc.meteringMode),
> 			},
> 			.yHist = {
> 				/*
> 				 * The lower 4 bits are fractional and meant to
> 				 * be discarded.
> 				 */
> 				{ params->hist.hist_bins, context.hw.numHistogramBins },
> 				[](uint32_t x) { return x >> 4; },
> 			},
> 			.exposure = frameContext.sensor.exposure,
> 			/*
> 			 * Include the quantization gain if it was applied. Do
> 			 * not use compress.gain because it will include gains
> 			 * that shall not be reported to the user when HDR is
> 			 * implemented.
> 			 */
> 			.gain = frameContext.sensor.gain * qgain,
> 			.additionalConstraints = std::move(additionalConstraints),
> 			.lux = frameContext.lux.lux,
> 		} },
> 		metadata);
> 
> 
> Best regards,
> Stefan
> 
>>
>>
>>
>>> +     agc_.process(context.configuration.agc, context.activeState.agc,
>>> +                  frameContext.agc, std::move(processParams), metadata);
>>>    }
>>>    
>>>    REGISTER_IPA_ALGORITHM(Agc, "Agc")
>>> [...]

Hi Stefan,

On Wed, Sep 09, 2026 at 11:54:31AM +0200, Stefan Klug wrote:
> Reformat some code sections in a checkstyle compatible way while trying
> to stay within the 80/100 column limit.
> 
> Most of the changes are just reformatting.
> 
> In libipa::AgcAlgorithm the visitor patterns were reformatted using
> intermediate variables.
> 
> In rkisp1 Agc::process the code was reordered to spare one indentation
> level.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Hi,
> 
> There have been several discussions around the question if checkstyle is
> mandatory or not with a tendency towards *not mandatory*.
> 
> However during my daily work and many rebases it became even more
> painful to get constant warnings from checkstyle which eventually
> renders the tool useless. So I had a look at the code sections that
> popped up recently and tried to reformat them in a checkstyle compatible
> way, thereby keeping the indentation levels low. The result is this
> patch.
> 
> I'd like to get some feedback from the group if these formattings could
> be a viable option.

This patch combines indentation changes with code refactoring, making it
harder to review. It would help if it was split in two, with the
refactoring first followed by the rest of the changes.

> ---
>  src/ipa/ipu3/algorithms/agc.cpp     |  67 ++++++-----
>  src/ipa/libipa/agc.cpp              | 174 ++++++++++++++--------------
>  src/ipa/mali-c55/algorithms/lsc.cpp |  14 ++-
>  src/ipa/rkisp1/algorithms/agc.cpp   | 113 ++++++++++--------
>  src/ipa/rkisp1/algorithms/lsc.cpp   |  14 ++-
>  5 files changed, 203 insertions(+), 179 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 57cfb325f1fc..323021cb9a4b 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -67,11 +67,13 @@ Agc::Agc()
>   */
>  int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  {
> -	return agc_.init(tuningData, context.camHelper.get(), {
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -	});
> +	return agc_.init(
> +		tuningData, context.camHelper.get(),
> +		{
> +			.sensorInfo = context.sensorInfo,
> +			.sensorControls = context.sensorControls,
> +			.ctrlMap = context.ctrlMap,
> +		});
>  }
>  
>  /**
> @@ -84,14 +86,16 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  int Agc::configure(IPAContext &context,
>  		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
> -	stride_ =  context.configuration.grid.stride;
> +	stride_ = context.configuration.grid.stride;
>  	bdsGrid_ = context.configuration.grid.bdsGrid;
>  
> -	return agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -	});
> +	return agc_.configure(
> +		context.configuration.agc, context.activeState.agc,
> +		{
> +			.sensorInfo = context.sensorInfo,
> +			.sensorControls = context.sensorControls,
> +			.ctrlMap = context.ctrlMap,
> +		});
>  }
>  
>  /**
> @@ -129,11 +133,10 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
>  				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>  					&stats->awb_raw_buffer.meta_data[cellPosition]);
>  
> -			rgbTriples_.push_back({
> -				cell->R_avg,
> -				(cell->Gr_avg + cell->Gb_avg) / 2,
> -				cell->B_avg
> -			});
> +			rgbTriples_.push_back(
> +				{ cell->R_avg,
> +				  (cell->Gr_avg + cell->Gb_avg) / 2,
> +				  cell->B_avg });
>  
>  			/*
>  			 * Store the average green value to estimate the
> @@ -219,20 +222,24 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  {
>  	Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid);
>  
> -	agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> -		.traits = AgcTraits{
> -			rgbTriples_,
> -			{{
> -				context.activeState.awb.gains.red,
> -				context.activeState.awb.gains.blue,
> -				context.activeState.awb.gains.green,
> -			}},
> -			bdsGrid_,
> -		},
> -		.yHist = hist,
> -		.exposure = frameContext.sensor.exposure,
> -		.gain = frameContext.sensor.gain,
> -	}}, metadata);
> +	agc_.process(
> +		context.configuration.agc, context.activeState.agc,
> +		frameContext.agc,
> +		{ {
> +			.traits = AgcTraits{
> +				rgbTriples_,
> +				{ {
> +					context.activeState.awb.gains.red,
> +					context.activeState.awb.gains.blue,
> +					context.activeState.awb.gains.green,
> +				} },
> +				bdsGrid_,
> +			},
> +			.yHist = hist,
> +			.exposure = frameContext.sensor.exposure,
> +			.gain = frameContext.sensor.gain,
> +		} },
> +		metadata);
>  }
>  
>  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 51b05e3c2acd..e1ded0f87097 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -485,51 +485,50 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	add(controls::AnalogueGainMode,
>  	    controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
>  
> -	std::visit(utils::overloaded{
> -		[&](AgcMSV &) {
> -			/* No constraint/exposure mode support. */
> -			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> -			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> +	auto configureMSV = [&](AgcMSV &) {
> +		/* No constraint/exposure mode support. */
> +		state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> +		state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
>  
> -			state.automatic.yTarget = 0; /* Not supported. */
> +		state.automatic.yTarget = 0; /* Not supported. */
>  
> -			if (!session.autoAllowed)
> -				return;
> +		if (!session.autoAllowed)
> +			return;
>  
> -			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(
> -				std::array{ ControlValue(state.constraintMode) }
> -			);
> +		config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(
> +			std::array{ ControlValue(state.constraintMode) });
>  
> -			config.ctrlMap[&controls::AeExposureMode] = ControlInfo(
> -				std::array{ ControlValue(state.exposureMode) }
> -			);
> -		},
> -		[&](AgcMeanLuminance &impl) {
> -			state.constraintMode =
> -				static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> -			state.exposureMode =
> -				static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
> +		config.ctrlMap[&controls::AeExposureMode] = ControlInfo(
> +			std::array{ ControlValue(state.exposureMode) });
> +	};
>  
> -			state.automatic.yTarget = impl.effectiveYTarget(0, 1);
> +	auto configureMeanLuminance = [&](AgcMeanLuminance &impl) {
> +		state.constraintMode =
> +			static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> +		state.exposureMode =
> +			static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
>  
> -			impl.configure(session.lineDuration, sensor_);
> +		state.automatic.yTarget = impl.effectiveYTarget(0, 1);
>  
> -			if (!session.autoAllowed)
> -				return;
> +		impl.configure(session.lineDuration, sensor_);
>  
> -			config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> +		if (!session.autoAllowed)
> +			return;
>  
> -			std::vector<ControlValue> options;
> -			for (const auto &[id, _] : impl.constraintModes())
> -				options.emplace_back(id);
> -			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
> +		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
>  
> -			options.clear();
> -			for (const auto &[id, _] : impl.exposureModeHelpers())
> -				options.emplace_back(id);
> -			config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> -		},
> -	}, impl_);
> +		std::vector<ControlValue> options;
> +		for (const auto &[id, _] : impl.constraintModes())
> +			options.emplace_back(id);
> +		config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
> +
> +		options.clear();
> +		for (const auto &[id, _] : impl.exposureModeHelpers())
> +			options.emplace_back(id);
> +		config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> +	};
> +
> +	std::visit(utils::overloaded{ configureMSV, configureMeanLuminance }, impl_);
>  
>  	return 0;
>  }
> @@ -762,63 +761,64 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  		maxAnalogueGain = frameContext.gain;
>  	}
>  
> -	std::visit(utils::overloaded{
> -		[&](AgcMSV& impl) {
> -			impl.setLimits({
> -				.exposure = {
> -					static_cast<uint32_t>(minExposureTime / lineDuration),
> -					static_cast<uint32_t>(maxExposureTime / lineDuration),
> -				},
> -				.gain = {
> -					minAnalogueGain,
> -					maxAnalogueGain,
> -				},
> -				/* gain codes -> step size of 1 */
> -				.gainMinStep = 1,
> -				/* assume default gain is close to 1.0 */
> -				.gain1 = session.defAnalogueGain,
> -			});
> +	auto processMSV = [&](AgcMSV &impl) {
> +		impl.setLimits({
> +			.exposure = {
> +				static_cast<uint32_t>(minExposureTime / lineDuration),
> +				static_cast<uint32_t>(maxExposureTime / lineDuration),
> +			},
> +			.gain = {
> +				minAnalogueGain,
> +				maxAnalogueGain,
> +			},
> +			/* gain codes -> step size of 1 */
> +			.gainMinStep = 1,
> +			/* assume default gain is close to 1.0 */
> +			.gain1 = session.defAnalogueGain,
> +		});
>  
> -			const auto& newEv = impl.calculateNewEv({
> -				.yHist = params->yHist,
> -				.exposure = params->exposure,
> -				.gain = params->gain,
> -			});
> +		const auto &newEv = impl.calculateNewEv({
> +			.yHist = params->yHist,
> +			.exposure = params->exposure,
> +			.gain = params->gain,
> +		});
>  
> -			state.automatic.exposure = newEv.exposure;
> -			state.automatic.gain = newEv.analogueGain;
> -		},
> -		[&](AgcMeanLuminance& impl) {
> -			/*
> -			 * The Agc algorithm needs to know the effective exposure
> -			 * value that was applied to the sensor when the statistics
> -			 * were collected.
> -			 */
> -			utils::Duration effectiveExposureValue =
> -				lineDuration * params->exposure * params->gain;
> +		state.automatic.exposure = newEv.exposure;
> +		state.automatic.gain = newEv.analogueGain;
> +	};
>  
> -			impl.setLimits(minExposureTime, maxExposureTime,
> -				       minAnalogueGain, maxAnalogueGain,
> -				       std::move(params->additionalConstraints));
> +	auto processMeanLuminance = [&](AgcMeanLuminance &impl) {
> +		/*
> +		 * The Agc algorithm needs to know the effective exposure value
> +		 * that was applied to the sensor when the statistics were
> +		 * collected.
> +		 */
> +		utils::Duration effectiveExposureValue =
> +			lineDuration * params->exposure * params->gain;
>  
> -			const auto &newEv = impl.calculateNewEv({
> -				.traits = params->traits,
> -				.yHist = params->yHist,
> -				.effectiveExposureValue = effectiveExposureValue,
> -				.constraintModeIndex = frameContext.constraintMode,
> -				.exposureModeIndex = frameContext.exposureMode,
> -				.lux = params->lux,
> -				.exposureCompensation = std::pow(2.0, frameContext.exposureValue),
> -			});
> +		impl.setLimits(minExposureTime, maxExposureTime,
> +			       minAnalogueGain, maxAnalogueGain,
> +			       std::move(params->additionalConstraints));
>  
> -			/* Update the estimated exposure and gain. */
> -			state.automatic.exposure = newEv.exposureTime / lineDuration;
> -			state.automatic.gain = newEv.analogueGain;
> -			state.automatic.quantizationGain = newEv.quantizationGain;
> -			state.automatic.digitalGain = newEv.digitalGain;
> -			state.automatic.yTarget = newEv.yTarget;
> -		},
> -	}, impl_);
> +		const auto &newEv = impl.calculateNewEv({
> +			.traits = params->traits,
> +			.yHist = params->yHist,
> +			.effectiveExposureValue = effectiveExposureValue,
> +			.constraintModeIndex = frameContext.constraintMode,
> +			.exposureModeIndex = frameContext.exposureMode,
> +			.lux = params->lux,
> +			.exposureCompensation = std::pow(2.0, frameContext.exposureValue),
> +		});
> +
> +		/* Update the estimated exposure and gain. */
> +		state.automatic.exposure = newEv.exposureTime / lineDuration;
> +		state.automatic.gain = newEv.analogueGain;
> +		state.automatic.quantizationGain = newEv.quantizationGain;
> +		state.automatic.digitalGain = newEv.digitalGain;
> +		state.automatic.yTarget = newEv.yTarget;
> +	};
> +
> +	std::visit(utils::overloaded{ processMSV, processMeanLuminance }, impl_);
>  
>  	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
>  
> diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp
> index fff0dc7d0e64..38c552e712b8 100644
> --- a/src/ipa/mali-c55/algorithms/lsc.cpp
> +++ b/src/ipa/mali-c55/algorithms/lsc.cpp
> @@ -53,12 +53,14 @@ int Lsc::init(IPAContext &context, const ValueNode &tuningData)
>  {
>  	gridPos_ = segmentsToPosition();
>  
> -	return lscAlgo_.init(tuningData, context.ctrlMap, {
> -				.keys = { "r", "g", "b" },
> -				.numHSamples = kMeshSize,
> -				.numVSamples = kMeshSize,
> -				.sensorSize = context.sensorInfo.activeAreaSize
> -			     });
> +	return lscAlgo_.init(
> +		tuningData, context.ctrlMap,
> +		{
> +			.keys = { "r", "g", "b" },
> +			.numHSamples = kMeshSize,
> +			.numVSamples = kMeshSize,
> +			.sensorSize = context.sensorInfo.activeAreaSize,
> +		});
>  }
>  
>  int Lsc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 1f1d6c96a3c6..a4a79cc792a7 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -162,12 +162,14 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>   */
>  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  {
> -	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -		.autoAllowed = !context.configuration.raw,
> -	});
> +	int ret = agc_.configure(
> +		context.configuration.agc, context.activeState.agc,
> +		{
> +			.sensorInfo = context.sensorInfo,
> +			.sensorControls = context.sensorControls,
> +			.ctrlMap = context.ctrlMap,
> +			.autoAllowed = !context.configuration.raw,
> +		});
>  	if (ret)
>  		return ret;
>  
> @@ -266,9 +268,9 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>  
>  	struct rkisp1_cif_isp_window window = hstConfig->meas_window;
>  	Size windowSize = { window.h_size, window.v_size };
> -	hstConfig->histogram_predivider =
> -		computeHistogramPredivider(windowSize,
> -					   static_cast<rkisp1_cif_isp_histogram_mode>(hstConfig->mode));
> +	hstConfig->histogram_predivider = computeHistogramPredivider(
> +		windowSize,
> +		static_cast<rkisp1_cif_isp_histogram_mode>(hstConfig->mode));
>  }
>  
>  namespace {
> @@ -285,19 +287,22 @@ public:
>  	 * \brief Estimate the relative luminance of the frame with a given gain
>  	 * \param[in] gain The gain to apply to the frame
>  	 *
> -	 * This function estimates the average relative luminance of the frame that
> -	 * would be output by the sensor if an additional \a gain was applied.
> +	 * This function estimates the average relative luminance of the frame
> +	 * that would be output by the sensor if an additional \a gain was
> +	 * applied.
>  	 *
>  	 * The estimation is based on the AE statistics for the current frame. Y
> -	 * averages for all cells are first multiplied by the gain, and then saturated
> -	 * to approximate the sensor behaviour at high brightness values. The
> -	 * approximation is quite rough, as it doesn't take into account non-linearities
> -	 * when approaching saturation. In this case, saturating after the conversion to
> -	 * YUV doesn't take into account the fact that the R, G and B components
> -	 * contribute differently to the relative luminance.
> +	 * averages for all cells are first multiplied by the gain, and then
> +	 * saturated to approximate the sensor behaviour at high brightness
> +	 * values. The approximation is quite rough, as it doesn't take into
> +	 * account non-linearities when approaching saturation. In this case,
> +	 * saturating after the conversion to YUV doesn't take into account the
> +	 * fact that the R, G and B components contribute differently to the
> +	 * relative luminance.
>  	 *
> -	 * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
> -	 * theoretical perfect reflector of 100% reference white.
> +	 * The values are normalized to the [0.0, 1.0] range, where 1.0
> +	 * corresponds to a theoretical perfect reflector of 100% reference
> +	 * white.
>  	 *
>  	 * More detailed information can be found in:
>  	 * https://en.wikipedia.org/wiki/Relative_luminance
> @@ -358,40 +363,48 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  		if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
>  			params = &stats->params;
>  		else
> -			LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> -	}
> -
> -	if (params) {
> -		std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> -		if (context.activeState.wdr.mode != controls::WdrOff)
> -			additionalConstraints.push_back(context.activeState.wdr.constraint);
> -
> -		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> -			.traits = AgcTraits{
> -				{ params->ae.exp_mean, context.hw.numAeCells },
> -				meteringModes_.at(frameContext.agc.meteringMode),
> -			},
> -			.yHist = {
> -				/* The lower 4 bits are fractional and meant to be discarded. */
> -				{ params->hist.hist_bins, context.hw.numHistogramBins },
> -				[](uint32_t x) { return x >> 4; },
> -			},
> -			.exposure = frameContext.sensor.exposure,
> -			/*
> -			 * Include the quantization gain if it was applied. Do not use
> -			 * compress.gain because it will include gains that shall not be
> -			 * reported to the user when HDR is implemented.
> -			 */
> -			.gain = frameContext.sensor.gain
> -			        * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> -			.additionalConstraints = std::move(additionalConstraints),
> -			.lux = frameContext.lux.lux,
> -		}}, metadata);
> -	} else {
> -		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> +			LOG(RkISP1Agc, Error)
> +				<< "AUTOEXP data is missing in statistics";
>  	}
>  
>  	metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> +
> +	if (!params) {
> +		agc_.process(context.configuration.agc, context.activeState.agc,
> +			     frameContext.agc, {}, metadata);
> +		return;
> +	}
> +
> +	std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> +	if (context.activeState.wdr.mode != controls::WdrOff)
> +		additionalConstraints.push_back(context.activeState.wdr.constraint);
> +
> +	double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
> +	std::optional<AgcAlgorithm::ProcessParams> processParams{ {
> +		.traits = AgcTraits{
> +			{ params->ae.exp_mean, context.hw.numAeCells },
> +			meteringModes_.at(frameContext.agc.meteringMode),
> +		},
> +		.yHist = {
> +			/*
> +			 * The lower 4 bits are fractional and meant to be
> +			 * discarded.
> +			 */
> +			{ params->hist.hist_bins, context.hw.numHistogramBins },
> +			[](uint32_t x) { return x >> 4; },
> +		},
> +		.exposure = frameContext.sensor.exposure,
> +		/*
> +		 * Include the quantization gain if it was applied. Do not use
> +		 * compress.gain because it will include gains that shall not be
> +		 * reported to the user when HDR is implemented.
> +		 */
> +		.gain = frameContext.sensor.gain * qgain,
> +		.additionalConstraints = std::move(additionalConstraints),
> +		.lux = frameContext.lux.lux,
> +	} };
> +	agc_.process(context.configuration.agc, context.activeState.agc,
> +		     frameContext.agc, std::move(processParams), metadata);
>  }
>  
>  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index 36f9a344ed3b..230fb70f4aa8 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -124,12 +124,14 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>  	xPos_ = sizesListToPositions(xSize_);
>  	yPos_ = sizesListToPositions(ySize_);
>  
> -	return lscAlgo_.init(tuningData,  context.ctrlMap, {
> -				.keys = { "r", "gr", "gb", "b" },
> -				.numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> -				.numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> -				.sensorSize = context.sensorInfo.activeAreaSize
> -			     });
> +	return lscAlgo_.init(
> +		tuningData, context.ctrlMap,
> +		{
> +			.keys = { "r", "gr", "gb", "b" },
> +			.numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> +			.numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> +			.sensorSize = context.sensorInfo.activeAreaSize,
> +		});
>  }
>  
>  /**

On Wed, Sep 09, 2026 at 01:12:47PM +0200, Barnabás Pőcze wrote:
> 2026. 09. 09. 12:52 keltezéssel, Stefan Klug írta:
> > Quoting Barnabás Pőcze (2026-09-09 12:23:30)
> >> 2026. 09. 09. 11:54 keltezéssel, Stefan Klug írta:
> >>> Reformat some code sections in a checkstyle compatible way while trying
> >>> to stay within the 80/100 column limit.
> >>>
> >>> Most of the changes are just reformatting.
> >>>
> >>> In libipa::AgcAlgorithm the visitor patterns were reformatted using
> >>> intermediate variables.
> >>
> >> I cannot say that I like what I'm seeing, in fact, I really dislike the
> >> separate variables. But I am still of the opinion that a code formatter
> >> is either mandatory or useless.
> > 
> > Really? I actually found it easier to parse that way.
> > Performance/optimization wise it is no difference, is it?
> 
> I feel like I'm too used to how I usually write these. In any case, this is all
> irrelevant if we can agree on an automatically enforced mandatory formatting.

It will likely be hard to convince me, but possibly still worth trying.
As it stands, applying clang-format through the whole code base would be
a good way to make me give up on libcamera development.

> >>> In rkisp1 Agc::process the code was reordered to spare one indentation
> >>> level.
> >>>
> >>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> >>>
> >>> ---
> >>>
> >>> Hi,
> >>>
> >>> There have been several discussions around the question if checkstyle is
> >>> mandatory or not with a tendency towards *not mandatory*.
> >>>
> >>> However during my daily work and many rebases it became even more
> >>> painful to get constant warnings from checkstyle which eventually
> >>> renders the tool useless. So I had a look at the code sections that
> >>> popped up recently and tried to reformat them in a checkstyle compatible
> >>> way, thereby keeping the indentation levels low. The result is this
> >>> patch.
> >>>
> >>> I'd like to get some feedback from the group if these formattings could
> >>> be a viable option.
> >>>
> >>> Best regards,
> >>> Stefan
> >>> ---
> >>>    src/ipa/ipu3/algorithms/agc.cpp     |  67 ++++++-----
> >>>    src/ipa/libipa/agc.cpp              | 174 ++++++++++++++--------------
> >>>    src/ipa/mali-c55/algorithms/lsc.cpp |  14 ++-
> >>>    src/ipa/rkisp1/algorithms/agc.cpp   | 113 ++++++++++--------
> >>>    src/ipa/rkisp1/algorithms/lsc.cpp   |  14 ++-
> >>>    5 files changed, 203 insertions(+), 179 deletions(-)
> >>>
> >>> [...]
> >>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> >>> index 1f1d6c96a3c6..a4a79cc792a7 100644
> >>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> >>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> >>> [...]
> >>> @@ -358,40 +363,48 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >>>                if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
> >>>                        params = &stats->params;
> >>>                else
> >>> -                     LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> >>> -     }
> >>> -
> >>> -     if (params) {
> >>> -             std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> >>> -             if (context.activeState.wdr.mode != controls::WdrOff)
> >>> -                     additionalConstraints.push_back(context.activeState.wdr.constraint);
> >>> -
> >>> -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> >>> -                     .traits = AgcTraits{
> >>> -                             { params->ae.exp_mean, context.hw.numAeCells },
> >>> -                             meteringModes_.at(frameContext.agc.meteringMode),
> >>> -                     },
> >>> -                     .yHist = {
> >>> -                             /* The lower 4 bits are fractional and meant to be discarded. */
> >>> -                             { params->hist.hist_bins, context.hw.numHistogramBins },
> >>> -                             [](uint32_t x) { return x >> 4; },
> >>> -                     },
> >>> -                     .exposure = frameContext.sensor.exposure,
> >>> -                     /*
> >>> -                      * Include the quantization gain if it was applied. Do not use
> >>> -                      * compress.gain because it will include gains that shall not be
> >>> -                      * reported to the user when HDR is implemented.
> >>> -                      */
> >>> -                     .gain = frameContext.sensor.gain
> >>> -                             * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> >>> -                     .additionalConstraints = std::move(additionalConstraints),
> >>> -                     .lux = frameContext.lux.lux,
> >>> -             }}, metadata);
> >>> -     } else {
> >>> -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> >>> +                     LOG(RkISP1Agc, Error)
> >>> +                             << "AUTOEXP data is missing in statistics";
> >>>        }
> >>>    
> >>>        metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> >>> +
> >>> +     if (!params) {
> >>> +             agc_.process(context.configuration.agc, context.activeState.agc,
> >>> +                          frameContext.agc, {}, metadata);
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> >>> +     if (context.activeState.wdr.mode != controls::WdrOff)
> >>> +             additionalConstraints.push_back(context.activeState.wdr.constraint);
> >>> +
> >>> +     double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
> >>> +     std::optional<AgcAlgorithm::ProcessParams> processParams{ {
> >>> +             .traits = AgcTraits{
> >>> +                     { params->ae.exp_mean, context.hw.numAeCells },
> >>> +                     meteringModes_.at(frameContext.agc.meteringMode),
> >>> +             },
> >>> +             .yHist = {
> >>> +                     /*
> >>> +                      * The lower 4 bits are fractional and meant to be
> >>> +                      * discarded.
> >>> +                      */
> >>> +                     { params->hist.hist_bins, context.hw.numHistogramBins },
> >>> +                     [](uint32_t x) { return x >> 4; },
> >>> +             },
> >>> +             .exposure = frameContext.sensor.exposure,
> >>> +             /*
> >>> +              * Include the quantization gain if it was applied. Do not use
> >>> +              * compress.gain because it will include gains that shall not be
> >>> +              * reported to the user when HDR is implemented.
> >>> +              */
> >>> +             .gain = frameContext.sensor.gain * qgain,
> >>> +             .additionalConstraints = std::move(additionalConstraints),
> >>> +             .lux = frameContext.lux.lux,
> >>> +     } };
> >>
> >> This doesn't work like this because `traits` and `yHist` will be dangling references.
> > 
> > Bummer. Well it compiles :-). This is a situation where I really dislike
> > C++ and am wondering if yHist and traits should just be normal values
> > as this has a high potential of misuse. And the Histogram instance is
> > created anyways. So waht is the benefit of having references here?
> 
> `traits` must either be a pointer or reference because of the polymorphism.
> `yHist` could be a normal value, but I would try to avoid that because e.g.
> in the softisp agc algorithm the same histogram is used in two places,
> so it would be nice to be able to avoid having to construct the same thing twice.
> 
> They could be separate variables like `qgain`, though.

In addition to pleasing clang-format, separate variables can often
increase readability in my opinion.

> > But aside from that it could also be formatted as
> 
> Given that this is most similar to the proposed formatting in the ipu3 agc
> algorithm, I don't see any issues, long lines never bothered me in any case.
> 
> > 
> > 	std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> > 	if (context.activeState.wdr.mode != controls::WdrOff)
> > 		additionalConstraints.push_back(context.activeState.wdr.constraint);
> > 
> > 	double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
> > 	agc_.process(
> > 		context.configuration.agc, context.activeState.agc,
> > 		frameContext.agc,
> > 		{ {
> > 			.traits = AgcTraits{
> > 				{ params->ae.exp_mean, context.hw.numAeCells },
> > 				meteringModes_.at(frameContext.agc.meteringMode),
> > 			},
> > 			.yHist = {
> > 				/*
> > 				 * The lower 4 bits are fractional and meant to
> > 				 * be discarded.
> > 				 */
> > 				{ params->hist.hist_bins, context.hw.numHistogramBins },
> > 				[](uint32_t x) { return x >> 4; },
> > 			},
> > 			.exposure = frameContext.sensor.exposure,
> > 			/*
> > 			 * Include the quantization gain if it was applied. Do
> > 			 * not use compress.gain because it will include gains
> > 			 * that shall not be reported to the user when HDR is
> > 			 * implemented.
> > 			 */
> > 			.gain = frameContext.sensor.gain * qgain,
> > 			.additionalConstraints = std::move(additionalConstraints),
> > 			.lux = frameContext.lux.lux,
> > 		} },
> > 		metadata);
> > 
> >>> +     agc_.process(context.configuration.agc, context.activeState.agc,
> >>> +                  frameContext.agc, std::move(processParams), metadata);
> >>>    }
> >>>    
> >>>    REGISTER_IPA_ALGORITHM(Agc, "Agc")
> >>> [...]

Hi Laurent,

Quoting Laurent Pinchart (2026-09-09 13:27:45)
> Hi Stefan,
> 
> On Wed, Sep 09, 2026 at 11:54:31AM +0200, Stefan Klug wrote:
> > Reformat some code sections in a checkstyle compatible way while trying
> > to stay within the 80/100 column limit.
> > 
> > Most of the changes are just reformatting.
> > 
> > In libipa::AgcAlgorithm the visitor patterns were reformatted using
> > intermediate variables.
> > 
> > In rkisp1 Agc::process the code was reordered to spare one indentation
> > level.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > 
> > Hi,
> > 
> > There have been several discussions around the question if checkstyle is
> > mandatory or not with a tendency towards *not mandatory*.
> > 
> > However during my daily work and many rebases it became even more
> > painful to get constant warnings from checkstyle which eventually
> > renders the tool useless. So I had a look at the code sections that
> > popped up recently and tried to reformat them in a checkstyle compatible
> > way, thereby keeping the indentation levels low. The result is this
> > patch.
> > 
> > I'd like to get some feedback from the group if these formattings could
> > be a viable option.
> 
> This patch combines indentation changes with code refactoring, making it
> harder to review. It would help if it was split in two, with the
> refactoring first followed by the rest of the changes.

Sure I can and will change that. But that was not the intended point here. My
question was more "do people dislike the formatting of this patch?" as I
tried to create a formatting that passes checkstyle but still better
fits the libcamera style.

Do you have comments on the formatting?

Best regards,
Stefan

> 
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp     |  67 ++++++-----
> >  src/ipa/libipa/agc.cpp              | 174 ++++++++++++++--------------
> >  src/ipa/mali-c55/algorithms/lsc.cpp |  14 ++-
> >  src/ipa/rkisp1/algorithms/agc.cpp   | 113 ++++++++++--------
> >  src/ipa/rkisp1/algorithms/lsc.cpp   |  14 ++-
> >  5 files changed, 203 insertions(+), 179 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index 57cfb325f1fc..323021cb9a4b 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -67,11 +67,13 @@ Agc::Agc()
> >   */
> >  int Agc::init(IPAContext &context, const ValueNode &tuningData)
> >  {
> > -     return agc_.init(tuningData, context.camHelper.get(), {
> > -             .sensorInfo = context.sensorInfo,
> > -             .sensorControls = context.sensorControls,
> > -             .ctrlMap = context.ctrlMap,
> > -     });
> > +     return agc_.init(
> > +             tuningData, context.camHelper.get(),
> > +             {
> > +                     .sensorInfo = context.sensorInfo,
> > +                     .sensorControls = context.sensorControls,
> > +                     .ctrlMap = context.ctrlMap,
> > +             });
> >  }
> >  
> >  /**
> > @@ -84,14 +86,16 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
> >  int Agc::configure(IPAContext &context,
> >                  [[maybe_unused]] const IPAConfigInfo &configInfo)
> >  {
> > -     stride_ =  context.configuration.grid.stride;
> > +     stride_ = context.configuration.grid.stride;
> >       bdsGrid_ = context.configuration.grid.bdsGrid;
> >  
> > -     return agc_.configure(context.configuration.agc, context.activeState.agc, {
> > -             .sensorInfo = context.sensorInfo,
> > -             .sensorControls = context.sensorControls,
> > -             .ctrlMap = context.ctrlMap,
> > -     });
> > +     return agc_.configure(
> > +             context.configuration.agc, context.activeState.agc,
> > +             {
> > +                     .sensorInfo = context.sensorInfo,
> > +                     .sensorControls = context.sensorControls,
> > +                     .ctrlMap = context.ctrlMap,
> > +             });
> >  }
> >  
> >  /**
> > @@ -129,11 +133,10 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
> >                               reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> >                                       &stats->awb_raw_buffer.meta_data[cellPosition]);
> >  
> > -                     rgbTriples_.push_back({
> > -                             cell->R_avg,
> > -                             (cell->Gr_avg + cell->Gb_avg) / 2,
> > -                             cell->B_avg
> > -                     });
> > +                     rgbTriples_.push_back(
> > +                             { cell->R_avg,
> > +                               (cell->Gr_avg + cell->Gb_avg) / 2,
> > +                               cell->B_avg });
> >  
> >                       /*
> >                        * Store the average green value to estimate the
> > @@ -219,20 +222,24 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  {
> >       Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid);
> >  
> > -     agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> > -             .traits = AgcTraits{
> > -                     rgbTriples_,
> > -                     {{
> > -                             context.activeState.awb.gains.red,
> > -                             context.activeState.awb.gains.blue,
> > -                             context.activeState.awb.gains.green,
> > -                     }},
> > -                     bdsGrid_,
> > -             },
> > -             .yHist = hist,
> > -             .exposure = frameContext.sensor.exposure,
> > -             .gain = frameContext.sensor.gain,
> > -     }}, metadata);
> > +     agc_.process(
> > +             context.configuration.agc, context.activeState.agc,
> > +             frameContext.agc,
> > +             { {
> > +                     .traits = AgcTraits{
> > +                             rgbTriples_,
> > +                             { {
> > +                                     context.activeState.awb.gains.red,
> > +                                     context.activeState.awb.gains.blue,
> > +                                     context.activeState.awb.gains.green,
> > +                             } },
> > +                             bdsGrid_,
> > +                     },
> > +                     .yHist = hist,
> > +                     .exposure = frameContext.sensor.exposure,
> > +                     .gain = frameContext.sensor.gain,
> > +             } },
> > +             metadata);
> >  }
> >  
> >  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> > index 51b05e3c2acd..e1ded0f87097 100644
> > --- a/src/ipa/libipa/agc.cpp
> > +++ b/src/ipa/libipa/agc.cpp
> > @@ -485,51 +485,50 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
> >       add(controls::AnalogueGainMode,
> >           controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
> >  
> > -     std::visit(utils::overloaded{
> > -             [&](AgcMSV &) {
> > -                     /* No constraint/exposure mode support. */
> > -                     state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> > -                     state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> > +     auto configureMSV = [&](AgcMSV &) {
> > +             /* No constraint/exposure mode support. */
> > +             state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> > +             state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> >  
> > -                     state.automatic.yTarget = 0; /* Not supported. */
> > +             state.automatic.yTarget = 0; /* Not supported. */
> >  
> > -                     if (!session.autoAllowed)
> > -                             return;
> > +             if (!session.autoAllowed)
> > +                     return;
> >  
> > -                     config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(
> > -                             std::array{ ControlValue(state.constraintMode) }
> > -                     );
> > +             config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(
> > +                     std::array{ ControlValue(state.constraintMode) });
> >  
> > -                     config.ctrlMap[&controls::AeExposureMode] = ControlInfo(
> > -                             std::array{ ControlValue(state.exposureMode) }
> > -                     );
> > -             },
> > -             [&](AgcMeanLuminance &impl) {
> > -                     state.constraintMode =
> > -                             static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> > -                     state.exposureMode =
> > -                             static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
> > +             config.ctrlMap[&controls::AeExposureMode] = ControlInfo(
> > +                     std::array{ ControlValue(state.exposureMode) });
> > +     };
> >  
> > -                     state.automatic.yTarget = impl.effectiveYTarget(0, 1);
> > +     auto configureMeanLuminance = [&](AgcMeanLuminance &impl) {
> > +             state.constraintMode =
> > +                     static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> > +             state.exposureMode =
> > +                     static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
> >  
> > -                     impl.configure(session.lineDuration, sensor_);
> > +             state.automatic.yTarget = impl.effectiveYTarget(0, 1);
> >  
> > -                     if (!session.autoAllowed)
> > -                             return;
> > +             impl.configure(session.lineDuration, sensor_);
> >  
> > -                     config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> > +             if (!session.autoAllowed)
> > +                     return;
> >  
> > -                     std::vector<ControlValue> options;
> > -                     for (const auto &[id, _] : impl.constraintModes())
> > -                             options.emplace_back(id);
> > -                     config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
> > +             config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> >  
> > -                     options.clear();
> > -                     for (const auto &[id, _] : impl.exposureModeHelpers())
> > -                             options.emplace_back(id);
> > -                     config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> > -             },
> > -     }, impl_);
> > +             std::vector<ControlValue> options;
> > +             for (const auto &[id, _] : impl.constraintModes())
> > +                     options.emplace_back(id);
> > +             config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
> > +
> > +             options.clear();
> > +             for (const auto &[id, _] : impl.exposureModeHelpers())
> > +                     options.emplace_back(id);
> > +             config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> > +     };
> > +
> > +     std::visit(utils::overloaded{ configureMSV, configureMeanLuminance }, impl_);
> >  
> >       return 0;
> >  }
> > @@ -762,63 +761,64 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> >               maxAnalogueGain = frameContext.gain;
> >       }
> >  
> > -     std::visit(utils::overloaded{
> > -             [&](AgcMSV& impl) {
> > -                     impl.setLimits({
> > -                             .exposure = {
> > -                                     static_cast<uint32_t>(minExposureTime / lineDuration),
> > -                                     static_cast<uint32_t>(maxExposureTime / lineDuration),
> > -                             },
> > -                             .gain = {
> > -                                     minAnalogueGain,
> > -                                     maxAnalogueGain,
> > -                             },
> > -                             /* gain codes -> step size of 1 */
> > -                             .gainMinStep = 1,
> > -                             /* assume default gain is close to 1.0 */
> > -                             .gain1 = session.defAnalogueGain,
> > -                     });
> > +     auto processMSV = [&](AgcMSV &impl) {
> > +             impl.setLimits({
> > +                     .exposure = {
> > +                             static_cast<uint32_t>(minExposureTime / lineDuration),
> > +                             static_cast<uint32_t>(maxExposureTime / lineDuration),
> > +                     },
> > +                     .gain = {
> > +                             minAnalogueGain,
> > +                             maxAnalogueGain,
> > +                     },
> > +                     /* gain codes -> step size of 1 */
> > +                     .gainMinStep = 1,
> > +                     /* assume default gain is close to 1.0 */
> > +                     .gain1 = session.defAnalogueGain,
> > +             });
> >  
> > -                     const auto& newEv = impl.calculateNewEv({
> > -                             .yHist = params->yHist,
> > -                             .exposure = params->exposure,
> > -                             .gain = params->gain,
> > -                     });
> > +             const auto &newEv = impl.calculateNewEv({
> > +                     .yHist = params->yHist,
> > +                     .exposure = params->exposure,
> > +                     .gain = params->gain,
> > +             });
> >  
> > -                     state.automatic.exposure = newEv.exposure;
> > -                     state.automatic.gain = newEv.analogueGain;
> > -             },
> > -             [&](AgcMeanLuminance& impl) {
> > -                     /*
> > -                      * The Agc algorithm needs to know the effective exposure
> > -                      * value that was applied to the sensor when the statistics
> > -                      * were collected.
> > -                      */
> > -                     utils::Duration effectiveExposureValue =
> > -                             lineDuration * params->exposure * params->gain;
> > +             state.automatic.exposure = newEv.exposure;
> > +             state.automatic.gain = newEv.analogueGain;
> > +     };
> >  
> > -                     impl.setLimits(minExposureTime, maxExposureTime,
> > -                                    minAnalogueGain, maxAnalogueGain,
> > -                                    std::move(params->additionalConstraints));
> > +     auto processMeanLuminance = [&](AgcMeanLuminance &impl) {
> > +             /*
> > +              * The Agc algorithm needs to know the effective exposure value
> > +              * that was applied to the sensor when the statistics were
> > +              * collected.
> > +              */
> > +             utils::Duration effectiveExposureValue =
> > +                     lineDuration * params->exposure * params->gain;
> >  
> > -                     const auto &newEv = impl.calculateNewEv({
> > -                             .traits = params->traits,
> > -                             .yHist = params->yHist,
> > -                             .effectiveExposureValue = effectiveExposureValue,
> > -                             .constraintModeIndex = frameContext.constraintMode,
> > -                             .exposureModeIndex = frameContext.exposureMode,
> > -                             .lux = params->lux,
> > -                             .exposureCompensation = std::pow(2.0, frameContext.exposureValue),
> > -                     });
> > +             impl.setLimits(minExposureTime, maxExposureTime,
> > +                            minAnalogueGain, maxAnalogueGain,
> > +                            std::move(params->additionalConstraints));
> >  
> > -                     /* Update the estimated exposure and gain. */
> > -                     state.automatic.exposure = newEv.exposureTime / lineDuration;
> > -                     state.automatic.gain = newEv.analogueGain;
> > -                     state.automatic.quantizationGain = newEv.quantizationGain;
> > -                     state.automatic.digitalGain = newEv.digitalGain;
> > -                     state.automatic.yTarget = newEv.yTarget;
> > -             },
> > -     }, impl_);
> > +             const auto &newEv = impl.calculateNewEv({
> > +                     .traits = params->traits,
> > +                     .yHist = params->yHist,
> > +                     .effectiveExposureValue = effectiveExposureValue,
> > +                     .constraintModeIndex = frameContext.constraintMode,
> > +                     .exposureModeIndex = frameContext.exposureMode,
> > +                     .lux = params->lux,
> > +                     .exposureCompensation = std::pow(2.0, frameContext.exposureValue),
> > +             });
> > +
> > +             /* Update the estimated exposure and gain. */
> > +             state.automatic.exposure = newEv.exposureTime / lineDuration;
> > +             state.automatic.gain = newEv.analogueGain;
> > +             state.automatic.quantizationGain = newEv.quantizationGain;
> > +             state.automatic.digitalGain = newEv.digitalGain;
> > +             state.automatic.yTarget = newEv.yTarget;
> > +     };
> > +
> > +     std::visit(utils::overloaded{ processMSV, processMeanLuminance }, impl_);
> >  
> >       const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
> >  
> > diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp
> > index fff0dc7d0e64..38c552e712b8 100644
> > --- a/src/ipa/mali-c55/algorithms/lsc.cpp
> > +++ b/src/ipa/mali-c55/algorithms/lsc.cpp
> > @@ -53,12 +53,14 @@ int Lsc::init(IPAContext &context, const ValueNode &tuningData)
> >  {
> >       gridPos_ = segmentsToPosition();
> >  
> > -     return lscAlgo_.init(tuningData, context.ctrlMap, {
> > -                             .keys = { "r", "g", "b" },
> > -                             .numHSamples = kMeshSize,
> > -                             .numVSamples = kMeshSize,
> > -                             .sensorSize = context.sensorInfo.activeAreaSize
> > -                          });
> > +     return lscAlgo_.init(
> > +             tuningData, context.ctrlMap,
> > +             {
> > +                     .keys = { "r", "g", "b" },
> > +                     .numHSamples = kMeshSize,
> > +                     .numVSamples = kMeshSize,
> > +                     .sensorSize = context.sensorInfo.activeAreaSize,
> > +             });
> >  }
> >  
> >  int Lsc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 1f1d6c96a3c6..a4a79cc792a7 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -162,12 +162,14 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
> >   */
> >  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >  {
> > -     int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> > -             .sensorInfo = context.sensorInfo,
> > -             .sensorControls = context.sensorControls,
> > -             .ctrlMap = context.ctrlMap,
> > -             .autoAllowed = !context.configuration.raw,
> > -     });
> > +     int ret = agc_.configure(
> > +             context.configuration.agc, context.activeState.agc,
> > +             {
> > +                     .sensorInfo = context.sensorInfo,
> > +                     .sensorControls = context.sensorControls,
> > +                     .ctrlMap = context.ctrlMap,
> > +                     .autoAllowed = !context.configuration.raw,
> > +             });
> >       if (ret)
> >               return ret;
> >  
> > @@ -266,9 +268,9 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
> >  
> >       struct rkisp1_cif_isp_window window = hstConfig->meas_window;
> >       Size windowSize = { window.h_size, window.v_size };
> > -     hstConfig->histogram_predivider =
> > -             computeHistogramPredivider(windowSize,
> > -                                        static_cast<rkisp1_cif_isp_histogram_mode>(hstConfig->mode));
> > +     hstConfig->histogram_predivider = computeHistogramPredivider(
> > +             windowSize,
> > +             static_cast<rkisp1_cif_isp_histogram_mode>(hstConfig->mode));
> >  }
> >  
> >  namespace {
> > @@ -285,19 +287,22 @@ public:
> >        * \brief Estimate the relative luminance of the frame with a given gain
> >        * \param[in] gain The gain to apply to the frame
> >        *
> > -      * This function estimates the average relative luminance of the frame that
> > -      * would be output by the sensor if an additional \a gain was applied.
> > +      * This function estimates the average relative luminance of the frame
> > +      * that would be output by the sensor if an additional \a gain was
> > +      * applied.
> >        *
> >        * The estimation is based on the AE statistics for the current frame. Y
> > -      * averages for all cells are first multiplied by the gain, and then saturated
> > -      * to approximate the sensor behaviour at high brightness values. The
> > -      * approximation is quite rough, as it doesn't take into account non-linearities
> > -      * when approaching saturation. In this case, saturating after the conversion to
> > -      * YUV doesn't take into account the fact that the R, G and B components
> > -      * contribute differently to the relative luminance.
> > +      * averages for all cells are first multiplied by the gain, and then
> > +      * saturated to approximate the sensor behaviour at high brightness
> > +      * values. The approximation is quite rough, as it doesn't take into
> > +      * account non-linearities when approaching saturation. In this case,
> > +      * saturating after the conversion to YUV doesn't take into account the
> > +      * fact that the R, G and B components contribute differently to the
> > +      * relative luminance.
> >        *
> > -      * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
> > -      * theoretical perfect reflector of 100% reference white.
> > +      * The values are normalized to the [0.0, 1.0] range, where 1.0
> > +      * corresponds to a theoretical perfect reflector of 100% reference
> > +      * white.
> >        *
> >        * More detailed information can be found in:
> >        * https://en.wikipedia.org/wiki/Relative_luminance
> > @@ -358,40 +363,48 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >               if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
> >                       params = &stats->params;
> >               else
> > -                     LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> > -     }
> > -
> > -     if (params) {
> > -             std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> > -             if (context.activeState.wdr.mode != controls::WdrOff)
> > -                     additionalConstraints.push_back(context.activeState.wdr.constraint);
> > -
> > -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> > -                     .traits = AgcTraits{
> > -                             { params->ae.exp_mean, context.hw.numAeCells },
> > -                             meteringModes_.at(frameContext.agc.meteringMode),
> > -                     },
> > -                     .yHist = {
> > -                             /* The lower 4 bits are fractional and meant to be discarded. */
> > -                             { params->hist.hist_bins, context.hw.numHistogramBins },
> > -                             [](uint32_t x) { return x >> 4; },
> > -                     },
> > -                     .exposure = frameContext.sensor.exposure,
> > -                     /*
> > -                      * Include the quantization gain if it was applied. Do not use
> > -                      * compress.gain because it will include gains that shall not be
> > -                      * reported to the user when HDR is implemented.
> > -                      */
> > -                     .gain = frameContext.sensor.gain
> > -                             * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> > -                     .additionalConstraints = std::move(additionalConstraints),
> > -                     .lux = frameContext.lux.lux,
> > -             }}, metadata);
> > -     } else {
> > -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> > +                     LOG(RkISP1Agc, Error)
> > +                             << "AUTOEXP data is missing in statistics";
> >       }
> >  
> >       metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> > +
> > +     if (!params) {
> > +             agc_.process(context.configuration.agc, context.activeState.agc,
> > +                          frameContext.agc, {}, metadata);
> > +             return;
> > +     }
> > +
> > +     std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> > +     if (context.activeState.wdr.mode != controls::WdrOff)
> > +             additionalConstraints.push_back(context.activeState.wdr.constraint);
> > +
> > +     double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
> > +     std::optional<AgcAlgorithm::ProcessParams> processParams{ {
> > +             .traits = AgcTraits{
> > +                     { params->ae.exp_mean, context.hw.numAeCells },
> > +                     meteringModes_.at(frameContext.agc.meteringMode),
> > +             },
> > +             .yHist = {
> > +                     /*
> > +                      * The lower 4 bits are fractional and meant to be
> > +                      * discarded.
> > +                      */
> > +                     { params->hist.hist_bins, context.hw.numHistogramBins },
> > +                     [](uint32_t x) { return x >> 4; },
> > +             },
> > +             .exposure = frameContext.sensor.exposure,
> > +             /*
> > +              * Include the quantization gain if it was applied. Do not use
> > +              * compress.gain because it will include gains that shall not be
> > +              * reported to the user when HDR is implemented.
> > +              */
> > +             .gain = frameContext.sensor.gain * qgain,
> > +             .additionalConstraints = std::move(additionalConstraints),
> > +             .lux = frameContext.lux.lux,
> > +     } };
> > +     agc_.process(context.configuration.agc, context.activeState.agc,
> > +                  frameContext.agc, std::move(processParams), metadata);
> >  }
> >  
> >  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > index 36f9a344ed3b..230fb70f4aa8 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > @@ -124,12 +124,14 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> >       xPos_ = sizesListToPositions(xSize_);
> >       yPos_ = sizesListToPositions(ySize_);
> >  
> > -     return lscAlgo_.init(tuningData,  context.ctrlMap, {
> > -                             .keys = { "r", "gr", "gb", "b" },
> > -                             .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > -                             .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > -                             .sensorSize = context.sensorInfo.activeAreaSize
> > -                          });
> > +     return lscAlgo_.init(
> > +             tuningData, context.ctrlMap,
> > +             {
> > +                     .keys = { "r", "gr", "gb", "b" },
> > +                     .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > +                     .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > +                     .sensorSize = context.sensorInfo.activeAreaSize,
> > +             });
> >  }
> >  
> >  /**
> 
> -- 
> Regards,
> 
> Laurent Pinchart

On Wed, Sep 09, 2026 at 02:51:57PM +0200, Stefan Klug wrote:
> Quoting Laurent Pinchart (2026-09-09 13:27:45)
> > On Wed, Sep 09, 2026 at 11:54:31AM +0200, Stefan Klug wrote:
> > > Reformat some code sections in a checkstyle compatible way while trying
> > > to stay within the 80/100 column limit.
> > > 
> > > Most of the changes are just reformatting.
> > > 
> > > In libipa::AgcAlgorithm the visitor patterns were reformatted using
> > > intermediate variables.
> > > 
> > > In rkisp1 Agc::process the code was reordered to spare one indentation
> > > level.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Hi,
> > > 
> > > There have been several discussions around the question if checkstyle is
> > > mandatory or not with a tendency towards *not mandatory*.
> > > 
> > > However during my daily work and many rebases it became even more
> > > painful to get constant warnings from checkstyle which eventually
> > > renders the tool useless. So I had a look at the code sections that
> > > popped up recently and tried to reformat them in a checkstyle compatible
> > > way, thereby keeping the indentation levels low. The result is this
> > > patch.
> > > 
> > > I'd like to get some feedback from the group if these formattings could
> > > be a viable option.
> > 
> > This patch combines indentation changes with code refactoring, making it
> > harder to review. It would help if it was split in two, with the
> > refactoring first followed by the rest of the changes.
> 
> Sure I can and will change that. But that was not the intended point here. My
> question was more "do people dislike the formatting of this patch?" as I
> tried to create a formatting that passes checkstyle but still better
> fits the libcamera style.
> 
> Do you have comments on the formatting?

Not going further than the first change, ...

> > > ---
> > >  src/ipa/ipu3/algorithms/agc.cpp     |  67 ++++++-----
> > >  src/ipa/libipa/agc.cpp              | 174 ++++++++++++++--------------
> > >  src/ipa/mali-c55/algorithms/lsc.cpp |  14 ++-
> > >  src/ipa/rkisp1/algorithms/agc.cpp   | 113 ++++++++++--------
> > >  src/ipa/rkisp1/algorithms/lsc.cpp   |  14 ++-
> > >  5 files changed, 203 insertions(+), 179 deletions(-)
> > > 
> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > index 57cfb325f1fc..323021cb9a4b 100644
> > > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > > @@ -67,11 +67,13 @@ Agc::Agc()
> > >   */
> > >  int Agc::init(IPAContext &context, const ValueNode &tuningData)
> > >  {
> > > -     return agc_.init(tuningData, context.camHelper.get(), {
> > > -             .sensorInfo = context.sensorInfo,
> > > -             .sensorControls = context.sensorControls,
> > > -             .ctrlMap = context.ctrlMap,
> > > -     });
> > > +     return agc_.init(
> > > +             tuningData, context.camHelper.get(),
> > > +             {
> > > +                     .sensorInfo = context.sensorInfo,
> > > +                     .sensorControls = context.sensorControls,
> > > +                     .ctrlMap = context.ctrlMap,
> > > +             });

... I don't like this. The original code isn't great either though. A
local variable would probably help quite a bit. The same applies to many
locations below.

> > >  }
> > >  
> > >  /**
> > > @@ -84,14 +86,16 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
> > >  int Agc::configure(IPAContext &context,
> > >                  [[maybe_unused]] const IPAConfigInfo &configInfo)
> > >  {
> > > -     stride_ =  context.configuration.grid.stride;
> > > +     stride_ = context.configuration.grid.stride;
> > >       bdsGrid_ = context.configuration.grid.bdsGrid;

I'm fine with not aligning '=' operators here, but in some other cases
alignment improves readability. I wouldn't want to forbid alignment
everywhere unconditionally.

> > >  
> > > -     return agc_.configure(context.configuration.agc, context.activeState.agc, {
> > > -             .sensorInfo = context.sensorInfo,
> > > -             .sensorControls = context.sensorControls,
> > > -             .ctrlMap = context.ctrlMap,
> > > -     });
> > > +     return agc_.configure(
> > > +             context.configuration.agc, context.activeState.agc,
> > > +             {
> > > +                     .sensorInfo = context.sensorInfo,
> > > +                     .sensorControls = context.sensorControls,
> > > +                     .ctrlMap = context.ctrlMap,
> > > +             });
> > >  }
> > >  
> > >  /**
> > > @@ -129,11 +133,10 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
> > >                               reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> > >                                       &stats->awb_raw_buffer.meta_data[cellPosition]);
> > >  
> > > -                     rgbTriples_.push_back({
> > > -                             cell->R_avg,
> > > -                             (cell->Gr_avg + cell->Gb_avg) / 2,
> > > -                             cell->B_avg
> > > -                     });
> > > +                     rgbTriples_.push_back(
> > > +                             { cell->R_avg,
> > > +                               (cell->Gr_avg + cell->Gb_avg) / 2,
> > > +                               cell->B_avg });

This is also not nice.

> > >  
> > >                       /*
> > >                        * Store the average green value to estimate the
> > > @@ -219,20 +222,24 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >  {
> > >       Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid);
> > >  
> > > -     agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> > > -             .traits = AgcTraits{
> > > -                     rgbTriples_,
> > > -                     {{
> > > -                             context.activeState.awb.gains.red,
> > > -                             context.activeState.awb.gains.blue,
> > > -                             context.activeState.awb.gains.green,
> > > -                     }},
> > > -                     bdsGrid_,
> > > -             },
> > > -             .yHist = hist,
> > > -             .exposure = frameContext.sensor.exposure,
> > > -             .gain = frameContext.sensor.gain,
> > > -     }}, metadata);
> > > +     agc_.process(
> > > +             context.configuration.agc, context.activeState.agc,
> > > +             frameContext.agc,
> > > +             { {

Standardizing on spaces between curly brackets is probably fine. We use
both styles currently. We could also possibly standardize on no space,
there's likely a clang-format configuration option for that.

> > > +                     .traits = AgcTraits{
> > > +                             rgbTriples_,
> > > +                             { {
> > > +                                     context.activeState.awb.gains.red,
> > > +                                     context.activeState.awb.gains.blue,
> > > +                                     context.activeState.awb.gains.green,
> > > +                             } },
> > > +                             bdsGrid_,
> > > +                     },
> > > +                     .yHist = hist,
> > > +                     .exposure = frameContext.sensor.exposure,
> > > +                     .gain = frameContext.sensor.gain,
> > > +             } },
> > > +             metadata);
> > >  }
> > >  
> > >  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> > > index 51b05e3c2acd..e1ded0f87097 100644
> > > --- a/src/ipa/libipa/agc.cpp
> > > +++ b/src/ipa/libipa/agc.cpp
> > > @@ -485,51 +485,50 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
> > >       add(controls::AnalogueGainMode,
> > >           controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
> > >  
> > > -     std::visit(utils::overloaded{
> > > -             [&](AgcMSV &) {
> > > -                     /* No constraint/exposure mode support. */
> > > -                     state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> > > -                     state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> > > +     auto configureMSV = [&](AgcMSV &) {
> > > +             /* No constraint/exposure mode support. */
> > > +             state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> > > +             state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> > >  
> > > -                     state.automatic.yTarget = 0; /* Not supported. */
> > > +             state.automatic.yTarget = 0; /* Not supported. */
> > >  
> > > -                     if (!session.autoAllowed)
> > > -                             return;
> > > +             if (!session.autoAllowed)
> > > +                     return;
> > >  
> > > -                     config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(
> > > -                             std::array{ ControlValue(state.constraintMode) }
> > > -                     );
> > > +             config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(
> > > +                     std::array{ ControlValue(state.constraintMode) });
> > >  
> > > -                     config.ctrlMap[&controls::AeExposureMode] = ControlInfo(
> > > -                             std::array{ ControlValue(state.exposureMode) }
> > > -                     );
> > > -             },
> > > -             [&](AgcMeanLuminance &impl) {
> > > -                     state.constraintMode =
> > > -                             static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> > > -                     state.exposureMode =
> > > -                             static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
> > > +             config.ctrlMap[&controls::AeExposureMode] = ControlInfo(
> > > +                     std::array{ ControlValue(state.exposureMode) });
> > > +     };
> > >  
> > > -                     state.automatic.yTarget = impl.effectiveYTarget(0, 1);
> > > +     auto configureMeanLuminance = [&](AgcMeanLuminance &impl) {
> > > +             state.constraintMode =
> > > +                     static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> > > +             state.exposureMode =
> > > +                     static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
> > >  
> > > -                     impl.configure(session.lineDuration, sensor_);
> > > +             state.automatic.yTarget = impl.effectiveYTarget(0, 1);
> > >  
> > > -                     if (!session.autoAllowed)
> > > -                             return;
> > > +             impl.configure(session.lineDuration, sensor_);
> > >  
> > > -                     config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> > > +             if (!session.autoAllowed)
> > > +                     return;
> > >  
> > > -                     std::vector<ControlValue> options;
> > > -                     for (const auto &[id, _] : impl.constraintModes())
> > > -                             options.emplace_back(id);
> > > -                     config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
> > > +             config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> > >  
> > > -                     options.clear();
> > > -                     for (const auto &[id, _] : impl.exposureModeHelpers())
> > > -                             options.emplace_back(id);
> > > -                     config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> > > -             },
> > > -     }, impl_);
> > > +             std::vector<ControlValue> options;
> > > +             for (const auto &[id, _] : impl.constraintModes())
> > > +                     options.emplace_back(id);
> > > +             config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
> > > +
> > > +             options.clear();
> > > +             for (const auto &[id, _] : impl.exposureModeHelpers())
> > > +                     options.emplace_back(id);
> > > +             config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> > > +     };
> > > +
> > > +     std::visit(utils::overloaded{ configureMSV, configureMeanLuminance }, impl_);

Note that in this case I don't like std::visit() and usage of a variant.
Refactoring to use irtual functions in the base class would be better.
Given that std::visit() leads to less readable code that clang-format
doesn't improve, the best option could be to avoid std::visit() when
possible.

> > >  
> > >       return 0;
> > >  }
> > > @@ -762,63 +761,64 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > >               maxAnalogueGain = frameContext.gain;
> > >       }
> > >  
> > > -     std::visit(utils::overloaded{
> > > -             [&](AgcMSV& impl) {
> > > -                     impl.setLimits({
> > > -                             .exposure = {
> > > -                                     static_cast<uint32_t>(minExposureTime / lineDuration),
> > > -                                     static_cast<uint32_t>(maxExposureTime / lineDuration),
> > > -                             },
> > > -                             .gain = {
> > > -                                     minAnalogueGain,
> > > -                                     maxAnalogueGain,
> > > -                             },
> > > -                             /* gain codes -> step size of 1 */
> > > -                             .gainMinStep = 1,
> > > -                             /* assume default gain is close to 1.0 */
> > > -                             .gain1 = session.defAnalogueGain,
> > > -                     });
> > > +     auto processMSV = [&](AgcMSV &impl) {
> > > +             impl.setLimits({
> > > +                     .exposure = {
> > > +                             static_cast<uint32_t>(minExposureTime / lineDuration),
> > > +                             static_cast<uint32_t>(maxExposureTime / lineDuration),
> > > +                     },
> > > +                     .gain = {
> > > +                             minAnalogueGain,
> > > +                             maxAnalogueGain,
> > > +                     },
> > > +                     /* gain codes -> step size of 1 */
> > > +                     .gainMinStep = 1,
> > > +                     /* assume default gain is close to 1.0 */
> > > +                     .gain1 = session.defAnalogueGain,
> > > +             });
> > >  
> > > -                     const auto& newEv = impl.calculateNewEv({
> > > -                             .yHist = params->yHist,
> > > -                             .exposure = params->exposure,
> > > -                             .gain = params->gain,
> > > -                     });
> > > +             const auto &newEv = impl.calculateNewEv({
> > > +                     .yHist = params->yHist,
> > > +                     .exposure = params->exposure,
> > > +                     .gain = params->gain,
> > > +             });
> > >  
> > > -                     state.automatic.exposure = newEv.exposure;
> > > -                     state.automatic.gain = newEv.analogueGain;
> > > -             },
> > > -             [&](AgcMeanLuminance& impl) {
> > > -                     /*
> > > -                      * The Agc algorithm needs to know the effective exposure
> > > -                      * value that was applied to the sensor when the statistics
> > > -                      * were collected.
> > > -                      */
> > > -                     utils::Duration effectiveExposureValue =
> > > -                             lineDuration * params->exposure * params->gain;
> > > +             state.automatic.exposure = newEv.exposure;
> > > +             state.automatic.gain = newEv.analogueGain;
> > > +     };
> > >  
> > > -                     impl.setLimits(minExposureTime, maxExposureTime,
> > > -                                    minAnalogueGain, maxAnalogueGain,
> > > -                                    std::move(params->additionalConstraints));
> > > +     auto processMeanLuminance = [&](AgcMeanLuminance &impl) {
> > > +             /*
> > > +              * The Agc algorithm needs to know the effective exposure value
> > > +              * that was applied to the sensor when the statistics were
> > > +              * collected.
> > > +              */
> > > +             utils::Duration effectiveExposureValue =
> > > +                     lineDuration * params->exposure * params->gain;
> > >  
> > > -                     const auto &newEv = impl.calculateNewEv({
> > > -                             .traits = params->traits,
> > > -                             .yHist = params->yHist,
> > > -                             .effectiveExposureValue = effectiveExposureValue,
> > > -                             .constraintModeIndex = frameContext.constraintMode,
> > > -                             .exposureModeIndex = frameContext.exposureMode,
> > > -                             .lux = params->lux,
> > > -                             .exposureCompensation = std::pow(2.0, frameContext.exposureValue),
> > > -                     });
> > > +             impl.setLimits(minExposureTime, maxExposureTime,
> > > +                            minAnalogueGain, maxAnalogueGain,
> > > +                            std::move(params->additionalConstraints));
> > >  
> > > -                     /* Update the estimated exposure and gain. */
> > > -                     state.automatic.exposure = newEv.exposureTime / lineDuration;
> > > -                     state.automatic.gain = newEv.analogueGain;
> > > -                     state.automatic.quantizationGain = newEv.quantizationGain;
> > > -                     state.automatic.digitalGain = newEv.digitalGain;
> > > -                     state.automatic.yTarget = newEv.yTarget;
> > > -             },
> > > -     }, impl_);
> > > +             const auto &newEv = impl.calculateNewEv({
> > > +                     .traits = params->traits,
> > > +                     .yHist = params->yHist,
> > > +                     .effectiveExposureValue = effectiveExposureValue,
> > > +                     .constraintModeIndex = frameContext.constraintMode,
> > > +                     .exposureModeIndex = frameContext.exposureMode,
> > > +                     .lux = params->lux,
> > > +                     .exposureCompensation = std::pow(2.0, frameContext.exposureValue),
> > > +             });
> > > +
> > > +             /* Update the estimated exposure and gain. */
> > > +             state.automatic.exposure = newEv.exposureTime / lineDuration;
> > > +             state.automatic.gain = newEv.analogueGain;
> > > +             state.automatic.quantizationGain = newEv.quantizationGain;
> > > +             state.automatic.digitalGain = newEv.digitalGain;
> > > +             state.automatic.yTarget = newEv.yTarget;
> > > +     };
> > > +
> > > +     std::visit(utils::overloaded{ processMSV, processMeanLuminance }, impl_);
> > >  
> > >       const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
> > >  
> > > diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp
> > > index fff0dc7d0e64..38c552e712b8 100644
> > > --- a/src/ipa/mali-c55/algorithms/lsc.cpp
> > > +++ b/src/ipa/mali-c55/algorithms/lsc.cpp
> > > @@ -53,12 +53,14 @@ int Lsc::init(IPAContext &context, const ValueNode &tuningData)
> > >  {
> > >       gridPos_ = segmentsToPosition();
> > >  
> > > -     return lscAlgo_.init(tuningData, context.ctrlMap, {
> > > -                             .keys = { "r", "g", "b" },
> > > -                             .numHSamples = kMeshSize,
> > > -                             .numVSamples = kMeshSize,
> > > -                             .sensorSize = context.sensorInfo.activeAreaSize
> > > -                          });
> > > +     return lscAlgo_.init(
> > > +             tuningData, context.ctrlMap,
> > > +             {
> > > +                     .keys = { "r", "g", "b" },
> > > +                     .numHSamples = kMeshSize,
> > > +                     .numVSamples = kMeshSize,
> > > +                     .sensorSize = context.sensorInfo.activeAreaSize,
> > > +             });
> > >  }
> > >  
> > >  int Lsc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 1f1d6c96a3c6..a4a79cc792a7 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -162,12 +162,14 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
> > >   */
> > >  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >  {
> > > -     int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> > > -             .sensorInfo = context.sensorInfo,
> > > -             .sensorControls = context.sensorControls,
> > > -             .ctrlMap = context.ctrlMap,
> > > -             .autoAllowed = !context.configuration.raw,
> > > -     });
> > > +     int ret = agc_.configure(
> > > +             context.configuration.agc, context.activeState.agc,
> > > +             {
> > > +                     .sensorInfo = context.sensorInfo,
> > > +                     .sensorControls = context.sensorControls,
> > > +                     .ctrlMap = context.ctrlMap,
> > > +                     .autoAllowed = !context.configuration.raw,
> > > +             });
> > >       if (ret)
> > >               return ret;
> > >  
> > > @@ -266,9 +268,9 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
> > >  
> > >       struct rkisp1_cif_isp_window window = hstConfig->meas_window;
> > >       Size windowSize = { window.h_size, window.v_size };
> > > -     hstConfig->histogram_predivider =
> > > -             computeHistogramPredivider(windowSize,
> > > -                                        static_cast<rkisp1_cif_isp_histogram_mode>(hstConfig->mode));
> > > +     hstConfig->histogram_predivider = computeHistogramPredivider(
> > > +             windowSize,
> > > +             static_cast<rkisp1_cif_isp_histogram_mode>(hstConfig->mode));
> > >  }
> > >  
> > >  namespace {
> > > @@ -285,19 +287,22 @@ public:
> > >        * \brief Estimate the relative luminance of the frame with a given gain
> > >        * \param[in] gain The gain to apply to the frame
> > >        *
> > > -      * This function estimates the average relative luminance of the frame that
> > > -      * would be output by the sensor if an additional \a gain was applied.
> > > +      * This function estimates the average relative luminance of the frame
> > > +      * that would be output by the sensor if an additional \a gain was
> > > +      * applied.
> > >        *
> > >        * The estimation is based on the AE statistics for the current frame. Y
> > > -      * averages for all cells are first multiplied by the gain, and then saturated
> > > -      * to approximate the sensor behaviour at high brightness values. The
> > > -      * approximation is quite rough, as it doesn't take into account non-linearities
> > > -      * when approaching saturation. In this case, saturating after the conversion to
> > > -      * YUV doesn't take into account the fact that the R, G and B components
> > > -      * contribute differently to the relative luminance.
> > > +      * averages for all cells are first multiplied by the gain, and then
> > > +      * saturated to approximate the sensor behaviour at high brightness
> > > +      * values. The approximation is quite rough, as it doesn't take into
> > > +      * account non-linearities when approaching saturation. In this case,
> > > +      * saturating after the conversion to YUV doesn't take into account the
> > > +      * fact that the R, G and B components contribute differently to the
> > > +      * relative luminance.
> > >        *
> > > -      * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
> > > -      * theoretical perfect reflector of 100% reference white.
> > > +      * The values are normalized to the [0.0, 1.0] range, where 1.0
> > > +      * corresponds to a theoretical perfect reflector of 100% reference
> > > +      * white.
> > >        *
> > >        * More detailed information can be found in:
> > >        * https://en.wikipedia.org/wiki/Relative_luminance
> > > @@ -358,40 +363,48 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >               if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
> > >                       params = &stats->params;
> > >               else
> > > -                     LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> > > -     }
> > > -
> > > -     if (params) {
> > > -             std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> > > -             if (context.activeState.wdr.mode != controls::WdrOff)
> > > -                     additionalConstraints.push_back(context.activeState.wdr.constraint);
> > > -
> > > -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> > > -                     .traits = AgcTraits{
> > > -                             { params->ae.exp_mean, context.hw.numAeCells },
> > > -                             meteringModes_.at(frameContext.agc.meteringMode),
> > > -                     },
> > > -                     .yHist = {
> > > -                             /* The lower 4 bits are fractional and meant to be discarded. */
> > > -                             { params->hist.hist_bins, context.hw.numHistogramBins },
> > > -                             [](uint32_t x) { return x >> 4; },
> > > -                     },
> > > -                     .exposure = frameContext.sensor.exposure,
> > > -                     /*
> > > -                      * Include the quantization gain if it was applied. Do not use
> > > -                      * compress.gain because it will include gains that shall not be
> > > -                      * reported to the user when HDR is implemented.
> > > -                      */
> > > -                     .gain = frameContext.sensor.gain
> > > -                             * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> > > -                     .additionalConstraints = std::move(additionalConstraints),
> > > -                     .lux = frameContext.lux.lux,
> > > -             }}, metadata);
> > > -     } else {
> > > -             agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> > > +                     LOG(RkISP1Agc, Error)
> > > +                             << "AUTOEXP data is missing in statistics";
> > >       }
> > >  
> > >       metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> > > +
> > > +     if (!params) {
> > > +             agc_.process(context.configuration.agc, context.activeState.agc,
> > > +                          frameContext.agc, {}, metadata);
> > > +             return;
> > > +     }
> > > +
> > > +     std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> > > +     if (context.activeState.wdr.mode != controls::WdrOff)
> > > +             additionalConstraints.push_back(context.activeState.wdr.constraint);
> > > +
> > > +     double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1);
> > > +     std::optional<AgcAlgorithm::ProcessParams> processParams{ {
> > > +             .traits = AgcTraits{
> > > +                     { params->ae.exp_mean, context.hw.numAeCells },
> > > +                     meteringModes_.at(frameContext.agc.meteringMode),
> > > +             },
> > > +             .yHist = {
> > > +                     /*
> > > +                      * The lower 4 bits are fractional and meant to be
> > > +                      * discarded.
> > > +                      */
> > > +                     { params->hist.hist_bins, context.hw.numHistogramBins },
> > > +                     [](uint32_t x) { return x >> 4; },
> > > +             },
> > > +             .exposure = frameContext.sensor.exposure,
> > > +             /*
> > > +              * Include the quantization gain if it was applied. Do not use
> > > +              * compress.gain because it will include gains that shall not be
> > > +              * reported to the user when HDR is implemented.
> > > +              */
> > > +             .gain = frameContext.sensor.gain * qgain,
> > > +             .additionalConstraints = std::move(additionalConstraints),
> > > +             .lux = frameContext.lux.lux,
> > > +     } };
> > > +     agc_.process(context.configuration.agc, context.activeState.agc,
> > > +                  frameContext.agc, std::move(processParams), metadata);
> > >  }
> > >  
> > >  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > > index 36f9a344ed3b..230fb70f4aa8 100644
> > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > > @@ -124,12 +124,14 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> > >       xPos_ = sizesListToPositions(xSize_);
> > >       yPos_ = sizesListToPositions(ySize_);
> > >  
> > > -     return lscAlgo_.init(tuningData,  context.ctrlMap, {
> > > -                             .keys = { "r", "gr", "gb", "b" },
> > > -                             .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > > -                             .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > > -                             .sensorSize = context.sensorInfo.activeAreaSize
> > > -                          });
> > > +     return lscAlgo_.init(
> > > +             tuningData, context.ctrlMap,
> > > +             {
> > > +                     .keys = { "r", "gr", "gb", "b" },
> > > +                     .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > > +                     .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > > +                     .sensorSize = context.sensorInfo.activeAreaSize,
> > > +             });
> > >  }
> > >  
> > >  /**