[PATCH] libcamera: software_isp: Add saturation control
Milan Zamazal
mzamazal at redhat.com
Mon Apr 14 15:15:38 CEST 2025
Hi Kieran,
thank you for review.
Kieran Bingham <kieran.bingham at ideasonboard.com> 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).
>> + * 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.
> 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 } };
>
> 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.
> 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...
>
>> }
>>
>> 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 {
>> --
>> 2.49.0
>>
More information about the libcamera-devel
mailing list