[1/6] ipa: libipa: Add miscellaneous helpers
diff mbox series

Message ID 20241031160741.253855-2-dan.scally@ideasonboard.com
State Superseded
Headers show
Series
  • Centralise common functions in IPA modules
Related show

Commit Message

Daniel Scally Oct. 31, 2024, 4:07 p.m. UTC
We start to have functions that are effectively identical crop up
across the IPA modules. Add a file allowing those to be centralised
within libipa so that a single implementation can be used in all of
the IPAs.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/helpers.h   |  23 +++++++++
 src/ipa/libipa/meson.build |   2 +
 3 files changed, 128 insertions(+)
 create mode 100644 src/ipa/libipa/helpers.cpp
 create mode 100644 src/ipa/libipa/helpers.h

Comments

Jacopo Mondi Nov. 4, 2024, 11:07 a.m. UTC | #1
Hi Dan

On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:
> We start to have functions that are effectively identical crop up
> across the IPA modules. Add a file allowing those to be centralised
> within libipa so that a single implementation can be used in all of
> the IPAs.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/helpers.h   |  23 +++++++++
>  src/ipa/libipa/meson.build |   2 +
>  3 files changed, 128 insertions(+)
>  create mode 100644 src/ipa/libipa/helpers.cpp
>  create mode 100644 src/ipa/libipa/helpers.h
>
> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp
> new file mode 100644
> index 00000000..cc0cd586
> --- /dev/null
> +++ b/src/ipa/libipa/helpers.cpp

Not a huge fan of a collection of C functions, but I guess
alternatives could be considered over-design

> @@ -0,0 +1,103 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2024, Ideas on Board Oy
> + *
> + * libipa miscellaneous helpers
> + */
> +
> +#include "helpers.h"
> +
> +#include <algorithm>
> +#include <cmath>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +/**
> + * \file helpers.h
> + * \brief Functions to reduce code duplication between IPA modules
> + */
> +
> +/**
> + * \brief Estimate luminance from RGB values following ITU-R BT.601
> + * \param[in] r The red value
> + * \param[in] g The green value
> + * \param[in] b The blue valye
> + * \return The estimated luminance value

\return statements usually go after the longer function documentation


> + *
> + * This function estimates a luminance value from a triplet of Red, Green and
> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7
> + * which can be found at https://www.itu.int/rec/R-REC-BT.601
> + */
> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)

In example, in the RPi IPA, the return value is assigned to a double,
and the calculation suggests it might be worth not casing the result to
unsigned int.

> +{
> +	return (r * .299) + (g * .587) + (b * .114);
> +}
> +
> +/**
> + * \brief Estimate correlated colour temperature from RGB color space input
> + * \param[in] red The input red value
> + * \param[in] green The input green value
> + * \param[in] blue The input blue value
> + * \return The estimated color temperature

same here

> + *
> + * This function estimates the correlated color temperature RGB color space
> + * input. In physics and color science, the Planckian locus or black body locus
> + * is the path or locus that the color of an incandescent black body would take
> + * in a particular chromaticity space as the blackbody temperature changes.
> + *
> + * If a narrow range of color temperatures is considered (those encapsulating
> + * daylight being the most practical case) one can approximate the Planckian
> + * locus in order to calculate the CCT in terms of chromaticity coordinates.
> + *
> + * More detailed information can be found in:
> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation
> + */
> +uint32_t estimateCCT(double red, double green, double blue)
> +{
> +	/* Convert the RGB values to CIE tristimulus values (XYZ) */
> +	double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
> +	double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
> +	double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);
> +
> +	/* Calculate the normalized chromaticity values */
> +	double x = X / (X + Y + Z);
> +	double y = Y / (X + Y + Z);
> +
> +	/* Calculate CCT */
> +	double n = (x - 0.3320) / (0.1858 - y);
> +	return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
> +}

This seems to come straight from the IPU3 IPA, so ack to the text and
implementation

> +
> +/**
> + * \brief Convert double to Q(m.n) format

Note these two helpers seems to be un-used.

What Q formats are we referring to ? Wikipedia lists two (TI version
and ARM version).
https://en.wikipedia.org/wiki/Q_(number_format)

> + * \param[in] value The value to convert
> + * \param[in] m The number of bits used to represent the integer part
> + * \param[in] n The number of bits used to represent the fraction part
> + *
> + * This function converts a double into an unsigned Q format, clamping at the
> + * largest possible value given m and n.

\return ?

> + */
> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)
> +{
> +	double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);
> +
> +	return std::clamp(value, 0.0, maxVal) * std::pow(2, n);
> +}
> +
> +/**
> + * \brief Convert Q(m.n) formatted number to double
> + * \param[in] value The value to convert
> + * \param[in] n The number of bits used to represent the fraction part
> + *
> + * This function converts an unsigned Q formatted value into a double.
> + */
> +double fromQFormat(unsigned int value, unsigned int n)
> +{
> +	return value / std::pow(2, n);
> +}

Let's discuss these once the above is clarified

> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
> new file mode 100644
> index 00000000..361b63bd
> --- /dev/null
> +++ b/src/ipa/libipa/helpers.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2024, Ideas on Board Oy
> + *
> + * libipa miscellaneous helpers
> + */
> +
> +#pragma once
> +
> +#include <stdint.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
> +uint32_t estimateCCT(double red, double green, double blue);
> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);
> +double fromQFormat(unsigned int value, unsigned int n);
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index e78cbcd6..ca4f3e28 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -6,6 +6,7 @@ libipa_headers = files([
>      'camera_sensor_helper.h',
>      'exposure_mode_helper.h',
>      'fc_queue.h',
> +    'helpers.h',
>      'histogram.h',
>      'interpolator.h',
>      'lsc_polynomial.h',
> @@ -21,6 +22,7 @@ libipa_sources = files([
>      'camera_sensor_helper.cpp',
>      'exposure_mode_helper.cpp',
>      'fc_queue.cpp',
> +    'helpers.cpp',
>      'histogram.cpp',
>      'interpolator.cpp',
>      'lsc_polynomial.cpp',
> --
> 2.30.2
>
Daniel Scally Nov. 6, 2024, 9:39 a.m. UTC | #2
Hi Jacopo - thanks for the review

On 04/11/2024 11:07, Jacopo Mondi wrote:
> Hi Dan
>
> On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:
>> We start to have functions that are effectively identical crop up
>> across the IPA modules. Add a file allowing those to be centralised
>> within libipa so that a single implementation can be used in all of
>> the IPAs.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>>   src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++
>>   src/ipa/libipa/helpers.h   |  23 +++++++++
>>   src/ipa/libipa/meson.build |   2 +
>>   3 files changed, 128 insertions(+)
>>   create mode 100644 src/ipa/libipa/helpers.cpp
>>   create mode 100644 src/ipa/libipa/helpers.h
>>
>> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp
>> new file mode 100644
>> index 00000000..cc0cd586
>> --- /dev/null
>> +++ b/src/ipa/libipa/helpers.cpp
> Not a huge fan of a collection of C functions, but I guess
> alternatives could be considered over-design
That was my thinking basically.
>
>> @@ -0,0 +1,103 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2024, Ideas on Board Oy
>> + *
>> + * libipa miscellaneous helpers
>> + */
>> +
>> +#include "helpers.h"
>> +
>> +#include <algorithm>
>> +#include <cmath>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +/**
>> + * \file helpers.h
>> + * \brief Functions to reduce code duplication between IPA modules
>> + */
>> +
>> +/**
>> + * \brief Estimate luminance from RGB values following ITU-R BT.601
>> + * \param[in] r The red value
>> + * \param[in] g The green value
>> + * \param[in] b The blue valye
>> + * \return The estimated luminance value
> \return statements usually go after the longer function documentation


Ack

>
>
>> + *
>> + * This function estimates a luminance value from a triplet of Red, Green and
>> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7
>> + * which can be found at https://www.itu.int/rec/R-REC-BT.601
>> + */
>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)
> In example, in the RPi IPA, the return value is assigned to a double,
> and the calculation suggests it might be worth not casing the result to
> unsigned int.
You're right, that's wrong. Thanks.
>
>> +{
>> +	return (r * .299) + (g * .587) + (b * .114);
>> +}
>> +
>> +/**
>> + * \brief Estimate correlated colour temperature from RGB color space input
>> + * \param[in] red The input red value
>> + * \param[in] green The input green value
>> + * \param[in] blue The input blue value
>> + * \return The estimated color temperature
> same here
>
>> + *
>> + * This function estimates the correlated color temperature RGB color space
>> + * input. In physics and color science, the Planckian locus or black body locus
>> + * is the path or locus that the color of an incandescent black body would take
>> + * in a particular chromaticity space as the blackbody temperature changes.
>> + *
>> + * If a narrow range of color temperatures is considered (those encapsulating
>> + * daylight being the most practical case) one can approximate the Planckian
>> + * locus in order to calculate the CCT in terms of chromaticity coordinates.
>> + *
>> + * More detailed information can be found in:
>> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation
>> + */
>> +uint32_t estimateCCT(double red, double green, double blue)
>> +{
>> +	/* Convert the RGB values to CIE tristimulus values (XYZ) */
>> +	double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
>> +	double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
>> +	double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);
>> +
>> +	/* Calculate the normalized chromaticity values */
>> +	double x = X / (X + Y + Z);
>> +	double y = Y / (X + Y + Z);
>> +
>> +	/* Calculate CCT */
>> +	double n = (x - 0.3320) / (0.1858 - y);
>> +	return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
>> +}
> This seems to come straight from the IPU3 IPA, so ack to the text and
> implementation
>
>> +
>> +/**
>> + * \brief Convert double to Q(m.n) format
> Note these two helpers seems to be un-used.
Ah yes, they're used in the C55 series which will be based on top of this...I can introduce them 
there if that's better?
> What Q formats are we referring to ? Wikipedia lists two (TI version
> and ARM version).
> https://en.wikipedia.org/wiki/Q_(number_format)


So far I'm only using numbers specified in documentation as being unsigned, and so it's the TI 
version, but the "UQ" format rather than just "Q"...possibly renaming the function 
"toUnsignedQFormat()" would be clearer?

>
>> + * \param[in] value The value to convert
>> + * \param[in] m The number of bits used to represent the integer part
>> + * \param[in] n The number of bits used to represent the fraction part
>> + *
>> + * This function converts a double into an unsigned Q format, clamping at the
>> + * largest possible value given m and n.
> \return ?
>
>> + */
>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)
>> +{
>> +	double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);
>> +
>> +	return std::clamp(value, 0.0, maxVal) * std::pow(2, n);
>> +}
>> +
>> +/**
>> + * \brief Convert Q(m.n) formatted number to double
>> + * \param[in] value The value to convert
>> + * \param[in] n The number of bits used to represent the fraction part
>> + *
>> + * This function converts an unsigned Q formatted value into a double.
>> + */
>> +double fromQFormat(unsigned int value, unsigned int n)
>> +{
>> +	return value / std::pow(2, n);
>> +}
> Let's discuss these once the above is clarified
>
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
>> new file mode 100644
>> index 00000000..361b63bd
>> --- /dev/null
>> +++ b/src/ipa/libipa/helpers.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2024, Ideas on Board Oy
>> + *
>> + * libipa miscellaneous helpers
>> + */
>> +
>> +#pragma once
>> +
>> +#include <stdint.h>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
>> +uint32_t estimateCCT(double red, double green, double blue);
>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);
>> +double fromQFormat(unsigned int value, unsigned int n);
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>> index e78cbcd6..ca4f3e28 100644
>> --- a/src/ipa/libipa/meson.build
>> +++ b/src/ipa/libipa/meson.build
>> @@ -6,6 +6,7 @@ libipa_headers = files([
>>       'camera_sensor_helper.h',
>>       'exposure_mode_helper.h',
>>       'fc_queue.h',
>> +    'helpers.h',
>>       'histogram.h',
>>       'interpolator.h',
>>       'lsc_polynomial.h',
>> @@ -21,6 +22,7 @@ libipa_sources = files([
>>       'camera_sensor_helper.cpp',
>>       'exposure_mode_helper.cpp',
>>       'fc_queue.cpp',
>> +    'helpers.cpp',
>>       'histogram.cpp',
>>       'interpolator.cpp',
>>       'lsc_polynomial.cpp',
>> --
>> 2.30.2
>>
Kieran Bingham Nov. 6, 2024, 1:56 p.m. UTC | #3
Quoting Dan Scally (2024-11-06 09:39:09)
> Hi Jacopo - thanks for the review
> 
> On 04/11/2024 11:07, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:
> >> We start to have functions that are effectively identical crop up
> >> across the IPA modules. Add a file allowing those to be centralised
> >> within libipa so that a single implementation can be used in all of
> >> the IPAs.
> >>
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >>   src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++
> >>   src/ipa/libipa/helpers.h   |  23 +++++++++
> >>   src/ipa/libipa/meson.build |   2 +
> >>   3 files changed, 128 insertions(+)
> >>   create mode 100644 src/ipa/libipa/helpers.cpp
> >>   create mode 100644 src/ipa/libipa/helpers.h
> >>
> >> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp
> >> new file mode 100644
> >> index 00000000..cc0cd586
> >> --- /dev/null
> >> +++ b/src/ipa/libipa/helpers.cpp
> > Not a huge fan of a collection of C functions, but I guess
> > alternatives could be considered over-design
> That was my thinking basically.

I agree here. I'm fine moving these to a common location to get started.

If things grow - or become more specific then they could be separated to
a defined structure, but that could also be done on top ... I have one
example below for that...


> >
> >> @@ -0,0 +1,103 @@
> >> +/* SPDX-License-Identifier: BSD-2-Clause */
> >> +/*
> >> + * Copyright (C) 2024, Ideas on Board Oy
> >> + *
> >> + * libipa miscellaneous helpers
> >> + */
> >> +
> >> +#include "helpers.h"
> >> +
> >> +#include <algorithm>
> >> +#include <cmath>
> >> +
> >> +namespace libcamera {
> >> +
> >> +namespace ipa {
> >> +
> >> +/**
> >> + * \file helpers.h
> >> + * \brief Functions to reduce code duplication between IPA modules
> >> + */
> >> +
> >> +/**
> >> + * \brief Estimate luminance from RGB values following ITU-R BT.601
> >> + * \param[in] r The red value
> >> + * \param[in] g The green value
> >> + * \param[in] b The blue valye

s/valye/value/

> >> + * \return The estimated luminance value
> > \return statements usually go after the longer function documentation
> 
> 
> Ack
> 
> >
> >
> >> + *
> >> + * This function estimates a luminance value from a triplet of Red, Green and
> >> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7
> >> + * which can be found at https://www.itu.int/rec/R-REC-BT.601
> >> + */
> >> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)
> > In example, in the RPi IPA, the return value is assigned to a double,
> > and the calculation suggests it might be worth not casing the result to
> > unsigned int.
> You're right, that's wrong. Thanks.
> >
> >> +{
> >> +    return (r * .299) + (g * .587) + (b * .114);
> >> +}
> >> +
> >> +/**
> >> + * \brief Estimate correlated colour temperature from RGB color space input
> >> + * \param[in] red The input red value
> >> + * \param[in] green The input green value
> >> + * \param[in] blue The input blue value
> >> + * \return The estimated color temperature
> > same here
> >
> >> + *
> >> + * This function estimates the correlated color temperature RGB color space
> >> + * input. In physics and color science, the Planckian locus or black body locus
> >> + * is the path or locus that the color of an incandescent black body would take
> >> + * in a particular chromaticity space as the blackbody temperature changes.
> >> + *
> >> + * If a narrow range of color temperatures is considered (those encapsulating
> >> + * daylight being the most practical case) one can approximate the Planckian
> >> + * locus in order to calculate the CCT in terms of chromaticity coordinates.
> >> + *
> >> + * More detailed information can be found in:
> >> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation
> >> + */
> >> +uint32_t estimateCCT(double red, double green, double blue)
> >> +{
> >> +    /* Convert the RGB values to CIE tristimulus values (XYZ) */
> >> +    double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
> >> +    double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
> >> +    double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);
> >> +
> >> +    /* Calculate the normalized chromaticity values */
> >> +    double x = X / (X + Y + Z);
> >> +    double y = Y / (X + Y + Z);
> >> +
> >> +    /* Calculate CCT */
> >> +    double n = (x - 0.3320) / (0.1858 - y);
> >> +    return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
> >> +}
> > This seems to come straight from the IPU3 IPA, so ack to the text and
> > implementation

Perhaps those are both helpers for 'colorspace' so maybe this is a
colorspace.cpp ... but lets see...


> >
> >> +
> >> +/**
> >> + * \brief Convert double to Q(m.n) format
> > Note these two helpers seems to be un-used.
> Ah yes, they're used in the C55 series which will be based on top of this...I can introduce them 
> there if that's better?
> > What Q formats are we referring to ? Wikipedia lists two (TI version
> > and ARM version).
> > https://en.wikipedia.org/wiki/Q_(number_format)
> 
> 
> So far I'm only using numbers specified in documentation as being unsigned, and so it's the TI 
> version, but the "UQ" format rather than just "Q"...possibly renaming the function 
> "toUnsignedQFormat()" would be clearer?
> 
> >
> >> + * \param[in] value The value to convert
> >> + * \param[in] m The number of bits used to represent the integer part
> >> + * \param[in] n The number of bits used to represent the fraction part
> >> + *
> >> + * This function converts a double into an unsigned Q format, clamping at the
> >> + * largest possible value given m and n.
> > \return ?
> >
> >> + */
> >> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)
> >> +{
> >> +    double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);
> >> +
> >> +    return std::clamp(value, 0.0, maxVal) * std::pow(2, n);
> >> +}
> >> +
> >> +/**
> >> + * \brief Convert Q(m.n) formatted number to double
> >> + * \param[in] value The value to convert
> >> + * \param[in] n The number of bits used to represent the fraction part
> >> + *
> >> + * This function converts an unsigned Q formatted value into a double.
> >> + */
> >> +double fromQFormat(unsigned int value, unsigned int n)
> >> +{
> >> +    return value / std::pow(2, n);
> >> +}
> > Let's discuss these once the above is clarified

Q(a.b) formats come up a lot. I'd always thought we need a FixedPoint
(templated?) class to help with this so we can consider it as a real
type.

If so - it would be it's own class, and warrant a set of unit tests...

So if you did want to split this helpers.cpp,h up - I'd move the color
space functionality to colorspace.cpp and Q helpers to fixedpoint.cpp
...

Ultimately it depends how far you want to take the cleanup...



> >
> >> +
> >> +} /* namespace ipa */
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
> >> new file mode 100644
> >> index 00000000..361b63bd
> >> --- /dev/null
> >> +++ b/src/ipa/libipa/helpers.h
> >> @@ -0,0 +1,23 @@
> >> +/* SPDX-License-Identifier: BSD-2-Clause */
> >> +/*
> >> + * Copyright (C) 2024, Ideas on Board Oy
> >> + *
> >> + * libipa miscellaneous helpers
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include <stdint.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +namespace ipa {
> >> +
> >> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
> >> +uint32_t estimateCCT(double red, double green, double blue);
> >> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);
> >> +double fromQFormat(unsigned int value, unsigned int n);
> >> +
> >> +} /* namespace ipa */
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> >> index e78cbcd6..ca4f3e28 100644
> >> --- a/src/ipa/libipa/meson.build
> >> +++ b/src/ipa/libipa/meson.build
> >> @@ -6,6 +6,7 @@ libipa_headers = files([
> >>       'camera_sensor_helper.h',
> >>       'exposure_mode_helper.h',
> >>       'fc_queue.h',
> >> +    'helpers.h',
> >>       'histogram.h',
> >>       'interpolator.h',
> >>       'lsc_polynomial.h',
> >> @@ -21,6 +22,7 @@ libipa_sources = files([
> >>       'camera_sensor_helper.cpp',
> >>       'exposure_mode_helper.cpp',
> >>       'fc_queue.cpp',
> >> +    'helpers.cpp',
> >>       'histogram.cpp',
> >>       'interpolator.cpp',
> >>       'lsc_polynomial.cpp',
> >> --
> >> 2.30.2
> >>
Daniel Scally Nov. 6, 2024, 2:01 p.m. UTC | #4
Hi Kiean

On 06/11/2024 13:56, Kieran Bingham wrote:
> Quoting Dan Scally (2024-11-06 09:39:09)
>> Hi Jacopo - thanks for the review
>>
>> On 04/11/2024 11:07, Jacopo Mondi wrote:
>>> Hi Dan
>>>
>>> On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:
>>>> We start to have functions that are effectively identical crop up
>>>> across the IPA modules. Add a file allowing those to be centralised
>>>> within libipa so that a single implementation can be used in all of
>>>> the IPAs.
>>>>
>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>> ---
>>>>    src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++
>>>>    src/ipa/libipa/helpers.h   |  23 +++++++++
>>>>    src/ipa/libipa/meson.build |   2 +
>>>>    3 files changed, 128 insertions(+)
>>>>    create mode 100644 src/ipa/libipa/helpers.cpp
>>>>    create mode 100644 src/ipa/libipa/helpers.h
>>>>
>>>> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp
>>>> new file mode 100644
>>>> index 00000000..cc0cd586
>>>> --- /dev/null
>>>> +++ b/src/ipa/libipa/helpers.cpp
>>> Not a huge fan of a collection of C functions, but I guess
>>> alternatives could be considered over-design
>> That was my thinking basically.
> I agree here. I'm fine moving these to a common location to get started.
>
> If things grow - or become more specific then they could be separated to
> a defined structure, but that could also be done on top ... I have one
> example below for that...
>
>
>>>> @@ -0,0 +1,103 @@
>>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>>> +/*
>>>> + * Copyright (C) 2024, Ideas on Board Oy
>>>> + *
>>>> + * libipa miscellaneous helpers
>>>> + */
>>>> +
>>>> +#include "helpers.h"
>>>> +
>>>> +#include <algorithm>
>>>> +#include <cmath>
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +namespace ipa {
>>>> +
>>>> +/**
>>>> + * \file helpers.h
>>>> + * \brief Functions to reduce code duplication between IPA modules
>>>> + */
>>>> +
>>>> +/**
>>>> + * \brief Estimate luminance from RGB values following ITU-R BT.601
>>>> + * \param[in] r The red value
>>>> + * \param[in] g The green value
>>>> + * \param[in] b The blue valye
> s/valye/value/
>
>>>> + * \return The estimated luminance value
>>> \return statements usually go after the longer function documentation
>>
>> Ack
>>
>>>
>>>> + *
>>>> + * This function estimates a luminance value from a triplet of Red, Green and
>>>> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7
>>>> + * which can be found at https://www.itu.int/rec/R-REC-BT.601
>>>> + */
>>>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)
>>> In example, in the RPi IPA, the return value is assigned to a double,
>>> and the calculation suggests it might be worth not casing the result to
>>> unsigned int.
>> You're right, that's wrong. Thanks.
>>>> +{
>>>> +    return (r * .299) + (g * .587) + (b * .114);
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Estimate correlated colour temperature from RGB color space input
>>>> + * \param[in] red The input red value
>>>> + * \param[in] green The input green value
>>>> + * \param[in] blue The input blue value
>>>> + * \return The estimated color temperature
>>> same here
>>>
>>>> + *
>>>> + * This function estimates the correlated color temperature RGB color space
>>>> + * input. In physics and color science, the Planckian locus or black body locus
>>>> + * is the path or locus that the color of an incandescent black body would take
>>>> + * in a particular chromaticity space as the blackbody temperature changes.
>>>> + *
>>>> + * If a narrow range of color temperatures is considered (those encapsulating
>>>> + * daylight being the most practical case) one can approximate the Planckian
>>>> + * locus in order to calculate the CCT in terms of chromaticity coordinates.
>>>> + *
>>>> + * More detailed information can be found in:
>>>> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation
>>>> + */
>>>> +uint32_t estimateCCT(double red, double green, double blue)
>>>> +{
>>>> +    /* Convert the RGB values to CIE tristimulus values (XYZ) */
>>>> +    double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
>>>> +    double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
>>>> +    double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);
>>>> +
>>>> +    /* Calculate the normalized chromaticity values */
>>>> +    double x = X / (X + Y + Z);
>>>> +    double y = Y / (X + Y + Z);
>>>> +
>>>> +    /* Calculate CCT */
>>>> +    double n = (x - 0.3320) / (0.1858 - y);
>>>> +    return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
>>>> +}
>>> This seems to come straight from the IPU3 IPA, so ack to the text and
>>> implementation
> Perhaps those are both helpers for 'colorspace' so maybe this is a
> colorspace.cpp ... but lets see...
Yeah but then we get lots of little bitty filesĀ  - that seems worse to me.
>
>>>> +
>>>> +/**
>>>> + * \brief Convert double to Q(m.n) format
>>> Note these two helpers seems to be un-used.
>> Ah yes, they're used in the C55 series which will be based on top of this...I can introduce them
>> there if that's better?
>>> What Q formats are we referring to ? Wikipedia lists two (TI version
>>> and ARM version).
>>> https://en.wikipedia.org/wiki/Q_(number_format)
>>
>> So far I'm only using numbers specified in documentation as being unsigned, and so it's the TI
>> version, but the "UQ" format rather than just "Q"...possibly renaming the function
>> "toUnsignedQFormat()" would be clearer?
>>
>>>> + * \param[in] value The value to convert
>>>> + * \param[in] m The number of bits used to represent the integer part
>>>> + * \param[in] n The number of bits used to represent the fraction part
>>>> + *
>>>> + * This function converts a double into an unsigned Q format, clamping at the
>>>> + * largest possible value given m and n.
>>> \return ?
>>>
>>>> + */
>>>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)
>>>> +{
>>>> +    double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);
>>>> +
>>>> +    return std::clamp(value, 0.0, maxVal) * std::pow(2, n);
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Convert Q(m.n) formatted number to double
>>>> + * \param[in] value The value to convert
>>>> + * \param[in] n The number of bits used to represent the fraction part
>>>> + *
>>>> + * This function converts an unsigned Q formatted value into a double.
>>>> + */
>>>> +double fromQFormat(unsigned int value, unsigned int n)
>>>> +{
>>>> +    return value / std::pow(2, n);
>>>> +}
>>> Let's discuss these once the above is clarified
> Q(a.b) formats come up a lot. I'd always thought we need a FixedPoint
> (templated?) class to help with this so we can consider it as a real
> type.
>
> If so - it would be it's own class, and warrant a set of unit tests...
>
> So if you did want to split this helpers.cpp,h up - I'd move the color
> space functionality to colorspace.cpp and Q helpers to fixedpoint.cpp
> ...
>
> Ultimately it depends how far you want to take the cleanup...


Well, where else have they come up that we have the conversion open-coded at the moment? It seems a 
bit heavy handed for just the above, but if there're more places that could be tidied up perhaps 
it'd be worthwhile

>
>
>
>>>> +
>>>> +} /* namespace ipa */
>>>> +
>>>> +} /* namespace libcamera */
>>>> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
>>>> new file mode 100644
>>>> index 00000000..361b63bd
>>>> --- /dev/null
>>>> +++ b/src/ipa/libipa/helpers.h
>>>> @@ -0,0 +1,23 @@
>>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>>> +/*
>>>> + * Copyright (C) 2024, Ideas on Board Oy
>>>> + *
>>>> + * libipa miscellaneous helpers
>>>> + */
>>>> +
>>>> +#pragma once
>>>> +
>>>> +#include <stdint.h>
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +namespace ipa {
>>>> +
>>>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
>>>> +uint32_t estimateCCT(double red, double green, double blue);
>>>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);
>>>> +double fromQFormat(unsigned int value, unsigned int n);
>>>> +
>>>> +} /* namespace ipa */
>>>> +
>>>> +} /* namespace libcamera */
>>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>>>> index e78cbcd6..ca4f3e28 100644
>>>> --- a/src/ipa/libipa/meson.build
>>>> +++ b/src/ipa/libipa/meson.build
>>>> @@ -6,6 +6,7 @@ libipa_headers = files([
>>>>        'camera_sensor_helper.h',
>>>>        'exposure_mode_helper.h',
>>>>        'fc_queue.h',
>>>> +    'helpers.h',
>>>>        'histogram.h',
>>>>        'interpolator.h',
>>>>        'lsc_polynomial.h',
>>>> @@ -21,6 +22,7 @@ libipa_sources = files([
>>>>        'camera_sensor_helper.cpp',
>>>>        'exposure_mode_helper.cpp',
>>>>        'fc_queue.cpp',
>>>> +    'helpers.cpp',
>>>>        'histogram.cpp',
>>>>        'interpolator.cpp',
>>>>        'lsc_polynomial.cpp',
>>>> --
>>>> 2.30.2
>>>>
Kieran Bingham Nov. 6, 2024, 2:09 p.m. UTC | #5
Quoting Dan Scally (2024-11-06 14:01:25)
> Hi Kiean
> 
> On 06/11/2024 13:56, Kieran Bingham wrote:
> > Quoting Dan Scally (2024-11-06 09:39:09)
> >> Hi Jacopo - thanks for the review
> >>
> >> On 04/11/2024 11:07, Jacopo Mondi wrote:
> >>> Hi Dan
> >>>
> >>> On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:
> >>>> We start to have functions that are effectively identical crop up
> >>>> across the IPA modules. Add a file allowing those to be centralised
> >>>> within libipa so that a single implementation can be used in all of
> >>>> the IPAs.
> >>>>
> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >>>> ---
> >>>>    src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++
> >>>>    src/ipa/libipa/helpers.h   |  23 +++++++++
> >>>>    src/ipa/libipa/meson.build |   2 +
> >>>>    3 files changed, 128 insertions(+)
> >>>>    create mode 100644 src/ipa/libipa/helpers.cpp
> >>>>    create mode 100644 src/ipa/libipa/helpers.h
> >>>>
> >>>> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp
> >>>> new file mode 100644
> >>>> index 00000000..cc0cd586
> >>>> --- /dev/null
> >>>> +++ b/src/ipa/libipa/helpers.cpp
> >>> Not a huge fan of a collection of C functions, but I guess
> >>> alternatives could be considered over-design
> >> That was my thinking basically.
> > I agree here. I'm fine moving these to a common location to get started.
> >
> > If things grow - or become more specific then they could be separated to
> > a defined structure, but that could also be done on top ... I have one
> > example below for that...
> >
> >
> >>>> @@ -0,0 +1,103 @@
> >>>> +/* SPDX-License-Identifier: BSD-2-Clause */
> >>>> +/*
> >>>> + * Copyright (C) 2024, Ideas on Board Oy
> >>>> + *
> >>>> + * libipa miscellaneous helpers
> >>>> + */
> >>>> +
> >>>> +#include "helpers.h"
> >>>> +
> >>>> +#include <algorithm>
> >>>> +#include <cmath>
> >>>> +
> >>>> +namespace libcamera {
> >>>> +
> >>>> +namespace ipa {
> >>>> +
> >>>> +/**
> >>>> + * \file helpers.h
> >>>> + * \brief Functions to reduce code duplication between IPA modules
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Estimate luminance from RGB values following ITU-R BT.601
> >>>> + * \param[in] r The red value
> >>>> + * \param[in] g The green value
> >>>> + * \param[in] b The blue valye
> > s/valye/value/
> >
> >>>> + * \return The estimated luminance value
> >>> \return statements usually go after the longer function documentation
> >>
> >> Ack
> >>
> >>>
> >>>> + *
> >>>> + * This function estimates a luminance value from a triplet of Red, Green and
> >>>> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7
> >>>> + * which can be found at https://www.itu.int/rec/R-REC-BT.601
> >>>> + */
> >>>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)
> >>> In example, in the RPi IPA, the return value is assigned to a double,
> >>> and the calculation suggests it might be worth not casing the result to
> >>> unsigned int.
> >> You're right, that's wrong. Thanks.
> >>>> +{
> >>>> +    return (r * .299) + (g * .587) + (b * .114);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \brief Estimate correlated colour temperature from RGB color space input
> >>>> + * \param[in] red The input red value
> >>>> + * \param[in] green The input green value
> >>>> + * \param[in] blue The input blue value
> >>>> + * \return The estimated color temperature
> >>> same here
> >>>
> >>>> + *
> >>>> + * This function estimates the correlated color temperature RGB color space
> >>>> + * input. In physics and color science, the Planckian locus or black body locus
> >>>> + * is the path or locus that the color of an incandescent black body would take
> >>>> + * in a particular chromaticity space as the blackbody temperature changes.
> >>>> + *
> >>>> + * If a narrow range of color temperatures is considered (those encapsulating
> >>>> + * daylight being the most practical case) one can approximate the Planckian
> >>>> + * locus in order to calculate the CCT in terms of chromaticity coordinates.
> >>>> + *
> >>>> + * More detailed information can be found in:
> >>>> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation
> >>>> + */
> >>>> +uint32_t estimateCCT(double red, double green, double blue)
> >>>> +{
> >>>> +    /* Convert the RGB values to CIE tristimulus values (XYZ) */
> >>>> +    double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
> >>>> +    double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
> >>>> +    double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);
> >>>> +
> >>>> +    /* Calculate the normalized chromaticity values */
> >>>> +    double x = X / (X + Y + Z);
> >>>> +    double y = Y / (X + Y + Z);
> >>>> +
> >>>> +    /* Calculate CCT */
> >>>> +    double n = (x - 0.3320) / (0.1858 - y);
> >>>> +    return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
> >>>> +}
> >>> This seems to come straight from the IPU3 IPA, so ack to the text and
> >>> implementation
> > Perhaps those are both helpers for 'colorspace' so maybe this is a
> > colorspace.cpp ... but lets see...
> Yeah but then we get lots of little bitty filesĀ  - that seems worse to me.
> >
> >>>> +
> >>>> +/**
> >>>> + * \brief Convert double to Q(m.n) format
> >>> Note these two helpers seems to be un-used.
> >> Ah yes, they're used in the C55 series which will be based on top of this...I can introduce them
> >> there if that's better?
> >>> What Q formats are we referring to ? Wikipedia lists two (TI version
> >>> and ARM version).
> >>> https://en.wikipedia.org/wiki/Q_(number_format)
> >>
> >> So far I'm only using numbers specified in documentation as being unsigned, and so it's the TI
> >> version, but the "UQ" format rather than just "Q"...possibly renaming the function
> >> "toUnsignedQFormat()" would be clearer?
> >>
> >>>> + * \param[in] value The value to convert
> >>>> + * \param[in] m The number of bits used to represent the integer part
> >>>> + * \param[in] n The number of bits used to represent the fraction part
> >>>> + *
> >>>> + * This function converts a double into an unsigned Q format, clamping at the
> >>>> + * largest possible value given m and n.
> >>> \return ?
> >>>
> >>>> + */
> >>>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)
> >>>> +{
> >>>> +    double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);
> >>>> +
> >>>> +    return std::clamp(value, 0.0, maxVal) * std::pow(2, n);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \brief Convert Q(m.n) formatted number to double
> >>>> + * \param[in] value The value to convert
> >>>> + * \param[in] n The number of bits used to represent the fraction part
> >>>> + *
> >>>> + * This function converts an unsigned Q formatted value into a double.
> >>>> + */
> >>>> +double fromQFormat(unsigned int value, unsigned int n)
> >>>> +{
> >>>> +    return value / std::pow(2, n);
> >>>> +}
> >>> Let's discuss these once the above is clarified
> > Q(a.b) formats come up a lot. I'd always thought we need a FixedPoint
> > (templated?) class to help with this so we can consider it as a real
> > type.
> >
> > If so - it would be it's own class, and warrant a set of unit tests...
> >
> > So if you did want to split this helpers.cpp,h up - I'd move the color
> > space functionality to colorspace.cpp and Q helpers to fixedpoint.cpp
> > ...
> >
> > Ultimately it depends how far you want to take the cleanup...
> 
> 
> Well, where else have they come up that we have the conversion open-coded at the moment? It seems a 
> bit heavy handed for just the above, but if there're more places that could be tidied up perhaps 
> it'd be worthwhile

Good question, I don't know off the top of my head. My first though
would have been - C55 ... But in fact, I think they're used in dewarper
on i.MX8MP too.

It seems like the sort of thing we'd need a lot - but I'm absolutely
fine keeping this as is for now until there are more clear users.

--
Kieran


> 
> >
> >
> >
> >>>> +
> >>>> +} /* namespace ipa */
> >>>> +
> >>>> +} /* namespace libcamera */
> >>>> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
> >>>> new file mode 100644
> >>>> index 00000000..361b63bd
> >>>> --- /dev/null
> >>>> +++ b/src/ipa/libipa/helpers.h
> >>>> @@ -0,0 +1,23 @@
> >>>> +/* SPDX-License-Identifier: BSD-2-Clause */
> >>>> +/*
> >>>> + * Copyright (C) 2024, Ideas on Board Oy
> >>>> + *
> >>>> + * libipa miscellaneous helpers
> >>>> + */
> >>>> +
> >>>> +#pragma once
> >>>> +
> >>>> +#include <stdint.h>
> >>>> +
> >>>> +namespace libcamera {
> >>>> +
> >>>> +namespace ipa {
> >>>> +
> >>>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
> >>>> +uint32_t estimateCCT(double red, double green, double blue);
> >>>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);
> >>>> +double fromQFormat(unsigned int value, unsigned int n);
> >>>> +
> >>>> +} /* namespace ipa */
> >>>> +
> >>>> +} /* namespace libcamera */
> >>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> >>>> index e78cbcd6..ca4f3e28 100644
> >>>> --- a/src/ipa/libipa/meson.build
> >>>> +++ b/src/ipa/libipa/meson.build
> >>>> @@ -6,6 +6,7 @@ libipa_headers = files([
> >>>>        'camera_sensor_helper.h',
> >>>>        'exposure_mode_helper.h',
> >>>>        'fc_queue.h',
> >>>> +    'helpers.h',
> >>>>        'histogram.h',
> >>>>        'interpolator.h',
> >>>>        'lsc_polynomial.h',
> >>>> @@ -21,6 +22,7 @@ libipa_sources = files([
> >>>>        'camera_sensor_helper.cpp',
> >>>>        'exposure_mode_helper.cpp',
> >>>>        'fc_queue.cpp',
> >>>> +    'helpers.cpp',
> >>>>        'histogram.cpp',
> >>>>        'interpolator.cpp',
> >>>>        'lsc_polynomial.cpp',
> >>>> --
> >>>> 2.30.2
> >>>>

Patch
diff mbox series

diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp
new file mode 100644
index 00000000..cc0cd586
--- /dev/null
+++ b/src/ipa/libipa/helpers.cpp
@@ -0,0 +1,103 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2024, Ideas on Board Oy
+ *
+ * libipa miscellaneous helpers
+ */
+
+#include "helpers.h"
+
+#include <algorithm>
+#include <cmath>
+
+namespace libcamera {
+
+namespace ipa {
+
+/**
+ * \file helpers.h
+ * \brief Functions to reduce code duplication between IPA modules
+ */
+
+/**
+ * \brief Estimate luminance from RGB values following ITU-R BT.601
+ * \param[in] r The red value
+ * \param[in] g The green value
+ * \param[in] b The blue valye
+ * \return The estimated luminance value
+ *
+ * This function estimates a luminance value from a triplet of Red, Green and
+ * Blue values, following the formula defined by ITU-R Recommendation BT.601-7
+ * which can be found at https://www.itu.int/rec/R-REC-BT.601
+ */
+unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)
+{
+	return (r * .299) + (g * .587) + (b * .114);
+}
+
+/**
+ * \brief Estimate correlated colour temperature from RGB color space input
+ * \param[in] red The input red value
+ * \param[in] green The input green value
+ * \param[in] blue The input blue value
+ * \return The estimated color temperature
+ *
+ * This function estimates the correlated color temperature RGB color space
+ * input. In physics and color science, the Planckian locus or black body locus
+ * is the path or locus that the color of an incandescent black body would take
+ * in a particular chromaticity space as the blackbody temperature changes.
+ *
+ * If a narrow range of color temperatures is considered (those encapsulating
+ * daylight being the most practical case) one can approximate the Planckian
+ * locus in order to calculate the CCT in terms of chromaticity coordinates.
+ *
+ * More detailed information can be found in:
+ * https://en.wikipedia.org/wiki/Color_temperature#Approximation
+ */
+uint32_t estimateCCT(double red, double green, double blue)
+{
+	/* Convert the RGB values to CIE tristimulus values (XYZ) */
+	double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
+	double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
+	double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);
+
+	/* Calculate the normalized chromaticity values */
+	double x = X / (X + Y + Z);
+	double y = Y / (X + Y + Z);
+
+	/* Calculate CCT */
+	double n = (x - 0.3320) / (0.1858 - y);
+	return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
+}
+
+/**
+ * \brief Convert double to Q(m.n) format
+ * \param[in] value The value to convert
+ * \param[in] m The number of bits used to represent the integer part
+ * \param[in] n The number of bits used to represent the fraction part
+ *
+ * This function converts a double into an unsigned Q format, clamping at the
+ * largest possible value given m and n.
+ */
+unsigned int toQFormat(double value, unsigned int m, unsigned int n)
+{
+	double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);
+
+	return std::clamp(value, 0.0, maxVal) * std::pow(2, n);
+}
+
+/**
+ * \brief Convert Q(m.n) formatted number to double
+ * \param[in] value The value to convert
+ * \param[in] n The number of bits used to represent the fraction part
+ *
+ * This function converts an unsigned Q formatted value into a double.
+ */
+double fromQFormat(unsigned int value, unsigned int n)
+{
+	return value / std::pow(2, n);
+}
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
new file mode 100644
index 00000000..361b63bd
--- /dev/null
+++ b/src/ipa/libipa/helpers.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2024, Ideas on Board Oy
+ *
+ * libipa miscellaneous helpers
+ */
+
+#pragma once
+
+#include <stdint.h>
+
+namespace libcamera {
+
+namespace ipa {
+
+unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
+uint32_t estimateCCT(double red, double green, double blue);
+unsigned int toQFormat(double value, unsigned int m, unsigned int n);
+double fromQFormat(unsigned int value, unsigned int n);
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
index e78cbcd6..ca4f3e28 100644
--- a/src/ipa/libipa/meson.build
+++ b/src/ipa/libipa/meson.build
@@ -6,6 +6,7 @@  libipa_headers = files([
     'camera_sensor_helper.h',
     'exposure_mode_helper.h',
     'fc_queue.h',
+    'helpers.h',
     'histogram.h',
     'interpolator.h',
     'lsc_polynomial.h',
@@ -21,6 +22,7 @@  libipa_sources = files([
     'camera_sensor_helper.cpp',
     'exposure_mode_helper.cpp',
     'fc_queue.cpp',
+    'helpers.cpp',
     'histogram.cpp',
     'interpolator.cpp',
     'lsc_polynomial.cpp',