[v1,05/11] libipa: Add grey world AWB algorithm

22492 diff mbox series

Add the grey world algorithm that is currently used in rkisp1 to libipa.
No changes in functionality were made.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/libipa/awb_grey.cpp | 114 ++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/awb_grey.h   |  35 +++++++++++
 src/ipa/libipa/meson.build  |   2 +
 3 files changed, 151 insertions(+)
 create mode 100644 src/ipa/libipa/awb_grey.cpp
 create mode 100644 src/ipa/libipa/awb_grey.h

On Thu, Jan 09, 2025 at 12:53:56PM +0100, Stefan Klug wrote:
> Add the grey world algorithm that is currently used in rkisp1 to libipa.
> No changes in functionality were made.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/libipa/awb_grey.cpp | 114 ++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/awb_grey.h   |  35 +++++++++++
>  src/ipa/libipa/meson.build  |   2 +
>  3 files changed, 151 insertions(+)
>  create mode 100644 src/ipa/libipa/awb_grey.cpp
>  create mode 100644 src/ipa/libipa/awb_grey.h
> 
> diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> new file mode 100644
> index 000000000000..192a7cf3834a
> --- /dev/null
> +++ b/src/ipa/libipa/awb_grey.cpp
> @@ -0,0 +1,114 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Ideas on Board Oy
> + *
> + * Base class for bayesian AWB algorithm
> + */
> +
> +#include "awb_grey.h"
> +
> +#include <cmath>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/control_ids.h>
> +
> +#include "colours.h"
> +
> +using namespace libcamera::controls;
> +
> +/**
> + * \file awb_grey.h
> + * \brief Implementation of a grey world AWB algorithm
> + */
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Awb)
> +namespace ipa {
> +
> +/**
> + * \class AwbGrey
> + * \brief A Grey world auto white balance algorithm
> + */
> +
> +/**
> + * \brief Initialize the algorithm with the given tuning data
> + * \param[in] tuningData The tuning data for the algorithm
> + *
> + * Load the colour temperature curve from the tuning data. If there is no tuning
> + * data available, continue with a warning. Manual colour temperature will not
> + * work in that case.
> + *
> + * \return 0 on success, a negative error code otherwise
> + */
> +int AwbGrey::init(const YamlObject &tuningData)
> +{
> +	Interpolator<Vector<double, 2>> gains;
> +	int ret = gains.readYaml(tuningData["colourGains"], "ct", "gains");
> +	if (ret < 0)
> +		LOG(Awb, Warning)
> +			<< "Failed to parse 'colourGains' "
> +			<< "parameter from tuning file; "
> +			<< "manual colour temperature will not work properly";
> +	else
> +		colourGainCurve_ = gains;
> +
> +	return 0;
> +}
> +
> +/**
> + * \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
> + *
> + * Estimates the colour temperature based on the coulours::estimateCCT function.

s/coulours/colors/ -- wait no -- s/coulours/colours/

> + * The gains are calculated purely based on the RGB means provided by the \a
> + * stats. The colour temperature is not taken into account when calculating the
> + * gains.
> + *
> + * The \a lux parameter is not used in this algorithm.
> + *
> + * \return The awb result
> + */
> +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)
> +{
> +	AwbResult result;
> +	auto means = stats.getRGBMeans();
> +	result.colourTemperature = 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.
> +	 */
> +	result.gains.r() = means.g() / std::max(means.r(), 1.0);
> +	result.gains.g() = 1.0;
> +	result.gains.b() = means.g() / std::max(means.b(), 1.0);
> +	return result;
> +}
> +
> +/**
> + * \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 simply interpolates the colour
> + * gains configured in the colour temperature curve.
> + *
> + * \return The colour gains if a colour temperature curve is available,
> + * [1, 1, 1] otherwise.
> + */
> +RGB<double> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> +{
> +	if (!colourGainCurve_) {
> +		LOG(Awb, Error) << "No gains defined";
> +		return RGB<double>({ 1.0, 1.0, 1.0 });
> +	}
> +
> +	auto gains = colourGainCurve_->getInterpolated(colourTemperature);
> +	return { { gains[0], 1.0, gains[0] } };

Should the second gains[0] be gains[1]...?

Otherwise looks good to me.


Paul


> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> new file mode 100644
> index 000000000000..6eda8e5498fb
> --- /dev/null
> +++ b/src/ipa/libipa/awb_grey.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Ideas on Board Oy
> + *
> + * AWB grey world algorithm
> + */
> +
> +#pragma once
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "awb.h"
> +#include "interpolator.h"
> +#include "vector.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class AwbGrey : public AwbAlgorithm
> +{
> +public:
> +	AwbGrey() = default;
> +
> +	int init(const YamlObject &tuningData) override;
> +	AwbResult calculateAwb(const AwbStats &stats, int lux) override;
> +	RGB<double> gainsFromColourTemperature(double coulourTemperature) override;
> +
> +private:
> +	std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 03e879c5834f..c550a6eb45b6 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_grey.h',
>      'awb.h',
>      'camera_sensor_helper.h',
>      'colours.h',
> @@ -21,6 +22,7 @@ libipa_headers = files([
>  libipa_sources = files([
>      'agc_mean_luminance.cpp',
>      'algorithm.cpp',
> +    'awb_grey.cpp',
>      'awb.cpp',
>      'camera_sensor_helper.cpp',
>      'colours.cpp',
> -- 
> 2.43.0
>

Hi Paul,

Thank you for the review. 

On Mon, Jan 13, 2025 at 04:49:08PM -0600, Paul Elder wrote:
> On Thu, Jan 09, 2025 at 12:53:56PM +0100, Stefan Klug wrote:
> > Add the grey world algorithm that is currently used in rkisp1 to libipa.
> > No changes in functionality were made.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/libipa/awb_grey.cpp | 114 ++++++++++++++++++++++++++++++++++++
> >  src/ipa/libipa/awb_grey.h   |  35 +++++++++++
> >  src/ipa/libipa/meson.build  |   2 +
> >  3 files changed, 151 insertions(+)
> >  create mode 100644 src/ipa/libipa/awb_grey.cpp
> >  create mode 100644 src/ipa/libipa/awb_grey.h
> > 
> > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> > new file mode 100644
> > index 000000000000..192a7cf3834a
> > --- /dev/null
> > +++ b/src/ipa/libipa/awb_grey.cpp
> > @@ -0,0 +1,114 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024 Ideas on Board Oy
> > + *
> > + * Base class for bayesian AWB algorithm
> > + */
> > +
> > +#include "awb_grey.h"
> > +
> > +#include <cmath>
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/control_ids.h>
> > +
> > +#include "colours.h"
> > +
> > +using namespace libcamera::controls;
> > +
> > +/**
> > + * \file awb_grey.h
> > + * \brief Implementation of a grey world AWB algorithm
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(Awb)
> > +namespace ipa {
> > +
> > +/**
> > + * \class AwbGrey
> > + * \brief A Grey world auto white balance algorithm
> > + */
> > +
> > +/**
> > + * \brief Initialize the algorithm with the given tuning data
> > + * \param[in] tuningData The tuning data for the algorithm
> > + *
> > + * Load the colour temperature curve from the tuning data. If there is no tuning
> > + * data available, continue with a warning. Manual colour temperature will not
> > + * work in that case.
> > + *
> > + * \return 0 on success, a negative error code otherwise
> > + */
> > +int AwbGrey::init(const YamlObject &tuningData)
> > +{
> > +	Interpolator<Vector<double, 2>> gains;
> > +	int ret = gains.readYaml(tuningData["colourGains"], "ct", "gains");
> > +	if (ret < 0)
> > +		LOG(Awb, Warning)
> > +			<< "Failed to parse 'colourGains' "
> > +			<< "parameter from tuning file; "
> > +			<< "manual colour temperature will not work properly";
> > +	else
> > +		colourGainCurve_ = gains;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \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
> > + *
> > + * Estimates the colour temperature based on the coulours::estimateCCT function.
> 
> s/coulours/colors/ -- wait no -- s/coulours/colours/
> 
> > + * The gains are calculated purely based on the RGB means provided by the \a
> > + * stats. The colour temperature is not taken into account when calculating the
> > + * gains.
> > + *
> > + * The \a lux parameter is not used in this algorithm.
> > + *
> > + * \return The awb result
> > + */
> > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)
> > +{
> > +	AwbResult result;
> > +	auto means = stats.getRGBMeans();
> > +	result.colourTemperature = 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.
> > +	 */
> > +	result.gains.r() = means.g() / std::max(means.r(), 1.0);
> > +	result.gains.g() = 1.0;
> > +	result.gains.b() = means.g() / std::max(means.b(), 1.0);
> > +	return result;
> > +}
> > +
> > +/**
> > + * \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 simply interpolates the colour
> > + * gains configured in the colour temperature curve.
> > + *
> > + * \return The colour gains if a colour temperature curve is available,
> > + * [1, 1, 1] otherwise.
> > + */
> > +RGB<double> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> > +{
> > +	if (!colourGainCurve_) {
> > +		LOG(Awb, Error) << "No gains defined";
> > +		return RGB<double>({ 1.0, 1.0, 1.0 });
> > +	}
> > +
> > +	auto gains = colourGainCurve_->getInterpolated(colourTemperature);
> > +	return { { gains[0], 1.0, gains[0] } };
> 
> Should the second gains[0] be gains[1]...?

Ouch. How did that slip through...

Thanks for spotting.

Cheers,
Stefan

> 
> Otherwise looks good to me.
> 
> 
> Paul
> 
> 
> > +}
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> > new file mode 100644
> > index 000000000000..6eda8e5498fb
> > --- /dev/null
> > +++ b/src/ipa/libipa/awb_grey.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024 Ideas on Board Oy
> > + *
> > + * AWB grey world algorithm
> > + */
> > +
> > +#pragma once
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "awb.h"
> > +#include "interpolator.h"
> > +#include "vector.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +class AwbGrey : public AwbAlgorithm
> > +{
> > +public:
> > +	AwbGrey() = default;
> > +
> > +	int init(const YamlObject &tuningData) override;
> > +	AwbResult calculateAwb(const AwbStats &stats, int lux) override;
> > +	RGB<double> gainsFromColourTemperature(double coulourTemperature) override;
> > +
> > +private:
> > +	std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 03e879c5834f..c550a6eb45b6 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_grey.h',
> >      'awb.h',
> >      'camera_sensor_helper.h',
> >      'colours.h',
> > @@ -21,6 +22,7 @@ libipa_headers = files([
> >  libipa_sources = files([
> >      'agc_mean_luminance.cpp',
> >      'algorithm.cpp',
> > +    'awb_grey.cpp',
> >      'awb.cpp',
> >      'camera_sensor_helper.cpp',
> >      'colours.cpp',
> > -- 
> > 2.43.0
> >

Hi Stefan

On 09/01/2025 11:53, Stefan Klug wrote:
> Add the grey world algorithm that is currently used in rkisp1 to libipa.
> No changes in functionality were made.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   src/ipa/libipa/awb_grey.cpp | 114 ++++++++++++++++++++++++++++++++++++
>   src/ipa/libipa/awb_grey.h   |  35 +++++++++++
>   src/ipa/libipa/meson.build  |   2 +
>   3 files changed, 151 insertions(+)
>   create mode 100644 src/ipa/libipa/awb_grey.cpp
>   create mode 100644 src/ipa/libipa/awb_grey.h
>
> diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> new file mode 100644
> index 000000000000..192a7cf3834a
> --- /dev/null
> +++ b/src/ipa/libipa/awb_grey.cpp
> @@ -0,0 +1,114 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Ideas on Board Oy
> + *
> + * Base class for bayesian AWB algorithm
> + */
> +
> +#include "awb_grey.h"
> +
> +#include <cmath>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/control_ids.h>
> +
> +#include "colours.h"
> +
> +using namespace libcamera::controls;
> +
> +/**
> + * \file awb_grey.h
> + * \brief Implementation of a grey world AWB algorithm
> + */
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Awb)
Is it worth differentiating between this log category and that in awb.cpp - which is also just 
"Awb"? The logs will include the file path and line number of course so they can be distinguished 
anyway so probably it doesn't matter, but I thought I'd mention it just to bring it to mind - I'm 
happy either way.
> +namespace ipa {
> +
> +/**
> + * \class AwbGrey
> + * \brief A Grey world auto white balance algorithm
> + */
> +
> +/**
> + * \brief Initialize the algorithm with the given tuning data
> + * \param[in] tuningData The tuning data for the algorithm
> + *
> + * Load the colour temperature curve from the tuning data. If there is no tuning
> + * data available, continue with a warning. Manual colour temperature will not
> + * work in that case.
> + *
> + * \return 0 on success, a negative error code otherwise
> + */
> +int AwbGrey::init(const YamlObject &tuningData)
> +{
> +	Interpolator<Vector<double, 2>> gains;
> +	int ret = gains.readYaml(tuningData["colourGains"], "ct", "gains");
> +	if (ret < 0)
> +		LOG(Awb, Warning)
> +			<< "Failed to parse 'colourGains' "
> +			<< "parameter from tuning file; "
> +			<< "manual colour temperature will not work properly";
> +	else
> +		colourGainCurve_ = gains;
> +
> +	return 0;
> +}
> +
> +/**
> + * \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
> + *
> + * Estimates the colour temperature based on the coulours::estimateCCT function.
s/coulours/colours.
> + * The gains are calculated purely based on the RGB means provided by the \a
> + * stats. The colour temperature is not taken into account when calculating the
> + * gains.
> + *
> + * The \a lux parameter is not used in this algorithm.
> + *
> + * \return The awb result
> + */
> +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)
> +{
> +	AwbResult result;
> +	auto means = stats.getRGBMeans();
> +	result.colourTemperature = 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.
> +	 */
> +	result.gains.r() = means.g() / std::max(means.r(), 1.0);
> +	result.gains.g() = 1.0;
> +	result.gains.b() = means.g() / std::max(means.b(), 1.0);
> +	return result;
> +}
> +
> +/**
> + * \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 simply interpolates the colour
> + * gains configured in the colour temperature curve.
> + *
> + * \return The colour gains if a colour temperature curve is available,
> + * [1, 1, 1] otherwise.
> + */
> +RGB<double> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> +{
> +	if (!colourGainCurve_) {
> +		LOG(Awb, Error) << "No gains defined";
> +		return RGB<double>({ 1.0, 1.0, 1.0 });
> +	}
> +
> +	auto gains = colourGainCurve_->getInterpolated(colourTemperature);
> +	return { { gains[0], 1.0, gains[0] } };
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> new file mode 100644
> index 000000000000..6eda8e5498fb
> --- /dev/null
> +++ b/src/ipa/libipa/awb_grey.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Ideas on Board Oy
> + *
> + * AWB grey world algorithm
> + */
> +
> +#pragma once
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "awb.h"
> +#include "interpolator.h"
> +#include "vector.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class AwbGrey : public AwbAlgorithm
> +{
> +public:
> +	AwbGrey() = default;
> +
> +	int init(const YamlObject &tuningData) override;
> +	AwbResult calculateAwb(const AwbStats &stats, int lux) override;
> +	RGB<double> gainsFromColourTemperature(double coulourTemperature) override;

s/coulourTemperature/colourTemperature


Otherwise looks good I think:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> +
> +private:
> +	std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 03e879c5834f..c550a6eb45b6 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_grey.h',
>       'awb.h',
>       'camera_sensor_helper.h',
>       'colours.h',
> @@ -21,6 +22,7 @@ libipa_headers = files([
>   libipa_sources = files([
>       'agc_mean_luminance.cpp',
>       'algorithm.cpp',
> +    'awb_grey.cpp',
>       'awb.cpp',
>       'camera_sensor_helper.cpp',
>       'colours.cpp',

Hi Stefan

On 09/01/2025 11:53, Stefan Klug wrote:
> Add the grey world algorithm that is currently used in rkisp1 to libipa.
> No changes in functionality were made.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   src/ipa/libipa/awb_grey.cpp | 114 ++++++++++++++++++++++++++++++++++++
>   src/ipa/libipa/awb_grey.h   |  35 +++++++++++
>   src/ipa/libipa/meson.build  |   2 +
>   3 files changed, 151 insertions(+)
>   create mode 100644 src/ipa/libipa/awb_grey.cpp
>   create mode 100644 src/ipa/libipa/awb_grey.h
>
> diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> new file mode 100644
> index 000000000000..192a7cf3834a
> --- /dev/null
> +++ b/src/ipa/libipa/awb_grey.cpp
> @@ -0,0 +1,114 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Ideas on Board Oy
> + *
> + * Base class for bayesian AWB algorithm
oops - just noticed here s/bayesian/grey world
> + */
> +
> +#include "awb_grey.h"
> +
> +#include <cmath>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/control_ids.h>
> +
> +#include "colours.h"
> +
> +using namespace libcamera::controls;
> +
> +/**
> + * \file awb_grey.h
> + * \brief Implementation of a grey world AWB algorithm
> + */
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Awb)
> +namespace ipa {
> +
> +/**
> + * \class AwbGrey
> + * \brief A Grey world auto white balance algorithm
> + */
> +
> +/**
> + * \brief Initialize the algorithm with the given tuning data
> + * \param[in] tuningData The tuning data for the algorithm
> + *
> + * Load the colour temperature curve from the tuning data. If there is no tuning
> + * data available, continue with a warning. Manual colour temperature will not
> + * work in that case.
> + *
> + * \return 0 on success, a negative error code otherwise
> + */
> +int AwbGrey::init(const YamlObject &tuningData)
> +{
> +	Interpolator<Vector<double, 2>> gains;
> +	int ret = gains.readYaml(tuningData["colourGains"], "ct", "gains");
> +	if (ret < 0)
> +		LOG(Awb, Warning)
> +			<< "Failed to parse 'colourGains' "
> +			<< "parameter from tuning file; "
> +			<< "manual colour temperature will not work properly";
> +	else
> +		colourGainCurve_ = gains;
> +
> +	return 0;
> +}
> +
> +/**
> + * \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
> + *
> + * Estimates the colour temperature based on the coulours::estimateCCT function.
> + * The gains are calculated purely based on the RGB means provided by the \a
> + * stats. The colour temperature is not taken into account when calculating the
> + * gains.
> + *
> + * The \a lux parameter is not used in this algorithm.
> + *
> + * \return The awb result
> + */
> +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)
> +{
> +	AwbResult result;
> +	auto means = stats.getRGBMeans();
> +	result.colourTemperature = 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.
> +	 */
> +	result.gains.r() = means.g() / std::max(means.r(), 1.0);
> +	result.gains.g() = 1.0;
> +	result.gains.b() = means.g() / std::max(means.b(), 1.0);
> +	return result;
> +}
> +
> +/**
> + * \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 simply interpolates the colour
> + * gains configured in the colour temperature curve.
> + *
> + * \return The colour gains if a colour temperature curve is available,
> + * [1, 1, 1] otherwise.
> + */
> +RGB<double> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> +{
> +	if (!colourGainCurve_) {
> +		LOG(Awb, Error) << "No gains defined";
> +		return RGB<double>({ 1.0, 1.0, 1.0 });
> +	}
> +
> +	auto gains = colourGainCurve_->getInterpolated(colourTemperature);
> +	return { { gains[0], 1.0, gains[0] } };
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> new file mode 100644
> index 000000000000..6eda8e5498fb
> --- /dev/null
> +++ b/src/ipa/libipa/awb_grey.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Ideas on Board Oy
> + *
> + * AWB grey world algorithm
> + */
> +
> +#pragma once
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "awb.h"
> +#include "interpolator.h"
> +#include "vector.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class AwbGrey : public AwbAlgorithm
> +{
> +public:
> +	AwbGrey() = default;
> +
> +	int init(const YamlObject &tuningData) override;
> +	AwbResult calculateAwb(const AwbStats &stats, int lux) override;
> +	RGB<double> gainsFromColourTemperature(double coulourTemperature) override;
> +
> +private:
> +	std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 03e879c5834f..c550a6eb45b6 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_grey.h',
>       'awb.h',
>       'camera_sensor_helper.h',
>       'colours.h',
> @@ -21,6 +22,7 @@ libipa_headers = files([
>   libipa_sources = files([
>       'agc_mean_luminance.cpp',
>       'algorithm.cpp',
> +    'awb_grey.cpp',
>       'awb.cpp',
>       'camera_sensor_helper.cpp',
>       'colours.cpp',

Hi Dan,

Thank you for the review. 

On Mon, Jan 20, 2025 at 10:37:18AM +0000, Dan Scally wrote:
> Hi Stefan
> 
> On 09/01/2025 11:53, Stefan Klug wrote:
> > Add the grey world algorithm that is currently used in rkisp1 to libipa.
> > No changes in functionality were made.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >   src/ipa/libipa/awb_grey.cpp | 114 ++++++++++++++++++++++++++++++++++++
> >   src/ipa/libipa/awb_grey.h   |  35 +++++++++++
> >   src/ipa/libipa/meson.build  |   2 +
> >   3 files changed, 151 insertions(+)
> >   create mode 100644 src/ipa/libipa/awb_grey.cpp
> >   create mode 100644 src/ipa/libipa/awb_grey.h
> > 
> > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> > new file mode 100644
> > index 000000000000..192a7cf3834a
> > --- /dev/null
> > +++ b/src/ipa/libipa/awb_grey.cpp
> > @@ -0,0 +1,114 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024 Ideas on Board Oy
> > + *
> > + * Base class for bayesian AWB algorithm
> > + */
> > +
> > +#include "awb_grey.h"
> > +
> > +#include <cmath>
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/control_ids.h>
> > +
> > +#include "colours.h"
> > +
> > +using namespace libcamera::controls;
> > +
> > +/**
> > + * \file awb_grey.h
> > + * \brief Implementation of a grey world AWB algorithm
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(Awb)
> Is it worth differentiating between this log category and that in awb.cpp -
> which is also just "Awb"? The logs will include the file path and line
> number of course so they can be distinguished anyway so probably it doesn't
> matter, but I thought I'd mention it just to bring it to mind - I'm happy
> either way.

I was also quite undecided here. The reason I went for the short one, is
that there will always be only one AWB algorithm active per camera. So
there seems to be no need to further differentiate that at runtime. But
both options are valid. I think I'd like to stick to the current
(simple) solution for now.

> > +namespace ipa {
> > +
> > +/**
> > + * \class AwbGrey
> > + * \brief A Grey world auto white balance algorithm
> > + */
> > +
> > +/**
> > + * \brief Initialize the algorithm with the given tuning data
> > + * \param[in] tuningData The tuning data for the algorithm
> > + *
> > + * Load the colour temperature curve from the tuning data. If there is no tuning
> > + * data available, continue with a warning. Manual colour temperature will not
> > + * work in that case.
> > + *
> > + * \return 0 on success, a negative error code otherwise
> > + */
> > +int AwbGrey::init(const YamlObject &tuningData)
> > +{
> > +	Interpolator<Vector<double, 2>> gains;
> > +	int ret = gains.readYaml(tuningData["colourGains"], "ct", "gains");
> > +	if (ret < 0)
> > +		LOG(Awb, Warning)
> > +			<< "Failed to parse 'colourGains' "
> > +			<< "parameter from tuning file; "
> > +			<< "manual colour temperature will not work properly";
> > +	else
> > +		colourGainCurve_ = gains;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \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
> > + *
> > + * Estimates the colour temperature based on the coulours::estimateCCT function.
> s/coulours/colours.
> > + * The gains are calculated purely based on the RGB means provided by the \a
> > + * stats. The colour temperature is not taken into account when calculating the
> > + * gains.
> > + *
> > + * The \a lux parameter is not used in this algorithm.
> > + *
> > + * \return The awb result
> > + */
> > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)
> > +{
> > +	AwbResult result;
> > +	auto means = stats.getRGBMeans();
> > +	result.colourTemperature = 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.
> > +	 */
> > +	result.gains.r() = means.g() / std::max(means.r(), 1.0);
> > +	result.gains.g() = 1.0;
> > +	result.gains.b() = means.g() / std::max(means.b(), 1.0);
> > +	return result;
> > +}
> > +
> > +/**
> > + * \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 simply interpolates the colour
> > + * gains configured in the colour temperature curve.
> > + *
> > + * \return The colour gains if a colour temperature curve is available,
> > + * [1, 1, 1] otherwise.
> > + */
> > +RGB<double> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> > +{
> > +	if (!colourGainCurve_) {
> > +		LOG(Awb, Error) << "No gains defined";
> > +		return RGB<double>({ 1.0, 1.0, 1.0 });
> > +	}
> > +
> > +	auto gains = colourGainCurve_->getInterpolated(colourTemperature);
> > +	return { { gains[0], 1.0, gains[0] } };
> > +}
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> > new file mode 100644
> > index 000000000000..6eda8e5498fb
> > --- /dev/null
> > +++ b/src/ipa/libipa/awb_grey.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024 Ideas on Board Oy
> > + *
> > + * AWB grey world algorithm
> > + */
> > +
> > +#pragma once
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "awb.h"
> > +#include "interpolator.h"
> > +#include "vector.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +class AwbGrey : public AwbAlgorithm
> > +{
> > +public:
> > +	AwbGrey() = default;
> > +
> > +	int init(const YamlObject &tuningData) override;
> > +	AwbResult calculateAwb(const AwbStats &stats, int lux) override;
> > +	RGB<double> gainsFromColourTemperature(double coulourTemperature) override;
> 
> s/coulourTemperature/colourTemperature

hmpf..

> 
> 
> Otherwise looks good I think:
> 
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Thanks,
Stefan

> 
> > +
> > +private:
> > +	std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 03e879c5834f..c550a6eb45b6 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_grey.h',
> >       'awb.h',
> >       'camera_sensor_helper.h',
> >       'colours.h',
> > @@ -21,6 +22,7 @@ libipa_headers = files([
> >   libipa_sources = files([
> >       'agc_mean_luminance.cpp',
> >       'algorithm.cpp',
> > +    'awb_grey.cpp',
> >       'awb.cpp',
> >       'camera_sensor_helper.cpp',
> >       'colours.cpp',

Hi Stefan

On 20/01/2025 13:52, Stefan Klug wrote:
> Hi Dan,
>
> Thank you for the review.
>
> On Mon, Jan 20, 2025 at 10:37:18AM +0000, Dan Scally wrote:
>> Hi Stefan
>>
>> On 09/01/2025 11:53, Stefan Klug wrote:
>>> Add the grey world algorithm that is currently used in rkisp1 to libipa.
>>> No changes in functionality were made.
>>>
>>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
>>> ---
>>>    src/ipa/libipa/awb_grey.cpp | 114 ++++++++++++++++++++++++++++++++++++
>>>    src/ipa/libipa/awb_grey.h   |  35 +++++++++++
>>>    src/ipa/libipa/meson.build  |   2 +
>>>    3 files changed, 151 insertions(+)
>>>    create mode 100644 src/ipa/libipa/awb_grey.cpp
>>>    create mode 100644 src/ipa/libipa/awb_grey.h
>>>
>>> diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
>>> new file mode 100644
>>> index 000000000000..192a7cf3834a
>>> --- /dev/null
>>> +++ b/src/ipa/libipa/awb_grey.cpp
>>> @@ -0,0 +1,114 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2024 Ideas on Board Oy
>>> + *
>>> + * Base class for bayesian AWB algorithm
>>> + */
>>> +
>>> +#include "awb_grey.h"
>>> +
>>> +#include <cmath>
>>> +
>>> +#include <libcamera/base/log.h>
>>> +#include <libcamera/control_ids.h>
>>> +
>>> +#include "colours.h"
>>> +
>>> +using namespace libcamera::controls;
>>> +
>>> +/**
>>> + * \file awb_grey.h
>>> + * \brief Implementation of a grey world AWB algorithm
>>> + */
>>> +
>>> +namespace libcamera {
>>> +
>>> +LOG_DECLARE_CATEGORY(Awb)
>> Is it worth differentiating between this log category and that in awb.cpp -
>> which is also just "Awb"? The logs will include the file path and line
>> number of course so they can be distinguished anyway so probably it doesn't
>> matter, but I thought I'd mention it just to bring it to mind - I'm happy
>> either way.
> I was also quite undecided here. The reason I went for the short one, is
> that there will always be only one AWB algorithm active per camera. So
> there seems to be no need to further differentiate that at runtime.
That seems like a solid argument to me - also makes it easier to remember which to turn on I suppose.
>   But
> both options are valid. I think I'd like to stick to the current
> (simple) solution for now.
Alright - fine by me.
>
>>> +namespace ipa {
>>> +
>>> +/**
>>> + * \class AwbGrey
>>> + * \brief A Grey world auto white balance algorithm
>>> + */
>>> +
>>> +/**
>>> + * \brief Initialize the algorithm with the given tuning data
>>> + * \param[in] tuningData The tuning data for the algorithm
>>> + *
>>> + * Load the colour temperature curve from the tuning data. If there is no tuning
>>> + * data available, continue with a warning. Manual colour temperature will not
>>> + * work in that case.
>>> + *
>>> + * \return 0 on success, a negative error code otherwise
>>> + */
>>> +int AwbGrey::init(const YamlObject &tuningData)
>>> +{
>>> +	Interpolator<Vector<double, 2>> gains;
>>> +	int ret = gains.readYaml(tuningData["colourGains"], "ct", "gains");
>>> +	if (ret < 0)
>>> +		LOG(Awb, Warning)
>>> +			<< "Failed to parse 'colourGains' "
>>> +			<< "parameter from tuning file; "
>>> +			<< "manual colour temperature will not work properly";
>>> +	else
>>> +		colourGainCurve_ = gains;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * \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
>>> + *
>>> + * Estimates the colour temperature based on the coulours::estimateCCT function.
>> s/coulours/colours.
>>> + * The gains are calculated purely based on the RGB means provided by the \a
>>> + * stats. The colour temperature is not taken into account when calculating the
>>> + * gains.
>>> + *
>>> + * The \a lux parameter is not used in this algorithm.
>>> + *
>>> + * \return The awb result
>>> + */
>>> +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)
>>> +{
>>> +	AwbResult result;
>>> +	auto means = stats.getRGBMeans();
>>> +	result.colourTemperature = 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.
>>> +	 */
>>> +	result.gains.r() = means.g() / std::max(means.r(), 1.0);
>>> +	result.gains.g() = 1.0;
>>> +	result.gains.b() = means.g() / std::max(means.b(), 1.0);
>>> +	return result;
>>> +}
>>> +
>>> +/**
>>> + * \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 simply interpolates the colour
>>> + * gains configured in the colour temperature curve.
>>> + *
>>> + * \return The colour gains if a colour temperature curve is available,
>>> + * [1, 1, 1] otherwise.
>>> + */
>>> +RGB<double> AwbGrey::gainsFromColourTemperature(double colourTemperature)
>>> +{
>>> +	if (!colourGainCurve_) {
>>> +		LOG(Awb, Error) << "No gains defined";
>>> +		return RGB<double>({ 1.0, 1.0, 1.0 });
>>> +	}
>>> +
>>> +	auto gains = colourGainCurve_->getInterpolated(colourTemperature);
>>> +	return { { gains[0], 1.0, gains[0] } };
>>> +}
>>> +
>>> +} /* namespace ipa */
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
>>> new file mode 100644
>>> index 000000000000..6eda8e5498fb
>>> --- /dev/null
>>> +++ b/src/ipa/libipa/awb_grey.h
>>> @@ -0,0 +1,35 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2024 Ideas on Board Oy
>>> + *
>>> + * AWB grey world algorithm
>>> + */
>>> +
>>> +#pragma once
>>> +
>>> +#include "libcamera/internal/yaml_parser.h"
>>> +
>>> +#include "awb.h"
>>> +#include "interpolator.h"
>>> +#include "vector.h"
>>> +
>>> +namespace libcamera {
>>> +
>>> +namespace ipa {
>>> +
>>> +class AwbGrey : public AwbAlgorithm
>>> +{
>>> +public:
>>> +	AwbGrey() = default;
>>> +
>>> +	int init(const YamlObject &tuningData) override;
>>> +	AwbResult calculateAwb(const AwbStats &stats, int lux) override;
>>> +	RGB<double> gainsFromColourTemperature(double coulourTemperature) override;
>> s/coulourTemperature/colourTemperature
> hmpf..
>
>>
>> Otherwise looks good I think:
>>
>>
>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> Thanks,
> Stefan
>
>>> +
>>> +private:
>>> +	std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_;
>>> +};
>>> +
>>> +} /* namespace ipa */
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>>> index 03e879c5834f..c550a6eb45b6 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_grey.h',
>>>        'awb.h',
>>>        'camera_sensor_helper.h',
>>>        'colours.h',
>>> @@ -21,6 +22,7 @@ libipa_headers = files([
>>>    libipa_sources = files([
>>>        'agc_mean_luminance.cpp',
>>>        'algorithm.cpp',
>>> +    'awb_grey.cpp',
>>>        'awb.cpp',
>>>        'camera_sensor_helper.cpp',
>>>        'colours.cpp',