[v2,01/11] ipa: libipa: awb: Reimplement AwbAlgorithm

27012 diff mbox series

The current implementation of AwbAlgorithm is the following one:

AwbAlgorithm o---- RkISP1Awb
    ^
    |
 |-----|
 |     |
Bayes Grey

The platform-specific Awb implementation instantiate one of
the Bayes or Grey Awb helper classes and both inherit from the
AwbAlgorithm base class.

The handling of libcamera controls is split between the base and the
derived (mostly Bayes) class.

We want instead the AwbAlgorithm to handle all-things-libcamera and let
the Bayes and Grey classes implement the logic. In order to achieve that
use composition also in the AwbAlgorithm class and introduce an
AwbImplementation base class from which Bayes and Grey derive from.

AwbImplementation o---- AwbAlgorithmBase
    ^                           ^
    |                           |
 |-----|                AwbAlgorithm<Q<x,y>> o---- RkISP1Awb
 |     |
Bayes Grey

The AwbAlgorithm<> class is instantiated with a Q<x, y> template
argument that represents the platform-specific register format which
is used to initialize controls and clamp gains to the hardware limits.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/ipa/libipa/awb.cpp            | 527 +++++++++++++++++++++++++++++++++-----
 src/ipa/libipa/awb.h              | 114 +++++++--
 src/ipa/libipa/awb_bayes.cpp      |  42 +--
 src/ipa/libipa/awb_bayes.h        |  11 +-
 src/ipa/libipa/awb_grey.cpp       |  25 +-
 src/ipa/libipa/awb_grey.h         |   5 +-
 src/ipa/rkisp1/algorithms/awb.cpp | 223 +++-------------
 src/ipa/rkisp1/algorithms/awb.h   |  18 +-
 src/ipa/rkisp1/ipa_context.h      |  29 +--
 9 files changed, 640 insertions(+), 354 deletions(-)

Hi Jacopo,

Thank you for the patch.

