[03/13] libcamera: software_isp: Fix black level handling in CPU ISP
diff mbox series

Message ID 20260407-kbingham-awb-split-v1-3-a39af3f4dc20@ideasonboard.com
State New
Headers show
Series
  • ipa: simple: Convert to libipa AWB implementation
Related show

Commit Message

Kieran Bingham April 7, 2026, 10:01 p.m. UTC 
From: Milan Zamazal <mzamazal@redhat.com>

The black level handling in CPU ISP has two flaws:

- The black level is applied after white balance rather than before.

- It doesn't handle black levels with different values for individual
  colour channels.

The flaws are in both CCM and non-CCM cases.  The wrong black level and
white balance application order is well visible when the white balance
gains are significantly different from 1.0.  Then the output differs
significantly from GPU ISP output, which uses the correct order.

This patch changes the computations of the lookup tables in a way that
fixes both the problems.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/software_isp/debayer_cpu.cpp | 60 +++++++++++++++---------------
 1 file changed, 30 insertions(+), 30 deletions(-)

Comments

Laurent Pinchart April 8, 2026, 12:34 a.m. UTC | #1 
On Tue, Apr 07, 2026 at 11:01:06PM +0100, Kieran Bingham wrote:
> From: Milan Zamazal <mzamazal@redhat.com>
> 
> The black level handling in CPU ISP has two flaws:
> 
> - The black level is applied after white balance rather than before.
> 
> - It doesn't handle black levels with different values for individual
>   colour channels.
> 
> The flaws are in both CCM and non-CCM cases.  The wrong black level and
> white balance application order is well visible when the white balance
> gains are significantly different from 1.0.  Then the output differs
> significantly from GPU ISP output, which uses the correct order.
> 
> This patch changes the computations of the lookup tables in a way that
> fixes both the problems.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 60 +++++++++++++++---------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index dd0fff8714144417467bac16a1055d1185782d14..5356d6bbec11c30fa0b05659d44a91e69e2b79d0 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -879,15 +879,12 @@ void DebayerCpuThread::process4(uint32_t frame, const uint8_t *src, uint8_t *dst
>  
>  void DebayerCpu::updateGammaTable(const DebayerParams &params)
>  {
> -	const RGB<float> blackLevel = params.blackLevel;
> -	/* Take let's say the green channel black level */
> -	const unsigned int blackIndex = blackLevel[1] * gammaTable_.size();
>  	const float gamma = params.gamma;
>  	const float contrastExp = params.contrastExp;
>  
> -	const float divisor = gammaTable_.size() - blackIndex - 1.0;
> -	for (unsigned int i = blackIndex; i < gammaTable_.size(); i++) {
> -		float normalized = (i - blackIndex) / divisor;
> +	const float divisor = gammaTable_.size() - 1.0;
> +	for (unsigned int i = 0; i < gammaTable_.size(); i++) {


I think this could now be written as

	for (auto [i, value] : utils::enumerate(gammaTable_)) {

> +		float normalized = i / divisor;
>  		/* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */
>  		/* Apply simple S-curve */
>  		if (normalized < 0.5)
> @@ -897,14 +894,6 @@ void DebayerCpu::updateGammaTable(const DebayerParams &params)
>  		gammaTable_[i] = UINT8_MAX *
>  				 std::pow(normalized, gamma);

and here,

		value = UINT8_MAX * std::pow(normalized, gamma);

if you think that's clearer.

>  	}
> -	/*
> -	 * Due to CCM operations, the table lookup may reach indices below the black
> -	 * level. Let's set the table values below black level to the minimum
> -	 * non-black value to prevent problems when the minimum value is
> -	 * significantly non-zero (for example, when the image should be all grey).
> -	 */
> -	std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex,
> -		  gammaTable_[blackIndex]);
>  }
>  
>  void DebayerCpu::updateLookupTables(const DebayerParams &params)
> @@ -916,11 +905,13 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  	if (gammaUpdateNeeded)
>  		updateGammaTable(params);
>  
> +	/* Processing order: black level -> gains -> gamma */
>  	auto matrixChanged = [](const Matrix<float, 3, 3> &m1, const Matrix<float, 3, 3> &m2) -> bool {
>  		return !std::equal(m1.data().begin(), m1.data().end(), m2.data().begin());
>  	};
>  	const unsigned int gammaTableSize = gammaTable_.size();
> -	const double div = static_cast<double>(kRGBLookupSize) / gammaTableSize;
> +	RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;
> +
>  	if (ccmEnabled_) {
>  		if (gammaUpdateNeeded ||
>  		    matrixChanged(params.combinedMatrix, params_.combinedMatrix)) {
> @@ -930,17 +921,21 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  			const unsigned int redIndex = swapRedBlueGains_ ? 2 : 0;
>  			const unsigned int greenIndex = 1;
>  			const unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;
> +			const RGB<float> div =
> +				(RGB<float>(kRGBLookupSize) - blackIndex).max(1.0) /
> +				kRGBLookupSize;
>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> -				red[i].r = std::round(i * params.combinedMatrix[redIndex][0]);
> -				red[i].g = std::round(i * params.combinedMatrix[greenIndex][0]);
> -				red[i].b = std::round(i * params.combinedMatrix[blueIndex][0]);
> -				green[i].r = std::round(i * params.combinedMatrix[redIndex][1]);
> -				green[i].g = std::round(i * params.combinedMatrix[greenIndex][1]);
> -				green[i].b = std::round(i * params.combinedMatrix[blueIndex][1]);
> -				blue[i].r = std::round(i * params.combinedMatrix[redIndex][2]);
> -				blue[i].g = std::round(i * params.combinedMatrix[greenIndex][2]);
> -				blue[i].b = std::round(i * params.combinedMatrix[blueIndex][2]);
> -				gammaLut_[i] = gammaTable_[i / div];
> +				const RGB<float> rgb = ((RGB<float>(i) - blackIndex) / div).max(0.0);
> +				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
> +				red[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][0]);
> +				red[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][0]);
> +				green[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][1]);
> +				green[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][1]);
> +				green[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][1]);
> +				blue[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][2]);
> +				blue[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][2]);
> +				blue[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][2]);
> +				gammaLut_[i] = gammaTable_[i * gammaTableSize / kRGBLookupSize];
>  			}
>  		}
>  	} else {
> @@ -949,12 +944,17 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  			auto &red = swapRedBlueGains_ ? blue_ : red_;
>  			auto &green = green_;
>  			auto &blue = swapRedBlueGains_ ? red_ : blue_;
> +			const RGB<float> div =
> +				(RGB<float>(kRGBLookupSize) - blackIndex).max(1.0) /
> +				gammaTableSize;
>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> -				/* Apply gamma after gain! */
> -				const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1);
> -				red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())];
> -				green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())];
> -				blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())];
> +				const RGB<float> lutGains =
> +					(gains * (RGB<float>(i) - blackIndex) / div)
> +						.min(gammaTableSize - 1)
> +						.max(0.0);
> +				red[i] = gammaTable_[lutGains.r()];
> +				green[i] = gammaTable_[lutGains.g()];
> +				blue[i] = gammaTable_[lutGains.b()];
>  			}
>  		}
>  	}
>
Laurent Pinchart April 8, 2026, 7:45 p.m. UTC | #2 
On Tue, Apr 07, 2026 at 11:01:06PM +0100, Kieran Bingham wrote:
> From: Milan Zamazal <mzamazal@redhat.com>
> 
> The black level handling in CPU ISP has two flaws:
> 
> - The black level is applied after white balance rather than before.
> 
> - It doesn't handle black levels with different values for individual
>   colour channels.
> 
> The flaws are in both CCM and non-CCM cases.  The wrong black level and
> white balance application order is well visible when the white balance
> gains are significantly different from 1.0.  Then the output differs
> significantly from GPU ISP output, which uses the correct order.
> 
> This patch changes the computations of the lookup tables in a way that
> fixes both the problems.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 60 +++++++++++++++---------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index dd0fff8714144417467bac16a1055d1185782d14..5356d6bbec11c30fa0b05659d44a91e69e2b79d0 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -879,15 +879,12 @@ void DebayerCpuThread::process4(uint32_t frame, const uint8_t *src, uint8_t *dst
>  
>  void DebayerCpu::updateGammaTable(const DebayerParams &params)
>  {
> -	const RGB<float> blackLevel = params.blackLevel;
> -	/* Take let's say the green channel black level */
> -	const unsigned int blackIndex = blackLevel[1] * gammaTable_.size();
>  	const float gamma = params.gamma;
>  	const float contrastExp = params.contrastExp;
>  
> -	const float divisor = gammaTable_.size() - blackIndex - 1.0;
> -	for (unsigned int i = blackIndex; i < gammaTable_.size(); i++) {
> -		float normalized = (i - blackIndex) / divisor;
> +	const float divisor = gammaTable_.size() - 1.0;
> +	for (unsigned int i = 0; i < gammaTable_.size(); i++) {
> +		float normalized = i / divisor;
>  		/* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */
>  		/* Apply simple S-curve */
>  		if (normalized < 0.5)
> @@ -897,14 +894,6 @@ void DebayerCpu::updateGammaTable(const DebayerParams &params)
>  		gammaTable_[i] = UINT8_MAX *
>  				 std::pow(normalized, gamma);
>  	}
> -	/*
> -	 * Due to CCM operations, the table lookup may reach indices below the black
> -	 * level. Let's set the table values below black level to the minimum
> -	 * non-black value to prevent problems when the minimum value is
> -	 * significantly non-zero (for example, when the image should be all grey).
> -	 */
> -	std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex,
> -		  gammaTable_[blackIndex]);
>  }
>  
>  void DebayerCpu::updateLookupTables(const DebayerParams &params)
> @@ -916,11 +905,13 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  	if (gammaUpdateNeeded)
>  		updateGammaTable(params);
>  
> +	/* Processing order: black level -> gains -> gamma */
>  	auto matrixChanged = [](const Matrix<float, 3, 3> &m1, const Matrix<float, 3, 3> &m2) -> bool {
>  		return !std::equal(m1.data().begin(), m1.data().end(), m2.data().begin());
>  	};
>  	const unsigned int gammaTableSize = gammaTable_.size();
> -	const double div = static_cast<double>(kRGBLookupSize) / gammaTableSize;
> +	RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;

The div variable is moved back here in the next patch. We can avoid
going back and forth:

	const RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;
	const RGB<float> div = (RGB<float>(kRGBLookupSize) - blackIndex).max(1.0);

and...

> +
>  	if (ccmEnabled_) {
>  		if (gammaUpdateNeeded ||
>  		    matrixChanged(params.combinedMatrix, params_.combinedMatrix)) {
> @@ -930,17 +921,21 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  			const unsigned int redIndex = swapRedBlueGains_ ? 2 : 0;
>  			const unsigned int greenIndex = 1;
>  			const unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;
> +			const RGB<float> div =
> +				(RGB<float>(kRGBLookupSize) - blackIndex).max(1.0) /
> +				kRGBLookupSize;

... drop this...

>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> -				red[i].r = std::round(i * params.combinedMatrix[redIndex][0]);
> -				red[i].g = std::round(i * params.combinedMatrix[greenIndex][0]);
> -				red[i].b = std::round(i * params.combinedMatrix[blueIndex][0]);
> -				green[i].r = std::round(i * params.combinedMatrix[redIndex][1]);
> -				green[i].g = std::round(i * params.combinedMatrix[greenIndex][1]);
> -				green[i].b = std::round(i * params.combinedMatrix[blueIndex][1]);
> -				blue[i].r = std::round(i * params.combinedMatrix[redIndex][2]);
> -				blue[i].g = std::round(i * params.combinedMatrix[greenIndex][2]);
> -				blue[i].b = std::round(i * params.combinedMatrix[blueIndex][2]);
> -				gammaLut_[i] = gammaTable_[i / div];
> +				const RGB<float> rgb = ((RGB<float>(i) - blackIndex) / div).max(0.0);

				const RGB<float> rgb = ((RGB<float>(i) - blackIndex) * kRGBLookupSize / div).max(0.0);

> +				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
> +				red[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][0]);
> +				red[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][0]);
> +				green[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][1]);
> +				green[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][1]);
> +				green[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][1]);
> +				blue[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][2]);
> +				blue[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][2]);
> +				blue[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][2]);
> +				gammaLut_[i] = gammaTable_[i * gammaTableSize / kRGBLookupSize];
>  			}
>  		}
>  	} else {
> @@ -949,12 +944,17 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  			auto &red = swapRedBlueGains_ ? blue_ : red_;
>  			auto &green = green_;
>  			auto &blue = swapRedBlueGains_ ? red_ : blue_;
> +			const RGB<float> div =
> +				(RGB<float>(kRGBLookupSize) - blackIndex).max(1.0) /
> +				gammaTableSize;

... drop too ...

>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> -				/* Apply gamma after gain! */
> -				const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1);
> -				red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())];
> -				green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())];
> -				blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())];
> +				const RGB<float> lutGains =
> +					(gains * (RGB<float>(i) - blackIndex) / div)

					(gains * (RGB<float>(i) - blackIndex) * gammaTableSize / div)

