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

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

Commit Message

Dan Scally Nov. 7, 2024, 10:25 a.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>
---
Changes in v2:

	- Dropped the Q(m, n) format helpers until they're needed.
	- \return statements after long description
	- Switched rec601LuminanceFromRGB() to return a double

 src/ipa/libipa/helpers.cpp | 77 ++++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/helpers.h   | 21 +++++++++++
 src/ipa/libipa/meson.build |  2 +
 3 files changed, 100 insertions(+)
 create mode 100644 src/ipa/libipa/helpers.cpp
 create mode 100644 src/ipa/libipa/helpers.h

Comments

Jacopo Mondi Nov. 8, 2024, 3:18 p.m. UTC | #1 
Hi Dan

On Thu, Nov 07, 2024 at 10:25:03AM +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>
> ---
> Changes in v2:
>
> 	- Dropped the Q(m, n) format helpers until they're needed.
> 	- \return statements after long description
> 	- Switched rec601LuminanceFromRGB() to return a double
>
>  src/ipa/libipa/helpers.cpp | 77 ++++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/helpers.h   | 21 +++++++++++
>  src/ipa/libipa/meson.build |  2 +
>  3 files changed, 100 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..6c038895
> --- /dev/null
> +++ b/src/ipa/libipa/helpers.cpp
> @@ -0,0 +1,77 @@
> +/* 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 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
> + *
> + * \return The estimated luminance value
> + */
> +double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)

Is passing arguments in as integers correct or will result in a lost
of precision ?

I see in the next patch the IPU3 IPA is converted to use this helper

double Agc::estimateLuminance(double gain) const
{
	double redSum = 0, greenSum = 0, blueSum = 0;

	for (unsigned int i = 0; i < rgbTriples_.size(); i++) {
		redSum += std::min(std::get<0>(rgbTriples_[i]) * gain, 255.0);
		greenSum += std::min(std::get<1>(rgbTriples_[i]) * gain, 255.0);
		blueSum += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0);
	}


-       double ySum = redSum * rGain_ * 0.299
-                   + greenSum * gGain_ * 0.587
-                   + blueSum * bGain_ * 0.114;
+	double ySum = rec601LuminanceFromRGB(redSum * rGain_,
+					     greenSum * gGain_,
+					     blueSum * bGain_);

	return ySum / (bdsGrid_.height * bdsGrid_.width) / 255;
}

Now redSum * rGain_ (and greenSum * gGain_ and blueSum * bGain_)
are indeed doubles. If you pass them in as integers I'm afraid you're
implicitly converting and rounding them. I made a little test program

double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)
{
	return (r * .299) + (g * .587) + (b * .114);
}

double rec601LuminanceFromRGBDouble(double r, double g, double b)
{
	return (r * .299) + (g * .587) + (b * .114);
}

int main()
{
	double r = 1.234;
	double g = 2.345;
	double b = 3.456;

	std::cout << rec601LuminanceFromRGB(r, g, b) << std::endl;
	std::cout << rec601LuminanceFromRGBDouble(r, g, b) << std::endl;

	return 0;
}

And indeed it prints different results

$ ./a.out
1.815
2.13946

Have you checked in the IPU3 IPA if the results of the computation
are the same ?

