[PATCH v2 06/20] libcamera: rkisp1: Formatting improvements

[Milan Zamazal]

Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Fri, Aug 30, 2024 at 05:27:03PM +0200, Milan Zamazal wrote:
>> The LSP autoformatter doesn't like some of the current formatting, let's
>> make it happy.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/ipa/rkisp1/algorithms/awb.cpp | 18 +++++++++---------
>>  src/ipa/rkisp1/algorithms/dpf.cpp |  4 ++--
>>  src/ipa/rkisp1/rkisp1.cpp         | 20 +++++++++++---------
>>  3 files changed, 22 insertions(+), 20 deletions(-)
>> 
>> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
>> index a5972015..35435fe2 100644
>> --- a/src/ipa/rkisp1/algorithms/awb.cpp
>> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
>> @@ -12,6 +12,7 @@
>>  #include <libcamera/base/log.h>
>>  
>>  #include <libcamera/control_ids.h>
>> +
>
> Ack.
>
>>  #include <libcamera/ipa/core_ipa_interface.h>
>>  
>>  /**
>> @@ -209,10 +210,9 @@ void Awb::process(IPAContext &context,
>>  	double blueMean;
>>  
>>  	metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
>> -	metadata.set(controls::ColourGains, {
>> -			static_cast<float>(frameContext.awb.gains.red),
>> -			static_cast<float>(frameContext.awb.gains.blue)
>> -		});
>> +	metadata.set(controls::ColourGains,
>> +		     { static_cast<float>(frameContext.awb.gains.red),
>> +		       static_cast<float>(frameContext.awb.gains.blue) });
>
> Less readable in my opinion.

I'll revert it.

>>  	if (rgbMode_) {
>>  		greenMean = awb->awb_mean[0].mean_y_or_g;
>> @@ -305,11 +305,11 @@ void Awb::process(IPAContext &context,
>>  	activeState.awb.gains.automatic.green = 1.0;
>>  
>>  	LOG(RkISP1Awb, Debug) << std::showpoint
>> -		<< "Means [" << redMean << ", " << greenMean << ", " << blueMean
>> -		<< "], gains [" << activeState.awb.gains.automatic.red << ", "
>> -		<< activeState.awb.gains.automatic.green << ", "
>> -		<< activeState.awb.gains.automatic.blue << "], temp "
>> -		<< activeState.awb.temperatureK << "K";
>> +			      << "Means [" << redMean << ", " << greenMean << ", " << blueMean
>> +			      << "], gains [" << activeState.awb.gains.automatic.red << ", "
>> +			      << activeState.awb.gains.automatic.green << ", "
>> +			      << activeState.awb.gains.automatic.blue << "], temp "
>> +			      << activeState.awb.temperatureK << "K";
>
> Nack, that's against the coding style. You could move
> '<< std::showpoint' to the next line though if you want.

I will move it since it helps to keep the lines below the recommended
line length limit, without breaking them artificially.

>>  }
>>  
>>  REGISTER_IPA_ALGORITHM(Awb, "Awb")
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
>> index 94080b28..ef1f25d7 100644
>> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
>> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
>> @@ -116,8 +116,8 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>>  	}
>>  
>>  	config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>> -			       ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
>> -			       : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
>> +					 ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
>> +					 : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
>
> Nack.
>
>>  
>>  	std::copy_n(values.begin(), values.size(),
>>  		    std::begin(config_.rb_flt.spatial_coeff));
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 55269579..3c13c82e 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -17,10 +17,11 @@
>>  
>>  #include <libcamera/control_ids.h>
>>  #include <libcamera/framebuffer.h>
>> +#include <libcamera/request.h>
>> +
>>  #include <libcamera/ipa/ipa_interface.h>
>>  #include <libcamera/ipa/ipa_module_info.h>
>>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>> -#include <libcamera/request.h>
>
> Ack.
>
>>  
>>  #include "libcamera/internal/formats.h"
>>  #include "libcamera/internal/mapped_framebuffer.h"
>> @@ -162,8 +163,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>>  		return -ENODEV;
>>  	}
>>  
>> -	context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
>> -						   * 1.0s / sensorInfo.pixelRate;
>> +	context_.configuration.sensor.lineDuration =
>> +		sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;
>
> I prefer the existing formatting slightly, but don't mind the change.

I prefer the changed version, so I'll keep it.

>>  	/* Load the tuning data file. */
>>  	File file(settings.configurationFile);
>> @@ -264,12 +265,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>>  	context_.configuration.sensor.maxAnalogueGain =
>>  		context_.camHelper->gain(maxGain);
>>  
>> -	context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),
>> -		[](auto &cfg) -> bool {
>> -			PixelFormat pixelFormat{ cfg.second.pixelFormat };
>> -			const PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);
>> -			return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>> -		});
>> +	context_.configuration.raw =
>> +		std::any_of(streamConfig.begin(), streamConfig.end(),
>> +			    [](auto &cfg) -> bool {
>> +				    PixelFormat pixelFormat{ cfg.second.pixelFormat };
>> +				    const PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);
>> +				    return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>> +			    });
>
> Formatting lambdas is an absolutely horrible problem :-(

I'll keep the original formatting.

>>  	for (auto const &a : algorithms()) {
>>  		Algorithm *algo = static_cast<Algorithm *>(a.get());