[PATCH] libcamera: software_isp: Add saturation control
Milan Zamazal
mzamazal at redhat.com
Tue Apr 29 17:37:39 CEST 2025
Hi Laurent,
Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> On Mon, Apr 14, 2025 at 04:49:58PM +0200, Milan Zamazal wrote:
>> Kieran Bingham writes:
>> > Quoting Milan Zamazal (2025-04-14 14:15:38)
>
>> >> Kieran Bingham writes:
>> >> > Quoting Milan Zamazal (2025-04-07 09:58:02)
>> >> >> Saturation control is added on top of the colour correction matrix. A
>> >> >> way of saturation adjustment that can be fully integrated into the
>> >> >> colour correction matrix is used. The control is available only if Ccm
>> >> >> algorithm is enabled.
>> >> >>
>> >> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified
>> >> >> saturation, 0.0 full desaturation and 2.0 quite saturated.
>> >> >>
>> >> >> The matrix for saturation adjustment is taken from
>> >> >> https://www.graficaobscura.com/matrix/index.html. The colour correction
>> >> >> matrix is applied before gamma and the given matrix is suitable for such
>> >> >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp
>> >> >> could be used.
>> >> >>
>> >> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> >> >> ---
>> >> >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++--
>> >> >> src/ipa/simple/algorithms/ccm.h | 11 ++++++
>> >> >> src/ipa/simple/ipa_context.h | 4 +++
>> >> >> 3 files changed, 69 insertions(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
>> >> >> index d5ba928d..2700a247 100644
>> >> >> --- a/src/ipa/simple/algorithms/ccm.cpp
>> >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp
>> >> >> @@ -3,7 +3,7 @@
>> >> >> * Copyright (C) 2024, Ideas On Board
>> >> >> * Copyright (C) 2024-2025, Red Hat Inc.
>> >> >> *
>> >> >> - * Color correction matrix
>> >> >> + * Color correction matrix + saturation
>> >> >> */
>> >> >>
>> >> >> #include "ccm.h"
>> >> >> @@ -13,6 +13,8 @@
>> >> >>
>> >> >> #include <libcamera/control_ids.h>
>> >> >>
>> >> >> +#include "libcamera/internal/matrix.h"
>> >> >> +
>> >> >> namespace {
>> >> >>
>> >> >> constexpr unsigned int kTemperatureThreshold = 100;
>> >> >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
>> >> >> }
>> >> >>
>> >> >> context.ccmEnabled = true;
>> >> >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);
>> >> >> +
>> >> >> + return 0;
>> >> >> +}
>> >> >> +
>> >> >> +int Ccm::configure(IPAContext &context,
>> >> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)
>> >> >> +{
>> >> >> + context.activeState.knobs.saturation = std::optional<double>();
>> >> >>
>> >> >> return 0;
>> >> >> }
>> >> >>
>> >> >> +void Ccm::queueRequest(typename Module::Context &context,
>> >> >> + [[maybe_unused]] const uint32_t frame,
>> >> >> + [[maybe_unused]] typename Module::FrameContext &frameContext,
>> >> >> + const ControlList &controls)
>> >> >> +{
>> >> >> + const auto &saturation = controls.get(controls::Saturation);
>> >> >> + if (saturation.has_value()) {
>> >> >> + context.activeState.knobs.saturation = saturation;
>> >> >> + LOG(IPASoftCcm, Debug) << "Setting saturation to " << saturation.value();
>> >> >> + }
>> >> >> +}
>> >> >> +
>> >> >> +void Ccm::updateSaturation(Matrix<float, 3, 3> &ccm, float saturation)
>> >> >> +{
>> >> >> + /*
>> >> >> + * See https://www.graficaobscura.com/matrix/index.html.
>> >> >
>> >> > Looks like it would be easy to add a brightness control too after this.
>> >>
>> >> Yes, when CCM is used, it's easy to add brightness. And when CCM is not
>> >> used, it's easy too, applying it when making the LUT values -- this is
>> >> important for CPU ISP when CCM is not used (having to use CCM just to
>> >> adjust brightness would be overkill).
>> >
>> > Aha, then indeed lets just apply it directly to the LUT (later) :-)
>> >
>> >> >> + * This is applied before gamma thus a matrix for linear RGB must be used.
>> >> >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0
>> >> >> + * no saturation (monochrome).
>> >> >> + */
>> >> >> + constexpr float r = 0.3086;
>> >> >> + constexpr float g = 0.6094;
>> >> >> + constexpr float b = 0.0820;
>> >> >
>> >> > it would be interesting to dig into where these numbers are derived
>> >> > from exactly ...
>> >>
>> >> I guess some educated magic, similarly to other luminance related
>> >> conversions.
>
> There are multiple ways to apply saturation to an RGB value:
>
> - Convert to HSV, apply a factor to the saturation, and convert back to
> RGB. This is rarely (if at all) used in hardware as far as I know.
> Software implementations are computationally expensive and can't be
> expressed by a simple matrix multiplication.
>
> - Convert to Y'CbCr, apply a factor to the Cb and Cr components, and
> convert back to RGB. This method is more commonly used in ISPs. As
> they often output YUV data, the conversion to Y'CbCr is already
> present in the pipeline. Note that the saturation is applied in
> non-linear space in this case.
>
> Assuming a CSC matrix that converts from RGB to YUV, a saturation
> factor 'sat' can be applied on RGB data by multiplying the RGB vector
> by
>
> [ 1.0 0.0 0.0 ]
> CSC^-1 * [ 0.0 sat 0.0 ] * CSC
> [ 0.0 0.0 sat ]
I initially thought about this, but the way below looked simpler. Well,
actually, the Y'CbCr method is not more complicated and it works for me.
Except that I don't know which of the ITU-R BT.* conversions is best to
use in libcamera. Is there an established practice already?
> - Desaturating the image completely by replacing the R, G and B
> components by a luminance value, and interpolating linearly between
> the saturated and non-saturated pixel values.
>
> I'm also not aware of this method being commonly used in hardware.
> Software implementations are less costly thatn HSV-based saturation as
> they can be expressed by a matrix multiplication. This is what
> https://www.graficaobscura.com/matrix/index.html does.
>
> Can we express the math below more explicitly, based on either the
> Y'CbCr method or the interpolation method ? Magic values should also be
> explained.
I'm afraid I'm completely ignorant about the math behind the conversions
and I don't know what's the primary ultimate source. But I believe the
Y'CbCr methods are well established, "ITU-R BT" looks like a good enough
standard body behind to accept the values without further questioning.
>> >> > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms
>> >> > seems to report others have looked, but I don't see a direct answer yet
>> >> > (but I won't dig deep) ...
>> >> >
>> >> > And the referenced article is quite helpful.
>> >> >
>> >> > As this is the 'luminance' vector - I wonder if writing it as
>> >> > RGB<float> luminance = { 0.3086, 0.6094, 0.0820 };
>> >> >
>> >> > makes sense...
>> >> >
>> >> >> + const float s1 = 1.0 - saturation;
>> >> >> + ccm = ccm * Matrix<float, 3, 3>{ { s1 * r + saturation, s1 * g, s1 * b,
>> >> >> + s1 * r, s1 * g + saturation, s1 * b,
>> >> >> + s1 * r, s1 * g, s1 * b + saturation } };
>>
>> This multiplication should be also performed in the reverse order.
>>
>> >> >
>> >> > but that would become much more terse ... so I'm not sure it's worth it
>> >> > here? Or would it then be more explicitly readable like the
>> >> > applySaturation in RPi?
>> >>
>> >> I'll try and let's see.
>> >>
>> >> >> +}
>> >> >> +
>> >> >> void Ccm::prepare(IPAContext &context, const uint32_t frame,
>> >> >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>> >> >> {
>> >> >> + auto &saturation = context.activeState.knobs.saturation;
>> >> >> +
>> >> >> const unsigned int ct = context.activeState.awb.temperatureK;
>> >> >>
>> >> >> - /* Change CCM only on bigger temperature changes. */
>> >> >> + /* Change CCM only on saturation or bigger temperature changes. */
>> >> >> if (frame > 0 &&
>> >> >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {
>> >> >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&
>> >> >> + saturation == lastSaturation_) {
>> >> >> frameContext.ccm.ccm = context.activeState.ccm.ccm;
>> >> >> context.activeState.ccm.changed = false;
>> >> >> return;
>> >> >> }
>> >> >>
>> >> >> lastCt_ = ct;
>> >> >> + lastSaturation_ = saturation;
>> >> >> Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);
>> >> >> + if (saturation)
>> >> >> + updateSaturation(ccm, saturation.value());
>> >> >>
>> >> >> context.activeState.ccm.ccm = ccm;
>> >> >> frameContext.ccm.ccm = ccm;
>> >> >> + frameContext.saturation = saturation;
>> >> >> context.activeState.ccm.changed = true;
>> >> >> }
>> >> >>
>> >> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context,
>> >> >> ControlList &metadata)
>> >> >> {
>> >> >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());
>> >> >> +
>> >> >> + const auto &saturation = frameContext.saturation;
>> >> >> + if (saturation)
>> >> >> + metadata.set(controls::Saturation, saturation.value());
>> >> >
>> >> > This matches what we currently do for Contrast, but I wonder if we
>> >> > should report a metadata of '1.0' for both Contrast and Saturation if
>> >> > they aren't set ... as that's what the 'state' is/should be ?
>> >>
>> >> I don't have an opinion on this, both of "no saturation request -> no
>> >> metadata" and "no saturation request -> the corresponding default value
>> >> in metadata" make sense to me.
>
> As controls persist, we should keep reporting the value. Every frame
> should be entirely described by its metadata.
OK.
>> >> > Anyway, I wouldn't block this patch on that - as it would be on top for
>> >> > both controls...
>> >> >
>> >> > Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> >> >
>> >> > Though, while it's functional - looking at the resulting image in
>> >> > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' -
>> >> > with all pixels having roughly the same r=g=b - and I don't think that's
>> >> > the case ... It looks like there's definitely a bias against green. It
>> >> > looks like R=B, but G is always slightly less by at least ~20% ? ...
>> >> > leaving the image looking slightly tinted purple.
>> >>
>> >> Oh, now, under daylight, I get a prominent purple tint too.
>> >>
>> >> > That could be because I'm running an untuned/identity matrix for CCM
>> >> > perhaps? or is it because white balance needs to be taken into account ?
>> >>
>> >> No, it's a math mistake I did in CCM patches. White balance must be
>> >> applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix,
>> >> and P the pixel RGB vector. I thought "let's apply C after W" and
>> >> computed (W * C) * P. Which is wrong, to apply C after W means
>> >> C * (W * P) = (C * W) * P. The last matrix applied must be the first
>> >> one in the multiplication. When I swap the multiplication in
>> >> src/ipa/simple/algorithms/lut.cpp from
>> >>
>> >> auto ccm = gainCcm * context.activeState.ccm.ccm;
>> >>
>> >> to
>> >>
>> >> auto ccm = context.activeState.ccm.ccm * gainCcm;
>> >>
>> >> it gets desaturated as it should be (there cannot be any colour cast
>> >> when desaturation is applied correctly, regardless of sensor
>> >> calibration). Maybe this will also explain and improve the colour casts
>> >> I previously experienced when experimenting with my sensor CCMs from
>> >> other pipelines.
>> >>
>> >> > But G = (RB-20%) doesn't sound like white balance to me at the moment...
>> >
>> > aha, so maybe it is ;-)
>> >
>> > Great, well if that's explained ... we have a path forwards. Does it
>> > impact this patch much? Would it help to merge this first? or fix the
>> > CCM then rebase this one ?
>>
>> The latter. I'll post the CCM fix separately. The saturation patch
>> needs a multiplication order fix too and I'd like to simplify
>> updateSaturation() a bit; I'll post v2.
>
> Agreed, fixing the CCM first is better.
Done in the meantime.
>> Then there is a question what to do about saturation + contrast. I
>> believe contrast should be applied between sensor-CCM and saturation but
>> this would be too expensive on a CPU.
>
> Contrast is typically applied at the end of the processing pipeline, in
> non-linear YUV space, at the same time as saturation.
But (a reasonable) contrast adjustment is still non-linear, right? Then
we have
CCM -> RGB2YCbCr -> contrast + saturation -> YCbCr2RGB
that cannot be collapsed into a single matrix multiplication.
> Once we will have a GPU-accelerated implementation of the software ISP,
> applying multiple matrix multiplications in different stages of the
> pipeline would become much cheaper. I know this may be a controversial
> opinion, but I think it will be important to have a CPU-based
> implementation that matches the same processing pipeline as the
> GPU-accelerated implementation, even if it gets costly for the CPU.
There can still be a need to run a camera on systems without working
GPU. Then it would be odd to have a CPU implementation running
unnecessarily slow on purpose. Perhaps we can decide what can be run
reasonably on CPU and have a configuration option to switch between a
reference and efficient implementations?
>> > I think the colour tint is the only thing stopping me from adding a tag,
>> > and if that's clarified, then for Saturation:
>> >
>> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> >
>> > If you respin with the calculation split out in Ccm::updateSaturation()
>> > you can still collect the tag I think. (perhaps there's a new series
>> > with the above change and the color fix on the way?)
>> >
>> >> >> }
>> >> >>
>> >> >> REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
>> >> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h
>> >> >> index f4e2b85b..2c5d2170 100644
>> >> >> --- a/src/ipa/simple/algorithms/ccm.h
>> >> >> +++ b/src/ipa/simple/algorithms/ccm.h
>> >> >> @@ -7,6 +7,8 @@
>> >> >>
>> >> >> #pragma once
>> >> >>
>> >> >> +#include <optional>
>> >> >> +
>> >> >> #include "libcamera/internal/matrix.h"
>> >> >>
>> >> >> #include <libipa/interpolator.h>
>> >> >> @@ -24,6 +26,12 @@ public:
>> >> >> ~Ccm() = default;
>> >> >>
>> >> >> int init(IPAContext &context, const YamlObject &tuningData) override;
>> >> >> + int configure(IPAContext &context,
>> >> >> + const IPAConfigInfo &configInfo) override;
>> >> >> + void queueRequest(typename Module::Context &context,
>> >> >> + const uint32_t frame,
>> >> >> + typename Module::FrameContext &frameContext,
>> >> >> + const ControlList &controls) override;
>> >> >> void prepare(IPAContext &context,
>> >> >> const uint32_t frame,
>> >> >> IPAFrameContext &frameContext,
>> >> >> @@ -34,7 +42,10 @@ public:
>> >> >> ControlList &metadata) override;
>> >> >>
>> >> >> private:
>> >> >> + void updateSaturation(Matrix<float, 3, 3> &ccm, float saturation);
>> >> >> +
>> >> >> unsigned int lastCt_;
>> >> >> + std::optional<float> lastSaturation_;
>> >> >> Interpolator<Matrix<float, 3, 3>> ccm_;
>> >> >> };
>> >> >>
>> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> >> >> index 88cc6c35..56792981 100644
>> >> >> --- a/src/ipa/simple/ipa_context.h
>> >> >> +++ b/src/ipa/simple/ipa_context.h
>> >> >> @@ -63,6 +63,7 @@ struct IPAActiveState {
>> >> >> struct {
>> >> >> /* 0..2 range, 1.0 = normal */
>> >> >> std::optional<double> contrast;
>> >> >> + std::optional<double> saturation;
>> >> >> } knobs;
>> >> >> };
>> >> >>
>> >> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext {
>> >> >> int32_t exposure;
>> >> >> double gain;
>> >> >> } sensor;
>> >> >> +
>> >> >> struct {
>> >> >> double red;
>> >> >> double blue;
>> >> >> } gains;
>> >> >> +
>> >> >> std::optional<double> contrast;
>> >> >> + std::optional<double> saturation;
>> >> >> };
>> >> >>
>> >> >> struct IPAContext {
More information about the libcamera-devel
mailing list