[{"id":39665,"web_url":"https://patchwork.libcamera.org/comment/39665/","msgid":"<85ldbj5h85.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2026-07-10T09:25:30","subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Jacopo,\n\nJacopo Mondi writes:\n\n> From: Kieran Bingham \n>\n> Port the SoftISP Awb algorithm to use the new libipa implementation\n> of AwbAlgorithm.\n>\n> The awbAlgo_ class member is initialized with the Q<4, 8> type even if\n> there is no physical register representation for SoftISP.\n>\n> The usage of libipa::awb::Context, which defines the gains vector as\n> RGB, in the SimpleIPA ActiveState and FrameContext requires\n> changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB\n> and RGB.\n>\n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> Tested-by: Robert Mader \n> ---\n> .../internal/software_isp/debayer_params.h | 4 +-\n> src/ipa/simple/algorithms/awb.cpp | 141 ++++++++++++++++-----\n> src/ipa/simple/algorithms/awb.h | 29 +++++\n> src/ipa/simple/algorithms/blc.cpp | 2 +-\n> src/ipa/simple/algorithms/ccm.cpp | 2 +-\n> src/ipa/simple/ipa_context.h | 12 +-\n> src/libcamera/software_isp/debayer_cpu.cpp | 12 +-\n> 7 files changed, 151 insertions(+), 51 deletions(-)\n>\n> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n> index 6772b43bced4..1074720d73c2 100644\n> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> @@ -21,10 +21,10 @@ struct DebayerParams {\n> \tMatrix combinedMatrix = { { 1.0, 0.0, 0.0,\n> \t\t\t\t\t\t 0.0, 1.0, 0.0,\n> \t\t\t\t\t\t 0.0, 0.0, 1.0 } };\n> -\tRGB blackLevel = RGB({ 0.0, 0.0, 0.0 });\n> +\tRGB blackLevel = RGB({ 0.0, 0.0, 0.0 });\n> \tfloat gamma = 1.0;\n> \tfloat contrastExp = 1.0;\n> -\tRGB gains = RGB({ 1.0, 1.0, 1.0 });\n> +\tRGB gains = RGB({ 1.0, 1.0, 1.0 });\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp\n> index 05155c83d172..64ce4f91b21c 100644\n> --- a/src/ipa/simple/algorithms/awb.cpp\n> +++ b/src/ipa/simple/algorithms/awb.cpp\n> @@ -15,7 +15,6 @@\n> #include \n> \n> #include \"libipa/colours.h\"\n\nThis include can be removed too.\n\n> -#include \"simple/ipa_context.h\"\n> \n> namespace libcamera {\n> \n> @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)\n> \n> namespace ipa::soft::algorithms {\n> \n> +/*\n> + * \\todo Replace it with a proper Lux algorithm\n> + */\n> +static constexpr unsigned int kDefaultLux = 500;\n> +\n> +class SimpleAwbStats final : public AwbStats\n\nThis class is the same across three pipelines -- should it be a part of\nlibipa?\n\n> +{\n> +public:\n> +\tSimpleAwbStats() = default;\n> +\n> +\tSimpleAwbStats(const RGB &rgbMeans)\n> +\t{\n> +\t\trgbMeans_ = rgbMeans;\n> +\n> +\t\trg_ = rgbMeans_.r() / rgbMeans_.g();\n> +\t\tbg_ = rgbMeans_.b() / rgbMeans_.g();\n> +\t}\n> +\n> +\tdouble computeColourError(const RGB &gains) const override\n> +\t{\n> +\t\t/*\n> +\t\t * Compute the sum of the squared colour error (non-greyness) as\n> +\t\t * it appears in the log likelihood equation.\n> +\t\t */\n> +\t\tdouble deltaR = gains.r() * rg_ - 1.0;\n> +\t\tdouble deltaB = gains.b() * bg_ - 1.0;\n> +\t\tdouble delta2 = deltaR * deltaR + deltaB * deltaB;\n> +\n> +\t\treturn delta2;\n> +\t}\n> +\n> +\tRGB rgbMeans() const override\n> +\t{\n> +\t\treturn rgbMeans_;\n> +\t}\n> +\n> +\tbool valid() const override\n> +\t{\n> +\t\t/* Minimum mean value below which AWB can't operate. */\n> +\t\tconstexpr double minValue = 0.2;\n> +\n> +\t\treturn rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||\n> +\t\t rgbMeans_.b() > minValue;\n> +\t}\n> +\n> +private:\n> +\tRGB rgbMeans_;\n> +\tdouble rg_;\n> +\tdouble bg_;\n> +};\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int Awb::init(IPAContext &context, const ValueNode &tuningData)\n> +{\n> +\treturn awbAlgo_.init(tuningData, context.ctrlMap);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::configure\n> + */\n> int Awb::configure(IPAContext &context,\n> \t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> {\n> -\tauto &gains = context.activeState.awb.gains;\n> -\tgains = { { 1.0, 1.0, 1.0 } };\n> +\treturn awbAlgo_.configure(context.activeState.awb);\n> +}\n> \n> -\treturn 0;\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Awb::queueRequest(IPAContext &context,\n> +\t\t const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t const ControlList &controls)\n> +{\n> +\tawbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,\n> +\t\t\t controls);\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> void Awb::prepare(IPAContext &context,\n> \t\t [[maybe_unused]] const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t DebayerParams *params)\n> {\n> -\tauto &gains = context.activeState.awb.gains;\n> +\tawbAlgo_.prepare(context.activeState.awb, frameContext.awb);\n> \n> -\tframeContext.gains = gains;\n> -\tparams->gains = gains;\n> +\tparams->gains = frameContext.awb.gains;\n> }\n> \n> -void Awb::process(IPAContext &context,\n> -\t\t [[maybe_unused]] const uint32_t frame,\n> -\t\t IPAFrameContext &frameContext,\n> -\t\t const SwIspStats *stats,\n> -\t\t ControlList &metadata)\n> +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,\n> +\t\t\t\t const SwIspStats *stats) const\n> {\n> +\tif (!stats->valid)\n> +\t\treturn {};\n> +\n> \tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n> \tconst uint8_t blackLevel = context.activeState.blc.level;\n> \n> -\tmetadata.set(controls::ColourGains, { frameContext.gains.r(),\n> -\t\t\t\t\t frameContext.gains.b() });\n> -\n> -\tif (!stats->valid)\n> -\t\treturn;\n> -\n> \t/*\n> \t * Black level must be subtracted to get the correct AWB ratios, they\n> \t * would be off if they were computed from the whole brightness range\n> @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,\n> \t\thistogram.begin(), histogram.end(), uint64_t(0));\n> \tconst uint64_t offset = blackLevel * nPixels;\n> \tconst uint64_t minValid = 1;\n> +\n> \t/*\n> \t * Make sure the sums are at least minValid, while preventing unsigned\n> \t * integer underflow.\n> \t */\n> \tconst RGB sum = stats->sum_.max(offset + minValid) - offset;\n> \n> +\tRGB rgbMeans = { { static_cast(sum.r() / nPixels),\n> +\t\t\t\t static_cast(sum.g() / nPixels),\n> +\t\t\t\t static_cast(sum.b() / nPixels) } };\n\n\nIt would be a little bit more correct to cast the sums, before applying\nthe division, rather than the integer division result:\n\n static_cast(sum.r()) / nPixels\n\n> +\n> \t/*\n> -\t * Calculate red and blue gains for AWB.\n> -\t * Clamp max gain at 4.0, this also avoids 0 division.\n> +\t * \\todo Determine the minimum allowed thresholds from the mean\n> +\t * but we currently have the sum - not the mean value!\n\nI don't understand the TODO -- what is it talking about and what mean\nand sum are referred here?\n\n> +\t *\n> +\t * Currently set to SimpleAwbStats::minColourValue() = 0.2.\n> \t */\n> -\tauto &gains = context.activeState.awb.gains;\n> -\tgains = { {\n> -\t\tsum.r() <= sum.g() / 4 ? 4.0f : static_cast(sum.g()) / sum.r(),\n> -\t\t1.0,\n> -\t\tsum.b() <= sum.g() / 4 ? 4.0f : static_cast(sum.g()) / sum.b(),\n> -\t} };\n> -\n> -\tRGB rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };\n> -\tcontext.activeState.awb.temperatureK = estimateCCT(rgbGains);\n> -\tmetadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK);\n> -\n> -\tLOG(IPASoftAwb, Debug)\n> -\t\t<< \"gain R/B: \" << gains << \"; temperature: \"\n> -\t\t<< context.activeState.awb.temperatureK;\n> +\treturn SimpleAwbStats(rgbMeans);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::process\n> + */\n> +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext, const SwIspStats *stats,\n> +\t\t ControlList &metadata)\n> +{\n> +\tSimpleAwbStats awbStats = calculateRgbMeans(context, stats);\n> +\n> +\tawbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,\n> +\t\t\t kDefaultLux, metadata);\n> }\n> \n> REGISTER_IPA_ALGORITHM(Awb, \"Awb\")\n> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h\n> index ad993f39c180..fec48c6eea1c 100644\n> --- a/src/ipa/simple/algorithms/awb.h\n> +++ b/src/ipa/simple/algorithms/awb.h\n> @@ -7,19 +7,37 @@\n> \n> #pragma once\n> \n> +#include \n> +\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +#include \"libcamera/internal/value_node.h\"\n> +\n> +#include \"libipa/awb.h\"\n> +#include \"libipa/fixedpoint.h\"\n> +#include \"simple/ipa_context.h\"\n> +\n> #include \"algorithm.h\"\n> \n> namespace libcamera {\n> \n> namespace ipa::soft::algorithms {\n> \n> +class SimpleAwbStats;\n> +\n> class Awb : public Algorithm\n> {\n> public:\n> \tAwb() = default;\n> \t~Awb() = default;\n> \n> +\tint init(IPAContext &context,\n> +\t\t const ValueNode &tuningData) override;\n> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> +\n> +\tvoid queueRequest(IPAContext &context,\n> +\t\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t\t IPAFrameContext &frameContext,\n> +\t\t\t const ControlList &controls) override;\n> \tvoid prepare(IPAContext &context,\n> \t\t const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> @@ -29,6 +47,17 @@ public:\n> \t\t IPAFrameContext &frameContext,\n> \t\t const SwIspStats *stats,\n> \t\t ControlList &metadata) override;\n> +\n> +private:\n> +\tSimpleAwbStats calculateRgbMeans(IPAContext &context,\n> +\t\t\t\t\t const SwIspStats *stats) const;\n> +\n> +\t/*\n> +\t * There actually is no Q register format for SoftISP, but allow the\n> +\t * colour gains to range in the [0.0f, 3.999f] interval, which seems\n> +\t * reasonable.\n> +\t */\n> +\tAwbAlgorithm> awbAlgo_;\n> };\n> \n> } /* namespace ipa::soft::algorithms */\n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index 677be56ed669..e45a913cd970 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -53,7 +53,7 @@ void BlackLevel::prepare(IPAContext &context,\n> \t\t\t DebayerParams *params)\n> {\n> \t/* Latch the blacklevel gain so GPUISP can apply. */\n> -\tparams->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n> +\tparams->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n> }\n> \n> void BlackLevel::process(IPAContext &context,\n> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> index ace9c35dc462..1174784edc7e 100644\n> --- a/src/ipa/simple/algorithms/ccm.cpp\n> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)\n> void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> {\n> -\tconst unsigned int ct = context.activeState.awb.temperatureK;\n> +\tconst unsigned int ct = frameContext.awb.colourTemperature;\n> \n> \t/* Change CCM only on bigger temperature changes. */\n> \tif (!currentCcm_ ||\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index 8ccfacb46a59..29643a655ce1 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -16,6 +16,7 @@\n> #include \"libcamera/internal/matrix.h\"\n> #include \"libcamera/internal/vector.h\"\n> \n> +#include \n> #include \n> \n> #include \"core_ipa_interface.h\"\n> @@ -36,6 +37,8 @@ struct IPASessionConfiguration {\n> };\n> \n> struct IPAActiveState {\n> +\tipa::awb::ActiveState awb;\n> +\n> \tstruct {\n> \t\tint32_t exposure;\n> \t\tdouble again;\n> @@ -48,11 +51,6 @@ struct IPAActiveState {\n> \t\tdouble lastGain;\n> \t} blc;\n> \n> -\tstruct {\n> -\t\tRGB gains;\n> -\t\tunsigned int temperatureK;\n> -\t} awb;\n> -\n> \tMatrix combinedMatrix;\n> \n> \tstruct {\n> @@ -64,6 +62,8 @@ struct IPAActiveState {\n> };\n> \n> struct IPAFrameContext : public FrameContext {\n> +\tipa::awb::FrameContext awb;\n> +\n> \tMatrix ccm;\n> \n> \tstruct {\n> @@ -71,8 +71,6 @@ struct IPAFrameContext : public FrameContext {\n> \t\tdouble gain;\n> \t} sensor;\n> \n> -\tRGB gains;\n> -\n> \tfloat gamma;\n> \tstd::optional contrast;\n> \tstd::optional saturation;\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 49382b4c2719..ab22635fdfaf 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -1010,9 +1010,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> \t};\n> \tconst unsigned int gammaTableSize = gammaTable_.size();\n> \n> -\tconst RGB blackIndex = params.blackLevel * kRGBLookupSize;\n> -\tconst RGB gains = params.gains;\n> -\tconst RGB div = (RGB(kRGBLookupSize) - blackIndex).max(1.0);\n> +\tconst RGB blackIndex = params.blackLevel * kRGBLookupSize;\n> +\tconst RGB gains = params.gains;\n> +\tconst RGB div = (RGB(kRGBLookupSize) - blackIndex).max(1.0);\n> \n> \tif (ccmEnabled_) {\n> \t\tif (gammaUpdateNeeded ||\n> @@ -1025,7 +1025,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> \t\t\tconst unsigned int greenIndex = 1;\n> \t\t\tconst unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;\n> \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> -\t\t\t\tconst RGB rgb = (gains * (RGB(i) - blackIndex) * kRGBLookupSize / div)\n> +\t\t\t\tconst RGB rgb = (gains * (RGB(i) - blackIndex) * kRGBLookupSize / div)\n> \t\t\t\t\t\t\t .clamp(0.0, kRGBLookupSize - 1);\n> \t\t\t\tred[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);\n> \t\t\t\tred[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);\n> @@ -1045,8 +1045,8 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> \t\t\tauto &green = green_;\n> \t\t\tauto &blue = swapRedBlueGains_ ? red_ : blue_;\n> \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> -\t\t\t\tconst RGB lutGains =\n> -\t\t\t\t\t(gains * (RGB(i) - blackIndex) * gammaTableSize / div)\n> +\t\t\t\tconst RGB lutGains =\n> +\t\t\t\t\t(gains * (RGB(i) - blackIndex) * gammaTableSize / div)\n> \t\t\t\t\t\t.clamp(0.0, gammaTableSize - 1);\n> \t\t\t\tred[i] = gammaTable_[lutGains.r()];\n> \t\t\t\tgreen[i] = gammaTable_[lutGains.g()];","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 3B92BC331D\n\tfor ;\n\tFri, 10 Jul 2026 09:25:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2229B66104;\n\tFri, 10 Jul 2026 11:25:39 +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 A0E6165F7C\n\tfor ;\n\tFri, 10 Jul 2026 11:25:37 +0200 (CEST)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-57-5sfM6aDQOTqT96WTul5aYQ-1; Fri, 10 Jul 2026 05:25:34 -0400","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-475b0b4e8d5so707083f8f.1\n\tfor ;\n\tFri, 10 Jul 2026 02:25:34 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-4.net.vodafone.cz. [77.48.47.4])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-47a9de1e736sm62459698f8f.7.2026.07.10.02.25.31\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 10 Jul 2026 02:25:31 -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=\"QPfhkLIq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1783675536;\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=2iXWuJHuNvnjsFE5Zgtkd04vw5GwUmLUPx2pTHTVF+c=;\n\tb=QPfhkLIqNRv0XGOKVmqG+ee5Pre8a8F14sCpsETcV5aV711VBbyLqk+lnmBALdknT/Dxub\n\tF3vdpF5+CBb5MFCOlIRkUjqsAHwKFpdfFPv1Oy8gZgsU2nQNfV+x9e+bDHg6FtmWOrHu4c\n\tWPcVRz/uZtTJPNAQzYDRny7EsFw5Ljo=","X-MC-Unique":"5sfM6aDQOTqT96WTul5aYQ-1","X-Mimecast-MFC-AGG-ID":"5sfM6aDQOTqT96WTul5aYQ_1783675533","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20251104; t=1783675533; x=1784280333;\n\th=content-type:mime-version:user-agent:message-id:date:references\n\t:in-reply-to:subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to\n\t:cc:subject:date:message-id:reply-to:content-type;\n\tbh=2iXWuJHuNvnjsFE5Zgtkd04vw5GwUmLUPx2pTHTVF+c=;\n\tb=HIOm/XJQkgo0crns46muOq0j1HEKzdxnBdXWwNWqoLRom2UKG78bibeKvpVpvqVRjV\n\tusT8tc5z9CsMzrvo5i5OKlOwwad0Y2J2TTK5BmHJBnKxaeBwGGBTAmqqWyuYtqAAa63W\n\tW/fvW/gjqnEKrFy++UxvOPwt+iSnBJfn53a3AmwAZPozt1Ef9urn6zu9zhyzLg0b2dUD\n\t4qQBvtAD0TUMmjVWj/3ne0xJ/ihGXfkyIfwmbEB8KAWu0IHSWEvQMLa8XeS45iv/Ii/O\n\tsLMNE3zFjpmpOVh5Ld+BYBFuRr1F7KfJNVTZ/Mso3eKguQr+Z0r0kyMRL0Sii1MhNqM1\n\tWR4w==","X-Gm-Message-State":"AOJu0Yz9ShPa3+osYSJmaoQSjki/EGwTMc/mjZYdGX3abA/Ddn8E2bsi\n\t34Ch42oupU+1UqQkow5wnlgoyqri6aykdIO3zN5/1fURw4zDWVfZuPTOq8cjsp0APIID7XizYId\n\tY7xCKTuva+u44qGVtncrojZvKSvkQNxPkvsO72j2S7p6fkXDL4uKWP/9Ze8Fl1lGR5IlYvgnNbZ\n\ts=","X-Gm-Gg":"AfdE7ck+sxHq3kmqz7nCyCEioE5e981sMq5EGt0Gxlh9s2hLDWMDxyHHYJqQsIQ2PIz\n\tyNutiGC2gtIQa42H+vWDCoDp3BkD8g07jZNeyuREsOTEmfpsBh0K/da5GixyaTp73wpFBkGQ+Vi\n\tUNDkoiOW157eeMMlcq60bJY2EPDM8Ln1xL3XCTToLT50Ly4JBWp2he1HB+m8LSmrN7Jlw50/aJn\n\tKMN25NSAmXziJnMMHcU/bRC+Y+tF2PXKEFEi+Ctb9MoVTg0DYaeUxo0V6d8/2Wc9031CweoRMHe\n\tcs9nXFUGGuAqLIulIUsUYPaK4PRsn4mZucaFe6P5e6JL91DiM/RtStuAJRY9KVRAO3Cm5SkNgWP\n\t9yQLE80DjNb8yapcQ5zKgpIt32XCtTh4AGtNj0OPlhP2iN0/PuawyH++8/IWFGP9r","X-Received":["by 2002:a05:6000:2209:b0:478:19e7:22a2 with SMTP id\n\tffacd0b85a97d-47df07307c6mr11798870f8f.25.1783675533077; \n\tFri, 10 Jul 2026 02:25:33 -0700 (PDT)","by 2002:a05:6000:2209:b0:478:19e7:22a2 with SMTP id\n\tffacd0b85a97d-47df07307c6mr11798825f8f.25.1783675532537; \n\tFri, 10 Jul 2026 02:25:32 -0700 (PDT)"],"From":"Milan Zamazal ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Stefan Klug\n\t, Kieran Bingham\n\t, Robert Mader\n\t","Subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","In-Reply-To":"<20260708-libipa-algorithms-v5-3-0759d0359f52@ideasonboard.com>\n\t(Jacopo Mondi's message of \"Wed, 08 Jul 2026 17:50:45 +0200\")","References":"<20260708-libipa-algorithms-v5-0-0759d0359f52@ideasonboard.com>\n\t<20260708-libipa-algorithms-v5-3-0759d0359f52@ideasonboard.com>","Date":"Fri, 10 Jul 2026 11:25:30 +0200","Message-ID":"<85ldbj5h85.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":"t44zBG2tJPDJ-Zpg1V5Se-Nm_lstdLcQenkILzvyoSM_1783675533","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":39666,"web_url":"https://patchwork.libcamera.org/comment/39666/","msgid":"<85echb5gnn.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2026-07-10T09:37:48","subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Sorry, I've forgotten about awb.h...\n\nMilan Zamazal writes:\n\n> Hi Jacopo,\n>\n> Jacopo Mondi writes:\n>\n>> From: Kieran Bingham \n>>\n>> Port the SoftISP Awb algorithm to use the new libipa implementation\n>> of AwbAlgorithm.\n>>\n>> The awbAlgo_ class member is initialized with the Q<4, 8> type even if\n\nIt's actually UQ<2, 8>.\n\n>> there is no physical register representation for SoftISP.\n>>\n>> The usage of libipa::awb::Context, which defines the gains vector as\n>> RGB, in the SimpleIPA ActiveState and FrameContext requires\n>> changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB\n>> and RGB.\n>>\n>> Signed-off-by: Kieran Bingham \n>> Signed-off-by: Jacopo Mondi \n>> Reviewed-by: Kieran Bingham \n>> Tested-by: Robert Mader \n>> ---\n>> .../internal/software_isp/debayer_params.h | 4 +-\n>> src/ipa/simple/algorithms/awb.cpp | 141 ++++++++++++++++-----\n>> src/ipa/simple/algorithms/awb.h | 29 +++++\n>> src/ipa/simple/algorithms/blc.cpp | 2 +-\n>> src/ipa/simple/algorithms/ccm.cpp | 2 +-\n>> src/ipa/simple/ipa_context.h | 12 +-\n>> src/libcamera/software_isp/debayer_cpu.cpp | 12 +-\n>> 7 files changed, 151 insertions(+), 51 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n>> index 6772b43bced4..1074720d73c2 100644\n>> --- a/include/libcamera/internal/software_isp/debayer_params.h\n>> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n>> @@ -21,10 +21,10 @@ struct DebayerParams {\n>> \tMatrix combinedMatrix = { { 1.0, 0.0, 0.0,\n>> \t\t\t\t\t\t 0.0, 1.0, 0.0,\n>> \t\t\t\t\t\t 0.0, 0.0, 1.0 } };\n>> -\tRGB blackLevel = RGB({ 0.0, 0.0, 0.0 });\n>> +\tRGB blackLevel = RGB({ 0.0, 0.0, 0.0 });\n>> \tfloat gamma = 1.0;\n>> \tfloat contrastExp = 1.0;\n>> -\tRGB gains = RGB({ 1.0, 1.0, 1.0 });\n>> +\tRGB gains = RGB({ 1.0, 1.0, 1.0 });\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp\n>> index 05155c83d172..64ce4f91b21c 100644\n>> --- a/src/ipa/simple/algorithms/awb.cpp\n>> +++ b/src/ipa/simple/algorithms/awb.cpp\n>> @@ -15,7 +15,6 @@\n>> #include \n>> \n>> #include \"libipa/colours.h\"\n>\n> This include can be removed too.\n>\n>> -#include \"simple/ipa_context.h\"\n>> \n>> namespace libcamera {\n>> \n>> @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)\n>> \n>> namespace ipa::soft::algorithms {\n>> \n>> +/*\n>> + * \\todo Replace it with a proper Lux algorithm\n>> + */\n>> +static constexpr unsigned int kDefaultLux = 500;\n>> +\n>> +class SimpleAwbStats final : public AwbStats\n>\n> This class is the same across three pipelines -- should it be a part of\n> libipa?\n>\n>> +{\n>> +public:\n>> +\tSimpleAwbStats() = default;\n>> +\n>> +\tSimpleAwbStats(const RGB &rgbMeans)\n>> +\t{\n>> +\t\trgbMeans_ = rgbMeans;\n>> +\n>> +\t\trg_ = rgbMeans_.r() / rgbMeans_.g();\n>> +\t\tbg_ = rgbMeans_.b() / rgbMeans_.g();\n>> +\t}\n>> +\n>> +\tdouble computeColourError(const RGB &gains) const override\n>> +\t{\n>> +\t\t/*\n>> +\t\t * Compute the sum of the squared colour error (non-greyness) as\n>> +\t\t * it appears in the log likelihood equation.\n>> +\t\t */\n>> +\t\tdouble deltaR = gains.r() * rg_ - 1.0;\n>> +\t\tdouble deltaB = gains.b() * bg_ - 1.0;\n>> +\t\tdouble delta2 = deltaR * deltaR + deltaB * deltaB;\n>> +\n>> +\t\treturn delta2;\n>> +\t}\n>> +\n>> +\tRGB rgbMeans() const override\n>> +\t{\n>> +\t\treturn rgbMeans_;\n>> +\t}\n>> +\n>> +\tbool valid() const override\n>> +\t{\n>> +\t\t/* Minimum mean value below which AWB can't operate. */\n>> +\t\tconstexpr double minValue = 0.2;\n>> +\n>> +\t\treturn rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||\n>> +\t\t rgbMeans_.b() > minValue;\n>> +\t}\n>> +\n>> +private:\n>> +\tRGB rgbMeans_;\n>> +\tdouble rg_;\n>> +\tdouble bg_;\n>> +};\n>> +\n>> +/**\n>> + * \\copydoc libcamera::ipa::Algorithm::init\n>> + */\n>> +int Awb::init(IPAContext &context, const ValueNode &tuningData)\n>> +{\n>> +\treturn awbAlgo_.init(tuningData, context.ctrlMap);\n>> +}\n>> +\n>> +/**\n>> + * \\copydoc libcamera::ipa::Algorithm::configure\n>> + */\n>> int Awb::configure(IPAContext &context,\n>> \t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> {\n>> -\tauto &gains = context.activeState.awb.gains;\n>> -\tgains = { { 1.0, 1.0, 1.0 } };\n>> +\treturn awbAlgo_.configure(context.activeState.awb);\n>> +}\n>> \n>> -\treturn 0;\n>> +/**\n>> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n>> + */\n>> +void Awb::queueRequest(IPAContext &context,\n>> +\t\t const uint32_t frame,\n>> +\t\t IPAFrameContext &frameContext,\n>> +\t\t const ControlList &controls)\n>> +{\n>> +\tawbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,\n>> +\t\t\t controls);\n>> }\n>> \n>> +/**\n>> + * \\copydoc libcamera::ipa::Algorithm::prepare\n>> + */\n>> void Awb::prepare(IPAContext &context,\n>> \t\t [[maybe_unused]] const uint32_t frame,\n>> \t\t IPAFrameContext &frameContext,\n>> \t\t DebayerParams *params)\n>> {\n>> -\tauto &gains = context.activeState.awb.gains;\n>> +\tawbAlgo_.prepare(context.activeState.awb, frameContext.awb);\n>> \n>> -\tframeContext.gains = gains;\n>> -\tparams->gains = gains;\n>> +\tparams->gains = frameContext.awb.gains;\n>> }\n>> \n>> -void Awb::process(IPAContext &context,\n>> -\t\t [[maybe_unused]] const uint32_t frame,\n>> -\t\t IPAFrameContext &frameContext,\n>> -\t\t const SwIspStats *stats,\n>> -\t\t ControlList &metadata)\n>> +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,\n>> +\t\t\t\t const SwIspStats *stats) const\n>> {\n>> +\tif (!stats->valid)\n>> +\t\treturn {};\n>> +\n>> \tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n>> \tconst uint8_t blackLevel = context.activeState.blc.level;\n>> \n>> -\tmetadata.set(controls::ColourGains, { frameContext.gains.r(),\n>> -\t\t\t\t\t frameContext.gains.b() });\n>> -\n>> -\tif (!stats->valid)\n>> -\t\treturn;\n>> -\n>> \t/*\n>> \t * Black level must be subtracted to get the correct AWB ratios, they\n>> \t * would be off if they were computed from the whole brightness range\n>> @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,\n>> \t\thistogram.begin(), histogram.end(), uint64_t(0));\n>> \tconst uint64_t offset = blackLevel * nPixels;\n>> \tconst uint64_t minValid = 1;\n>> +\n>> \t/*\n>> \t * Make sure the sums are at least minValid, while preventing unsigned\n>> \t * integer underflow.\n>> \t */\n>> \tconst RGB sum = stats->sum_.max(offset + minValid) - offset;\n>> \n>> +\tRGB rgbMeans = { { static_cast(sum.r() / nPixels),\n>> +\t\t\t\t static_cast(sum.g() / nPixels),\n>> +\t\t\t\t static_cast(sum.b() / nPixels) } };\n>\n>\n> It would be a little bit more correct to cast the sums, before applying\n> the division, rather than the integer division result:\n>\n> static_cast(sum.r()) / nPixels\n>\n>> +\n>> \t/*\n>> -\t * Calculate red and blue gains for AWB.\n>> -\t * Clamp max gain at 4.0, this also avoids 0 division.\n>> +\t * \\todo Determine the minimum allowed thresholds from the mean\n>> +\t * but we currently have the sum - not the mean value!\n>\n> I don't understand the TODO -- what is it talking about and what mean\n> and sum are referred here?\n>\n>> +\t *\n>> +\t * Currently set to SimpleAwbStats::minColourValue() = 0.2.\n>> \t */\n>> -\tauto &gains = context.activeState.awb.gains;\n>> -\tgains = { {\n>> -\t\tsum.r() <= sum.g() / 4 ? 4.0f : static_cast(sum.g()) / sum.r(),\n>> -\t\t1.0,\n>> -\t\tsum.b() <= sum.g() / 4 ? 4.0f : static_cast(sum.g()) / sum.b(),\n>> -\t} };\n>> -\n>> -\tRGB rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };\n>> -\tcontext.activeState.awb.temperatureK = estimateCCT(rgbGains);\n>> -\tmetadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK);\n>> -\n>> -\tLOG(IPASoftAwb, Debug)\n>> -\t\t<< \"gain R/B: \" << gains << \"; temperature: \"\n>> -\t\t<< context.activeState.awb.temperatureK;\n>> +\treturn SimpleAwbStats(rgbMeans);\n>> +}\n>> +\n>> +/**\n>> + * \\copydoc libcamera::ipa::Algorithm::process\n>> + */\n>> +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> +\t\t IPAFrameContext &frameContext, const SwIspStats *stats,\n>> +\t\t ControlList &metadata)\n>> +{\n>> +\tSimpleAwbStats awbStats = calculateRgbMeans(context, stats);\n>> +\n>> +\tawbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,\n>> +\t\t\t kDefaultLux, metadata);\n>> }\n>> \n>> REGISTER_IPA_ALGORITHM(Awb, \"Awb\")\n>> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h\n>> index ad993f39c180..fec48c6eea1c 100644\n>> --- a/src/ipa/simple/algorithms/awb.h\n>> +++ b/src/ipa/simple/algorithms/awb.h\n>> @@ -7,19 +7,37 @@\n>> \n>> #pragma once\n>> \n>> +#include \n>> +\n>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> +#include \"libcamera/internal/value_node.h\"\n>> +\n>> +#include \"libipa/awb.h\"\n>> +#include \"libipa/fixedpoint.h\"\n>> +#include \"simple/ipa_context.h\"\n>> +\n>> #include \"algorithm.h\"\n>> \n>> namespace libcamera {\n>> \n>> namespace ipa::soft::algorithms {\n>> \n>> +class SimpleAwbStats;\n>> +\n>> class Awb : public Algorithm\n>> {\n>> public:\n>> \tAwb() = default;\n>> \t~Awb() = default;\n>> \n>> +\tint init(IPAContext &context,\n>> +\t\t const ValueNode &tuningData) override;\n>> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> +\n>> +\tvoid queueRequest(IPAContext &context,\n>> +\t\t\t [[maybe_unused]] const uint32_t frame,\n\nNo need to use [[maybe_unused]] in a header file.\n\n>> +\t\t\t IPAFrameContext &frameContext,\n>> +\t\t\t const ControlList &controls) override;\n>> \tvoid prepare(IPAContext &context,\n>> \t\t const uint32_t frame,\n>> \t\t IPAFrameContext &frameContext,\n>> @@ -29,6 +47,17 @@ public:\n>> \t\t IPAFrameContext &frameContext,\n>> \t\t const SwIspStats *stats,\n>> \t\t ControlList &metadata) override;\n>> +\n>> +private:\n>> +\tSimpleAwbStats calculateRgbMeans(IPAContext &context,\n>> +\t\t\t\t\t const SwIspStats *stats) const;\n>> +\n>> +\t/*\n>> +\t * There actually is no Q register format for SoftISP, but allow the\n>> +\t * colour gains to range in the [0.0f, 3.999f] interval, which seems\n>> +\t * reasonable.\n>> +\t */\n>> +\tAwbAlgorithm> awbAlgo_;\n>> };\n>> \n>> } /* namespace ipa::soft::algorithms */\n>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>> index 677be56ed669..e45a913cd970 100644\n>> --- a/src/ipa/simple/algorithms/blc.cpp\n>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> @@ -53,7 +53,7 @@ void BlackLevel::prepare(IPAContext &context,\n>> \t\t\t DebayerParams *params)\n>> {\n>> \t/* Latch the blacklevel gain so GPUISP can apply. */\n>> -\tparams->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n>> +\tparams->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n>> }\n>> \n>> void BlackLevel::process(IPAContext &context,\n>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> index ace9c35dc462..1174784edc7e 100644\n>> --- a/src/ipa/simple/algorithms/ccm.cpp\n>> +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)\n>> void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> \t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> {\n>> -\tconst unsigned int ct = context.activeState.awb.temperatureK;\n>> +\tconst unsigned int ct = frameContext.awb.colourTemperature;\n>> \n>> \t/* Change CCM only on bigger temperature changes. */\n>> \tif (!currentCcm_ ||\n>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> index 8ccfacb46a59..29643a655ce1 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -16,6 +16,7 @@\n>> #include \"libcamera/internal/matrix.h\"\n>> #include \"libcamera/internal/vector.h\"\n>> \n>> +#include \n>> #include \n>> \n>> #include \"core_ipa_interface.h\"\n>> @@ -36,6 +37,8 @@ struct IPASessionConfiguration {\n>> };\n>> \n>> struct IPAActiveState {\n>> +\tipa::awb::ActiveState awb;\n>> +\n>> \tstruct {\n>> \t\tint32_t exposure;\n>> \t\tdouble again;\n>> @@ -48,11 +51,6 @@ struct IPAActiveState {\n>> \t\tdouble lastGain;\n>> \t} blc;\n>> \n>> -\tstruct {\n>> -\t\tRGB gains;\n>> -\t\tunsigned int temperatureK;\n>> -\t} awb;\n>> -\n>> \tMatrix combinedMatrix;\n>> \n>> \tstruct {\n>> @@ -64,6 +62,8 @@ struct IPAActiveState {\n>> };\n>> \n>> struct IPAFrameContext : public FrameContext {\n>> +\tipa::awb::FrameContext awb;\n>> +\n>> \tMatrix ccm;\n>> \n>> \tstruct {\n>> @@ -71,8 +71,6 @@ struct IPAFrameContext : public FrameContext {\n>> \t\tdouble gain;\n>> \t} sensor;\n>> \n>> -\tRGB gains;\n>> -\n>> \tfloat gamma;\n>> \tstd::optional contrast;\n>> \tstd::optional saturation;\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 49382b4c2719..ab22635fdfaf 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -1010,9 +1010,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n>> \t};\n>> \tconst unsigned int gammaTableSize = gammaTable_.size();\n>> \n>> -\tconst RGB blackIndex = params.blackLevel * kRGBLookupSize;\n>> -\tconst RGB gains = params.gains;\n>> -\tconst RGB div = (RGB(kRGBLookupSize) - blackIndex).max(1.0);\n>> +\tconst RGB blackIndex = params.blackLevel * kRGBLookupSize;\n>> +\tconst RGB gains = params.gains;\n>> +\tconst RGB div = (RGB(kRGBLookupSize) - blackIndex).max(1.0);\n>> \n>> \tif (ccmEnabled_) {\n>> \t\tif (gammaUpdateNeeded ||\n>> @@ -1025,7 +1025,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n>> \t\t\tconst unsigned int greenIndex = 1;\n>> \t\t\tconst unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;\n>> \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n>> -\t\t\t\tconst RGB rgb = (gains * (RGB(i) - blackIndex) * kRGBLookupSize / div)\n>> +\t\t\t\tconst RGB rgb = (gains * (RGB(i) - blackIndex) * kRGBLookupSize / div)\n>> \t\t\t\t\t\t\t .clamp(0.0, kRGBLookupSize - 1);\n>> \t\t\t\tred[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);\n>> \t\t\t\tred[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);\n>> @@ -1045,8 +1045,8 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n>> \t\t\tauto &green = green_;\n>> \t\t\tauto &blue = swapRedBlueGains_ ? red_ : blue_;\n>> \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n>> -\t\t\t\tconst RGB lutGains =\n>> -\t\t\t\t\t(gains * (RGB(i) - blackIndex) * gammaTableSize / div)\n>> +\t\t\t\tconst RGB lutGains =\n>> +\t\t\t\t\t(gains * (RGB(i) - blackIndex) * gammaTableSize / div)\n>> \t\t\t\t\t\t.clamp(0.0, gammaTableSize - 1);\n>> \t\t\t\tred[i] = gammaTable_[lutGains.r()];\n>> \t\t\t\tgreen[i] = gammaTable_[lutGains.g()];","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 5C8A6C3330\n\tfor ;\n\tFri, 10 Jul 2026 09:37:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96B7166104;\n\tFri, 10 Jul 2026 11:37:56 +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 699E965F7C\n\tfor ;\n\tFri, 10 Jul 2026 11:37:54 +0200 (CEST)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-623-cSCXdSfbP-GnA9HN3VPOHA-1; Fri, 10 Jul 2026 05:37:51 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-4926fa2cb17so6522585e9.1\n\tfor ;\n\tFri, 10 Jul 2026 02:37:51 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-4.net.vodafone.cz. [77.48.47.4])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-493f2e0eff1sm29596775e9.0.2026.07.10.02.37.48\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 10 Jul 2026 02:37:49 -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=\"ZzJ7Kt1B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1783676273;\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=Ss6Fe23PD8Kfbm9rI2h+RKTNYFo+ur+wL9HdwLTwgR8=;\n\tb=ZzJ7Kt1BMeAq0gjzfClQxYTY4CxeqQJCR5Gs+Mwscem3wY8B6gE5BN2YN0NHalRpXDkKSz\n\t0EsZ1AsNCqhJQaOJMbdNusbd5wFwb1SGqNFYeJfbEG318wlMWhor7rjnx6eTfP9yc0KJEy\n\tcmPtuTYIH6j5np922MqSK8DgdoWc+jM=","X-MC-Unique":"cSCXdSfbP-GnA9HN3VPOHA-1","X-Mimecast-MFC-AGG-ID":"cSCXdSfbP-GnA9HN3VPOHA_1783676270","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20251104; t=1783676270; x=1784281070;\n\th=content-type:mime-version:user-agent:message-id:date:references\n\t:in-reply-to:subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to\n\t:cc:subject:date:message-id:reply-to:content-type;\n\tbh=Ss6Fe23PD8Kfbm9rI2h+RKTNYFo+ur+wL9HdwLTwgR8=;\n\tb=bhKzTt6s5JJSKqTNjdfPsqoLaWvj7uXysAe5WJSI62eI9/wqWW5s358x/7VkItGQz9\n\twEOFFs5QAIF2cx6plBidER7PegoLgjhPpKS5K+CaT23HSBOIRZBeYAmDOCywS5ssA0WY\n\tWjTz9vVOGFe8EMbT1UXXBBB3W8HH9XCqquyw07nOCIs2+9tFbIDtYGQbUgCe6SCCStyb\n\thnw0ZqdN6hY8txDUenk9GF7Uqqy6P5pR4jqoiVJGGEJWxPS/b98xwoZXhmvAP1gcq3q8\n\tWW8lDylFEn0OXnR0jmK4/TpU1ttFxMiN4Qb9LZ2+p/bfLHLFFw6btnUl31mEa0fLYh5Q\n\tUYIQ==","X-Gm-Message-State":"AOJu0Ywq8PezmB/3ZpmmD+TmoAYp3YtaTt5c0Nts2+u72OfahR1N9f8x\n\tRXlMN6SmbWXlS68U0j4N4EmrQpyKAtvR+p+gnFgJKluLqQBv7oiEdKLSeToWiA4YlT2sPTKQ1iz\n\tMgcjtNWNd3k8xKA5wewZWrYTh2BOAkMT3NvF7yBajdHdY1/9yRhdXuk98TWH9AP1KEvp7haonDz\n\tw=","X-Gm-Gg":"AfdE7cmrr+aOrjdJYsCw7uAI8YrUhEXQ31IX+7hiRwulZlpi4T4fC3zfquWXLM8L76+\n\tsFchFg5qdj3vMe00qXU3k3dpdc66dBQ5hKm1Xmx3iXcPigknEFOpBGOwiy47EgwqfPgOvxFGLhJ\n\tpiyC8dTqZKJK4P8upkAm2/qIhfNQUnt7QH/5oOIj23OmXXRSdAMfjlRiCMqhchOHmtFCXImaL2W\n\t/WTGDtfVJhmJxk6So+8zQUJwtahc7WlWgtQJDtn0QovfvW3ygyAlgPTUW7eDCVq26gHPg7IhtUz\n\tWf35YnJpPbJleosEx7c6vvjnO42MNn3XdRh4t+nJstqwTJrZ8rceWSizyuu4tRSP6TcI+uq0gk1\n\td+5phaS6IuC8o2mSaV28Cad5XHgrgjwo/dEjIbgM6mz9CriifDJN6bzwUi7r79ftJ","X-Received":["by 2002:a05:600c:1908:b0:493:c566:7bd6 with SMTP id\n\t5b1f17b1804b1-493f2b4baa1mr24586825e9.18.1783676270285; \n\tFri, 10 Jul 2026 02:37:50 -0700 (PDT)","by 2002:a05:600c:1908:b0:493:c566:7bd6 with SMTP id\n\t5b1f17b1804b1-493f2b4baa1mr24586565e9.18.1783676269773; \n\tFri, 10 Jul 2026 02:37:49 -0700 (PDT)"],"From":"Milan Zamazal ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Stefan Klug\n\t, Kieran Bingham\n\t, Robert Mader\n\t","Subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","In-Reply-To":"<85ldbj5h85.fsf@mzamazal-thinkpadp1gen7.tpbc.csb> (Milan\n\tZamazal's message of \"Fri, 10 Jul 2026 11:25:30 +0200\")","References":"<20260708-libipa-algorithms-v5-0-0759d0359f52@ideasonboard.com>\n\t<20260708-libipa-algorithms-v5-3-0759d0359f52@ideasonboard.com>\n\t<85ldbj5h85.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Date":"Fri, 10 Jul 2026 11:37:48 +0200","Message-ID":"<85echb5gnn.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":"mlXHldvpQeP00BKdIQ7a0p1pkyyc8FmninCYixTNFR8_1783676270","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":39668,"web_url":"https://patchwork.libcamera.org/comment/39668/","msgid":"","date":"2026-07-10T12:32:53","subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Milan\n\nOn Fri, Jul 10, 2026 at 11:25:30AM +0200, Milan Zamazal wrote:\n> Hi Jacopo,\n>\n> Jacopo Mondi writes:\n>\n> > From: Kieran Bingham \n> >\n> > Port the SoftISP Awb algorithm to use the new libipa implementation\n> > of AwbAlgorithm.\n> >\n> > The awbAlgo_ class member is initialized with the Q<4, 8> type even if\n> > there is no physical register representation for SoftISP.\n> >\n> > The usage of libipa::awb::Context, which defines the gains vector as\n> > RGB, in the SimpleIPA ActiveState and FrameContext requires\n> > changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB\n> > and RGB.\n> >\n> > Signed-off-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Kieran Bingham \n> > Tested-by: Robert Mader \n> > ---\n> > .../internal/software_isp/debayer_params.h | 4 +-\n> > src/ipa/simple/algorithms/awb.cpp | 141 ++++++++++++++++-----\n> > src/ipa/simple/algorithms/awb.h | 29 +++++\n> > src/ipa/simple/algorithms/blc.cpp | 2 +-\n> > src/ipa/simple/algorithms/ccm.cpp | 2 +-\n> > src/ipa/simple/ipa_context.h | 12 +-\n> > src/libcamera/software_isp/debayer_cpu.cpp | 12 +-\n> > 7 files changed, 151 insertions(+), 51 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n> > index 6772b43bced4..1074720d73c2 100644\n> > --- a/include/libcamera/internal/software_isp/debayer_params.h\n> > +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> > @@ -21,10 +21,10 @@ struct DebayerParams {\n> > \tMatrix combinedMatrix = { { 1.0, 0.0, 0.0,\n> > \t\t\t\t\t\t 0.0, 1.0, 0.0,\n> > \t\t\t\t\t\t 0.0, 0.0, 1.0 } };\n> > -\tRGB blackLevel = RGB({ 0.0, 0.0, 0.0 });\n> > +\tRGB blackLevel = RGB({ 0.0, 0.0, 0.0 });\n> > \tfloat gamma = 1.0;\n> > \tfloat contrastExp = 1.0;\n> > -\tRGB gains = RGB({ 1.0, 1.0, 1.0 });\n> > +\tRGB gains = RGB({ 1.0, 1.0, 1.0 });\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp\n> > index 05155c83d172..64ce4f91b21c 100644\n> > --- a/src/ipa/simple/algorithms/awb.cpp\n> > +++ b/src/ipa/simple/algorithms/awb.cpp\n> > @@ -15,7 +15,6 @@\n> > #include \n> >\n> > #include \"libipa/colours.h\"\n>\n> This include can be removed too.\n>\n\nack!\n\n> > -#include \"simple/ipa_context.h\"\n> >\n> > namespace libcamera {\n> >\n> > @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)\n> >\n> > namespace ipa::soft::algorithms {\n> >\n> > +/*\n> > + * \\todo Replace it with a proper Lux algorithm\n> > + */\n> > +static constexpr unsigned int kDefaultLux = 500;\n> > +\n> > +class SimpleAwbStats final : public AwbStats\n>\n> This class is the same across three pipelines -- should it be a part of\n> libipa?\n>\n\nEh, that's what I had initially, but I've been asked to move the\nimplementation to each single IPA. I understand why, right now we just\ncopied around the RkISP1 implementation instead of having each\nplatform calculate statistics in their own way and moving the RkISP1\nimplementation to the base class would have communicated that this is\nwhat we expect all platforms to do, while in reality, we just copied\ncode around.\n\n> > +{\n> > +public:\n> > +\tSimpleAwbStats() = default;\n> > +\n> > +\tSimpleAwbStats(const RGB &rgbMeans)\n> > +\t{\n> > +\t\trgbMeans_ = rgbMeans;\n> > +\n> > +\t\trg_ = rgbMeans_.r() / rgbMeans_.g();\n> > +\t\tbg_ = rgbMeans_.b() / rgbMeans_.g();\n> > +\t}\n> > +\n> > +\tdouble computeColourError(const RGB &gains) const override\n> > +\t{\n> > +\t\t/*\n> > +\t\t * Compute the sum of the squared colour error (non-greyness) as\n> > +\t\t * it appears in the log likelihood equation.\n> > +\t\t */\n> > +\t\tdouble deltaR = gains.r() * rg_ - 1.0;\n> > +\t\tdouble deltaB = gains.b() * bg_ - 1.0;\n> > +\t\tdouble delta2 = deltaR * deltaR + deltaB * deltaB;\n> > +\n> > +\t\treturn delta2;\n> > +\t}\n> > +\n> > +\tRGB rgbMeans() const override\n> > +\t{\n> > +\t\treturn rgbMeans_;\n> > +\t}\n> > +\n> > +\tbool valid() const override\n> > +\t{\n> > +\t\t/* Minimum mean value below which AWB can't operate. */\n> > +\t\tconstexpr double minValue = 0.2;\n> > +\n> > +\t\treturn rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||\n> > +\t\t rgbMeans_.b() > minValue;\n> > +\t}\n> > +\n> > +private:\n> > +\tRGB rgbMeans_;\n> > +\tdouble rg_;\n> > +\tdouble bg_;\n> > +};\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::init\n> > + */\n> > +int Awb::init(IPAContext &context, const ValueNode &tuningData)\n> > +{\n> > +\treturn awbAlgo_.init(tuningData, context.ctrlMap);\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::configure\n> > + */\n> > int Awb::configure(IPAContext &context,\n> > \t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> > {\n> > -\tauto &gains = context.activeState.awb.gains;\n> > -\tgains = { { 1.0, 1.0, 1.0 } };\n> > +\treturn awbAlgo_.configure(context.activeState.awb);\n> > +}\n> >\n> > -\treturn 0;\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > + */\n> > +void Awb::queueRequest(IPAContext &context,\n> > +\t\t const uint32_t frame,\n> > +\t\t IPAFrameContext &frameContext,\n> > +\t\t const ControlList &controls)\n> > +{\n> > +\tawbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,\n> > +\t\t\t controls);\n> > }\n> >\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > + */\n> > void Awb::prepare(IPAContext &context,\n> > \t\t [[maybe_unused]] const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext,\n> > \t\t DebayerParams *params)\n> > {\n> > -\tauto &gains = context.activeState.awb.gains;\n> > +\tawbAlgo_.prepare(context.activeState.awb, frameContext.awb);\n> >\n> > -\tframeContext.gains = gains;\n> > -\tparams->gains = gains;\n> > +\tparams->gains = frameContext.awb.gains;\n> > }\n> >\n> > -void Awb::process(IPAContext &context,\n> > -\t\t [[maybe_unused]] const uint32_t frame,\n> > -\t\t IPAFrameContext &frameContext,\n> > -\t\t const SwIspStats *stats,\n> > -\t\t ControlList &metadata)\n> > +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,\n> > +\t\t\t\t const SwIspStats *stats) const\n> > {\n> > +\tif (!stats->valid)\n> > +\t\treturn {};\n> > +\n> > \tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n> > \tconst uint8_t blackLevel = context.activeState.blc.level;\n> >\n> > -\tmetadata.set(controls::ColourGains, { frameContext.gains.r(),\n> > -\t\t\t\t\t frameContext.gains.b() });\n> > -\n> > -\tif (!stats->valid)\n> > -\t\treturn;\n> > -\n> > \t/*\n> > \t * Black level must be subtracted to get the correct AWB ratios, they\n> > \t * would be off if they were computed from the whole brightness range\n> > @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,\n> > \t\thistogram.begin(), histogram.end(), uint64_t(0));\n> > \tconst uint64_t offset = blackLevel * nPixels;\n> > \tconst uint64_t minValid = 1;\n> > +\n> > \t/*\n> > \t * Make sure the sums are at least minValid, while preventing unsigned\n> > \t * integer underflow.\n> > \t */\n> > \tconst RGB sum = stats->sum_.max(offset + minValid) - offset;\n> >\n> > +\tRGB rgbMeans = { { static_cast(sum.r() / nPixels),\n> > +\t\t\t\t static_cast(sum.g() / nPixels),\n> > +\t\t\t\t static_cast(sum.b() / nPixels) } };\n>\n>\n> It would be a little bit more correct to cast the sums, before applying\n> the division, rather than the integer division result:\n>\n> static_cast(sum.r()) / nPixels\n>\n> > +\n> > \t/*\n> > -\t * Calculate red and blue gains for AWB.\n> > -\t * Clamp max gain at 4.0, this also avoids 0 division.\n> > +\t * \\todo Determine the minimum allowed thresholds from the mean\n> > +\t * but we currently have the sum - not the mean value!\n>\n> I don't understand the TODO -- what is it talking about and what mean\n> and sum are referred here?\n\nTo be honest I don't know how the soft ISP calculate statistics and I\ndidn't write the \\todo, but if I just look at struct SwIspStats\ndefinition:\n\n\t/**\n\t * \\brief Sums of colour channels of all the sampled pixels\n\t */\n\tRGB sum_;\n\nI presume to generate stats the image is divided into grids, the mean\nvalue of each colour channel is calculated per zone, the averaged and\nreported in sums_.\n\nSo we don't have a per-window mean value which we can compared with\nSimpleAwbStats::minColourValue() to discard zones with not enough\ncolour information, but we only have the sum when we get here.\n\nI guess the comment means we can't really discard zones by filtering\non the mean colour value.\n\n>\n> > +\t *\n> > +\t * Currently set to SimpleAwbStats::minColourValue() = 0.2.\n> > \t */\n> > -\tauto &gains = context.activeState.awb.gains;\n> > -\tgains = { {\n> > -\t\tsum.r() <= sum.g() / 4 ? 4.0f : static_cast(sum.g()) / sum.r(),\n> > -\t\t1.0,\n> > -\t\tsum.b() <= sum.g() / 4 ? 4.0f : static_cast(sum.g()) / sum.b(),\n> > -\t} };\n> > -\n> > -\tRGB rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };\n> > -\tcontext.activeState.awb.temperatureK = estimateCCT(rgbGains);\n> > -\tmetadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK);\n> > -\n> > -\tLOG(IPASoftAwb, Debug)\n> > -\t\t<< \"gain R/B: \" << gains << \"; temperature: \"\n> > -\t\t<< context.activeState.awb.temperatureK;\n> > +\treturn SimpleAwbStats(rgbMeans);\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::process\n> > + */\n> > +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > +\t\t IPAFrameContext &frameContext, const SwIspStats *stats,\n> > +\t\t ControlList &metadata)\n> > +{\n> > +\tSimpleAwbStats awbStats = calculateRgbMeans(context, stats);\n> > +\n> > +\tawbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,\n> > +\t\t\t kDefaultLux, metadata);\n> > }\n> >\n> > REGISTER_IPA_ALGORITHM(Awb, \"Awb\")\n> > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h\n> > index ad993f39c180..fec48c6eea1c 100644\n> > --- a/src/ipa/simple/algorithms/awb.h\n> > +++ b/src/ipa/simple/algorithms/awb.h\n> > @@ -7,19 +7,37 @@\n> >\n> > #pragma once\n> >\n> > +#include \n> > +\n> > +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> > +#include \"libcamera/internal/value_node.h\"\n> > +\n> > +#include \"libipa/awb.h\"\n> > +#include \"libipa/fixedpoint.h\"\n> > +#include \"simple/ipa_context.h\"\n> > +\n> > #include \"algorithm.h\"\n> >\n> > namespace libcamera {\n> >\n> > namespace ipa::soft::algorithms {\n> >\n> > +class SimpleAwbStats;\n> > +\n> > class Awb : public Algorithm\n> > {\n> > public:\n> > \tAwb() = default;\n> > \t~Awb() = default;\n> >\n> > +\tint init(IPAContext &context,\n> > +\t\t const ValueNode &tuningData) override;\n> > \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> > +\n> > +\tvoid queueRequest(IPAContext &context,\n> > +\t\t\t [[maybe_unused]] const uint32_t frame,\n> > +\t\t\t IPAFrameContext &frameContext,\n> > +\t\t\t const ControlList &controls) override;\n> > \tvoid prepare(IPAContext &context,\n> > \t\t const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext,\n> > @@ -29,6 +47,17 @@ public:\n> > \t\t IPAFrameContext &frameContext,\n> > \t\t const SwIspStats *stats,\n> > \t\t ControlList &metadata) override;\n> > +\n> > +private:\n> > +\tSimpleAwbStats calculateRgbMeans(IPAContext &context,\n> > +\t\t\t\t\t const SwIspStats *stats) const;\n> > +\n> > +\t/*\n> > +\t * There actually is no Q register format for SoftISP, but allow the\n> > +\t * colour gains to range in the [0.0f, 3.999f] interval, which seems\n> > +\t * reasonable.\n> > +\t */\n> > +\tAwbAlgorithm> awbAlgo_;\n> > };\n> >\n> > } /* namespace ipa::soft::algorithms */\n> > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> > index 677be56ed669..e45a913cd970 100644\n> > --- a/src/ipa/simple/algorithms/blc.cpp\n> > +++ b/src/ipa/simple/algorithms/blc.cpp\n> > @@ -53,7 +53,7 @@ void BlackLevel::prepare(IPAContext &context,\n> > \t\t\t DebayerParams *params)\n> > {\n> > \t/* Latch the blacklevel gain so GPUISP can apply. */\n> > -\tparams->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n> > +\tparams->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n> > }\n> >\n> > void BlackLevel::process(IPAContext &context,\n> > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> > index ace9c35dc462..1174784edc7e 100644\n> > --- a/src/ipa/simple/algorithms/ccm.cpp\n> > +++ b/src/ipa/simple/algorithms/ccm.cpp\n> > @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)\n> > void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> > {\n> > -\tconst unsigned int ct = context.activeState.awb.temperatureK;\n> > +\tconst unsigned int ct = frameContext.awb.colourTemperature;\n> >\n> > \t/* Change CCM only on bigger temperature changes. */\n> > \tif (!currentCcm_ ||\n> > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> > index 8ccfacb46a59..29643a655ce1 100644\n> > --- a/src/ipa/simple/ipa_context.h\n> > +++ b/src/ipa/simple/ipa_context.h\n> > @@ -16,6 +16,7 @@\n> > #include \"libcamera/internal/matrix.h\"\n> > #include \"libcamera/internal/vector.h\"\n> >\n> > +#include \n> > #include \n> >\n> > #include \"core_ipa_interface.h\"\n> > @@ -36,6 +37,8 @@ struct IPASessionConfiguration {\n> > };\n> >\n> > struct IPAActiveState {\n> > +\tipa::awb::ActiveState awb;\n> > +\n> > \tstruct {\n> > \t\tint32_t exposure;\n> > \t\tdouble again;\n> > @@ -48,11 +51,6 @@ struct IPAActiveState {\n> > \t\tdouble lastGain;\n> > \t} blc;\n> >\n> > -\tstruct {\n> > -\t\tRGB gains;\n> > -\t\tunsigned int temperatureK;\n> > -\t} awb;\n> > -\n> > \tMatrix combinedMatrix;\n> >\n> > \tstruct {\n> > @@ -64,6 +62,8 @@ struct IPAActiveState {\n> > };\n> >\n> > struct IPAFrameContext : public FrameContext {\n> > +\tipa::awb::FrameContext awb;\n> > +\n> > \tMatrix ccm;\n> >\n> > \tstruct {\n> > @@ -71,8 +71,6 @@ struct IPAFrameContext : public FrameContext {\n> > \t\tdouble gain;\n> > \t} sensor;\n> >\n> > -\tRGB gains;\n> > -\n> > \tfloat gamma;\n> > \tstd::optional contrast;\n> > \tstd::optional saturation;\n> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> > index 49382b4c2719..ab22635fdfaf 100644\n> > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> > @@ -1010,9 +1010,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> > \t};\n> > \tconst unsigned int gammaTableSize = gammaTable_.size();\n> >\n> > -\tconst RGB blackIndex = params.blackLevel * kRGBLookupSize;\n> > -\tconst RGB gains = params.gains;\n> > -\tconst RGB div = (RGB(kRGBLookupSize) - blackIndex).max(1.0);\n> > +\tconst RGB blackIndex = params.blackLevel * kRGBLookupSize;\n> > +\tconst RGB gains = params.gains;\n> > +\tconst RGB div = (RGB(kRGBLookupSize) - blackIndex).max(1.0);\n> >\n> > \tif (ccmEnabled_) {\n> > \t\tif (gammaUpdateNeeded ||\n> > @@ -1025,7 +1025,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> > \t\t\tconst unsigned int greenIndex = 1;\n> > \t\t\tconst unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;\n> > \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> > -\t\t\t\tconst RGB rgb = (gains * (RGB(i) - blackIndex) * kRGBLookupSize / div)\n> > +\t\t\t\tconst RGB rgb = (gains * (RGB(i) - blackIndex) * kRGBLookupSize / div)\n> > \t\t\t\t\t\t\t .clamp(0.0, kRGBLookupSize - 1);\n> > \t\t\t\tred[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);\n> > \t\t\t\tred[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);\n> > @@ -1045,8 +1045,8 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> > \t\t\tauto &green = green_;\n> > \t\t\tauto &blue = swapRedBlueGains_ ? red_ : blue_;\n> > \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> > -\t\t\t\tconst RGB lutGains =\n> > -\t\t\t\t\t(gains * (RGB(i) - blackIndex) * gammaTableSize / div)\n> > +\t\t\t\tconst RGB lutGains =\n> > +\t\t\t\t\t(gains * (RGB(i) - blackIndex) * gammaTableSize / div)\n> > \t\t\t\t\t\t.clamp(0.0, gammaTableSize - 1);\n> > \t\t\t\tred[i] = gammaTable_[lutGains.r()];\n> > \t\t\t\tgreen[i] = gammaTable_[lutGains.g()];\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 E7A0AC3332\n\tfor ;\n\tFri, 10 Jul 2026 12:32:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13CAB66104;\n\tFri, 10 Jul 2026 14:32:59 +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 1750665F7C\n\tfor ;\n\tFri, 10 Jul 2026 14:32:57 +0200 (CEST)","from ideasonboard.com (mob-109-113-15-151.net.vodafone.it\n\t[109.113.15.151])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 57A701ADF;\n\tFri, 10 Jul 2026 14:32:05 +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=\"hqt0w7Lo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1783686725;\n\tbh=54MMmjq7Ig3FijIYP1XPoFEe6sdU7XDOTgG5Ux/ZN1o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hqt0w7LoyZ3JJw1eHbGmS7pOP9nGW/Ck2TRRkOK+QRbh9Ha+0UQYdh2hYVi81fSJ0\n\tYLxfRC93TXdZM+PAuHa9A9S1dtDs4hgosS1uESawHeJaOEDeKyMLFzpldUhyHcnlXG\n\tZCFKWJfpERS4NfCz5bU8FWmDvr3lYtLUFeYORgXo=","Date":"Fri, 10 Jul 2026 14:32:53 +0200","From":"Jacopo Mondi ","To":"Milan Zamazal ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug , \n\tKieran Bingham ,\n\tRobert Mader ","Subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","Message-ID":"","References":"<20260708-libipa-algorithms-v5-0-0759d0359f52@ideasonboard.com>\n\t<20260708-libipa-algorithms-v5-3-0759d0359f52@ideasonboard.com>\n\t<85ldbj5h85.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<85ldbj5h85.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":39670,"web_url":"https://patchwork.libcamera.org/comment/39670/","msgid":"<85zezy50r1.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2026-07-10T15:21:22","subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Jacopo Mondi writes:\n\n> Hi Milan\n>\n> On Fri, Jul 10, 2026 at 11:25:30AM +0200, Milan Zamazal wrote:\n>> Hi Jacopo,\n>>\n>> Jacopo Mondi writes:\n>>\n>> > From: Kieran Bingham \n>> >\n>> > Port the SoftISP Awb algorithm to use the new libipa implementation\n>> > of AwbAlgorithm.\n>> >\n>> > The awbAlgo_ class member is initialized with the Q<4, 8> type even if\n>> > there is no physical register representation for SoftISP.\n>> >\n>> > The usage of libipa::awb::Context, which defines the gains vector as\n>> > RGB, in the SimpleIPA ActiveState and FrameContext requires\n>> > changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB\n>> > and RGB.\n>> >\n>> > Signed-off-by: Kieran Bingham \n>> > Signed-off-by: Jacopo Mondi \n>> > Reviewed-by: Kieran Bingham \n>> > Tested-by: Robert Mader \n>> > ---\n>> > .../internal/software_isp/debayer_params.h | 4 +-\n>> > src/ipa/simple/algorithms/awb.cpp | 141 ++++++++++++++++-----\n>> > src/ipa/simple/algorithms/awb.h | 29 +++++\n>> > src/ipa/simple/algorithms/blc.cpp | 2 +-\n>> > src/ipa/simple/algorithms/ccm.cpp | 2 +-\n>> > src/ipa/simple/ipa_context.h | 12 +-\n>> > src/libcamera/software_isp/debayer_cpu.cpp | 12 +-\n>> > 7 files changed, 151 insertions(+), 51 deletions(-)\n>> >\n>> > diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n>> > index 6772b43bced4..1074720d73c2 100644\n>> > --- a/include/libcamera/internal/software_isp/debayer_params.h\n>> > +++ b/include/libcamera/internal/software_isp/debayer_params.h\n>> > @@ -21,10 +21,10 @@ struct DebayerParams {\n>> > \tMatrix combinedMatrix = { { 1.0, 0.0, 0.0,\n>> > \t\t\t\t\t\t 0.0, 1.0, 0.0,\n>> > \t\t\t\t\t\t 0.0, 0.0, 1.0 } };\n>> > -\tRGB blackLevel = RGB({ 0.0, 0.0, 0.0 });\n>> > +\tRGB blackLevel = RGB({ 0.0, 0.0, 0.0 });\n>> > \tfloat gamma = 1.0;\n>> > \tfloat contrastExp = 1.0;\n>> > -\tRGB gains = RGB({ 1.0, 1.0, 1.0 });\n>> > +\tRGB gains = RGB({ 1.0, 1.0, 1.0 });\n>> > };\n>> >\n>> > } /* namespace libcamera */\n>> > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp\n>> > index 05155c83d172..64ce4f91b21c 100644\n>> > --- a/src/ipa/simple/algorithms/awb.cpp\n>> > +++ b/src/ipa/simple/algorithms/awb.cpp\n>> > @@ -15,7 +15,6 @@\n>> > #include \n>> >\n>> > #include \"libipa/colours.h\"\n>>\n>> This include can be removed too.\n>>\n>\n> ack!\n>\n>> > -#include \"simple/ipa_context.h\"\n>> >\n>> > namespace libcamera {\n>> >\n>> > @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)\n>> >\n>> > namespace ipa::soft::algorithms {\n>> >\n>> > +/*\n>> > + * \\todo Replace it with a proper Lux algorithm\n>> > + */\n>> > +static constexpr unsigned int kDefaultLux = 500;\n>> > +\n>> > +class SimpleAwbStats final : public AwbStats\n>>\n>> This class is the same across three pipelines -- should it be a part of\n>> libipa?\n>>\n>\n> Eh, that's what I had initially, but I've been asked to move the\n> implementation to each single IPA. I understand why, right now we just\n> copied around the RkISP1 implementation instead of having each\n> platform calculate statistics in their own way and moving the RkISP1\n> implementation to the base class would have communicated that this is\n> what we expect all platforms to do, while in reality, we just copied\n> code around.\n\nOK, let's see...\n\n>> > +{\n>> > +public:\n>> > +\tSimpleAwbStats() = default;\n>> > +\n>> > +\tSimpleAwbStats(const RGB &rgbMeans)\n>> > +\t{\n>> > +\t\trgbMeans_ = rgbMeans;\n>> > +\n>> > +\t\trg_ = rgbMeans_.r() / rgbMeans_.g();\n>> > +\t\tbg_ = rgbMeans_.b() / rgbMeans_.g();\n>> > +\t}\n>> > +\n>> > +\tdouble computeColourError(const RGB &gains) const override\n>> > +\t{\n>> > +\t\t/*\n>> > +\t\t * Compute the sum of the squared colour error (non-greyness) as\n>> > +\t\t * it appears in the log likelihood equation.\n>> > +\t\t */\n>> > +\t\tdouble deltaR = gains.r() * rg_ - 1.0;\n>> > +\t\tdouble deltaB = gains.b() * bg_ - 1.0;\n>> > +\t\tdouble delta2 = deltaR * deltaR + deltaB * deltaB;\n>> > +\n>> > +\t\treturn delta2;\n>> > +\t}\n>> > +\n>> > +\tRGB rgbMeans() const override\n>> > +\t{\n>> > +\t\treturn rgbMeans_;\n>> > +\t}\n>> > +\n>> > +\tbool valid() const override\n>> > +\t{\n>> > +\t\t/* Minimum mean value below which AWB can't operate. */\n>> > +\t\tconstexpr double minValue = 0.2;\n>> > +\n>> > +\t\treturn rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||\n>> > +\t\t rgbMeans_.b() > minValue;\n>> > +\t}\n>> > +\n>> > +private:\n>> > +\tRGB rgbMeans_;\n>> > +\tdouble rg_;\n>> > +\tdouble bg_;\n>> > +};\n>> > +\n>> > +/**\n>> > + * \\copydoc libcamera::ipa::Algorithm::init\n>> > + */\n>> > +int Awb::init(IPAContext &context, const ValueNode &tuningData)\n>> > +{\n>> > +\treturn awbAlgo_.init(tuningData, context.ctrlMap);\n>> > +}\n>> > +\n>> > +/**\n>> > + * \\copydoc libcamera::ipa::Algorithm::configure\n>> > + */\n>> > int Awb::configure(IPAContext &context,\n>> > \t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> > {\n>> > -\tauto &gains = context.activeState.awb.gains;\n>> > -\tgains = { { 1.0, 1.0, 1.0 } };\n>> > +\treturn awbAlgo_.configure(context.activeState.awb);\n>> > +}\n>> >\n>> > -\treturn 0;\n>> > +/**\n>> > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n>> > + */\n>> > +void Awb::queueRequest(IPAContext &context,\n>> > +\t\t const uint32_t frame,\n>> > +\t\t IPAFrameContext &frameContext,\n>> > +\t\t const ControlList &controls)\n>> > +{\n>> > +\tawbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,\n>> > +\t\t\t controls);\n>> > }\n>> >\n>> > +/**\n>> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n>> > + */\n>> > void Awb::prepare(IPAContext &context,\n>> > \t\t [[maybe_unused]] const uint32_t frame,\n>> > \t\t IPAFrameContext &frameContext,\n>> > \t\t DebayerParams *params)\n>> > {\n>> > -\tauto &gains = context.activeState.awb.gains;\n>> > +\tawbAlgo_.prepare(context.activeState.awb, frameContext.awb);\n>> >\n>> > -\tframeContext.gains = gains;\n>> > -\tparams->gains = gains;\n>> > +\tparams->gains = frameContext.awb.gains;\n>> > }\n>> >\n>> > -void Awb::process(IPAContext &context,\n>> > -\t\t [[maybe_unused]] const uint32_t frame,\n>> > -\t\t IPAFrameContext &frameContext,\n>> > -\t\t const SwIspStats *stats,\n>> > -\t\t ControlList &metadata)\n>> > +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,\n>> > +\t\t\t\t const SwIspStats *stats) const\n>> > {\n>> > +\tif (!stats->valid)\n>> > +\t\treturn {};\n>> > +\n>> > \tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n>> > \tconst uint8_t blackLevel = context.activeState.blc.level;\n>> >\n>> > -\tmetadata.set(controls::ColourGains, { frameContext.gains.r(),\n>> > -\t\t\t\t\t frameContext.gains.b() });\n>> > -\n>> > -\tif (!stats->valid)\n>> > -\t\treturn;\n>> > -\n>> > \t/*\n>> > \t * Black level must be subtracted to get the correct AWB ratios, they\n>> > \t * would be off if they were computed from the whole brightness range\n>> > @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,\n>> > \t\thistogram.begin(), histogram.end(), uint64_t(0));\n>> > \tconst uint64_t offset = blackLevel * nPixels;\n>> > \tconst uint64_t minValid = 1;\n>> > +\n>> > \t/*\n>> > \t * Make sure the sums are at least minValid, while preventing unsigned\n>> > \t * integer underflow.\n>> > \t */\n>> > \tconst RGB sum = stats->sum_.max(offset + minValid) - offset;\n>> >\n>> > +\tRGB rgbMeans = { { static_cast(sum.r() / nPixels),\n>> > +\t\t\t\t static_cast(sum.g() / nPixels),\n>> > +\t\t\t\t static_cast(sum.b() / nPixels) } };\n>>\n>>\n>> It would be a little bit more correct to cast the sums, before applying\n>> the division, rather than the integer division result:\n>>\n>> static_cast(sum.r()) / nPixels\n>>\n>> > +\n>> > \t/*\n>> > -\t * Calculate red and blue gains for AWB.\n>> > -\t * Clamp max gain at 4.0, this also avoids 0 division.\n>> > +\t * \\todo Determine the minimum allowed thresholds from the mean\n>> > +\t * but we currently have the sum - not the mean value!\n>>\n>> I don't understand the TODO -- what is it talking about and what mean\n>> and sum are referred here?\n>\n> To be honest I don't know how the soft ISP calculate statistics and I\n> didn't write the \\todo, but if I just look at struct SwIspStats\n> definition:\n>\n> \t/**\n> \t * \\brief Sums of colour channels of all the sampled pixels\n> \t */\n> \tRGB sum_;\n>\n> I presume to generate stats the image is divided into grids, the mean\n> value of each colour channel is calculated per zone, the averaged and\n> reported in sums_.\n\nThere are no real zones, the image may be split by lines to areas\nprocessed separately by different threads (if enabled), purely for\nspeedup. But there is no inherent reason why zones couldn't be used in\nfuture, especially in GPU ISP.\n\n> So we don't have a per-window mean value which we can compared with\n> SimpleAwbStats::minColourValue() to discard zones with not enough\n> colour information, but we only have the sum when we get here.\n>\n> I guess the comment means we can't really discard zones by filtering\n> on the mean colour value.\n\nI see, thank you for clarification. Since there are currently no zones,\nSimpleAwbStats::minColourValue() mentioned below doesn't exist, and the\n\\todo is especially confusing in the context of the (whole image) RGB\nmeans computed just above it, I'd suggest dropping the whole comment.\nIf anybody thinks it's important to retain it, a clearer wording should\nbe suggested.\n\n>>\n>> > +\t *\n>> > +\t * Currently set to SimpleAwbStats::minColourValue() = 0.2.\n>> > \t */\n>> > -\tauto &gains = context.activeState.awb.gains;\n>> > -\tgains = { {\n>> > -\t\tsum.r() <= sum.g() / 4 ? 4.0f : static_cast(sum.g()) / sum.r(),\n>> > -\t\t1.0,\n>> > -\t\tsum.b() <= sum.g() / 4 ? 4.0f : static_cast(sum.g()) / sum.b(),\n>> > -\t} };\n>> > -\n>> > -\tRGB rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };\n>> > -\tcontext.activeState.awb.temperatureK = estimateCCT(rgbGains);\n>> > -\tmetadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK);\n>> > -\n>> > -\tLOG(IPASoftAwb, Debug)\n>> > -\t\t<< \"gain R/B: \" << gains << \"; temperature: \"\n>> > -\t\t<< context.activeState.awb.temperatureK;\n>> > +\treturn SimpleAwbStats(rgbMeans);\n>> > +}\n>> > +\n>> > +/**\n>> > + * \\copydoc libcamera::ipa::Algorithm::process\n>> > + */\n>> > +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> > +\t\t IPAFrameContext &frameContext, const SwIspStats *stats,\n>> > +\t\t ControlList &metadata)\n>> > +{\n>> > +\tSimpleAwbStats awbStats = calculateRgbMeans(context, stats);\n>> > +\n>> > +\tawbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,\n>> > +\t\t\t kDefaultLux, metadata);\n>> > }\n>> >\n>> > REGISTER_IPA_ALGORITHM(Awb, \"Awb\")\n>> > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h\n>> > index ad993f39c180..fec48c6eea1c 100644\n>> > --- a/src/ipa/simple/algorithms/awb.h\n>> > +++ b/src/ipa/simple/algorithms/awb.h\n>> > @@ -7,19 +7,37 @@\n>> >\n>> > #pragma once\n>> >\n>> > +#include \n>> > +\n>> > +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> > +#include \"libcamera/internal/value_node.h\"\n>> > +\n>> > +#include \"libipa/awb.h\"\n>> > +#include \"libipa/fixedpoint.h\"\n>> > +#include \"simple/ipa_context.h\"\n>> > +\n>> > #include \"algorithm.h\"\n>> >\n>> > namespace libcamera {\n>> >\n>> > namespace ipa::soft::algorithms {\n>> >\n>> > +class SimpleAwbStats;\n>> > +\n>> > class Awb : public Algorithm\n>> > {\n>> > public:\n>> > \tAwb() = default;\n>> > \t~Awb() = default;\n>> >\n>> > +\tint init(IPAContext &context,\n>> > +\t\t const ValueNode &tuningData) override;\n>> > \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> > +\n>> > +\tvoid queueRequest(IPAContext &context,\n>> > +\t\t\t [[maybe_unused]] const uint32_t frame,\n>> > +\t\t\t IPAFrameContext &frameContext,\n>> > +\t\t\t const ControlList &controls) override;\n>> > \tvoid prepare(IPAContext &context,\n>> > \t\t const uint32_t frame,\n>> > \t\t IPAFrameContext &frameContext,\n>> > @@ -29,6 +47,17 @@ public:\n>> > \t\t IPAFrameContext &frameContext,\n>> > \t\t const SwIspStats *stats,\n>> > \t\t ControlList &metadata) override;\n>> > +\n>> > +private:\n>> > +\tSimpleAwbStats calculateRgbMeans(IPAContext &context,\n>> > +\t\t\t\t\t const SwIspStats *stats) const;\n>> > +\n>> > +\t/*\n>> > +\t * There actually is no Q register format for SoftISP, but allow the\n>> > +\t * colour gains to range in the [0.0f, 3.999f] interval, which seems\n>> > +\t * reasonable.\n>> > +\t */\n>> > +\tAwbAlgorithm> awbAlgo_;\n>> > };\n>> >\n>> > } /* namespace ipa::soft::algorithms */\n>> > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>> > index 677be56ed669..e45a913cd970 100644\n>> > --- a/src/ipa/simple/algorithms/blc.cpp\n>> > +++ b/src/ipa/simple/algorithms/blc.cpp\n>> > @@ -53,7 +53,7 @@ void BlackLevel::prepare(IPAContext &context,\n>> > \t\t\t DebayerParams *params)\n>> > {\n>> > \t/* Latch the blacklevel gain so GPUISP can apply. */\n>> > -\tparams->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n>> > +\tparams->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n>> > }\n>> >\n>> > void BlackLevel::process(IPAContext &context,\n>> > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> > index ace9c35dc462..1174784edc7e 100644\n>> > --- a/src/ipa/simple/algorithms/ccm.cpp\n>> > +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> > @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)\n>> > void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> > \t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>> > {\n>> > -\tconst unsigned int ct = context.activeState.awb.temperatureK;\n>> > +\tconst unsigned int ct = frameContext.awb.colourTemperature;\n>> >\n>> > \t/* Change CCM only on bigger temperature changes. */\n>> > \tif (!currentCcm_ ||\n>> > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> > index 8ccfacb46a59..29643a655ce1 100644\n>> > --- a/src/ipa/simple/ipa_context.h\n>> > +++ b/src/ipa/simple/ipa_context.h\n>> > @@ -16,6 +16,7 @@\n>> > #include \"libcamera/internal/matrix.h\"\n>> > #include \"libcamera/internal/vector.h\"\n>> >\n>> > +#include \n>> > #include \n>> >\n>> > #include \"core_ipa_interface.h\"\n>> > @@ -36,6 +37,8 @@ struct IPASessionConfiguration {\n>> > };\n>> >\n>> > struct IPAActiveState {\n>> > +\tipa::awb::ActiveState awb;\n>> > +\n>> > \tstruct {\n>> > \t\tint32_t exposure;\n>> > \t\tdouble again;\n>> > @@ -48,11 +51,6 @@ struct IPAActiveState {\n>> > \t\tdouble lastGain;\n>> > \t} blc;\n>> >\n>> > -\tstruct {\n>> > -\t\tRGB gains;\n>> > -\t\tunsigned int temperatureK;\n>> > -\t} awb;\n>> > -\n>> > \tMatrix combinedMatrix;\n>> >\n>> > \tstruct {\n>> > @@ -64,6 +62,8 @@ struct IPAActiveState {\n>> > };\n>> >\n>> > struct IPAFrameContext : public FrameContext {\n>> > +\tipa::awb::FrameContext awb;\n>> > +\n>> > \tMatrix ccm;\n>> >\n>> > \tstruct {\n>> > @@ -71,8 +71,6 @@ struct IPAFrameContext : public FrameContext {\n>> > \t\tdouble gain;\n>> > \t} sensor;\n>> >\n>> > -\tRGB gains;\n>> > -\n>> > \tfloat gamma;\n>> > \tstd::optional contrast;\n>> > \tstd::optional saturation;\n>> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> > index 49382b4c2719..ab22635fdfaf 100644\n>> > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> > @@ -1010,9 +1010,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n>> > \t};\n>> > \tconst unsigned int gammaTableSize = gammaTable_.size();\n>> >\n>> > -\tconst RGB blackIndex = params.blackLevel * kRGBLookupSize;\n>> > -\tconst RGB gains = params.gains;\n>> > -\tconst RGB div = (RGB(kRGBLookupSize) - blackIndex).max(1.0);\n>> > +\tconst RGB blackIndex = params.blackLevel * kRGBLookupSize;\n>> > +\tconst RGB gains = params.gains;\n>> > +\tconst RGB div = (RGB(kRGBLookupSize) - blackIndex).max(1.0);\n>> >\n>> > \tif (ccmEnabled_) {\n>> > \t\tif (gammaUpdateNeeded ||\n>> > @@ -1025,7 +1025,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n>> > \t\t\tconst unsigned int greenIndex = 1;\n>> > \t\t\tconst unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;\n>> > \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n>> > -\t\t\t\tconst RGB rgb = (gains * (RGB(i) - blackIndex) * kRGBLookupSize / div)\n>> > +\t\t\t\tconst RGB rgb = (gains * (RGB(i) - blackIndex) * kRGBLookupSize / div)\n>> > \t\t\t\t\t\t\t .clamp(0.0, kRGBLookupSize - 1);\n>> > \t\t\t\tred[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);\n>> > \t\t\t\tred[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);\n>> > @@ -1045,8 +1045,8 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n>> > \t\t\tauto &green = green_;\n>> > \t\t\tauto &blue = swapRedBlueGains_ ? red_ : blue_;\n>> > \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n>> > -\t\t\t\tconst RGB lutGains =\n>> > -\t\t\t\t\t(gains * (RGB(i) - blackIndex) * gammaTableSize / div)\n>> > +\t\t\t\tconst RGB lutGains =\n>> > +\t\t\t\t\t(gains * (RGB(i) - blackIndex) * gammaTableSize / div)\n>> > \t\t\t\t\t\t.clamp(0.0, gammaTableSize - 1);\n>> > \t\t\t\tred[i] = gammaTable_[lutGains.r()];\n>> > \t\t\t\tgreen[i] = gammaTable_[lutGains.g()];\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 E8BC2C3332\n\tfor ;\n\tFri, 10 Jul 2026 15:21:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0D0E66112;\n\tFri, 10 Jul 2026 17:21:30 +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 D45AC65F7C\n\tfor ;\n\tFri, 10 Jul 2026 17:21:28 +0200 (CEST)","from mail-wr1-f69.google.com (mail-wr1-f69.google.com\n\t[209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-663-UiRzydtfOM65Rqpqvvvkrw-1; Fri, 10 Jul 2026 11:21:26 -0400","by mail-wr1-f69.google.com with SMTP id\n\tffacd0b85a97d-47dee8adf4bso1057388f8f.1\n\tfor ;\n\tFri, 10 Jul 2026 08:21:25 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-4.net.vodafone.cz. [77.48.47.4])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-47aa039ad21sm59904224f8f.20.2026.07.10.08.21.22\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 10 Jul 2026 08:21:23 -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=\"QlvA5dsd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1783696887;\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=t5x3PffJYDbQJ0VeC7IPvWsozrPhVrnNkFFa6q958aw=;\n\tb=QlvA5dsdF97WsAb3IyqAwYjNd5+gLM4SHzse6jnImBMn+XxpT+9IJMGAJkZSJX5e7jVzni\n\tVuQI1Mg3RrGZXBrSBgYoINp9g3ES7IluYlz3LCeCCcpvhXCWE2XUgU1iPGvEmVnv99gjpz\n\texAh0V2ffU+8ukZDkAMEYKNkbpZrHyo=","X-MC-Unique":"UiRzydtfOM65Rqpqvvvkrw-1","X-Mimecast-MFC-AGG-ID":"UiRzydtfOM65Rqpqvvvkrw_1783696885","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20251104; t=1783696885; x=1784301685;\n\th=content-type:mime-version:user-agent:message-id:date:references\n\t:in-reply-to:subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to\n\t:cc:subject:date:message-id:reply-to:content-type;\n\tbh=t5x3PffJYDbQJ0VeC7IPvWsozrPhVrnNkFFa6q958aw=;\n\tb=JTpZyGXk0U550NzA3jCMI3Pye4lRDVHmzXjc9M8kYv+Zy3kVR7LHKLxC4mT2U4w4uL\n\tf7nudUbyukq9Lvrtqwv8YJTDp+6UQWTVNyNu1a2PIVDpTDaZuyGnkIrqQavkLQO0HAzl\n\tD5QDNfodc4Ilr40m6W8Aa+cEypCpK4uCKQF/mWDQAzBihn6QLAcTJlE4tqvfvelHUGTd\n\t4YZdnUcw4ZM7GjuhV8J5eqwnqoAWoRT4dvjaxZH9V2sL73S+z1KmXUBVnWksMI9Ys4LZ\n\td/hbPyP1Aem2QBgpDrpMzxhx4sf6xVC0fAFwysmuJzwFg1BJWAt2NcAX0GNJE3sUwxlj\n\t8uKw==","X-Gm-Message-State":"AOJu0YzQztMEALaKo8cHNx21JxgGynVF4GZIA09yq3Oguu2rcIFOFzGd\n\toPqWSq21MJ0sBB0AsLApKLxcbmcAs8nlDpNiVAWn/LlVM7Cgp1Pf4KQsyhOm4+DhLeCAZGf/SoB\n\tj1RzPSoLMGY1CaqtT6clqcvzImwH8/qAGojpM85SC+5IIwxR7jKo0yDW5L05JOfx+KRUbFxX45F\n\tY=","X-Gm-Gg":"AfdE7ck2yGlVf2BPqWTXDJzIWT1ZWqO9D2MXf9mNR8rtT1KLCVenBg/jBUIovkxneNy\n\tEQOkBTbsV/tcDor73g3UKWXHKBg0xV+33+6LICpSdsdAUt1K2zU8yF8YIqHs4aJFA9/XdewgUxp\n\tMksOCj7HAg4+/o7T+Ba/lPn7f5CdwMy1rAoj9X9tdRbDhlxVfK3DoO7STTw5YTygLeqbhGOonYz\n\tsTNSEWHm1ZcNeBXiiLh/UHxSPBPLiSLm9QKZrE4hgfwVhrTOFc0iOpnO9q6WJ8bO3hfeWBsGoJR\n\tTuP1isvRWBjFlFqiKCdfWtf29B0U++p3tuyVdBlhJxahJaogzYsEEw3Gdy2wZDca5vA+6h+R5z6\n\txXEm6nUxdPRJXwlGIy3TZbANWIZx2hgVwxnHmy6gyQT5Y1tuloBeg5oIenwVtxqQZ","X-Received":["by 2002:a05:600c:e555:10b0:493:cd3f:d051 with SMTP id\n\t5b1f17b1804b1-493e68ddcf7mr89178225e9.25.1783696884639; \n\tFri, 10 Jul 2026 08:21:24 -0700 (PDT)","by 2002:a05:600c:e555:10b0:493:cd3f:d051 with SMTP id\n\t5b1f17b1804b1-493e68ddcf7mr89177915e9.25.1783696884084; \n\tFri, 10 Jul 2026 08:21:24 -0700 (PDT)"],"From":"Milan Zamazal ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Stefan Klug\n\t, Kieran Bingham\n\t, Robert Mader\n\t","Subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","In-Reply-To":" (Jacopo Mondi's message of \"Fri, 10 Jul\n\t2026 14:32:53 +0200\")","References":"<20260708-libipa-algorithms-v5-0-0759d0359f52@ideasonboard.com>\n\t<20260708-libipa-algorithms-v5-3-0759d0359f52@ideasonboard.com>\n\t<85ldbj5h85.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t","Date":"Fri, 10 Jul 2026 17:21:22 +0200","Message-ID":"<85zezy50r1.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":"IxbXJRyXRgLTwomaryWEBwrdIJsEiOWtS3jhY3Lf9uI_1783696885","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":39671,"web_url":"https://patchwork.libcamera.org/comment/39671/","msgid":"<178369908133.727359.245546119031834639@ping.linuxembedded.co.uk>","date":"2026-07-10T15:58:01","subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2026-07-10 16:21:22)\n> Jacopo Mondi writes:\n> \n> > Hi Milan\n> >\n> > On Fri, Jul 10, 2026 at 11:25:30AM +0200, Milan Zamazal wrote:\n> >> Hi Jacopo,\n> >>\n> >> Jacopo Mondi writes:\n> >>\n> >> > From: Kieran Bingham \n> >> >\n> >> > Port the SoftISP Awb algorithm to use the new libipa implementation\n> >> > of AwbAlgorithm.\n> >> >\n> >> > The awbAlgo_ class member is initialized with the Q<4, 8> type even if\n> >> > there is no physical register representation for SoftISP.\n> >> >\n> >> > The usage of libipa::awb::Context, which defines the gains vector as\n> >> > RGB, in the SimpleIPA ActiveState and FrameContext requires\n> >> > changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB\n> >> > and RGB.\n> >> >\n> >> > Signed-off-by: Kieran Bingham \n> >> > Signed-off-by: Jacopo Mondi \n> >> > Reviewed-by: Kieran Bingham \n> >> > Tested-by: Robert Mader \n> >> > ---\n> >> > .../internal/software_isp/debayer_params.h | 4 +-\n\n\n\n> >> > -#include \"simple/ipa_context.h\"\n> >> >\n> >> > namespace libcamera {\n> >> >\n> >> > @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)\n> >> >\n> >> > namespace ipa::soft::algorithms {\n> >> >\n> >> > +/*\n> >> > + * \\todo Replace it with a proper Lux algorithm\n> >> > + */\n> >> > +static constexpr unsigned int kDefaultLux = 500;\n> >> > +\n> >> > +class SimpleAwbStats final : public AwbStats\n> >>\n> >> This class is the same across three pipelines -- should it be a part of\n> >> libipa?\n> >>\n> >\n> > Eh, that's what I had initially, but I've been asked to move the\n> > implementation to each single IPA. I understand why, right now we just\n> > copied around the RkISP1 implementation instead of having each\n> > platform calculate statistics in their own way and moving the RkISP1\n> > implementation to the base class would have communicated that this is\n> > what we expect all platforms to do, while in reality, we just copied\n> > code around.\n> \n> OK, let's see...\n> \n> >> > +{\n> >> > +public:\n> >> > + SimpleAwbStats() = default;\n> >> > +\n> >> > + SimpleAwbStats(const RGB &rgbMeans)\n> >> > + {\n> >> > + rgbMeans_ = rgbMeans;\n> >> > +\n> >> > + rg_ = rgbMeans_.r() / rgbMeans_.g();\n> >> > + bg_ = rgbMeans_.b() / rgbMeans_.g();\n> >> > + }\n> >> > +\n> >> > + double computeColourError(const RGB &gains) const override\n> >> > + {\n> >> > + /*\n> >> > + * Compute the sum of the squared colour error (non-greyness) as\n> >> > + * it appears in the log likelihood equation.\n> >> > + */\n> >> > + double deltaR = gains.r() * rg_ - 1.0;\n> >> > + double deltaB = gains.b() * bg_ - 1.0;\n> >> > + double delta2 = deltaR * deltaR + deltaB * deltaB;\n> >> > +\n> >> > + return delta2;\n> >> > + }\n> >> > +\n> >> > + RGB rgbMeans() const override\n> >> > + {\n> >> > + return rgbMeans_;\n> >> > + }\n> >> > +\n> >> > + bool valid() const override\n> >> > + {\n> >> > + /* Minimum mean value below which AWB can't operate. */\n> >> > + constexpr double minValue = 0.2;\n> >> > +\n> >> > + return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||\n> >> > + rgbMeans_.b() > minValue;\n> >> > + }\n> >> > +\n> >> > +private:\n> >> > + RGB rgbMeans_;\n> >> > + double rg_;\n> >> > + double bg_;\n> >> > +};\n> >> > +\n> >> > +/**\n> >> > + * \\copydoc libcamera::ipa::Algorithm::init\n> >> > + */\n> >> > +int Awb::init(IPAContext &context, const ValueNode &tuningData)\n> >> > +{\n> >> > + return awbAlgo_.init(tuningData, context.ctrlMap);\n> >> > +}\n> >> > +\n> >> > +/**\n> >> > + * \\copydoc libcamera::ipa::Algorithm::configure\n> >> > + */\n> >> > int Awb::configure(IPAContext &context,\n> >> > [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >> > {\n> >> > - auto &gains = context.activeState.awb.gains;\n> >> > - gains = { { 1.0, 1.0, 1.0 } };\n> >> > + return awbAlgo_.configure(context.activeState.awb);\n> >> > +}\n> >> >\n> >> > - return 0;\n> >> > +/**\n> >> > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> >> > + */\n> >> > +void Awb::queueRequest(IPAContext &context,\n> >> > + const uint32_t frame,\n> >> > + IPAFrameContext &frameContext,\n> >> > + const ControlList &controls)\n> >> > +{\n> >> > + awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,\n> >> > + controls);\n> >> > }\n> >> >\n> >> > +/**\n> >> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> >> > + */\n> >> > void Awb::prepare(IPAContext &context,\n> >> > [[maybe_unused]] const uint32_t frame,\n> >> > IPAFrameContext &frameContext,\n> >> > DebayerParams *params)\n> >> > {\n> >> > - auto &gains = context.activeState.awb.gains;\n> >> > + awbAlgo_.prepare(context.activeState.awb, frameContext.awb);\n> >> >\n> >> > - frameContext.gains = gains;\n> >> > - params->gains = gains;\n> >> > + params->gains = frameContext.awb.gains;\n> >> > }\n> >> >\n> >> > -void Awb::process(IPAContext &context,\n> >> > - [[maybe_unused]] const uint32_t frame,\n> >> > - IPAFrameContext &frameContext,\n> >> > - const SwIspStats *stats,\n> >> > - ControlList &metadata)\n> >> > +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,\n> >> > + const SwIspStats *stats) const\n> >> > {\n> >> > + if (!stats->valid)\n> >> > + return {};\n> >> > +\n> >> > const SwIspStats::Histogram &histogram = stats->yHistogram;\n> >> > const uint8_t blackLevel = context.activeState.blc.level;\n> >> >\n> >> > - metadata.set(controls::ColourGains, { frameContext.gains.r(),\n> >> > - frameContext.gains.b() });\n> >> > -\n> >> > - if (!stats->valid)\n> >> > - return;\n> >> > -\n> >> > /*\n> >> > * Black level must be subtracted to get the correct AWB ratios, they\n> >> > * would be off if they were computed from the whole brightness range\n> >> > @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,\n> >> > histogram.begin(), histogram.end(), uint64_t(0));\n> >> > const uint64_t offset = blackLevel * nPixels;\n> >> > const uint64_t minValid = 1;\n> >> > +\n> >> > /*\n> >> > * Make sure the sums are at least minValid, while preventing unsigned\n> >> > * integer underflow.\n> >> > */\n> >> > const RGB sum = stats->sum_.max(offset + minValid) - offset;\n> >> >\n> >> > + RGB rgbMeans = { { static_cast(sum.r() / nPixels),\n> >> > + static_cast(sum.g() / nPixels),\n> >> > + static_cast(sum.b() / nPixels) } };\n> >>\n> >>\n> >> It would be a little bit more correct to cast the sums, before applying\n> >> the division, rather than the integer division result:\n> >>\n> >> static_cast(sum.r()) / nPixels\n\nCan't we do anything helpful with\n\tconstexpr Vector operator/(T scalar) const?\n\nconstexpr Vector operator/(T scalar) const\n\t{\n\t\treturn apply(*this, std::divides<>{}, scalar);\n\t}\n\n\nOr does the casting in from uint64_t still cause a fuss there anyway.\nIt would be nice if we could do:\n\n\tRGB rgbMeans = static_cast(sum) / nPixels;\n\nBut I think we'd have to add some sort of casting or template\nconstructor for that.\n\nI think that's close to something I tried before but maybe it needs a\ndifferent thought.\n\nAnyway, yes, we should do the cast first then the divide!\n\n> >>\n> >> > +\n> >> > /*\n> >> > - * Calculate red and blue gains for AWB.\n> >> > - * Clamp max gain at 4.0, this also avoids 0 division.\n> >> > + * \\todo Determine the minimum allowed thresholds from the mean\n> >> > + * but we currently have the sum - not the mean value!\n> >>\n> >> I don't understand the TODO -- what is it talking about and what mean\n> >> and sum are referred here?\n> >\n\nSo it looks likely that I might have written that about 6 or 7 months\nago, but I don't really remember it.\n\n\nhttps://git.ideasonboard.com/kbingham/libcamera/pulls/26/commits/54a7a8c7f674f281fd5bf59b7e0192e3e0cb1abd\nhas:\n\n \t\t/*\n\t\t * Todo: Determine the minimum allowed thresholds from the mean\n\t\t * but we currently have the sum - not the mean value!\n\t\t */\n\t\tSimpleAwbStats awbStats{ rgbMeans };\n\nI'm not really sure what I meant - because ... we have the mean ? don't\nwe ?\n\nMaybe we drop the comment/todo/\n\n\n\n> > To be honest I don't know how the soft ISP calculate statistics and I\n> > didn't write the \\todo, but if I just look at struct SwIspStats\n> > definition:\n> >\n> > /**\n> > * \\brief Sums of colour channels of all the sampled pixels\n> > */\n> > RGB sum_;\n> >\n> > I presume to generate stats the image is divided into grids, the mean\n> > value of each colour channel is calculated per zone, the averaged and\n> > reported in sums_.\n> \n> There are no real zones, the image may be split by lines to areas\n> processed separately by different threads (if enabled), purely for\n> speedup. But there is no inherent reason why zones couldn't be used in\n> future, especially in GPU ISP.\n> \n> > So we don't have a per-window mean value which we can compared with\n> > SimpleAwbStats::minColourValue() to discard zones with not enough\n> > colour information, but we only have the sum when we get here.\n> >\n> > I guess the comment means we can't really discard zones by filtering\n> > on the mean colour value.\n> \n> I see, thank you for clarification. Since there are currently no zones,\n> SimpleAwbStats::minColourValue() mentioned below doesn't exist, and the\n> \\todo is especially confusing in the context of the (whole image) RGB\n> means computed just above it, I'd suggest dropping the whole comment.\n> If anybody thinks it's important to retain it, a clearer wording should\n> be suggested.\n\nMaybe drop it if it's not adding value. I won't be offended :-) I\npresume I wrote it while reading something from a different IPA that I\nwas interpreting while trying to shoe-horn this in here ?\n\n--\nKieran","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 95317C3318\n\tfor ;\n\tFri, 10 Jul 2026 15:58:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68F7966113;\n\tFri, 10 Jul 2026 17:58:06 +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 6808265F7C\n\tfor ;\n\tFri, 10 Jul 2026 17:58:04 +0200 (CEST)","from monstersaurus.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 D943D12F;\n\tFri, 10 Jul 2026 17:57:12 +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=\"oLQ2MIOf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1783699032;\n\tbh=CJfKsh1kQOqSzX6EDCVc/6ABUZZnKV+DpXUIjEjVxWA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=oLQ2MIOfovXcPG4arbhjYjPJduHxeB28hxjSw3ldl959Kk2vOea2gGafEXMKnab1a\n\t5Z4iX/9Kwpk3B0YQWXRJPAw6NM9uBi4D8MKT+NwjD6KuW1HAUuxYDvdsqGB7Ztn6uW\n\tCea70b25WJT1JbNbf4nfSIzBk1X3sQffdskSSLYQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<85zezy50r1.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","References":"<20260708-libipa-algorithms-v5-0-0759d0359f52@ideasonboard.com>\n\t<20260708-libipa-algorithms-v5-3-0759d0359f52@ideasonboard.com>\n\t<85ldbj5h85.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t\n\t<85zezy50r1.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Subject":"Re: [PATCH v5 03/36] ipa: simple: awb: Port to use libipa\n\tAwbAlgorithm","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tRobert Mader ","To":"Jacopo Mondi ,\n\tMilan Zamazal ","Date":"Fri, 10 Jul 2026 16:58:01 +0100","Message-ID":"<178369908133.727359.245546119031834639@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]