| Message ID | 20260407-kbingham-awb-split-v1-3-a39af3f4dc20@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 ¶ms) > { > - 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 ¶ms) > 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 ¶ms) > @@ -916,11 +905,13 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) > 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 ¶ms) > 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 ¶ms) > 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()]; > } > } > } >
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 ¶ms) { - 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 ¶ms) 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 ¶ms) @@ -916,11 +905,13 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) 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 ¶ms) 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 ¶ms) 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()]; } } }