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

[Laurent Pinchart]

On Mon, Sep 02, 2024 at 12:39:48PM +0200, Milan Zamazal wrote:
> Laurent Pinchart writes:
> > 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.

To be clear, I meant this:

	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";

I don't know if that's better or worse.

> >>  }
> >>  
> >>  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.

To be clear, I like neither version :-) Whoever invented lambdas in C++
clearly never thought about code formatting.

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

-- 
Regards,

Laurent Pinchart