> +{
> +	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
> + *
> + * 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
> + *
> + * \return The estimated color temperature
> + */
> +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;
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
> new file mode 100644
> index 00000000..51c74a36
> --- /dev/null
> +++ b/src/ipa/libipa/helpers.h
> @@ -0,0 +1,21 @@
> +/* 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 {
> +
> +double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
> +uint32_t estimateCCT(double red, double green, double blue);
> +
> +} /* 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
>
Laurent Pinchart Nov. 12, 2024, 8:07 a.m. UTC | #2 
Hi Dan,

Thank you for the patch.

On Thu, Nov 07, 2024 at 10:25:03AM +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>
> ---
> Changes in v2:
> 
> 	- Dropped the Q(m, n) format helpers until they're needed.
> 	- \return statements after long description
> 	- Switched rec601LuminanceFromRGB() to return a double
> 
>  src/ipa/libipa/helpers.cpp | 77 ++++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/helpers.h   | 21 +++++++++++

Could we call this colours.{cpp,h} ? I'd like to keep libipa organized,
and add a generic "helpers" file only if we really can classify the
helpers in any meaningful way. I expect we'll get more colour-related
helpers in the future.

>  src/ipa/libipa/meson.build |  2 +
>  3 files changed, 100 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..6c038895
> --- /dev/null
> +++ b/src/ipa/libipa/helpers.cpp
> @@ -0,0 +1,77 @@
> +/* 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 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
> + *
> + * \return The estimated luminance value
> + */
> +double 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
> + *
> + * 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
> + *
> + * \return The estimated color temperature
> + */
> +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;
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
> new file mode 100644
> index 00000000..51c74a36
> --- /dev/null
> +++ b/src/ipa/libipa/helpers.h
> @@ -0,0 +1,21 @@
> +/* 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 {
> +
> +double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
> +uint32_t estimateCCT(double red, double green, double blue);
> +
> +} /* 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',
Dan Scally Nov. 13, 2024, 10:46 a.m. UTC | #3 
Hello Laurent

On 12/11/2024 08:07, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Thu, Nov 07, 2024 at 10:25:03AM +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>
>> ---
>> Changes in v2:
>>
>> 	- Dropped the Q(m, n) format helpers until they're needed.
>> 	- \return statements after long description
>> 	- Switched rec601LuminanceFromRGB() to return a double
>>
>>   src/ipa/libipa/helpers.cpp | 77 ++++++++++++++++++++++++++++++++++++++
>>   src/ipa/libipa/helpers.h   | 21 +++++++++++
> Could we call this colours.{cpp,h} ? I'd like to keep libipa organized,
> and add a generic "helpers" file only if we really can classify the
> helpers in any meaningful way. I expect we'll get more colour-related
> helpers in the future.


Okedokey - will do.

>
>>   src/ipa/libipa/meson.build |  2 +
>>   3 files changed, 100 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..6c038895
>> --- /dev/null
>> +++ b/src/ipa/libipa/helpers.cpp
>> @@ -0,0 +1,77 @@
>> +/* 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 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
>> + *
>> + * \return The estimated luminance value
>> + */
>> +double 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
>> + *
>> + * 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
>> + *
>> + * \return The estimated color temperature
>> + */
>> +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;
>> +}
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
>> new file mode 100644
>> index 00000000..51c74a36
>> --- /dev/null
>> +++ b/src/ipa/libipa/helpers.h
>> @@ -0,0 +1,21 @@
>> +/* 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 {
>> +
>> +double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
>> +uint32_t estimateCCT(double red, double green, double blue);
>> +
>> +} /* 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',
Dan Scally Nov. 14, 2024, 10:25 p.m. UTC | #4 
Hi Jacopo

On 08/11/2024 15:18, Jacopo Mondi wrote:
> Hi Dan
>
> On Thu, Nov 07, 2024 at 10:25:03AM +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>
>> ---
>> Changes in v2:
>>
>> 	- Dropped the Q(m, n) format helpers until they're needed.
>> 	- \return statements after long description
>> 	- Switched rec601LuminanceFromRGB() to return a double
>>
>>   src/ipa/libipa/helpers.cpp | 77 ++++++++++++++++++++++++++++++++++++++
>>   src/ipa/libipa/helpers.h   | 21 +++++++++++
>>   src/ipa/libipa/meson.build |  2 +
>>   3 files changed, 100 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..6c038895
>> --- /dev/null
>> +++ b/src/ipa/libipa/helpers.cpp
>> @@ -0,0 +1,77 @@
>> +/* 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 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
>> + *
>> + * \return The estimated luminance value
>> + */
>> +double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)
> Is passing arguments in as integers correct or will result in a lost
> of precision ?
>
> I see in the next patch the IPU3 IPA is converted to use this helper
>
> double Agc::estimateLuminance(double gain) const
> {
> 	double redSum = 0, greenSum = 0, blueSum = 0;
>
> 	for (unsigned int i = 0; i < rgbTriples_.size(); i++) {
> 		redSum += std::min(std::get<0>(rgbTriples_[i]) * gain, 255.0);
> 		greenSum += std::min(std::get<1>(rgbTriples_[i]) * gain, 255.0);
> 		blueSum += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0);
> 	}
>
>
> -       double ySum = redSum * rGain_ * 0.299
> -                   + greenSum * gGain_ * 0.587
> -                   + blueSum * bGain_ * 0.114;
> +	double ySum = rec601LuminanceFromRGB(redSum * rGain_,
> +					     greenSum * gGain_,
> +					     blueSum * bGain_);
>
> 	return ySum / (bdsGrid_.height * bdsGrid_.width) / 255;
> }
>
> Now redSum * rGain_ (and greenSum * gGain_ and blueSum * bGain_)
> are indeed doubles. If you pass them in as integers I'm afraid you're
> implicitly converting and rounding them. I made a little test program
>
> double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)
> {
> 	return (r * .299) + (g * .587) + (b * .114);
> }
>
> double rec601LuminanceFromRGBDouble(double r, double g, double b)
> {
> 	return (r * .299) + (g * .587) + (b * .114);
> }
>
> int main()
> {
> 	double r = 1.234;
> 	double g = 2.345;
> 	double b = 3.456;
>
> 	std::cout << rec601LuminanceFromRGB(r, g, b) << std::endl;
> 	std::cout << rec601LuminanceFromRGBDouble(r, g, b) << std::endl;
>
> 	return 0;
> }
>
> And indeed it prints different results
>
> $ ./a.out
> 1.815
> 2.13946
>
> Have you checked in the IPU3 IPA if the results of the computation
> are the same ?


You are right of course - I hadn't checked in the IPU3 one carefully enough, for some reason I 
thought they were all the same. I'll use doubles across the board so there's no rounding.

>> +{
>> +	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
>> + *
>> + * 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
>> + *
>> + * \return The estimated color temperature
>> + */
>> +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;
>> +}
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
>> new file mode 100644
>> index 00000000..51c74a36
>> --- /dev/null
>> +++ b/src/ipa/libipa/helpers.h
>> @@ -0,0 +1,21 @@
>> +/* 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 {
>> +
>> +double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
>> +uint32_t estimateCCT(double red, double green, double blue);
>> +
>> +} /* 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..6c038895
--- /dev/null
+++ b/src/ipa/libipa/helpers.cpp
@@ -0,0 +1,77 @@ 
+/* 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 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
+ *
+ * \return The estimated luminance value
+ */
+double 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
+ *
+ * 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
+ *
+ * \return The estimated color temperature
+ */
+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;
+}
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h
new file mode 100644
index 00000000..51c74a36
--- /dev/null
+++ b/src/ipa/libipa/helpers.h
@@ -0,0 +1,21 @@ 
+/* 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 {
+
+double rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);
+uint32_t estimateCCT(double red, double green, double blue);
+
+} /* 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',