[PATCH 1/6] ipa: libipa: Add miscellaneous helpers

[Dan Scally]

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 at 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
>>>>