| Message ID | 20260407-kbingham-awb-split-v1-4-a39af3f4dc20@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Apr 07, 2026 at 11:01:07PM +0100, Kieran Bingham wrote: > From: Milan Zamazal <mzamazal@redhat.com> > > One of the preceding patches removed white balance gains from the > combined matrix. Which is correct but the gains are not applied now in > CPU ISP when a CCM is used. > > This patch applies the gains in the CCM table. It also clamps the pixel > values if they are out of range after applying the gains. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/software_isp/debayer_cpu.cpp | 45 +++++++++++++++--------------- > 1 file changed, 23 insertions(+), 22 deletions(-) > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index 5356d6bbec11c30fa0b05659d44a91e69e2b79d0..2dc0df5597938cc19317187f7b1ed8a10b1cc111 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -911,45 +911,46 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) > }; > const unsigned int gammaTableSize = gammaTable_.size(); > RGB<float> blackIndex = params.blackLevel * kRGBLookupSize; > + RGB<float> gains = params.gains; This is never modified, const RGB<float> &gains = params.gains; > + const RGB<float> div = (RGB<float>(kRGBLookupSize) - blackIndex).max(1.0); > > if (ccmEnabled_) { > if (gammaUpdateNeeded || > - matrixChanged(params.combinedMatrix, params_.combinedMatrix)) { > + matrixChanged(params.combinedMatrix, params_.combinedMatrix) || > + params.gains != params_.gains) { > auto &red = swapRedBlueGains_ ? blueCcm_ : redCcm_; > auto &green = greenCcm_; > auto &blue = swapRedBlueGains_ ? redCcm_ : blueCcm_; > - 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++) { > - 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]); > + const RGB<float> rgb = > + (gains * (RGB<float>(i) - blackIndex) * kRGBLookupSize / div) > + .min(kRGBLookupSize - 1) > + .max(0.0); Maybe this would be a bit more readable ? RGB<float> rgb = gains * (RGB<float>(i) - blackIndex) * kRGBLookupSize / div; rgb = rgb.max(0.0).min(kRGBLookupSize - 1); (maybe we should add a clamp() member function to Vector) You could also add a variable before the loop: const float scale = kRGBLookupSize / div; and write here RGB<float> rgb = gains * (RGB<float>(i) - blackIndex) * scale; rgb = rgb.max(0.0).min(kRGBLookupSize - 1); Or even scale the gains before the loop const RGB<float> gains = params.gain * kRGBLookupSize / div; ... RGB<float> rgb = (RGB<float>(i) - blackIndex) * gains; rgb = rgb.max(0.0).min(kRGBLookupSize - 1); I'm starting to think the could would actually become more readable if we dropped the div variable. /* * Combine the colour gains with the black level * subtraction scaling factor. Clamp the black level to * an arbitrary limit of 0.5 to avoid divisions by 0. */ const RGB<float> blackLevel = params.blackLevel.min(0.5); const RGB<float> gains = params.gain / (RGB<float>(1.0) - blackLevel); ... RGB<float> rgb = (RGB<float>(i) - blackIndex) * gains; rgb = rgb.max(0.0).min(kRGBLookupSize - 1); (possibly combining the two lines in one if you want to make the variable const) The gain calculation now uses variables in the [0.0, 1.0] range only instead of mixing them with [0, kRGBLookupSize-1], combined with the comment I think this is as readable as it can get. The second branch of the if below should be adapted similarly. I'm pretty sure this invalidates some of my comments in 03/13, sorry about that. > + red[i].r = std::round(rgb.r() * params.combinedMatrix[0][0]); > + red[i].g = std::round(rgb.g() * params.combinedMatrix[1][0]); > + red[i].b = std::round(rgb.b() * params.combinedMatrix[2][0]); > + green[i].r = std::round(rgb.r() * params.combinedMatrix[0][1]); > + green[i].g = std::round(rgb.g() * params.combinedMatrix[1][1]); > + green[i].b = std::round(rgb.b() * params.combinedMatrix[2][1]); > + blue[i].r = std::round(rgb.r() * params.combinedMatrix[0][2]); > + blue[i].g = std::round(rgb.g() * params.combinedMatrix[1][2]); > + blue[i].b = std::round(rgb.b() * params.combinedMatrix[2][2]); > + if (swapRedBlueGains_) { > + std::swap(red[i].r, red[i].b); > + std::swap(green[i].r, green[i].b); > + std::swap(blue[i].r, blue[i].b); > + } I think there's a change in behaviour here. Before this patch, when swapping red and blue gains, we had red.r = rgb.r * params.combinedMatrix[2][0] red.b = rgb.b * params.combinedMatrix[0][0] With this patch, we first compute red.r = rgb.r * params.combinedMatrix[0][0] red.b = rgb.b * params.combinedMatrix[2][0] and then swap red.r and red.b, so red.r = rgb.b * params.combinedMatrix[2][0] red.b = rgb.r * params.combinedMatrix[0][0] Note how rgb.r and rgb.b have been swapped. > gammaLut_[i] = gammaTable_[i * gammaTableSize / kRGBLookupSize]; > } > } > } else { > if (gammaUpdateNeeded || params.gains != params_.gains) { > - auto &gains = params.gains; > 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++) { > const RGB<float> lutGains = > - (gains * (RGB<float>(i) - blackIndex) / div) > + (gains * (RGB<float>(i) - blackIndex) * gammaTableSize / div) > .min(gammaTableSize - 1) > .max(0.0); So here I think it would become /* * Combine the colour gains with the black level * subtraction scaling factor. Clamp the black level to * an arbitrary limit of 0.5 to avoid divisions by 0. */ const RGB<float> blackLevel = params.blackLevel.min(0.5); const RGB<float> gains = params.gain / (RGB<float>(1.0) - blackLevel); ... RGB<float> rgb = (RGB<float>(i) - blackIndex) * gains * gammaTableSize / kRGBLookupSize; rgb = rgb.max(0.0).min(gammaTableSize - 1); The blackLevel and gains variables being identical in the two branches, they can be moved to the top of the function. > red[i] = gammaTable_[lutGains.r()]; >
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Tue, Apr 07, 2026 at 11:01:07PM +0100, Kieran Bingham wrote: >> From: Milan Zamazal <mzamazal@redhat.com> >> > >> One of the preceding patches removed white balance gains from the >> combined matrix. Which is correct but the gains are not applied now in >> CPU ISP when a CCM is used. >> >> This patch applies the gains in the CCM table. It also clamps the pixel >> values if they are out of range after applying the gains. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/software_isp/debayer_cpu.cpp | 45 +++++++++++++++--------------- >> 1 file changed, 23 insertions(+), 22 deletions(-) >> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index 5356d6bbec11c30fa0b05659d44a91e69e2b79d0..2dc0df5597938cc19317187f7b1ed8a10b1cc111 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -911,45 +911,46 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) >> }; >> const unsigned int gammaTableSize = gammaTable_.size(); >> RGB<float> blackIndex = params.blackLevel * kRGBLookupSize; >> + RGB<float> gains = params.gains; > > This is never modified, > > const RGB<float> &gains = params.gains; > >> + const RGB<float> div = (RGB<float>(kRGBLookupSize) - blackIndex).max(1.0); >> >> if (ccmEnabled_) { >> if (gammaUpdateNeeded || >> - matrixChanged(params.combinedMatrix, params_.combinedMatrix)) { >> + matrixChanged(params.combinedMatrix, params_.combinedMatrix) || >> + params.gains != params_.gains) { >> auto &red = swapRedBlueGains_ ? blueCcm_ : redCcm_; >> auto &green = greenCcm_; >> auto &blue = swapRedBlueGains_ ? redCcm_ : blueCcm_; >> - 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++) { >> - 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]); >> + const RGB<float> rgb = >> + (gains * (RGB<float>(i) - blackIndex) * kRGBLookupSize / div) >> + .min(kRGBLookupSize - 1) >> + .max(0.0); > > Maybe this would be a bit more readable ? > > RGB<float> rgb = gains * (RGB<float>(i) - blackIndex) > * kRGBLookupSize / div; > rgb = rgb.max(0.0).min(kRGBLookupSize - 1); > > (maybe we should add a clamp() member function to Vector) > > You could also add a variable before the loop: > > const float scale = kRGBLookupSize / div; > > and write here > > RGB<float> rgb = gains * (RGB<float>(i) - blackIndex) * scale; > rgb = rgb.max(0.0).min(kRGBLookupSize - 1); > > Or even scale the gains before the loop > > const RGB<float> gains = params.gain * kRGBLookupSize / div; > > ... > > RGB<float> rgb = (RGB<float>(i) - blackIndex) * gains; > rgb = rgb.max(0.0).min(kRGBLookupSize - 1); > > I'm starting to think the could would actually become more readable if > we dropped the div variable. > > /* > * Combine the colour gains with the black level > * subtraction scaling factor. Clamp the black level to > * an arbitrary limit of 0.5 to avoid divisions by 0. > */ > const RGB<float> blackLevel = params.blackLevel.min(0.5); > const RGB<float> gains = params.gain / (RGB<float>(1.0) - blackLevel); > > ... > > RGB<float> rgb = (RGB<float>(i) - blackIndex) * gains; > rgb = rgb.max(0.0).min(kRGBLookupSize - 1); > > (possibly combining the two lines in one if you want to make the > variable const) > > The gain calculation now uses variables in the [0.0, 1.0] range only > instead of mixing them with [0, kRGBLookupSize-1], combined with the > comment I think this is as readable as it can get. > > The second branch of the if below should be adapted similarly. > > I'm pretty sure this invalidates some of my comments in 03/13, sorry > about that. > >> + red[i].r = std::round(rgb.r() * params.combinedMatrix[0][0]); >> + red[i].g = std::round(rgb.g() * params.combinedMatrix[1][0]); >> + red[i].b = std::round(rgb.b() * params.combinedMatrix[2][0]); >> + green[i].r = std::round(rgb.r() * params.combinedMatrix[0][1]); >> + green[i].g = std::round(rgb.g() * params.combinedMatrix[1][1]); >> + green[i].b = std::round(rgb.b() * params.combinedMatrix[2][1]); >> + blue[i].r = std::round(rgb.r() * params.combinedMatrix[0][2]); >> + blue[i].g = std::round(rgb.g() * params.combinedMatrix[1][2]); >> + blue[i].b = std::round(rgb.b() * params.combinedMatrix[2][2]); >> + if (swapRedBlueGains_) { >> + std::swap(red[i].r, red[i].b); >> + std::swap(green[i].r, green[i].b); >> + std::swap(blue[i].r, blue[i].b); >> + } > > I think there's a change in behaviour here. > > Before this patch, when swapping red and blue gains, we had > > red.r = rgb.r * params.combinedMatrix[2][0] > red.b = rgb.b * params.combinedMatrix[0][0] > > With this patch, we first compute > > red.r = rgb.r * params.combinedMatrix[0][0] > red.b = rgb.b * params.combinedMatrix[2][0] > > and then swap red.r and red.b, so > > red.r = rgb.b * params.combinedMatrix[2][0] > red.b = rgb.r * params.combinedMatrix[0][0] > > Note how rgb.r and rgb.b have been swapped. Indeed, good catch. The bug is in the preceding black level patch, `red' table items should be all multiplied by rgb.r(), similarly for the other colours. >> gammaLut_[i] = gammaTable_[i * gammaTableSize / kRGBLookupSize]; >> } >> } >> } else { >> if (gammaUpdateNeeded || params.gains != params_.gains) { >> - auto &gains = params.gains; >> 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++) { >> const RGB<float> lutGains = >> - (gains * (RGB<float>(i) - blackIndex) / div) >> + (gains * (RGB<float>(i) - blackIndex) * gammaTableSize / div) >> .min(gammaTableSize - 1) >> .max(0.0); > > So here I think it would become > > /* > * Combine the colour gains with the black level > * subtraction scaling factor. Clamp the black level to > * an arbitrary limit of 0.5 to avoid divisions by 0. > */ > const RGB<float> blackLevel = params.blackLevel.min(0.5); > const RGB<float> gains = params.gain / (RGB<float>(1.0) - blackLevel); > > ... > > RGB<float> rgb = (RGB<float>(i) - blackIndex) * gains > * gammaTableSize / kRGBLookupSize; > rgb = rgb.max(0.0).min(gammaTableSize - 1); > > The blackLevel and gains variables being identical in the two branches, > they can be moved to the top of the function. > >> red[i] = gammaTable_[lutGains.r()]; >>
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 5356d6bbec11c30fa0b05659d44a91e69e2b79d0..2dc0df5597938cc19317187f7b1ed8a10b1cc111 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -911,45 +911,46 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) }; const unsigned int gammaTableSize = gammaTable_.size(); RGB<float> blackIndex = params.blackLevel * kRGBLookupSize; + RGB<float> gains = params.gains; + const RGB<float> div = (RGB<float>(kRGBLookupSize) - blackIndex).max(1.0); if (ccmEnabled_) { if (gammaUpdateNeeded || - matrixChanged(params.combinedMatrix, params_.combinedMatrix)) { + matrixChanged(params.combinedMatrix, params_.combinedMatrix) || + params.gains != params_.gains) { auto &red = swapRedBlueGains_ ? blueCcm_ : redCcm_; auto &green = greenCcm_; auto &blue = swapRedBlueGains_ ? redCcm_ : blueCcm_; - 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++) { - 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]); + const RGB<float> rgb = + (gains * (RGB<float>(i) - blackIndex) * kRGBLookupSize / div) + .min(kRGBLookupSize - 1) + .max(0.0); + red[i].r = std::round(rgb.r() * params.combinedMatrix[0][0]); + red[i].g = std::round(rgb.g() * params.combinedMatrix[1][0]); + red[i].b = std::round(rgb.b() * params.combinedMatrix[2][0]); + green[i].r = std::round(rgb.r() * params.combinedMatrix[0][1]); + green[i].g = std::round(rgb.g() * params.combinedMatrix[1][1]); + green[i].b = std::round(rgb.b() * params.combinedMatrix[2][1]); + blue[i].r = std::round(rgb.r() * params.combinedMatrix[0][2]); + blue[i].g = std::round(rgb.g() * params.combinedMatrix[1][2]); + blue[i].b = std::round(rgb.b() * params.combinedMatrix[2][2]); + if (swapRedBlueGains_) { + std::swap(red[i].r, red[i].b); + std::swap(green[i].r, green[i].b); + std::swap(blue[i].r, blue[i].b); + } gammaLut_[i] = gammaTable_[i * gammaTableSize / kRGBLookupSize]; } } } else { if (gammaUpdateNeeded || params.gains != params_.gains) { - auto &gains = params.gains; 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++) { const RGB<float> lutGains = - (gains * (RGB<float>(i) - blackIndex) / div) + (gains * (RGB<float>(i) - blackIndex) * gammaTableSize / div) .min(gammaTableSize - 1) .max(0.0); red[i] = gammaTable_[lutGains.r()];