Quoting Jacopo Mondi (2026-06-23 15:54:56)
> The current implementation of AwbAlgorithm is the following one:
> 
> AwbAlgorithm o---- RkISP1Awb
>     ^
>     |
>  |-----|
>  |     |
> Bayes Grey
> 
> The platform-specific Awb implementation instantiate one of
> the Bayes or Grey Awb helper classes and both inherit from the
> AwbAlgorithm base class.
> 
> The handling of libcamera controls is split between the base and the
> derived (mostly Bayes) class.
> 
> We want instead the AwbAlgorithm to handle all-things-libcamera and let
> the Bayes and Grey classes implement the logic. In order to achieve that
> use composition also in the AwbAlgorithm class and introduce an
> AwbImplementation base class from which Bayes and Grey derive from.
> 
> AwbImplementation o---- AwbAlgorithmBase
>     ^                           ^
>     |                           |
>  |-----|                AwbAlgorithm<Q<x,y>> o---- RkISP1Awb
>  |     |
> Bayes Grey
> 
> The AwbAlgorithm<> class is instantiated with a Q<x, y> template
> argument that represents the platform-specific register format which
> is used to initialize controls and clamp gains to the hardware limits.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  src/ipa/libipa/awb.cpp            | 527 +++++++++++++++++++++++++++++++++-----
>  src/ipa/libipa/awb.h              | 114 +++++++--
>  src/ipa/libipa/awb_bayes.cpp      |  42 +--
>  src/ipa/libipa/awb_bayes.h        |  11 +-
>  src/ipa/libipa/awb_grey.cpp       |  25 +-
>  src/ipa/libipa/awb_grey.h         |   5 +-
>  src/ipa/rkisp1/algorithms/awb.cpp | 223 +++-------------
>  src/ipa/rkisp1/algorithms/awb.h   |  18 +-
>  src/ipa/rkisp1/ipa_context.h      |  29 +--
>  9 files changed, 640 insertions(+), 354 deletions(-)
> 
> diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> index 29a78069c52c..94707faf421b 100644
> --- a/src/ipa/libipa/awb.cpp
> +++ b/src/ipa/libipa/awb.cpp
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (C) 2024 Ideas on Board Oy
>   *
> - * Generic AWB algorithms
> + * libIPA Awb algorithms
>   */
>  
>  #include "awb.h"
> @@ -11,9 +11,16 @@
>  
>  #include <libcamera/control_ids.h>
>  
> +#include "awb_bayes.h"
> +#include "awb_grey.h"
> +
> +constexpr int32_t kMinColourTemperature = 2500;
> +constexpr int32_t kMaxColourTemperature = 10000;
> +constexpr int32_t kDefaultColourTemperature = 5000;
> +
>  /**
>   * \file awb.h
> - * \brief Base classes for AWB algorithms
> + * \brief libipa awb implementation
>   */
>  
>  namespace libcamera {
> @@ -22,34 +29,74 @@ LOG_DEFINE_CATEGORY(Awb)
>  
>  namespace ipa {
>  
> +namespace awb {
> +
>  /**
> - * \class AwbResult
> - * \brief The result of an AWB calculation
> + * \struct Context

I named it Result because it is the result of a algorithm run. For me,
context implies a state that is needed for the next run of an algorithm.
Both implemented awb algorithms don't need a context as they always
start from scratch based on the stats.

In the case of FrameContext it makes sense as that is the context that
is used to process the frame.

> + * \brief Awb gains and colour temperature
>   *
> - * This class holds the result of an auto white balance calculation.
> + * \var Context::gains
> + * \brief The white balance gains
> + *
> + * \var Context::temperatureK
> + * \brief The colour temperature, in Kelvin
>   */
>  
>  /**
> - * \var AwbResult::gains
> - * \brief The calculated white balance gains
> + * \struct ActiveState
> + * \brief Active awb state shared across frames
> + *
> + * \var ActiveState::manual
> + * \brief The most recent manually requested awb state
> + *
> + * \var ActiveState::automatic
> + * \brief The most recent automatically calculated awb state
> + *
> + * \var ActiveState::autoEnabled
> + * \brief True when automatic awb is selected
>   */
>  
>  /**
> - * \var AwbResult::colourTemperature
> - * \brief The calculated colour temperature in Kelvin
> + * \struct FrameContext
> + * \brief Per-frame awb state
> + *
> + * \var FrameContext::autoEnabled
> + * \brief True when automatic awb is in use
>   */
>  
> +} /* namespace awb */
> +
>  /**
>   * \class AwbStats
> - * \brief An abstraction class wrapping hardware-specific AWB statistics
> + * \brief An abstraction class wrapping hardware-specific awb statistics
>   *
> - * IPA modules using an AWB algorithm based on the AwbAlgorithm class need to
> - * implement this class to give the algorithm access to the hardware-specific
> - * statistics data.
> + * IPA modules shall provide a derived implementation of this class to give the
> + * awb algorithm access to the hardware-specific statistics data in a common
> + * format.
> + */
> +
> +/**
> + * \fn AwbStats::AwbStats()
> + * \brief Construct an empty AwbStat
> + */
> +
> +/**
> + * \brief Construct Awb statistics from RGB mean values
> + * \param[in] rgbMeans The RGB mean values
> + */
> +AwbStats::AwbStats(const RGB<double> &rgbMeans)
> +       : rgbMeans_(rgbMeans)
> +{
> +       rg_ = rgbMeans_.r() / rgbMeans_.g();
> +       bg_ = rgbMeans_.b() / rgbMeans_.g();
> +}

I think this should stay in the platform specific implementation. I'll
add more comments on the corresponding header.

> +
> +/**
> + * AwbStat::~AwbStat
> + * \brief Virtual class destructor
>   */
>  
>  /**
> - * \fn AwbStats::computeColourError()
>   * \brief Compute an error value for when the given gains would be applied
>   * \param[in] gains The gains to apply
>   *
> @@ -57,87 +104,383 @@ namespace ipa {
>   * applied. To keep the actual implementations computationally inexpensive,
>   * the squared colour error shall be returned.
>   *
> - * If the AWB statistics provide multiple zones, the average of the individual
> + * If the awb statistics provide multiple zones, the average of the individual
>   * squared errors shall be returned. Averaging/normalizing is necessary so that
>   * the numeric dimensions are the same on all hardware platforms.
>   *
>   * \return The computed error value
>   */
> +double AwbStats::computeColourError(const RGB<double> &gains) const
> +{
> +       /*
> +        * Compute the sum of the squared colour error (non-greyness) as
> +        * it appears in the log likelihood equation.
> +        */
> +       double deltaR = gains.r() * rg_ - 1.0;
> +       double deltaB = gains.b() * bg_ - 1.0;
> +       double delta2 = deltaR * deltaR + deltaB * deltaB;
> +
> +       return delta2;
> +}

IMHO: This also belongs into the platform specific implementation. Otherwise
we will only have implementations that take one measurement into account
only because the rkisp1 happened to provide only oine.

> +
> +/**
> + * \brief Retrieve if the awb statistics are valid
> + *
> + * If the colour mean values are too small, the AwbAlgorithm class doesn't have
> + * enough information to meaningfully calculate white-balance gains. Freeze the
> + * algorithm in that case.
> + *
> + * \return True if the awb statistics are valid, false otherwise
> + */
> +bool AwbStats::valid() const
> +{
> +       double minValue = minColourValue();
> +
> +       return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||
> +              rgbMeans_.b() > minValue;
> +}
> +
> +/**
> + * \fn AwbStats::rgRatio()
> + * \brief Retrieve the Red/Green ratio
> + * \return The Red/Green ratio
> + */
>  
>  /**
> - * \fn AwbStats::rgbMeans()
> - * \brief Get RGB means of the statistics
> + * \fn AwbStats::bgRatio()
> + * \brief Retrieve the Blue/Green ratio
> + * \return The Blue/Green ratio
> + */
> +
> +/**
> + * \brief Retrieve the RGB means values
>   *
>   * Fetch the RGB means from the statistics. The values of each channel are
>   * dimensionless and only the ratios are used for further calculations. This is
> - * used by the simple grey world model to calculate the gains to apply.
> + * used by the AwbGrey to calculate the gains to apply.

Coming back after writing the rest: I think we should change this to say
it is RGB values in the [0..1] range. The hardware specific part will
know how to scale that properly. And then we can get rid of xxRatio()
and there is no need for the minColourValue() anymore.

>   *
>   * \return The RGB means
>   */
> +RGB<double> AwbStats::rgbMeans() const
> +{
> +       return rgbMeans_;
> +}
>  
>  /**
> - * \class AwbAlgorithm
> - * \brief A base class for auto white balance algorithms
> + * \fn AwbStats::minColourValue
> + * \brief Retrieve the threshold below which a colour information is not valid
> + *
> + * Mean colour values as reported by the ISP through the white balance
> + * statistics are considered valid for colour gain calculation purposes when
> + * they are above a certain value. The awb algorithm needs to know what is the
> + * minimum value below which it should ignore the colour mean values.
> + *
> + * As different ISP platforms report gains in different formats, the threshold
> + * is then platform specific, and all IPA implementations that use the
> + * AwbAlgorithm class should provide that information by implementing this
> + * function.
> + *
> + * The reported minimum value applies to all three colour channels.
>   *
> - * This class is a base class for auto white balance algorithms. It defines an
> - * interface for the algorithms to implement, and is used by the IPAs to
> - * interact with the concrete implementation.
> + * This function is used by AwbStats::valid().
> + *
> + * \return The minimum valid colour value
>   */
>  
>  /**
> - * \fn AwbAlgorithm::init()
> - * \brief Initialize the algorithm with the given tuning data
> - * \param[in] tuningData The tuning data to use for the algorithm
> + * \var AwbStats::rgbMeans_
> + * \brief Mean values of colour channels
> + */
> +
> +/**
> + * \var AwbStats::rg_
> + * \brief Red/Green ratio
> + */
> +
> +/**
> + * \var AwbStats::bg_
> + * \brief Blue/Green ratio
> + */
> +
> +/**
> + * \class AwbImplementation
> + * \brief Pure virtual base class for awb algorithms implementations
>   *
> - * \return 0 on success, a negative error code otherwise
> + * The AwbImplementation class defines the interface for the awb algorithm
> + * implementations.
> + *
> + * It is currently implemented by the AwbGrey and AwbBayes classes.
> + *
> + * The interface defines an init() function to initialize the algorithm with the
> + * content of the tuning file and two functions to compute colour gains
> + * according to the algorithm operating mode (auto or manual) in use.
> + *
> + * The calculateAwb() function calculates colour gains given a set of statistics
> + * provided by the IPA module. It is used when the algorithm operates in auto
> + * mode and gains are dynamically computed given a new set of statistics from
> + * the awb engine.
> + *
> + * The gainsFromColourTemperature() function instead interpolates a gain curve
> + * if specified in the tuning file with the supplied colour temperature. This
> + * function is used in manual mode where it is expected the application to
> + * supply a colour temperature and the algorithm to adjust the white balance
> + * gains to it.
> + */
> +
> +/**
> + * \fn AwbImplementation::~AwbImplementation
> + * \brief Virtual class destructor
> + */
> +
> +/**
> + * \fn AwbImplementation::init()
> + * \param[in] tuningData
> + * \brief Initialize the algorithm by parsing \a tuningData
>   */
>  
>  /**
> - * \fn AwbAlgorithm::calculateAwb()
> - * \brief Calculate AWB data from the given statistics
> - * \param[in] stats The statistics to use for the calculation
> - * \param[in] lux The lux value of the scene
> + * \fn AwbImplementation::calculateAwb()
> + * \param[in] stats The awb statistics
> + * \param[in] lux The estimated lux level
> + * \param[in] ranges The colour temperature search limits (AwbBayes only)
>   *
> - * Calculate an AwbResult object from the given statistics and lux value. A \a
> - * lux value of 0 means it is unknown or invalid and the algorithm shall ignore
> - * it.
> + * Calculate a new set of colour gains and a colour temperature given a new
> + * set of statistics \a stats, an estimated luminance \a lux and a range of
> + * colour temperature to limit the search (for AwbBayes only).

I would drop the reference to AwbBayes

>   *
> - * \return The AWB result
> + * \return An awb::Context computed using the new statistics
> + */
> +/**
> + * \fn AwbImplementation::gainsFromColourTemperature()
> + * \param[in] temperatureK Colour temperature in Kelvin
> + *
> + * Calculate a new set of colour gains by interpolating a gain curve (if
> + * provided in the tuning file) with a new colour temperature \a temperatureK.

This (interpolating a curve) is one way to do it. But it is an
implementation detail of the specific algo. So I think it doesn't belong
to the base class documentation or be marked as example.

> + *
> + * \return The RGB gain values adjusted to \a temperatureK or std::nullopt if
> + * no gain curve is specified in the tuning data

Hm, for a base class, this feels a bit too specific. Assume a neural
network based awb. That would potentially not require the curve.

>   */
>  
>  /**
> - * \fn AwbAlgorithm::gainsFromColourTemperature()
> - * \brief Compute white balance gains from a colour temperature
> - * \param[in] colourTemperature The colour temperature in Kelvin
> + * \class AwbAlgorithmBase
> + * \brief Base class for AwbAlgorithm for non-templated functions implementation
>   *
> - * Compute the white balance gains from a \a colourTemperature. This function
> - * does not take any statistics into account. It is used to compute the colour
> - * gains when the user manually specifies a colour temperature.

This information got lost, but is IMHO important for the implementer to
understand why he needs to implement this.

> + * Base class for AwbAlgorithm where non-templated functions are implemented.
> + * IPA implementations shall use AwbAlgorithm and not this class.
> + */
> +
> +/**
> + * \brief Initialize the algorithm with the given tuning data
> + * \param[in] tuningData The tuning data to use for the algorithm
> + *
> + * Parse \a tuningData to initialize the awb algorithm and register controls.
> + * IPA modules are expected to call this function as part of their
> + * implementation of Algorithm::init().
>   *
> - * \return The colour gains or std::nullopt if the conversion is not possible
> + * \return 0 on success, a negative error code otherwise
> + */
> +int AwbAlgorithmBase::init(const ValueNode &tuningData)
> +{
> +       bayes_ = false;
> +
> +       if (!tuningData.contains("algorithm"))
> +               LOG(Awb, Info) << "No Awb algorithm specified, using grey world";
> +
> +       auto mode = tuningData["algorithm"].get<std::string>("grey");
> +       if (mode == "grey") {
> +               impl_ = std::make_unique<AwbGrey>();
> +       } else if (mode == "bayes") {
> +               impl_ = std::make_unique<AwbBayes>();
> +               bayes_ = true;
> +       } else {
> +               LOG(Awb, Error) << "Unknown Awb algorithm: " << mode;
> +               return -EINVAL;
> +       }
> +
> +       LOG(Awb, Debug) << "Using Awb algorithm: " << mode;
> +
> +       int ret = impl_->init(tuningData);
> +       if (ret)
> +               return ret;
> +
> +       controls_[&controls::ColourTemperature] =
> +               ControlInfo(kMinColourTemperature, kMaxColourTemperature,
> +                           kDefaultColourTemperature);
> +       controls_[&controls::AwbEnable] = ControlInfo(false, true);
> +
> +       return parseModeConfigs(tuningData, controls::AwbAuto);
> +}
> +
> +/**
> + * \brief Configure the awb algorithm
> + * \param[in] state The awb active state
> + * \return 0 if successful, an error code otherwise
>   */
> +int AwbAlgorithmBase::configure(awb::ActiveState &state)
> +{
> +       state.manual.gains = RGB<double>{ 1.0 };
> +       auto gains = impl_->gainsFromColourTemperature(kDefaultColourTemperature);
> +       if (gains)
> +               state.automatic.gains = *gains;
> +       else
> +               state.automatic.gains = RGB<double>{ 1.0 };
> +
> +       state.autoEnabled = true;
> +       state.manual.temperatureK = kDefaultColourTemperature;
> +       state.automatic.temperatureK = kDefaultColourTemperature;
> +
> +       return 0;
> +}
>  
>  /**
> - * \fn AwbAlgorithm::controls()
> - * \brief Get the controls info map for this algorithm
> + * \brief Queue a Request to the awb algorithm
> + * \param[in] state The awb active state
> + * \param[in] frame The frame number
> + * \param[in] frameContext The awb frame context
> + * \param[in] controls The list of controls part of the Request
> + *
> + * Queue a new Request to the awb algorithm and modify its behaviour according
> + * to the provided controls.
>   *
> - * \return The controls info map
> + * The currently handled controls are:
> + * - controls::AwbEnable
> + * - controls::AwbMode
> + * - controls::ColourGains
> + * - controls::ColourTemperature
>   */
> +void AwbAlgorithmBase::queueRequest(awb::ActiveState &state,
> +                                   [[maybe_unused]] const uint32_t frame,
> +                                   awb::FrameContext &frameContext,
> +                                   const ControlList &controls)
> +{
> +       const auto &awbEnable = controls.get(controls::AwbEnable);
> +       if (awbEnable && *awbEnable != state.autoEnabled) {
> +               state.autoEnabled = *awbEnable;
> +
> +               LOG(Awb, Debug)
> +                       << (*awbEnable ? "Enabling" : "Disabling") << " Awb";
> +       }
> +
> +       auto mode = controls.get(controls::AwbMode);
> +       if (mode) {
> +               auto it = modes_.find(static_cast<controls::AwbModeEnum>(*mode));
> +               if (it == modes_.end()) {
> +                       LOG(Awb, Error) << "Unsupported Awb mode " << *mode;
> +                       return;
> +               }
> +
> +               currentMode_ = &it->second;
> +       }
> +
> +       frameContext.autoEnabled = state.autoEnabled;
> +
> +       if (frameContext.autoEnabled)
> +               return;
> +
> +       const auto &colourGains = controls.get(controls::ColourGains);
> +       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> +       bool update = false;
> +       if (colourGains) {
> +               state.manual.gains.r() = (*colourGains)[0];
> +               state.manual.gains.b() = (*colourGains)[1];
> +               /*
> +                * \todo Colour temperature reported in metadata is now
> +                * incorrect, as we can't deduce the temperature from the gains.
> +                * This will be fixed with the bayes AWB algorithm.
> +                */
> +               update = true;
> +       } else if (colourTemperature) {
> +               state.manual.temperatureK = *colourTemperature;
> +               const auto &gains = impl_->gainsFromColourTemperature(*colourTemperature);
> +               if (gains) {
> +                       state.manual.gains.r() = gains->r();
> +                       state.manual.gains.b() = gains->b();
> +                       update = true;
> +               }
> +       }
> +
> +       if (update)
> +               LOG(Awb, Debug)
> +                       << "Set colour gains to " << state.manual.gains;
> +
> +       frameContext.gains = state.manual.gains;
> +       frameContext.temperatureK = state.manual.temperatureK;
> +}
>  
>  /**
> - * \fn AwbAlgorithm::handleControls()
> - * \param[in] controls The controls to handle
> - * \brief Handle the controls supplied in a request
> + * \brief Set the gains and colour temperature values in \a frameContext
> + * \param[in] state The awb active state
> + * \param[in] frameContext The awb frame context
> + *
> + * If auto mode is enabled, take the most recently computed gains and use them
> + * for the current frame. Otherwise, if in manual mode, gains and colour
> + * temperature for a frame are set at queueRequest() time.
>   */
> +void AwbAlgorithmBase::prepare(awb::ActiveState &state,
> +                              awb::FrameContext &frameContext)
> +{
> +       if (frameContext.autoEnabled) {
> +               frameContext.gains = state.automatic.gains;
> +               frameContext.temperatureK = state.automatic.temperatureK;
> +       }
> +}
>  
>  /**
> + * \brief Process awb statistics to calculate gains and populate metadata
> + * \param[in] state The awb active state
> + * \param[in] frameContext The awb frame context
> + * \param[in] stats The awb statistics
> + * \param[in] lux The lux value as estimated by the IPA module
> + * \param[out] metadata The metadata list
> + *
> + * Process \a stats to calculate new gains and colour temperature and populate
> + * \a metadata with the results.
> + */
> +void AwbAlgorithmBase::process(awb::ActiveState &state,
> +                              awb::FrameContext &frameContext,
> +                              const AwbStats &stats, unsigned int lux,
> +                              ControlList &metadata)
> +{
> +       if (!stats.valid())
> +               return;
> +
> +       auto awbResult = impl_->calculateAwb(stats, lux, { currentMode_->ctLo,
> +                                                          currentMode_->ctHi });
> +
> +       /*
> +        * Clamp the gain values to the hardware, according to the gainMin_
> +        * and gainMax_ values.
> +        */
> +       awbResult.gains = awbResult.gains.clamp(gainMin_, gainMax_);
> +
> +       /* Smooth color gains adjustments. */
> +       double speed = 0.2;
> +       double ct = awbResult.temperatureK;
> +       ct = ct * speed + state.automatic.temperatureK * (1 - speed);
> +
> +       state.automatic.temperatureK = awbResult.temperatureK;
> +       state.automatic.gains = awbResult.gains * speed +
> +                               state.automatic.gains * (1 - speed);
> +
> +       /* Populate metadata. */
> +       metadata.set(controls::AwbEnable, frameContext.autoEnabled);
> +       metadata.set(controls::ColourGains, { static_cast<float>(frameContext.gains.r()),
> +                                             static_cast<float>(frameContext.gains.b()) });
> +       metadata.set(controls::ColourTemperature, frameContext.temperatureK);
> +
> +       LOG(Awb, Debug) << std::showpoint << "Means " << stats.rgbMeans()
> +                       << ", gains " << state.automatic.gains
> +                       << ", temp " << state.automatic.temperatureK << "K";
> +}
> +
> +/*
>   * \brief Parse the mode configurations from the tuning data
>   * \param[in] tuningData the ValueNode representing the tuning data
>   * \param[in] def The default value for the AwbMode control
>   *
>   * Utility function to parse the tuning data for an AwbMode entry and read all
> - * provided modes. It adds controls::AwbMode to AwbAlgorithm::controls_ and
> - * populates AwbAlgorithm::modes_. For a list of possible modes see \ref
> + * provided modes. It adds controls::AwbMode to AwbAlgorithmBase::controls_ and
> + * populates AwbAlgorithmBase::modes_. For a list of possible modes see \ref
>   * controls::AwbModeEnum.
>   *
>   * Each mode entry must contain a "lo" and "hi" key to specify the lower and
> @@ -156,15 +499,23 @@ namespace ipa {
>   *       ...
>   * \endcode
>   *
> - * If \a def is supplied but not contained in the the \a tuningData, -EINVAL is
> + * If \a def is supplied but not contained in the \a tuningData, -EINVAL is
>   * returned.
>   *
> + * AwbModes are only used by the AwbBayes implementation.
> + *
>   * \sa controls::AwbModeEnum
>   * \return Zero on success, negative error code otherwise
>   */
> -int AwbAlgorithm::parseModeConfigs(const ValueNode &tuningData,
> -                                  const ControlValue &def)
> +int AwbAlgorithmBase::parseModeConfigs(const ValueNode &tuningData,
> +                                      const ControlValue &def)
>  {
> +       if (!bayes_) {
> +               /* AwbGrey does not support and does not use modes. */
> +               currentMode_ = &AwbGreyMode;
> +               return 0;
> +       }
> +
>         std::vector<ControlValue> availableModes;
>  
>         const ValueNode &modes = tuningData[controls::AwbMode.name()];
> @@ -227,37 +578,83 @@ int AwbAlgorithm::parseModeConfigs(const ValueNode &tuningData,
>         }
>  
>         controls_[&controls::AwbMode] = ControlInfo(availableModes, def);
> +       currentMode_ = &modes_[controls::AwbAuto];
>  
>         return 0;
>  }
>  
>  /**
> - * \class AwbAlgorithm::ModeConfig
> - * \brief Holds the configuration of a single AWB mode
> + * \var AwbAlgorithmBase::controls_
> + * \brief Controls info map for the controls registered by the awb algorithm
> + */
> +
> +/**
> + * \var AwbAlgorithmBase::gainMin_
> + * \brief The minimum supported gain value
>   *
> - * AWB modes limit the regulation of the AWB algorithm to a specific range of
> - * colour temperatures.
> + * Minimum gain value used to clamp the awb algorithm calculation results in the
> + * range supported by the platform awb engine.
> + *
> + * The min and max gain values are initialized by AwbAlgorithm::init().
>   */
>  
>  /**
> - * \var AwbAlgorithm::ModeConfig::ctLo
> + * \var AwbAlgorithmBase::gainMax_
> + * \brief The maximum supported gain value
> + *
> + * Maximum gain value used to clamp the awb algorithm calculation results in the
> + * range supported by the platform awb engine.
> + *
> + * The min and max gain values are initialized by AwbAlgorithm::init().
> + */
> +
> +/*
> + * \class AwbAlgorithmBase::ModeConfig
> + * \brief Holds the configuration of a single awb mode
> + *
> + * AWB modes limit the regulation of the awb algorithm to a specific range of
> + * colour temperatures. Use by AwbBayes only.
> + */
> +
> +/*
> + * \var AwbAlgorithmBase::ModeConfig::ctLo
>   * \brief The lowest valid colour temperature of that mode
>   */
>  
> -/**
> - * \var AwbAlgorithm::ModeConfig::ctHi
> +/*
> + * \var AwbAlgorithmBase::ModeConfig::ctHi
>   * \brief The highest valid colour temperature of that mode
>   */
>  
> +/*
> + * \var AwbAlgorithmBase::modes_
> + * \brief Map of all configured modes
> + * \sa AwbAlgorithmBase::parseModeConfigs
> + */
> +
>  /**
> - * \var AwbAlgorithm::controls_
> - * \brief Controls info map for the controls provided by the algorithm
> + * \class AwbAlgorithm
> + * \brief The libipa awb algorithm
> + * \tparam Q The fixedpoint register representation of gain values
> + *
> + * Implement the awb algorithm for libipa.
> + *
> + * The AwbAlgorithm class implements an interface similar in spirit to the one
> + * of the Algorithm class. IPA modules are expected to store an instance of
> + * AwbAlgorithm as class member, template it with the awb engine gain register
> + * representation and call its function in their implementations of the
> + * Algorithm interface.
> + *
> + * The AwbAlgorithm instantiates an AwbImplementation implementation (AwbGrey or
> + * AwbBayes) at init() time by parsing the tuning data and uses it to compute
> + * the RGB gains and estimate a colour temperature given a set of statistics
> + * in the form of a AwbStats derived implementation.
>   */
>  
>  /**
> - * \var AwbAlgorithm::modes_
> - * \brief Map of all configured modes
> - * \sa AwbAlgorithm::parseModeConfigs
> + * \fn AwbAlgorithm::init()
> + * \param[in] controls The info map of the IPA controls
> + * \copydoc AwbAlgorithmBase::init()
>   */
>  
>  } /* namespace ipa */
> diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> index 09c00e47d604..f453e1f9fd3d 100644
> --- a/src/ipa/libipa/awb.h
> +++ b/src/ipa/libipa/awb.h
> @@ -2,11 +2,12 @@
>  /*
>   * Copyright (C) 2024 Ideas on Board Oy
>   *
> - * Generic AWB algorithms
> + * libIPA AWB algorithms
>   */
>  
>  #pragma once
>  
> +#include <array>
>  #include <map>
>  #include <optional>
>  
> @@ -20,46 +21,119 @@ namespace libcamera {
>  
>  namespace ipa {
>  
> -struct AwbResult {
> +namespace awb {
> +
> +struct Context {

As written above, I don't particularly like that new name.

>         RGB<double> gains;
> -       double colourTemperature;
> +       unsigned int temperatureK;
> +};
> +
> +struct ActiveState {
> +       Context manual;
> +       Context automatic;
> +
> +       bool autoEnabled;
> +};
> +
> +struct FrameContext : public Context {
> +       bool autoEnabled;
>  };
>  
> +} /* namespace awb */
> +
>  struct AwbStats {
> -       virtual double computeColourError(const RGB<double> &gains) const = 0;
> -       virtual RGB<double> rgbMeans() const = 0;

This was designed as the interface that the platform specific
implementation has to provide. Now it is interface and partial
implementation at the same time. Can we keep the interface separate and
- if we really want to have this implementation at a central place -
maybe call it "AwbStatsMinimal" or something?

> +       AwbStats() = default;
> +       AwbStats(const RGB<double> &means);
> +       virtual ~AwbStats() = default;
> +
> +       bool valid() const;
> +
> +       virtual double rgRatio() const { return rg_; }
> +       virtual double bgRatio() const { return bg_; }
> +       virtual double computeColourError(const RGB<double> &gains) const;
> +       virtual RGB<double> rgbMeans() const;

rgbMeans and xxRatio seem to server a very similar purpose now. I need
to look further.

...

Coming back. I don't understand why we can't go for rgbMeans() only. I
think the old interface should be sufficient. For cases where the
hardware only provides a ratio, we could IMHO calculate a means value
based on that.

I know we talked about this in Nice, but now reading it, I can't see the
reason anymore.

>  
>  protected:
> -       ~AwbStats() = default;
> +       virtual double minColourValue() const = 0;
> +
> +       RGB<double> rgbMeans_;
> +       double rg_;
> +       double bg_;
>  };
>  
> -class AwbAlgorithm
> +class AwbImplementation
>  {
>  public:
> -       virtual ~AwbAlgorithm() = default;
> -
> +       virtual ~AwbImplementation() = default;
>         virtual int init(const ValueNode &tuningData) = 0;
> -       virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
> -       virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
> +       virtual awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> +                                         std::array<double, 2> ranges) = 0;
> +       virtual std::optional<RGB<double>>
> +       gainsFromColourTemperature(double temperatureK) = 0;
> +};
>  
> -       const ControlInfoMap::Map &controls() const
> -       {
> -               return controls_;
> -       }
> +class AwbAlgorithmBase
> +{
> +public:
> +       int init(const ValueNode &tuningData);
> +
> +       int configure(awb::ActiveState &state);
> +
> +       void queueRequest(awb::ActiveState &state,
> +                         const uint32_t frame,
> +                         awb::FrameContext &frameContext,
> +                         const ControlList &controls);
> +
> +       void prepare(awb::ActiveState &state, awb::FrameContext &frameContext);
>  
> -       virtual void handleControls([[maybe_unused]] const ControlList &controls) {}
> +       void process(awb::ActiveState &state, awb::FrameContext &frameContext,
> +                    const AwbStats &stats, unsigned int lux,
> +                    ControlList &metadata);
>  
>  protected:
> -       int parseModeConfigs(const ValueNode &tuningData,
> -                            const ControlValue &def = {});
> +       AwbAlgorithmBase() = default;
>  
> +       ControlInfoMap::Map controls_;
> +       float gainMin_;
> +       float gainMax_;
> +
> +private:
>         struct ModeConfig {
>                 double ctHi;
>                 double ctLo;
>         };
>  
> -       ControlInfoMap::Map controls_;
> -       std::map<controls::AwbModeEnum, AwbAlgorithm::ModeConfig> modes_;
> +       /* AwbGrey does not support modes; */
> +       static constexpr ModeConfig AwbGreyMode = { 0.0, 0.0 };
> +
> +       int parseModeConfigs(const ValueNode &tuningData,
> +                            const ControlValue &def = {});
> +
> +       std::map<controls::AwbModeEnum, AwbAlgorithmBase::ModeConfig> modes_;
> +       const ModeConfig *currentMode_ = nullptr;
> +       std::unique_ptr<AwbImplementation> impl_;
> +       bool bayes_ = false;
> +};
> +
> +template<typename Q>
> +class AwbAlgorithm : public AwbAlgorithmBase
> +{
> +public:
> +       int init(const ValueNode &tuningData, ControlInfoMap::Map &controls)
> +       {
> +               AwbAlgorithmBase::init(tuningData);
> +
> +               gainMin_ = std::max(Q::TraitsType::min, 1.0f);
> +               gainMax_ = Q::TraitsType::max;
> +
> +               controls_[&controls::ColourGains] =
> +                       ControlInfo(gainMin_, gainMax_,
> +                                   Span<const float, 2>{ { 1.0f, 1.0f } });
> +
> +               controls.insert(controls_.begin(), controls_.end());
> +
> +               return 0;
> +       }
>  };

This feels like an overkill for a template. The only reason is to deduce
the min max values for the gainMin and gainMax values. Why not adding a
gainRange param to init()?

>  
>  } /* namespace ipa */
> diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp
> index 9fd85e5a4505..34d86ce6938b 100644
> --- a/src/ipa/libipa/awb_bayes.cpp
> +++ b/src/ipa/libipa/awb_bayes.cpp
> @@ -140,11 +140,6 @@ void Interpolator<Pwl>::interpolate(const Pwl &a, const Pwl &b, Pwl &dest, doubl
>   * \brief How far to wander off CT curve towards "more green"
>   */
>  
> -/**
> - * \var AwbBayes::currentMode_
> - * \brief The currently selected mode
> - */
> -
>  int AwbBayes::init(const ValueNode &tuningData)
>  {
>         int ret = colourGainCurve_.readYaml(tuningData["colourGains"], "ct", "gains");
> @@ -172,14 +167,6 @@ int AwbBayes::init(const ValueNode &tuningData)
>                 return ret;
>         }
>  
> -       ret = parseModeConfigs(tuningData, controls::AwbAuto);
> -       if (ret) {
> -               LOG(Awb, Error)
> -                       << "Failed to parse mode parameter from tuning file";
> -               return ret;
> -       }
> -       currentMode_ = &modes_[controls::AwbAuto];
> -
>         transversePos_ = tuningData["transversePos"].get<double>(0.01);
>         transverseNeg_ = tuningData["transverseNeg"].get<double>(0.01);
>         if (transversePos_ <= 0 || transverseNeg_ <= 0) {
> @@ -260,18 +247,6 @@ int AwbBayes::readPriors(const ValueNode &tuningData)
>         return 0;
>  }
>  
> -void AwbBayes::handleControls(const ControlList &controls)
> -{
> -       auto mode = controls.get(controls::AwbMode);
> -       if (mode) {
> -               auto it = modes_.find(static_cast<controls::AwbModeEnum>(*mode));
> -               if (it != modes_.end())
> -                       currentMode_ = &it->second;
> -               else
> -                       LOG(Awb, Error) << "Unsupported AWB mode " << *mode;
> -       }
> -}
> -
>  std::optional<RGB<double>> AwbBayes::gainsFromColourTemperature(double colourTemperature)
>  {
>         /*
> @@ -283,7 +258,9 @@ std::optional<RGB<double>> AwbBayes::gainsFromColourTemperature(double colourTem
>         return RGB<double>{ { gains[0], 1.0, gains[1] } };
>  }
>  
> -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
> +awb::Context
> +AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux,
> +                      std::array<double, 2> ranges)

Nit: ranges is confusion as it is a single range. So I'd rename it to
range or even ctRange to make it easier to understand everywhere.

And I guess it could be Span.

>  {
>         ipa::Pwl prior;
>         if (lux > 0) {
> @@ -295,7 +272,7 @@ AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
>                 prior.append(0, 1.0);
>         }
>  
> -       double t = coarseSearch(prior, stats);
> +       double t = coarseSearch(prior, stats, ranges);
>         double r = ctR_.eval(t);
>         double b = ctB_.eval(t);
>         LOG(Awb, Debug)
> @@ -316,14 +293,15 @@ AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
>                 << "After fine search: r " << r << " b " << b << " (gains r "
>                 << 1 / r << " b " << 1 / b << ")";
>  
> -       return { { { 1.0 / r, 1.0, 1.0 / b } }, t };
> +       return { { { 1.0 / r, 1.0, 1.0 / b } }, static_cast<unsigned int>(t) };
>  }
>  
> -double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) const
> +double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats,
> +                             std::array<double, 2> ranges) const
>  {
>         std::vector<Pwl::Point> points;
>         size_t bestPoint = 0;
> -       double t = currentMode_->ctLo;
> +       double t = ranges[0];
>         int spanR = -1;
>         int spanB = -1;
>         LimitsRecorder<double> errorLimits;
> @@ -345,14 +323,14 @@ double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) cons
>                 if (points.back().y() < points[bestPoint].y())
>                         bestPoint = points.size() - 1;
>  
> -               if (t == currentMode_->ctHi)
> +               if (t == ranges[1])
>                         break;
>  
>                 /*
>                  * Ensure even steps along the r/b curve by scaling them by the
>                  * current t.
>                  */
> -               t = std::min(t + t / 10 * kSearchStep, currentMode_->ctHi);
> +               t = std::min(t + t / 10 * kSearchStep, ranges[1]);
>         }
>  
>         t = points[bestPoint].x();
> diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h
> index 1e3373676bc0..8264bcb09f38 100644
> --- a/src/ipa/libipa/awb_bayes.h
> +++ b/src/ipa/libipa/awb_bayes.h
> @@ -20,22 +20,23 @@ namespace libcamera {
>  
>  namespace ipa {
>  
> -class AwbBayes : public AwbAlgorithm
> +class AwbBayes : public AwbImplementation
>  {
>  public:
>         AwbBayes() = default;
>  
>         int init(const ValueNode &tuningData) override;
> -       AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;
> +       awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> +                                 std::array<double, 2> ranges) override;
>         std::optional<RGB<double>> gainsFromColourTemperature(double temperatureK) override;
> -       void handleControls(const ControlList &controls) override;
>  
>  private:
>         int readPriors(const ValueNode &tuningData);
>  
>         void fineSearch(double &t, double &r, double &b, ipa::Pwl const &prior,
>                         const AwbStats &stats) const;
> -       double coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) const;
> +       double coarseSearch(const ipa::Pwl &prior, const AwbStats &stats,
> +                           std::array<double, 2> ranges) const;
>         double interpolateQuadratic(ipa::Pwl::Point const &a,
>                                     ipa::Pwl::Point const &b,
>                                     ipa::Pwl::Point const &c) const;
> @@ -50,8 +51,6 @@ private:
>  
>         double transversePos_;
>         double transverseNeg_;
> -
> -       ModeConfig *currentMode_ = nullptr;
>  };
>  
>  } /* namespace ipa */
> diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> index b14b096810ae..2e31492bda3c 100644
> --- a/src/ipa/libipa/awb_grey.cpp
> +++ b/src/ipa/libipa/awb_grey.cpp
> @@ -60,6 +60,7 @@ int AwbGrey::init(const ValueNode &tuningData)
>   * \brief Calculate AWB data from the given statistics
>   * \param[in] stats The statistics to use for the calculation
>   * \param[in] lux The lux value of the scene
> + * \param[in] ranges The colour temperature search limits (AwbBayes only)

Maybe invert this, to say "range support is not implemented in this
algorithm"? Then we could drop the reference to AwbBayes.

>   *
>   * The colour temperature is estimated based on the colours::estimateCCT()
>   * function. The gains are calculated purely based on the RGB means provided by
> @@ -70,20 +71,23 @@ int AwbGrey::init(const ValueNode &tuningData)
>   *
>   * \return The AWB result
>   */
> -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux)
> +awb::Context
> +AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux,
> +                     [[maybe_unused]] std::array<double, 2> ranges)
>  {
> -       AwbResult result;
> +       awb::Context result;
>         auto means = stats.rgbMeans();
> -       result.colourTemperature = estimateCCT(means);
> +       result.temperatureK = estimateCCT(means);
>  
>         /*
> -        * Estimate the red and blue gains to apply in a grey world. The green
> -        * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the
> -        * divisor to a minimum value of 1.0.
> +        * Calculate the red and blue gains to apply in a grey world by simply
> +        * inverting the red/green and blue/green ratios as reported in
> +        * statistics. The green gain is hardcoded to 1.0.
>          */
> -       result.gains.r() = means.g() / std::max(means.r(), 1.0);
> +       result.gains.r() = 1.0 / stats.rgRatio();
>         result.gains.g() = 1.0;
> -       result.gains.b() = means.g() / std::max(means.b(), 1.0);
> +       result.gains.b() = 1.0 / stats.bgRatio();
> +
>         return result;
>  }
>  
> @@ -96,12 +100,13 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned
>   * gains configured in the colour temperature curve.
>   *
>   * \return The colour gains if a colour temperature curve is available,
> - * [1, 1, 1] otherwise.
> + * std::nullopt otherwise
>   */
>  std::optional<RGB<double>> AwbGrey::gainsFromColourTemperature(double colourTemperature)
>  {
>         if (!colourGainCurve_) {
> -               LOG(Awb, Error) << "No gains defined";
> +               LOG(Awb, Info) << "No gains curve defined, "
> +                              << "unable to interpolate gains to colour temperature";
>                 return std::nullopt;
>         }
>  
> diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> index 154a2af97f15..438fb850c223 100644
> --- a/src/ipa/libipa/awb_grey.h
> +++ b/src/ipa/libipa/awb_grey.h
> @@ -18,13 +18,14 @@ namespace libcamera {
>  
>  namespace ipa {
>  
> -class AwbGrey : public AwbAlgorithm
> +class AwbGrey : public AwbImplementation
>  {
>  public:
>         AwbGrey() = default;
>  
>         int init(const ValueNode &tuningData) override;
> -       AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;
> +       awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> +                                 std::array<double, 2> ranges) override;
>         std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) override;
>  
>  private:
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 0eb05c2b6e1a..0ad0794b4a0f 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -8,17 +8,12 @@
>  #include "awb.h"
>  
>  #include <algorithm>
> -#include <ios>
>  
>  #include <libcamera/base/log.h>
>  
> -#include <libcamera/control_ids.h>
> -
>  #include <libcamera/ipa/core_ipa_interface.h>
>  
> -#include "libipa/awb_bayes.h"
> -#include "libipa/awb_grey.h"
> -#include "libipa/colours.h"
> +#include "libcamera/internal/vector.h"
>  
>  /**
>   * \file awb.h
> @@ -28,54 +23,29 @@ namespace libcamera {
>  
>  namespace ipa::rkisp1::algorithms {
>  
> -/**
> - * \class Awb
> - * \brief Manage the white balance with automatic and manual controls
> - */
> -
>  LOG_DEFINE_CATEGORY(RkISP1Awb)
>  
> -constexpr int32_t kMinColourTemperature = 2500;
> -constexpr int32_t kMaxColourTemperature = 10000;
> -constexpr int32_t kDefaultColourTemperature = 5000;
> -
> -/* Minimum mean value below which AWB can't operate. */
> -constexpr double kMeanMinThreshold = 2.0;
> -
>  class RkISP1AwbStats final : public AwbStats
>  {
>  public:
> -       RkISP1AwbStats(const RGB<double> &rgbMeans)
> -               : rgbMeans_(rgbMeans)
> +       RkISP1AwbStats() = default;
> +       RkISP1AwbStats(const RGB<double> means)
> +               : AwbStats(means)
>         {
> -               rg_ = rgbMeans_.r() / rgbMeans_.g();
> -               bg_ = rgbMeans_.b() / rgbMeans_.g();
>         }
>  
> -       double computeColourError(const RGB<double> &gains) const override
> +       /* Minimum mean value below which AWB can't operate. */
> +       double minColourValue() const override
>         {
> -               /*
> -                * Compute the sum of the squared colour error (non-greyness) as
> -                * it appears in the log likelihood equation.
> -                */
> -               double deltaR = gains.r() * rg_ - 1.0;
> -               double deltaB = gains.b() * bg_ - 1.0;
> -               double delta2 = deltaR * deltaR + deltaB * deltaB;
> -
> -               return delta2;
> +               return 2.0;
>         }
> -
> -       RGB<double> rgbMeans() const override
> -       {
> -               return rgbMeans_;
> -       }
> -
> -private:
> -       RGB<double> rgbMeans_;
> -       double rg_;
> -       double bg_;
>  };
>  
> +/**
> + * \class Awb
> + * \brief Manage the white balance with automatic and manual controls
> + */
> +
>  Awb::Awb()
>         : rgbMode_(false)
>  {
> @@ -86,40 +56,7 @@ Awb::Awb()
>   */
>  int Awb::init(IPAContext &context, const ValueNode &tuningData)
>  {
> -       auto &cmap = context.ctrlMap;
> -       cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,
> -                                                        kMaxColourTemperature,
> -                                                        kDefaultColourTemperature);
> -       cmap[&controls::AwbEnable] = ControlInfo(false, true);
> -
> -       cmap[&controls::ColourGains] = ControlInfo(0.0f, 3.996f,
> -                                                  Span<const float, 2>{ { 1.0f, 1.0f } });
> -
> -       if (!tuningData.contains("algorithm"))
> -               LOG(RkISP1Awb, Info) << "No AWB algorithm specified."
> -                                    << " Default to grey world";
> -
> -       auto mode = tuningData["algorithm"].get<std::string>("grey");
> -       if (mode == "grey") {
> -               awbAlgo_ = std::make_unique<AwbGrey>();
> -       } else if (mode == "bayes") {
> -               awbAlgo_ = std::make_unique<AwbBayes>();
> -       } else {
> -               LOG(RkISP1Awb, Error) << "Unknown AWB algorithm: " << mode;
> -               return -EINVAL;
> -       }
> -       LOG(RkISP1Awb, Debug) << "Using AWB algorithm: " << mode;
> -
> -       int ret = awbAlgo_->init(tuningData);
> -       if (ret) {
> -               LOG(RkISP1Awb, Error) << "Failed to init AWB algorithm";
> -               return ret;
> -       }
> -
> -       const auto &src = awbAlgo_->controls();
> -       cmap.insert(src.begin(), src.end());
> -
> -       return 0;
> +       return awbAlgo_.init(tuningData, context.ctrlMap);
>  }
>  
>  /**
> @@ -128,16 +65,7 @@ int Awb::init(IPAContext &context, const ValueNode &tuningData)
>  int Awb::configure(IPAContext &context,
>                    const IPACameraSensorInfo &configInfo)
>  {
> -       context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> -       auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> -       if (gains)
> -               context.activeState.awb.automatic.gains = *gains;
> -       else
> -               context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
> -
> -       context.activeState.awb.autoEnabled = true;
> -       context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> -       context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> +       awbAlgo_.configure(context.activeState.awb);
>  
>         /*
>          * Define the measurement window for AWB as a centered rectangle
> @@ -156,56 +84,12 @@ int Awb::configure(IPAContext &context,
>  /**
>   * \copydoc libcamera::ipa::Algorithm::queueRequest
>   */
> -void Awb::queueRequest(IPAContext &context,
> -                      [[maybe_unused]] const uint32_t frame,
> +void Awb::queueRequest(IPAContext &context, const uint32_t frame,
>                        IPAFrameContext &frameContext,
>                        const ControlList &controls)
>  {
> -       auto &awb = context.activeState.awb;
> -
> -       const auto &awbEnable = controls.get(controls::AwbEnable);
> -       if (awbEnable && *awbEnable != awb.autoEnabled) {
> -               awb.autoEnabled = *awbEnable;
> -
> -               LOG(RkISP1Awb, Debug)
> -                       << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> -       }
> -
> -       awbAlgo_->handleControls(controls);
> -
> -       frameContext.awb.autoEnabled = awb.autoEnabled;
> -
> -       if (awb.autoEnabled)
> -               return;
> -
> -       const auto &colourGains = controls.get(controls::ColourGains);
> -       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> -       bool update = false;
> -       if (colourGains) {
> -               awb.manual.gains.r() = (*colourGains)[0];
> -               awb.manual.gains.b() = (*colourGains)[1];
> -               /*
> -                * \todo Colour temperature reported in metadata is now
> -                * incorrect, as we can't deduce the temperature from the gains.
> -                * This will be fixed with the bayes AWB algorithm.
> -                */
> -               update = true;
> -       } else if (colourTemperature) {
> -               awb.manual.temperatureK = *colourTemperature;
> -               const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> -               if (gains) {
> -                       awb.manual.gains.r() = gains->r();
> -                       awb.manual.gains.b() = gains->b();
> -                       update = true;
> -               }
> -       }
> -
> -       if (update)
> -               LOG(RkISP1Awb, Debug)
> -                       << "Set colour gains to " << awb.manual.gains;
> -
> -       frameContext.awb.gains = awb.manual.gains;
> -       frameContext.awb.temperatureK = awb.manual.temperatureK;
> +       awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,
> +                             controls);
>  }
>  
>  /**
> @@ -214,15 +98,7 @@ void Awb::queueRequest(IPAContext &context,
>  void Awb::prepare(IPAContext &context, const uint32_t frame,
>                   IPAFrameContext &frameContext, RkISP1Params *params)
>  {
> -       /*
> -        * This is the latest time we can read the active state. This is the
> -        * most up-to-date automatic values we can read.
> -        */
> -       if (frameContext.awb.autoEnabled) {
> -               const auto &awb = context.activeState.awb;
> -               frameContext.awb.gains = awb.automatic.gains;
> -               frameContext.awb.temperatureK = awb.automatic.temperatureK;
> -       }
> +       awbAlgo_.prepare(context.activeState.awb, frameContext.awb);
>  
>         auto gainConfig = params->block<BlockType::AwbGain>();
>         gainConfig.setEnabled(true);
> @@ -291,68 +167,27 @@ void Awb::process(IPAContext &context,
>                   const rkisp1_stat_buffer *stats,
>                   ControlList &metadata)
>  {
> -       IPAActiveState &activeState = context.activeState;
> +       RkISP1AwbStats awbStats = calculateRgbMeans(frameContext, stats);

Uh not too easy to see what's going on here. Maybe we should have done a
bit of preparatory refactoring on rkisp1 first.

This partially defeats the initial intention of AwbStats. I think it
would be clearer if we do

RkISP1AwbStats awbStats(frameContext, stats);

and move the code from calculateRgbMeans into RkISP1AwbStats. This would
make it clearer that AwbStats should *not* be a single rgb means triple.
That is unfortunately the only thing the rkisp1 can provide, but it
shouldn't be the usual implementation (especially the
computeColourError() only makes real sense if there is more than one
meaurement.).

>  
> -       metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
> -       metadata.set(controls::ColourGains, {
> -                       static_cast<float>(frameContext.awb.gains.r()),
> -                       static_cast<float>(frameContext.awb.gains.b())
> -               });
> -       metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
> +       awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,
> +                        frameContext.lux.lux, metadata);
> +}
>  
> +RkISP1AwbStats Awb::calculateRgbMeans(const IPAFrameContext &frameContext,
> +                                     const rkisp1_stat_buffer *stats) const
> +{
>         if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {
>                 LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
> -               return;
> +               return {};
>         }
>  
> -       const rkisp1_cif_isp_stat *params = &stats->params;
> -       const rkisp1_cif_isp_awb_stat *awb = &params->awb;
> +       const rkisp1_cif_isp_awb_stat *awb = &stats->params.awb;
>  
>         if (awb->awb_mean[0].cnt == 0) {
>                 LOG(RkISP1Awb, Debug) << "AWB statistics are empty";
> -               return;
> +               return {};
>         }
>  
> -       RGB<double> rgbMeans = calculateRgbMeans(frameContext, awb);
> -
> -       /*
> -        * If the means are too small we don't have enough information to
> -        * meaningfully calculate gains. Freeze the algorithm in that case.
> -        */
> -       if (rgbMeans.r() < kMeanMinThreshold && rgbMeans.g() < kMeanMinThreshold &&
> -           rgbMeans.b() < kMeanMinThreshold)
> -               return;
> -
> -       RkISP1AwbStats awbStats{ rgbMeans };
> -       AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);
> -
> -       /*
> -        * Clamp the gain values to the hardware, which expresses gains as Q2.8
> -        * unsigned integer values. Set the minimum just above zero to avoid
> -        * divisions by zero when computing the raw means in subsequent
> -        * iterations.
> -        */
> -       awbResult.gains = awbResult.gains.clamp(1.0 / 256, 1023.0 / 256);
> -
> -       /* Filter the values to avoid oscillations. */
> -       double speed = 0.2;
> -       double ct = awbResult.colourTemperature;
> -       ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);
> -       awbResult.gains = awbResult.gains * speed +
> -                         activeState.awb.automatic.gains * (1 - speed);
> -
> -       activeState.awb.automatic.temperatureK = static_cast<unsigned int>(ct);
> -       activeState.awb.automatic.gains = awbResult.gains;
> -
> -       LOG(RkISP1Awb, Debug)
> -               << std::showpoint
> -               << "Means " << rgbMeans << ", gains "
> -               << activeState.awb.automatic.gains << ", temp "
> -               << activeState.awb.automatic.temperatureK << "K";
> -}
> -
> -RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const
> -{
>         Vector<double, 3> rgbMeans;
>  
>         if (rgbMode_) {
> @@ -419,7 +254,7 @@ RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rk
>          */
>         rgbMeans /= frameContext.awb.gains.max(0.01);
>  
> -       return rgbMeans;
> +       return RkISP1AwbStats(rgbMeans);

Reading this change I realize I should have moved this function in
RkISP1AwbStats in first place.

Best regards,
Stefan

>  }
>  
>  REGISTER_IPA_ALGORITHM(Awb, "Awb")
> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
> index 60d9ef111495..c7cb59a75bde 100644
> --- a/src/ipa/rkisp1/algorithms/awb.h
> +++ b/src/ipa/rkisp1/algorithms/awb.h
> @@ -7,17 +7,25 @@
>  
>  #pragma once
>  
> -#include "libcamera/internal/vector.h"
> +#include <linux/rkisp1-config.h>
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/value_node.h"
>  
>  #include "libipa/awb.h"
> -#include "libipa/interpolator.h"
> +#include "libipa/fixedpoint.h"
>  
>  #include "algorithm.h"
> +#include "ipa_context.h"
> +#include "params.h"
>  
>  namespace libcamera {
>  
>  namespace ipa::rkisp1::algorithms {
>  
> +class RkISP1AwbStats;
> +
>  class Awb : public Algorithm
>  {
>  public:
> @@ -38,10 +46,10 @@ public:
>                      ControlList &metadata) override;
>  
>  private:
> -       RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext,
> -                                     const rkisp1_cif_isp_awb_stat *awb) const;
> +       RkISP1AwbStats calculateRgbMeans(const IPAFrameContext &frameContext,
> +                                        const rkisp1_stat_buffer *stats) const;
>  
> -       std::unique_ptr<AwbAlgorithm> awbAlgo_;
> +       AwbAlgorithm<UQ<2, 8>> awbAlgo_;
>  
>         bool rgbMode_;
>  };
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index e5f70d934d67..c36c1f7e0084 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -25,6 +25,7 @@
>  #include "libcamera/internal/vector.h"
>  
>  #include "libipa/agc_mean_luminance.h"
> +#include "libipa/awb.h"
>  #include "libipa/camera_sensor_helper.h"
>  #include "libipa/fc_queue.h"
>  #include "libipa/fixedpoint.h"
> @@ -48,15 +49,17 @@ struct IPAHwSettings {
>         bool compand;
>  };
>  
> +struct RKISP1AwbSession {
> +       struct rkisp1_cif_isp_window measureWindow;
> +       bool enabled;
> +};
> +
>  struct IPASessionConfiguration {
>         struct {
>                 struct rkisp1_cif_isp_window measureWindow;
>         } agc;
>  
> -       struct {
> -               struct rkisp1_cif_isp_window measureWindow;
> -               bool enabled;
> -       } awb;
> +       struct RKISP1AwbSession awb;
>  
>         struct {
>                 bool supported;
> @@ -100,17 +103,7 @@ struct IPAActiveState {
>                 utils::Duration maxFrameDuration;
>         } agc;
>  
> -       struct {
> -               struct AwbState {
> -                       RGB<double> gains;
> -                       unsigned int temperatureK;
> -               };
> -
> -               AwbState manual;
> -               AwbState automatic;
> -
> -               bool autoEnabled;
> -       } awb;
> +       ipa::awb::ActiveState awb;
>  
>         struct {
>                 Matrix<float, 3, 3> manual;
> @@ -176,11 +169,7 @@ struct IPAFrameContext : public FrameContext {
>                 bool autoGainModeChange;
>         } agc;
>  
> -       struct {
> -               RGB<double> gains;
> -               bool autoEnabled;
> -               unsigned int temperatureK;
> -       } awb;
> +       ipa::awb::FrameContext awb;
>  
>         struct {
>                 float actualBrightness;
> 
> -- 
> 2.54.0
>

Hi Stefan

On Wed, Jun 24, 2026 at 05:04:41PM +0200, Stefan Klug wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> Quoting Jacopo Mondi (2026-06-23 15:54:56)
> > The current implementation of AwbAlgorithm is the following one:
> >
> > AwbAlgorithm o---- RkISP1Awb
> >     ^
> >     |
> >  |-----|
> >  |     |
> > Bayes Grey
> >
> > The platform-specific Awb implementation instantiate one of
> > the Bayes or Grey Awb helper classes and both inherit from the
> > AwbAlgorithm base class.
> >
> > The handling of libcamera controls is split between the base and the
> > derived (mostly Bayes) class.
> >
> > We want instead the AwbAlgorithm to handle all-things-libcamera and let
> > the Bayes and Grey classes implement the logic. In order to achieve that
> > use composition also in the AwbAlgorithm class and introduce an
> > AwbImplementation base class from which Bayes and Grey derive from.
> >
> > AwbImplementation o---- AwbAlgorithmBase
> >     ^                           ^
> >     |                           |
> >  |-----|                AwbAlgorithm<Q<x,y>> o---- RkISP1Awb
> >  |     |
> > Bayes Grey
> >
> > The AwbAlgorithm<> class is instantiated with a Q<x, y> template
> > argument that represents the platform-specific register format which
> > is used to initialize controls and clamp gains to the hardware limits.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> >  src/ipa/libipa/awb.cpp            | 527 +++++++++++++++++++++++++++++++++-----
> >  src/ipa/libipa/awb.h              | 114 +++++++--
> >  src/ipa/libipa/awb_bayes.cpp      |  42 +--
> >  src/ipa/libipa/awb_bayes.h        |  11 +-
> >  src/ipa/libipa/awb_grey.cpp       |  25 +-
> >  src/ipa/libipa/awb_grey.h         |   5 +-
> >  src/ipa/rkisp1/algorithms/awb.cpp | 223 +++-------------
> >  src/ipa/rkisp1/algorithms/awb.h   |  18 +-
> >  src/ipa/rkisp1/ipa_context.h      |  29 +--
> >  9 files changed, 640 insertions(+), 354 deletions(-)
> >
> > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> > index 29a78069c52c..94707faf421b 100644
> > --- a/src/ipa/libipa/awb.cpp
> > +++ b/src/ipa/libipa/awb.cpp
> > @@ -2,7 +2,7 @@
> >  /*
> >   * Copyright (C) 2024 Ideas on Board Oy
> >   *
> > - * Generic AWB algorithms
> > + * libIPA Awb algorithms
> >   */
> >
> >  #include "awb.h"
> > @@ -11,9 +11,16 @@
> >
> >  #include <libcamera/control_ids.h>
> >
> > +#include "awb_bayes.h"
> > +#include "awb_grey.h"
> > +
> > +constexpr int32_t kMinColourTemperature = 2500;
> > +constexpr int32_t kMaxColourTemperature = 10000;
> > +constexpr int32_t kDefaultColourTemperature = 5000;
> > +
> >  /**
> >   * \file awb.h
> > - * \brief Base classes for AWB algorithms
> > + * \brief libipa awb implementation
> >   */
> >
> >  namespace libcamera {
> > @@ -22,34 +29,74 @@ LOG_DEFINE_CATEGORY(Awb)
> >
> >  namespace ipa {
> >
> > +namespace awb {
> > +
> >  /**
> > - * \class AwbResult
> > - * \brief The result of an AWB calculation
> > + * \struct Context
>
> I named it Result because it is the result of a algorithm run. For me,
> context implies a state that is needed for the next run of an algorithm.
> Both implemented awb algorithms don't need a context as they always
> start from scratch based on the stats.
>
> In the case of FrameContext it makes sense as that is the context that
> is used to process the frame.
>

I see.

I think I initially had the result of Awb calculation as a separate
type, but someone noticed the fields were the same and proposed to
unify them.

I would be fine with:

struct Context {
	RGB<double> gains;
	unsigned int temperatureK;
};

struct ActiveState {
	Context manual;
	Context automatic;

	bool autoEnabled;
};

struct FrameContext : public Context {
	bool autoEnabled;
};

struct AwbResult {
	RGB<double> gains;
	unsigned int temperatureK;
};
virtual awb::Result calculateAwb(const AwbStats &stats, unsigned int lux,
                                 std::array<double, 2> ranges) = 0;

I would be less fine with having a "result" type in ActiveState and
FrameContext.

> > + * \brief Awb gains and colour temperature
> >   *
> > - * This class holds the result of an auto white balance calculation.
> > + * \var Context::gains
> > + * \brief The white balance gains
> > + *
> > + * \var Context::temperatureK
> > + * \brief The colour temperature, in Kelvin
> >   */
> >
> >  /**
> > - * \var AwbResult::gains
> > - * \brief The calculated white balance gains
> > + * \struct ActiveState
> > + * \brief Active awb state shared across frames
> > + *
> > + * \var ActiveState::manual
> > + * \brief The most recent manually requested awb state
> > + *
> > + * \var ActiveState::automatic
> > + * \brief The most recent automatically calculated awb state
> > + *
> > + * \var ActiveState::autoEnabled
> > + * \brief True when automatic awb is selected
> >   */
> >
> >  /**
> > - * \var AwbResult::colourTemperature
> > - * \brief The calculated colour temperature in Kelvin
> > + * \struct FrameContext
> > + * \brief Per-frame awb state
> > + *
> > + * \var FrameContext::autoEnabled
> > + * \brief True when automatic awb is in use
> >   */
> >
> > +} /* namespace awb */
> > +
> >  /**
> >   * \class AwbStats
> > - * \brief An abstraction class wrapping hardware-specific AWB statistics
> > + * \brief An abstraction class wrapping hardware-specific awb statistics
> >   *
> > - * IPA modules using an AWB algorithm based on the AwbAlgorithm class need to
> > - * implement this class to give the algorithm access to the hardware-specific
> > - * statistics data.
> > + * IPA modules shall provide a derived implementation of this class to give the
> > + * awb algorithm access to the hardware-specific statistics data in a common
> > + * format.
> > + */
> > +
> > +/**
> > + * \fn AwbStats::AwbStats()
> > + * \brief Construct an empty AwbStat
> > + */
> > +
> > +/**
> > + * \brief Construct Awb statistics from RGB mean values
> > + * \param[in] rgbMeans The RGB mean values
> > + */
> > +AwbStats::AwbStats(const RGB<double> &rgbMeans)
> > +       : rgbMeans_(rgbMeans)
> > +{
> > +       rg_ = rgbMeans_.r() / rgbMeans_.g();
> > +       bg_ = rgbMeans_.b() / rgbMeans_.g();
> > +}
>
> I think this should stay in the platform specific implementation. I'll
> add more comments on the corresponding header.
>
> > +
> > +/**
> > + * AwbStat::~AwbStat
> > + * \brief Virtual class destructor
> >   */
> >
> >  /**
> > - * \fn AwbStats::computeColourError()
> >   * \brief Compute an error value for when the given gains would be applied
> >   * \param[in] gains The gains to apply
> >   *
> > @@ -57,87 +104,383 @@ namespace ipa {
> >   * applied. To keep the actual implementations computationally inexpensive,
> >   * the squared colour error shall be returned.
> >   *
> > - * If the AWB statistics provide multiple zones, the average of the individual
> > + * If the awb statistics provide multiple zones, the average of the individual
> >   * squared errors shall be returned. Averaging/normalizing is necessary so that
> >   * the numeric dimensions are the same on all hardware platforms.
> >   *
> >   * \return The computed error value
> >   */
> > +double AwbStats::computeColourError(const RGB<double> &gains) const
> > +{
> > +       /*
> > +        * Compute the sum of the squared colour error (non-greyness) as
> > +        * it appears in the log likelihood equation.
> > +        */
> > +       double deltaR = gains.r() * rg_ - 1.0;
> > +       double deltaB = gains.b() * bg_ - 1.0;
> > +       double delta2 = deltaR * deltaR + deltaB * deltaB;
> > +
> > +       return delta2;
> > +}
>
> IMHO: This also belongs into the platform specific implementation. Otherwise
> we will only have implementations that take one measurement into account
> only because the rkisp1 happened to provide only oine.
>

ok

I'll copy it into Mali and Simple though ?

> > +
> > +/**
> > + * \brief Retrieve if the awb statistics are valid
> > + *
> > + * If the colour mean values are too small, the AwbAlgorithm class doesn't have
> > + * enough information to meaningfully calculate white-balance gains. Freeze the
> > + * algorithm in that case.
> > + *
> > + * \return True if the awb statistics are valid, false otherwise
> > + */
> > +bool AwbStats::valid() const
> > +{
> > +       double minValue = minColourValue();
> > +
> > +       return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||
> > +              rgbMeans_.b() > minValue;
> > +}
> > +
> > +/**
> > + * \fn AwbStats::rgRatio()
> > + * \brief Retrieve the Red/Green ratio
> > + * \return The Red/Green ratio
> > + */
> >
> >  /**
> > - * \fn AwbStats::rgbMeans()
> > - * \brief Get RGB means of the statistics
> > + * \fn AwbStats::bgRatio()
> > + * \brief Retrieve the Blue/Green ratio
> > + * \return The Blue/Green ratio
> > + */
> > +
> > +/**
> > + * \brief Retrieve the RGB means values
> >   *
> >   * Fetch the RGB means from the statistics. The values of each channel are
> >   * dimensionless and only the ratios are used for further calculations. This is
> > - * used by the simple grey world model to calculate the gains to apply.
> > + * used by the AwbGrey to calculate the gains to apply.
>
> Coming back after writing the rest: I think we should change this to say
> it is RGB values in the [0..1] range. The hardware specific part will
> know how to scale that properly. And then we can get rid of xxRatio()
> and there is no need for the minColourValue() anymore.
>

I might have missed how you would like to get rid of them ?

> >   *
> >   * \return The RGB means
> >   */
> > +RGB<double> AwbStats::rgbMeans() const
> > +{
> > +       return rgbMeans_;
> > +}
> >
> >  /**
> > - * \class AwbAlgorithm
> > - * \brief A base class for auto white balance algorithms
> > + * \fn AwbStats::minColourValue
> > + * \brief Retrieve the threshold below which a colour information is not valid
> > + *
> > + * Mean colour values as reported by the ISP through the white balance
> > + * statistics are considered valid for colour gain calculation purposes when
> > + * they are above a certain value. The awb algorithm needs to know what is the
> > + * minimum value below which it should ignore the colour mean values.
> > + *
> > + * As different ISP platforms report gains in different formats, the threshold
> > + * is then platform specific, and all IPA implementations that use the
> > + * AwbAlgorithm class should provide that information by implementing this
> > + * function.
> > + *
> > + * The reported minimum value applies to all three colour channels.
> >   *
> > - * This class is a base class for auto white balance algorithms. It defines an
> > - * interface for the algorithms to implement, and is used by the IPAs to
> > - * interact with the concrete implementation.
> > + * This function is used by AwbStats::valid().
> > + *
> > + * \return The minimum valid colour value
> >   */
> >
> >  /**
> > - * \fn AwbAlgorithm::init()
> > - * \brief Initialize the algorithm with the given tuning data
> > - * \param[in] tuningData The tuning data to use for the algorithm
> > + * \var AwbStats::rgbMeans_
> > + * \brief Mean values of colour channels
> > + */
> > +
> > +/**
> > + * \var AwbStats::rg_
> > + * \brief Red/Green ratio
> > + */
> > +
> > +/**
> > + * \var AwbStats::bg_
> > + * \brief Blue/Green ratio
> > + */
> > +
> > +/**
> > + * \class AwbImplementation
> > + * \brief Pure virtual base class for awb algorithms implementations
> >   *
> > - * \return 0 on success, a negative error code otherwise
> > + * The AwbImplementation class defines the interface for the awb algorithm
> > + * implementations.
> > + *
> > + * It is currently implemented by the AwbGrey and AwbBayes classes.
> > + *
> > + * The interface defines an init() function to initialize the algorithm with the
> > + * content of the tuning file and two functions to compute colour gains
> > + * according to the algorithm operating mode (auto or manual) in use.
> > + *
> > + * The calculateAwb() function calculates colour gains given a set of statistics
> > + * provided by the IPA module. It is used when the algorithm operates in auto
> > + * mode and gains are dynamically computed given a new set of statistics from
> > + * the awb engine.
> > + *
> > + * The gainsFromColourTemperature() function instead interpolates a gain curve
> > + * if specified in the tuning file with the supplied colour temperature. This
> > + * function is used in manual mode where it is expected the application to
> > + * supply a colour temperature and the algorithm to adjust the white balance
> > + * gains to it.
> > + */
> > +
> > +/**
> > + * \fn AwbImplementation::~AwbImplementation
> > + * \brief Virtual class destructor
> > + */
> > +
> > +/**
> > + * \fn AwbImplementation::init()
> > + * \param[in] tuningData
> > + * \brief Initialize the algorithm by parsing \a tuningData
> >   */
> >
> >  /**
> > - * \fn AwbAlgorithm::calculateAwb()
> > - * \brief Calculate AWB data from the given statistics
> > - * \param[in] stats The statistics to use for the calculation
> > - * \param[in] lux The lux value of the scene
> > + * \fn AwbImplementation::calculateAwb()
> > + * \param[in] stats The awb statistics
> > + * \param[in] lux The estimated lux level
> > + * \param[in] ranges The colour temperature search limits (AwbBayes only)
> >   *
> > - * Calculate an AwbResult object from the given statistics and lux value. A \a
> > - * lux value of 0 means it is unknown or invalid and the algorithm shall ignore
> > - * it.
> > + * Calculate a new set of colour gains and a colour temperature given a new
> > + * set of statistics \a stats, an estimated luminance \a lux and a range of
> > + * colour temperature to limit the search (for AwbBayes only).
>
> I would drop the reference to AwbBayes
>

ok

> >   *
> > - * \return The AWB result
> > + * \return An awb::Context computed using the new statistics
> > + */
> > +/**
> > + * \fn AwbImplementation::gainsFromColourTemperature()
> > + * \param[in] temperatureK Colour temperature in Kelvin
> > + *
> > + * Calculate a new set of colour gains by interpolating a gain curve (if
> > + * provided in the tuning file) with a new colour temperature \a temperatureK.
>
> This (interpolating a curve) is one way to do it. But it is an
> implementation detail of the specific algo. So I think it doesn't belong
> to the base class documentation or be marked as example.

I can try to move this to the documentation of AwbBayes and AwbGrey ?

I'll also change the above description of gainsFromColourTemperature()
in the class documentation

>
> > + *
> > + * \return The RGB gain values adjusted to \a temperatureK or std::nullopt if
> > + * no gain curve is specified in the tuning data
>
> Hm, for a base class, this feels a bit too specific. Assume a neural
> network based awb. That would potentially not require the curve.

I'll drop "if no gain curve is specified in the tuning data"

>
> >   */
> >
> >  /**
> > - * \fn AwbAlgorithm::gainsFromColourTemperature()
> > - * \brief Compute white balance gains from a colour temperature
> > - * \param[in] colourTemperature The colour temperature in Kelvin
> > + * \class AwbAlgorithmBase
> > + * \brief Base class for AwbAlgorithm for non-templated functions implementation
> >   *
> > - * Compute the white balance gains from a \a colourTemperature. This function
> > - * does not take any statistics into account. It is used to compute the colour
> > - * gains when the user manually specifies a colour temperature.
>
> This information got lost, but is IMHO important for the implementer to
> understand why he needs to implement this.

I'll re-use it to replace/adjust the function description in the class
documentation

>
> > + * Base class for AwbAlgorithm where non-templated functions are implemented.
> > + * IPA implementations shall use AwbAlgorithm and not this class.
> > + */
> > +
> > +/**
> > + * \brief Initialize the algorithm with the given tuning data
> > + * \param[in] tuningData The tuning data to use for the algorithm
> > + *
> > + * Parse \a tuningData to initialize the awb algorithm and register controls.
> > + * IPA modules are expected to call this function as part of their
> > + * implementation of Algorithm::init().
> >   *
> > - * \return The colour gains or std::nullopt if the conversion is not possible
> > + * \return 0 on success, a negative error code otherwise
> > + */
> > +int AwbAlgorithmBase::init(const ValueNode &tuningData)
> > +{
> > +       bayes_ = false;
> > +
> > +       if (!tuningData.contains("algorithm"))
> > +               LOG(Awb, Info) << "No Awb algorithm specified, using grey world";
> > +
> > +       auto mode = tuningData["algorithm"].get<std::string>("grey");
> > +       if (mode == "grey") {
> > +               impl_ = std::make_unique<AwbGrey>();
> > +       } else if (mode == "bayes") {
> > +               impl_ = std::make_unique<AwbBayes>();
> > +               bayes_ = true;
> > +       } else {
> > +               LOG(Awb, Error) << "Unknown Awb algorithm: " << mode;
> > +               return -EINVAL;
> > +       }
> > +
> > +       LOG(Awb, Debug) << "Using Awb algorithm: " << mode;
> > +
> > +       int ret = impl_->init(tuningData);
> > +       if (ret)
> > +               return ret;
> > +
> > +       controls_[&controls::ColourTemperature] =
> > +               ControlInfo(kMinColourTemperature, kMaxColourTemperature,
> > +                           kDefaultColourTemperature);
> > +       controls_[&controls::AwbEnable] = ControlInfo(false, true);
> > +
> > +       return parseModeConfigs(tuningData, controls::AwbAuto);
> > +}
> > +
> > +/**
> > + * \brief Configure the awb algorithm
> > + * \param[in] state The awb active state
> > + * \return 0 if successful, an error code otherwise
> >   */
> > +int AwbAlgorithmBase::configure(awb::ActiveState &state)
> > +{
> > +       state.manual.gains = RGB<double>{ 1.0 };
> > +       auto gains = impl_->gainsFromColourTemperature(kDefaultColourTemperature);
> > +       if (gains)
> > +               state.automatic.gains = *gains;
> > +       else
> > +               state.automatic.gains = RGB<double>{ 1.0 };
> > +
> > +       state.autoEnabled = true;
> > +       state.manual.temperatureK = kDefaultColourTemperature;
> > +       state.automatic.temperatureK = kDefaultColourTemperature;
> > +
> > +       return 0;
> > +}
> >
> >  /**
> > - * \fn AwbAlgorithm::controls()
> > - * \brief Get the controls info map for this algorithm
> > + * \brief Queue a Request to the awb algorithm
> > + * \param[in] state The awb active state
> > + * \param[in] frame The frame number
> > + * \param[in] frameContext The awb frame context
> > + * \param[in] controls The list of controls part of the Request
> > + *
> > + * Queue a new Request to the awb algorithm and modify its behaviour according
> > + * to the provided controls.
> >   *
> > - * \return The controls info map
> > + * The currently handled controls are:
> > + * - controls::AwbEnable
> > + * - controls::AwbMode
> > + * - controls::ColourGains
> > + * - controls::ColourTemperature
> >   */
> > +void AwbAlgorithmBase::queueRequest(awb::ActiveState &state,
> > +                                   [[maybe_unused]] const uint32_t frame,
> > +                                   awb::FrameContext &frameContext,
> > +                                   const ControlList &controls)
> > +{
> > +       const auto &awbEnable = controls.get(controls::AwbEnable);
> > +       if (awbEnable && *awbEnable != state.autoEnabled) {
> > +               state.autoEnabled = *awbEnable;
> > +
> > +               LOG(Awb, Debug)
> > +                       << (*awbEnable ? "Enabling" : "Disabling") << " Awb";
> > +       }
> > +
> > +       auto mode = controls.get(controls::AwbMode);
> > +       if (mode) {
> > +               auto it = modes_.find(static_cast<controls::AwbModeEnum>(*mode));
> > +               if (it == modes_.end()) {
> > +                       LOG(Awb, Error) << "Unsupported Awb mode " << *mode;
> > +                       return;
> > +               }
> > +
> > +               currentMode_ = &it->second;
> > +       }
> > +
> > +       frameContext.autoEnabled = state.autoEnabled;
> > +
> > +       if (frameContext.autoEnabled)
> > +               return;
> > +
> > +       const auto &colourGains = controls.get(controls::ColourGains);
> > +       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > +       bool update = false;
> > +       if (colourGains) {
> > +               state.manual.gains.r() = (*colourGains)[0];
> > +               state.manual.gains.b() = (*colourGains)[1];
> > +               /*
> > +                * \todo Colour temperature reported in metadata is now
> > +                * incorrect, as we can't deduce the temperature from the gains.
> > +                * This will be fixed with the bayes AWB algorithm.
> > +                */
> > +               update = true;
> > +       } else if (colourTemperature) {
> > +               state.manual.temperatureK = *colourTemperature;
> > +               const auto &gains = impl_->gainsFromColourTemperature(*colourTemperature);
> > +               if (gains) {
> > +                       state.manual.gains.r() = gains->r();
> > +                       state.manual.gains.b() = gains->b();
> > +                       update = true;
> > +               }
> > +       }
> > +
> > +       if (update)
> > +               LOG(Awb, Debug)
> > +                       << "Set colour gains to " << state.manual.gains;
> > +
> > +       frameContext.gains = state.manual.gains;
> > +       frameContext.temperatureK = state.manual.temperatureK;
> > +}
> >
> >  /**
> > - * \fn AwbAlgorithm::handleControls()
> > - * \param[in] controls The controls to handle
> > - * \brief Handle the controls supplied in a request
> > + * \brief Set the gains and colour temperature values in \a frameContext
> > + * \param[in] state The awb active state
> > + * \param[in] frameContext The awb frame context
> > + *
> > + * If auto mode is enabled, take the most recently computed gains and use them
> > + * for the current frame. Otherwise, if in manual mode, gains and colour
> > + * temperature for a frame are set at queueRequest() time.
> >   */
> > +void AwbAlgorithmBase::prepare(awb::ActiveState &state,
> > +                              awb::FrameContext &frameContext)
> > +{
> > +       if (frameContext.autoEnabled) {
> > +               frameContext.gains = state.automatic.gains;
> > +               frameContext.temperatureK = state.automatic.temperatureK;
> > +       }
> > +}
> >
> >  /**
> > + * \brief Process awb statistics to calculate gains and populate metadata
> > + * \param[in] state The awb active state
> > + * \param[in] frameContext The awb frame context
> > + * \param[in] stats The awb statistics
> > + * \param[in] lux The lux value as estimated by the IPA module
> > + * \param[out] metadata The metadata list
> > + *
> > + * Process \a stats to calculate new gains and colour temperature and populate
> > + * \a metadata with the results.
> > + */
> > +void AwbAlgorithmBase::process(awb::ActiveState &state,
> > +                              awb::FrameContext &frameContext,
> > +                              const AwbStats &stats, unsigned int lux,
> > +                              ControlList &metadata)
> > +{
> > +       if (!stats.valid())
> > +               return;
> > +
> > +       auto awbResult = impl_->calculateAwb(stats, lux, { currentMode_->ctLo,
> > +                                                          currentMode_->ctHi });
> > +
> > +       /*
> > +        * Clamp the gain values to the hardware, according to the gainMin_
> > +        * and gainMax_ values.
> > +        */
> > +       awbResult.gains = awbResult.gains.clamp(gainMin_, gainMax_);
> > +
> > +       /* Smooth color gains adjustments. */
> > +       double speed = 0.2;
> > +       double ct = awbResult.temperatureK;
> > +       ct = ct * speed + state.automatic.temperatureK * (1 - speed);
> > +
> > +       state.automatic.temperatureK = awbResult.temperatureK;
> > +       state.automatic.gains = awbResult.gains * speed +
> > +                               state.automatic.gains * (1 - speed);
> > +
> > +       /* Populate metadata. */
> > +       metadata.set(controls::AwbEnable, frameContext.autoEnabled);
> > +       metadata.set(controls::ColourGains, { static_cast<float>(frameContext.gains.r()),
> > +                                             static_cast<float>(frameContext.gains.b()) });
> > +       metadata.set(controls::ColourTemperature, frameContext.temperatureK);
> > +
> > +       LOG(Awb, Debug) << std::showpoint << "Means " << stats.rgbMeans()
> > +                       << ", gains " << state.automatic.gains
> > +                       << ", temp " << state.automatic.temperatureK << "K";
> > +}
> > +
> > +/*
> >   * \brief Parse the mode configurations from the tuning data
> >   * \param[in] tuningData the ValueNode representing the tuning data
> >   * \param[in] def The default value for the AwbMode control
> >   *
> >   * Utility function to parse the tuning data for an AwbMode entry and read all
> > - * provided modes. It adds controls::AwbMode to AwbAlgorithm::controls_ and
> > - * populates AwbAlgorithm::modes_. For a list of possible modes see \ref
> > + * provided modes. It adds controls::AwbMode to AwbAlgorithmBase::controls_ and
> > + * populates AwbAlgorithmBase::modes_. For a list of possible modes see \ref
> >   * controls::AwbModeEnum.
> >   *
> >   * Each mode entry must contain a "lo" and "hi" key to specify the lower and
> > @@ -156,15 +499,23 @@ namespace ipa {
> >   *       ...
> >   * \endcode
> >   *
> > - * If \a def is supplied but not contained in the the \a tuningData, -EINVAL is
> > + * If \a def is supplied but not contained in the \a tuningData, -EINVAL is
> >   * returned.
> >   *
> > + * AwbModes are only used by the AwbBayes implementation.
> > + *
> >   * \sa controls::AwbModeEnum
> >   * \return Zero on success, negative error code otherwise
> >   */
> > -int AwbAlgorithm::parseModeConfigs(const ValueNode &tuningData,
> > -                                  const ControlValue &def)
> > +int AwbAlgorithmBase::parseModeConfigs(const ValueNode &tuningData,
> > +                                      const ControlValue &def)
> >  {
> > +       if (!bayes_) {
> > +               /* AwbGrey does not support and does not use modes. */
> > +               currentMode_ = &AwbGreyMode;
> > +               return 0;
> > +       }
> > +
> >         std::vector<ControlValue> availableModes;
> >
> >         const ValueNode &modes = tuningData[controls::AwbMode.name()];
> > @@ -227,37 +578,83 @@ int AwbAlgorithm::parseModeConfigs(const ValueNode &tuningData,
> >         }
> >
> >         controls_[&controls::AwbMode] = ControlInfo(availableModes, def);
> > +       currentMode_ = &modes_[controls::AwbAuto];
> >
> >         return 0;
> >  }
> >
> >  /**
> > - * \class AwbAlgorithm::ModeConfig
> > - * \brief Holds the configuration of a single AWB mode
> > + * \var AwbAlgorithmBase::controls_
> > + * \brief Controls info map for the controls registered by the awb algorithm
> > + */
> > +
> > +/**
> > + * \var AwbAlgorithmBase::gainMin_
> > + * \brief The minimum supported gain value
> >   *
> > - * AWB modes limit the regulation of the AWB algorithm to a specific range of
> > - * colour temperatures.
> > + * Minimum gain value used to clamp the awb algorithm calculation results in the
> > + * range supported by the platform awb engine.
> > + *
> > + * The min and max gain values are initialized by AwbAlgorithm::init().
> >   */
> >
> >  /**
> > - * \var AwbAlgorithm::ModeConfig::ctLo
> > + * \var AwbAlgorithmBase::gainMax_
> > + * \brief The maximum supported gain value
> > + *
> > + * Maximum gain value used to clamp the awb algorithm calculation results in the
> > + * range supported by the platform awb engine.
> > + *
> > + * The min and max gain values are initialized by AwbAlgorithm::init().
> > + */
> > +
> > +/*
> > + * \class AwbAlgorithmBase::ModeConfig
> > + * \brief Holds the configuration of a single awb mode
> > + *
> > + * AWB modes limit the regulation of the awb algorithm to a specific range of
> > + * colour temperatures. Use by AwbBayes only.
> > + */
> > +
> > +/*
> > + * \var AwbAlgorithmBase::ModeConfig::ctLo
> >   * \brief The lowest valid colour temperature of that mode
> >   */
> >
> > -/**
> > - * \var AwbAlgorithm::ModeConfig::ctHi
> > +/*
> > + * \var AwbAlgorithmBase::ModeConfig::ctHi
> >   * \brief The highest valid colour temperature of that mode
> >   */
> >
> > +/*
> > + * \var AwbAlgorithmBase::modes_
> > + * \brief Map of all configured modes
> > + * \sa AwbAlgorithmBase::parseModeConfigs
> > + */
> > +
> >  /**
> > - * \var AwbAlgorithm::controls_
> > - * \brief Controls info map for the controls provided by the algorithm
> > + * \class AwbAlgorithm
> > + * \brief The libipa awb algorithm
> > + * \tparam Q The fixedpoint register representation of gain values
> > + *
> > + * Implement the awb algorithm for libipa.
> > + *
> > + * The AwbAlgorithm class implements an interface similar in spirit to the one
> > + * of the Algorithm class. IPA modules are expected to store an instance of
> > + * AwbAlgorithm as class member, template it with the awb engine gain register
> > + * representation and call its function in their implementations of the
> > + * Algorithm interface.
> > + *
> > + * The AwbAlgorithm instantiates an AwbImplementation implementation (AwbGrey or
> > + * AwbBayes) at init() time by parsing the tuning data and uses it to compute
> > + * the RGB gains and estimate a colour temperature given a set of statistics
> > + * in the form of a AwbStats derived implementation.
> >   */
> >
> >  /**
> > - * \var AwbAlgorithm::modes_
> > - * \brief Map of all configured modes
> > - * \sa AwbAlgorithm::parseModeConfigs
> > + * \fn AwbAlgorithm::init()
> > + * \param[in] controls The info map of the IPA controls
> > + * \copydoc AwbAlgorithmBase::init()
> >   */
> >
> >  } /* namespace ipa */
> > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> > index 09c00e47d604..f453e1f9fd3d 100644
> > --- a/src/ipa/libipa/awb.h
> > +++ b/src/ipa/libipa/awb.h
> > @@ -2,11 +2,12 @@
> >  /*
> >   * Copyright (C) 2024 Ideas on Board Oy
> >   *
> > - * Generic AWB algorithms
> > + * libIPA AWB algorithms
> >   */
> >
> >  #pragma once
> >
> > +#include <array>
> >  #include <map>
> >  #include <optional>
> >
> > @@ -20,46 +21,119 @@ namespace libcamera {
> >
> >  namespace ipa {
> >
> > -struct AwbResult {
> > +namespace awb {
> > +
> > +struct Context {
>
> As written above, I don't particularly like that new name.
>

Even if I split Context and Result ?
I'll happily accept proposals.

> >         RGB<double> gains;
> > -       double colourTemperature;
> > +       unsigned int temperatureK;
> > +};
> > +
> > +struct ActiveState {
> > +       Context manual;
> > +       Context automatic;
> > +
> > +       bool autoEnabled;
> > +};
> > +
> > +struct FrameContext : public Context {
> > +       bool autoEnabled;
> >  };
> >
> > +} /* namespace awb */
> > +
> >  struct AwbStats {
> > -       virtual double computeColourError(const RGB<double> &gains) const = 0;
> > -       virtual RGB<double> rgbMeans() const = 0;
>
> This was designed as the interface that the platform specific
> implementation has to provide. Now it is interface and partial
> implementation at the same time. Can we keep the interface separate and
> - if we really want to have this implementation at a central place -
> maybe call it "AwbStatsMinimal" or something?
>

Are you suggesting a pure virtual base class, derived by a partial
implementation which implementations have to derive and complete ?

What would the gain be in splitting the base class in 2 ?

> > +       AwbStats() = default;
> > +       AwbStats(const RGB<double> &means);
> > +       virtual ~AwbStats() = default;
> > +
> > +       bool valid() const;
> > +
> > +       virtual double rgRatio() const { return rg_; }
> > +       virtual double bgRatio() const { return bg_; }
> > +       virtual double computeColourError(const RGB<double> &gains) const;
> > +       virtual RGB<double> rgbMeans() const;
>
> rgbMeans and xxRatio seem to server a very similar purpose now. I need
> to look further.
>
> ...
>
> Coming back. I don't understand why we can't go for rgbMeans() only. I
> think the old interface should be sufficient. For cases where the
> hardware only provides a ratio, we could IMHO calculate a means value
> based on that.
>

The existing implementation of RkISP1AwbStats already calculates rg_
and bg_ as

        {
               rg_ = rgbMeans_.r() / rgbMeans_.g();
               bg_ = rgbMeans_.b() / rgbMeans_.g();
        }

Mali provides stats already in the r/g and b/g format

	MaliC55AwbStats(const RGB<double> &rgbMeans)
	{
		/* The Mali-C55 ISP already provides stats as R/G and B/G ratios. */
		rg_ = rgbMeans.r();
		bg_ = rgbMeans.b();
        }

With rgbMeans.g() fixed to 1.

So I can reconstruct r and b by inverting them in the MaliC55AwbStats
constructor and revert the diff in AwbGrey::calculateAwb()

        /*
-        * Estimate the red and blue gains to apply in a grey world. The green
-        * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the
-        * divisor to a minimum value of 1.0.
+        * Calculate the red and blue gains to apply in a grey world by simply
+        * inverting the red/green and blue/green ratios as reported in
+        * statistics. The green gain is hardcoded to 1.0.
         */
-       result.gains.r() = means.g() / std::max(means.r(), 1.0);
+       result.gains.r() = 1.0 / stats.rgRatio();
        result.gains.g() = 1.0;
-       result.gains.b() = means.g() / std::max(means.b(), 1.0);
+       result.gains.b() = 1.0 / stats.bgRatio();

To be honest I like

+       result.gains.r() = 1.0 / stats.rgRatio();

better than

-       result.gains.r() = means.g() / std::max(means.r(), 1.0);

but I won't bother.


> I know we talked about this in Nice, but now reading it, I can't see the
> reason anymore.
>
> >
> >  protected:
> > -       ~AwbStats() = default;
> > +       virtual double minColourValue() const = 0;
> > +
> > +       RGB<double> rgbMeans_;
> > +       double rg_;
> > +       double bg_;
> >  };
> >
> > -class AwbAlgorithm
> > +class AwbImplementation
> >  {
> >  public:
> > -       virtual ~AwbAlgorithm() = default;
> > -
> > +       virtual ~AwbImplementation() = default;
> >         virtual int init(const ValueNode &tuningData) = 0;
> > -       virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
> > -       virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
> > +       virtual awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> > +                                         std::array<double, 2> ranges) = 0;
> > +       virtual std::optional<RGB<double>>
> > +       gainsFromColourTemperature(double temperatureK) = 0;
> > +};
> >
> > -       const ControlInfoMap::Map &controls() const
> > -       {
> > -               return controls_;
> > -       }
> > +class AwbAlgorithmBase
> > +{
> > +public:
> > +       int init(const ValueNode &tuningData);
> > +
> > +       int configure(awb::ActiveState &state);
> > +
> > +       void queueRequest(awb::ActiveState &state,
> > +                         const uint32_t frame,
> > +                         awb::FrameContext &frameContext,
> > +                         const ControlList &controls);
> > +
> > +       void prepare(awb::ActiveState &state, awb::FrameContext &frameContext);
> >
> > -       virtual void handleControls([[maybe_unused]] const ControlList &controls) {}
> > +       void process(awb::ActiveState &state, awb::FrameContext &frameContext,
> > +                    const AwbStats &stats, unsigned int lux,
> > +                    ControlList &metadata);
> >
> >  protected:
> > -       int parseModeConfigs(const ValueNode &tuningData,
> > -                            const ControlValue &def = {});
> > +       AwbAlgorithmBase() = default;
> >
> > +       ControlInfoMap::Map controls_;
> > +       float gainMin_;
> > +       float gainMax_;
> > +
> > +private:
> >         struct ModeConfig {
> >                 double ctHi;
> >                 double ctLo;
> >         };
> >
> > -       ControlInfoMap::Map controls_;
> > -       std::map<controls::AwbModeEnum, AwbAlgorithm::ModeConfig> modes_;
> > +       /* AwbGrey does not support modes; */
> > +       static constexpr ModeConfig AwbGreyMode = { 0.0, 0.0 };
> > +
> > +       int parseModeConfigs(const ValueNode &tuningData,
> > +                            const ControlValue &def = {});
> > +
> > +       std::map<controls::AwbModeEnum, AwbAlgorithmBase::ModeConfig> modes_;
> > +       const ModeConfig *currentMode_ = nullptr;
> > +       std::unique_ptr<AwbImplementation> impl_;
> > +       bool bayes_ = false;
> > +};
> > +
> > +template<typename Q>
> > +class AwbAlgorithm : public AwbAlgorithmBase
> > +{
> > +public:
> > +       int init(const ValueNode &tuningData, ControlInfoMap::Map &controls)
> > +       {
> > +               AwbAlgorithmBase::init(tuningData);
> > +
> > +               gainMin_ = std::max(Q::TraitsType::min, 1.0f);
> > +               gainMax_ = Q::TraitsType::max;
> > +
> > +               controls_[&controls::ColourGains] =
> > +                       ControlInfo(gainMin_, gainMax_,
> > +                                   Span<const float, 2>{ { 1.0f, 1.0f } });
> > +
> > +               controls.insert(controls_.begin(), controls_.end());
> > +
> > +               return 0;
> > +       }
> >  };
>
> This feels like an overkill for a template. The only reason is to deduce
> the min max values for the gainMin and gainMax values. Why not adding a
> gainRange param to init()?
>

I think initializing the algorithm with the register format
representation is elegant and all algorithms in libipa follow the same
design pattern.

Given the register format is fixed, I don't see any advantage in
passing parameters to init()

> >
> >  } /* namespace ipa */
> > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp
> > index 9fd85e5a4505..34d86ce6938b 100644
> > --- a/src/ipa/libipa/awb_bayes.cpp
> > +++ b/src/ipa/libipa/awb_bayes.cpp
> > @@ -140,11 +140,6 @@ void Interpolator<Pwl>::interpolate(const Pwl &a, const Pwl &b, Pwl &dest, doubl
> >   * \brief How far to wander off CT curve towards "more green"
> >   */
> >
> > -/**
> > - * \var AwbBayes::currentMode_
> > - * \brief The currently selected mode
> > - */
> > -
> >  int AwbBayes::init(const ValueNode &tuningData)
> >  {
> >         int ret = colourGainCurve_.readYaml(tuningData["colourGains"], "ct", "gains");
> > @@ -172,14 +167,6 @@ int AwbBayes::init(const ValueNode &tuningData)
> >                 return ret;
> >         }
> >
> > -       ret = parseModeConfigs(tuningData, controls::AwbAuto);
> > -       if (ret) {
> > -               LOG(Awb, Error)
> > -                       << "Failed to parse mode parameter from tuning file";
> > -               return ret;
> > -       }
> > -       currentMode_ = &modes_[controls::AwbAuto];
> > -
> >         transversePos_ = tuningData["transversePos"].get<double>(0.01);
> >         transverseNeg_ = tuningData["transverseNeg"].get<double>(0.01);
> >         if (transversePos_ <= 0 || transverseNeg_ <= 0) {
> > @@ -260,18 +247,6 @@ int AwbBayes::readPriors(const ValueNode &tuningData)
> >         return 0;
> >  }
> >
> > -void AwbBayes::handleControls(const ControlList &controls)
> > -{
> > -       auto mode = controls.get(controls::AwbMode);
> > -       if (mode) {
> > -               auto it = modes_.find(static_cast<controls::AwbModeEnum>(*mode));
> > -               if (it != modes_.end())
> > -                       currentMode_ = &it->second;
> > -               else
> > -                       LOG(Awb, Error) << "Unsupported AWB mode " << *mode;
> > -       }
> > -}
> > -
> >  std::optional<RGB<double>> AwbBayes::gainsFromColourTemperature(double colourTemperature)
> >  {
> >         /*
> > @@ -283,7 +258,9 @@ std::optional<RGB<double>> AwbBayes::gainsFromColourTemperature(double colourTem
> >         return RGB<double>{ { gains[0], 1.0, gains[1] } };
> >  }
> >
> > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
> > +awb::Context
> > +AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux,
> > +                      std::array<double, 2> ranges)
>
> Nit: ranges is confusion as it is a single range. So I'd rename it to
> range or even ctRange to make it easier to understand everywhere.
>
> And I guess it could be Span.
>

I'll try a Span

> >  {
> >         ipa::Pwl prior;
> >         if (lux > 0) {
> > @@ -295,7 +272,7 @@ AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
> >                 prior.append(0, 1.0);
> >         }
> >
> > -       double t = coarseSearch(prior, stats);
> > +       double t = coarseSearch(prior, stats, ranges);
> >         double r = ctR_.eval(t);
> >         double b = ctB_.eval(t);
> >         LOG(Awb, Debug)
> > @@ -316,14 +293,15 @@ AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
> >                 << "After fine search: r " << r << " b " << b << " (gains r "
> >                 << 1 / r << " b " << 1 / b << ")";
> >
> > -       return { { { 1.0 / r, 1.0, 1.0 / b } }, t };
> > +       return { { { 1.0 / r, 1.0, 1.0 / b } }, static_cast<unsigned int>(t) };
> >  }
> >
> > -double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) const
> > +double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats,
> > +                             std::array<double, 2> ranges) const
> >  {
> >         std::vector<Pwl::Point> points;
> >         size_t bestPoint = 0;
> > -       double t = currentMode_->ctLo;
> > +       double t = ranges[0];
> >         int spanR = -1;
> >         int spanB = -1;
> >         LimitsRecorder<double> errorLimits;
> > @@ -345,14 +323,14 @@ double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) cons
> >                 if (points.back().y() < points[bestPoint].y())
> >                         bestPoint = points.size() - 1;
> >
> > -               if (t == currentMode_->ctHi)
> > +               if (t == ranges[1])
> >                         break;
> >
> >                 /*
> >                  * Ensure even steps along the r/b curve by scaling them by the
> >                  * current t.
> >                  */
> > -               t = std::min(t + t / 10 * kSearchStep, currentMode_->ctHi);
> > +               t = std::min(t + t / 10 * kSearchStep, ranges[1]);
> >         }
> >
> >         t = points[bestPoint].x();
> > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h
> > index 1e3373676bc0..8264bcb09f38 100644
> > --- a/src/ipa/libipa/awb_bayes.h
> > +++ b/src/ipa/libipa/awb_bayes.h
> > @@ -20,22 +20,23 @@ namespace libcamera {
> >
> >  namespace ipa {
> >
> > -class AwbBayes : public AwbAlgorithm
> > +class AwbBayes : public AwbImplementation
> >  {
> >  public:
> >         AwbBayes() = default;
> >
> >         int init(const ValueNode &tuningData) override;
> > -       AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;
> > +       awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> > +                                 std::array<double, 2> ranges) override;
> >         std::optional<RGB<double>> gainsFromColourTemperature(double temperatureK) override;
> > -       void handleControls(const ControlList &controls) override;
> >
> >  private:
> >         int readPriors(const ValueNode &tuningData);
> >
> >         void fineSearch(double &t, double &r, double &b, ipa::Pwl const &prior,
> >                         const AwbStats &stats) const;
> > -       double coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) const;
> > +       double coarseSearch(const ipa::Pwl &prior, const AwbStats &stats,
> > +                           std::array<double, 2> ranges) const;
> >         double interpolateQuadratic(ipa::Pwl::Point const &a,
> >                                     ipa::Pwl::Point const &b,
> >                                     ipa::Pwl::Point const &c) const;
> > @@ -50,8 +51,6 @@ private:
> >
> >         double transversePos_;
> >         double transverseNeg_;
> > -
> > -       ModeConfig *currentMode_ = nullptr;
> >  };
> >
> >  } /* namespace ipa */
> > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> > index b14b096810ae..2e31492bda3c 100644
> > --- a/src/ipa/libipa/awb_grey.cpp
> > +++ b/src/ipa/libipa/awb_grey.cpp
> > @@ -60,6 +60,7 @@ int AwbGrey::init(const ValueNode &tuningData)
> >   * \brief Calculate AWB data from the given statistics
> >   * \param[in] stats The statistics to use for the calculation
> >   * \param[in] lux The lux value of the scene
> > + * \param[in] ranges The colour temperature search limits (AwbBayes only)
>
> Maybe invert this, to say "range support is not implemented in this
> algorithm"? Then we could drop the reference to AwbBayes.

Ok

>
> >   *
> >   * The colour temperature is estimated based on the colours::estimateCCT()
> >   * function. The gains are calculated purely based on the RGB means provided by
> > @@ -70,20 +71,23 @@ int AwbGrey::init(const ValueNode &tuningData)
> >   *
> >   * \return The AWB result
> >   */
> > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux)
> > +awb::Context
> > +AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux,
> > +                     [[maybe_unused]] std::array<double, 2> ranges)
> >  {
> > -       AwbResult result;
> > +       awb::Context result;
> >         auto means = stats.rgbMeans();
> > -       result.colourTemperature = estimateCCT(means);
> > +       result.temperatureK = estimateCCT(means);
> >
> >         /*
> > -        * Estimate the red and blue gains to apply in a grey world. The green
> > -        * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the
> > -        * divisor to a minimum value of 1.0.
> > +        * Calculate the red and blue gains to apply in a grey world by simply
> > +        * inverting the red/green and blue/green ratios as reported in
> > +        * statistics. The green gain is hardcoded to 1.0.
> >          */
> > -       result.gains.r() = means.g() / std::max(means.r(), 1.0);
> > +       result.gains.r() = 1.0 / stats.rgRatio();
> >         result.gains.g() = 1.0;
> > -       result.gains.b() = means.g() / std::max(means.b(), 1.0);
> > +       result.gains.b() = 1.0 / stats.bgRatio();
> > +
> >         return result;
> >  }
> >
> > @@ -96,12 +100,13 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned
> >   * gains configured in the colour temperature curve.
> >   *
> >   * \return The colour gains if a colour temperature curve is available,
> > - * [1, 1, 1] otherwise.
> > + * std::nullopt otherwise
> >   */
> >  std::optional<RGB<double>> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> >  {
> >         if (!colourGainCurve_) {
> > -               LOG(Awb, Error) << "No gains defined";
> > +               LOG(Awb, Info) << "No gains curve defined, "
> > +                              << "unable to interpolate gains to colour temperature";
> >                 return std::nullopt;
> >         }
> >
> > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> > index 154a2af97f15..438fb850c223 100644
> > --- a/src/ipa/libipa/awb_grey.h
> > +++ b/src/ipa/libipa/awb_grey.h
> > @@ -18,13 +18,14 @@ namespace libcamera {
> >
> >  namespace ipa {
> >
> > -class AwbGrey : public AwbAlgorithm
> > +class AwbGrey : public AwbImplementation
> >  {
> >  public:
> >         AwbGrey() = default;
> >
> >         int init(const ValueNode &tuningData) override;
> > -       AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;
> > +       awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> > +                                 std::array<double, 2> ranges) override;
> >         std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) override;
> >
> >  private:
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 0eb05c2b6e1a..0ad0794b4a0f 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -8,17 +8,12 @@
> >  #include "awb.h"
> >
> >  #include <algorithm>
> > -#include <ios>
> >
> >  #include <libcamera/base/log.h>
> >
> > -#include <libcamera/control_ids.h>
> > -
> >  #include <libcamera/ipa/core_ipa_interface.h>
> >
> > -#include "libipa/awb_bayes.h"
> > -#include "libipa/awb_grey.h"
> > -#include "libipa/colours.h"
> > +#include "libcamera/internal/vector.h"
> >
> >  /**
> >   * \file awb.h
> > @@ -28,54 +23,29 @@ namespace libcamera {
> >
> >  namespace ipa::rkisp1::algorithms {
> >
> > -/**
> > - * \class Awb
> > - * \brief Manage the white balance with automatic and manual controls
> > - */
> > -
> >  LOG_DEFINE_CATEGORY(RkISP1Awb)
> >
> > -constexpr int32_t kMinColourTemperature = 2500;
> > -constexpr int32_t kMaxColourTemperature = 10000;
> > -constexpr int32_t kDefaultColourTemperature = 5000;
> > -
> > -/* Minimum mean value below which AWB can't operate. */
> > -constexpr double kMeanMinThreshold = 2.0;
> > -
> >  class RkISP1AwbStats final : public AwbStats
> >  {
> >  public:
> > -       RkISP1AwbStats(const RGB<double> &rgbMeans)
> > -               : rgbMeans_(rgbMeans)
> > +       RkISP1AwbStats() = default;
> > +       RkISP1AwbStats(const RGB<double> means)
> > +               : AwbStats(means)
> >         {
> > -               rg_ = rgbMeans_.r() / rgbMeans_.g();
> > -               bg_ = rgbMeans_.b() / rgbMeans_.g();
> >         }
> >
> > -       double computeColourError(const RGB<double> &gains) const override
> > +       /* Minimum mean value below which AWB can't operate. */
> > +       double minColourValue() const override
> >         {
> > -               /*
> > -                * Compute the sum of the squared colour error (non-greyness) as
> > -                * it appears in the log likelihood equation.
> > -                */
> > -               double deltaR = gains.r() * rg_ - 1.0;
> > -               double deltaB = gains.b() * bg_ - 1.0;
> > -               double delta2 = deltaR * deltaR + deltaB * deltaB;
> > -
> > -               return delta2;
> > +               return 2.0;
> >         }
> > -
> > -       RGB<double> rgbMeans() const override
> > -       {
> > -               return rgbMeans_;
> > -       }
> > -
> > -private:
> > -       RGB<double> rgbMeans_;
> > -       double rg_;
> > -       double bg_;
> >  };
> >
> > +/**
> > + * \class Awb
> > + * \brief Manage the white balance with automatic and manual controls
> > + */
> > +
> >  Awb::Awb()
> >         : rgbMode_(false)
> >  {
> > @@ -86,40 +56,7 @@ Awb::Awb()
> >   */
> >  int Awb::init(IPAContext &context, const ValueNode &tuningData)
> >  {
> > -       auto &cmap = context.ctrlMap;
> > -       cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,
> > -                                                        kMaxColourTemperature,
> > -                                                        kDefaultColourTemperature);
> > -       cmap[&controls::AwbEnable] = ControlInfo(false, true);
> > -
> > -       cmap[&controls::ColourGains] = ControlInfo(0.0f, 3.996f,
> > -                                                  Span<const float, 2>{ { 1.0f, 1.0f } });
> > -
> > -       if (!tuningData.contains("algorithm"))
> > -               LOG(RkISP1Awb, Info) << "No AWB algorithm specified."
> > -                                    << " Default to grey world";
> > -
> > -       auto mode = tuningData["algorithm"].get<std::string>("grey");
> > -       if (mode == "grey") {
> > -               awbAlgo_ = std::make_unique<AwbGrey>();
> > -       } else if (mode == "bayes") {
> > -               awbAlgo_ = std::make_unique<AwbBayes>();
> > -       } else {
> > -               LOG(RkISP1Awb, Error) << "Unknown AWB algorithm: " << mode;
> > -               return -EINVAL;
> > -       }
> > -       LOG(RkISP1Awb, Debug) << "Using AWB algorithm: " << mode;
> > -
> > -       int ret = awbAlgo_->init(tuningData);
> > -       if (ret) {
> > -               LOG(RkISP1Awb, Error) << "Failed to init AWB algorithm";
> > -               return ret;
> > -       }
> > -
> > -       const auto &src = awbAlgo_->controls();
> > -       cmap.insert(src.begin(), src.end());
> > -
> > -       return 0;
> > +       return awbAlgo_.init(tuningData, context.ctrlMap);
> >  }
> >
> >  /**
> > @@ -128,16 +65,7 @@ int Awb::init(IPAContext &context, const ValueNode &tuningData)
> >  int Awb::configure(IPAContext &context,
> >                    const IPACameraSensorInfo &configInfo)
> >  {
> > -       context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> > -       auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> > -       if (gains)
> > -               context.activeState.awb.automatic.gains = *gains;
> > -       else
> > -               context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
> > -
> > -       context.activeState.awb.autoEnabled = true;
> > -       context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> > -       context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> > +       awbAlgo_.configure(context.activeState.awb);
> >
> >         /*
> >          * Define the measurement window for AWB as a centered rectangle
> > @@ -156,56 +84,12 @@ int Awb::configure(IPAContext &context,
> >  /**
> >   * \copydoc libcamera::ipa::Algorithm::queueRequest
> >   */
> > -void Awb::queueRequest(IPAContext &context,
> > -                      [[maybe_unused]] const uint32_t frame,
> > +void Awb::queueRequest(IPAContext &context, const uint32_t frame,
> >                        IPAFrameContext &frameContext,
> >                        const ControlList &controls)
> >  {
> > -       auto &awb = context.activeState.awb;
> > -
> > -       const auto &awbEnable = controls.get(controls::AwbEnable);
> > -       if (awbEnable && *awbEnable != awb.autoEnabled) {
> > -               awb.autoEnabled = *awbEnable;
> > -
> > -               LOG(RkISP1Awb, Debug)
> > -                       << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> > -       }
> > -
> > -       awbAlgo_->handleControls(controls);
> > -
> > -       frameContext.awb.autoEnabled = awb.autoEnabled;
> > -
> > -       if (awb.autoEnabled)
> > -               return;
> > -
> > -       const auto &colourGains = controls.get(controls::ColourGains);
> > -       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > -       bool update = false;
> > -       if (colourGains) {
> > -               awb.manual.gains.r() = (*colourGains)[0];
> > -               awb.manual.gains.b() = (*colourGains)[1];
> > -               /*
> > -                * \todo Colour temperature reported in metadata is now
> > -                * incorrect, as we can't deduce the temperature from the gains.
> > -                * This will be fixed with the bayes AWB algorithm.
> > -                */
> > -               update = true;
> > -       } else if (colourTemperature) {
> > -               awb.manual.temperatureK = *colourTemperature;
> > -               const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> > -               if (gains) {
> > -                       awb.manual.gains.r() = gains->r();
> > -                       awb.manual.gains.b() = gains->b();
> > -                       update = true;
> > -               }
> > -       }
> > -
> > -       if (update)
> > -               LOG(RkISP1Awb, Debug)
> > -                       << "Set colour gains to " << awb.manual.gains;
> > -
> > -       frameContext.awb.gains = awb.manual.gains;
> > -       frameContext.awb.temperatureK = awb.manual.temperatureK;
> > +       awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,
> > +                             controls);
> >  }
> >
> >  /**
> > @@ -214,15 +98,7 @@ void Awb::queueRequest(IPAContext &context,
> >  void Awb::prepare(IPAContext &context, const uint32_t frame,
> >                   IPAFrameContext &frameContext, RkISP1Params *params)
> >  {
> > -       /*
> > -        * This is the latest time we can read the active state. This is the
> > -        * most up-to-date automatic values we can read.
> > -        */
> > -       if (frameContext.awb.autoEnabled) {
> > -               const auto &awb = context.activeState.awb;
> > -               frameContext.awb.gains = awb.automatic.gains;
> > -               frameContext.awb.temperatureK = awb.automatic.temperatureK;
> > -       }
> > +       awbAlgo_.prepare(context.activeState.awb, frameContext.awb);
> >
> >         auto gainConfig = params->block<BlockType::AwbGain>();
> >         gainConfig.setEnabled(true);
> > @@ -291,68 +167,27 @@ void Awb::process(IPAContext &context,
> >                   const rkisp1_stat_buffer *stats,
> >                   ControlList &metadata)
> >  {
> > -       IPAActiveState &activeState = context.activeState;
> > +       RkISP1AwbStats awbStats = calculateRgbMeans(frameContext, stats);
>
> Uh not too easy to see what's going on here. Maybe we should have done a
> bit of preparatory refactoring on rkisp1 first.
>

What refactoring would you have done on rkisp1 ?

I should have made rkisp1 implementation as similar as possible to
AwbAlgorithm and then moved it to libipa, but I don't think I would be
able to refactor rkisp1 completely mostly because the AwbAlgorithm,
AwbBayes and AwbGrey class already existed/

I suggest you look at the end result in the rkisp1 awb algorithm
instead of the diff ?

> This partially defeats the initial intention of AwbStats. I think it

How so ?

> would be clearer if we do
>
> RkISP1AwbStats awbStats(frameContext, stats);
>
> and move the code from calculateRgbMeans into RkISP1AwbStats. This would
> make it clearer that AwbStats should *not* be a single rgb means triple.

This isn't generic code, so I don't see any big difference having it
in an helper function or having AwbStats call that same function in
the constructor.

> That is unfortunately the only thing the rkisp1 can provide, but it
> shouldn't be the usual implementation (especially the
> computeColourError() only makes real sense if there is more than one
> meaurement.).

I don't see where the confusion could come from to be honest. This is
IPA code, which knows how stats are produced

Maybe I'm missing something

>
> >
> > -       metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
> > -       metadata.set(controls::ColourGains, {
> > -                       static_cast<float>(frameContext.awb.gains.r()),
> > -                       static_cast<float>(frameContext.awb.gains.b())
> > -               });
> > -       metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
> > +       awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,
> > +                        frameContext.lux.lux, metadata);
> > +}
> >
> > +RkISP1AwbStats Awb::calculateRgbMeans(const IPAFrameContext &frameContext,
> > +                                     const rkisp1_stat_buffer *stats) const
> > +{
> >         if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {
> >                 LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
> > -               return;
> > +               return {};
> >         }
> >
> > -       const rkisp1_cif_isp_stat *params = &stats->params;
> > -       const rkisp1_cif_isp_awb_stat *awb = &params->awb;
> > +       const rkisp1_cif_isp_awb_stat *awb = &stats->params.awb;
> >
> >         if (awb->awb_mean[0].cnt == 0) {
> >                 LOG(RkISP1Awb, Debug) << "AWB statistics are empty";
> > -               return;
> > +               return {};
> >         }
> >
> > -       RGB<double> rgbMeans = calculateRgbMeans(frameContext, awb);
> > -
> > -       /*
> > -        * If the means are too small we don't have enough information to
> > -        * meaningfully calculate gains. Freeze the algorithm in that case.
> > -        */
> > -       if (rgbMeans.r() < kMeanMinThreshold && rgbMeans.g() < kMeanMinThreshold &&
> > -           rgbMeans.b() < kMeanMinThreshold)
> > -               return;
> > -
> > -       RkISP1AwbStats awbStats{ rgbMeans };
> > -       AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);
> > -
> > -       /*
> > -        * Clamp the gain values to the hardware, which expresses gains as Q2.8
> > -        * unsigned integer values. Set the minimum just above zero to avoid
> > -        * divisions by zero when computing the raw means in subsequent
> > -        * iterations.
> > -        */
> > -       awbResult.gains = awbResult.gains.clamp(1.0 / 256, 1023.0 / 256);
> > -
> > -       /* Filter the values to avoid oscillations. */
> > -       double speed = 0.2;
> > -       double ct = awbResult.colourTemperature;
> > -       ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);
> > -       awbResult.gains = awbResult.gains * speed +
> > -                         activeState.awb.automatic.gains * (1 - speed);
> > -
> > -       activeState.awb.automatic.temperatureK = static_cast<unsigned int>(ct);
> > -       activeState.awb.automatic.gains = awbResult.gains;
> > -
> > -       LOG(RkISP1Awb, Debug)
> > -               << std::showpoint
> > -               << "Means " << rgbMeans << ", gains "
> > -               << activeState.awb.automatic.gains << ", temp "
> > -               << activeState.awb.automatic.temperatureK << "K";
> > -}
> > -
> > -RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const
> > -{
> >         Vector<double, 3> rgbMeans;
> >
> >         if (rgbMode_) {
> > @@ -419,7 +254,7 @@ RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rk
> >          */
> >         rgbMeans /= frameContext.awb.gains.max(0.01);
> >
> > -       return rgbMeans;
> > +       return RkISP1AwbStats(rgbMeans);
>
> Reading this change I realize I should have moved this function in
> RkISP1AwbStats in first place.
>

Is this comment related to the above suggestion of having

 RkISP1AwbStats awbStats(frameContext, stats);

?

> Best regards,
> Stefan
>
> >  }
> >
> >  REGISTER_IPA_ALGORITHM(Awb, "Awb")
> > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
> > index 60d9ef111495..c7cb59a75bde 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.h
> > +++ b/src/ipa/rkisp1/algorithms/awb.h
> > @@ -7,17 +7,25 @@
> >
> >  #pragma once
> >
> > -#include "libcamera/internal/vector.h"
> > +#include <linux/rkisp1-config.h>
> > +
> > +#include <libcamera/controls.h>
> > +
> > +#include "libcamera/internal/value_node.h"
> >
> >  #include "libipa/awb.h"
> > -#include "libipa/interpolator.h"
> > +#include "libipa/fixedpoint.h"
> >
> >  #include "algorithm.h"
> > +#include "ipa_context.h"
> > +#include "params.h"
> >
> >  namespace libcamera {
> >
> >  namespace ipa::rkisp1::algorithms {
> >
> > +class RkISP1AwbStats;
> > +
> >  class Awb : public Algorithm
> >  {
> >  public:
> > @@ -38,10 +46,10 @@ public:
> >                      ControlList &metadata) override;
> >
> >  private:
> > -       RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext,
> > -                                     const rkisp1_cif_isp_awb_stat *awb) const;
> > +       RkISP1AwbStats calculateRgbMeans(const IPAFrameContext &frameContext,
> > +                                        const rkisp1_stat_buffer *stats) const;
> >
> > -       std::unique_ptr<AwbAlgorithm> awbAlgo_;
> > +       AwbAlgorithm<UQ<2, 8>> awbAlgo_;
> >
> >         bool rgbMode_;
> >  };
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index e5f70d934d67..c36c1f7e0084 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -25,6 +25,7 @@
> >  #include "libcamera/internal/vector.h"
> >
> >  #include "libipa/agc_mean_luminance.h"
> > +#include "libipa/awb.h"
> >  #include "libipa/camera_sensor_helper.h"
> >  #include "libipa/fc_queue.h"
> >  #include "libipa/fixedpoint.h"
> > @@ -48,15 +49,17 @@ struct IPAHwSettings {
> >         bool compand;
> >  };
> >
> > +struct RKISP1AwbSession {
> > +       struct rkisp1_cif_isp_window measureWindow;
> > +       bool enabled;
> > +};
> > +
> >  struct IPASessionConfiguration {
> >         struct {
> >                 struct rkisp1_cif_isp_window measureWindow;
> >         } agc;
> >
> > -       struct {
> > -               struct rkisp1_cif_isp_window measureWindow;
> > -               bool enabled;
> > -       } awb;
> > +       struct RKISP1AwbSession awb;
> >
> >         struct {
> >                 bool supported;
> > @@ -100,17 +103,7 @@ struct IPAActiveState {
> >                 utils::Duration maxFrameDuration;
> >         } agc;
> >
> > -       struct {
> > -               struct AwbState {
> > -                       RGB<double> gains;
> > -                       unsigned int temperatureK;
> > -               };
> > -
> > -               AwbState manual;
> > -               AwbState automatic;
> > -
> > -               bool autoEnabled;
> > -       } awb;
> > +       ipa::awb::ActiveState awb;
> >
> >         struct {
> >                 Matrix<float, 3, 3> manual;
> > @@ -176,11 +169,7 @@ struct IPAFrameContext : public FrameContext {
> >                 bool autoGainModeChange;
> >         } agc;
> >
> > -       struct {
> > -               RGB<double> gains;
> > -               bool autoEnabled;
> > -               unsigned int temperatureK;
> > -       } awb;
> > +       ipa::awb::FrameContext awb;
> >
> >         struct {
> >                 float actualBrightness;
> >
> > --
> > 2.54.0
> >

Hi Jacopo,

Quoting Jacopo Mondi (2026-06-26 12:25:43)
> Hi Stefan
> 
> On Wed, Jun 24, 2026 at 05:04:41PM +0200, Stefan Klug wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > Quoting Jacopo Mondi (2026-06-23 15:54:56)
> > > The current implementation of AwbAlgorithm is the following one:
> > >
> > > AwbAlgorithm o---- RkISP1Awb
> > >     ^
> > >     |
> > >  |-----|
> > >  |     |
> > > Bayes Grey
> > >
> > > The platform-specific Awb implementation instantiate one of
> > > the Bayes or Grey Awb helper classes and both inherit from the
> > > AwbAlgorithm base class.
> > >
> > > The handling of libcamera controls is split between the base and the
> > > derived (mostly Bayes) class.
> > >
> > > We want instead the AwbAlgorithm to handle all-things-libcamera and let
> > > the Bayes and Grey classes implement the logic. In order to achieve that
> > > use composition also in the AwbAlgorithm class and introduce an
> > > AwbImplementation base class from which Bayes and Grey derive from.
> > >
> > > AwbImplementation o---- AwbAlgorithmBase
> > >     ^                           ^
> > >     |                           |
> > >  |-----|                AwbAlgorithm<Q<x,y>> o---- RkISP1Awb
> > >  |     |
> > > Bayes Grey
> > >
> > > The AwbAlgorithm<> class is instantiated with a Q<x, y> template
> > > argument that represents the platform-specific register format which
> > > is used to initialize controls and clamp gains to the hardware limits.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > ---
> > >  src/ipa/libipa/awb.cpp            | 527 +++++++++++++++++++++++++++++++++-----
> > >  src/ipa/libipa/awb.h              | 114 +++++++--
> > >  src/ipa/libipa/awb_bayes.cpp      |  42 +--
> > >  src/ipa/libipa/awb_bayes.h        |  11 +-
> > >  src/ipa/libipa/awb_grey.cpp       |  25 +-
> > >  src/ipa/libipa/awb_grey.h         |   5 +-
> > >  src/ipa/rkisp1/algorithms/awb.cpp | 223 +++-------------
> > >  src/ipa/rkisp1/algorithms/awb.h   |  18 +-
> > >  src/ipa/rkisp1/ipa_context.h      |  29 +--
> > >  9 files changed, 640 insertions(+), 354 deletions(-)
> > >
> > > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> > > index 29a78069c52c..94707faf421b 100644
> > > --- a/src/ipa/libipa/awb.cpp
> > > +++ b/src/ipa/libipa/awb.cpp
> > > @@ -2,7 +2,7 @@
> > >  /*
> > >   * Copyright (C) 2024 Ideas on Board Oy
> > >   *
> > > - * Generic AWB algorithms
> > > + * libIPA Awb algorithms
> > >   */
> > >
> > >  #include "awb.h"
> > > @@ -11,9 +11,16 @@
> > >
> > >  #include <libcamera/control_ids.h>
> > >
> > > +#include "awb_bayes.h"
> > > +#include "awb_grey.h"
> > > +
> > > +constexpr int32_t kMinColourTemperature = 2500;
> > > +constexpr int32_t kMaxColourTemperature = 10000;
> > > +constexpr int32_t kDefaultColourTemperature = 5000;
> > > +
> > >  /**
> > >   * \file awb.h
> > > - * \brief Base classes for AWB algorithms
> > > + * \brief libipa awb implementation
> > >   */
> > >
> > >  namespace libcamera {
> > > @@ -22,34 +29,74 @@ LOG_DEFINE_CATEGORY(Awb)
> > >
> > >  namespace ipa {
> > >
> > > +namespace awb {
> > > +
> > >  /**
> > > - * \class AwbResult
> > > - * \brief The result of an AWB calculation
> > > + * \struct Context
> >
> > I named it Result because it is the result of a algorithm run. For me,
> > context implies a state that is needed for the next run of an algorithm.
> > Both implemented awb algorithms don't need a context as they always
> > start from scratch based on the stats.
> >
> > In the case of FrameContext it makes sense as that is the context that
> > is used to process the frame.
> >
> 
> I see.
> 
> I think I initially had the result of Awb calculation as a separate
> type, but someone noticed the fields were the same and proposed to
> unify them.

Sorry I missed that ping-pong. What about

struct Context: public AwbResult {};

> 
> I would be fine with:
> 
> struct Context {
>         RGB<double> gains;
>         unsigned int temperatureK;
> };
> 
> struct ActiveState {
>         Context manual;
>         Context automatic;
> 
>         bool autoEnabled;
> };
> 
> struct FrameContext : public Context {
>         bool autoEnabled;
> };
> 
> struct AwbResult {
>         RGB<double> gains;
>         unsigned int temperatureK;
> };
> virtual awb::Result calculateAwb(const AwbStats &stats, unsigned int lux,
>                                  std::array<double, 2> ranges) = 0;
> 
> I would be less fine with having a "result" type in ActiveState and
> FrameContext.

I can understand that for symmetry reasons.

> 
> > > + * \brief Awb gains and colour temperature
> > >   *
> > > - * This class holds the result of an auto white balance calculation.
> > > + * \var Context::gains
> > > + * \brief The white balance gains
> > > + *
> > > + * \var Context::temperatureK
> > > + * \brief The colour temperature, in Kelvin
> > >   */
> > >
> > >  /**
> > > - * \var AwbResult::gains
> > > - * \brief The calculated white balance gains
> > > + * \struct ActiveState
> > > + * \brief Active awb state shared across frames
> > > + *
> > > + * \var ActiveState::manual
> > > + * \brief The most recent manually requested awb state
> > > + *
> > > + * \var ActiveState::automatic
> > > + * \brief The most recent automatically calculated awb state
> > > + *
> > > + * \var ActiveState::autoEnabled
> > > + * \brief True when automatic awb is selected
> > >   */
> > >
> > >  /**
> > > - * \var AwbResult::colourTemperature
> > > - * \brief The calculated colour temperature in Kelvin
> > > + * \struct FrameContext
> > > + * \brief Per-frame awb state
> > > + *
> > > + * \var FrameContext::autoEnabled
> > > + * \brief True when automatic awb is in use
> > >   */
> > >
> > > +} /* namespace awb */
> > > +
> > >  /**
> > >   * \class AwbStats
> > > - * \brief An abstraction class wrapping hardware-specific AWB statistics
> > > + * \brief An abstraction class wrapping hardware-specific awb statistics
> > >   *
> > > - * IPA modules using an AWB algorithm based on the AwbAlgorithm class need to
> > > - * implement this class to give the algorithm access to the hardware-specific
> > > - * statistics data.
> > > + * IPA modules shall provide a derived implementation of this class to give the
> > > + * awb algorithm access to the hardware-specific statistics data in a common
> > > + * format.
> > > + */
> > > +
> > > +/**
> > > + * \fn AwbStats::AwbStats()
> > > + * \brief Construct an empty AwbStat
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct Awb statistics from RGB mean values
> > > + * \param[in] rgbMeans The RGB mean values
> > > + */
> > > +AwbStats::AwbStats(const RGB<double> &rgbMeans)
> > > +       : rgbMeans_(rgbMeans)
> > > +{
> > > +       rg_ = rgbMeans_.r() / rgbMeans_.g();
> > > +       bg_ = rgbMeans_.b() / rgbMeans_.g();
> > > +}
> >
> > I think this should stay in the platform specific implementation. I'll
> > add more comments on the corresponding header.
> >
> > > +
> > > +/**
> > > + * AwbStat::~AwbStat
> > > + * \brief Virtual class destructor
> > >   */
> > >
> > >  /**
> > > - * \fn AwbStats::computeColourError()
> > >   * \brief Compute an error value for when the given gains would be applied
> > >   * \param[in] gains The gains to apply
> > >   *
> > > @@ -57,87 +104,383 @@ namespace ipa {
> > >   * applied. To keep the actual implementations computationally inexpensive,
> > >   * the squared colour error shall be returned.
> > >   *
> > > - * If the AWB statistics provide multiple zones, the average of the individual
> > > + * If the awb statistics provide multiple zones, the average of the individual
> > >   * squared errors shall be returned. Averaging/normalizing is necessary so that
> > >   * the numeric dimensions are the same on all hardware platforms.
> > >   *
> > >   * \return The computed error value
> > >   */
> > > +double AwbStats::computeColourError(const RGB<double> &gains) const
> > > +{
> > > +       /*
> > > +        * Compute the sum of the squared colour error (non-greyness) as
> > > +        * it appears in the log likelihood equation.
> > > +        */
> > > +       double deltaR = gains.r() * rg_ - 1.0;
> > > +       double deltaB = gains.b() * bg_ - 1.0;
> > > +       double delta2 = deltaR * deltaR + deltaB * deltaB;
> > > +
> > > +       return delta2;
> > > +}
> >
> > IMHO: This also belongs into the platform specific implementation. Otherwise
> > we will only have implementations that take one measurement into account
> > only because the rkisp1 happened to provide only oine.
> >
> 
> ok
> 
> I'll copy it into Mali and Simple though ?

Welil, in mali I saw that you already run over the individual
measurements, I think that should be moved there (but could be in a
later step).

> 
> > > +
> > > +/**
> > > + * \brief Retrieve if the awb statistics are valid
> > > + *
> > > + * If the colour mean values are too small, the AwbAlgorithm class doesn't have
> > > + * enough information to meaningfully calculate white-balance gains. Freeze the
> > > + * algorithm in that case.
> > > + *
> > > + * \return True if the awb statistics are valid, false otherwise
> > > + */
> > > +bool AwbStats::valid() const
> > > +{
> > > +       double minValue = minColourValue();
> > > +
> > > +       return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||
> > > +              rgbMeans_.b() > minValue;
> > > +}
> > > +
> > > +/**
> > > + * \fn AwbStats::rgRatio()
> > > + * \brief Retrieve the Red/Green ratio
> > > + * \return The Red/Green ratio
> > > + */
> > >
> > >  /**
> > > - * \fn AwbStats::rgbMeans()
> > > - * \brief Get RGB means of the statistics
> > > + * \fn AwbStats::bgRatio()
> > > + * \brief Retrieve the Blue/Green ratio
> > > + * \return The Blue/Green ratio
> > > + */
> > > +
> > > +/**
> > > + * \brief Retrieve the RGB means values
> > >   *
> > >   * Fetch the RGB means from the statistics. The values of each channel are
> > >   * dimensionless and only the ratios are used for further calculations. This is
> > > - * used by the simple grey world model to calculate the gains to apply.
> > > + * used by the AwbGrey to calculate the gains to apply.
> >
> > Coming back after writing the rest: I think we should change this to say
> > it is RGB values in the [0..1] range. The hardware specific part will
> > know how to scale that properly. And then we can get rid of xxRatio()
> > and there is no need for the minColourValue() anymore.
> >
> 
> I might have missed how you would like to get rid of them ?

Well xxRation can be computed based on the means. And the minColourValue
is only needed because at the moment rgbMeans is dimensionless. By
limiting it to 0..1 we can define the minColourValue as constant inside
libipa. Or did I miss something?

> 
> > >   *
> > >   * \return The RGB means
> > >   */
> > > +RGB<double> AwbStats::rgbMeans() const
> > > +{
> > > +       return rgbMeans_;
> > > +}
> > >
> > >  /**
> > > - * \class AwbAlgorithm
> > > - * \brief A base class for auto white balance algorithms
> > > + * \fn AwbStats::minColourValue
> > > + * \brief Retrieve the threshold below which a colour information is not valid
> > > + *
> > > + * Mean colour values as reported by the ISP through the white balance
> > > + * statistics are considered valid for colour gain calculation purposes when
> > > + * they are above a certain value. The awb algorithm needs to know what is the
> > > + * minimum value below which it should ignore the colour mean values.
> > > + *
> > > + * As different ISP platforms report gains in different formats, the threshold
> > > + * is then platform specific, and all IPA implementations that use the
> > > + * AwbAlgorithm class should provide that information by implementing this
> > > + * function.
> > > + *
> > > + * The reported minimum value applies to all three colour channels.
> > >   *
> > > - * This class is a base class for auto white balance algorithms. It defines an
> > > - * interface for the algorithms to implement, and is used by the IPAs to
> > > - * interact with the concrete implementation.
> > > + * This function is used by AwbStats::valid().
> > > + *
> > > + * \return The minimum valid colour value
> > >   */
> > >
> > >  /**
> > > - * \fn AwbAlgorithm::init()
> > > - * \brief Initialize the algorithm with the given tuning data
> > > - * \param[in] tuningData The tuning data to use for the algorithm
> > > + * \var AwbStats::rgbMeans_
> > > + * \brief Mean values of colour channels
> > > + */
> > > +
> > > +/**
> > > + * \var AwbStats::rg_
> > > + * \brief Red/Green ratio
> > > + */
> > > +
> > > +/**
> > > + * \var AwbStats::bg_
> > > + * \brief Blue/Green ratio
> > > + */
> > > +
> > > +/**
> > > + * \class AwbImplementation
> > > + * \brief Pure virtual base class for awb algorithms implementations
> > >   *
> > > - * \return 0 on success, a negative error code otherwise
> > > + * The AwbImplementation class defines the interface for the awb algorithm
> > > + * implementations.
> > > + *
> > > + * It is currently implemented by the AwbGrey and AwbBayes classes.
> > > + *
> > > + * The interface defines an init() function to initialize the algorithm with the
> > > + * content of the tuning file and two functions to compute colour gains
> > > + * according to the algorithm operating mode (auto or manual) in use.
> > > + *
> > > + * The calculateAwb() function calculates colour gains given a set of statistics
> > > + * provided by the IPA module. It is used when the algorithm operates in auto
> > > + * mode and gains are dynamically computed given a new set of statistics from
> > > + * the awb engine.
> > > + *
> > > + * The gainsFromColourTemperature() function instead interpolates a gain curve
> > > + * if specified in the tuning file with the supplied colour temperature. This
> > > + * function is used in manual mode where it is expected the application to
> > > + * supply a colour temperature and the algorithm to adjust the white balance
> > > + * gains to it.
> > > + */
> > > +
> > > +/**
> > > + * \fn AwbImplementation::~AwbImplementation
> > > + * \brief Virtual class destructor
> > > + */
> > > +
> > > +/**
> > > + * \fn AwbImplementation::init()
> > > + * \param[in] tuningData
> > > + * \brief Initialize the algorithm by parsing \a tuningData
> > >   */
> > >
> > >  /**
> > > - * \fn AwbAlgorithm::calculateAwb()
> > > - * \brief Calculate AWB data from the given statistics
> > > - * \param[in] stats The statistics to use for the calculation
> > > - * \param[in] lux The lux value of the scene
> > > + * \fn AwbImplementation::calculateAwb()
> > > + * \param[in] stats The awb statistics
> > > + * \param[in] lux The estimated lux level
> > > + * \param[in] ranges The colour temperature search limits (AwbBayes only)
> > >   *
> > > - * Calculate an AwbResult object from the given statistics and lux value. A \a
> > > - * lux value of 0 means it is unknown or invalid and the algorithm shall ignore
> > > - * it.
> > > + * Calculate a new set of colour gains and a colour temperature given a new
> > > + * set of statistics \a stats, an estimated luminance \a lux and a range of
> > > + * colour temperature to limit the search (for AwbBayes only).
> >
> > I would drop the reference to AwbBayes
> >
> 
> ok
> 
> > >   *
> > > - * \return The AWB result
> > > + * \return An awb::Context computed using the new statistics
> > > + */
> > > +/**
> > > + * \fn AwbImplementation::gainsFromColourTemperature()
> > > + * \param[in] temperatureK Colour temperature in Kelvin
> > > + *
> > > + * Calculate a new set of colour gains by interpolating a gain curve (if
> > > + * provided in the tuning file) with a new colour temperature \a temperatureK.
> >
> > This (interpolating a curve) is one way to do it. But it is an
> > implementation detail of the specific algo. So I think it doesn't belong
> > to the base class documentation or be marked as example.
> 
> I can try to move this to the documentation of AwbBayes and AwbGrey ?
> 
> I'll also change the above description of gainsFromColourTemperature()
> in the class documentation
> 
> >
> > > + *
> > > + * \return The RGB gain values adjusted to \a temperatureK or std::nullopt if
> > > + * no gain curve is specified in the tuning data
> >
> > Hm, for a base class, this feels a bit too specific. Assume a neural
> > network based awb. That would potentially not require the curve.
> 
> I'll drop "if no gain curve is specified in the tuning data"
> 
> >
> > >   */
> > >
> > >  /**
> > > - * \fn AwbAlgorithm::gainsFromColourTemperature()
> > > - * \brief Compute white balance gains from a colour temperature
> > > - * \param[in] colourTemperature The colour temperature in Kelvin
> > > + * \class AwbAlgorithmBase
> > > + * \brief Base class for AwbAlgorithm for non-templated functions implementation
> > >   *
> > > - * Compute the white balance gains from a \a colourTemperature. This function
> > > - * does not take any statistics into account. It is used to compute the colour
> > > - * gains when the user manually specifies a colour temperature.
> >
> > This information got lost, but is IMHO important for the implementer to
> > understand why he needs to implement this.
> 
> I'll re-use it to replace/adjust the function description in the class
> documentation
> 
> >
> > > + * Base class for AwbAlgorithm where non-templated functions are implemented.
> > > + * IPA implementations shall use AwbAlgorithm and not this class.
> > > + */
> > > +
> > > +/**
> > > + * \brief Initialize the algorithm with the given tuning data
> > > + * \param[in] tuningData The tuning data to use for the algorithm
> > > + *
> > > + * Parse \a tuningData to initialize the awb algorithm and register controls.
> > > + * IPA modules are expected to call this function as part of their
> > > + * implementation of Algorithm::init().
> > >   *
> > > - * \return The colour gains or std::nullopt if the conversion is not possible
> > > + * \return 0 on success, a negative error code otherwise
> > > + */
> > > +int AwbAlgorithmBase::init(const ValueNode &tuningData)
> > > +{
> > > +       bayes_ = false;
> > > +
> > > +       if (!tuningData.contains("algorithm"))
> > > +               LOG(Awb, Info) << "No Awb algorithm specified, using grey world";
> > > +
> > > +       auto mode = tuningData["algorithm"].get<std::string>("grey");
> > > +       if (mode == "grey") {
> > > +               impl_ = std::make_unique<AwbGrey>();
> > > +       } else if (mode == "bayes") {
> > > +               impl_ = std::make_unique<AwbBayes>();
> > > +               bayes_ = true;
> > > +       } else {
> > > +               LOG(Awb, Error) << "Unknown Awb algorithm: " << mode;
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       LOG(Awb, Debug) << "Using Awb algorithm: " << mode;
> > > +
> > > +       int ret = impl_->init(tuningData);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       controls_[&controls::ColourTemperature] =
> > > +               ControlInfo(kMinColourTemperature, kMaxColourTemperature,
> > > +                           kDefaultColourTemperature);
> > > +       controls_[&controls::AwbEnable] = ControlInfo(false, true);
> > > +
> > > +       return parseModeConfigs(tuningData, controls::AwbAuto);
> > > +}
> > > +
> > > +/**
> > > + * \brief Configure the awb algorithm
> > > + * \param[in] state The awb active state
> > > + * \return 0 if successful, an error code otherwise
> > >   */
> > > +int AwbAlgorithmBase::configure(awb::ActiveState &state)
> > > +{
> > > +       state.manual.gains = RGB<double>{ 1.0 };
> > > +       auto gains = impl_->gainsFromColourTemperature(kDefaultColourTemperature);
> > > +       if (gains)
> > > +               state.automatic.gains = *gains;
> > > +       else
> > > +               state.automatic.gains = RGB<double>{ 1.0 };
> > > +
> > > +       state.autoEnabled = true;
> > > +       state.manual.temperatureK = kDefaultColourTemperature;
> > > +       state.automatic.temperatureK = kDefaultColourTemperature;
> > > +
> > > +       return 0;
> > > +}
> > >
> > >  /**
> > > - * \fn AwbAlgorithm::controls()
> > > - * \brief Get the controls info map for this algorithm
> > > + * \brief Queue a Request to the awb algorithm
> > > + * \param[in] state The awb active state
> > > + * \param[in] frame The frame number
> > > + * \param[in] frameContext The awb frame context
> > > + * \param[in] controls The list of controls part of the Request
> > > + *
> > > + * Queue a new Request to the awb algorithm and modify its behaviour according
> > > + * to the provided controls.
> > >   *
> > > - * \return The controls info map
> > > + * The currently handled controls are:
> > > + * - controls::AwbEnable
> > > + * - controls::AwbMode
> > > + * - controls::ColourGains
> > > + * - controls::ColourTemperature
> > >   */
> > > +void AwbAlgorithmBase::queueRequest(awb::ActiveState &state,
> > > +                                   [[maybe_unused]] const uint32_t frame,
> > > +                                   awb::FrameContext &frameContext,
> > > +                                   const ControlList &controls)
> > > +{
> > > +       const auto &awbEnable = controls.get(controls::AwbEnable);
> > > +       if (awbEnable && *awbEnable != state.autoEnabled) {
> > > +               state.autoEnabled = *awbEnable;
> > > +
> > > +               LOG(Awb, Debug)
> > > +                       << (*awbEnable ? "Enabling" : "Disabling") << " Awb";
> > > +       }
> > > +
> > > +       auto mode = controls.get(controls::AwbMode);
> > > +       if (mode) {
> > > +               auto it = modes_.find(static_cast<controls::AwbModeEnum>(*mode));
> > > +               if (it == modes_.end()) {
> > > +                       LOG(Awb, Error) << "Unsupported Awb mode " << *mode;
> > > +                       return;
> > > +               }
> > > +
> > > +               currentMode_ = &it->second;
> > > +       }
> > > +
> > > +       frameContext.autoEnabled = state.autoEnabled;
> > > +
> > > +       if (frameContext.autoEnabled)
> > > +               return;
> > > +
> > > +       const auto &colourGains = controls.get(controls::ColourGains);
> > > +       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > > +       bool update = false;
> > > +       if (colourGains) {
> > > +               state.manual.gains.r() = (*colourGains)[0];
> > > +               state.manual.gains.b() = (*colourGains)[1];
> > > +               /*
> > > +                * \todo Colour temperature reported in metadata is now
> > > +                * incorrect, as we can't deduce the temperature from the gains.
> > > +                * This will be fixed with the bayes AWB algorithm.
> > > +                */
> > > +               update = true;
> > > +       } else if (colourTemperature) {
> > > +               state.manual.temperatureK = *colourTemperature;
> > > +               const auto &gains = impl_->gainsFromColourTemperature(*colourTemperature);
> > > +               if (gains) {
> > > +                       state.manual.gains.r() = gains->r();
> > > +                       state.manual.gains.b() = gains->b();
> > > +                       update = true;
> > > +               }
> > > +       }
> > > +
> > > +       if (update)
> > > +               LOG(Awb, Debug)
> > > +                       << "Set colour gains to " << state.manual.gains;
> > > +
> > > +       frameContext.gains = state.manual.gains;
> > > +       frameContext.temperatureK = state.manual.temperatureK;
> > > +}
> > >
> > >  /**
> > > - * \fn AwbAlgorithm::handleControls()
> > > - * \param[in] controls The controls to handle
> > > - * \brief Handle the controls supplied in a request
> > > + * \brief Set the gains and colour temperature values in \a frameContext
> > > + * \param[in] state The awb active state
> > > + * \param[in] frameContext The awb frame context
> > > + *
> > > + * If auto mode is enabled, take the most recently computed gains and use them
> > > + * for the current frame. Otherwise, if in manual mode, gains and colour
> > > + * temperature for a frame are set at queueRequest() time.
> > >   */
> > > +void AwbAlgorithmBase::prepare(awb::ActiveState &state,
> > > +                              awb::FrameContext &frameContext)
> > > +{
> > > +       if (frameContext.autoEnabled) {
> > > +               frameContext.gains = state.automatic.gains;
> > > +               frameContext.temperatureK = state.automatic.temperatureK;
> > > +       }
> > > +}
> > >
> > >  /**
> > > + * \brief Process awb statistics to calculate gains and populate metadata
> > > + * \param[in] state The awb active state
> > > + * \param[in] frameContext The awb frame context
> > > + * \param[in] stats The awb statistics
> > > + * \param[in] lux The lux value as estimated by the IPA module
> > > + * \param[out] metadata The metadata list
> > > + *
> > > + * Process \a stats to calculate new gains and colour temperature and populate
> > > + * \a metadata with the results.
> > > + */
> > > +void AwbAlgorithmBase::process(awb::ActiveState &state,
> > > +                              awb::FrameContext &frameContext,
> > > +                              const AwbStats &stats, unsigned int lux,
> > > +                              ControlList &metadata)
> > > +{
> > > +       if (!stats.valid())
> > > +               return;
> > > +
> > > +       auto awbResult = impl_->calculateAwb(stats, lux, { currentMode_->ctLo,
> > > +                                                          currentMode_->ctHi });
> > > +
> > > +       /*
> > > +        * Clamp the gain values to the hardware, according to the gainMin_
> > > +        * and gainMax_ values.
> > > +        */
> > > +       awbResult.gains = awbResult.gains.clamp(gainMin_, gainMax_);
> > > +
> > > +       /* Smooth color gains adjustments. */
> > > +       double speed = 0.2;
> > > +       double ct = awbResult.temperatureK;
> > > +       ct = ct * speed + state.automatic.temperatureK * (1 - speed);
> > > +
> > > +       state.automatic.temperatureK = awbResult.temperatureK;
> > > +       state.automatic.gains = awbResult.gains * speed +
> > > +                               state.automatic.gains * (1 - speed);
> > > +
> > > +       /* Populate metadata. */
> > > +       metadata.set(controls::AwbEnable, frameContext.autoEnabled);
> > > +       metadata.set(controls::ColourGains, { static_cast<float>(frameContext.gains.r()),
> > > +                                             static_cast<float>(frameContext.gains.b()) });
> > > +       metadata.set(controls::ColourTemperature, frameContext.temperatureK);
> > > +
> > > +       LOG(Awb, Debug) << std::showpoint << "Means " << stats.rgbMeans()
> > > +                       << ", gains " << state.automatic.gains
> > > +                       << ", temp " << state.automatic.temperatureK << "K";
> > > +}
> > > +
> > > +/*
> > >   * \brief Parse the mode configurations from the tuning data
> > >   * \param[in] tuningData the ValueNode representing the tuning data
> > >   * \param[in] def The default value for the AwbMode control
> > >   *
> > >   * Utility function to parse the tuning data for an AwbMode entry and read all
> > > - * provided modes. It adds controls::AwbMode to AwbAlgorithm::controls_ and
> > > - * populates AwbAlgorithm::modes_. For a list of possible modes see \ref
> > > + * provided modes. It adds controls::AwbMode to AwbAlgorithmBase::controls_ and
> > > + * populates AwbAlgorithmBase::modes_. For a list of possible modes see \ref
> > >   * controls::AwbModeEnum.
> > >   *
> > >   * Each mode entry must contain a "lo" and "hi" key to specify the lower and
> > > @@ -156,15 +499,23 @@ namespace ipa {
> > >   *       ...
> > >   * \endcode
> > >   *
> > > - * If \a def is supplied but not contained in the the \a tuningData, -EINVAL is
> > > + * If \a def is supplied but not contained in the \a tuningData, -EINVAL is
> > >   * returned.
> > >   *
> > > + * AwbModes are only used by the AwbBayes implementation.
> > > + *
> > >   * \sa controls::AwbModeEnum
> > >   * \return Zero on success, negative error code otherwise
> > >   */
> > > -int AwbAlgorithm::parseModeConfigs(const ValueNode &tuningData,
> > > -                                  const ControlValue &def)
> > > +int AwbAlgorithmBase::parseModeConfigs(const ValueNode &tuningData,
> > > +                                      const ControlValue &def)
> > >  {
> > > +       if (!bayes_) {
> > > +               /* AwbGrey does not support and does not use modes. */
> > > +               currentMode_ = &AwbGreyMode;
> > > +               return 0;
> > > +       }
> > > +
> > >         std::vector<ControlValue> availableModes;
> > >
> > >         const ValueNode &modes = tuningData[controls::AwbMode.name()];
> > > @@ -227,37 +578,83 @@ int AwbAlgorithm::parseModeConfigs(const ValueNode &tuningData,
> > >         }
> > >
> > >         controls_[&controls::AwbMode] = ControlInfo(availableModes, def);
> > > +       currentMode_ = &modes_[controls::AwbAuto];
> > >
> > >         return 0;
> > >  }
> > >
> > >  /**
> > > - * \class AwbAlgorithm::ModeConfig
> > > - * \brief Holds the configuration of a single AWB mode
> > > + * \var AwbAlgorithmBase::controls_
> > > + * \brief Controls info map for the controls registered by the awb algorithm
> > > + */
> > > +
> > > +/**
> > > + * \var AwbAlgorithmBase::gainMin_
> > > + * \brief The minimum supported gain value
> > >   *
> > > - * AWB modes limit the regulation of the AWB algorithm to a specific range of
> > > - * colour temperatures.
> > > + * Minimum gain value used to clamp the awb algorithm calculation results in the
> > > + * range supported by the platform awb engine.
> > > + *
> > > + * The min and max gain values are initialized by AwbAlgorithm::init().
> > >   */
> > >
> > >  /**
> > > - * \var AwbAlgorithm::ModeConfig::ctLo
> > > + * \var AwbAlgorithmBase::gainMax_
> > > + * \brief The maximum supported gain value
> > > + *
> > > + * Maximum gain value used to clamp the awb algorithm calculation results in the
> > > + * range supported by the platform awb engine.
> > > + *
> > > + * The min and max gain values are initialized by AwbAlgorithm::init().
> > > + */
> > > +
> > > +/*
> > > + * \class AwbAlgorithmBase::ModeConfig
> > > + * \brief Holds the configuration of a single awb mode
> > > + *
> > > + * AWB modes limit the regulation of the awb algorithm to a specific range of
> > > + * colour temperatures. Use by AwbBayes only.
> > > + */
> > > +
> > > +/*
> > > + * \var AwbAlgorithmBase::ModeConfig::ctLo
> > >   * \brief The lowest valid colour temperature of that mode
> > >   */
> > >
> > > -/**
> > > - * \var AwbAlgorithm::ModeConfig::ctHi
> > > +/*
> > > + * \var AwbAlgorithmBase::ModeConfig::ctHi
> > >   * \brief The highest valid colour temperature of that mode
> > >   */
> > >
> > > +/*
> > > + * \var AwbAlgorithmBase::modes_
> > > + * \brief Map of all configured modes
> > > + * \sa AwbAlgorithmBase::parseModeConfigs
> > > + */
> > > +
> > >  /**
> > > - * \var AwbAlgorithm::controls_
> > > - * \brief Controls info map for the controls provided by the algorithm
> > > + * \class AwbAlgorithm
> > > + * \brief The libipa awb algorithm
> > > + * \tparam Q The fixedpoint register representation of gain values
> > > + *
> > > + * Implement the awb algorithm for libipa.
> > > + *
> > > + * The AwbAlgorithm class implements an interface similar in spirit to the one
> > > + * of the Algorithm class. IPA modules are expected to store an instance of
> > > + * AwbAlgorithm as class member, template it with the awb engine gain register
> > > + * representation and call its function in their implementations of the
> > > + * Algorithm interface.
> > > + *
> > > + * The AwbAlgorithm instantiates an AwbImplementation implementation (AwbGrey or
> > > + * AwbBayes) at init() time by parsing the tuning data and uses it to compute
> > > + * the RGB gains and estimate a colour temperature given a set of statistics
> > > + * in the form of a AwbStats derived implementation.
> > >   */
> > >
> > >  /**
> > > - * \var AwbAlgorithm::modes_
> > > - * \brief Map of all configured modes
> > > - * \sa AwbAlgorithm::parseModeConfigs
> > > + * \fn AwbAlgorithm::init()
> > > + * \param[in] controls The info map of the IPA controls
> > > + * \copydoc AwbAlgorithmBase::init()
> > >   */
> > >
> > >  } /* namespace ipa */
> > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> > > index 09c00e47d604..f453e1f9fd3d 100644
> > > --- a/src/ipa/libipa/awb.h
> > > +++ b/src/ipa/libipa/awb.h
> > > @@ -2,11 +2,12 @@
> > >  /*
> > >   * Copyright (C) 2024 Ideas on Board Oy
> > >   *
> > > - * Generic AWB algorithms
> > > + * libIPA AWB algorithms
> > >   */
> > >
> > >  #pragma once
> > >
> > > +#include <array>
> > >  #include <map>
> > >  #include <optional>
> > >
> > > @@ -20,46 +21,119 @@ namespace libcamera {
> > >
> > >  namespace ipa {
> > >
> > > -struct AwbResult {
> > > +namespace awb {
> > > +
> > > +struct Context {
> >
> > As written above, I don't particularly like that new name.
> >
> 
> Even if I split Context and Result ?
> I'll happily accept proposals.

With the split, I'm fine.

> 
> > >         RGB<double> gains;
> > > -       double colourTemperature;
> > > +       unsigned int temperatureK;
> > > +};
> > > +
> > > +struct ActiveState {
> > > +       Context manual;
> > > +       Context automatic;
> > > +
> > > +       bool autoEnabled;
> > > +};
> > > +
> > > +struct FrameContext : public Context {
> > > +       bool autoEnabled;
> > >  };
> > >
> > > +} /* namespace awb */
> > > +
> > >  struct AwbStats {
> > > -       virtual double computeColourError(const RGB<double> &gains) const = 0;
> > > -       virtual RGB<double> rgbMeans() const = 0;
> >
> > This was designed as the interface that the platform specific
> > implementation has to provide. Now it is interface and partial
> > implementation at the same time. Can we keep the interface separate and
> > - if we really want to have this implementation at a central place -
> > maybe call it "AwbStatsMinimal" or something?
> >
> 
> Are you suggesting a pure virtual base class, derived by a partial
> implementation which implementations have to derive and complete ?

Yes, a pure virtual base class, and every platform does it's own
implementation.

The AwbStatsSimple could be equal to the current
proposal. When we now ditch the minColourValue, rkisp could just use
AwbStatsSimple without deriving from it at all.

But I'm really unsure if we should provide AwbStatsSimple. I am afraid
that everyone treats that as the right thing to do, but it should be the
last resort if we can't do better. As long as we keep that in mind, it
won't hurt and makes it easier to get started.

My hope is that after we implement a few more capable ISPs there will be
a common pattern for awb stats handling with multiple measurements. That
could then again be moved in libipa to provide AwbStatsMultiwindow or
so.

> 
> What would the gain be in splitting the base class in 2 ?

To clarify that it should not be done like that and that a constructor
that gets a awbMeans vector is normally not the correct thing to do.

> 
> > > +       AwbStats() = default;
> > > +       AwbStats(const RGB<double> &means);
> > > +       virtual ~AwbStats() = default;
> > > +
> > > +       bool valid() const;
> > > +
> > > +       virtual double rgRatio() const { return rg_; }
> > > +       virtual double bgRatio() const { return bg_; }
> > > +       virtual double computeColourError(const RGB<double> &gains) const;
> > > +       virtual RGB<double> rgbMeans() const;
> >
> > rgbMeans and xxRatio seem to server a very similar purpose now. I need
> > to look further.
> >
> > ...
> >
> > Coming back. I don't understand why we can't go for rgbMeans() only. I
> > think the old interface should be sufficient. For cases where the
> > hardware only provides a ratio, we could IMHO calculate a means value
> > based on that.
> >
> 
> The existing implementation of RkISP1AwbStats already calculates rg_
> and bg_ as
> 
>         {
>                rg_ = rgbMeans_.r() / rgbMeans_.g();
>                bg_ = rgbMeans_.b() / rgbMeans_.g();
>         }
> 
> Mali provides stats already in the r/g and b/g format
> 
>         MaliC55AwbStats(const RGB<double> &rgbMeans)
>         {
>                 /* The Mali-C55 ISP already provides stats as R/G and B/G ratios. */
>                 rg_ = rgbMeans.r();
>                 bg_ = rgbMeans.b();
>         }
> 
> With rgbMeans.g() fixed to 1.
> 
> So I can reconstruct r and b by inverting them in the MaliC55AwbStats
> constructor and revert the diff in AwbGrey::calculateAwb()
> 
>         /*
> -        * Estimate the red and blue gains to apply in a grey world. The green
> -        * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the
> -        * divisor to a minimum value of 1.0.
> +        * Calculate the red and blue gains to apply in a grey world by simply
> +        * inverting the red/green and blue/green ratios as reported in
> +        * statistics. The green gain is hardcoded to 1.0.
>          */
> -       result.gains.r() = means.g() / std::max(means.r(), 1.0);
> +       result.gains.r() = 1.0 / stats.rgRatio();
>         result.gains.g() = 1.0;
> -       result.gains.b() = means.g() / std::max(means.b(), 1.0);
> +       result.gains.b() = 1.0 / stats.bgRatio();
> 
> To be honest I like
> 
> +       result.gains.r() = 1.0 / stats.rgRatio();
> 
> better than
> 
> -       result.gains.r() = means.g() / std::max(means.r(), 1.0);
> 
> but I won't bother.

Yes the maths look nicer and the result is the same. But still a rgb
mean can carry a bit more information as it encodes brightness
information as well. We do not make use of that right now, but I feel we
should keep it.

Oh and when we limit the means to 0..1 the above std::max() would need to
change to something like 0.01

> 
> 
> > I know we talked about this in Nice, but now reading it, I can't see the
> > reason anymore.
> >
> > >
> > >  protected:
> > > -       ~AwbStats() = default;
> > > +       virtual double minColourValue() const = 0;
> > > +
> > > +       RGB<double> rgbMeans_;
> > > +       double rg_;
> > > +       double bg_;
> > >  };
> > >
> > > -class AwbAlgorithm
> > > +class AwbImplementation
> > >  {
> > >  public:
> > > -       virtual ~AwbAlgorithm() = default;
> > > -
> > > +       virtual ~AwbImplementation() = default;
> > >         virtual int init(const ValueNode &tuningData) = 0;
> > > -       virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
> > > -       virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
> > > +       virtual awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> > > +                                         std::array<double, 2> ranges) = 0;
> > > +       virtual std::optional<RGB<double>>
> > > +       gainsFromColourTemperature(double temperatureK) = 0;
> > > +};
> > >
> > > -       const ControlInfoMap::Map &controls() const
> > > -       {
> > > -               return controls_;
> > > -       }
> > > +class AwbAlgorithmBase
> > > +{
> > > +public:
> > > +       int init(const ValueNode &tuningData);
> > > +
> > > +       int configure(awb::ActiveState &state);
> > > +
> > > +       void queueRequest(awb::ActiveState &state,
> > > +                         const uint32_t frame,
> > > +                         awb::FrameContext &frameContext,
> > > +                         const ControlList &controls);
> > > +
> > > +       void prepare(awb::ActiveState &state, awb::FrameContext &frameContext);
> > >
> > > -       virtual void handleControls([[maybe_unused]] const ControlList &controls) {}
> > > +       void process(awb::ActiveState &state, awb::FrameContext &frameContext,
> > > +                    const AwbStats &stats, unsigned int lux,
> > > +                    ControlList &metadata);
> > >
> > >  protected:
> > > -       int parseModeConfigs(const ValueNode &tuningData,
> > > -                            const ControlValue &def = {});
> > > +       AwbAlgorithmBase() = default;
> > >
> > > +       ControlInfoMap::Map controls_;
> > > +       float gainMin_;
> > > +       float gainMax_;
> > > +
> > > +private:
> > >         struct ModeConfig {
> > >                 double ctHi;
> > >                 double ctLo;
> > >         };
> > >
> > > -       ControlInfoMap::Map controls_;
> > > -       std::map<controls::AwbModeEnum, AwbAlgorithm::ModeConfig> modes_;
> > > +       /* AwbGrey does not support modes; */
> > > +       static constexpr ModeConfig AwbGreyMode = { 0.0, 0.0 };
> > > +
> > > +       int parseModeConfigs(const ValueNode &tuningData,
> > > +                            const ControlValue &def = {});
> > > +
> > > +       std::map<controls::AwbModeEnum, AwbAlgorithmBase::ModeConfig> modes_;
> > > +       const ModeConfig *currentMode_ = nullptr;
> > > +       std::unique_ptr<AwbImplementation> impl_;
> > > +       bool bayes_ = false;
> > > +};
> > > +
> > > +template<typename Q>
> > > +class AwbAlgorithm : public AwbAlgorithmBase
> > > +{
> > > +public:
> > > +       int init(const ValueNode &tuningData, ControlInfoMap::Map &controls)
> > > +       {
> > > +               AwbAlgorithmBase::init(tuningData);
> > > +
> > > +               gainMin_ = std::max(Q::TraitsType::min, 1.0f);
> > > +               gainMax_ = Q::TraitsType::max;
> > > +
> > > +               controls_[&controls::ColourGains] =
> > > +                       ControlInfo(gainMin_, gainMax_,
> > > +                                   Span<const float, 2>{ { 1.0f, 1.0f } });
> > > +
> > > +               controls.insert(controls_.begin(), controls_.end());
> > > +
> > > +               return 0;
> > > +       }
> > >  };
> >
> > This feels like an overkill for a template. The only reason is to deduce
> > the min max values for the gainMin and gainMax values. Why not adding a
> > gainRange param to init()?
> >
> 
> I think initializing the algorithm with the register format
> representation is elegant and all algorithms in libipa follow the same
> design pattern.
> 
> Given the register format is fixed, I don't see any advantage in
> passing parameters to init()

I can go with that. I am a bit opinionated against templates in that
they a have this infectious property. If one thing is a template,
everything becomes a template and you can't runtime decide anymore. So
if we get two similar ISPs with different ranges but otherwise equal
functionality we need a second instantiation. So it's more of a gut
feeling to me than a strong technical argument. 

> 
> > >
> > >  } /* namespace ipa */
> > > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp
> > > index 9fd85e5a4505..34d86ce6938b 100644
> > > --- a/src/ipa/libipa/awb_bayes.cpp
> > > +++ b/src/ipa/libipa/awb_bayes.cpp
> > > @@ -140,11 +140,6 @@ void Interpolator<Pwl>::interpolate(const Pwl &a, const Pwl &b, Pwl &dest, doubl
> > >   * \brief How far to wander off CT curve towards "more green"
> > >   */
> > >
> > > -/**
> > > - * \var AwbBayes::currentMode_
> > > - * \brief The currently selected mode
> > > - */
> > > -
> > >  int AwbBayes::init(const ValueNode &tuningData)
> > >  {
> > >         int ret = colourGainCurve_.readYaml(tuningData["colourGains"], "ct", "gains");
> > > @@ -172,14 +167,6 @@ int AwbBayes::init(const ValueNode &tuningData)
> > >                 return ret;
> > >         }
> > >
> > > -       ret = parseModeConfigs(tuningData, controls::AwbAuto);
> > > -       if (ret) {
> > > -               LOG(Awb, Error)
> > > -                       << "Failed to parse mode parameter from tuning file";
> > > -               return ret;
> > > -       }
> > > -       currentMode_ = &modes_[controls::AwbAuto];
> > > -
> > >         transversePos_ = tuningData["transversePos"].get<double>(0.01);
> > >         transverseNeg_ = tuningData["transverseNeg"].get<double>(0.01);
> > >         if (transversePos_ <= 0 || transverseNeg_ <= 0) {
> > > @@ -260,18 +247,6 @@ int AwbBayes::readPriors(const ValueNode &tuningData)
> > >         return 0;
> > >  }
> > >
> > > -void AwbBayes::handleControls(const ControlList &controls)
> > > -{
> > > -       auto mode = controls.get(controls::AwbMode);
> > > -       if (mode) {
> > > -               auto it = modes_.find(static_cast<controls::AwbModeEnum>(*mode));
> > > -               if (it != modes_.end())
> > > -                       currentMode_ = &it->second;
> > > -               else
> > > -                       LOG(Awb, Error) << "Unsupported AWB mode " << *mode;
> > > -       }
> > > -}
> > > -
> > >  std::optional<RGB<double>> AwbBayes::gainsFromColourTemperature(double colourTemperature)
> > >  {
> > >         /*
> > > @@ -283,7 +258,9 @@ std::optional<RGB<double>> AwbBayes::gainsFromColourTemperature(double colourTem
> > >         return RGB<double>{ { gains[0], 1.0, gains[1] } };
> > >  }
> > >
> > > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
> > > +awb::Context
> > > +AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux,
> > > +                      std::array<double, 2> ranges)
> >
> > Nit: ranges is confusion as it is a single range. So I'd rename it to
> > range or even ctRange to make it easier to understand everywhere.
> >
> > And I guess it could be Span.
> >
> 
> I'll try a Span
> 
> > >  {
> > >         ipa::Pwl prior;
> > >         if (lux > 0) {
> > > @@ -295,7 +272,7 @@ AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
> > >                 prior.append(0, 1.0);
> > >         }
> > >
> > > -       double t = coarseSearch(prior, stats);
> > > +       double t = coarseSearch(prior, stats, ranges);
> > >         double r = ctR_.eval(t);
> > >         double b = ctB_.eval(t);
> > >         LOG(Awb, Debug)
> > > @@ -316,14 +293,15 @@ AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
> > >                 << "After fine search: r " << r << " b " << b << " (gains r "
> > >                 << 1 / r << " b " << 1 / b << ")";
> > >
> > > -       return { { { 1.0 / r, 1.0, 1.0 / b } }, t };
> > > +       return { { { 1.0 / r, 1.0, 1.0 / b } }, static_cast<unsigned int>(t) };
> > >  }
> > >
> > > -double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) const
> > > +double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats,
> > > +                             std::array<double, 2> ranges) const
> > >  {
> > >         std::vector<Pwl::Point> points;
> > >         size_t bestPoint = 0;
> > > -       double t = currentMode_->ctLo;
> > > +       double t = ranges[0];
> > >         int spanR = -1;
> > >         int spanB = -1;
> > >         LimitsRecorder<double> errorLimits;
> > > @@ -345,14 +323,14 @@ double AwbBayes::coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) cons
> > >                 if (points.back().y() < points[bestPoint].y())
> > >                         bestPoint = points.size() - 1;
> > >
> > > -               if (t == currentMode_->ctHi)
> > > +               if (t == ranges[1])
> > >                         break;
> > >
> > >                 /*
> > >                  * Ensure even steps along the r/b curve by scaling them by the
> > >                  * current t.
> > >                  */
> > > -               t = std::min(t + t / 10 * kSearchStep, currentMode_->ctHi);
> > > +               t = std::min(t + t / 10 * kSearchStep, ranges[1]);
> > >         }
> > >
> > >         t = points[bestPoint].x();
> > > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h
> > > index 1e3373676bc0..8264bcb09f38 100644
> > > --- a/src/ipa/libipa/awb_bayes.h
> > > +++ b/src/ipa/libipa/awb_bayes.h
> > > @@ -20,22 +20,23 @@ namespace libcamera {
> > >
> > >  namespace ipa {
> > >
> > > -class AwbBayes : public AwbAlgorithm
> > > +class AwbBayes : public AwbImplementation
> > >  {
> > >  public:
> > >         AwbBayes() = default;
> > >
> > >         int init(const ValueNode &tuningData) override;
> > > -       AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;
> > > +       awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> > > +                                 std::array<double, 2> ranges) override;
> > >         std::optional<RGB<double>> gainsFromColourTemperature(double temperatureK) override;
> > > -       void handleControls(const ControlList &controls) override;
> > >
> > >  private:
> > >         int readPriors(const ValueNode &tuningData);
> > >
> > >         void fineSearch(double &t, double &r, double &b, ipa::Pwl const &prior,
> > >                         const AwbStats &stats) const;
> > > -       double coarseSearch(const ipa::Pwl &prior, const AwbStats &stats) const;
> > > +       double coarseSearch(const ipa::Pwl &prior, const AwbStats &stats,
> > > +                           std::array<double, 2> ranges) const;
> > >         double interpolateQuadratic(ipa::Pwl::Point const &a,
> > >                                     ipa::Pwl::Point const &b,
> > >                                     ipa::Pwl::Point const &c) const;
> > > @@ -50,8 +51,6 @@ private:
> > >
> > >         double transversePos_;
> > >         double transverseNeg_;
> > > -
> > > -       ModeConfig *currentMode_ = nullptr;
> > >  };
> > >
> > >  } /* namespace ipa */
> > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> > > index b14b096810ae..2e31492bda3c 100644
> > > --- a/src/ipa/libipa/awb_grey.cpp
> > > +++ b/src/ipa/libipa/awb_grey.cpp
> > > @@ -60,6 +60,7 @@ int AwbGrey::init(const ValueNode &tuningData)
> > >   * \brief Calculate AWB data from the given statistics
> > >   * \param[in] stats The statistics to use for the calculation
> > >   * \param[in] lux The lux value of the scene
> > > + * \param[in] ranges The colour temperature search limits (AwbBayes only)
> >
> > Maybe invert this, to say "range support is not implemented in this
> > algorithm"? Then we could drop the reference to AwbBayes.
> 
> Ok
> 
> >
> > >   *
> > >   * The colour temperature is estimated based on the colours::estimateCCT()
> > >   * function. The gains are calculated purely based on the RGB means provided by
> > > @@ -70,20 +71,23 @@ int AwbGrey::init(const ValueNode &tuningData)
> > >   *
> > >   * \return The AWB result
> > >   */
> > > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux)
> > > +awb::Context
> > > +AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux,
> > > +                     [[maybe_unused]] std::array<double, 2> ranges)
> > >  {
> > > -       AwbResult result;
> > > +       awb::Context result;
> > >         auto means = stats.rgbMeans();
> > > -       result.colourTemperature = estimateCCT(means);
> > > +       result.temperatureK = estimateCCT(means);
> > >
> > >         /*
> > > -        * Estimate the red and blue gains to apply in a grey world. The green
> > > -        * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the
> > > -        * divisor to a minimum value of 1.0.
> > > +        * Calculate the red and blue gains to apply in a grey world by simply
> > > +        * inverting the red/green and blue/green ratios as reported in
> > > +        * statistics. The green gain is hardcoded to 1.0.
> > >          */
> > > -       result.gains.r() = means.g() / std::max(means.r(), 1.0);
> > > +       result.gains.r() = 1.0 / stats.rgRatio();
> > >         result.gains.g() = 1.0;
> > > -       result.gains.b() = means.g() / std::max(means.b(), 1.0);
> > > +       result.gains.b() = 1.0 / stats.bgRatio();
> > > +
> > >         return result;
> > >  }
> > >
> > > @@ -96,12 +100,13 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned
> > >   * gains configured in the colour temperature curve.
> > >   *
> > >   * \return The colour gains if a colour temperature curve is available,
> > > - * [1, 1, 1] otherwise.
> > > + * std::nullopt otherwise
> > >   */
> > >  std::optional<RGB<double>> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> > >  {
> > >         if (!colourGainCurve_) {
> > > -               LOG(Awb, Error) << "No gains defined";
> > > +               LOG(Awb, Info) << "No gains curve defined, "
> > > +                              << "unable to interpolate gains to colour temperature";
> > >                 return std::nullopt;
> > >         }
> > >
> > > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> > > index 154a2af97f15..438fb850c223 100644
> > > --- a/src/ipa/libipa/awb_grey.h
> > > +++ b/src/ipa/libipa/awb_grey.h
> > > @@ -18,13 +18,14 @@ namespace libcamera {
> > >
> > >  namespace ipa {
> > >
> > > -class AwbGrey : public AwbAlgorithm
> > > +class AwbGrey : public AwbImplementation
> > >  {
> > >  public:
> > >         AwbGrey() = default;
> > >
> > >         int init(const ValueNode &tuningData) override;
> > > -       AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;
> > > +       awb::Context calculateAwb(const AwbStats &stats, unsigned int lux,
> > > +                                 std::array<double, 2> ranges) override;
> > >         std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) override;
> > >
> > >  private:
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > > index 0eb05c2b6e1a..0ad0794b4a0f 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > > @@ -8,17 +8,12 @@
> > >  #include "awb.h"
> > >
> > >  #include <algorithm>
> > > -#include <ios>
> > >
> > >  #include <libcamera/base/log.h>
> > >
> > > -#include <libcamera/control_ids.h>
> > > -
> > >  #include <libcamera/ipa/core_ipa_interface.h>
> > >
> > > -#include "libipa/awb_bayes.h"
> > > -#include "libipa/awb_grey.h"
> > > -#include "libipa/colours.h"
> > > +#include "libcamera/internal/vector.h"
> > >
> > >  /**
> > >   * \file awb.h
> > > @@ -28,54 +23,29 @@ namespace libcamera {
> > >
> > >  namespace ipa::rkisp1::algorithms {
> > >
> > > -/**
> > > - * \class Awb
> > > - * \brief Manage the white balance with automatic and manual controls
> > > - */
> > > -
> > >  LOG_DEFINE_CATEGORY(RkISP1Awb)
> > >
> > > -constexpr int32_t kMinColourTemperature = 2500;
> > > -constexpr int32_t kMaxColourTemperature = 10000;
> > > -constexpr int32_t kDefaultColourTemperature = 5000;
> > > -
> > > -/* Minimum mean value below which AWB can't operate. */
> > > -constexpr double kMeanMinThreshold = 2.0;
> > > -
> > >  class RkISP1AwbStats final : public AwbStats
> > >  {
> > >  public:
> > > -       RkISP1AwbStats(const RGB<double> &rgbMeans)
> > > -               : rgbMeans_(rgbMeans)
> > > +       RkISP1AwbStats() = default;
> > > +       RkISP1AwbStats(const RGB<double> means)
> > > +               : AwbStats(means)
> > >         {
> > > -               rg_ = rgbMeans_.r() / rgbMeans_.g();
> > > -               bg_ = rgbMeans_.b() / rgbMeans_.g();
> > >         }
> > >
> > > -       double computeColourError(const RGB<double> &gains) const override
> > > +       /* Minimum mean value below which AWB can't operate. */
> > > +       double minColourValue() const override
> > >         {
> > > -               /*
> > > -                * Compute the sum of the squared colour error (non-greyness) as
> > > -                * it appears in the log likelihood equation.
> > > -                */
> > > -               double deltaR = gains.r() * rg_ - 1.0;
> > > -               double deltaB = gains.b() * bg_ - 1.0;
> > > -               double delta2 = deltaR * deltaR + deltaB * deltaB;
> > > -
> > > -               return delta2;
> > > +               return 2.0;
> > >         }
> > > -
> > > -       RGB<double> rgbMeans() const override
> > > -       {
> > > -               return rgbMeans_;
> > > -       }
> > > -
> > > -private:
> > > -       RGB<double> rgbMeans_;
> > > -       double rg_;
> > > -       double bg_;
> > >  };
> > >
> > > +/**
> > > + * \class Awb
> > > + * \brief Manage the white balance with automatic and manual controls
> > > + */
> > > +
> > >  Awb::Awb()
> > >         : rgbMode_(false)
> > >  {
> > > @@ -86,40 +56,7 @@ Awb::Awb()
> > >   */
> > >  int Awb::init(IPAContext &context, const ValueNode &tuningData)
> > >  {
> > > -       auto &cmap = context.ctrlMap;
> > > -       cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,
> > > -                                                        kMaxColourTemperature,
> > > -                                                        kDefaultColourTemperature);
> > > -       cmap[&controls::AwbEnable] = ControlInfo(false, true);
> > > -
> > > -       cmap[&controls::ColourGains] = ControlInfo(0.0f, 3.996f,
> > > -                                                  Span<const float, 2>{ { 1.0f, 1.0f } });
> > > -
> > > -       if (!tuningData.contains("algorithm"))
> > > -               LOG(RkISP1Awb, Info) << "No AWB algorithm specified."
> > > -                                    << " Default to grey world";
> > > -
> > > -       auto mode = tuningData["algorithm"].get<std::string>("grey");
> > > -       if (mode == "grey") {
> > > -               awbAlgo_ = std::make_unique<AwbGrey>();
> > > -       } else if (mode == "bayes") {
> > > -               awbAlgo_ = std::make_unique<AwbBayes>();
> > > -       } else {
> > > -               LOG(RkISP1Awb, Error) << "Unknown AWB algorithm: " << mode;
> > > -               return -EINVAL;
> > > -       }
> > > -       LOG(RkISP1Awb, Debug) << "Using AWB algorithm: " << mode;
> > > -
> > > -       int ret = awbAlgo_->init(tuningData);
> > > -       if (ret) {
> > > -               LOG(RkISP1Awb, Error) << "Failed to init AWB algorithm";
> > > -               return ret;
> > > -       }
> > > -
> > > -       const auto &src = awbAlgo_->controls();
> > > -       cmap.insert(src.begin(), src.end());
> > > -
> > > -       return 0;
> > > +       return awbAlgo_.init(tuningData, context.ctrlMap);
> > >  }
> > >
> > >  /**
> > > @@ -128,16 +65,7 @@ int Awb::init(IPAContext &context, const ValueNode &tuningData)
> > >  int Awb::configure(IPAContext &context,
> > >                    const IPACameraSensorInfo &configInfo)
> > >  {
> > > -       context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> > > -       auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> > > -       if (gains)
> > > -               context.activeState.awb.automatic.gains = *gains;
> > > -       else
> > > -               context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
> > > -
> > > -       context.activeState.awb.autoEnabled = true;
> > > -       context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> > > -       context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> > > +       awbAlgo_.configure(context.activeState.awb);
> > >
> > >         /*
> > >          * Define the measurement window for AWB as a centered rectangle
> > > @@ -156,56 +84,12 @@ int Awb::configure(IPAContext &context,
> > >  /**
> > >   * \copydoc libcamera::ipa::Algorithm::queueRequest
> > >   */
> > > -void Awb::queueRequest(IPAContext &context,
> > > -                      [[maybe_unused]] const uint32_t frame,
> > > +void Awb::queueRequest(IPAContext &context, const uint32_t frame,
> > >                        IPAFrameContext &frameContext,
> > >                        const ControlList &controls)
> > >  {
> > > -       auto &awb = context.activeState.awb;
> > > -
> > > -       const auto &awbEnable = controls.get(controls::AwbEnable);
> > > -       if (awbEnable && *awbEnable != awb.autoEnabled) {
> > > -               awb.autoEnabled = *awbEnable;
> > > -
> > > -               LOG(RkISP1Awb, Debug)
> > > -                       << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> > > -       }
> > > -
> > > -       awbAlgo_->handleControls(controls);
> > > -
> > > -       frameContext.awb.autoEnabled = awb.autoEnabled;
> > > -
> > > -       if (awb.autoEnabled)
> > > -               return;
> > > -
> > > -       const auto &colourGains = controls.get(controls::ColourGains);
> > > -       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > > -       bool update = false;
> > > -       if (colourGains) {
> > > -               awb.manual.gains.r() = (*colourGains)[0];
> > > -               awb.manual.gains.b() = (*colourGains)[1];
> > > -               /*
> > > -                * \todo Colour temperature reported in metadata is now
> > > -                * incorrect, as we can't deduce the temperature from the gains.
> > > -                * This will be fixed with the bayes AWB algorithm.
> > > -                */
> > > -               update = true;
> > > -       } else if (colourTemperature) {
> > > -               awb.manual.temperatureK = *colourTemperature;
> > > -               const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> > > -               if (gains) {
> > > -                       awb.manual.gains.r() = gains->r();
> > > -                       awb.manual.gains.b() = gains->b();
> > > -                       update = true;
> > > -               }
> > > -       }
> > > -
> > > -       if (update)
> > > -               LOG(RkISP1Awb, Debug)
> > > -                       << "Set colour gains to " << awb.manual.gains;
> > > -
> > > -       frameContext.awb.gains = awb.manual.gains;
> > > -       frameContext.awb.temperatureK = awb.manual.temperatureK;
> > > +       awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,
> > > +                             controls);
> > >  }
> > >
> > >  /**
> > > @@ -214,15 +98,7 @@ void Awb::queueRequest(IPAContext &context,
> > >  void Awb::prepare(IPAContext &context, const uint32_t frame,
> > >                   IPAFrameContext &frameContext, RkISP1Params *params)
> > >  {
> > > -       /*
> > > -        * This is the latest time we can read the active state. This is the
> > > -        * most up-to-date automatic values we can read.
> > > -        */
> > > -       if (frameContext.awb.autoEnabled) {
> > > -               const auto &awb = context.activeState.awb;
> > > -               frameContext.awb.gains = awb.automatic.gains;
> > > -               frameContext.awb.temperatureK = awb.automatic.temperatureK;
> > > -       }
> > > +       awbAlgo_.prepare(context.activeState.awb, frameContext.awb);
> > >
> > >         auto gainConfig = params->block<BlockType::AwbGain>();
> > >         gainConfig.setEnabled(true);
> > > @@ -291,68 +167,27 @@ void Awb::process(IPAContext &context,
> > >                   const rkisp1_stat_buffer *stats,
> > >                   ControlList &metadata)
> > >  {
> > > -       IPAActiveState &activeState = context.activeState;
> > > +       RkISP1AwbStats awbStats = calculateRgbMeans(frameContext, stats);
> >
> > Uh not too easy to see what's going on here. Maybe we should have done a
> > bit of preparatory refactoring on rkisp1 first.
> >
> 
> What refactoring would you have done on rkisp1 ?

Yes, I see that it's difficult. But then there are also a few conceptual
changes that could be done seperately from the moves.

> 
> I should have made rkisp1 implementation as similar as possible to
> AwbAlgorithm and then moved it to libipa, but I don't think I would be
> able to refactor rkisp1 completely mostly because the AwbAlgorithm,
> AwbBayes and AwbGrey class already existed/
> 
> I suggest you look at the end result in the rkisp1 awb algorithm
> instead of the diff ?

I did in the end.

> 
> > This partially defeats the initial intention of AwbStats. I think it
> 
> How so ?
> 
> > would be clearer if we do
> >
> > RkISP1AwbStats awbStats(frameContext, stats);
> >
> > and move the code from calculateRgbMeans into RkISP1AwbStats. This would
> > make it clearer that AwbStats should *not* be a single rgb means triple.
> 
> This isn't generic code, so I don't see any big difference having it
> in an helper function or having AwbStats call that same function in
> the constructor.
> 
> > That is unfortunately the only thing the rkisp1 can provide, but it
> > shouldn't be the usual implementation (especially the
> > computeColourError() only makes real sense if there is more than one
> > meaurement.).
> 
> I don't see where the confusion could come from to be honest. This is
> IPA code, which knows how stats are produced
> 
> Maybe I'm missing something

The idea was that AwbStats is an adapter from the hardware stats to the
data the algorithm needs. Ideally I would have dropped rgbMeans
altogether and only provide the computeError function. But that was
impossible because of grey world. So the implementer should ask himself
"How can I implement computeColourError() for my hardware" instead of
"How can I calculate rgbMenas from my hardware". And I think that doesn't
happen if we provide the AwbStatsSimple implementation that is just
passed the rgb means triplet.

> 
> >
> > >
> > > -       metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
> > > -       metadata.set(controls::ColourGains, {
> > > -                       static_cast<float>(frameContext.awb.gains.r()),
> > > -                       static_cast<float>(frameContext.awb.gains.b())
> > > -               });
> > > -       metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
> > > +       awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,
> > > +                        frameContext.lux.lux, metadata);
> > > +}
> > >
> > > +RkISP1AwbStats Awb::calculateRgbMeans(const IPAFrameContext &frameContext,
> > > +                                     const rkisp1_stat_buffer *stats) const
> > > +{
> > >         if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {
> > >                 LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
> > > -               return;
> > > +               return {};
> > >         }
> > >
> > > -       const rkisp1_cif_isp_stat *params = &stats->params;
> > > -       const rkisp1_cif_isp_awb_stat *awb = &params->awb;
> > > +       const rkisp1_cif_isp_awb_stat *awb = &stats->params.awb;
> > >
> > >         if (awb->awb_mean[0].cnt == 0) {
> > >                 LOG(RkISP1Awb, Debug) << "AWB statistics are empty";
> > > -               return;
> > > +               return {};
> > >         }
> > >
> > > -       RGB<double> rgbMeans = calculateRgbMeans(frameContext, awb);
> > > -
> > > -       /*
> > > -        * If the means are too small we don't have enough information to
> > > -        * meaningfully calculate gains. Freeze the algorithm in that case.
> > > -        */
> > > -       if (rgbMeans.r() < kMeanMinThreshold && rgbMeans.g() < kMeanMinThreshold &&
> > > -           rgbMeans.b() < kMeanMinThreshold)
> > > -               return;
> > > -
> > > -       RkISP1AwbStats awbStats{ rgbMeans };
> > > -       AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);
> > > -
> > > -       /*
> > > -        * Clamp the gain values to the hardware, which expresses gains as Q2.8
> > > -        * unsigned integer values. Set the minimum just above zero to avoid
> > > -        * divisions by zero when computing the raw means in subsequent
> > > -        * iterations.
> > > -        */
> > > -       awbResult.gains = awbResult.gains.clamp(1.0 / 256, 1023.0 / 256);
> > > -
> > > -       /* Filter the values to avoid oscillations. */
> > > -       double speed = 0.2;
> > > -       double ct = awbResult.colourTemperature;
> > > -       ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);
> > > -       awbResult.gains = awbResult.gains * speed +
> > > -                         activeState.awb.automatic.gains * (1 - speed);
> > > -
> > > -       activeState.awb.automatic.temperatureK = static_cast<unsigned int>(ct);
> > > -       activeState.awb.automatic.gains = awbResult.gains;
> > > -
> > > -       LOG(RkISP1Awb, Debug)
> > > -               << std::showpoint
> > > -               << "Means " << rgbMeans << ", gains "
> > > -               << activeState.awb.automatic.gains << ", temp "
> > > -               << activeState.awb.automatic.temperatureK << "K";
> > > -}
> > > -
> > > -RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const
> > > -{
> > >         Vector<double, 3> rgbMeans;
> > >
> > >         if (rgbMode_) {
> > > @@ -419,7 +254,7 @@ RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rk
> > >          */
> > >         rgbMeans /= frameContext.awb.gains.max(0.01);
> > >
> > > -       return rgbMeans;
> > > +       return RkISP1AwbStats(rgbMeans);
> >
> > Reading this change I realize I should have moved this function in
> > RkISP1AwbStats in first place.
> >
> 
> Is this comment related to the above suggestion of having
> 
>  RkISP1AwbStats awbStats(frameContext, stats);

Yes

Best regards,
Stefan

> 
> ?
> 
> > Best regards,
> > Stefan
> >
> > >  }
> > >
> > >  REGISTER_IPA_ALGORITHM(Awb, "Awb")
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
> > > index 60d9ef111495..c7cb59a75bde 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.h
> > > +++ b/src/ipa/rkisp1/algorithms/awb.h
> > > @@ -7,17 +7,25 @@
> > >
> > >  #pragma once
> > >
> > > -#include "libcamera/internal/vector.h"
> > > +#include <linux/rkisp1-config.h>
> > > +
> > > +#include <libcamera/controls.h>
> > > +
> > > +#include "libcamera/internal/value_node.h"
> > >
> > >  #include "libipa/awb.h"
> > > -#include "libipa/interpolator.h"
> > > +#include "libipa/fixedpoint.h"
> > >
> > >  #include "algorithm.h"
> > > +#include "ipa_context.h"
> > > +#include "params.h"
> > >
> > >  namespace libcamera {
> > >
> > >  namespace ipa::rkisp1::algorithms {
> > >
> > > +class RkISP1AwbStats;
> > > +
> > >  class Awb : public Algorithm
> > >  {
> > >  public:
> > > @@ -38,10 +46,10 @@ public:
> > >                      ControlList &metadata) override;
> > >
> > >  private:
> > > -       RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext,
> > > -                                     const rkisp1_cif_isp_awb_stat *awb) const;
> > > +       RkISP1AwbStats calculateRgbMeans(const IPAFrameContext &frameContext,
> > > +                                        const rkisp1_stat_buffer *stats) const;
> > >
> > > -       std::unique_ptr<AwbAlgorithm> awbAlgo_;
> > > +       AwbAlgorithm<UQ<2, 8>> awbAlgo_;
> > >
> > >         bool rgbMode_;
> > >  };
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index e5f70d934d67..c36c1f7e0084 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -25,6 +25,7 @@
> > >  #include "libcamera/internal/vector.h"
> > >
> > >  #include "libipa/agc_mean_luminance.h"
> > > +#include "libipa/awb.h"
> > >  #include "libipa/camera_sensor_helper.h"
> > >  #include "libipa/fc_queue.h"
> > >  #include "libipa/fixedpoint.h"
> > > @@ -48,15 +49,17 @@ struct IPAHwSettings {
> > >         bool compand;
> > >  };
> > >
> > > +struct RKISP1AwbSession {
> > > +       struct rkisp1_cif_isp_window measureWindow;
> > > +       bool enabled;
> > > +};
> > > +
> > >  struct IPASessionConfiguration {
> > >         struct {
> > >                 struct rkisp1_cif_isp_window measureWindow;
> > >         } agc;
> > >
> > > -       struct {
> > > -               struct rkisp1_cif_isp_window measureWindow;
> > > -               bool enabled;
> > > -       } awb;
> > > +       struct RKISP1AwbSession awb;
> > >
> > >         struct {
> > >                 bool supported;
> > > @@ -100,17 +103,7 @@ struct IPAActiveState {
> > >                 utils::Duration maxFrameDuration;
> > >         } agc;
> > >
> > > -       struct {
> > > -               struct AwbState {
> > > -                       RGB<double> gains;
> > > -                       unsigned int temperatureK;
> > > -               };
> > > -
> > > -               AwbState manual;
> > > -               AwbState automatic;
> > > -
> > > -               bool autoEnabled;
> > > -       } awb;
> > > +       ipa::awb::ActiveState awb;
> > >
> > >         struct {
> > >                 Matrix<float, 3, 3> manual;
> > > @@ -176,11 +169,7 @@ struct IPAFrameContext : public FrameContext {
> > >                 bool autoGainModeChange;
> > >         } agc;
> > >
> > > -       struct {
> > > -               RGB<double> gains;
> > > -               bool autoEnabled;
> > > -               unsigned int temperatureK;
> > > -       } awb;
> > > +       ipa::awb::FrameContext awb;
> > >
> > >         struct {
> > >                 float actualBrightness;
> > >
> > > --
> > > 2.54.0
> > >