[RFC,3/7] libcamera: ipa: simple: Generalise tracking matrix changes

25001 diff mbox series

IPAActiveState::ccm stores the colour correction matrix (CCM) and
whether it has been changed.  The change flag is later used when
recomputing or not the lookup tables.

But the CCM may include other corrections than just the sensor colour
correction, for example white balance and saturation adjustments.  These
things should be separated and IPAActiveState::ccm should represent just
the CCM itself.

As the first step towards that cleanup, let's separate the change flag
from the CCM.  And wrap the only remaining member of
IPAActiveState::ccm.

Also, let's reset the separated change flag in the lookup tables; it'll
be no longer tight to just CCM handling.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/ccm.cpp | 7 +++----
 src/ipa/simple/algorithms/lut.cpp | 6 ++++--
 src/ipa/simple/ipa_context.h      | 6 ++----
 3 files changed, 9 insertions(+), 10 deletions(-)

Quoting Milan Zamazal (2025-11-12 08:27:11)
> IPAActiveState::ccm stores the colour correction matrix (CCM) and
> whether it has been changed.  The change flag is later used when
> recomputing or not the lookup tables.
> 
> But the CCM may include other corrections than just the sensor colour
> correction, for example white balance and saturation adjustments.  These
> things should be separated and IPAActiveState::ccm should represent just
> the CCM itself.
> 
> As the first step towards that cleanup, let's separate the change flag
> from the CCM.  And wrap the only remaining member of
> IPAActiveState::ccm.
> 
> Also, let's reset the separated change flag in the lookup tables; it'll
> be no longer tight to just CCM handling.

tight? tied?

> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/ccm.cpp | 7 +++----
>  src/ipa/simple/algorithms/lut.cpp | 6 ++++--
>  src/ipa/simple/ipa_context.h      | 6 ++----
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> index d7d3dda76..e05e5bc28 100644
> --- a/src/ipa/simple/algorithms/ccm.cpp
> +++ b/src/ipa/simple/algorithms/ccm.cpp
> @@ -94,8 +94,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>         if (frame > 0 &&
>             utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&
>             saturation == lastSaturation_) {
> -               frameContext.ccm = context.activeState.ccm.ccm;
> -               context.activeState.ccm.changed = false;
> +               frameContext.ccm = context.activeState.ccm;

I'm a bit weary here that we're reporting CCM as the metadata which is
now actually a combined result.

Should we splitting out the components and reporting them individually
in the metadata? The fact we're combining them to a single Matrix is an
implementation detail isn't it ?

--
Kieran



>                 return;
>         }
>  
> @@ -105,9 +104,9 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>         if (saturation)
>                 applySaturation(ccm, saturation.value());
>  
> -       context.activeState.ccm.ccm = ccm;
> +       context.activeState.ccm = ccm;
>         frameContext.saturation = saturation;
> -       context.activeState.ccm.changed = true;
> +       context.activeState.matrixChanged = true;
>         frameContext.ccm = ccm;
>  }
>  
> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
> index d1d5f7271..dead55f34 100644
> --- a/src/ipa/simple/algorithms/lut.cpp
> +++ b/src/ipa/simple/algorithms/lut.cpp
> @@ -118,11 +118,12 @@ void Lut::prepare(IPAContext &context,
>                         params->green[i] = gammaTable[static_cast<unsigned int>(lutGains.g())];
>                         params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())];
>                 }
> -       } else if (context.activeState.ccm.changed || gammaUpdateNeeded) {
> +
> +       } else if (context.activeState.matrixChanged || gammaUpdateNeeded) {
>                 Matrix<float, 3, 3> gainCcm = { { gains.r(), 0, 0,
>                                                   0, gains.g(), 0,
>                                                   0, 0, gains.b() } };
> -               auto ccm = context.activeState.ccm.ccm * gainCcm;
> +               auto ccm = context.activeState.ccm * gainCcm;
>                 auto &red = params->redCcm;
>                 auto &green = params->greenCcm;
>                 auto &blue = params->blueCcm;
> @@ -138,6 +139,7 @@ void Lut::prepare(IPAContext &context,
>                         blue[i].b = ccmValue(i, ccm[2][2]);
>                         params->gammaLut[i] = gammaTable[i / div];
>                 }
> +               context.activeState.matrixChanged = false;
>         }
>  }
>  
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 330fe39cb..b6b3cbfed 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -60,10 +60,8 @@ struct IPAActiveState {
>                 double contrast;
>         } gamma;
>  
> -       struct {
> -               Matrix<float, 3, 3> ccm;
> -               bool changed;
> -       } ccm;
> +       Matrix<float, 3, 3> ccm;
> +       bool matrixChanged = false;
>  
>         struct {
>                 /* 0..2 range, 1.0 = normal */
> -- 
> 2.51.1
>