[v3,20/21] ipa: rppx1: lsc: Add

28358 diff mbox series

From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Add the algorithm to the rppx1 ipa module based on the corresponding
algorithm in the rkisp1 ipa module.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/rppx1/algorithms/lsc.cpp     | 272 +++++++++++++++++++++++++++
 src/ipa/rppx1/algorithms/lsc.h       |  58 ++++++
 src/ipa/rppx1/algorithms/meson.build |   1 +
 src/ipa/rppx1/ipa_context.h          |   5 +
 src/ipa/rppx1/params.h               |   4 +
 5 files changed, 340 insertions(+)
 create mode 100644 src/ipa/rppx1/algorithms/lsc.cpp
 create mode 100644 src/ipa/rppx1/algorithms/lsc.h

Hi Barnabás

On Fri, Sep 18, 2026 at 02:09:48PM +0200, Barnabás Pőcze wrote:
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Add the algorithm to the rppx1 ipa module based on the corresponding
> algorithm in the rkisp1 ipa module.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/rppx1/algorithms/lsc.cpp     | 272 +++++++++++++++++++++++++++
>  src/ipa/rppx1/algorithms/lsc.h       |  58 ++++++
>  src/ipa/rppx1/algorithms/meson.build |   1 +
>  src/ipa/rppx1/ipa_context.h          |   5 +
>  src/ipa/rppx1/params.h               |   4 +
>  5 files changed, 340 insertions(+)
>  create mode 100644 src/ipa/rppx1/algorithms/lsc.cpp
>  create mode 100644 src/ipa/rppx1/algorithms/lsc.h
>
> diff --git a/src/ipa/rppx1/algorithms/lsc.cpp b/src/ipa/rppx1/algorithms/lsc.cpp
> new file mode 100644
> index 0000000000..1b0dbc792d
> --- /dev/null
> +++ b/src/ipa/rppx1/algorithms/lsc.cpp
> @@ -0,0 +1,272 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2026, Ideas On Board
> + *
> + * RPP-X1 Lens Shading Correction control
> + */
> +
> +#include "lsc.h"
> +
> +#include <algorithm>
> +#include <cmath>
> +#include <numeric>
> +#include <span>
> +#include <string.h>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/utils.h>
> +
> +#include "libcamera/internal/value_node.h"
> +
> +/**
> + * \file lsc.h
> + */
> +
> +namespace libcamera {
> +
> +namespace ipa::rppx1::algorithms {
> +
> +LOG_DEFINE_CATEGORY(RppX1Lsc)
> +
> +namespace {
> +
> +constexpr int kColourTemperatureQuantization = 10;
> +
> +std::vector<double> parseSizes(const ValueNode &tuningData,
> +			       const char *prop)
> +{
> +	std::vector<double> sizes =
> +		tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
> +	if (sizes.size() != RPPX1_LSC_NUM_SECTORS) {
> +		LOG(RppX1Lsc, Error)
> +			<< "Invalid '" << prop << "' values: expected "
> +			<< RPPX1_LSC_NUM_SECTORS
> +			<< " elements, got " << sizes.size();
> +		return {};
> +	}
> +
> +	/*
> +	 * The sum of all elements must satisfy hardware constraints.
> +	 * Validate it here, allowing a 1% tolerance as rounding errors may
> +	 * prevent an exact match (further adjustments will be performed in
> +	 * LensShadingCorrection::prepare()).
> +	 */
> +	constexpr double expectedSum = 1.0;
> +
> +	double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
> +	if (std::abs(sum - expectedSum) > 0.01) {
> +		LOG(RppX1Lsc, Error)
> +			<< "Invalid '" << prop << "' values: sum of the elements"
> +			<< " should be " << expectedSum << ", got " << sum;
> +		return {};
> +	}
> +
> +	const auto &[min, max] = std::minmax_element(sizes.begin(), sizes.end());
> +	if (*min <= 0 || *max > 1) {
> +		LOG(RppX1Lsc, Error)
> +			<< "Invalid '" << prop << "' values: elements must be in (0;1]";
> +		return {};
> +	}
> +
> +	return sizes;
> +}
> +
> +std::vector<double> sizesListToPositions(std::span<const double> sizes)
> +{
> +	std::vector<double> positions(sizes.size() + 1);
> +
> +	positions[0] = 0;
> +	for (size_t i = 0; i < sizes.size(); i++)
> +		positions[i + 1] = positions[i] + sizes[i];
> +
> +	return positions;
> +}
> +
> +unsigned int quantize(unsigned int value, unsigned int step)
> +{
> +	return std::lround(value / static_cast<double>(step)) * step;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::init
> + */
> +int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> +				const ValueNode &tuningData)
> +{
> +	xSize_ = parseSizes(tuningData, "x-size");
> +	ySize_ = parseSizes(tuningData, "y-size");
> +
> +	if (xSize_.empty() || ySize_.empty())
> +		return -EINVAL;
> +
> +	xPos_ = sizesListToPositions(xSize_);
> +	yPos_ = sizesListToPositions(ySize_);
> +
> +	return lscAlgo_.init(tuningData,  context.ctrlMap, {
> +				.keys = { "r", "gr", "gb", "b" },
> +				.numHSamples = RPPX1_LSC_SAMPLES_MAX,
> +				.numVSamples = RPPX1_LSC_SAMPLES_MAX,
> +				.sensorSize = context.sensorInfo.activeAreaSize
> +			     });
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::configure
> + */
> +int LensShadingCorrection::configure(IPAContext &context,
> +				     const IPACameraSensorInfo &configInfo)
> +{
> +	const Size &size = configInfo.outputSize;
> +	Size totalSize{};
> +
> +	for (unsigned int i = 0; i < RPPX1_LSC_NUM_SECTORS; ++i) {
> +		xSizes_[i] = xSize_[i] * size.width;
> +		ySizes_[i] = ySize_[i] * size.height;
> +
> +		/*
> +		 * > The sum of all 16 {x,y}-size values must exactly match
> +		 * > image {width,height}.

Stray >

> +		 *
> +		 * Enforce it by computing the last table value separately to
> +		 * avoid rounding-induced errors.
> +		 */
> +		if (i == RPPX1_LSC_NUM_SECTORS - 1) {
> +			xSizes_[i] = size.width - totalSize.width;
> +			ySizes_[i] = size.height - totalSize.height;
> +		}
> +
> +		LOG(RppX1Lsc, Debug)
> +			<< i << ": " << xSizes_[i] << "=" << size.width << '*' << xSize_[i] << ", "
> +			<< ySizes_[i] << "=" << size.height << '*' << ySize_[i];
> +
> +		/*
> +		 * > The sector size in {x,y}-direction must be greater than 7.
> +		 */

Stray >, fits on one line ?

> +		if (xSizes_[i] < 8 || ySizes_[i] < 8)
> +			return -EINVAL;
> +		if (xSizes_[i] > 1023 || ySizes_[i] > 1023)
> +			return -EINVAL;
> +
> +		totalSize.width += xSizes_[i];
> +		totalSize.height += ySizes_[i];
> +
> +		/*
> +		 * > The gradient values are related to the sector sizes and can be
> +		 * > calculated for both x and y direction using the following equation:
> +		 * >   gradient[i] = int(2^15 / size[i] + 1/2) where i in [0;15]
> +		 */
> +		xGrad_[i] = std::lround((1u << 15) / xSizes_[i]);
> +		yGrad_[i] = std::lround((1u << 15) / ySizes_[i]);
> +	}
> +
> +	lastAppliedCt_ = 0;
> +	lastAppliedQuantizedCt_ = 0;
> +
> +	return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop,
> +				  xPos_, yPos_);
> +}
> +
> +void LensShadingCorrection::setParameters(rppx1_lsc_params &config)
> +{
> +	static_assert(sizeof(config.x_sect_size) == sizeof(xSizes_));
> +	memcpy(config.x_sect_size, xSizes_, sizeof(xSizes_));
> +
> +	static_assert(sizeof(config.y_sect_size) == sizeof(ySizes_));
> +	memcpy(config.y_sect_size, ySizes_, sizeof(ySizes_));
> +
> +	static_assert(sizeof(config.x_grad) == sizeof(xGrad_));
> +	memcpy(config.x_grad, xGrad_, sizeof(xGrad_));
> +
> +	static_assert(sizeof(config.y_grad) == sizeof(yGrad_));
> +	memcpy(config.y_grad, yGrad_, sizeof(yGrad_));
> +}
> +
> +void LensShadingCorrection::copyTable(rppx1_lsc_params &config,
> +				      const lsc::Components<uint16_t> &set)
> +{
> +	const auto &r = set.at("r");
> +	std::copy(r.begin(), r.end(), &config.r_data[0][0]);
> +	const auto &gr = set.at("gr");
> +	std::copy(gr.begin(), gr.end(), &config.gr_data[0][0]);
> +	const auto &gb = set.at("gb");
> +	std::copy(gb.begin(), gb.end(), &config.gb_data[0][0]);
> +	const auto &b = set.at("b");
> +	std::copy(b.begin(), b.end(), &config.b_data[0][0]);
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void LensShadingCorrection::queueRequest(IPAContext &context,
> +					 [[maybe_unused]] const uint32_t frame,
> +					 IPAFrameContext &frameContext,
> +					 const ControlList &controls)
> +{
> +	lscAlgo_.queueRequest(context.activeState.lsc, frameContext.lsc,
> +			      controls);
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::prepare
> + */
> +void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
> +				    [[maybe_unused]] const uint32_t frame,
> +				    IPAFrameContext &frameContext,
> +				    RppX1Params *params)
> +{
> +	uint32_t ct = frameContext.awb.colourTemperature;
> +	unsigned int quantizedCt = quantize(ct, kColourTemperatureQuantization);
> +
> +	/* Check if we can skip the update. */
> +	if (!frameContext.lsc.update) {
> +		if (!frameContext.lsc.enabled)
> +			return;
> +
> +		/*
> +		 * Add a threshold so that oscillations around a quantization
> +		 * step don't lead to constant changes.
> +		 */
> +		if (utils::abs_diff(ct, lastAppliedCt_) < kColourTemperatureQuantization / 2)
> +			return;
> +
> +		if (quantizedCt == lastAppliedQuantizedCt_)
> +			return;
> +	}
> +
> +	auto config = params->block<BlockType::LscPre1>();
> +	config.setEnabled(frameContext.lsc.enabled);
> +
> +	if (!frameContext.lsc.enabled)
> +		return;
> +
> +	setParameters(*config);
> +
> +	copyTable(*config, lscAlgo_.interpolateComponents(quantizedCt));
> +
> +	lastAppliedCt_ = ct;
> +	lastAppliedQuantizedCt_ = quantizedCt;
> +
> +	LOG(RppX1Lsc, Debug)
> +		<< "ct is " << ct << ", quantized to "
> +		<< quantizedCt;

fits on less lines

> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
> +void LensShadingCorrection::process([[maybe_unused]] IPAContext &context,
> +				    [[maybe_unused]] const uint32_t frame,
> +				    IPAFrameContext &frameContext,
> +				    [[maybe_unused]] const RppX1Stats *stats,
> +				    ControlList &metadata)
> +{
> +	lscAlgo_.process(frameContext.lsc, metadata);
> +}
> +
> +REGISTER_IPA_ALGORITHM(LensShadingCorrection, "LensShadingCorrection")
> +
> +} /* namespace ipa::rppx1::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rppx1/algorithms/lsc.h b/src/ipa/rppx1/algorithms/lsc.h
> new file mode 100644
> index 0000000000..c731637bee
> --- /dev/null
> +++ b/src/ipa/rppx1/algorithms/lsc.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2026, Ideas On Board
> + *
> + * RPP-X1 Lens Shading Correction control
> + */
> +
> +#pragma once
> +
> +#include <vector>
> +
> +#include "libipa/fixedpoint.h"
> +#include "libipa/lsc.h"
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::rppx1::algorithms {
> +
> +class LensShadingCorrection : public Algorithm
> +{
> +public:
> +	int init(IPAContext &context, const ValueNode &tuningData) override;
> +	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> +	void queueRequest(IPAContext &context, const uint32_t frame,
> +			  IPAFrameContext &frameContext,
> +			  const ControlList &controls) override;
> +	void prepare(IPAContext &context, const uint32_t frame,
> +		     IPAFrameContext &frameContext,
> +		     RppX1Params *params) override;
> +	void process(IPAContext &context, const uint32_t frame,
> +		     IPAFrameContext &frameContext,
> +		     const RppX1Stats *stats,
> +		     ControlList &metadata) override;
> +private:
> +	void setParameters(rppx1_lsc_params &config);
> +	void copyTable(rppx1_lsc_params &config,
> +		       const lsc::Components<uint16_t> &set);
> +
> +	std::vector<double> xSize_;
> +	std::vector<double> ySize_;
> +	std::vector<double> xPos_;
> +	std::vector<double> yPos_;
> +	uint16_t xGrad_[RPPX1_LSC_NUM_SECTORS];
> +	uint16_t yGrad_[RPPX1_LSC_NUM_SECTORS];
> +	uint16_t xSizes_[RPPX1_LSC_NUM_SECTORS];
> +	uint16_t ySizes_[RPPX1_LSC_NUM_SECTORS];
> +
> +	unsigned int lastAppliedCt_ = 0;
> +	unsigned int lastAppliedQuantizedCt_ = 0;
> +
> +	LscAlgorithm<UQ<2, 10>> lscAlgo_;
> +};
> +
> +} /* namespace ipa::rppx1::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rppx1/algorithms/meson.build b/src/ipa/rppx1/algorithms/meson.build
> index e7d78a00d2..7883880f9b 100644
> --- a/src/ipa/rppx1/algorithms/meson.build
> +++ b/src/ipa/rppx1/algorithms/meson.build
> @@ -7,5 +7,6 @@ rppx1_ipa_algorithms = files([
>      'ccm.cpp',
>      'goc.cpp',
>      'gsl.cpp',
> +    'lsc.cpp',
>      'lux.cpp',
>  ])
> diff --git a/src/ipa/rppx1/ipa_context.h b/src/ipa/rppx1/ipa_context.h
> index 0d7b298f43..0be3cef0e4 100644
> --- a/src/ipa/rppx1/ipa_context.h
> +++ b/src/ipa/rppx1/ipa_context.h
> @@ -22,6 +22,7 @@
>  #include <libipa/camera_sensor_helper.h>
>  #include <libipa/fc_queue.h>
>  #include <libipa/gamma.h>
> +#include <libipa/lsc.h>
>
>  namespace libcamera {
>
> @@ -49,6 +50,8 @@ struct IPAActiveState {
>
>  	ipa::gamma::ActiveState goc;
>
> +	ipa::lsc::ActiveState lsc;
> +
>  	struct {
>  		double lux;
>  	} lux;
> @@ -71,6 +74,8 @@ struct IPAFrameContext : public FrameContext {
>
>  	ipa::gamma::FrameContext goc;
>
> +	ipa::lsc::FrameContext lsc;
> +
>  	struct {
>  		double lux;
>  	} lux;
> diff --git a/src/ipa/rppx1/params.h b/src/ipa/rppx1/params.h
> index 87a70d960f..0f9de424fe 100644
> --- a/src/ipa/rppx1/params.h
> +++ b/src/ipa/rppx1/params.h
> @@ -22,6 +22,8 @@ enum class BlockType : uint16_t {
>  	ExmPre1,
>  	GaHv,
>  	HistPost,
> +	LscPre1,
> +	LscPre2,
>  	LinPre1,
>  	LinPre2,
>  	WbMeasPost,
> @@ -47,6 +49,8 @@ RPPX1_DEFINE_BLOCK_TYPE(CcorPost, ccor, CCOR_POST)
>  RPPX1_DEFINE_BLOCK_TYPE(ExmPre1, exm, EXM_PRE1)
>  RPPX1_DEFINE_BLOCK_TYPE(GaHv, ga, GA_HV)
>  RPPX1_DEFINE_BLOCK_TYPE(HistPost, hist, HIST_POST)
> +RPPX1_DEFINE_BLOCK_TYPE(LscPre1, lsc, LSC_PRE1)
> +RPPX1_DEFINE_BLOCK_TYPE(LscPre2, lsc, LSC_PRE2)
>  RPPX1_DEFINE_BLOCK_TYPE(LinPre1, lin, LIN_PRE1)
>  RPPX1_DEFINE_BLOCK_TYPE(LinPre2, lin, LIN_PRE2)
>  RPPX1_DEFINE_BLOCK_TYPE(WbMeasPost, wbmeas, WBMEAS_POST)

Only minors.

However, as rppx1 doesn't mirror the segment sizes and gradients like
rkisp1 one does, the tuning file is different.

Do we need to only mirror (in tuning file) the x-size and y-size
properties ?

Only minors though
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> --
> 2.55.0
>

2026. 09. 25. 10:32 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Fri, Sep 18, 2026 at 02:09:48PM +0200, Barnabás Pőcze wrote:
>> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>
>> Add the algorithm to the rppx1 ipa module based on the corresponding
>> algorithm in the rkisp1 ipa module.
>>
>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> ---
>>   src/ipa/rppx1/algorithms/lsc.cpp     | 272 +++++++++++++++++++++++++++
>>   src/ipa/rppx1/algorithms/lsc.h       |  58 ++++++
>>   src/ipa/rppx1/algorithms/meson.build |   1 +
>>   src/ipa/rppx1/ipa_context.h          |   5 +
>>   src/ipa/rppx1/params.h               |   4 +
>>   5 files changed, 340 insertions(+)
>>   create mode 100644 src/ipa/rppx1/algorithms/lsc.cpp
>>   create mode 100644 src/ipa/rppx1/algorithms/lsc.h
>>
>> diff --git a/src/ipa/rppx1/algorithms/lsc.cpp b/src/ipa/rppx1/algorithms/lsc.cpp
>> new file mode 100644
>> index 0000000000..1b0dbc792d
>> --- /dev/null
>> +++ b/src/ipa/rppx1/algorithms/lsc.cpp
>> @@ -0,0 +1,272 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2026, Ideas On Board
>> + *
>> + * RPP-X1 Lens Shading Correction control
>> + */
>> +
>> +#include "lsc.h"
>> +
>> +#include <algorithm>
>> +#include <cmath>
>> +#include <numeric>
>> +#include <span>
>> +#include <string.h>
>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/base/utils.h>
>> +
>> +#include "libcamera/internal/value_node.h"
>> +
>> +/**
>> + * \file lsc.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::rppx1::algorithms {
>> +
>> +LOG_DEFINE_CATEGORY(RppX1Lsc)
>> +
>> +namespace {
>> +
>> +constexpr int kColourTemperatureQuantization = 10;
>> +
>> +std::vector<double> parseSizes(const ValueNode &tuningData,
>> +			       const char *prop)
>> +{
>> +	std::vector<double> sizes =
>> +		tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
>> +	if (sizes.size() != RPPX1_LSC_NUM_SECTORS) {
>> +		LOG(RppX1Lsc, Error)
>> +			<< "Invalid '" << prop << "' values: expected "
>> +			<< RPPX1_LSC_NUM_SECTORS
>> +			<< " elements, got " << sizes.size();
>> +		return {};
>> +	}
>> +
>> +	/*
>> +	 * The sum of all elements must satisfy hardware constraints.
>> +	 * Validate it here, allowing a 1% tolerance as rounding errors may
>> +	 * prevent an exact match (further adjustments will be performed in
>> +	 * LensShadingCorrection::prepare()).
>> +	 */
>> +	constexpr double expectedSum = 1.0;
>> +
>> +	double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
>> +	if (std::abs(sum - expectedSum) > 0.01) {
>> +		LOG(RppX1Lsc, Error)
>> +			<< "Invalid '" << prop << "' values: sum of the elements"
>> +			<< " should be " << expectedSum << ", got " << sum;
>> +		return {};
>> +	}
>> +
>> +	const auto &[min, max] = std::minmax_element(sizes.begin(), sizes.end());
>> +	if (*min <= 0 || *max > 1) {
>> +		LOG(RppX1Lsc, Error)
>> +			<< "Invalid '" << prop << "' values: elements must be in (0;1]";
>> +		return {};
>> +	}
>> +
>> +	return sizes;
>> +}
>> +
>> +std::vector<double> sizesListToPositions(std::span<const double> sizes)
>> +{
>> +	std::vector<double> positions(sizes.size() + 1);
>> +
>> +	positions[0] = 0;
>> +	for (size_t i = 0; i < sizes.size(); i++)
>> +		positions[i + 1] = positions[i] + sizes[i];
>> +
>> +	return positions;
>> +}
>> +
>> +unsigned int quantize(unsigned int value, unsigned int step)
>> +{
>> +	return std::lround(value / static_cast<double>(step)) * step;
>> +}
>> +
>> +} /* namespace */
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::init
>> + */
>> +int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>> +				const ValueNode &tuningData)
>> +{
>> +	xSize_ = parseSizes(tuningData, "x-size");
>> +	ySize_ = parseSizes(tuningData, "y-size");
>> +
>> +	if (xSize_.empty() || ySize_.empty())
>> +		return -EINVAL;
>> +
>> +	xPos_ = sizesListToPositions(xSize_);
>> +	yPos_ = sizesListToPositions(ySize_);
>> +
>> +	return lscAlgo_.init(tuningData,  context.ctrlMap, {
>> +				.keys = { "r", "gr", "gb", "b" },
>> +				.numHSamples = RPPX1_LSC_SAMPLES_MAX,
>> +				.numVSamples = RPPX1_LSC_SAMPLES_MAX,
>> +				.sensorSize = context.sensorInfo.activeAreaSize
>> +			     });
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::configure
>> + */
>> +int LensShadingCorrection::configure(IPAContext &context,
>> +				     const IPACameraSensorInfo &configInfo)
>> +{
>> +	const Size &size = configInfo.outputSize;
>> +	Size totalSize{};
>> +
>> +	for (unsigned int i = 0; i < RPPX1_LSC_NUM_SECTORS; ++i) {
>> +		xSizes_[i] = xSize_[i] * size.width;
>> +		ySizes_[i] = ySize_[i] * size.height;
>> +
>> +		/*
>> +		 * > The sum of all 16 {x,y}-size values must exactly match
>> +		 * > image {width,height}.
> 
> Stray >

The intention was to signal that this is more or less a direct quote.


> 
>> +		 *
>> +		 * Enforce it by computing the last table value separately to
>> +		 * avoid rounding-induced errors.
>> +		 */
>> +		if (i == RPPX1_LSC_NUM_SECTORS - 1) {
>> +			xSizes_[i] = size.width - totalSize.width;
>> +			ySizes_[i] = size.height - totalSize.height;
>> +		}
>> +
>> +		LOG(RppX1Lsc, Debug)
>> +			<< i << ": " << xSizes_[i] << "=" << size.width << '*' << xSize_[i] << ", "
>> +			<< ySizes_[i] << "=" << size.height << '*' << ySize_[i];
>> +
>> +		/*
>> +		 * > The sector size in {x,y}-direction must be greater than 7.
>> +		 */
> 
> Stray >, fits on one line ?
> 
>> +		if (xSizes_[i] < 8 || ySizes_[i] < 8)
>> +			return -EINVAL;
>> +		if (xSizes_[i] > 1023 || ySizes_[i] > 1023)
>> +			return -EINVAL;
>> +
>> +		totalSize.width += xSizes_[i];
>> +		totalSize.height += ySizes_[i];
>> +
>> +		/*
>> +		 * > The gradient values are related to the sector sizes and can be
>> +		 * > calculated for both x and y direction using the following equation:
>> +		 * >   gradient[i] = int(2^15 / size[i] + 1/2) where i in [0;15]
>> +		 */
>> +		xGrad_[i] = std::lround((1u << 15) / xSizes_[i]);
>> +		yGrad_[i] = std::lround((1u << 15) / ySizes_[i]);
>> +	}
>> +
>> +	lastAppliedCt_ = 0;
>> +	lastAppliedQuantizedCt_ = 0;
>> +
>> +	return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop,
>> +				  xPos_, yPos_);
>> +}
>> +
>> +void LensShadingCorrection::setParameters(rppx1_lsc_params &config)
>> +{
>> +	static_assert(sizeof(config.x_sect_size) == sizeof(xSizes_));
>> +	memcpy(config.x_sect_size, xSizes_, sizeof(xSizes_));
>> +
>> +	static_assert(sizeof(config.y_sect_size) == sizeof(ySizes_));
>> +	memcpy(config.y_sect_size, ySizes_, sizeof(ySizes_));
>> +
>> +	static_assert(sizeof(config.x_grad) == sizeof(xGrad_));
>> +	memcpy(config.x_grad, xGrad_, sizeof(xGrad_));
>> +
>> +	static_assert(sizeof(config.y_grad) == sizeof(yGrad_));
>> +	memcpy(config.y_grad, yGrad_, sizeof(yGrad_));
>> +}
>> +
>> +void LensShadingCorrection::copyTable(rppx1_lsc_params &config,
>> +				      const lsc::Components<uint16_t> &set)
>> +{
>> +	const auto &r = set.at("r");
>> +	std::copy(r.begin(), r.end(), &config.r_data[0][0]);
>> +	const auto &gr = set.at("gr");
>> +	std::copy(gr.begin(), gr.end(), &config.gr_data[0][0]);
>> +	const auto &gb = set.at("gb");
>> +	std::copy(gb.begin(), gb.end(), &config.gb_data[0][0]);
>> +	const auto &b = set.at("b");
>> +	std::copy(b.begin(), b.end(), &config.b_data[0][0]);
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::queueRequest
>> + */
>> +void LensShadingCorrection::queueRequest(IPAContext &context,
>> +					 [[maybe_unused]] const uint32_t frame,
>> +					 IPAFrameContext &frameContext,
>> +					 const ControlList &controls)
>> +{
>> +	lscAlgo_.queueRequest(context.activeState.lsc, frameContext.lsc,
>> +			      controls);
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::prepare
>> + */
>> +void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
>> +				    [[maybe_unused]] const uint32_t frame,
>> +				    IPAFrameContext &frameContext,
>> +				    RppX1Params *params)
>> +{
>> +	uint32_t ct = frameContext.awb.colourTemperature;
>> +	unsigned int quantizedCt = quantize(ct, kColourTemperatureQuantization);
>> +
>> +	/* Check if we can skip the update. */
>> +	if (!frameContext.lsc.update) {
>> +		if (!frameContext.lsc.enabled)
>> +			return;
>> +
>> +		/*
>> +		 * Add a threshold so that oscillations around a quantization
>> +		 * step don't lead to constant changes.
>> +		 */
>> +		if (utils::abs_diff(ct, lastAppliedCt_) < kColourTemperatureQuantization / 2)
>> +			return;
>> +
>> +		if (quantizedCt == lastAppliedQuantizedCt_)
>> +			return;
>> +	}
>> +
>> +	auto config = params->block<BlockType::LscPre1>();
>> +	config.setEnabled(frameContext.lsc.enabled);
>> +
>> +	if (!frameContext.lsc.enabled)
>> +		return;
>> +
>> +	setParameters(*config);
>> +
>> +	copyTable(*config, lscAlgo_.interpolateComponents(quantizedCt));
>> +
>> +	lastAppliedCt_ = ct;
>> +	lastAppliedQuantizedCt_ = quantizedCt;
>> +
>> +	LOG(RppX1Lsc, Debug)
>> +		<< "ct is " << ct << ", quantized to "
>> +		<< quantizedCt;
> 
> fits on less lines
> 
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::process
>> + */
>> +void LensShadingCorrection::process([[maybe_unused]] IPAContext &context,
>> +				    [[maybe_unused]] const uint32_t frame,
>> +				    IPAFrameContext &frameContext,
>> +				    [[maybe_unused]] const RppX1Stats *stats,
>> +				    ControlList &metadata)
>> +{
>> +	lscAlgo_.process(frameContext.lsc, metadata);
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(LensShadingCorrection, "LensShadingCorrection")
>> +
>> +} /* namespace ipa::rppx1::algorithms */
>> +
>> +} /* namespace libcamera */
> [...]
>> diff --git a/src/ipa/rppx1/params.h b/src/ipa/rppx1/params.h
>> index 87a70d960f..0f9de424fe 100644
>> --- a/src/ipa/rppx1/params.h
>> +++ b/src/ipa/rppx1/params.h
>> @@ -22,6 +22,8 @@ enum class BlockType : uint16_t {
>>   	ExmPre1,
>>   	GaHv,
>>   	HistPost,
>> +	LscPre1,
>> +	LscPre2,
>>   	LinPre1,
>>   	LinPre2,
>>   	WbMeasPost,
>> @@ -47,6 +49,8 @@ RPPX1_DEFINE_BLOCK_TYPE(CcorPost, ccor, CCOR_POST)
>>   RPPX1_DEFINE_BLOCK_TYPE(ExmPre1, exm, EXM_PRE1)
>>   RPPX1_DEFINE_BLOCK_TYPE(GaHv, ga, GA_HV)
>>   RPPX1_DEFINE_BLOCK_TYPE(HistPost, hist, HIST_POST)
>> +RPPX1_DEFINE_BLOCK_TYPE(LscPre1, lsc, LSC_PRE1)
>> +RPPX1_DEFINE_BLOCK_TYPE(LscPre2, lsc, LSC_PRE2)
>>   RPPX1_DEFINE_BLOCK_TYPE(LinPre1, lin, LIN_PRE1)
>>   RPPX1_DEFINE_BLOCK_TYPE(LinPre2, lin, LIN_PRE2)
>>   RPPX1_DEFINE_BLOCK_TYPE(WbMeasPost, wbmeas, WBMEAS_POST)
> 
> Only minors.
> 
> However, as rppx1 doesn't mirror the segment sizes and gradients like
> rkisp1 one does, the tuning file is different.
> 
> Do we need to only mirror (in tuning file) the x-size and y-size
> properties ?

Yes, I think so.


> 
> Only minors though
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>    j
> 
>> --
>> 2.55.0
>>