This will reduce the churn in patch 04/13.

> +						.min(gammaTableSize - 1)
> +						.max(0.0);
> +				red[i] = gammaTable_[lutGains.r()];
> +				green[i] = gammaTable_[lutGains.g()];
> +				blue[i] = gammaTable_[lutGains.b()];
>  			}
>  		}
>  	}
>
Milan Zamazal April 9, 2026, 12:39 p.m. UTC | #3 
Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> From: Milan Zamazal <mzamazal@redhat.com>
>
> The black level handling in CPU ISP has two flaws:
>
> - The black level is applied after white balance rather than before.
>
> - It doesn't handle black levels with different values for individual
>   colour channels.
>
> The flaws are in both CCM and non-CCM cases.  The wrong black level and
> white balance application order is well visible when the white balance
> gains are significantly different from 1.0.  Then the output differs
> significantly from GPU ISP output, which uses the correct order.
>
> This patch changes the computations of the lookup tables in a way that
> fixes both the problems.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 60 +++++++++++++++---------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index dd0fff8714144417467bac16a1055d1185782d14..5356d6bbec11c30fa0b05659d44a91e69e2b79d0 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -879,15 +879,12 @@ void DebayerCpuThread::process4(uint32_t frame, const uint8_t *src, uint8_t *dst
>  
>  void DebayerCpu::updateGammaTable(const DebayerParams &params)
>  {
> -	const RGB<float> blackLevel = params.blackLevel;
> -	/* Take let's say the green channel black level */
> -	const unsigned int blackIndex = blackLevel[1] * gammaTable_.size();
>  	const float gamma = params.gamma;
>  	const float contrastExp = params.contrastExp;
>  
> -	const float divisor = gammaTable_.size() - blackIndex - 1.0;
> -	for (unsigned int i = blackIndex; i < gammaTable_.size(); i++) {
> -		float normalized = (i - blackIndex) / divisor;
> +	const float divisor = gammaTable_.size() - 1.0;
> +	for (unsigned int i = 0; i < gammaTable_.size(); i++) {
> +		float normalized = i / divisor;
>  		/* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */
>  		/* Apply simple S-curve */
>  		if (normalized < 0.5)
> @@ -897,14 +894,6 @@ void DebayerCpu::updateGammaTable(const DebayerParams &params)
>  		gammaTable_[i] = UINT8_MAX *
>  				 std::pow(normalized, gamma);
>  	}
> -	/*
> -	 * Due to CCM operations, the table lookup may reach indices below the black
> -	 * level. Let's set the table values below black level to the minimum
> -	 * non-black value to prevent problems when the minimum value is
> -	 * significantly non-zero (for example, when the image should be all grey).
> -	 */
> -	std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex,
> -		  gammaTable_[blackIndex]);
>  }
>  
>  void DebayerCpu::updateLookupTables(const DebayerParams &params)
> @@ -916,11 +905,13 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  	if (gammaUpdateNeeded)
>  		updateGammaTable(params);
>  
> +	/* Processing order: black level -> gains -> gamma */
>  	auto matrixChanged = [](const Matrix<float, 3, 3> &m1, const Matrix<float, 3, 3> &m2) -> bool {
>  		return !std::equal(m1.data().begin(), m1.data().end(), m2.data().begin());
>  	};
>  	const unsigned int gammaTableSize = gammaTable_.size();
> -	const double div = static_cast<double>(kRGBLookupSize) / gammaTableSize;
> +	RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;
> +
>  	if (ccmEnabled_) {
>  		if (gammaUpdateNeeded ||
>  		    matrixChanged(params.combinedMatrix, params_.combinedMatrix)) {
> @@ -930,17 +921,21 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  			const unsigned int redIndex = swapRedBlueGains_ ? 2 : 0;
>  			const unsigned int greenIndex = 1;
>  			const unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;
> +			const RGB<float> div =
> +				(RGB<float>(kRGBLookupSize) - blackIndex).max(1.0) /
> +				kRGBLookupSize;
>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> -				red[i].r = std::round(i * params.combinedMatrix[redIndex][0]);
> -				red[i].g = std::round(i * params.combinedMatrix[greenIndex][0]);
> -				red[i].b = std::round(i * params.combinedMatrix[blueIndex][0]);
> -				green[i].r = std::round(i * params.combinedMatrix[redIndex][1]);
> -				green[i].g = std::round(i * params.combinedMatrix[greenIndex][1]);
> -				green[i].b = std::round(i * params.combinedMatrix[blueIndex][1]);
> -				blue[i].r = std::round(i * params.combinedMatrix[redIndex][2]);
> -				blue[i].g = std::round(i * params.combinedMatrix[greenIndex][2]);
> -				blue[i].b = std::round(i * params.combinedMatrix[blueIndex][2]);
> -				gammaLut_[i] = gammaTable_[i / div];
> +				const RGB<float> rgb = ((RGB<float>(i) - blackIndex) / div).max(0.0);
> +				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
> +				red[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][0]);
> +				red[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][0]);
> +				green[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][1]);
> +				green[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][1]);
> +				green[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][1]);
> +				blue[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][2]);
> +				blue[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][2]);
> +				blue[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][2]);

