[PATCH 1/6] ipa: libipa: Add miscellaneous helpers
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Nov 6 15:09:49 CET 2024
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 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
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
> >>>>
More information about the libcamera-devel
mailing list