[PATCH v2 03/17] libipa: Add AWB algorithm base class

[Laurent Pinchart]

Hi Milan,

On Mon, Feb 24, 2025 at 08:06:23PM +0100, Milan Zamazal wrote:
> Stefan Klug writes:
> 
> > Add a class to provide a generic interface for auto white balance
> > algorithms. Concrete AWB algorithms are expected to subclass the
> > AwbAlgorithm class to implement their functionality.
> >
> > IPAs are expected to subclass the AwbStats class and implement the
> > necessary functions to give the algorithm access to the hardware
> > specific statistics data.
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>
> >
> > ---
> >
> > Changes in v2:
> > - Fixed language errors in the documentation and commit message
> > ---
> >  src/ipa/libipa/awb.cpp     | 137 +++++++++++++++++++++++++++++++++++++
> >  src/ipa/libipa/awb.h       |  51 ++++++++++++++
> >  src/ipa/libipa/meson.build |   2 +
> >  3 files changed, 190 insertions(+)
> >  create mode 100644 src/ipa/libipa/awb.cpp
> >  create mode 100644 src/ipa/libipa/awb.h
> >
> > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> > new file mode 100644
> > index 000000000000..4db97f6a10fc
> > --- /dev/null
> > +++ b/src/ipa/libipa/awb.cpp
> > @@ -0,0 +1,137 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024 Ideas on Board Oy
> > + *
> > + * Generic AWB algorithms
> > + */
> > +
> > +#include "awb.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +/**
> > + * \file awb.h
> > + * \brief Base classes for AWB algorithms
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Awb)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \class AwbResult
> > + * \brief The result of an awb calculation
> > + *
> > + * This class holds the result of an auto white balance calculation.
> > + */
> > +
> > +/**
> > + * \var AwbResult::gains
> > + * \brief The calculated white balance gains
> > + */
> > +
> > +/**
> > + * \var AwbResult::colourTemperature
> > + * \brief The calculated colour temperature in Kelvin
> > + */
> > +
> > +/**
> > + * \class AwbStats
> > + * \brief An abstraction class wrapping hardware-specific AWB statistics
> > + *
> > + * Pipeline handlers 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.
> > + */
> > +
> > +/**
> > + * \fn AwbStats::computeColourError
> > + * \brief Compute an error value for when the given gains would be applied
> > + * \param[in] gains The gains to apply
> > + *
> > + * Compute an error value (non-greyness) assuming the given \a gains would be
> > + * applied. To keep the actual implementations computationally inexpensive,
> > + * the squared colour error shall be returned.
> > + *
> > + * If the awb statistics provide multiple zones, the sum over all zones needs to
> > + * calculated.
> > + *
> > + * \return The computed error value
> > + */
> > +
> > +/**
> > + * \fn AwbStats::getRGBMeans
> > + * \brief Get RGB means of the statistics
> > + *
> > + * 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 gray world model to calculate the gains to apply.
> > + *
> > + * \return The RGB means
> > + */
> > +
> > +/**
> > + * \class AwbAlgorithm
> > + * \brief A base class for auto white balance algorithms
> > + *
> > + * 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.
> > + */
> > +
> > +/**
> > + * \fn AwbAlgorithm::init
> > + * \brief Initialize the algorithm with the given tuning data
> > + * \param[in] tuningData The tuning data to use for the algorithm
> > + *
> > + * \return 0 on success, a negative error code otherwise
> > + */
> > +
> > +/**
> > + * \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
> > + *
> > + * 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.
> > + *
> > + * \return The awb result
> > + */
> > +
> > +/**
> > + * \fn AwbAlgorithm::gainsFromColourTemperature
> > + * \brief Compute white balance gains from a colour temperature
> > + * \param[in] colourTemperature The colour temperature in Kelvin
> > + *
> > + * 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.
> > + *
> > + * \return The colour gains
> > + */
> > +
> > +/**
> > + * \fn AwbAlgorithm::controls
> > + * \brief Get the controls info map for this algorithm
> > + *
> > + * \return The controls info map
> > + */
> > +
> > +/**
> > + * \fn AwbAlgorithm::handleControls
> > + * \param[in] controls The controls to handle
> > + * \brief Handle the controls supplied in a request
> > + */
> > +
> > +/**
> > + * \var AwbAlgorithm::controls_
> > + * \brief Controls info map for the controls provided by the algorithm
> > + */
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> > new file mode 100644
> > index 000000000000..2dd471606ec4
> > --- /dev/null
> > +++ b/src/ipa/libipa/awb.h
> > @@ -0,0 +1,51 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024 Ideas on Board Oy
> > + *
> > + * Generic AWB algorithms
> > + */
> > +
> > +#pragma once
> > +
> > +#include <libcamera/controls.h>
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "vector.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +struct AwbResult {
> > +	RGB<double> gains;
> > +	double colourTemperature;
> > +};
> > +
> > +struct AwbStats {
> > +	virtual double computeColourError(const RGB<double> &gains) const = 0;
> > +	virtual RGB<double> getRGBMeans() const = 0;
> > +};
> 
> This causes a build failure with gcc 11.5.0 for me:
> 
>   In file included from ../src/ipa/libipa/awb_grey.h:13,
>                    from ../src/ipa/libipa/awb_grey.cpp:8:
>   ../src/ipa/libipa/awb.h:27:8: error: 'struct libcamera::ipa::AwbStats' has virtual functions and accessible non-virtual destructor [-Werror=non-virtual-dtor]
>      27 | struct AwbStats {
>         |        ^~~~~~~~
>   cc1plus: all warnings being treated as errors

I wondered why my gcc 11.5.0 doesn't warn about this. Adding
-Wnon-virtual-dtor triggered the warning. I'll send a patch to enable
it.

> gcc 14 doesn't suffer from this problem.  Adding
> 
>   virtual ~AwbStats() = default;
> 
> fixes the build with gcc 11.5.0.

I think declaring a protected non-virtual descriptor would be better, as
we never need to destroy instances from a base class pointer. I'll send
a patch as well.

> > +class AwbAlgorithm
> > +{
> > +public:
> > +	virtual ~AwbAlgorithm() = default;
> > +
> > +	virtual int init(const YamlObject &tuningData) = 0;
> > +	virtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0;
> > +	virtual RGB<double> gainsFromColourTemperature(double colourTemperature) = 0;
> > +
> > +	const ControlInfoMap::Map &controls() const
> > +	{
> > +		return controls_;
> > +	}
> > +
> > +	virtual void handleControls([[maybe_unused]] const ControlList &controls) {}
> > +
> > +protected:
> > +	ControlInfoMap::Map controls_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index f2b2f4be50db..03e879c5834f 100644
> > --- a/src/ipa/libipa/meson.build
> > +++ b/src/ipa/libipa/meson.build
> > @@ -3,6 +3,7 @@
> >  libipa_headers = files([
> >      'agc_mean_luminance.h',
> >      'algorithm.h',
> > +    'awb.h',
> >      'camera_sensor_helper.h',
> >      'colours.h',
> >      'exposure_mode_helper.h',
> > @@ -20,6 +21,7 @@ libipa_headers = files([
> >  libipa_sources = files([
> >      'agc_mean_luminance.cpp',
> >      'algorithm.cpp',
> > +    'awb.cpp',
> >      'camera_sensor_helper.cpp',
> >      'colours.cpp',
> >      'exposure_mode_helper.cpp',

-- 
Regards,

Laurent Pinchart