[PATCH] libcamera: software_isp: Add saturation control
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Apr 14 15:38:42 CEST 2025
Quoting Milan Zamazal (2025-04-14 14:15:38)
> 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).
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.
>
> > 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...
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 ?
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?)
--
Kieran
> >
> >> }
> >>
> >> 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