[{"id":33936,"web_url":"https://patchwork.libcamera.org/comment/33936/","msgid":"<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>","date":"2025-04-12T18:30:11","subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-04-07 09:58:02)\n> Saturation control is added on top of the colour correction matrix. A\n> way of saturation adjustment that can be fully integrated into the\n> colour correction matrix is used. The control is available only if Ccm\n> algorithm is enabled.\n> \n> The control uses 0.0-2.0 value range, with 1.0 being unmodified\n> saturation, 0.0 full desaturation and 2.0 quite saturated.\n> \n> The matrix for saturation adjustment is taken from\n> https://www.graficaobscura.com/matrix/index.html. The colour correction\n> matrix is applied before gamma and the given matrix is suitable for such\n> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp\n> could be used.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++--\n> src/ipa/simple/algorithms/ccm.h | 11 ++++++\n> src/ipa/simple/ipa_context.h | 4 +++\n> 3 files changed, 69 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> index d5ba928d..2700a247 100644\n> --- a/src/ipa/simple/algorithms/ccm.cpp\n> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> @@ -3,7 +3,7 @@\n> * Copyright (C) 2024, Ideas On Board\n> * Copyright (C) 2024-2025, Red Hat Inc.\n> *\n> - * Color correction matrix\n> + * Color correction matrix + saturation\n> */\n> \n> #include \"ccm.h\"\n> @@ -13,6 +13,8 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/matrix.h\"\n> +\n> namespace {\n> \n> constexpr unsigned int kTemperatureThreshold = 100;\n> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> }\n> \n> context.ccmEnabled = true;\n> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n> +\n> + return 0;\n> +}\n> +\n> +int Ccm::configure(IPAContext &context,\n> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n> +{\n> + context.activeState.knobs.saturation = std::optional();\n> \n> return 0;\n> }\n> \n> +void Ccm::queueRequest(typename Module::Context &context,\n> + [[maybe_unused]] const uint32_t frame,\n> + [[maybe_unused]] typename Module::FrameContext &frameContext,\n> + const ControlList &controls)\n> +{\n> + const auto &saturation = controls.get(controls::Saturation);\n> + if (saturation.has_value()) {\n> + context.activeState.knobs.saturation = saturation;\n> + LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n> + }\n> +}\n> +\n> +void Ccm::updateSaturation(Matrix &ccm, float saturation)\n> +{\n> + /*\n> + * See https://www.graficaobscura.com/matrix/index.html.\n\nLooks like it would be easy to add a brightness control too after this.\n\n> + * This is applied before gamma thus a matrix for linear RGB must be used.\n> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0\n> + * no saturation (monochrome).\n> + */\n> + constexpr float r = 0.3086;\n> + constexpr float g = 0.6094;\n> + constexpr float b = 0.0820;\n\nit would be interesting to dig into where these numbers are derived\nfrom exactly ...\n\nhttps://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms\nseems to report others have looked, but I don't see a direct answer yet\n(but I won't dig deep) ...\n\nAnd the referenced article is quite helpful.\n\nAs this is the 'luminance' vector - I wonder if writing it as \n\tRGB luminance = { 0.3086, 0.6094, 0.0820 };\n\nmakes sense...\n\n> + const float s1 = 1.0 - saturation;\n> + ccm = ccm * Matrix{ { s1 * r + saturation, s1 * g, s1 * b,\n> + s1 * r, s1 * g + saturation, s1 * b,\n> + s1 * r, s1 * g, s1 * b + saturation } };\n\nbut that would become much more terse ... so I'm not sure it's worth it\nhere? Or would it then be more explicitly readable like the\napplySaturation in RPi?\n\n\t\n\n> +}\n> +\n> void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> {\n> + auto &saturation = context.activeState.knobs.saturation;\n> +\n> const unsigned int ct = context.activeState.awb.temperatureK;\n> \n> - /* Change CCM only on bigger temperature changes. */\n> + /* Change CCM only on saturation or bigger temperature changes. */\n> if (frame > 0 &&\n> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n> + saturation == lastSaturation_) {\n> frameContext.ccm.ccm = context.activeState.ccm.ccm;\n> context.activeState.ccm.changed = false;\n> return;\n> }\n> \n> lastCt_ = ct;\n> + lastSaturation_ = saturation;\n> Matrix ccm = ccm_.getInterpolated(ct);\n> + if (saturation)\n> + updateSaturation(ccm, saturation.value());\n> \n> context.activeState.ccm.ccm = ccm;\n> frameContext.ccm.ccm = ccm;\n> + frameContext.saturation = saturation;\n> context.activeState.ccm.changed = true;\n> }\n> \n> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n> ControlList &metadata)\n> {\n> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());\n> +\n> + const auto &saturation = frameContext.saturation;\n> + if (saturation)\n> + metadata.set(controls::Saturation, saturation.value());\n\nThis matches what we currently do for Contrast, but I wonder if we\nshould report a metadata of '1.0' for both Contrast and Saturation if\nthey aren't set ... as that's what the 'state' is/should be ?\n\nAnyway, I wouldn't block this patch on that - as it would be on top for\nboth controls...\n\nTested-by: Kieran Bingham \n\nThough, while it's functional - looking at the resulting image in\ncamshark, I think I would expect a saturation of 0.0 to be 'greyscale' -\nwith all pixels having roughly the same r=g=b - and I don't think that's\nthe case ... It looks like there's definitely a bias against green. It\nlooks like R=B, but G is always slightly less by at least ~20% ? ...\nleaving the image looking slightly tinted purple.\n\nThat could be because I'm running an untuned/identity matrix for CCM\nperhaps? or is it because white balance needs to be taken into account ?\n\nBut G = (RB-20%) doesn't sound like white balance to me at the moment...\n\n> }\n> \n> REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> index f4e2b85b..2c5d2170 100644\n> --- a/src/ipa/simple/algorithms/ccm.h\n> +++ b/src/ipa/simple/algorithms/ccm.h\n> @@ -7,6 +7,8 @@\n> \n> #pragma once\n> \n> +#include \n> +\n> #include \"libcamera/internal/matrix.h\"\n> \n> #include \n> @@ -24,6 +26,12 @@ public:\n> ~Ccm() = default;\n> \n> int init(IPAContext &context, const YamlObject &tuningData) override;\n> + int configure(IPAContext &context,\n> + const IPAConfigInfo &configInfo) override;\n> + void queueRequest(typename Module::Context &context,\n> + const uint32_t frame,\n> + typename Module::FrameContext &frameContext,\n> + const ControlList &controls) override;\n> void prepare(IPAContext &context,\n> const uint32_t frame,\n> IPAFrameContext &frameContext,\n> @@ -34,7 +42,10 @@ public:\n> ControlList &metadata) override;\n> \n> private:\n> + void updateSaturation(Matrix &ccm, float saturation);\n> +\n> unsigned int lastCt_;\n> + std::optional lastSaturation_;\n> Interpolator> ccm_;\n> };\n> \n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index 88cc6c35..56792981 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -63,6 +63,7 @@ struct IPAActiveState {\n> struct {\n> /* 0..2 range, 1.0 = normal */\n> std::optional contrast;\n> + std::optional saturation;\n> } knobs;\n> };\n> \n> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext {\n> int32_t exposure;\n> double gain;\n> } sensor;\n> +\n> struct {\n> double red;\n> double blue;\n> } gains;\n> +\n> std::optional contrast;\n> + std::optional saturation;\n> };\n> \n> struct IPAContext {\n> -- \n> 2.49.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4541DC327D\n\tfor ;\n\tSat, 12 Apr 2025 18:30:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5978768AAF;\n\tSat, 12 Apr 2025 20:30:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D076568A7B\n\tfor ;\n\tSat, 12 Apr 2025 20:30:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E32675A;\n\tSat, 12 Apr 2025 20:28:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DXT6y7Zf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1744482493;\n\tbh=89TdKXLaz3Mhbmw5+zzl36UMZo4qrXUC78/0NMVa7XA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=DXT6y7ZfUCY9m8WwgwdvxGMTrB50W+LU1yzsRcIRhWPUNKzrgL5l2DdIarKNerZpH\n\tkY0T3SKYT6kEGlSmm/Bh5dp1AAYbOWFcIdNS0rV0iXiSlGpylQmYwPvoMqrZmNfY8A\n\tWgvFrT9sAJQS0iIR9eFod7b6VDyHnvbhEpL6wxz0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250407085802.16269-1-mzamazal@redhat.com>","References":"<20250407085802.16269-1-mzamazal@redhat.com>","Subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","From":"Kieran Bingham ","Cc":"Milan Zamazal ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Sat, 12 Apr 2025 19:30:11 +0100","Message-ID":"<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33945,"web_url":"https://patchwork.libcamera.org/comment/33945/","msgid":"<85r01unath.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-04-14T13:15:38","subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Kieran,\n\nthank you for review.\n\nKieran Bingham writes:\n\n> Quoting Milan Zamazal (2025-04-07 09:58:02)\n>> Saturation control is added on top of the colour correction matrix. A\n>> way of saturation adjustment that can be fully integrated into the\n>\n>> colour correction matrix is used. The control is available only if Ccm\n>> algorithm is enabled.\n>> \n>> The control uses 0.0-2.0 value range, with 1.0 being unmodified\n>> saturation, 0.0 full desaturation and 2.0 quite saturated.\n>> \n>> The matrix for saturation adjustment is taken from\n>> https://www.graficaobscura.com/matrix/index.html. The colour correction\n>> matrix is applied before gamma and the given matrix is suitable for such\n>> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp\n>> could be used.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++--\n>> src/ipa/simple/algorithms/ccm.h | 11 ++++++\n>> src/ipa/simple/ipa_context.h | 4 +++\n>> 3 files changed, 69 insertions(+), 3 deletions(-)\n>> \n>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> index d5ba928d..2700a247 100644\n>> --- a/src/ipa/simple/algorithms/ccm.cpp\n>> +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> @@ -3,7 +3,7 @@\n>> * Copyright (C) 2024, Ideas On Board\n>> * Copyright (C) 2024-2025, Red Hat Inc.\n>> *\n>> - * Color correction matrix\n>> + * Color correction matrix + saturation\n>> */\n>> \n>> #include \"ccm.h\"\n>> @@ -13,6 +13,8 @@\n>> \n>> #include \n>> \n>> +#include \"libcamera/internal/matrix.h\"\n>> +\n>> namespace {\n>> \n>> constexpr unsigned int kTemperatureThreshold = 100;\n>> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n>> }\n>> \n>> context.ccmEnabled = true;\n>> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n>> +\n>> + return 0;\n>> +}\n>> +\n>> +int Ccm::configure(IPAContext &context,\n>> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> +{\n>> + context.activeState.knobs.saturation = std::optional();\n>> \n>> return 0;\n>> }\n>> \n>> +void Ccm::queueRequest(typename Module::Context &context,\n>> + [[maybe_unused]] const uint32_t frame,\n>> + [[maybe_unused]] typename Module::FrameContext &frameContext,\n>> + const ControlList &controls)\n>> +{\n>> + const auto &saturation = controls.get(controls::Saturation);\n>> + if (saturation.has_value()) {\n>> + context.activeState.knobs.saturation = saturation;\n>> + LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n>> + }\n>> +}\n>> +\n>> +void Ccm::updateSaturation(Matrix &ccm, float saturation)\n>> +{\n>> + /*\n>> + * See https://www.graficaobscura.com/matrix/index.html.\n>\n> Looks like it would be easy to add a brightness control too after this.\n\nYes, when CCM is used, it's easy to add brightness. And when CCM is not\nused, it's easy too, applying it when making the LUT values -- this is\nimportant for CPU ISP when CCM is not used (having to use CCM just to\nadjust brightness would be overkill).\n\n>> + * This is applied before gamma thus a matrix for linear RGB must be used.\n>> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0\n>> + * no saturation (monochrome).\n>> + */\n>> + constexpr float r = 0.3086;\n>> + constexpr float g = 0.6094;\n>> + constexpr float b = 0.0820;\n>\n> it would be interesting to dig into where these numbers are derived\n> from exactly ...\n\nI guess some educated magic, similarly to other luminance related\nconversions.\n\n> https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms\n> seems to report others have looked, but I don't see a direct answer yet\n> (but I won't dig deep) ...\n>\n> And the referenced article is quite helpful.\n>\n> As this is the 'luminance' vector - I wonder if writing it as \n> \tRGB luminance = { 0.3086, 0.6094, 0.0820 };\n>\n> makes sense...\n>\n>> + const float s1 = 1.0 - saturation;\n>> + ccm = ccm * Matrix{ { s1 * r + saturation, s1 * g, s1 * b,\n>> + s1 * r, s1 * g + saturation, s1 * b,\n>> + s1 * r, s1 * g, s1 * b + saturation } };\n>\n> but that would become much more terse ... so I'm not sure it's worth it\n> here? Or would it then be more explicitly readable like the\n> applySaturation in RPi?\n\nI'll try and let's see.\n\n>> +}\n>> +\n>> void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> {\n>> + auto &saturation = context.activeState.knobs.saturation;\n>> +\n>> const unsigned int ct = context.activeState.awb.temperatureK;\n>> \n>> - /* Change CCM only on bigger temperature changes. */\n>> + /* Change CCM only on saturation or bigger temperature changes. */\n>> if (frame > 0 &&\n>> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n>> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n>> + saturation == lastSaturation_) {\n>> frameContext.ccm.ccm = context.activeState.ccm.ccm;\n>> context.activeState.ccm.changed = false;\n>> return;\n>> }\n>> \n>> lastCt_ = ct;\n>> + lastSaturation_ = saturation;\n>> Matrix ccm = ccm_.getInterpolated(ct);\n>> + if (saturation)\n>> + updateSaturation(ccm, saturation.value());\n>> \n>> context.activeState.ccm.ccm = ccm;\n>> frameContext.ccm.ccm = ccm;\n>> + frameContext.saturation = saturation;\n>> context.activeState.ccm.changed = true;\n>> }\n>> \n>> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n>> ControlList &metadata)\n>> {\n>> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());\n>> +\n>> + const auto &saturation = frameContext.saturation;\n>> + if (saturation)\n>> + metadata.set(controls::Saturation, saturation.value());\n>\n> This matches what we currently do for Contrast, but I wonder if we\n> should report a metadata of '1.0' for both Contrast and Saturation if\n> they aren't set ... as that's what the 'state' is/should be ?\n\nI don't have an opinion on this, both of \"no saturation request -> no\nmetadata\" and \"no saturation request -> the corresponding default value\nin metadata\" make sense to me.\n\n> Anyway, I wouldn't block this patch on that - as it would be on top for\n> both controls...\n>\n> Tested-by: Kieran Bingham \n>\n> Though, while it's functional - looking at the resulting image in\n> camshark, I think I would expect a saturation of 0.0 to be 'greyscale' -\n> with all pixels having roughly the same r=g=b - and I don't think that's\n> the case ... It looks like there's definitely a bias against green. It\n> looks like R=B, but G is always slightly less by at least ~20% ? ...\n> leaving the image looking slightly tinted purple.\n\nOh, now, under daylight, I get a prominent purple tint too.\n\n> That could be because I'm running an untuned/identity matrix for CCM\n> perhaps? or is it because white balance needs to be taken into account ?\n\nNo, it's a math mistake I did in CCM patches. White balance must be\napplied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix,\nand P the pixel RGB vector. I thought \"let's apply C after W\" and\ncomputed (W * C) * P. Which is wrong, to apply C after W means\nC * (W * P) = (C * W) * P. The last matrix applied must be the first\none in the multiplication. When I swap the multiplication in\nsrc/ipa/simple/algorithms/lut.cpp from\n\n auto ccm = gainCcm * context.activeState.ccm.ccm;\n\nto\n\n auto ccm = context.activeState.ccm.ccm * gainCcm;\n\nit gets desaturated as it should be (there cannot be any colour cast\nwhen desaturation is applied correctly, regardless of sensor\ncalibration). Maybe this will also explain and improve the colour casts\nI previously experienced when experimenting with my sensor CCMs from\nother pipelines.\n\n> But G = (RB-20%) doesn't sound like white balance to me at the moment...\n>\n>> }\n>> \n>> REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n>> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n>> index f4e2b85b..2c5d2170 100644\n>> --- a/src/ipa/simple/algorithms/ccm.h\n>> +++ b/src/ipa/simple/algorithms/ccm.h\n>> @@ -7,6 +7,8 @@\n>> \n>> #pragma once\n>> \n>> +#include \n>> +\n>> #include \"libcamera/internal/matrix.h\"\n>> \n>> #include \n>> @@ -24,6 +26,12 @@ public:\n>> ~Ccm() = default;\n>> \n>> int init(IPAContext &context, const YamlObject &tuningData) override;\n>> + int configure(IPAContext &context,\n>> + const IPAConfigInfo &configInfo) override;\n>> + void queueRequest(typename Module::Context &context,\n>> + const uint32_t frame,\n>> + typename Module::FrameContext &frameContext,\n>> + const ControlList &controls) override;\n>> void prepare(IPAContext &context,\n>> const uint32_t frame,\n>> IPAFrameContext &frameContext,\n>> @@ -34,7 +42,10 @@ public:\n>> ControlList &metadata) override;\n>> \n>> private:\n>> + void updateSaturation(Matrix &ccm, float saturation);\n>> +\n>> unsigned int lastCt_;\n>> + std::optional lastSaturation_;\n>> Interpolator> ccm_;\n>> };\n>> \n>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> index 88cc6c35..56792981 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -63,6 +63,7 @@ struct IPAActiveState {\n>> struct {\n>> /* 0..2 range, 1.0 = normal */\n>> std::optional contrast;\n>> + std::optional saturation;\n>> } knobs;\n>> };\n>> \n>> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext {\n>> int32_t exposure;\n>> double gain;\n>> } sensor;\n>> +\n>> struct {\n>> double red;\n>> double blue;\n>> } gains;\n>> +\n>> std::optional contrast;\n>> + std::optional saturation;\n>> };\n>> \n>> struct IPAContext {\n>> -- \n>> 2.49.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5CD12C327D\n\tfor ;\n\tMon, 14 Apr 2025 13:15:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6C5968AAA;\n\tMon, 14 Apr 2025 15:15:48 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8EC6627F2\n\tfor ;\n\tMon, 14 Apr 2025 15:15:46 +0200 (CEST)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-641-QFS_5YC-Nau5Sfqjg5lRow-1; Mon, 14 Apr 2025 09:15:43 -0400","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-43d08915f61so26039665e9.2\n\tfor ;\n\tMon, 14 Apr 2025 06:15:43 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-43f23158849sm180768935e9.0.2025.04.14.06.15.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Apr 2025 06:15:39 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"S+1+W45L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1744636545;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=+j5GAViRHh80YaPJvuaTJxnfSHHKwnhf5Llm6X6fANI=;\n\tb=S+1+W45LTlN088Xe8X6p5ILtuOpDE9wSIYLhjsku823ws5AC5Gw7+VSQBKsUfZUSMKG7YE\n\t3LWERHPh+kYDUjJum26BEpZIFYQETw5bTPxPLN5iyiT6GFnhsHbsoBgOMAR1NImRbYuZby\n\tw8JlO97AdwPacHooxRTEUKWS5PnWg2E=","X-MC-Unique":"QFS_5YC-Nau5Sfqjg5lRow-1","X-Mimecast-MFC-AGG-ID":"QFS_5YC-Nau5Sfqjg5lRow_1744636542","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1744636541; x=1745241341;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=+j5GAViRHh80YaPJvuaTJxnfSHHKwnhf5Llm6X6fANI=;\n\tb=bwM+pjltx2Ar1VyjUJ04kf21EtshP+8RHlqTqRBCVXsFzSLxJnC/hXSAz5FnmAwvkn\n\toK3jN2f+BusEiJXXVyS7P5oL0Wb/MVbYRiT1OT5RgfAMs7BW4oqzJigiUg6A9B1lUBZb\n\t5AQzMtGPtBvPHc2AJiX/E+pNyTI/Q/ObW9ImxarHtFR+AAhzbwfEhMGSIO2gxAcT1JyT\n\ti3pcUcxPsnwl0jHNsR1GVegz804bSQvQuVZVOOM0OLpN4ZtTLJhOZ7+sHtIBOw9mIUsA\n\tRIoYmF4FpNrySqEXND8CwI5wrTT4zO8nC1Ne7vfsUqNIiyNh99saP9Wqt7rXSHDYX5nk\n\tP59A==","X-Gm-Message-State":"AOJu0YxMUg2Wegf5zItNCBc+anZUZ0UBkqT+xXZYo5iXjpl6XhFnilm9\n\tv0csLPnR6jRPLG6N94DBfbd73ZlSC70r3zFrh+DWAgnZZnH3GmkKMJ5YmxAUWOBXUqFwdKUfOV4\n\tvz3ORfEuOfO+CNH0MVBuFRdrOKgAqEB9uxdvaB6CI4l9d9U0oFZKRciktmRXGVLlwkJQ+mg/GF4\n\tlBg3zA9sS2HNLJ2rRAZaqJ8aQjtPJlK9momzrAB6nZ/TVf1tMe+FikslQLLQ==","X-Gm-Gg":"ASbGnctU6jQuEsUNnt3VM0+qirPTk29f2ZvUtySQHGrcrxMPXtwYWQ9UOOa2Pzkej3h\n\tXQcHtXBcIJp1AnAQyivRjc5TaaKyDo4aTfsjJBZ6ByChPcOX0enhooUch7mqBfQNJm+MrKkZ2MR\n\tukTn5J58SSPTX1/Ugpo9y9S5VcteFEKHv37PvHGIT057ST36Eo0XUXPzgMJGialNIkGjhohuDyk\n\tI8DH739BnaOSMs6LIWblxgWohVUMH65PSL9QBg+aCUprdWJKo95uFAQTSXQDuXKRLabd/WJZMO/\n\tkoswocvvfOCLKxcQUEAAld2kaviy8iWAR4y0fro34ictRVaY7vlIm/BgJ7qomCQj","X-Received":["by 2002:a05:600c:3b15:b0:43d:300f:fa3d with SMTP id\n\t5b1f17b1804b1-43f3a9266e0mr84709215e9.5.1744636541483; \n\tMon, 14 Apr 2025 06:15:41 -0700 (PDT)","by 2002:a05:600c:3b15:b0:43d:300f:fa3d with SMTP id\n\t5b1f17b1804b1-43f3a9266e0mr84708815e9.5.1744636540873; \n\tMon, 14 Apr 2025 06:15:40 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFeeQQcK2ozMVMwGB69lT71ytjdaoTG+LeRC5B98YAzpvY5WAKh4eJdOE24Gch6AFhdLSCqDA==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","In-Reply-To":"<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Sat, 12 Apr 2025 19:30:11 +0100\")","References":"<20250407085802.16269-1-mzamazal@redhat.com>\n\t<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>","Date":"Mon, 14 Apr 2025 15:15:38 +0200","Message-ID":"<85r01unath.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"I918vhR5Lr97tJAffWrQfCDMrzMu_IZYwF1ZO49uDnU_1744636542","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33947,"web_url":"https://patchwork.libcamera.org/comment/33947/","msgid":"<174463792210.2882969.14630378418171625914@ping.linuxembedded.co.uk>","date":"2025-04-14T13:38:42","subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-04-14 14:15:38)\n> Hi Kieran,\n> \n> thank you for review.\n> \n> Kieran Bingham writes:\n> \n> > Quoting Milan Zamazal (2025-04-07 09:58:02)\n> >> Saturation control is added on top of the colour correction matrix. A\n> >> way of saturation adjustment that can be fully integrated into the\n> >\n> >> colour correction matrix is used. The control is available only if Ccm\n> >> algorithm is enabled.\n> >> \n> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified\n> >> saturation, 0.0 full desaturation and 2.0 quite saturated.\n> >> \n> >> The matrix for saturation adjustment is taken from\n> >> https://www.graficaobscura.com/matrix/index.html. The colour correction\n> >> matrix is applied before gamma and the given matrix is suitable for such\n> >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp\n> >> could be used.\n> >> \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++--\n> >> src/ipa/simple/algorithms/ccm.h | 11 ++++++\n> >> src/ipa/simple/ipa_context.h | 4 +++\n> >> 3 files changed, 69 insertions(+), 3 deletions(-)\n> >> \n> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> >> index d5ba928d..2700a247 100644\n> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> >> @@ -3,7 +3,7 @@\n> >> * Copyright (C) 2024, Ideas On Board\n> >> * Copyright (C) 2024-2025, Red Hat Inc.\n> >> *\n> >> - * Color correction matrix\n> >> + * Color correction matrix + saturation\n> >> */\n> >> \n> >> #include \"ccm.h\"\n> >> @@ -13,6 +13,8 @@\n> >> \n> >> #include \n> >> \n> >> +#include \"libcamera/internal/matrix.h\"\n> >> +\n> >> namespace {\n> >> \n> >> constexpr unsigned int kTemperatureThreshold = 100;\n> >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> >> }\n> >> \n> >> context.ccmEnabled = true;\n> >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n> >> +\n> >> + return 0;\n> >> +}\n> >> +\n> >> +int Ccm::configure(IPAContext &context,\n> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >> +{\n> >> + context.activeState.knobs.saturation = std::optional();\n> >> \n> >> return 0;\n> >> }\n> >> \n> >> +void Ccm::queueRequest(typename Module::Context &context,\n> >> + [[maybe_unused]] const uint32_t frame,\n> >> + [[maybe_unused]] typename Module::FrameContext &frameContext,\n> >> + const ControlList &controls)\n> >> +{\n> >> + const auto &saturation = controls.get(controls::Saturation);\n> >> + if (saturation.has_value()) {\n> >> + context.activeState.knobs.saturation = saturation;\n> >> + LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n> >> + }\n> >> +}\n> >> +\n> >> +void Ccm::updateSaturation(Matrix &ccm, float saturation)\n> >> +{\n> >> + /*\n> >> + * See https://www.graficaobscura.com/matrix/index.html.\n> >\n> > Looks like it would be easy to add a brightness control too after this.\n> \n> Yes, when CCM is used, it's easy to add brightness. And when CCM is not\n> used, it's easy too, applying it when making the LUT values -- this is\n> important for CPU ISP when CCM is not used (having to use CCM just to\n> adjust brightness would be overkill).\n\nAha, then indeed lets just apply it directly to the LUT (later) :-)\n\n> \n> >> + * This is applied before gamma thus a matrix for linear RGB must be used.\n> >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0\n> >> + * no saturation (monochrome).\n> >> + */\n> >> + constexpr float r = 0.3086;\n> >> + constexpr float g = 0.6094;\n> >> + constexpr float b = 0.0820;\n> >\n> > it would be interesting to dig into where these numbers are derived\n> > from exactly ...\n> \n> I guess some educated magic, similarly to other luminance related\n> conversions.\n> \n> > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms\n> > seems to report others have looked, but I don't see a direct answer yet\n> > (but I won't dig deep) ...\n> >\n> > And the referenced article is quite helpful.\n> >\n> > As this is the 'luminance' vector - I wonder if writing it as \n> > RGB luminance = { 0.3086, 0.6094, 0.0820 };\n> >\n> > makes sense...\n> >\n> >> + const float s1 = 1.0 - saturation;\n> >> + ccm = ccm * Matrix{ { s1 * r + saturation, s1 * g, s1 * b,\n> >> + s1 * r, s1 * g + saturation, s1 * b,\n> >> + s1 * r, s1 * g, s1 * b + saturation } };\n> >\n> > but that would become much more terse ... so I'm not sure it's worth it\n> > here? Or would it then be more explicitly readable like the\n> > applySaturation in RPi?\n> \n> I'll try and let's see.\n> \n> >> +}\n> >> +\n> >> void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> >> {\n> >> + auto &saturation = context.activeState.knobs.saturation;\n> >> +\n> >> const unsigned int ct = context.activeState.awb.temperatureK;\n> >> \n> >> - /* Change CCM only on bigger temperature changes. */\n> >> + /* Change CCM only on saturation or bigger temperature changes. */\n> >> if (frame > 0 &&\n> >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n> >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n> >> + saturation == lastSaturation_) {\n> >> frameContext.ccm.ccm = context.activeState.ccm.ccm;\n> >> context.activeState.ccm.changed = false;\n> >> return;\n> >> }\n> >> \n> >> lastCt_ = ct;\n> >> + lastSaturation_ = saturation;\n> >> Matrix ccm = ccm_.getInterpolated(ct);\n> >> + if (saturation)\n> >> + updateSaturation(ccm, saturation.value());\n> >> \n> >> context.activeState.ccm.ccm = ccm;\n> >> frameContext.ccm.ccm = ccm;\n> >> + frameContext.saturation = saturation;\n> >> context.activeState.ccm.changed = true;\n> >> }\n> >> \n> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n> >> ControlList &metadata)\n> >> {\n> >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());\n> >> +\n> >> + const auto &saturation = frameContext.saturation;\n> >> + if (saturation)\n> >> + metadata.set(controls::Saturation, saturation.value());\n> >\n> > This matches what we currently do for Contrast, but I wonder if we\n> > should report a metadata of '1.0' for both Contrast and Saturation if\n> > they aren't set ... as that's what the 'state' is/should be ?\n> \n> I don't have an opinion on this, both of \"no saturation request -> no\n> metadata\" and \"no saturation request -> the corresponding default value\n> in metadata\" make sense to me.\n> \n> > Anyway, I wouldn't block this patch on that - as it would be on top for\n> > both controls...\n> >\n> > Tested-by: Kieran Bingham \n> >\n> > Though, while it's functional - looking at the resulting image in\n> > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' -\n> > with all pixels having roughly the same r=g=b - and I don't think that's\n> > the case ... It looks like there's definitely a bias against green. It\n> > looks like R=B, but G is always slightly less by at least ~20% ? ...\n> > leaving the image looking slightly tinted purple.\n> \n> Oh, now, under daylight, I get a prominent purple tint too.\n> \n> > That could be because I'm running an untuned/identity matrix for CCM\n> > perhaps? or is it because white balance needs to be taken into account ?\n> \n> No, it's a math mistake I did in CCM patches. White balance must be\n> applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix,\n> and P the pixel RGB vector. I thought \"let's apply C after W\" and\n> computed (W * C) * P. Which is wrong, to apply C after W means\n> C * (W * P) = (C * W) * P. The last matrix applied must be the first\n> one in the multiplication. When I swap the multiplication in\n> src/ipa/simple/algorithms/lut.cpp from\n> \n> auto ccm = gainCcm * context.activeState.ccm.ccm;\n> \n> to\n> \n> auto ccm = context.activeState.ccm.ccm * gainCcm;\n> \n> it gets desaturated as it should be (there cannot be any colour cast\n> when desaturation is applied correctly, regardless of sensor\n> calibration). Maybe this will also explain and improve the colour casts\n> I previously experienced when experimenting with my sensor CCMs from\n> other pipelines.\n> \n> > But G = (RB-20%) doesn't sound like white balance to me at the moment...\n\naha, so maybe it is ;-)\n\nGreat, well if that's explained ... we have a path forwards. Does it\nimpact this patch much? Would it help to merge this first? or fix the\nCCM then rebase this one ?\n\nI think the colour tint is the only thing stopping me from adding a tag,\nand if that's clarified, then for Saturation:\n\nReviewed-by: Kieran Bingham \n\nIf you respin with the calculation split out in Ccm::updateSaturation()\nyou can still collect the tag I think. (perhaps there's a new series\nwith the above change and the color fix on the way?)\n\n--\nKieran\n\n> >\n> >> }\n> >> \n> >> REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> >> index f4e2b85b..2c5d2170 100644\n> >> --- a/src/ipa/simple/algorithms/ccm.h\n> >> +++ b/src/ipa/simple/algorithms/ccm.h\n> >> @@ -7,6 +7,8 @@\n> >> \n> >> #pragma once\n> >> \n> >> +#include \n> >> +\n> >> #include \"libcamera/internal/matrix.h\"\n> >> \n> >> #include \n> >> @@ -24,6 +26,12 @@ public:\n> >> ~Ccm() = default;\n> >> \n> >> int init(IPAContext &context, const YamlObject &tuningData) override;\n> >> + int configure(IPAContext &context,\n> >> + const IPAConfigInfo &configInfo) override;\n> >> + void queueRequest(typename Module::Context &context,\n> >> + const uint32_t frame,\n> >> + typename Module::FrameContext &frameContext,\n> >> + const ControlList &controls) override;\n> >> void prepare(IPAContext &context,\n> >> const uint32_t frame,\n> >> IPAFrameContext &frameContext,\n> >> @@ -34,7 +42,10 @@ public:\n> >> ControlList &metadata) override;\n> >> \n> >> private:\n> >> + void updateSaturation(Matrix &ccm, float saturation);\n> >> +\n> >> unsigned int lastCt_;\n> >> + std::optional lastSaturation_;\n> >> Interpolator> ccm_;\n> >> };\n> >> \n> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> >> index 88cc6c35..56792981 100644\n> >> --- a/src/ipa/simple/ipa_context.h\n> >> +++ b/src/ipa/simple/ipa_context.h\n> >> @@ -63,6 +63,7 @@ struct IPAActiveState {\n> >> struct {\n> >> /* 0..2 range, 1.0 = normal */\n> >> std::optional contrast;\n> >> + std::optional saturation;\n> >> } knobs;\n> >> };\n> >> \n> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext {\n> >> int32_t exposure;\n> >> double gain;\n> >> } sensor;\n> >> +\n> >> struct {\n> >> double red;\n> >> double blue;\n> >> } gains;\n> >> +\n> >> std::optional contrast;\n> >> + std::optional saturation;\n> >> };\n> >> \n> >> struct IPAContext {\n> >> -- \n> >> 2.49.0\n> >>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 978FAC327D\n\tfor ;\n\tMon, 14 Apr 2025 13:38:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9E3E627F2;\n\tMon, 14 Apr 2025 15:38:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6FB48627F2\n\tfor ;\n\tMon, 14 Apr 2025 15:38:44 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BFADA502;\n\tMon, 14 Apr 2025 15:36:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iTdFTkdK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1744637802;\n\tbh=xcdH8SfFxDf9o4ojMcUiktEPY1ToY67PEdz84AJwdps=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=iTdFTkdKXtKnKI6MtUoS/f4j3hU7FlWm3wT+wD02b5FaYASMuW76l+HTUkwXqJK7z\n\trYbsgqLpr3mobnmWs9ZMiGAcJU4ljZ+5jzvFrMQvau0yJbDFoySPFOAoEe7c/u/24N\n\t0dIwMtePct0chK7A//+uIsdpehc/wHsqnD0IPiPQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<85r01unath.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","References":"<20250407085802.16269-1-mzamazal@redhat.com>\n\t<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>\n\t<85r01unath.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Milan Zamazal ","Date":"Mon, 14 Apr 2025 14:38:42 +0100","Message-ID":"<174463792210.2882969.14630378418171625914@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33948,"web_url":"https://patchwork.libcamera.org/comment/33948/","msgid":"<85mscin6g9.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-04-14T14:49:58","subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Milan Zamazal (2025-04-14 14:15:38)\n>> Hi Kieran,\n>> \n>\n>> thank you for review.\n>> \n>> Kieran Bingham writes:\n>> \n>> > Quoting Milan Zamazal (2025-04-07 09:58:02)\n>> >> Saturation control is added on top of the colour correction matrix. A\n>> >> way of saturation adjustment that can be fully integrated into the\n>> >\n>> >> colour correction matrix is used. The control is available only if Ccm\n>> >> algorithm is enabled.\n>> >> \n>> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified\n>> >> saturation, 0.0 full desaturation and 2.0 quite saturated.\n>> >> \n>> >> The matrix for saturation adjustment is taken from\n>> >> https://www.graficaobscura.com/matrix/index.html. The colour correction\n>> >> matrix is applied before gamma and the given matrix is suitable for such\n>> >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp\n>> >> could be used.\n>> >> \n>> >> Signed-off-by: Milan Zamazal \n>> >> ---\n>> >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++--\n>> >> src/ipa/simple/algorithms/ccm.h | 11 ++++++\n>> >> src/ipa/simple/ipa_context.h | 4 +++\n>> >> 3 files changed, 69 insertions(+), 3 deletions(-)\n>> >> \n>> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> >> index d5ba928d..2700a247 100644\n>> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n>> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> >> @@ -3,7 +3,7 @@\n>> >> * Copyright (C) 2024, Ideas On Board\n>> >> * Copyright (C) 2024-2025, Red Hat Inc.\n>> >> *\n>> >> - * Color correction matrix\n>> >> + * Color correction matrix + saturation\n>> >> */\n>> >> \n>> >> #include \"ccm.h\"\n>> >> @@ -13,6 +13,8 @@\n>> >> \n>> >> #include \n>> >> \n>> >> +#include \"libcamera/internal/matrix.h\"\n>> >> +\n>> >> namespace {\n>> >> \n>> >> constexpr unsigned int kTemperatureThreshold = 100;\n>> >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n>> >> }\n>> >> \n>> >> context.ccmEnabled = true;\n>> >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n>> >> +\n>> >> + return 0;\n>> >> +}\n>> >> +\n>> >> +int Ccm::configure(IPAContext &context,\n>> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> >> +{\n>> >> + context.activeState.knobs.saturation = std::optional();\n>> >> \n>> >> return 0;\n>> >> }\n>> >> \n>> >> +void Ccm::queueRequest(typename Module::Context &context,\n>> >> + [[maybe_unused]] const uint32_t frame,\n>> >> + [[maybe_unused]] typename Module::FrameContext &frameContext,\n>> >> + const ControlList &controls)\n>> >> +{\n>> >> + const auto &saturation = controls.get(controls::Saturation);\n>> >> + if (saturation.has_value()) {\n>> >> + context.activeState.knobs.saturation = saturation;\n>> >> + LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n>> >> + }\n>> >> +}\n>> >> +\n>> >> +void Ccm::updateSaturation(Matrix &ccm, float saturation)\n>> >> +{\n>> >> + /*\n>> >> + * See https://www.graficaobscura.com/matrix/index.html.\n>> >\n>> > Looks like it would be easy to add a brightness control too after this.\n>> \n>> Yes, when CCM is used, it's easy to add brightness. And when CCM is not\n>> used, it's easy too, applying it when making the LUT values -- this is\n>> important for CPU ISP when CCM is not used (having to use CCM just to\n>> adjust brightness would be overkill).\n>\n> Aha, then indeed lets just apply it directly to the LUT (later) :-)\n>\n>> \n>> >> + * This is applied before gamma thus a matrix for linear RGB must be used.\n>> >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0\n>> >> + * no saturation (monochrome).\n>> >> + */\n>> >> + constexpr float r = 0.3086;\n>> >> + constexpr float g = 0.6094;\n>> >> + constexpr float b = 0.0820;\n>> >\n>> > it would be interesting to dig into where these numbers are derived\n>> > from exactly ...\n>> \n>> I guess some educated magic, similarly to other luminance related\n>> conversions.\n>> \n>> > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms\n>> > seems to report others have looked, but I don't see a direct answer yet\n>> > (but I won't dig deep) ...\n>> >\n>> > And the referenced article is quite helpful.\n>> >\n>> > As this is the 'luminance' vector - I wonder if writing it as \n>> > RGB luminance = { 0.3086, 0.6094, 0.0820 };\n>> >\n>> > makes sense...\n>> >\n>> >> + const float s1 = 1.0 - saturation;\n>> >> + ccm = ccm * Matrix{ { s1 * r + saturation, s1 * g, s1 * b,\n>> >> + s1 * r, s1 * g + saturation, s1 * b,\n>> >> + s1 * r, s1 * g, s1 * b + saturation } };\n\nThis multiplication should be also performed in the reverse order.\n\n>> >\n>> > but that would become much more terse ... so I'm not sure it's worth it\n>> > here? Or would it then be more explicitly readable like the\n>> > applySaturation in RPi?\n>> \n>> I'll try and let's see.\n>> \n>> >> +}\n>> >> +\n>> >> void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>> >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> >> {\n>> >> + auto &saturation = context.activeState.knobs.saturation;\n>> >> +\n>> >> const unsigned int ct = context.activeState.awb.temperatureK;\n>> >> \n>> >> - /* Change CCM only on bigger temperature changes. */\n>> >> + /* Change CCM only on saturation or bigger temperature changes. */\n>> >> if (frame > 0 &&\n>> >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n>> >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n>> >> + saturation == lastSaturation_) {\n>> >> frameContext.ccm.ccm = context.activeState.ccm.ccm;\n>> >> context.activeState.ccm.changed = false;\n>> >> return;\n>> >> }\n>> >> \n>> >> lastCt_ = ct;\n>> >> + lastSaturation_ = saturation;\n>> >> Matrix ccm = ccm_.getInterpolated(ct);\n>> >> + if (saturation)\n>> >> + updateSaturation(ccm, saturation.value());\n>> >> \n>> >> context.activeState.ccm.ccm = ccm;\n>> >> frameContext.ccm.ccm = ccm;\n>> >> + frameContext.saturation = saturation;\n>> >> context.activeState.ccm.changed = true;\n>> >> }\n>> >> \n>> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n>> >> ControlList &metadata)\n>> >> {\n>> >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());\n>> >> +\n>> >> + const auto &saturation = frameContext.saturation;\n>> >> + if (saturation)\n>> >> + metadata.set(controls::Saturation, saturation.value());\n>> >\n>> > This matches what we currently do for Contrast, but I wonder if we\n>> > should report a metadata of '1.0' for both Contrast and Saturation if\n>> > they aren't set ... as that's what the 'state' is/should be ?\n>> \n>> I don't have an opinion on this, both of \"no saturation request -> no\n>> metadata\" and \"no saturation request -> the corresponding default value\n>> in metadata\" make sense to me.\n>> \n>> > Anyway, I wouldn't block this patch on that - as it would be on top for\n>> > both controls...\n>> >\n>> > Tested-by: Kieran Bingham \n>> >\n>> > Though, while it's functional - looking at the resulting image in\n>> > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' -\n>> > with all pixels having roughly the same r=g=b - and I don't think that's\n>> > the case ... It looks like there's definitely a bias against green. It\n>> > looks like R=B, but G is always slightly less by at least ~20% ? ...\n>> > leaving the image looking slightly tinted purple.\n>> \n>> Oh, now, under daylight, I get a prominent purple tint too.\n>> \n>> > That could be because I'm running an untuned/identity matrix for CCM\n>> > perhaps? or is it because white balance needs to be taken into account ?\n>> \n>> No, it's a math mistake I did in CCM patches. White balance must be\n>> applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix,\n>> and P the pixel RGB vector. I thought \"let's apply C after W\" and\n>> computed (W * C) * P. Which is wrong, to apply C after W means\n>> C * (W * P) = (C * W) * P. The last matrix applied must be the first\n>> one in the multiplication. When I swap the multiplication in\n>> src/ipa/simple/algorithms/lut.cpp from\n>> \n>> auto ccm = gainCcm * context.activeState.ccm.ccm;\n>> \n>> to\n>> \n>> auto ccm = context.activeState.ccm.ccm * gainCcm;\n>> \n>> it gets desaturated as it should be (there cannot be any colour cast\n>> when desaturation is applied correctly, regardless of sensor\n>> calibration). Maybe this will also explain and improve the colour casts\n>> I previously experienced when experimenting with my sensor CCMs from\n>> other pipelines.\n>> \n>> > But G = (RB-20%) doesn't sound like white balance to me at the moment...\n>\n> aha, so maybe it is ;-)\n>\n> Great, well if that's explained ... we have a path forwards. Does it\n> impact this patch much? Would it help to merge this first? or fix the\n> CCM then rebase this one ?\n\nThe latter. I'll post the CCM fix separately. The saturation patch\nneeds a multiplication order fix too and I'd like to simplify\nupdateSaturation() a bit; I'll post v2.\n\nThen there is a question what to do about saturation + contrast. I\nbelieve contrast should be applied between sensor-CCM and saturation but\nthis would be too expensive on a CPU.\n\n> I think the colour tint is the only thing stopping me from adding a tag,\n> and if that's clarified, then for Saturation:\n>\n> Reviewed-by: Kieran Bingham \n>\n> If you respin with the calculation split out in Ccm::updateSaturation()\n> you can still collect the tag I think. (perhaps there's a new series\n> with the above change and the color fix on the way?)\n>\n> --\n> Kieran\n>\n>> >\n>> >> }\n>> >> \n>> >> REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n>> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n>> >> index f4e2b85b..2c5d2170 100644\n>> >> --- a/src/ipa/simple/algorithms/ccm.h\n>> >> +++ b/src/ipa/simple/algorithms/ccm.h\n>> >> @@ -7,6 +7,8 @@\n>> >> \n>> >> #pragma once\n>> >> \n>> >> +#include \n>> >> +\n>> >> #include \"libcamera/internal/matrix.h\"\n>> >> \n>> >> #include \n>> >> @@ -24,6 +26,12 @@ public:\n>> >> ~Ccm() = default;\n>> >> \n>> >> int init(IPAContext &context, const YamlObject &tuningData) override;\n>> >> + int configure(IPAContext &context,\n>> >> + const IPAConfigInfo &configInfo) override;\n>> >> + void queueRequest(typename Module::Context &context,\n>> >> + const uint32_t frame,\n>> >> + typename Module::FrameContext &frameContext,\n>> >> + const ControlList &controls) override;\n>> >> void prepare(IPAContext &context,\n>> >> const uint32_t frame,\n>> >> IPAFrameContext &frameContext,\n>> >> @@ -34,7 +42,10 @@ public:\n>> >> ControlList &metadata) override;\n>> >> \n>> >> private:\n>> >> + void updateSaturation(Matrix &ccm, float saturation);\n>> >> +\n>> >> unsigned int lastCt_;\n>> >> + std::optional lastSaturation_;\n>> >> Interpolator> ccm_;\n>> >> };\n>> >> \n>> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> >> index 88cc6c35..56792981 100644\n>> >> --- a/src/ipa/simple/ipa_context.h\n>> >> +++ b/src/ipa/simple/ipa_context.h\n>> >> @@ -63,6 +63,7 @@ struct IPAActiveState {\n>> >> struct {\n>> >> /* 0..2 range, 1.0 = normal */\n>> >> std::optional contrast;\n>> >> + std::optional saturation;\n>> >> } knobs;\n>> >> };\n>> >> \n>> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext {\n>> >> int32_t exposure;\n>> >> double gain;\n>> >> } sensor;\n>> >> +\n>> >> struct {\n>> >> double red;\n>> >> double blue;\n>> >> } gains;\n>> >> +\n>> >> std::optional contrast;\n>> >> + std::optional saturation;\n>> >> };\n>> >> \n>> >> struct IPAContext {\n>> >> -- \n>> >> 2.49.0\n>> >>\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 35B6FC3213\n\tfor ;\n\tMon, 14 Apr 2025 14:50:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CDEC68AAA;\n\tMon, 14 Apr 2025 16:50:07 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB7BF627F2\n\tfor ;\n\tMon, 14 Apr 2025 16:50:04 +0200 (CEST)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-662-i5YAcFzXOXidFZ3GXL73UA-1; Mon, 14 Apr 2025 10:50:02 -0400","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-43e9b0fd00cso22342305e9.0\n\tfor ;\n\tMon, 14 Apr 2025 07:50:02 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-43f2338d757sm178849995e9.5.2025.04.14.07.49.58\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Apr 2025 07:49:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"e0c9OuTv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1744642203;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=9SgK4XQ8XAZJd/rwxFYhuH29ysoE017WWRaxk5O4RWk=;\n\tb=e0c9OuTvtTwdXISuCRsKT/qkQBI9YsRDhzId3AZCdnLGvAvGrvQ7noxaVGeuumq8kh7Voq\n\tGLFFwCsm7q+avsvjVfJ1+XjwV4nRjb/o6M6M9UT+GtYB//8MQlW/AMCW7MQWRoQxUNZxcT\n\t3so7yLbLiSH6klElJLGTUoejKwiPLYM=","X-MC-Unique":"i5YAcFzXOXidFZ3GXL73UA-1","X-Mimecast-MFC-AGG-ID":"i5YAcFzXOXidFZ3GXL73UA_1744642201","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1744642200; x=1745247000;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=9SgK4XQ8XAZJd/rwxFYhuH29ysoE017WWRaxk5O4RWk=;\n\tb=FjSItHZtH5t0poHD2OojBDTMIcA1gUhHPFNFlFZG+BDXj0a80SS/M/QGPdB+wwHHsR\n\t04+/iMl/WQQCUPYyU2NO/SlkP7Hc9McmiEcgsVMwDNlFB+KmUeglM+3C22te8dGwsaP5\n\twgfEL4A25MY0lWtlM/TuyekQbEaXOtNaNCoIcgOTziHMxjpONwQoGH+h5eGVZNxhI3sF\n\tfPuHNtnjYIjdoTJwxR2R8BpCGCy1ZQETeubhxrxavz80PQ0eUQNsy7GsKnWkIotjEKbO\n\tf+KtIPAwsfdGN93HuOFTx56pCbyJR49vHpU00fcxnKOtua0zuxq8g6/webl/bn6XPqrT\n\tVokA==","X-Gm-Message-State":"AOJu0YwTeH0ia/rvv30KJdAYSXRkkFEz1H6GoGIKrTDyWA7Jitc8cSVL\n\tbHo1BSUA+rkD8Uc3P6osEbPUcnwiPPU1Pi2lrp8WXXdj2aMz5NOV6fFGPNGtwzz5w3uNQzU9NMw\n\tV53JgLVbo1zjjiCkmCTkf3mGGCzGlAx8irQXaKyBSXguCHnmaSBSeW5yMiJq9qN0puurJ8LKCpP\n\tf6tpzs/ofPrrt0rehsy3XGQUqzC5zSlDDL35LotNvObR9bGgeVoEOcNqLDLA==","X-Gm-Gg":"ASbGncuvJ5ViduZ6v5bHfmVL1jpK8G4BCDWKh+ph5tkYAbUv1cbl/wLlM3DH4dKuGqh\n\tUSEW/mlmpTazoxxh2ocRg5vDvU6UmHXfEYDjphhTRV+r2U+gYydsMnR0LPnzqP4SH97YefCVeKl\n\tNKhsls6olu4JJKATusXotKP0YLwDIuSW6xfKj+0gjdMjK5ZqXf3xTpbVAnhpHlc05t9Mz1F71KM\n\t8rfENZJjQ9bgfXORKZpm5GGZ0mDcKGS4d0SGSTXG2cw3ONLu9k312E1hYJxUFKegzu0bf5M23ua\n\tUox+7lMpqu2RDiztB1RRq3Qbfy4aRZOs0/MnuyA4/hcUHHFUgf6aoF2vRAITRwZg","X-Received":["by 2002:a05:600c:3b08:b0:439:8878:5029 with SMTP id\n\t5b1f17b1804b1-43f39622827mr84965085e9.2.1744642200416; \n\tMon, 14 Apr 2025 07:50:00 -0700 (PDT)","by 2002:a05:600c:3b08:b0:439:8878:5029 with SMTP id\n\t5b1f17b1804b1-43f39622827mr84964735e9.2.1744642199743; \n\tMon, 14 Apr 2025 07:49:59 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHTaaANyZvFOfzP8pWakFgr8SXxiTkaO4JOD/2Zk8vvOvCSQfoOfQzXDhD0Nd2/+PELOrLRxA==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","In-Reply-To":"<174463792210.2882969.14630378418171625914@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Mon, 14 Apr 2025 14:38:42 +0100\")","References":"<20250407085802.16269-1-mzamazal@redhat.com>\n\t<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>\n\t<85r01unath.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<174463792210.2882969.14630378418171625914@ping.linuxembedded.co.uk>","Date":"Mon, 14 Apr 2025 16:49:58 +0200","Message-ID":"<85mscin6g9.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"xLP_QMv6o6O_HkFyZVoM7nyC9xi8a38mGtFF1DhTdoA_1744642201","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33949,"web_url":"https://patchwork.libcamera.org/comment/33949/","msgid":"<20250414160528.GG26798@pendragon.ideasonboard.com>","date":"2025-04-14T16:05:28","subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Apr 14, 2025 at 04:49:58PM +0200, Milan Zamazal wrote:\n> Kieran Bingham writes:\n> > Quoting Milan Zamazal (2025-04-14 14:15:38)\n> >> Kieran Bingham writes:\n> >> > Quoting Milan Zamazal (2025-04-07 09:58:02)\n> >> >> Saturation control is added on top of the colour correction matrix. A\n> >> >> way of saturation adjustment that can be fully integrated into the\n> >> >> colour correction matrix is used. The control is available only if Ccm\n> >> >> algorithm is enabled.\n> >> >> \n> >> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified\n> >> >> saturation, 0.0 full desaturation and 2.0 quite saturated.\n> >> >> \n> >> >> The matrix for saturation adjustment is taken from\n> >> >> https://www.graficaobscura.com/matrix/index.html. The colour correction\n> >> >> matrix is applied before gamma and the given matrix is suitable for such\n> >> >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp\n> >> >> could be used.\n> >> >> \n> >> >> Signed-off-by: Milan Zamazal \n> >> >> ---\n> >> >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++--\n> >> >> src/ipa/simple/algorithms/ccm.h | 11 ++++++\n> >> >> src/ipa/simple/ipa_context.h | 4 +++\n> >> >> 3 files changed, 69 insertions(+), 3 deletions(-)\n> >> >> \n> >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> >> >> index d5ba928d..2700a247 100644\n> >> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n> >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> >> >> @@ -3,7 +3,7 @@\n> >> >> * Copyright (C) 2024, Ideas On Board\n> >> >> * Copyright (C) 2024-2025, Red Hat Inc.\n> >> >> *\n> >> >> - * Color correction matrix\n> >> >> + * Color correction matrix + saturation\n> >> >> */\n> >> >> \n> >> >> #include \"ccm.h\"\n> >> >> @@ -13,6 +13,8 @@\n> >> >> \n> >> >> #include \n> >> >> \n> >> >> +#include \"libcamera/internal/matrix.h\"\n> >> >> +\n> >> >> namespace {\n> >> >> \n> >> >> constexpr unsigned int kTemperatureThreshold = 100;\n> >> >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> >> >> }\n> >> >> \n> >> >> context.ccmEnabled = true;\n> >> >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n> >> >> +\n> >> >> + return 0;\n> >> >> +}\n> >> >> +\n> >> >> +int Ccm::configure(IPAContext &context,\n> >> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >> >> +{\n> >> >> + context.activeState.knobs.saturation = std::optional();\n> >> >> \n> >> >> return 0;\n> >> >> }\n> >> >> \n> >> >> +void Ccm::queueRequest(typename Module::Context &context,\n> >> >> + [[maybe_unused]] const uint32_t frame,\n> >> >> + [[maybe_unused]] typename Module::FrameContext &frameContext,\n> >> >> + const ControlList &controls)\n> >> >> +{\n> >> >> + const auto &saturation = controls.get(controls::Saturation);\n> >> >> + if (saturation.has_value()) {\n> >> >> + context.activeState.knobs.saturation = saturation;\n> >> >> + LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n> >> >> + }\n> >> >> +}\n> >> >> +\n> >> >> +void Ccm::updateSaturation(Matrix &ccm, float saturation)\n> >> >> +{\n> >> >> + /*\n> >> >> + * See https://www.graficaobscura.com/matrix/index.html.\n> >> >\n> >> > Looks like it would be easy to add a brightness control too after this.\n> >> \n> >> Yes, when CCM is used, it's easy to add brightness. And when CCM is not\n> >> used, it's easy too, applying it when making the LUT values -- this is\n> >> important for CPU ISP when CCM is not used (having to use CCM just to\n> >> adjust brightness would be overkill).\n> >\n> > Aha, then indeed lets just apply it directly to the LUT (later) :-)\n> >\n> >> >> + * This is applied before gamma thus a matrix for linear RGB must be used.\n> >> >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0\n> >> >> + * no saturation (monochrome).\n> >> >> + */\n> >> >> + constexpr float r = 0.3086;\n> >> >> + constexpr float g = 0.6094;\n> >> >> + constexpr float b = 0.0820;\n> >> >\n> >> > it would be interesting to dig into where these numbers are derived\n> >> > from exactly ...\n> >> \n> >> I guess some educated magic, similarly to other luminance related\n> >> conversions.\n\nThere are multiple ways to apply saturation to an RGB value:\n\n- Convert to HSV, apply a factor to the saturation, and convert back to\n RGB. This is rarely (if at all) used in hardware as far as I know.\n Software implementations are computationally expensive and can't be\n expressed by a simple matrix multiplication.\n\n- Convert to Y'CbCr, apply a factor to the Cb and Cr components, and\n convert back to RGB. This method is more commonly used in ISPs. As\n they often output YUV data, the conversion to Y'CbCr is already\n present in the pipeline. Note that the saturation is applied in\n non-linear space in this case.\n\n Assuming a CSC matrix that converts from RGB to YUV, a saturation\n factor 'sat' can be applied on RGB data by multiplying the RGB vector\n by\n\n [ 1.0 0.0 0.0 ]\n CSC^-1 * [ 0.0 sat 0.0 ] * CSC\n [ 0.0 0.0 sat ]\n\n- Desaturating the image completely by replacing the R, G and B\n components by a luminance value, and interpolating linearly between\n the saturated and non-saturated pixel values.\n\n I'm also not aware of this method being commonly used in hardware.\n Software implementations are less costly thatn HSV-based saturation as\n they can be expressed by a matrix multiplication. This is what\n https://www.graficaobscura.com/matrix/index.html does.\n\nCan we express the math below more explicitly, based on either the\nY'CbCr method or the interpolation method ? Magic values should also be\nexplained.\n\n> >> > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms\n> >> > seems to report others have looked, but I don't see a direct answer yet\n> >> > (but I won't dig deep) ...\n> >> >\n> >> > And the referenced article is quite helpful.\n> >> >\n> >> > As this is the 'luminance' vector - I wonder if writing it as \n> >> > RGB luminance = { 0.3086, 0.6094, 0.0820 };\n> >> >\n> >> > makes sense...\n> >> >\n> >> >> + const float s1 = 1.0 - saturation;\n> >> >> + ccm = ccm * Matrix{ { s1 * r + saturation, s1 * g, s1 * b,\n> >> >> + s1 * r, s1 * g + saturation, s1 * b,\n> >> >> + s1 * r, s1 * g, s1 * b + saturation } };\n> \n> This multiplication should be also performed in the reverse order.\n> \n> >> >\n> >> > but that would become much more terse ... so I'm not sure it's worth it\n> >> > here? Or would it then be more explicitly readable like the\n> >> > applySaturation in RPi?\n> >> \n> >> I'll try and let's see.\n> >> \n> >> >> +}\n> >> >> +\n> >> >> void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> >> >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> >> >> {\n> >> >> + auto &saturation = context.activeState.knobs.saturation;\n> >> >> +\n> >> >> const unsigned int ct = context.activeState.awb.temperatureK;\n> >> >> \n> >> >> - /* Change CCM only on bigger temperature changes. */\n> >> >> + /* Change CCM only on saturation or bigger temperature changes. */\n> >> >> if (frame > 0 &&\n> >> >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n> >> >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n> >> >> + saturation == lastSaturation_) {\n> >> >> frameContext.ccm.ccm = context.activeState.ccm.ccm;\n> >> >> context.activeState.ccm.changed = false;\n> >> >> return;\n> >> >> }\n> >> >> \n> >> >> lastCt_ = ct;\n> >> >> + lastSaturation_ = saturation;\n> >> >> Matrix ccm = ccm_.getInterpolated(ct);\n> >> >> + if (saturation)\n> >> >> + updateSaturation(ccm, saturation.value());\n> >> >> \n> >> >> context.activeState.ccm.ccm = ccm;\n> >> >> frameContext.ccm.ccm = ccm;\n> >> >> + frameContext.saturation = saturation;\n> >> >> context.activeState.ccm.changed = true;\n> >> >> }\n> >> >> \n> >> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n> >> >> ControlList &metadata)\n> >> >> {\n> >> >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());\n> >> >> +\n> >> >> + const auto &saturation = frameContext.saturation;\n> >> >> + if (saturation)\n> >> >> + metadata.set(controls::Saturation, saturation.value());\n> >> >\n> >> > This matches what we currently do for Contrast, but I wonder if we\n> >> > should report a metadata of '1.0' for both Contrast and Saturation if\n> >> > they aren't set ... as that's what the 'state' is/should be ?\n> >> \n> >> I don't have an opinion on this, both of \"no saturation request -> no\n> >> metadata\" and \"no saturation request -> the corresponding default value\n> >> in metadata\" make sense to me.\n\nAs controls persist, we should keep reporting the value. Every frame\nshould be entirely described by its metadata.\n\n> >> > Anyway, I wouldn't block this patch on that - as it would be on top for\n> >> > both controls...\n> >> >\n> >> > Tested-by: Kieran Bingham \n> >> >\n> >> > Though, while it's functional - looking at the resulting image in\n> >> > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' -\n> >> > with all pixels having roughly the same r=g=b - and I don't think that's\n> >> > the case ... It looks like there's definitely a bias against green. It\n> >> > looks like R=B, but G is always slightly less by at least ~20% ? ...\n> >> > leaving the image looking slightly tinted purple.\n> >> \n> >> Oh, now, under daylight, I get a prominent purple tint too.\n> >> \n> >> > That could be because I'm running an untuned/identity matrix for CCM\n> >> > perhaps? or is it because white balance needs to be taken into account ?\n> >> \n> >> No, it's a math mistake I did in CCM patches. White balance must be\n> >> applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix,\n> >> and P the pixel RGB vector. I thought \"let's apply C after W\" and\n> >> computed (W * C) * P. Which is wrong, to apply C after W means\n> >> C * (W * P) = (C * W) * P. The last matrix applied must be the first\n> >> one in the multiplication. When I swap the multiplication in\n> >> src/ipa/simple/algorithms/lut.cpp from\n> >> \n> >> auto ccm = gainCcm * context.activeState.ccm.ccm;\n> >> \n> >> to\n> >> \n> >> auto ccm = context.activeState.ccm.ccm * gainCcm;\n> >> \n> >> it gets desaturated as it should be (there cannot be any colour cast\n> >> when desaturation is applied correctly, regardless of sensor\n> >> calibration). Maybe this will also explain and improve the colour casts\n> >> I previously experienced when experimenting with my sensor CCMs from\n> >> other pipelines.\n> >> \n> >> > But G = (RB-20%) doesn't sound like white balance to me at the moment...\n> >\n> > aha, so maybe it is ;-)\n> >\n> > Great, well if that's explained ... we have a path forwards. Does it\n> > impact this patch much? Would it help to merge this first? or fix the\n> > CCM then rebase this one ?\n> \n> The latter. I'll post the CCM fix separately. The saturation patch\n> needs a multiplication order fix too and I'd like to simplify\n> updateSaturation() a bit; I'll post v2.\n\nAgreed, fixing the CCM first is better.\n\n> Then there is a question what to do about saturation + contrast. I\n> believe contrast should be applied between sensor-CCM and saturation but\n> this would be too expensive on a CPU.\n\nContrast is typically applied at the end of the processing pipeline, in\nnon-linear YUV space, at the same time as saturation.\n\nOnce we will have a GPU-accelerated implementation of the software ISP,\napplying multiple matrix multiplications in different stages of the\npipeline would become much cheaper. I know this may be a controversial\nopinion, but I think it will be important to have a CPU-based\nimplementation that matches the same processing pipeline as the\nGPU-accelerated implementation, even if it gets costly for the CPU.\n\n> > I think the colour tint is the only thing stopping me from adding a tag,\n> > and if that's clarified, then for Saturation:\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > If you respin with the calculation split out in Ccm::updateSaturation()\n> > you can still collect the tag I think. (perhaps there's a new series\n> > with the above change and the color fix on the way?)\n> >\n> >> >> }\n> >> >> \n> >> >> REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> >> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> >> >> index f4e2b85b..2c5d2170 100644\n> >> >> --- a/src/ipa/simple/algorithms/ccm.h\n> >> >> +++ b/src/ipa/simple/algorithms/ccm.h\n> >> >> @@ -7,6 +7,8 @@\n> >> >> \n> >> >> #pragma once\n> >> >> \n> >> >> +#include \n> >> >> +\n> >> >> #include \"libcamera/internal/matrix.h\"\n> >> >> \n> >> >> #include \n> >> >> @@ -24,6 +26,12 @@ public:\n> >> >> ~Ccm() = default;\n> >> >> \n> >> >> int init(IPAContext &context, const YamlObject &tuningData) override;\n> >> >> + int configure(IPAContext &context,\n> >> >> + const IPAConfigInfo &configInfo) override;\n> >> >> + void queueRequest(typename Module::Context &context,\n> >> >> + const uint32_t frame,\n> >> >> + typename Module::FrameContext &frameContext,\n> >> >> + const ControlList &controls) override;\n> >> >> void prepare(IPAContext &context,\n> >> >> const uint32_t frame,\n> >> >> IPAFrameContext &frameContext,\n> >> >> @@ -34,7 +42,10 @@ public:\n> >> >> ControlList &metadata) override;\n> >> >> \n> >> >> private:\n> >> >> + void updateSaturation(Matrix &ccm, float saturation);\n> >> >> +\n> >> >> unsigned int lastCt_;\n> >> >> + std::optional lastSaturation_;\n> >> >> Interpolator> ccm_;\n> >> >> };\n> >> >> \n> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> >> >> index 88cc6c35..56792981 100644\n> >> >> --- a/src/ipa/simple/ipa_context.h\n> >> >> +++ b/src/ipa/simple/ipa_context.h\n> >> >> @@ -63,6 +63,7 @@ struct IPAActiveState {\n> >> >> struct {\n> >> >> /* 0..2 range, 1.0 = normal */\n> >> >> std::optional contrast;\n> >> >> + std::optional saturation;\n> >> >> } knobs;\n> >> >> };\n> >> >> \n> >> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext {\n> >> >> int32_t exposure;\n> >> >> double gain;\n> >> >> } sensor;\n> >> >> +\n> >> >> struct {\n> >> >> double red;\n> >> >> double blue;\n> >> >> } gains;\n> >> >> +\n> >> >> std::optional contrast;\n> >> >> + std::optional saturation;\n> >> >> };\n> >> >> \n> >> >> struct IPAContext {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B3CFFC327D\n\tfor ;\n\tMon, 14 Apr 2025 16:05:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D35E868AB9;\n\tMon, 14 Apr 2025 18:05:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B371627F2\n\tfor ;\n\tMon, 14 Apr 2025 18:05:30 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 418AB502;\n\tMon, 14 Apr 2025 18:03:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"U4TiEPlJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1744646608;\n\tbh=o8bPHFn6kwgCmF6Fyqr5/dRqum5my+1hj1lhvPH0PTw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U4TiEPlJBxQRiSY1+z1GuSMLG4ECJ46s+mUd9h9i0ou1uSYK26ZPO3Nf8cP3ewZs9\n\tXySURJnnvxdCjwfyhzVP9//YzKbHmVd6h+3ynIkmybSN5mzDqTEPK2xUHol5RKkCqF\n\tEbzgWvyR/gBtuUDTjyWCuDdO1xmY7XARZOJsY5+0=","Date":"Mon, 14 Apr 2025 19:05:28 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","Message-ID":"<20250414160528.GG26798@pendragon.ideasonboard.com>","References":"<20250407085802.16269-1-mzamazal@redhat.com>\n\t<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>\n\t<85r01unath.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<174463792210.2882969.14630378418171625914@ping.linuxembedded.co.uk>\n\t<85mscin6g9.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<85mscin6g9.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34076,"web_url":"https://patchwork.libcamera.org/comment/34076/","msgid":"<85y0vjc724.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-04-29T15:37:39","subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nLaurent Pinchart writes:\n\n> On Mon, Apr 14, 2025 at 04:49:58PM +0200, Milan Zamazal wrote:\n>> Kieran Bingham writes:\n>> > Quoting Milan Zamazal (2025-04-14 14:15:38)\n>\n>> >> Kieran Bingham writes:\n>> >> > Quoting Milan Zamazal (2025-04-07 09:58:02)\n>> >> >> Saturation control is added on top of the colour correction matrix. A\n>> >> >> way of saturation adjustment that can be fully integrated into the\n>> >> >> colour correction matrix is used. The control is available only if Ccm\n>> >> >> algorithm is enabled.\n>> >> >> \n>> >> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified\n>> >> >> saturation, 0.0 full desaturation and 2.0 quite saturated.\n>> >> >> \n>> >> >> The matrix for saturation adjustment is taken from\n>> >> >> https://www.graficaobscura.com/matrix/index.html. The colour correction\n>> >> >> matrix is applied before gamma and the given matrix is suitable for such\n>> >> >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp\n>> >> >> could be used.\n>> >> >> \n>> >> >> Signed-off-by: Milan Zamazal \n>> >> >> ---\n>> >> >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++--\n>> >> >> src/ipa/simple/algorithms/ccm.h | 11 ++++++\n>> >> >> src/ipa/simple/ipa_context.h | 4 +++\n>> >> >> 3 files changed, 69 insertions(+), 3 deletions(-)\n>> >> >> \n>> >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> >> >> index d5ba928d..2700a247 100644\n>> >> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n>> >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> >> >> @@ -3,7 +3,7 @@\n>> >> >> * Copyright (C) 2024, Ideas On Board\n>> >> >> * Copyright (C) 2024-2025, Red Hat Inc.\n>> >> >> *\n>> >> >> - * Color correction matrix\n>> >> >> + * Color correction matrix + saturation\n>> >> >> */\n>> >> >> \n>> >> >> #include \"ccm.h\"\n>> >> >> @@ -13,6 +13,8 @@\n>> >> >> \n>> >> >> #include \n>> >> >> \n>> >> >> +#include \"libcamera/internal/matrix.h\"\n>> >> >> +\n>> >> >> namespace {\n>> >> >> \n>> >> >> constexpr unsigned int kTemperatureThreshold = 100;\n>> >> >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n>> >> >> }\n>> >> >> \n>> >> >> context.ccmEnabled = true;\n>> >> >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n>> >> >> +\n>> >> >> + return 0;\n>> >> >> +}\n>> >> >> +\n>> >> >> +int Ccm::configure(IPAContext &context,\n>> >> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> >> >> +{\n>> >> >> + context.activeState.knobs.saturation = std::optional();\n>> >> >> \n>> >> >> return 0;\n>> >> >> }\n>> >> >> \n>> >> >> +void Ccm::queueRequest(typename Module::Context &context,\n>> >> >> + [[maybe_unused]] const uint32_t frame,\n>> >> >> + [[maybe_unused]] typename Module::FrameContext &frameContext,\n>> >> >> + const ControlList &controls)\n>> >> >> +{\n>> >> >> + const auto &saturation = controls.get(controls::Saturation);\n>> >> >> + if (saturation.has_value()) {\n>> >> >> + context.activeState.knobs.saturation = saturation;\n>> >> >> + LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n>> >> >> + }\n>> >> >> +}\n>> >> >> +\n>> >> >> +void Ccm::updateSaturation(Matrix &ccm, float saturation)\n>> >> >> +{\n>> >> >> + /*\n>> >> >> + * See https://www.graficaobscura.com/matrix/index.html.\n>> >> >\n>> >> > Looks like it would be easy to add a brightness control too after this.\n>> >> \n>> >> Yes, when CCM is used, it's easy to add brightness. And when CCM is not\n>> >> used, it's easy too, applying it when making the LUT values -- this is\n>> >> important for CPU ISP when CCM is not used (having to use CCM just to\n>> >> adjust brightness would be overkill).\n>> >\n>> > Aha, then indeed lets just apply it directly to the LUT (later) :-)\n>> >\n>> >> >> + * This is applied before gamma thus a matrix for linear RGB must be used.\n>> >> >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0\n>> >> >> + * no saturation (monochrome).\n>> >> >> + */\n>> >> >> + constexpr float r = 0.3086;\n>> >> >> + constexpr float g = 0.6094;\n>> >> >> + constexpr float b = 0.0820;\n>> >> >\n>> >> > it would be interesting to dig into where these numbers are derived\n>> >> > from exactly ...\n>> >> \n>> >> I guess some educated magic, similarly to other luminance related\n>> >> conversions.\n>\n> There are multiple ways to apply saturation to an RGB value:\n>\n> - Convert to HSV, apply a factor to the saturation, and convert back to\n> RGB. This is rarely (if at all) used in hardware as far as I know.\n> Software implementations are computationally expensive and can't be\n> expressed by a simple matrix multiplication.\n>\n> - Convert to Y'CbCr, apply a factor to the Cb and Cr components, and\n> convert back to RGB. This method is more commonly used in ISPs. As\n> they often output YUV data, the conversion to Y'CbCr is already\n> present in the pipeline. Note that the saturation is applied in\n> non-linear space in this case.\n>\n> Assuming a CSC matrix that converts from RGB to YUV, a saturation\n> factor 'sat' can be applied on RGB data by multiplying the RGB vector\n> by\n>\n> [ 1.0 0.0 0.0 ]\n> CSC^-1 * [ 0.0 sat 0.0 ] * CSC\n> [ 0.0 0.0 sat ]\n\nI initially thought about this, but the way below looked simpler. Well,\nactually, the Y'CbCr method is not more complicated and it works for me.\nExcept that I don't know which of the ITU-R BT.* conversions is best to\nuse in libcamera. Is there an established practice already?\n\n> - Desaturating the image completely by replacing the R, G and B\n> components by a luminance value, and interpolating linearly between\n> the saturated and non-saturated pixel values.\n>\n> I'm also not aware of this method being commonly used in hardware.\n> Software implementations are less costly thatn HSV-based saturation as\n> they can be expressed by a matrix multiplication. This is what\n> https://www.graficaobscura.com/matrix/index.html does.\n>\n> Can we express the math below more explicitly, based on either the\n> Y'CbCr method or the interpolation method ? Magic values should also be\n> explained.\n\nI'm afraid I'm completely ignorant about the math behind the conversions\nand I don't know what's the primary ultimate source. But I believe the\nY'CbCr methods are well established, \"ITU-R BT\" looks like a good enough\nstandard body behind to accept the values without further questioning.\n\n>> >> > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms\n>> >> > seems to report others have looked, but I don't see a direct answer yet\n>> >> > (but I won't dig deep) ...\n>> >> >\n>> >> > And the referenced article is quite helpful.\n>> >> >\n>> >> > As this is the 'luminance' vector - I wonder if writing it as \n>> >> > RGB luminance = { 0.3086, 0.6094, 0.0820 };\n>> >> >\n>> >> > makes sense...\n>> >> >\n>> >> >> + const float s1 = 1.0 - saturation;\n>> >> >> + ccm = ccm * Matrix{ { s1 * r + saturation, s1 * g, s1 * b,\n>> >> >> + s1 * r, s1 * g + saturation, s1 * b,\n>> >> >> + s1 * r, s1 * g, s1 * b + saturation } };\n>> \n>> This multiplication should be also performed in the reverse order.\n>> \n>> >> >\n>> >> > but that would become much more terse ... so I'm not sure it's worth it\n>> >> > here? Or would it then be more explicitly readable like the\n>> >> > applySaturation in RPi?\n>> >> \n>> >> I'll try and let's see.\n>> >> \n>> >> >> +}\n>> >> >> +\n>> >> >> void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>> >> >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> >> >> {\n>> >> >> + auto &saturation = context.activeState.knobs.saturation;\n>> >> >> +\n>> >> >> const unsigned int ct = context.activeState.awb.temperatureK;\n>> >> >> \n>> >> >> - /* Change CCM only on bigger temperature changes. */\n>> >> >> + /* Change CCM only on saturation or bigger temperature changes. */\n>> >> >> if (frame > 0 &&\n>> >> >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n>> >> >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n>> >> >> + saturation == lastSaturation_) {\n>> >> >> frameContext.ccm.ccm = context.activeState.ccm.ccm;\n>> >> >> context.activeState.ccm.changed = false;\n>> >> >> return;\n>> >> >> }\n>> >> >> \n>> >> >> lastCt_ = ct;\n>> >> >> + lastSaturation_ = saturation;\n>> >> >> Matrix ccm = ccm_.getInterpolated(ct);\n>> >> >> + if (saturation)\n>> >> >> + updateSaturation(ccm, saturation.value());\n>> >> >> \n>> >> >> context.activeState.ccm.ccm = ccm;\n>> >> >> frameContext.ccm.ccm = ccm;\n>> >> >> + frameContext.saturation = saturation;\n>> >> >> context.activeState.ccm.changed = true;\n>> >> >> }\n>> >> >> \n>> >> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n>> >> >> ControlList &metadata)\n>> >> >> {\n>> >> >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());\n>> >> >> +\n>> >> >> + const auto &saturation = frameContext.saturation;\n>> >> >> + if (saturation)\n>> >> >> + metadata.set(controls::Saturation, saturation.value());\n>> >> >\n>> >> > This matches what we currently do for Contrast, but I wonder if we\n>> >> > should report a metadata of '1.0' for both Contrast and Saturation if\n>> >> > they aren't set ... as that's what the 'state' is/should be ?\n>> >> \n>> >> I don't have an opinion on this, both of \"no saturation request -> no\n>> >> metadata\" and \"no saturation request -> the corresponding default value\n>> >> in metadata\" make sense to me.\n>\n> As controls persist, we should keep reporting the value. Every frame\n> should be entirely described by its metadata.\n\nOK.\n\n>> >> > Anyway, I wouldn't block this patch on that - as it would be on top for\n>> >> > both controls...\n>> >> >\n>> >> > Tested-by: Kieran Bingham \n>> >> >\n>> >> > Though, while it's functional - looking at the resulting image in\n>> >> > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' -\n>> >> > with all pixels having roughly the same r=g=b - and I don't think that's\n>> >> > the case ... It looks like there's definitely a bias against green. It\n>> >> > looks like R=B, but G is always slightly less by at least ~20% ? ...\n>> >> > leaving the image looking slightly tinted purple.\n>> >> \n>> >> Oh, now, under daylight, I get a prominent purple tint too.\n>> >> \n>> >> > That could be because I'm running an untuned/identity matrix for CCM\n>> >> > perhaps? or is it because white balance needs to be taken into account ?\n>> >> \n>> >> No, it's a math mistake I did in CCM patches. White balance must be\n>> >> applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix,\n>> >> and P the pixel RGB vector. I thought \"let's apply C after W\" and\n>> >> computed (W * C) * P. Which is wrong, to apply C after W means\n>> >> C * (W * P) = (C * W) * P. The last matrix applied must be the first\n>> >> one in the multiplication. When I swap the multiplication in\n>> >> src/ipa/simple/algorithms/lut.cpp from\n>> >> \n>> >> auto ccm = gainCcm * context.activeState.ccm.ccm;\n>> >> \n>> >> to\n>> >> \n>> >> auto ccm = context.activeState.ccm.ccm * gainCcm;\n>> >> \n>> >> it gets desaturated as it should be (there cannot be any colour cast\n>> >> when desaturation is applied correctly, regardless of sensor\n>> >> calibration). Maybe this will also explain and improve the colour casts\n>> >> I previously experienced when experimenting with my sensor CCMs from\n>> >> other pipelines.\n>> >> \n>> >> > But G = (RB-20%) doesn't sound like white balance to me at the moment...\n>> >\n>> > aha, so maybe it is ;-)\n>> >\n>> > Great, well if that's explained ... we have a path forwards. Does it\n>> > impact this patch much? Would it help to merge this first? or fix the\n>> > CCM then rebase this one ?\n>> \n>> The latter. I'll post the CCM fix separately. The saturation patch\n>> needs a multiplication order fix too and I'd like to simplify\n>> updateSaturation() a bit; I'll post v2.\n>\n> Agreed, fixing the CCM first is better.\n\nDone in the meantime.\n\n>> Then there is a question what to do about saturation + contrast. I\n>> believe contrast should be applied between sensor-CCM and saturation but\n>> this would be too expensive on a CPU.\n>\n> Contrast is typically applied at the end of the processing pipeline, in\n> non-linear YUV space, at the same time as saturation.\n\nBut (a reasonable) contrast adjustment is still non-linear, right? Then\nwe have\n\n CCM -> RGB2YCbCr -> contrast + saturation -> YCbCr2RGB\n\nthat cannot be collapsed into a single matrix multiplication.\n\n> Once we will have a GPU-accelerated implementation of the software ISP,\n> applying multiple matrix multiplications in different stages of the\n> pipeline would become much cheaper. I know this may be a controversial\n> opinion, but I think it will be important to have a CPU-based\n> implementation that matches the same processing pipeline as the\n> GPU-accelerated implementation, even if it gets costly for the CPU.\n\nThere can still be a need to run a camera on systems without working\nGPU. Then it would be odd to have a CPU implementation running\nunnecessarily slow on purpose. Perhaps we can decide what can be run\nreasonably on CPU and have a configuration option to switch between a\nreference and efficient implementations?\n\n>> > I think the colour tint is the only thing stopping me from adding a tag,\n>> > and if that's clarified, then for Saturation:\n>> >\n>> > Reviewed-by: Kieran Bingham \n>> >\n>> > If you respin with the calculation split out in Ccm::updateSaturation()\n>> > you can still collect the tag I think. (perhaps there's a new series\n>> > with the above change and the color fix on the way?)\n>> >\n>> >> >> }\n>> >> >> \n>> >> >> REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n>> >> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n>> >> >> index f4e2b85b..2c5d2170 100644\n>> >> >> --- a/src/ipa/simple/algorithms/ccm.h\n>> >> >> +++ b/src/ipa/simple/algorithms/ccm.h\n>> >> >> @@ -7,6 +7,8 @@\n>> >> >> \n>> >> >> #pragma once\n>> >> >> \n>> >> >> +#include \n>> >> >> +\n>> >> >> #include \"libcamera/internal/matrix.h\"\n>> >> >> \n>> >> >> #include \n>> >> >> @@ -24,6 +26,12 @@ public:\n>> >> >> ~Ccm() = default;\n>> >> >> \n>> >> >> int init(IPAContext &context, const YamlObject &tuningData) override;\n>> >> >> + int configure(IPAContext &context,\n>> >> >> + const IPAConfigInfo &configInfo) override;\n>> >> >> + void queueRequest(typename Module::Context &context,\n>> >> >> + const uint32_t frame,\n>> >> >> + typename Module::FrameContext &frameContext,\n>> >> >> + const ControlList &controls) override;\n>> >> >> void prepare(IPAContext &context,\n>> >> >> const uint32_t frame,\n>> >> >> IPAFrameContext &frameContext,\n>> >> >> @@ -34,7 +42,10 @@ public:\n>> >> >> ControlList &metadata) override;\n>> >> >> \n>> >> >> private:\n>> >> >> + void updateSaturation(Matrix &ccm, float saturation);\n>> >> >> +\n>> >> >> unsigned int lastCt_;\n>> >> >> + std::optional lastSaturation_;\n>> >> >> Interpolator> ccm_;\n>> >> >> };\n>> >> >> \n>> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> >> >> index 88cc6c35..56792981 100644\n>> >> >> --- a/src/ipa/simple/ipa_context.h\n>> >> >> +++ b/src/ipa/simple/ipa_context.h\n>> >> >> @@ -63,6 +63,7 @@ struct IPAActiveState {\n>> >> >> struct {\n>> >> >> /* 0..2 range, 1.0 = normal */\n>> >> >> std::optional contrast;\n>> >> >> + std::optional saturation;\n>> >> >> } knobs;\n>> >> >> };\n>> >> >> \n>> >> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext {\n>> >> >> int32_t exposure;\n>> >> >> double gain;\n>> >> >> } sensor;\n>> >> >> +\n>> >> >> struct {\n>> >> >> double red;\n>> >> >> double blue;\n>> >> >> } gains;\n>> >> >> +\n>> >> >> std::optional contrast;\n>> >> >> + std::optional saturation;\n>> >> >> };\n>> >> >> \n>> >> >> struct IPAContext {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B2B7EBE08B\n\tfor ;\n\tTue, 29 Apr 2025 15:37:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B821768AD3;\n\tTue, 29 Apr 2025 17:37:47 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C138617DB\n\tfor ;\n\tTue, 29 Apr 2025 17:37:46 +0200 (CEST)","from mail-ed1-f72.google.com (mail-ed1-f72.google.com\n\t[209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-517-_Pd1MtawMleGNcL8y80xjQ-1; Tue, 29 Apr 2025 11:37:43 -0400","by mail-ed1-f72.google.com with SMTP id\n\t4fb4d7f45d1cf-5eb80d465b7so5604098a12.3\n\tfor ;\n\tTue, 29 Apr 2025 08:37:42 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5f85387a0f8sm729745a12.75.2025.04.29.08.37.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 29 Apr 2025 08:37:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"ipiXq/50\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1745941065;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=3BliUXtAyC1LgRVpMUvDwGhdQ2eIKaW139JBW4vGMzY=;\n\tb=ipiXq/50+eXZt/3nqahhkSlaXB7XoT6HLnHOOWaUWlc/0jgDA/b5g+UDRyUlgV37PcN2D6\n\taatA8iv/c1Q8n7w8qyaYIaCX030kniCqaa/WBYXG39lAy5Xts5v+Zu0WRtkbeptonRYjGO\n\tNnuVT6P66Ayvw/ukgxjA+C8/xbiWjIk=","X-MC-Unique":"_Pd1MtawMleGNcL8y80xjQ-1","X-Mimecast-MFC-AGG-ID":"_Pd1MtawMleGNcL8y80xjQ_1745941062","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1745941061; x=1746545861;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=3BliUXtAyC1LgRVpMUvDwGhdQ2eIKaW139JBW4vGMzY=;\n\tb=hVYQhkyAY2q2eT8MzKmEudAr6JXbbntQTBV75LxNZBB7Csm4z+shadRmNvIIhm04eE\n\tsneVZ1wpm64B4zGs1RLecOA5nVndK06eBNp339hJUrT0SAFQ+JmyzbnXRapgVSn5lChY\n\tR6jGCRAftZZYdCxwdyO4/J4cu02k/YJ28EeDgGLikJxdq0G5BA4E4qnvm6pDmcvrdsQT\n\tQWBLi3DfSNC3XIYaJebybNU/sXS2tMiyvnhqzBlRJOyZNrn12zAeFQzQM5uQMNYk5wTu\n\tnoGM7AJ+KiiP75wFXffKdp/1jY9vigmXAbDhuEUIEBCRmuEDzejusReVquGvHvJzw3sK\n\tIX2A==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWdEBq9MD4xRt1/XpF51gneafDQvI6T78/ZU3XKioIBJF2BQZRLoIl3sRVys3X/6bxwrfS5zO8LaQ4/FlLTpgs=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Ywv0rHyXSGgM/sVAtD4lmD7bz9eba18CwQ5WqNVSH1xaTLq3b/O\n\taf0IaOXEo7WlcC7aAK1M6F+deE44G5gFWQhhfV1AoHIDrIDAgz1oavysrQtuTEk6p4E4bP3e+a+\n\t/2gwHMr4GN1BaT465MXN1/BvawLD8zfU8zXWK9MtF9gFMQgwjfY7ae0/8zAWOMafuJO+GBdtquv\n\tPUGpqiFHH41nafxbOaD667sFXelz4N96Gvn2pLhxTbp0PvDGaRASa9J70=","X-Gm-Gg":"ASbGncsBCneN7UM7/51cHED0Y5m5PJ/zQsekI1z4oIQ/nu1egs7A7NSfFPKLJaSNCCt\n\t6DgbjAADH9fRrjbwJ3jGb03azJFMwSOB2QkdwYqh0ZKlV+ZPdECcMe/B2MUyocU+JH7bOpwaSkb\n\tM07KyMbSd3CSpCitCi6520cpcvXmrxU8Bsryy0YXKN5X6/AhcdKBjOBRl7bybnd82rdNJNDT9Kf\n\tp34f4VwiSf0Hu/iq2epzJKojXC1du33zDTEi9KPKYv1ube1mGU0wPcUGqUf9IYMb2Ts4VfFcH5q\n\tEicKKagSfF/HHHCaumChIwoJD9Wi117t4d9iVMWnZ8Zt8QNZ8dtEPEo+0W/nEh/g","X-Received":["by 2002:a50:cc06:0:b0:5f8:3b33:f838 with SMTP id\n\t4fb4d7f45d1cf-5f83b33f878mr2413364a12.0.1745941061287; \n\tTue, 29 Apr 2025 08:37:41 -0700 (PDT)","by 2002:a50:cc06:0:b0:5f8:3b33:f838 with SMTP id\n\t4fb4d7f45d1cf-5f83b33f878mr2413338a12.0.1745941060647; \n\tTue, 29 Apr 2025 08:37:40 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHb+IBtFiXYnLGtrCng5e3lQkTTvU4FqRoZtW1uMPRdqEgDbYgASbrkifXcXBPolsiw6o//jg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","In-Reply-To":"<20250414160528.GG26798@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 14 Apr 2025 19:05:28 +0300\")","References":"<20250407085802.16269-1-mzamazal@redhat.com>\n\t<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>\n\t<85r01unath.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<174463792210.2882969.14630378418171625914@ping.linuxembedded.co.uk>\n\t<85mscin6g9.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<20250414160528.GG26798@pendragon.ideasonboard.com>","Date":"Tue, 29 Apr 2025 17:37:39 +0200","Message-ID":"<85y0vjc724.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"z9l38aekANYhc5xYDuCaacl3cvjz6CFefWgP64vieyM_1745941062","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34249,"web_url":"https://patchwork.libcamera.org/comment/34249/","msgid":"<20250515220657.GA11649@pendragon.ideasonboard.com>","date":"2025-05-15T22:06:57","subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Tue, Apr 29, 2025 at 05:37:39PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Mon, Apr 14, 2025 at 04:49:58PM +0200, Milan Zamazal wrote:\n> >> Kieran Bingham writes:\n> >> > Quoting Milan Zamazal (2025-04-14 14:15:38)\n> >> >> Kieran Bingham writes:\n> >> >> > Quoting Milan Zamazal (2025-04-07 09:58:02)\n> >> >> >> Saturation control is added on top of the colour correction matrix. A\n> >> >> >> way of saturation adjustment that can be fully integrated into the\n> >> >> >> colour correction matrix is used. The control is available only if Ccm\n> >> >> >> algorithm is enabled.\n> >> >> >> \n> >> >> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified\n> >> >> >> saturation, 0.0 full desaturation and 2.0 quite saturated.\n> >> >> >> \n> >> >> >> The matrix for saturation adjustment is taken from\n> >> >> >> https://www.graficaobscura.com/matrix/index.html. The colour correction\n> >> >> >> matrix is applied before gamma and the given matrix is suitable for such\n> >> >> >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp\n> >> >> >> could be used.\n> >> >> >> \n> >> >> >> Signed-off-by: Milan Zamazal \n> >> >> >> ---\n> >> >> >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++--\n> >> >> >> src/ipa/simple/algorithms/ccm.h | 11 ++++++\n> >> >> >> src/ipa/simple/ipa_context.h | 4 +++\n> >> >> >> 3 files changed, 69 insertions(+), 3 deletions(-)\n> >> >> >> \n> >> >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> >> >> >> index d5ba928d..2700a247 100644\n> >> >> >> --- a/src/ipa/simple/algorithms/ccm.cpp\n> >> >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> >> >> >> @@ -3,7 +3,7 @@\n> >> >> >> * Copyright (C) 2024, Ideas On Board\n> >> >> >> * Copyright (C) 2024-2025, Red Hat Inc.\n> >> >> >> *\n> >> >> >> - * Color correction matrix\n> >> >> >> + * Color correction matrix + saturation\n> >> >> >> */\n> >> >> >> \n> >> >> >> #include \"ccm.h\"\n> >> >> >> @@ -13,6 +13,8 @@\n> >> >> >> \n> >> >> >> #include \n> >> >> >> \n> >> >> >> +#include \"libcamera/internal/matrix.h\"\n> >> >> >> +\n> >> >> >> namespace {\n> >> >> >> \n> >> >> >> constexpr unsigned int kTemperatureThreshold = 100;\n> >> >> >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> >> >> >> }\n> >> >> >> \n> >> >> >> context.ccmEnabled = true;\n> >> >> >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n> >> >> >> +\n> >> >> >> + return 0;\n> >> >> >> +}\n> >> >> >> +\n> >> >> >> +int Ccm::configure(IPAContext &context,\n> >> >> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >> >> >> +{\n> >> >> >> + context.activeState.knobs.saturation = std::optional();\n> >> >> >> \n> >> >> >> return 0;\n> >> >> >> }\n> >> >> >> \n> >> >> >> +void Ccm::queueRequest(typename Module::Context &context,\n> >> >> >> + [[maybe_unused]] const uint32_t frame,\n> >> >> >> + [[maybe_unused]] typename Module::FrameContext &frameContext,\n> >> >> >> + const ControlList &controls)\n> >> >> >> +{\n> >> >> >> + const auto &saturation = controls.get(controls::Saturation);\n> >> >> >> + if (saturation.has_value()) {\n> >> >> >> + context.activeState.knobs.saturation = saturation;\n> >> >> >> + LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n> >> >> >> + }\n> >> >> >> +}\n> >> >> >> +\n> >> >> >> +void Ccm::updateSaturation(Matrix &ccm, float saturation)\n> >> >> >> +{\n> >> >> >> + /*\n> >> >> >> + * See https://www.graficaobscura.com/matrix/index.html.\n> >> >> >\n> >> >> > Looks like it would be easy to add a brightness control too after this.\n> >> >> \n> >> >> Yes, when CCM is used, it's easy to add brightness. And when CCM is not\n> >> >> used, it's easy too, applying it when making the LUT values -- this is\n> >> >> important for CPU ISP when CCM is not used (having to use CCM just to\n> >> >> adjust brightness would be overkill).\n> >> >\n> >> > Aha, then indeed lets just apply it directly to the LUT (later) :-)\n> >> >\n> >> >> >> + * This is applied before gamma thus a matrix for linear RGB must be used.\n> >> >> >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0\n> >> >> >> + * no saturation (monochrome).\n> >> >> >> + */\n> >> >> >> + constexpr float r = 0.3086;\n> >> >> >> + constexpr float g = 0.6094;\n> >> >> >> + constexpr float b = 0.0820;\n> >> >> >\n> >> >> > it would be interesting to dig into where these numbers are derived\n> >> >> > from exactly ...\n> >> >> \n> >> >> I guess some educated magic, similarly to other luminance related\n> >> >> conversions.\n> >\n> > There are multiple ways to apply saturation to an RGB value:\n> >\n> > - Convert to HSV, apply a factor to the saturation, and convert back to\n> > RGB. This is rarely (if at all) used in hardware as far as I know.\n> > Software implementations are computationally expensive and can't be\n> > expressed by a simple matrix multiplication.\n> >\n> > - Convert to Y'CbCr, apply a factor to the Cb and Cr components, and\n> > convert back to RGB. This method is more commonly used in ISPs. As\n> > they often output YUV data, the conversion to Y'CbCr is already\n> > present in the pipeline. Note that the saturation is applied in\n> > non-linear space in this case.\n> >\n> > Assuming a CSC matrix that converts from RGB to YUV, a saturation\n> > factor 'sat' can be applied on RGB data by multiplying the RGB vector\n> > by\n> >\n> > [ 1.0 0.0 0.0 ]\n> > CSC^-1 * [ 0.0 sat 0.0 ] * CSC\n> > [ 0.0 0.0 sat ]\n> \n> I initially thought about this, but the way below looked simpler. Well,\n> actually, the Y'CbCr method is not more complicated and it works for me.\n> Except that I don't know which of the ITU-R BT.* conversions is best to\n> use in libcamera. Is there an established practice already?\n\nWhen capturing YUV formats, applications can select the RGB to YUV\nconversion matrix through the color space. When capturing RGB formats,\nhowever, I'm not sure what CSC would be best in the formula above, and\nhow much difference it would make.\n\n> > - Desaturating the image completely by replacing the R, G and B\n> > components by a luminance value, and interpolating linearly between\n> > the saturated and non-saturated pixel values.\n> >\n> > I'm also not aware of this method being commonly used in hardware.\n> > Software implementations are less costly thatn HSV-based saturation as\n> > they can be expressed by a matrix multiplication. This is what\n> > https://www.graficaobscura.com/matrix/index.html does.\n> >\n> > Can we express the math below more explicitly, based on either the\n> > Y'CbCr method or the interpolation method ? Magic values should also be\n> > explained.\n> \n> I'm afraid I'm completely ignorant about the math behind the conversions\n> and I don't know what's the primary ultimate source. But I believe the\n> Y'CbCr methods are well established, \"ITU-R BT\" looks like a good enough\n> standard body behind to accept the values without further questioning.\n> \n> >> >> > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms\n> >> >> > seems to report others have looked, but I don't see a direct answer yet\n> >> >> > (but I won't dig deep) ...\n> >> >> >\n> >> >> > And the referenced article is quite helpful.\n> >> >> >\n> >> >> > As this is the 'luminance' vector - I wonder if writing it as \n> >> >> > RGB luminance = { 0.3086, 0.6094, 0.0820 };\n> >> >> >\n> >> >> > makes sense...\n> >> >> >\n> >> >> >> + const float s1 = 1.0 - saturation;\n> >> >> >> + ccm = ccm * Matrix{ { s1 * r + saturation, s1 * g, s1 * b,\n> >> >> >> + s1 * r, s1 * g + saturation, s1 * b,\n> >> >> >> + s1 * r, s1 * g, s1 * b + saturation } };\n> >> \n> >> This multiplication should be also performed in the reverse order.\n> >> \n> >> >> >\n> >> >> > but that would become much more terse ... so I'm not sure it's worth it\n> >> >> > here? Or would it then be more explicitly readable like the\n> >> >> > applySaturation in RPi?\n> >> >> \n> >> >> I'll try and let's see.\n> >> >> \n> >> >> >> +}\n> >> >> >> +\n> >> >> >> void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> >> >> >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> >> >> >> {\n> >> >> >> + auto &saturation = context.activeState.knobs.saturation;\n> >> >> >> +\n> >> >> >> const unsigned int ct = context.activeState.awb.temperatureK;\n> >> >> >> \n> >> >> >> - /* Change CCM only on bigger temperature changes. */\n> >> >> >> + /* Change CCM only on saturation or bigger temperature changes. */\n> >> >> >> if (frame > 0 &&\n> >> >> >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n> >> >> >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n> >> >> >> + saturation == lastSaturation_) {\n> >> >> >> frameContext.ccm.ccm = context.activeState.ccm.ccm;\n> >> >> >> context.activeState.ccm.changed = false;\n> >> >> >> return;\n> >> >> >> }\n> >> >> >> \n> >> >> >> lastCt_ = ct;\n> >> >> >> + lastSaturation_ = saturation;\n> >> >> >> Matrix ccm = ccm_.getInterpolated(ct);\n> >> >> >> + if (saturation)\n> >> >> >> + updateSaturation(ccm, saturation.value());\n> >> >> >> \n> >> >> >> context.activeState.ccm.ccm = ccm;\n> >> >> >> frameContext.ccm.ccm = ccm;\n> >> >> >> + frameContext.saturation = saturation;\n> >> >> >> context.activeState.ccm.changed = true;\n> >> >> >> }\n> >> >> >> \n> >> >> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n> >> >> >> ControlList &metadata)\n> >> >> >> {\n> >> >> >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());\n> >> >> >> +\n> >> >> >> + const auto &saturation = frameContext.saturation;\n> >> >> >> + if (saturation)\n> >> >> >> + metadata.set(controls::Saturation, saturation.value());\n> >> >> >\n> >> >> > This matches what we currently do for Contrast, but I wonder if we\n> >> >> > should report a metadata of '1.0' for both Contrast and Saturation if\n> >> >> > they aren't set ... as that's what the 'state' is/should be ?\n> >> >> \n> >> >> I don't have an opinion on this, both of \"no saturation request -> no\n> >> >> metadata\" and \"no saturation request -> the corresponding default value\n> >> >> in metadata\" make sense to me.\n> >\n> > As controls persist, we should keep reporting the value. Every frame\n> > should be entirely described by its metadata.\n> \n> OK.\n> \n> >> >> > Anyway, I wouldn't block this patch on that - as it would be on top for\n> >> >> > both controls...\n> >> >> >\n> >> >> > Tested-by: Kieran Bingham \n> >> >> >\n> >> >> > Though, while it's functional - looking at the resulting image in\n> >> >> > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' -\n> >> >> > with all pixels having roughly the same r=g=b - and I don't think that's\n> >> >> > the case ... It looks like there's definitely a bias against green. It\n> >> >> > looks like R=B, but G is always slightly less by at least ~20% ? ...\n> >> >> > leaving the image looking slightly tinted purple.\n> >> >> \n> >> >> Oh, now, under daylight, I get a prominent purple tint too.\n> >> >> \n> >> >> > That could be because I'm running an untuned/identity matrix for CCM\n> >> >> > perhaps? or is it because white balance needs to be taken into account ?\n> >> >> \n> >> >> No, it's a math mistake I did in CCM patches. White balance must be\n> >> >> applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix,\n> >> >> and P the pixel RGB vector. I thought \"let's apply C after W\" and\n> >> >> computed (W * C) * P. Which is wrong, to apply C after W means\n> >> >> C * (W * P) = (C * W) * P. The last matrix applied must be the first\n> >> >> one in the multiplication. When I swap the multiplication in\n> >> >> src/ipa/simple/algorithms/lut.cpp from\n> >> >> \n> >> >> auto ccm = gainCcm * context.activeState.ccm.ccm;\n> >> >> \n> >> >> to\n> >> >> \n> >> >> auto ccm = context.activeState.ccm.ccm * gainCcm;\n> >> >> \n> >> >> it gets desaturated as it should be (there cannot be any colour cast\n> >> >> when desaturation is applied correctly, regardless of sensor\n> >> >> calibration). Maybe this will also explain and improve the colour casts\n> >> >> I previously experienced when experimenting with my sensor CCMs from\n> >> >> other pipelines.\n> >> >> \n> >> >> > But G = (RB-20%) doesn't sound like white balance to me at the moment...\n> >> >\n> >> > aha, so maybe it is ;-)\n> >> >\n> >> > Great, well if that's explained ... we have a path forwards. Does it\n> >> > impact this patch much? Would it help to merge this first? or fix the\n> >> > CCM then rebase this one ?\n> >> \n> >> The latter. I'll post the CCM fix separately. The saturation patch\n> >> needs a multiplication order fix too and I'd like to simplify\n> >> updateSaturation() a bit; I'll post v2.\n> >\n> > Agreed, fixing the CCM first is better.\n> \n> Done in the meantime.\n> \n> >> Then there is a question what to do about saturation + contrast. I\n> >> believe contrast should be applied between sensor-CCM and saturation but\n> >> this would be too expensive on a CPU.\n> >\n> > Contrast is typically applied at the end of the processing pipeline, in\n> > non-linear YUV space, at the same time as saturation.\n> \n> But (a reasonable) contrast adjustment is still non-linear, right? Then\n> we have\n> \n> CCM -> RGB2YCbCr -> contrast + saturation -> YCbCr2RGB\n> \n> that cannot be collapsed into a single matrix multiplication.\n\nBy non-linear I meant after gamma correction.\n\n> > Once we will have a GPU-accelerated implementation of the software ISP,\n> > applying multiple matrix multiplications in different stages of the\n> > pipeline would become much cheaper. I know this may be a controversial\n> > opinion, but I think it will be important to have a CPU-based\n> > implementation that matches the same processing pipeline as the\n> > GPU-accelerated implementation, even if it gets costly for the CPU.\n> \n> There can still be a need to run a camera on systems without working\n> GPU. Then it would be odd to have a CPU implementation running\n> unnecessarily slow on purpose. Perhaps we can decide what can be run\n> reasonably on CPU and have a configuration option to switch between a\n> reference and efficient implementations?\n\nThat could be an option. We'll discuss the topic during the libcamera\nworkshop tomorrow.\n\n> >> > I think the colour tint is the only thing stopping me from adding a tag,\n> >> > and if that's clarified, then for Saturation:\n> >> >\n> >> > Reviewed-by: Kieran Bingham \n> >> >\n> >> > If you respin with the calculation split out in Ccm::updateSaturation()\n> >> > you can still collect the tag I think. (perhaps there's a new series\n> >> > with the above change and the color fix on the way?)\n> >> >\n> >> >> >> }\n> >> >> >> \n> >> >> >> REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> >> >> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> >> >> >> index f4e2b85b..2c5d2170 100644\n> >> >> >> --- a/src/ipa/simple/algorithms/ccm.h\n> >> >> >> +++ b/src/ipa/simple/algorithms/ccm.h\n> >> >> >> @@ -7,6 +7,8 @@\n> >> >> >> \n> >> >> >> #pragma once\n> >> >> >> \n> >> >> >> +#include \n> >> >> >> +\n> >> >> >> #include \"libcamera/internal/matrix.h\"\n> >> >> >> \n> >> >> >> #include \n> >> >> >> @@ -24,6 +26,12 @@ public:\n> >> >> >> ~Ccm() = default;\n> >> >> >> \n> >> >> >> int init(IPAContext &context, const YamlObject &tuningData) override;\n> >> >> >> + int configure(IPAContext &context,\n> >> >> >> + const IPAConfigInfo &configInfo) override;\n> >> >> >> + void queueRequest(typename Module::Context &context,\n> >> >> >> + const uint32_t frame,\n> >> >> >> + typename Module::FrameContext &frameContext,\n> >> >> >> + const ControlList &controls) override;\n> >> >> >> void prepare(IPAContext &context,\n> >> >> >> const uint32_t frame,\n> >> >> >> IPAFrameContext &frameContext,\n> >> >> >> @@ -34,7 +42,10 @@ public:\n> >> >> >> ControlList &metadata) override;\n> >> >> >> \n> >> >> >> private:\n> >> >> >> + void updateSaturation(Matrix &ccm, float saturation);\n> >> >> >> +\n> >> >> >> unsigned int lastCt_;\n> >> >> >> + std::optional lastSaturation_;\n> >> >> >> Interpolator> ccm_;\n> >> >> >> };\n> >> >> >> \n> >> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> >> >> >> index 88cc6c35..56792981 100644\n> >> >> >> --- a/src/ipa/simple/ipa_context.h\n> >> >> >> +++ b/src/ipa/simple/ipa_context.h\n> >> >> >> @@ -63,6 +63,7 @@ struct IPAActiveState {\n> >> >> >> struct {\n> >> >> >> /* 0..2 range, 1.0 = normal */\n> >> >> >> std::optional contrast;\n> >> >> >> + std::optional saturation;\n> >> >> >> } knobs;\n> >> >> >> };\n> >> >> >> \n> >> >> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext {\n> >> >> >> int32_t exposure;\n> >> >> >> double gain;\n> >> >> >> } sensor;\n> >> >> >> +\n> >> >> >> struct {\n> >> >> >> double red;\n> >> >> >> double blue;\n> >> >> >> } gains;\n> >> >> >> +\n> >> >> >> std::optional contrast;\n> >> >> >> + std::optional saturation;\n> >> >> >> };\n> >> >> >> \n> >> >> >> struct IPAContext {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4E503C3200\n\tfor ;\n\tThu, 15 May 2025 22:07:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6E7168B70;\n\tFri, 16 May 2025 00:07:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CF586175B\n\tfor ;\n\tFri, 16 May 2025 00:07:05 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cust-east-par-46-193-72-226.cust.wifirst.net [46.193.72.226])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0EC163D;\n\tFri, 16 May 2025 00:06:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eDSEtD41\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747346807;\n\tbh=YXOO9ejshtQsqULgsZmt0J4aWwdeKihLS89Tq5eb3WA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eDSEtD41Vu33zuzr6SMnCCVrgHwC+O4s1HYtfPpMe8DREMMfMHK0IEFjSY+ixmhPA\n\tf3vA2TBtCG6vXQES9miFjImT4qFqqtbdf8/hrHQN3VFhPS9lNHzGlleDCo4HYaR1ol\n\tJgHYMtvpCkemBd682AX3WU5chr8kpaG4sEkPCLoI=","Date":"Fri, 16 May 2025 00:06:57 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: software_isp: Add saturation control","Message-ID":"<20250515220657.GA11649@pendragon.ideasonboard.com>","References":"<20250407085802.16269-1-mzamazal@redhat.com>\n\t<174448261100.1367185.3335207883574131633@ping.linuxembedded.co.uk>\n\t<85r01unath.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<174463792210.2882969.14630378418171625914@ping.linuxembedded.co.uk>\n\t<85mscin6g9.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<20250414160528.GG26798@pendragon.ideasonboard.com>\n\t<85y0vjc724.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<85y0vjc724.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]