[PATCH v3 6/9] ipa: rkisp1: Move loader functions into helper class

[Laurent Pinchart]

On Fri, Sep 20, 2024 at 03:39:21PM +0200, Stefan Klug wrote:
> In preparation for supporting polynomial LSC data, move the current
> loader into its own helper class.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> 
> Changes in v3:
> - Renamed LscClassicLoader to LscTableLoader
> - Collected tags
> - Removed type detection from this patch to the one adding the
>   polynomial loader
> ---
>  src/ipa/rkisp1/algorithms/lsc.cpp | 120 +++++++++++++++++-------------
>  1 file changed, 69 insertions(+), 51 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index fe84062bbc70..44c97f0e1a10 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -70,6 +70,70 @@ namespace ipa::rkisp1::algorithms {
>  
>  LOG_DEFINE_CATEGORY(RkISP1Lsc)
>  
> +class LscTableLoader
> +{
> +public:
> +	int parseLscData(const YamlObject &yamlSets,
> +			 std::map<unsigned int, LensShadingCorrection::Components> &lscData)
> +	{

As mentioned in the review of other patches in the series, please don't
make those functions inline. In hinders readability.

> +		const auto &sets = yamlSets.asList();
> +
> +		for (const auto &yamlSet : sets) {
> +			uint32_t ct = yamlSet["ct"].get<uint32_t>(0);
> +
> +			if (lscData.count(ct)) {
> +				LOG(RkISP1Lsc, Error)
> +					<< "Multiple sets found for color temperature "
> +					<< ct;
> +				return -EINVAL;
> +			}
> +
> +			LensShadingCorrection::Components &set = lscData[ct];
> +
> +			set.ct = ct;
> +			set.r = parseTable(yamlSet, "r");
> +			set.gr = parseTable(yamlSet, "gr");
> +			set.gb = parseTable(yamlSet, "gb");
> +			set.b = parseTable(yamlSet, "b");
> +
> +			if (set.r.empty() || set.gr.empty() ||
> +			    set.gb.empty() || set.b.empty()) {
> +				LOG(RkISP1Lsc, Error)
> +					<< "Set for color temperature " << ct
> +					<< " is missing tables";
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (lscData.empty()) {
> +			LOG(RkISP1Lsc, Error) << "Failed to load any sets";
> +			return -EINVAL;
> +		}
> +
> +		return 0;
> +	}
> +
> +private:
> +	std::vector<uint16_t> parseTable(const YamlObject &tuningData,
> +					 const char *prop)
> +	{
> +		static constexpr unsigned int kLscNumSamples =
> +			RKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;
> +
> +		std::vector<uint16_t> table =
> +			tuningData[prop].getList<uint16_t>().value_or(std::vector<uint16_t>{});
> +		if (table.size() != kLscNumSamples) {
> +			LOG(RkISP1Lsc, Error)
> +				<< "Invalid '" << prop << "' values: expected "
> +				<< kLscNumSamples
> +				<< " elements, got " << table.size();
> +			return {};
> +		}
> +
> +		return table;
> +	}
> +};
> +
>  static std::vector<double> parseSizes(const YamlObject &tuningData,
>  				      const char *prop)
>  {
> @@ -100,25 +164,6 @@ static std::vector<double> parseSizes(const YamlObject &tuningData,
>  	return sizes;
>  }
>  
> -static std::vector<uint16_t> parseTable(const YamlObject &tuningData,
> -					const char *prop)
> -{
> -	static constexpr unsigned int kLscNumSamples =
> -		RKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;
> -
> -	std::vector<uint16_t> table =
> -		tuningData[prop].getList<uint16_t>().value_or(std::vector<uint16_t>{});
> -	if (table.size() != kLscNumSamples) {
> -		LOG(RkISP1Lsc, Error)
> -			<< "Invalid '" << prop << "' values: expected "
> -			<< kLscNumSamples
> -			<< " elements, got " << table.size();
> -		return {};
> -	}
> -
> -	return table;
> -}
> -
>  LensShadingCorrection::LensShadingCorrection()
>  	: lastAppliedCt_(0), lastAppliedQuantizedCt_(0)
>  {
> @@ -145,39 +190,12 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>  		return -EINVAL;
>  	}
>  
> -	const auto &sets = yamlSets.asList();
>  	std::map<unsigned int, Components> lscData;
> -	for (const auto &yamlSet : sets) {
> -		uint32_t ct = yamlSet["ct"].get<uint32_t>(0);
> -
> -		if (lscData.count(ct)) {
> -			LOG(RkISP1Lsc, Error)
> -				<< "Multiple sets found for color temperature "
> -				<< ct;
> -			return -EINVAL;
> -		}
> -
> -		Components &set = lscData[ct];
> -
> -		set.ct = ct;
> -		set.r = parseTable(yamlSet, "r");
> -		set.gr = parseTable(yamlSet, "gr");
> -		set.gb = parseTable(yamlSet, "gb");
> -		set.b = parseTable(yamlSet, "b");
> -
> -		if (set.r.empty() || set.gr.empty() ||
> -		    set.gb.empty() || set.b.empty()) {
> -			LOG(RkISP1Lsc, Error)
> -				<< "Set for color temperature " << ct
> -				<< " is missing tables";
> -			return -EINVAL;
> -		}
> -	}
> -
> -	if (lscData.empty()) {
> -		LOG(RkISP1Lsc, Error) << "Failed to load any sets";
> -		return -EINVAL;
> -	}
> +	int res = 0;
> +	auto loader = LscTableLoader();
> +	res = loader.parseLscData(yamlSets, lscData);
> +	if (res)
> +		return res;
>  
>  	sets_.setData(std::move(lscData));
>  
> -- 
> 2.43.0
> 

-- 
Regards,

Laurent Pinchart