| Message ID | 20260202210225.65217-1-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 02. 02. 22:02 keltezéssel, Milan Zamazal írta: > The matrix multiplication in Awb is swapped: the gains should be applied > after combinedMatrix, i.e. on the left side. The mistake happened when > `ccm' was replaced with combinedMatrix; while CCM must be applied after > gains, the gains must be applied after the combined matrix. > > Since there is currently no algorithm modifying combinedMatrix before > Awb, combinedMatrix is an identity matrix there and the wrong order > doesn't influence the output at the moment. > > Fixes: 5f3cdafe0fee ("Introduce a general correction matrix") Why not da0926bc4b2dc28599ee2000ee9fb375b38d4f9e ("libcamera: ipa: simple: Apply gain matrix in awb") ? I would argue the mistake happened while moving the multiplication? Or I suppose both could be mentioned, or neither since this is a theoretical issue at the moment. > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/awb.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 6fdaacaba..4ed1be289 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -44,7 +44,7 @@ void Awb::prepare(IPAContext &context, > 0, gains.g(), 0, > 0, 0, gains.b() } }; > context.activeState.combinedMatrix = > - context.activeState.combinedMatrix * gainMatrix; > + gainMatrix * context.activeState.combinedMatrix; > > frameContext.gains.red = gains.r(); > frameContext.gains.blue = gains.b();
Hi Barnabás, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 02. 02. 22:02 keltezéssel, Milan Zamazal írta: >> The matrix multiplication in Awb is swapped: the gains should be applied >> after combinedMatrix, i.e. on the left side. The mistake happened when >> `ccm' was replaced with combinedMatrix; while CCM must be applied after >> gains, the gains must be applied after the combined matrix. >> Since there is currently no algorithm modifying combinedMatrix before >> Awb, combinedMatrix is an identity matrix there and the wrong order >> doesn't influence the output at the moment. >> Fixes: 5f3cdafe0fee ("Introduce a general correction matrix") > > Why not da0926bc4b2dc28599ee2000ee9fb375b38d4f9e ("libcamera: ipa: simple: Apply gain matrix in awb") ? > I would argue the mistake happened while moving the multiplication? No, that commit didn't change anything about the bug, the wrong multiplication context.activeState.combinedMatrix * gainMatrix was used both before and after. The commit just moved a piece of code, which happened to contain a bug, from one place to another and there is no error in the move itself. > Or I suppose both could be mentioned, or neither since this is a theoretical > issue at the moment. I don't know what's the exact policy, I'm fine with both removing it or keeping it. >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/awb.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index 6fdaacaba..4ed1be289 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -44,7 +44,7 @@ void Awb::prepare(IPAContext &context, >> 0, gains.g(), 0, >> 0, 0, gains.b() } }; >> context.activeState.combinedMatrix = >> - context.activeState.combinedMatrix * gainMatrix; >> + gainMatrix * context.activeState.combinedMatrix; >> frameContext.gains.red = gains.r(); >> frameContext.gains.blue = gains.b();
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Barnabás, > > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> 2026. 02. 02. 22:02 keltezéssel, Milan Zamazal írta: >>> The matrix multiplication in Awb is swapped: the gains should be applied >>> after combinedMatrix, i.e. on the left side. The mistake happened when >>> `ccm' was replaced with combinedMatrix; while CCM must be applied after >>> gains, the gains must be applied after the combined matrix. >>> Since there is currently no algorithm modifying combinedMatrix before >>> Awb, combinedMatrix is an identity matrix there and the wrong order >>> doesn't influence the output at the moment. >>> Fixes: 5f3cdafe0fee ("Introduce a general correction matrix") >> >> Why not da0926bc4b2dc28599ee2000ee9fb375b38d4f9e ("libcamera: ipa: simple: Apply gain matrix in awb") ? >> I would argue the mistake happened while moving the multiplication? > > No, that commit didn't change anything about the bug, the wrong > multiplication > > context.activeState.combinedMatrix * gainMatrix > > was used both before and after. The commit just moved a piece of code, > which happened to contain a bug, from one place to another and there is > no error in the move itself. Ah, but the final matrix is different due to the move. OK, I'll adjust the commit message. >> Or I suppose both could be mentioned, or neither since this is a theoretical >> issue at the moment. > > I don't know what's the exact policy, I'm fine with both removing it or > keeping it. > >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/ipa/simple/algorithms/awb.cpp | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >>> index 6fdaacaba..4ed1be289 100644 >>> --- a/src/ipa/simple/algorithms/awb.cpp >>> +++ b/src/ipa/simple/algorithms/awb.cpp >>> @@ -44,7 +44,7 @@ void Awb::prepare(IPAContext &context, >>> 0, gains.g(), 0, >>> 0, 0, gains.b() } }; >>> context.activeState.combinedMatrix = >>> - context.activeState.combinedMatrix * gainMatrix; >>> + gainMatrix * context.activeState.combinedMatrix; >>> frameContext.gains.red = gains.r(); >>> frameContext.gains.blue = gains.b();
2026. 02. 03. 10:49 keltezéssel, Milan Zamazal írta: > Milan Zamazal <mzamazal@redhat.com> writes: > >> Hi Barnabás, >> >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >> >>> 2026. 02. 02. 22:02 keltezéssel, Milan Zamazal írta: >>>> The matrix multiplication in Awb is swapped: the gains should be applied >>>> after combinedMatrix, i.e. on the left side. The mistake happened when >>>> `ccm' was replaced with combinedMatrix; while CCM must be applied after >>>> gains, the gains must be applied after the combined matrix. >>>> Since there is currently no algorithm modifying combinedMatrix before >>>> Awb, combinedMatrix is an identity matrix there and the wrong order >>>> doesn't influence the output at the moment. >>>> Fixes: 5f3cdafe0fee ("Introduce a general correction matrix") >>> >>> Why not da0926bc4b2dc28599ee2000ee9fb375b38d4f9e ("libcamera: ipa: simple: Apply gain matrix in awb") ? >>> I would argue the mistake happened while moving the multiplication? >> >> No, that commit didn't change anything about the bug, the wrong >> multiplication >> >> context.activeState.combinedMatrix * gainMatrix >> >> was used both before and after. The commit just moved a piece of code, >> which happened to contain a bug, from one place to another and there is >> no error in the move itself. > > Ah, but the final matrix is different due to the move. > > OK, I'll adjust the commit message. Wait, sorry, I think the final matrix is actually the same in both cases. Assuming awb->ccm->lut, before the move: (ccm * pre_awb) * awb after the move: ccm * (pre_awb * awb) Currently of course pre_awb is the identity. If I understand it correctly, you want it to be ccm * awb * pre_awb, that makes sense. What I've been trying to say is that the meaning of "combinedMatrix" changes due to the move, to me that line of argumentation offers an easier to understand motivation behind the change, but that may be just me. But since there is no issue, I find it hard to know what should be the commit message and the potential "fixes" tags. If we assume a non-identity pre_awb, then the code has been wrong even before the earlier series because it only did (ccm * gains), ignoring pre_awb completely. If we don't assume pre_awb, then I'd argue that it is the "move commit" that is problematic. Anyways, I'll stop with the bike-shedding. Please compose the commit message as you see fit! > >>> Or I suppose both could be mentioned, or neither since this is a theoretical >>> issue at the moment. >> >> I don't know what's the exact policy, I'm fine with both removing it or >> keeping it. >> >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/ipa/simple/algorithms/awb.cpp | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >>>> index 6fdaacaba..4ed1be289 100644 >>>> --- a/src/ipa/simple/algorithms/awb.cpp >>>> +++ b/src/ipa/simple/algorithms/awb.cpp >>>> @@ -44,7 +44,7 @@ void Awb::prepare(IPAContext &context, >>>> 0, gains.g(), 0, >>>> 0, 0, gains.b() } }; >>>> context.activeState.combinedMatrix = >>>> - context.activeState.combinedMatrix * gainMatrix; >>>> + gainMatrix * context.activeState.combinedMatrix; >>>> frameContext.gains.red = gains.r(); >>>> frameContext.gains.blue = gains.b(); >
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 02. 03. 10:49 keltezéssel, Milan Zamazal írta: >> Milan Zamazal <mzamazal@redhat.com> writes: >> >>> Hi Barnabás, >>> >>> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >>> >>>> 2026. 02. 02. 22:02 keltezéssel, Milan Zamazal írta: >>>>> The matrix multiplication in Awb is swapped: the gains should be applied >>>>> after combinedMatrix, i.e. on the left side. The mistake happened when >>>>> `ccm' was replaced with combinedMatrix; while CCM must be applied after >>>>> gains, the gains must be applied after the combined matrix. >>>>> Since there is currently no algorithm modifying combinedMatrix before >>>>> Awb, combinedMatrix is an identity matrix there and the wrong order >>>>> doesn't influence the output at the moment. >>>>> Fixes: 5f3cdafe0fee ("Introduce a general correction matrix") >>>> >>>> Why not da0926bc4b2dc28599ee2000ee9fb375b38d4f9e ("libcamera: ipa: simple: Apply gain matrix in awb") ? >>>> I would argue the mistake happened while moving the multiplication? >>> >>> No, that commit didn't change anything about the bug, the wrong >>> multiplication >>> >>> context.activeState.combinedMatrix * gainMatrix >>> >>> was used both before and after. The commit just moved a piece of code, >>> which happened to contain a bug, from one place to another and there is >>> no error in the move itself. >> Ah, but the final matrix is different due to the move. >> OK, I'll adjust the commit message. > > Wait, sorry, I think the final matrix is actually the same in both cases. > Assuming awb->ccm->lut, before the move: > > (ccm * pre_awb) * awb > > after the move: > > ccm * (pre_awb * awb) Oh yes, right. > Currently of course pre_awb is the identity. If I understand it correctly, you want > it to be ccm * awb * pre_awb, that makes sense. Yes. > What I've been trying to say is that the meaning of "combinedMatrix" changes > due to the move, to me that line of argumentation offers an easier to understand > motivation behind the change, but that may be just me. > > But since there is no issue, I find it hard to know what should be the commit message > and the potential "fixes" tags. If we assume a non-identity pre_awb, then the code has > been wrong even before the earlier series because it only did (ccm * gains), ignoring > pre_awb completely. If we don't assume pre_awb, then I'd argue that it is the "move commit" > that is problematic. > > Anyways, I'll stop with the bike-shedding. Please compose the commit message as you see fit! OK, I changed the commit message a bit in v2 and dropped the Fixes tag. If the maintainers want to have the tag, they can pick whichever one they like. >> >>>> Or I suppose both could be mentioned, or neither since this is a theoretical >>>> issue at the moment. >>> >>> I don't know what's the exact policy, I'm fine with both removing it or >>> keeping it. >>> >>>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>>> --- >>>>> src/ipa/simple/algorithms/awb.cpp | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >>>>> index 6fdaacaba..4ed1be289 100644 >>>>> --- a/src/ipa/simple/algorithms/awb.cpp >>>>> +++ b/src/ipa/simple/algorithms/awb.cpp >>>>> @@ -44,7 +44,7 @@ void Awb::prepare(IPAContext &context, >>>>> 0, gains.g(), 0, >>>>> 0, 0, gains.b() } }; >>>>> context.activeState.combinedMatrix = >>>>> - context.activeState.combinedMatrix * gainMatrix; >>>>> + gainMatrix * context.activeState.combinedMatrix; >>>>> frameContext.gains.red = gains.r(); >>>>> frameContext.gains.blue = gains.b(); >>
diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index 6fdaacaba..4ed1be289 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -44,7 +44,7 @@ void Awb::prepare(IPAContext &context, 0, gains.g(), 0, 0, 0, gains.b() } }; context.activeState.combinedMatrix = - context.activeState.combinedMatrix * gainMatrix; + gainMatrix * context.activeState.combinedMatrix; frameContext.gains.red = gains.r(); frameContext.gains.blue = gains.b();
The matrix multiplication in Awb is swapped: the gains should be applied after combinedMatrix, i.e. on the left side. The mistake happened when `ccm' was replaced with combinedMatrix; while CCM must be applied after gains, the gains must be applied after the combined matrix. Since there is currently no algorithm modifying combinedMatrix before Awb, combinedMatrix is an identity matrix there and the wrong order doesn't influence the output at the moment. Fixes: 5f3cdafe0fee ("Introduce a general correction matrix") Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/algorithms/awb.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)