[07/11] ipa: simple: Use libipa CcmAlgorithm
diff mbox series

Message ID 20260615-libipa-algorithms-v1-7-e949c937422e@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: libipa: Introduce libipa algorithms
Related show

Commit Message

Jacopo Mondi June 15, 2026, 2:05 p.m. UTC
From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Now that libipa provides a common CCM algorithm implementation,
replace the custom handling with the common Ccm.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/simple/algorithms/ccm.cpp | 69 +++++++++++++++++++++------------------
 src/ipa/simple/algorithms/ccm.h   | 23 ++++++++-----
 src/ipa/simple/ipa_context.h      |  5 +--
 3 files changed, 55 insertions(+), 42 deletions(-)

Comments

Milan Zamazal June 17, 2026, 3:16 p.m. UTC | #1
Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:

> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Now that libipa provides a common CCM algorithm implementation,
> replace the custom handling with the common Ccm.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/simple/algorithms/ccm.cpp | 69 +++++++++++++++++++++------------------
>  src/ipa/simple/algorithms/ccm.h   | 23 ++++++++-----
>  src/ipa/simple/ipa_context.h      |  5 +--
>  3 files changed, 55 insertions(+), 42 deletions(-)
>
> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> index ff37c718c6e4..6cf55bd93db0 100644
> --- a/src/ipa/simple/algorithms/ccm.cpp
> +++ b/src/ipa/simple/algorithms/ccm.cpp
> @@ -8,54 +8,61 @@
>  
>  #include "ccm.h"
>  
> -#include <libcamera/base/log.h>
> -#include <libcamera/base/utils.h>
> -
> -#include <libcamera/control_ids.h>
> -
>  #include "libcamera/internal/matrix.h"
>  
> -namespace {
> -
> -constexpr unsigned int kTemperatureThreshold = 100;
> -
> -}
> -
>  namespace libcamera {
>  
>  namespace ipa::soft::algorithms {
>  
>  LOG_DEFINE_CATEGORY(IPASoftCcm)
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::init
> + */
>  int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)
>  {
> -	int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> -	if (ret < 0) {
> -		LOG(IPASoftCcm, Error)
> -			<< "Failed to parse 'ccm' parameter from tuning file.";
> -		return ret;
> -	}
> +	return ccmAlgo_.init(tuningData, context.ctrlMap);
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::configure
> + */
> +int Ccm::configure(IPAContext &context,
> +		   [[maybe_unused]] const IPAConfigInfo &configInfo)
> +{
> +	return ccmAlgo_.configure(context.activeState.ccm,
> +				  context.activeState.awb.automatic.temperatureK);
> +}
>  
> -	context.ccmEnabled = true;
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void Ccm::queueRequest(IPAContext &context,
> +		       [[maybe_unused]] const uint32_t frame,
> +		       IPAFrameContext &frameContext,
> +		       const ControlList &controls)
> +{
> +	/* Nothing to do here, the ccm will be calculated in prepare() */
> +	if (frameContext.awb.autoEnabled)

Why does the way the CCM is computed depend on whether auto-AWB is enabled?

> +		return;
>  
> -	return 0;
> +	ccmAlgo_.queueRequest(context.activeState.ccm, frameContext.ccm, controls);
>  }
>  
>  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  		  IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>  {
> -	const unsigned int ct = frameContext.awb.temperatureK;
> -
> -	/* Change CCM only on bigger temperature changes. */
> -	if (!currentCcm_ ||
> -	    utils::abs_diff(ct, lastCt_) >= kTemperatureThreshold) {
> -		currentCcm_ = ccm_.getInterpolated(ct);
> -		lastCt_ = ct;
> -	}
> -
> +	if (frameContext.awb.autoEnabled)
> +		ccmAlgo_.prepare(context.activeState.ccm, frameContext.ccm,
> +				 frame, frameContext.awb.temperatureK);
> +
> +	/*
> +	 * \todo: Split out combined matrix into individual parameters in
> +	 * DebayerParams and perform any pre-multiplication combination in the
> +	 * SoftISP component directly.
> +	 */
>  	context.activeState.combinedMatrix =
> -		currentCcm_.value() * context.activeState.combinedMatrix;
> -	frameContext.ccm = currentCcm_.value();
> +		frameContext.ccm.ccm * context.activeState.combinedMatrix;
>  }
>  
>  void Ccm::process([[maybe_unused]] IPAContext &context,
> @@ -64,7 +71,7 @@ void Ccm::process([[maybe_unused]] IPAContext &context,
>  		  [[maybe_unused]] const SwIspStats *stats,
>  		  ControlList &metadata)
>  {
> -	metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.data());
> +	ccmAlgo_.process(frameContext.ccm, metadata);
>  }
>  
>  REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h
> index b20a7da8aa33..0d35347ea583 100644
> --- a/src/ipa/simple/algorithms/ccm.h
> +++ b/src/ipa/simple/algorithms/ccm.h
> @@ -7,13 +7,15 @@
>  
>  #pragma once
>  
> -#include <optional>
> +#include <libcamera/controls.h>
>  
> -#include "libcamera/internal/matrix.h"
> +#include "libcamera/internal/value_node.h"
>  
> -#include <libipa/interpolator.h>
> +#include "libipa/ccm.h"
> +#include "libipa/fixedpoint.h"
>  
>  #include "algorithm.h"
> +#include "ipa_context.h"
>  
>  namespace libcamera {
>  
> @@ -22,10 +24,15 @@ namespace ipa::soft::algorithms {
>  class Ccm : public Algorithm
>  {
>  public:
> -	Ccm() = default;
> -	~Ccm() = default;
> -
>  	int init(IPAContext &context, const ValueNode &tuningData) override;
> +
> +	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> +
> +	void queueRequest(IPAContext &context,
> +			  [[maybe_unused]] const uint32_t frame,
> +			  IPAFrameContext &frameContext,
> +			  const ControlList &controls) override;
> +
>  	void prepare(IPAContext &context,
>  		     const uint32_t frame,
>  		     IPAFrameContext &frameContext,
> @@ -36,9 +43,7 @@ public:
>  		     ControlList &metadata) override;
>  
>  private:
> -	unsigned int lastCt_;
> -	Interpolator<Matrix<float, 3, 3>> ccm_;
> -	std::optional<Matrix<float, 3, 3>> currentCcm_;
> +	CcmAlgorithm<Q<4, 16>> ccmAlgo_;
>  };
>  
>  } /* namespace ipa::soft::algorithms */
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 0646f42d5618..1b40591e39cb 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -17,6 +17,7 @@
>  #include "libcamera/internal/vector.h"
>  
>  #include <libipa/awb.h>
> +#include <libipa/ccm.h>
>  #include <libipa/fc_queue.h>
>  
>  #include "core_ipa_interface.h"
> @@ -40,6 +41,7 @@ struct IPASessionConfiguration {
>  
>  struct IPAActiveState {
>  	ipa::awb::ActiveState awb;
> +	ipa::ccm::ActiveState ccm;
>  
>  	struct {
>  		int32_t exposure;
> @@ -65,8 +67,7 @@ struct IPAActiveState {
>  
>  struct IPAFrameContext : public FrameContext {
>  	ipa::awb::FrameContext awb;
> -
> -	Matrix<float, 3, 3> ccm;
> +	ipa::ccm::FrameContext ccm;
>  
>  	struct {
>  		int32_t exposure;
Kieran Bingham June 17, 2026, 4:04 p.m. UTC | #2
Quoting Milan Zamazal (2026-06-17 16:16:39)
> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
> 
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Now that libipa provides a common CCM algorithm implementation,
> > replace the custom handling with the common Ccm.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---

<snip>

> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
> > + */
> > +void Ccm::queueRequest(IPAContext &context,
> > +                    [[maybe_unused]] const uint32_t frame,
> > +                    IPAFrameContext &frameContext,
> > +                    const ControlList &controls)
> > +{
> > +     /* Nothing to do here, the ccm will be calculated in prepare() */
> > +     if (frameContext.awb.autoEnabled)
> 
> Why does the way the CCM is computed depend on whether auto-AWB is enabled?

When AWB is automatic, then it calculates the ColourTemperature of the
scene. That color temperature is used to interpolate the CCMs.


> > +             return;
> >  
> > -     return 0;
> > +     ccmAlgo_.queueRequest(context.activeState.ccm, frameContext.ccm, controls);

When awb is disabled, we call into ccmAlgo_.queueRequest() to handle any
manual controls that will impact the CCM, such as a manually set
ColourTemperature value.


> >  }
> >  
> >  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >                 IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
> >  {
> > -     const unsigned int ct = frameContext.awb.temperatureK;
> > -
> > -     /* Change CCM only on bigger temperature changes. */
> > -     if (!currentCcm_ ||
> > -         utils::abs_diff(ct, lastCt_) >= kTemperatureThreshold) {
> > -             currentCcm_ = ccm_.getInterpolated(ct);
> > -             lastCt_ = ct;
> > -     }
> > -
> > +     if (frameContext.awb.autoEnabled)
> > +             ccmAlgo_.prepare(context.activeState.ccm, frameContext.ccm,
> > +                              frame, frameContext.awb.temperatureK);
> > +

So what matters is that ccmAlgo has been called into on one of those
paths to update the frameContext.ccm.ccm by this stage.


> > +     /*
> > +      * \todo: Split out combined matrix into individual parameters in
> > +      * DebayerParams and perform any pre-multiplication combination in the
> > +      * SoftISP component directly.
> > +      */
> >       context.activeState.combinedMatrix =
> > -             currentCcm_.value() * context.activeState.combinedMatrix;
> > -     frameContext.ccm = currentCcm_.value();
> > +             frameContext.ccm.ccm * context.activeState.combinedMatrix;
> >  }
> >  

--
Kieran
Milan Zamazal June 20, 2026, 5:35 p.m. UTC | #3
Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2026-06-17 16:16:39)
>> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
>> 
>
>> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> >
>> > Now that libipa provides a common CCM algorithm implementation,
>> > replace the custom handling with the common Ccm.
>> >
>> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> > ---
>
> <snip>
>
>> > +/**
>> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
>> > + */
>> > +void Ccm::queueRequest(IPAContext &context,
>> > +                    [[maybe_unused]] const uint32_t frame,
>> > +                    IPAFrameContext &frameContext,
>> > +                    const ControlList &controls)
>> > +{
>> > +     /* Nothing to do here, the ccm will be calculated in prepare() */
>> > +     if (frameContext.awb.autoEnabled)
>> 
>> Why does the way the CCM is computed depend on whether auto-AWB is enabled?
>
> When AWB is automatic, then it calculates the ColourTemperature of the
> scene. That color temperature is used to interpolate the CCMs.
>
>
>> > +             return;
>> >  
>> > -     return 0;
>> > +     ccmAlgo_.queueRequest(context.activeState.ccm, frameContext.ccm, controls);
>
> When awb is disabled, we call into ccmAlgo_.queueRequest() to handle any
> manual controls that will impact the CCM, such as a manually set
> ColourTemperature value.

What if auto-AWB is enabled and the CCM is specified manually?  Then the
manual CCM is not set because of the `return' above?  Is it a valid
case?

>> >  }
>> >  
>> >  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>> >                 IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>> >  {
>> > -     const unsigned int ct = frameContext.awb.temperatureK;
>> > -
>> > -     /* Change CCM only on bigger temperature changes. */
>> > -     if (!currentCcm_ ||
>> > -         utils::abs_diff(ct, lastCt_) >= kTemperatureThreshold) {
>> > -             currentCcm_ = ccm_.getInterpolated(ct);
>> > -             lastCt_ = ct;
>> > -     }
>> > -
>> > +     if (frameContext.awb.autoEnabled)
>> > +             ccmAlgo_.prepare(context.activeState.ccm, frameContext.ccm,
>> > +                              frame, frameContext.awb.temperatureK);
>> > +
>
> So what matters is that ccmAlgo has been called into on one of those
> paths to update the frameContext.ccm.ccm by this stage.

OK, explained in CcmAlgorithmBase::prepare docstring.  Although not
clear whether "auto" and "manual" there apply to CCM, AWB or both.
Might be worth to make it a bit more precise.

>> > +     /*
>> > +      * \todo: Split out combined matrix into individual parameters in
>> > +      * DebayerParams and perform any pre-multiplication combination in the
>> > +      * SoftISP component directly.
>> > +      */
>> >       context.activeState.combinedMatrix =
>> > -             currentCcm_.value() * context.activeState.combinedMatrix;
>> > -     frameContext.ccm = currentCcm_.value();
>> > +             frameContext.ccm.ccm * context.activeState.combinedMatrix;
>> >  }
>> >  
>
> --
> Kieran
Jacopo Mondi June 22, 2026, 6:45 a.m. UTC | #4
On Sat, Jun 20, 2026 at 07:35:48PM +0200, Milan Zamazal wrote:
> Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
>
> > Quoting Milan Zamazal (2026-06-17 16:16:39)
> >> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
> >>
> >
> >> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> >
> >> > Now that libipa provides a common CCM algorithm implementation,
> >> > replace the custom handling with the common Ccm.
> >> >
> >> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >> > ---
> >
> > <snip>
> >
> >> > +/**
> >> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
> >> > + */
> >> > +void Ccm::queueRequest(IPAContext &context,
> >> > +                    [[maybe_unused]] const uint32_t frame,
> >> > +                    IPAFrameContext &frameContext,
> >> > +                    const ControlList &controls)
> >> > +{
> >> > +     /* Nothing to do here, the ccm will be calculated in prepare() */
> >> > +     if (frameContext.awb.autoEnabled)
> >>
> >> Why does the way the CCM is computed depend on whether auto-AWB is enabled?
> >
> > When AWB is automatic, then it calculates the ColourTemperature of the
> > scene. That color temperature is used to interpolate the CCMs.
> >
> >
> >> > +             return;
> >> >
> >> > -     return 0;
> >> > +     ccmAlgo_.queueRequest(context.activeState.ccm, frameContext.ccm, controls);
> >
> > When awb is disabled, we call into ccmAlgo_.queueRequest() to handle any
> > manual controls that will impact the CCM, such as a manually set
> > ColourTemperature value.
>
> What if auto-AWB is enabled and the CCM is specified manually?  Then the

The CCM is ignored in that case

> manual CCM is not set because of the `return' above?  Is it a valid
> case?

Manual CCM is only available if AWB is disabled. CCM are specified in
tuning file, and interpolated on the colour temperature computed by
the AWB.

Now, I don't think there is strictly anything that prevents a manual
CCM to be applied with AWB enabled, but the algorithm was designed
like this already. If we want to modify how it behaves, it will be
done on top.

>
> >> >  }
> >> >
> >> >  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >> >                 IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
> >> >  {
> >> > -     const unsigned int ct = frameContext.awb.temperatureK;
> >> > -
> >> > -     /* Change CCM only on bigger temperature changes. */
> >> > -     if (!currentCcm_ ||
> >> > -         utils::abs_diff(ct, lastCt_) >= kTemperatureThreshold) {
> >> > -             currentCcm_ = ccm_.getInterpolated(ct);
> >> > -             lastCt_ = ct;
> >> > -     }
> >> > -
> >> > +     if (frameContext.awb.autoEnabled)
> >> > +             ccmAlgo_.prepare(context.activeState.ccm, frameContext.ccm,
> >> > +                              frame, frameContext.awb.temperatureK);
> >> > +
> >
> > So what matters is that ccmAlgo has been called into on one of those
> > paths to update the frameContext.ccm.ccm by this stage.
>
> OK, explained in CcmAlgorithmBase::prepare docstring.  Although not
> clear whether "auto" and "manual" there apply to CCM, AWB or both.
> Might be worth to make it a bit more precise.

It was intentional in the documentation of prepare() to talk about
"the IPA" and not the single algorithms because if I mentioned CCM or
AWB I should have explained that AWB mode controls CCM

 * The function shall only be called if the IPA algorithm is running in auto
 * mode. If running in manual mode the application supplied correction matrix is
 * stored in \a frameContext at queueRequest() time.

Which is done in the CcmAlgorithm class documentation already

 * CcmAlgorithm supports both automatic and manual colour correction operations,
 * but doesn't offer a way to select one of them. Enabling or disabling
 * automatic ccm operations usually goes through the Awb algorithm
 * enable/disable as the two algorithms should work with the same mode.
 *
 * When the Awb algorithm runs in manual mode a custom colour correction matrix
 * or a custom colour temperature can be supplied to the ccm algorithm at
 * queueRequest() time. If the Request contains a color correction matrix
 * (controls::ColourCorrectionMatrix) then the matrix coefficients gets saved in
 * the FrameContext and the IPA module can immediately use them and doesn't need
 * to call process(). If a custom colour temperature is provided
 * (controls::ColourTemperature) then the matrices loaded from configuration are
 * interpolated with it and the result is saved in the FrameContext. In this
 * case as well IPA modules can use the result immediately and should avoid
 * calling process().
 *
 * When the Awb algorithm runs in automatic mode instead, it estimates the scene
 * colour temperature. The estimated colour temperature shall be passed to
 * process(), where it is used to interpolate the matrices loaded from the
 * tuning file. The resulting coefficients are stored in the FrameContext for
 * the IPA algorithm to use them to program their ccm engine registers.

Does it make sense ?

>
> >> > +     /*
> >> > +      * \todo: Split out combined matrix into individual parameters in
> >> > +      * DebayerParams and perform any pre-multiplication combination in the
> >> > +      * SoftISP component directly.
> >> > +      */
> >> >       context.activeState.combinedMatrix =
> >> > -             currentCcm_.value() * context.activeState.combinedMatrix;
> >> > -     frameContext.ccm = currentCcm_.value();
> >> > +             frameContext.ccm.ccm * context.activeState.combinedMatrix;
> >> >  }
> >> >
> >
> > --
> > Kieran
>
Milan Zamazal June 22, 2026, 10:01 a.m. UTC | #5
Hi Jacopo,

Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:

> On Sat, Jun 20, 2026 at 07:35:48PM +0200, Milan Zamazal wrote:
>> Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
>>
>
>> > Quoting Milan Zamazal (2026-06-17 16:16:39)
>> >> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
>> >>
>> >
>> >> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> >> >
>> >> > Now that libipa provides a common CCM algorithm implementation,
>> >> > replace the custom handling with the common Ccm.
>> >> >
>> >> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> >> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> >> > ---
>> >
>> > <snip>
>> >
>> >> > +/**
>> >> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
>> >> > + */
>> >> > +void Ccm::queueRequest(IPAContext &context,
>> >> > +                    [[maybe_unused]] const uint32_t frame,
>> >> > +                    IPAFrameContext &frameContext,
>> >> > +                    const ControlList &controls)
>> >> > +{
>> >> > +     /* Nothing to do here, the ccm will be calculated in prepare() */
>> >> > +     if (frameContext.awb.autoEnabled)
>> >>
>> >> Why does the way the CCM is computed depend on whether auto-AWB is enabled?
>> >
>> > When AWB is automatic, then it calculates the ColourTemperature of the
>> > scene. That color temperature is used to interpolate the CCMs.
>> >
>> >
>> >> > +             return;
>> >> >
>> >> > -     return 0;
>> >> > +     ccmAlgo_.queueRequest(context.activeState.ccm, frameContext.ccm, controls);
>> >
>> > When awb is disabled, we call into ccmAlgo_.queueRequest() to handle any
>> > manual controls that will impact the CCM, such as a manually set
>> > ColourTemperature value.
>>
>> What if auto-AWB is enabled and the CCM is specified manually?  Then the
>
> The CCM is ignored in that case
>
>> manual CCM is not set because of the `return' above?  Is it a valid
>> case?
>
> Manual CCM is only available if AWB is disabled. CCM are specified in
> tuning file, and interpolated on the colour temperature computed by
> the AWB.
>
> Now, I don't think there is strictly anything that prevents a manual
> CCM to be applied with AWB enabled, but the algorithm was designed
> like this already. If we want to modify how it behaves, it will be
> done on top.

OK.

>>
>> >> >  }
>> >> >
>> >> >  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>> >> >                 IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>> >> >  {
>> >> > -     const unsigned int ct = frameContext.awb.temperatureK;
>> >> > -
>> >> > -     /* Change CCM only on bigger temperature changes. */
>> >> > -     if (!currentCcm_ ||
>> >> > -         utils::abs_diff(ct, lastCt_) >= kTemperatureThreshold) {
>> >> > -             currentCcm_ = ccm_.getInterpolated(ct);
>> >> > -             lastCt_ = ct;
>> >> > -     }
>> >> > -
>> >> > +     if (frameContext.awb.autoEnabled)
>> >> > +             ccmAlgo_.prepare(context.activeState.ccm, frameContext.ccm,
>> >> > +                              frame, frameContext.awb.temperatureK);
>> >> > +
>> >
>> > So what matters is that ccmAlgo has been called into on one of those
>> > paths to update the frameContext.ccm.ccm by this stage.
>>
>> OK, explained in CcmAlgorithmBase::prepare docstring.  Although not
>> clear whether "auto" and "manual" there apply to CCM, AWB or both.
>> Might be worth to make it a bit more precise.
>
> It was intentional in the documentation of prepare() to talk about
> "the IPA" and not the single algorithms because if I mentioned CCM or
> AWB I should have explained that AWB mode controls CCM
>
>  * The function shall only be called if the IPA algorithm is running in auto
>  * mode. If running in manual mode the application supplied correction matrix is
>  * stored in \a frameContext at queueRequest() time.
>
> Which is done in the CcmAlgorithm class documentation already
>
>  * CcmAlgorithm supports both automatic and manual colour correction operations,
>  * but doesn't offer a way to select one of them. Enabling or disabling
>  * automatic ccm operations usually goes through the Awb algorithm
>  * enable/disable as the two algorithms should work with the same mode.
>  *
>  * When the Awb algorithm runs in manual mode a custom colour correction matrix
>  * or a custom colour temperature can be supplied to the ccm algorithm at
>  * queueRequest() time. If the Request contains a color correction matrix
>  * (controls::ColourCorrectionMatrix) then the matrix coefficients gets saved in
>  * the FrameContext and the IPA module can immediately use them and doesn't need
>  * to call process(). If a custom colour temperature is provided
>  * (controls::ColourTemperature) then the matrices loaded from configuration are
>  * interpolated with it and the result is saved in the FrameContext. In this
>  * case as well IPA modules can use the result immediately and should avoid
>  * calling process().
>  *
>  * When the Awb algorithm runs in automatic mode instead, it estimates the scene
>  * colour temperature. The estimated colour temperature shall be passed to
>  * process(), where it is used to interpolate the matrices loaded from the
>  * tuning file. The resulting coefficients are stored in the FrameContext for
>  * the IPA algorithm to use them to program their ccm engine registers.
>
> Does it make sense ?

Yes, thanks for clarification.

>>
>> >> > +     /*
>> >> > +      * \todo: Split out combined matrix into individual parameters in
>> >> > +      * DebayerParams and perform any pre-multiplication combination in the
>> >> > +      * SoftISP component directly.
>> >> > +      */
>> >> >       context.activeState.combinedMatrix =
>> >> > -             currentCcm_.value() * context.activeState.combinedMatrix;
>> >> > -     frameContext.ccm = currentCcm_.value();
>> >> > +             frameContext.ccm.ccm * context.activeState.combinedMatrix;
>> >> >  }
>> >> >
>> >
>> > --
>> > Kieran
>>

Patch
diff mbox series

diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
index ff37c718c6e4..6cf55bd93db0 100644
--- a/src/ipa/simple/algorithms/ccm.cpp
+++ b/src/ipa/simple/algorithms/ccm.cpp
@@ -8,54 +8,61 @@ 
 
 #include "ccm.h"
 
-#include <libcamera/base/log.h>
-#include <libcamera/base/utils.h>
-
-#include <libcamera/control_ids.h>
-
 #include "libcamera/internal/matrix.h"
 
-namespace {
-
-constexpr unsigned int kTemperatureThreshold = 100;
-
-}
-
 namespace libcamera {
 
 namespace ipa::soft::algorithms {
 
 LOG_DEFINE_CATEGORY(IPASoftCcm)
 
+/**
+ * \copydoc libcamera::ipa::Algorithm::init
+ */
 int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)
 {
-	int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
-	if (ret < 0) {
-		LOG(IPASoftCcm, Error)
-			<< "Failed to parse 'ccm' parameter from tuning file.";
-		return ret;
-	}
+	return ccmAlgo_.init(tuningData, context.ctrlMap);
+}
+
+/**
+ * \copydoc libcamera::ipa::Algorithm::configure
+ */
+int Ccm::configure(IPAContext &context,
+		   [[maybe_unused]] const IPAConfigInfo &configInfo)
+{
+	return ccmAlgo_.configure(context.activeState.ccm,
+				  context.activeState.awb.automatic.temperatureK);
+}
 
-	context.ccmEnabled = true;
+/**
+ * \copydoc libcamera::ipa::Algorithm::queueRequest
+ */
+void Ccm::queueRequest(IPAContext &context,
+		       [[maybe_unused]] const uint32_t frame,
+		       IPAFrameContext &frameContext,
+		       const ControlList &controls)
+{
+	/* Nothing to do here, the ccm will be calculated in prepare() */
+	if (frameContext.awb.autoEnabled)
+		return;
 
-	return 0;
+	ccmAlgo_.queueRequest(context.activeState.ccm, frameContext.ccm, controls);
 }
 
 void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 		  IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
 {
-	const unsigned int ct = frameContext.awb.temperatureK;
-
-	/* Change CCM only on bigger temperature changes. */
-	if (!currentCcm_ ||
-	    utils::abs_diff(ct, lastCt_) >= kTemperatureThreshold) {
-		currentCcm_ = ccm_.getInterpolated(ct);
-		lastCt_ = ct;
-	}
-
+	if (frameContext.awb.autoEnabled)
+		ccmAlgo_.prepare(context.activeState.ccm, frameContext.ccm,
+				 frame, frameContext.awb.temperatureK);
+
+	/*
+	 * \todo: Split out combined matrix into individual parameters in
+	 * DebayerParams and perform any pre-multiplication combination in the
+	 * SoftISP component directly.
+	 */
 	context.activeState.combinedMatrix =
-		currentCcm_.value() * context.activeState.combinedMatrix;
-	frameContext.ccm = currentCcm_.value();
+		frameContext.ccm.ccm * context.activeState.combinedMatrix;
 }
 
 void Ccm::process([[maybe_unused]] IPAContext &context,
@@ -64,7 +71,7 @@  void Ccm::process([[maybe_unused]] IPAContext &context,
 		  [[maybe_unused]] const SwIspStats *stats,
 		  ControlList &metadata)
 {
-	metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.data());
+	ccmAlgo_.process(frameContext.ccm, metadata);
 }
 
 REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h
index b20a7da8aa33..0d35347ea583 100644
--- a/src/ipa/simple/algorithms/ccm.h
+++ b/src/ipa/simple/algorithms/ccm.h
@@ -7,13 +7,15 @@ 
 
 #pragma once
 
-#include <optional>
+#include <libcamera/controls.h>
 
-#include "libcamera/internal/matrix.h"
+#include "libcamera/internal/value_node.h"
 
-#include <libipa/interpolator.h>
+#include "libipa/ccm.h"
+#include "libipa/fixedpoint.h"
 
 #include "algorithm.h"
+#include "ipa_context.h"
 
 namespace libcamera {
 
@@ -22,10 +24,15 @@  namespace ipa::soft::algorithms {
 class Ccm : public Algorithm
 {
 public:
-	Ccm() = default;
-	~Ccm() = default;
-
 	int init(IPAContext &context, const ValueNode &tuningData) override;
+
+	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
+
+	void queueRequest(IPAContext &context,
+			  [[maybe_unused]] const uint32_t frame,
+			  IPAFrameContext &frameContext,
+			  const ControlList &controls) override;
+
 	void prepare(IPAContext &context,
 		     const uint32_t frame,
 		     IPAFrameContext &frameContext,
@@ -36,9 +43,7 @@  public:
 		     ControlList &metadata) override;
 
 private:
-	unsigned int lastCt_;
-	Interpolator<Matrix<float, 3, 3>> ccm_;
-	std::optional<Matrix<float, 3, 3>> currentCcm_;
+	CcmAlgorithm<Q<4, 16>> ccmAlgo_;
 };
 
 } /* namespace ipa::soft::algorithms */
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index 0646f42d5618..1b40591e39cb 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -17,6 +17,7 @@ 
 #include "libcamera/internal/vector.h"
 
 #include <libipa/awb.h>
+#include <libipa/ccm.h>
 #include <libipa/fc_queue.h>
 
 #include "core_ipa_interface.h"
@@ -40,6 +41,7 @@  struct IPASessionConfiguration {
 
 struct IPAActiveState {
 	ipa::awb::ActiveState awb;
+	ipa::ccm::ActiveState ccm;
 
 	struct {
 		int32_t exposure;
@@ -65,8 +67,7 @@  struct IPAActiveState {
 
 struct IPAFrameContext : public FrameContext {
 	ipa::awb::FrameContext awb;
-
-	Matrix<float, 3, 3> ccm;
+	ipa::ccm::FrameContext ccm;
 
 	struct {
 		int32_t exposure;