[PATCH] libcamera: software_isp: Add saturation control
Kieran Bingham
kieran.bingham at ideasonboard.com
Sat Apr 12 20:30:11 CEST 2025
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.
> + * 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 ...
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?
> +}
> +
> 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 ?
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.
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 ?
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