| Message ID | 20260128114402.31570-11-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Commit | da0926bc4b2dc28599ee2000ee9fb375b38d4f9e |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2026-01-28 11:43:57) > Now, when we have a combined matrix, we can apply AWB gains to it > directly in awb.cpp. > > Reviewed-by: Robert Mader <robert.mader@collabora.com> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/awb.cpp | 5 +++++ > src/ipa/simple/algorithms/lut.cpp | 5 +---- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 18d97f707..31726658a 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -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; I'm all for moving this out of lut.cpp - so : Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> But ordering matters here - so I think there's a likely hood we'll have to move the final combination out to the upper layer and leave the algorithms to just work out their parameters. > /* 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 9740f8c8d..d8e92c613 100644 > --- a/src/ipa/simple/algorithms/lut.cpp > +++ b/src/ipa/simple/algorithms/lut.cpp > @@ -107,10 +107,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; > -- > 2.52.0 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2026-01-28 11:43:57) >> Now, when we have a combined matrix, we can apply AWB gains to it >> directly in awb.cpp. > >> >> Reviewed-by: Robert Mader <robert.mader@collabora.com> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/awb.cpp | 5 +++++ >> src/ipa/simple/algorithms/lut.cpp | 5 +---- >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index 18d97f707..31726658a 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -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; > > I'm all for moving this out of lut.cpp - so : > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > But ordering matters here - so I think there's a likely hood we'll have > to move the final combination out to the upper layer and leave the > algorithms to just work out their parameters. Do you mean it's permitted to change the predefined order of the algorithms? Why would one need to do so? >> /* 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 9740f8c8d..d8e92c613 100644 >> --- a/src/ipa/simple/algorithms/lut.cpp >> +++ b/src/ipa/simple/algorithms/lut.cpp >> @@ -107,10 +107,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; >> -- >> 2.52.0 >>
Quoting Milan Zamazal (2026-01-28 14:48:42) > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2026-01-28 11:43:57) > >> Now, when we have a combined matrix, we can apply AWB gains to it > >> directly in awb.cpp. > > > >> > >> Reviewed-by: Robert Mader <robert.mader@collabora.com> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/ipa/simple/algorithms/awb.cpp | 5 +++++ > >> src/ipa/simple/algorithms/lut.cpp | 5 +---- > >> 2 files changed, 6 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > >> index 18d97f707..31726658a 100644 > >> --- a/src/ipa/simple/algorithms/awb.cpp > >> +++ b/src/ipa/simple/algorithms/awb.cpp > >> @@ -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; > > > > I'm all for moving this out of lut.cpp - so : > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > But ordering matters here - so I think there's a likely hood we'll have > > to move the final combination out to the upper layer and leave the > > algorithms to just work out their parameters. > > Do you mean it's permitted to change the predefined order of the > algorithms? Why would one need to do so? I mean at the moment it's *not* well defined, and therefore this could could break. I.e. if someone were to put in the tuning file: - Gamma - Awb it would be a different processing order than - Awb - Gamma And that could impact multiplications into the combinedMatrix. > > >> /* 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 9740f8c8d..d8e92c613 100644 > >> --- a/src/ipa/simple/algorithms/lut.cpp > >> +++ b/src/ipa/simple/algorithms/lut.cpp > >> @@ -107,10 +107,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; > >> -- > >> 2.52.0 > >> >
2026. 01. 28. 15:53 keltezéssel, Kieran Bingham írta: > Quoting Milan Zamazal (2026-01-28 14:48:42) >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> >>> Quoting Milan Zamazal (2026-01-28 11:43:57) >>>> Now, when we have a combined matrix, we can apply AWB gains to it >>>> directly in awb.cpp. >>> >>>> >>>> Reviewed-by: Robert Mader <robert.mader@collabora.com> >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/ipa/simple/algorithms/awb.cpp | 5 +++++ >>>> src/ipa/simple/algorithms/lut.cpp | 5 +---- >>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >>>> index 18d97f707..31726658a 100644 >>>> --- a/src/ipa/simple/algorithms/awb.cpp >>>> +++ b/src/ipa/simple/algorithms/awb.cpp >>>> @@ -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; >>> >>> I'm all for moving this out of lut.cpp - so : >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>> But ordering matters here - so I think there's a likely hood we'll have >>> to move the final combination out to the upper layer and leave the >>> algorithms to just work out their parameters. >> >> Do you mean it's permitted to change the predefined order of the >> algorithms? Why would one need to do so? > > I mean at the moment it's *not* well defined, and therefore this could > could break. > > I.e. if someone were to put in the tuning file: > > - Gamma > - Awb > > it would be a different processing order than > > - Awb > - Gamma > > And that could impact multiplications into the combinedMatrix. I think eventually this "footgun" should be removed and each algorithm should be able to specify forward/backward dependencies and the list should be sorted accordingly when loading the configuration. > >> >>>> /* 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 9740f8c8d..d8e92c613 100644 >>>> --- a/src/ipa/simple/algorithms/lut.cpp >>>> +++ b/src/ipa/simple/algorithms/lut.cpp >>>> @@ -107,10 +107,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; >>>> -- >>>> 2.52.0 >>>> >>
Quoting Barnabás Pőcze (2026-01-28 15:00:31) > 2026. 01. 28. 15:53 keltezéssel, Kieran Bingham írta: > > Quoting Milan Zamazal (2026-01-28 14:48:42) > >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > >> > >>> Quoting Milan Zamazal (2026-01-28 11:43:57) > >>>> Now, when we have a combined matrix, we can apply AWB gains to it > >>>> directly in awb.cpp. > >>> > >>>> > >>>> Reviewed-by: Robert Mader <robert.mader@collabora.com> > >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >>>> --- > >>>> src/ipa/simple/algorithms/awb.cpp | 5 +++++ > >>>> src/ipa/simple/algorithms/lut.cpp | 5 +---- > >>>> 2 files changed, 6 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > >>>> index 18d97f707..31726658a 100644 > >>>> --- a/src/ipa/simple/algorithms/awb.cpp > >>>> +++ b/src/ipa/simple/algorithms/awb.cpp > >>>> @@ -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; > >>> > >>> I'm all for moving this out of lut.cpp - so : > >>> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> > >>> But ordering matters here - so I think there's a likely hood we'll have > >>> to move the final combination out to the upper layer and leave the > >>> algorithms to just work out their parameters. > >> > >> Do you mean it's permitted to change the predefined order of the > >> algorithms? Why would one need to do so? > > > > I mean at the moment it's *not* well defined, and therefore this could > > could break. > > > > I.e. if someone were to put in the tuning file: > > > > - Gamma > > - Awb > > > > it would be a different processing order than > > > > - Awb > > - Gamma > > > > And that could impact multiplications into the combinedMatrix. > > I think eventually this "footgun" should be removed and each algorithm should be > able to specify forward/backward dependencies and the list should be sorted > accordingly when loading the configuration. > > Absolutely - that's why I haven't blocked this patch here - (this series is now merged). It's something we need to tackle on top. > > > > >> > >>>> /* 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 9740f8c8d..d8e92c613 100644 > >>>> --- a/src/ipa/simple/algorithms/lut.cpp > >>>> +++ b/src/ipa/simple/algorithms/lut.cpp > >>>> @@ -107,10 +107,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; > >>>> -- > >>>> 2.52.0 > >>>> > >> >
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 01. 28. 15:53 keltezéssel, Kieran Bingham írta: >> Quoting Milan Zamazal (2026-01-28 14:48:42) >>> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > >>> >>>> Quoting Milan Zamazal (2026-01-28 11:43:57) >>>>> Now, when we have a combined matrix, we can apply AWB gains to it >>>>> directly in awb.cpp. >>>> >>>>> >>>>> Reviewed-by: Robert Mader <robert.mader@collabora.com> >>>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>>> --- >>>>> src/ipa/simple/algorithms/awb.cpp | 5 +++++ >>>>> src/ipa/simple/algorithms/lut.cpp | 5 +---- >>>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >>>>> index 18d97f707..31726658a 100644 >>>>> --- a/src/ipa/simple/algorithms/awb.cpp >>>>> +++ b/src/ipa/simple/algorithms/awb.cpp >>>>> @@ -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; >>>> >>>> I'm all for moving this out of lut.cpp - so : >>>> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> >>>> But ordering matters here - so I think there's a likely hood we'll have >>>> to move the final combination out to the upper layer and leave the >>>> algorithms to just work out their parameters. >>> >>> Do you mean it's permitted to change the predefined order of the >>> algorithms? Why would one need to do so? >> I mean at the moment it's *not* well defined, and therefore this could >> could break. >> I.e. if someone were to put in the tuning file: >> - Gamma >> - Awb >> it would be a different processing order than >> - Awb >> - Gamma >> And that could impact multiplications into the combinedMatrix. > > I think eventually this "footgun" should be removed and each algorithm should be > able to specify forward/backward dependencies and the list should be sorted > accordingly when loading the configuration. Indeed, it's at least impractical to make the algorithms fully independent. Another example is that CCM depends on temperature, which is determined by AWB. Is there any use case for the user / tuning file to change the order of the algorithms? Or are we only talking about mistakes or is it just that all what's not forbidden is permitted? If the latter, could we simply define the order of the algorithms somewhere in the code? >> >>> >>>>> /* 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 9740f8c8d..d8e92c613 100644 >>>>> --- a/src/ipa/simple/algorithms/lut.cpp >>>>> +++ b/src/ipa/simple/algorithms/lut.cpp >>>>> @@ -107,10 +107,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; >>>>> -- >>>>> 2.52.0 >>>>> >>>
diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index 18d97f707..31726658a 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -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 9740f8c8d..d8e92c613 100644 --- a/src/ipa/simple/algorithms/lut.cpp +++ b/src/ipa/simple/algorithms/lut.cpp @@ -107,10 +107,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;