| Message ID | 20260114113016.25162-11-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: > Now, when we have a combined matrix, we can apply AWB gains to it > directly in awb.cpp. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/awb.cpp | 7 ++++++- > src/ipa/simple/algorithms/lut.cpp | 5 +---- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index a391359fb..4d2f1df15 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > - * Copyright (C) 2024, Red Hat Inc. > + * Copyright (C) 2024-2025 Red Hat Inc. > * > * Auto white balance > */ > @@ -40,6 +40,11 @@ void Awb::prepare(IPAContext &context, > [[maybe_unused]] DebayerParams *params) > { > auto &gains = context.activeState.awb.gains; > + Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, > + 0, gains.g(), 0, > + 0, 0, gains.b() } }; > + context.activeState.combinedMatrix = > + context.activeState.combinedMatrix * gainMatrix; > /* Just report, the gains are applied in LUT algorithm. */ > frameContext.gains.red = gains.r(); > frameContext.gains.blue = gains.b(); > diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp > index 25b188b91..a6dbd7b1d 100644 > --- a/src/ipa/simple/algorithms/lut.cpp > +++ b/src/ipa/simple/algorithms/lut.cpp > @@ -108,10 +108,7 @@ void Lut::prepare(IPAContext &context, > params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())]; > } > } else if (context.activeState.matrixChanged || gammaUpdateNeeded) { > - Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, > - 0, gains.g(), 0, > - 0, 0, gains.b() } }; > - auto matrix = context.activeState.combinedMatrix * gainMatrix; > + auto &matrix = context.activeState.combinedMatrix; Is it not a concern that this change will make it so that depending on `IPAContext::ccmEnabled` the gains are applied in two different places (files, even)? If I'm not mistaken, this split remains even after all changes have been applied? > auto &red = params->redCcm; > auto &green = params->greenCcm; > auto &blue = params->blueCcm;
Hi Barnabás, thank you for review. Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: >> Now, when we have a combined matrix, we can apply AWB gains to it >> directly in awb.cpp. >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/awb.cpp | 7 ++++++- >> src/ipa/simple/algorithms/lut.cpp | 5 +---- >> 2 files changed, 7 insertions(+), 5 deletions(-) >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index a391359fb..4d2f1df15 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -1,6 +1,6 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> - * Copyright (C) 2024, Red Hat Inc. >> + * Copyright (C) 2024-2025 Red Hat Inc. >> * >> * Auto white balance >> */ >> @@ -40,6 +40,11 @@ void Awb::prepare(IPAContext &context, >> [[maybe_unused]] DebayerParams *params) >> { >> auto &gains = context.activeState.awb.gains; >> + Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, >> + 0, gains.g(), 0, >> + 0, 0, gains.b() } }; >> + context.activeState.combinedMatrix = >> + context.activeState.combinedMatrix * gainMatrix; >> /* Just report, the gains are applied in LUT algorithm. */ >> frameContext.gains.red = gains.r(); >> frameContext.gains.blue = gains.b(); >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp >> index 25b188b91..a6dbd7b1d 100644 >> --- a/src/ipa/simple/algorithms/lut.cpp >> +++ b/src/ipa/simple/algorithms/lut.cpp >> @@ -108,10 +108,7 @@ void Lut::prepare(IPAContext &context, >> params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())]; >> } >> } else if (context.activeState.matrixChanged || gammaUpdateNeeded) { >> - Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, >> - 0, gains.g(), 0, >> - 0, 0, gains.b() } }; >> - auto matrix = context.activeState.combinedMatrix * gainMatrix; >> + auto &matrix = context.activeState.combinedMatrix; > > Is it not a concern that this change will make it so that depending on `IPAContext::ccmEnabled` > the gains are applied in two different places (files, even)? If I'm not mistaken, > this split remains even after all changes have been applied? It's not perfect but how to do better? I really don't want to keep LUT table construction in algorithms. And not applying the gains here would mean passing the algorithms common job on each debayering, just because of the special handling of non-CCM CPU ISP. >> auto &red = params->redCcm; >> auto &green = params->greenCcm; >> auto &blue = params->blueCcm;
2026. 01. 19. 15:39 keltezéssel, Milan Zamazal írta: > Hi Barnabás, > > thank you for review. > > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: >>> Now, when we have a combined matrix, we can apply AWB gains to it >>> directly in awb.cpp. >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/ipa/simple/algorithms/awb.cpp | 7 ++++++- >>> src/ipa/simple/algorithms/lut.cpp | 5 +---- >>> 2 files changed, 7 insertions(+), 5 deletions(-) >>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >>> index a391359fb..4d2f1df15 100644 >>> --- a/src/ipa/simple/algorithms/awb.cpp >>> +++ b/src/ipa/simple/algorithms/awb.cpp >>> @@ -1,6 +1,6 @@ >>> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >>> /* >>> - * Copyright (C) 2024, Red Hat Inc. >>> + * Copyright (C) 2024-2025 Red Hat Inc. >>> * >>> * Auto white balance >>> */ >>> @@ -40,6 +40,11 @@ void Awb::prepare(IPAContext &context, >>> [[maybe_unused]] DebayerParams *params) >>> { >>> auto &gains = context.activeState.awb.gains; >>> + Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, >>> + 0, gains.g(), 0, >>> + 0, 0, gains.b() } }; >>> + context.activeState.combinedMatrix = >>> + context.activeState.combinedMatrix * gainMatrix; >>> /* Just report, the gains are applied in LUT algorithm. */ >>> frameContext.gains.red = gains.r(); >>> frameContext.gains.blue = gains.b(); >>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp >>> index 25b188b91..a6dbd7b1d 100644 >>> --- a/src/ipa/simple/algorithms/lut.cpp >>> +++ b/src/ipa/simple/algorithms/lut.cpp >>> @@ -108,10 +108,7 @@ void Lut::prepare(IPAContext &context, >>> params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())]; >>> } >>> } else if (context.activeState.matrixChanged || gammaUpdateNeeded) { >>> - Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, >>> - 0, gains.g(), 0, >>> - 0, 0, gains.b() } }; >>> - auto matrix = context.activeState.combinedMatrix * gainMatrix; >>> + auto &matrix = context.activeState.combinedMatrix; >> >> Is it not a concern that this change will make it so that depending on `IPAContext::ccmEnabled` >> the gains are applied in two different places (files, even)? If I'm not mistaken, >> this split remains even after all changes have been applied? > > It's not perfect but how to do better? I really don't want to keep LUT > table construction in algorithms. And not applying the gains here would > mean passing the algorithms common job on each debayering, just because > of the special handling of non-CCM CPU ISP. Not sure to be honest, it's just that it seems to me that splitting things may not be ideal. The whole "Lut" algorithm is essentially merged into `DebayerCpu` by the end, right? As far as I can see `context.activeState.awb.gains` will go into `DebayerParams::gains`, so everything is available there to keep the awb gains handling together. Or am I missing something? Maybe something with the order? But as far as I can tell it is applied last (in the "Lut" algorithm before this change set). > >>> auto &red = params->redCcm; >>> auto &green = params->greenCcm; >>> auto &blue = params->blueCcm; >
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 01. 19. 15:39 keltezéssel, Milan Zamazal írta: >> Hi Barnabás, >> thank you for review. >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >> >>> 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: >>>> Now, when we have a combined matrix, we can apply AWB gains to it >>>> directly in awb.cpp. >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/ipa/simple/algorithms/awb.cpp | 7 ++++++- >>>> src/ipa/simple/algorithms/lut.cpp | 5 +---- >>>> 2 files changed, 7 insertions(+), 5 deletions(-) >>>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >>>> index a391359fb..4d2f1df15 100644 >>>> --- a/src/ipa/simple/algorithms/awb.cpp >>>> +++ b/src/ipa/simple/algorithms/awb.cpp >>>> @@ -1,6 +1,6 @@ >>>> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>> /* >>>> - * Copyright (C) 2024, Red Hat Inc. >>>> + * Copyright (C) 2024-2025 Red Hat Inc. >>>> * >>>> * Auto white balance >>>> */ >>>> @@ -40,6 +40,11 @@ void Awb::prepare(IPAContext &context, >>>> [[maybe_unused]] DebayerParams *params) >>>> { >>>> auto &gains = context.activeState.awb.gains; >>>> + Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, >>>> + 0, gains.g(), 0, >>>> + 0, 0, gains.b() } }; >>>> + context.activeState.combinedMatrix = >>>> + context.activeState.combinedMatrix * gainMatrix; >>>> /* Just report, the gains are applied in LUT algorithm. */ >>>> frameContext.gains.red = gains.r(); >>>> frameContext.gains.blue = gains.b(); >>>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp >>>> index 25b188b91..a6dbd7b1d 100644 >>>> --- a/src/ipa/simple/algorithms/lut.cpp >>>> +++ b/src/ipa/simple/algorithms/lut.cpp >>>> @@ -108,10 +108,7 @@ void Lut::prepare(IPAContext &context, >>>> params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())]; >>>> } >>>> } else if (context.activeState.matrixChanged || gammaUpdateNeeded) { >>>> - Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, >>>> - 0, gains.g(), 0, >>>> - 0, 0, gains.b() } }; >>>> - auto matrix = context.activeState.combinedMatrix * gainMatrix; >>>> + auto &matrix = context.activeState.combinedMatrix; >>> >>> Is it not a concern that this change will make it so that depending on `IPAContext::ccmEnabled` >>> the gains are applied in two different places (files, even)? If I'm not mistaken, >>> this split remains even after all changes have been applied? >> It's not perfect but how to do better? I really don't want to keep LUT >> table construction in algorithms. And not applying the gains here would >> mean passing the algorithms common job on each debayering, just because >> of the special handling of non-CCM CPU ISP. > > Not sure to be honest, it's just that it seems to me that splitting things > may not be ideal. The whole "Lut" algorithm is essentially merged into > `DebayerCpu` by the end, right? Right. > As far as I can see `context.activeState.awb.gains` will go into > `DebayerParams::gains`, so everything is available there to keep the > awb gains handling together. Or am I missing something? Maybe > something with the order? But as far as I can tell it is applied last > (in the "Lut" algorithm before this change set). White balance should be applied before saturation. This doesn't apply in LUT construction, because saturation is disabled when the correction matrix is not used. We can make a common code in the Debayer ancestor for CPU and GPU processing to deal with applying the gains and saturation but then I don't see a good enough reason not to do it in algorithms directly. One way to avoid the special handling of gains in CPU ISP without the matrix would be to always use the matrix in CPU ISP. But it has a big performance impact and GPU ISP is a new thing. We may consider this option later. >> >>>> auto &red = params->redCcm; >>>> auto &green = params->greenCcm; >>>> auto &blue = params->blueCcm; >>
diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index a391359fb..4d2f1df15 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* - * Copyright (C) 2024, Red Hat Inc. + * Copyright (C) 2024-2025 Red Hat Inc. * * Auto white balance */ @@ -40,6 +40,11 @@ void Awb::prepare(IPAContext &context, [[maybe_unused]] DebayerParams *params) { auto &gains = context.activeState.awb.gains; + Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, + 0, gains.g(), 0, + 0, 0, gains.b() } }; + context.activeState.combinedMatrix = + context.activeState.combinedMatrix * gainMatrix; /* Just report, the gains are applied in LUT algorithm. */ frameContext.gains.red = gains.r(); frameContext.gains.blue = gains.b(); diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp index 25b188b91..a6dbd7b1d 100644 --- a/src/ipa/simple/algorithms/lut.cpp +++ b/src/ipa/simple/algorithms/lut.cpp @@ -108,10 +108,7 @@ void Lut::prepare(IPAContext &context, params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())]; } } else if (context.activeState.matrixChanged || gammaUpdateNeeded) { - Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0, - 0, gains.g(), 0, - 0, 0, gains.b() } }; - auto matrix = context.activeState.combinedMatrix * gainMatrix; + auto &matrix = context.activeState.combinedMatrix; auto &red = params->redCcm; auto &green = params->greenCcm; auto &blue = params->blueCcm;
Now, when we have a combined matrix, we can apply AWB gains to it directly in awb.cpp. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/algorithms/awb.cpp | 7 ++++++- src/ipa/simple/algorithms/lut.cpp | 5 +---- 2 files changed, 7 insertions(+), 5 deletions(-)