| Message ID | 20260202-kbingham-fixups-v1-2-7d7dd4b2a27b@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Kieran Bingham (2026-02-02 19:25:04) > The CCM should be applied to the current combined Matrix. Fix the > multiplication ordering. Argh, I rewrote this commit message but messing with b4 I've lost the updated commit.. but now I can b4 send ... \o/ I'd reword this to : ipa: simple: Fix Ccm matrix multiplication The CCM should be applied to the current combined Matrix. Fix the composition order to apply the CCM after the previous combined colour transformations of the colour pipeline. > > Fixes: 82ed6c19c2b3 ("libcamera: ipa: simple: Initialise the general correction matrix") > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/simple/algorithms/ccm.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > index 911a5af2c90b55187f0d98519f3c29b8e0804567..90ea97b7f37ec78c747d355d7ba7ab517970b29d 100644 > --- a/src/ipa/simple/algorithms/ccm.cpp > +++ b/src/ipa/simple/algorithms/ccm.cpp > @@ -54,7 +54,7 @@ void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, > } > > context.activeState.combinedMatrix = > - currentCcm_.value() * context.activeState.combinedMatrix; > + context.activeState.combinedMatrix * currentCcm_.value(); > frameContext.ccm = currentCcm_.value(); > } > > > -- > 2.52.0 >
Hi Kieran, Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Kieran Bingham (2026-02-02 19:25:04) >> The CCM should be applied to the current combined Matrix. Fix the >> multiplication ordering. > > Argh, I rewrote this commit message but messing with b4 I've lost the > updated commit.. but now I can b4 send ... \o/ > > I'd reword this to : > > ipa: simple: Fix Ccm matrix multiplication > > The CCM should be applied to the current combined Matrix. Fix the > composition order to apply the CCM after the previous combined colour > transformations of the colour pipeline. It's a bit counter-intuitive, but the left-most matrix corresponds to the last operation to be applied. See https://git.libcamera.org/libcamera/libcamera.git/commit/?id=026ed6273969d931d38876345e01de626def8b07 for explanation. So I don't think this change is correct. There are other opportunities to mess the matrix operations. One of them is GPU ISP but I believe it was correct at least at the time when I reviewed it. The multiplication order is swapped in simple/algorithms/awb.cpp, which should be fixed, but since combinedMatrix is an identity matrix there, it cannot be the cause. When applying saturation in adjust.cpp, the order is correct. Do you get different results with GPU ISP and CPU ISP (with CCM enabled)? If yes then let's look into GPU ISP. If not then the problem might be outside of the matrix operations. >> >> Fixes: 82ed6c19c2b3 ("libcamera: ipa: simple: Initialise the general correction matrix") >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/ipa/simple/algorithms/ccm.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp >> index 911a5af2c90b55187f0d98519f3c29b8e0804567..90ea97b7f37ec78c747d355d7ba7ab517970b29d 100644 >> --- a/src/ipa/simple/algorithms/ccm.cpp >> +++ b/src/ipa/simple/algorithms/ccm.cpp >> @@ -54,7 +54,7 @@ void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, >> } >> >> context.activeState.combinedMatrix = >> - currentCcm_.value() * context.activeState.combinedMatrix; >> + context.activeState.combinedMatrix * currentCcm_.value(); >> frameContext.ccm = currentCcm_.value(); >> } >> >> >> -- >> 2.52.0 >>
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Kieran, > > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > >> Quoting Kieran Bingham (2026-02-02 19:25:04) >>> The CCM should be applied to the current combined Matrix. Fix the >>> multiplication ordering. >> >> Argh, I rewrote this commit message but messing with b4 I've lost the >> updated commit.. but now I can b4 send ... \o/ >> >> I'd reword this to : >> >> ipa: simple: Fix Ccm matrix multiplication >> >> The CCM should be applied to the current combined Matrix. Fix the >> composition order to apply the CCM after the previous combined colour >> transformations of the colour pipeline. > > It's a bit counter-intuitive, but the left-most matrix corresponds to > the last operation to be applied. See > https://git.libcamera.org/libcamera/libcamera.git/commit/?id=026ed6273969d931d38876345e01de626def8b07 > for explanation. > > So I don't think this change is correct. There are other opportunities > to mess the matrix operations. One of them is GPU ISP but I believe it > was correct at least at the time when I reviewed it. > > The multiplication order is swapped in simple/algorithms/awb.cpp, which > should be fixed, but since combinedMatrix is an identity matrix there, > it cannot be the cause. When applying saturation in adjust.cpp, the > order is correct. > > Do you get different results with GPU ISP and CPU ISP (with CCM > enabled)? Hmm, I do under a warm light (i.e. when the AWB gains are significantly different to each other). I'll look at it. > If yes then let's look into GPU ISP. If not then the problem might be > outside of the matrix operations. > >>> >>> Fixes: 82ed6c19c2b3 ("libcamera: ipa: simple: Initialise the general correction matrix") >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> src/ipa/simple/algorithms/ccm.cpp | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp >>> index 911a5af2c90b55187f0d98519f3c29b8e0804567..90ea97b7f37ec78c747d355d7ba7ab517970b29d 100644 >>> --- a/src/ipa/simple/algorithms/ccm.cpp >>> +++ b/src/ipa/simple/algorithms/ccm.cpp >>> @@ -54,7 +54,7 @@ void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, >>> } >>> >>> context.activeState.combinedMatrix = >>> - currentCcm_.value() * context.activeState.combinedMatrix; >>> + context.activeState.combinedMatrix * currentCcm_.value(); >>> frameContext.ccm = currentCcm_.value(); >>> } >>> >>> >>> -- >>> 2.52.0 >>>
diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp index 911a5af2c90b55187f0d98519f3c29b8e0804567..90ea97b7f37ec78c747d355d7ba7ab517970b29d 100644 --- a/src/ipa/simple/algorithms/ccm.cpp +++ b/src/ipa/simple/algorithms/ccm.cpp @@ -54,7 +54,7 @@ void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, } context.activeState.combinedMatrix = - currentCcm_.value() * context.activeState.combinedMatrix; + context.activeState.combinedMatrix * currentCcm_.value(); frameContext.ccm = currentCcm_.value(); }
The CCM should be applied to the current combined Matrix. Fix the multiplication ordering. Fixes: 82ed6c19c2b3 ("libcamera: ipa: simple: Initialise the general correction matrix") Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/simple/algorithms/ccm.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)