The replacements of `i' in the multiplications are wrong.  Each of the
`red', `green', `blue' tables is applied on a raw pixel of the
corresponding colour.  This means e.g. all the rgb components of
`red[i]' should use the red black level, i.e. rgb.r(); similarly for the
other colours.

This is the correct version:

				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
				red[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);
				red[i].b = std::round(rgb.r() * params.combinedMatrix[blueIndex][0]);
				green[i].r = std::round(rgb.g() * params.combinedMatrix[redIndex][1]);
				green[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][1]);
				green[i].b = std::round(rgb.g() * params.combinedMatrix[blueIndex][1]);
				blue[i].r = std::round(rgb.b() * params.combinedMatrix[redIndex][2]);
				blue[i].g = std::round(rgb.b() * params.combinedMatrix[greenIndex][2]);
				blue[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][2]);

This bug is usually not observed here as the black levels are often all
the same for the three colours.  But it gets more pronounced in the next
patch, where incorrect AWB gains are applied to non-diagonal elements of
the matrix.  It may still not be clearly visible unless the AWB gains
are very different for each of the colours and the matrix elements are
also sufficiently different.

> +				gammaLut_[i] = gammaTable_[i * gammaTableSize / kRGBLookupSize];
>  			}
>  		}
>  	} else {
> @@ -949,12 +944,17 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  			auto &red = swapRedBlueGains_ ? blue_ : red_;
>  			auto &green = green_;
>  			auto &blue = swapRedBlueGains_ ? red_ : blue_;
> +			const RGB<float> div =
> +				(RGB<float>(kRGBLookupSize) - blackIndex).max(1.0) /
> +				gammaTableSize;
>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> -				/* Apply gamma after gain! */
> -				const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1);
> -				red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())];
> -				green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())];
> -				blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())];
> +				const RGB<float> lutGains =
> +					(gains * (RGB<float>(i) - blackIndex) / div)
> +						.min(gammaTableSize - 1)
> +						.max(0.0);
> +				red[i] = gammaTable_[lutGains.r()];
> +				green[i] = gammaTable_[lutGains.g()];
> +				blue[i] = gammaTable_[lutGains.b()];
>  			}
>  		}
>  	}

Patch
diff mbox series 

diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index dd0fff8714144417467bac16a1055d1185782d14..5356d6bbec11c30fa0b05659d44a91e69e2b79d0 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -879,15 +879,12 @@  void DebayerCpuThread::process4(uint32_t frame, const uint8_t *src, uint8_t *dst
 
 void DebayerCpu::updateGammaTable(const DebayerParams &params)
 {
-	const RGB<float> blackLevel = params.blackLevel;
-	/* Take let's say the green channel black level */
-	const unsigned int blackIndex = blackLevel[1] * gammaTable_.size();
 	const float gamma = params.gamma;
 	const float contrastExp = params.contrastExp;
 
-	const float divisor = gammaTable_.size() - blackIndex - 1.0;
-	for (unsigned int i = blackIndex; i < gammaTable_.size(); i++) {
-		float normalized = (i - blackIndex) / divisor;
+	const float divisor = gammaTable_.size() - 1.0;
+	for (unsigned int i = 0; i < gammaTable_.size(); i++) {
+		float normalized = i / divisor;
 		/* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */
 		/* Apply simple S-curve */
 		if (normalized < 0.5)
@@ -897,14 +894,6 @@  void DebayerCpu::updateGammaTable(const DebayerParams &params)
 		gammaTable_[i] = UINT8_MAX *
 				 std::pow(normalized, gamma);
 	}
-	/*
-	 * Due to CCM operations, the table lookup may reach indices below the black
-	 * level. Let's set the table values below black level to the minimum
-	 * non-black value to prevent problems when the minimum value is
-	 * significantly non-zero (for example, when the image should be all grey).
-	 */
-	std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex,
-		  gammaTable_[blackIndex]);
 }
 
 void DebayerCpu::updateLookupTables(const DebayerParams &params)
@@ -916,11 +905,13 @@  void DebayerCpu::updateLookupTables(const DebayerParams &params)
 	if (gammaUpdateNeeded)
 		updateGammaTable(params);
 
+	/* Processing order: black level -> gains -> gamma */
 	auto matrixChanged = [](const Matrix<float, 3, 3> &m1, const Matrix<float, 3, 3> &m2) -> bool {
 		return !std::equal(m1.data().begin(), m1.data().end(), m2.data().begin());
 	};
 	const unsigned int gammaTableSize = gammaTable_.size();
-	const double div = static_cast<double>(kRGBLookupSize) / gammaTableSize;
+	RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;
+
 	if (ccmEnabled_) {
 		if (gammaUpdateNeeded ||
 		    matrixChanged(params.combinedMatrix, params_.combinedMatrix)) {
@@ -930,17 +921,21 @@  void DebayerCpu::updateLookupTables(const DebayerParams &params)
 			const unsigned int redIndex = swapRedBlueGains_ ? 2 : 0;
 			const unsigned int greenIndex = 1;
 			const unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;
+			const RGB<float> div =
+				(RGB<float>(kRGBLookupSize) - blackIndex).max(1.0) /
+				kRGBLookupSize;
 			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
-				red[i].r = std::round(i * params.combinedMatrix[redIndex][0]);
-				red[i].g = std::round(i * params.combinedMatrix[greenIndex][0]);
-				red[i].b = std::round(i * params.combinedMatrix[blueIndex][0]);
-				green[i].r = std::round(i * params.combinedMatrix[redIndex][1]);
-				green[i].g = std::round(i * params.combinedMatrix[greenIndex][1]);
-				green[i].b = std::round(i * params.combinedMatrix[blueIndex][1]);
-				blue[i].r = std::round(i * params.combinedMatrix[redIndex][2]);
-				blue[i].g = std::round(i * params.combinedMatrix[greenIndex][2]);
-				blue[i].b = std::round(i * params.combinedMatrix[blueIndex][2]);
-				gammaLut_[i] = gammaTable_[i / div];
+				const RGB<float> rgb = ((RGB<float>(i) - blackIndex) / div).max(0.0);
+				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
+				red[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][0]);
+				red[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][0]);
+				green[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][1]);
+				green[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][1]);
+				green[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][1]);
+				blue[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][2]);
+				blue[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][2]);
+				blue[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][2]);
+				gammaLut_[i] = gammaTable_[i * gammaTableSize / kRGBLookupSize];
 			}
 		}
 	} else {
@@ -949,12 +944,17 @@  void DebayerCpu::updateLookupTables(const DebayerParams &params)
 			auto &red = swapRedBlueGains_ ? blue_ : red_;
 			auto &green = green_;
 			auto &blue = swapRedBlueGains_ ? red_ : blue_;
+			const RGB<float> div =
+				(RGB<float>(kRGBLookupSize) - blackIndex).max(1.0) /
+				gammaTableSize;
 			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
-				/* Apply gamma after gain! */
-				const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1);
-				red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())];
-				green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())];
-				blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())];
+				const RGB<float> lutGains =
+					(gains * (RGB<float>(i) - blackIndex) / div)
+						.min(gammaTableSize - 1)
+						.max(0.0);
+				red[i] = gammaTable_[lutGains.r()];
+				green[i] = gammaTable_[lutGains.g()];
+				blue[i] = gammaTable_[lutGains.b()];
 			}
 		}
 	}