{"id":23584,"url":"https://patchwork.libcamera.org/api/1.1/patches/23584/?format=json","web_url":"https://patchwork.libcamera.org/patch/23584/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20250617082956.5699-2-david.plowman@raspberrypi.com>","date":"2025-06-17T08:29:49","name":"[1/7] ipa: rpi: agc: Change handling of colour gains less than 1","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"71d763375efcdbfdd14f83434ad32fa527bc4079","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/1.1/people/42/?format=json","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/23584/mbox/","series":[{"id":5225,"url":"https://patchwork.libcamera.org/api/1.1/series/5225/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=5225","date":"2025-06-17T08:29:48","name":"Raspberry Pi AEC/AGC update","version":1,"mbox":"https://patchwork.libcamera.org/series/5225/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/23584/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/23584/checks/","tags":{},"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 9151EBDE6B\n\tfor ;\n\tTue, 17 Jun 2025 08:30:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E02E68DD2;\n\tTue, 17 Jun 2025 10:30:05 +0200 (CEST)","from mail-wm1-x333.google.com (mail-wm1-x333.google.com\n\t[IPv6:2a00:1450:4864:20::333])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C00168DB4\n\tfor ;\n\tTue, 17 Jun 2025 10:30:02 +0200 (CEST)","by mail-wm1-x333.google.com with SMTP id\n\t5b1f17b1804b1-451e2f0d9c2so4698825e9.1\n\tfor ;\n\tTue, 17 Jun 2025 01:30:02 -0700 (PDT)","from raspberrypi.pitowers.org\n\t([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.00\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 17 Jun 2025 01:30:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"A6HjDcxG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1750149001; x=1750753801;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:mime-version:references:in-reply-to\n\t:message-id:date:subject:cc:to:from:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=YX87qigBl9t1AU9Ees2mQoY/o3WFqTSoOY5Nua1k+Lo=;\n\tb=A6HjDcxGiMOTyyB+SQMImepGUcxk9VE7qQpnqNE7rHpODLlxVyqiBpez0uaPpuvsIg\n\t0f7vBc+1oDjVFIpmMc/7WAT/aPwx39tsSIcWv2agmvzJwbvDizjp2z/ciJbxZWYCyMpU\n\txUEcrxHbcQBHiuZ3s6lU0BvRxUG+stGVSaWQZl4WoJuFky1scChEkf0l71yqyGuYtlcf\n\tzx7RSg2LjmoPP8nP5UcD9s2VP8+QX6lx/NF+1XgDH7ri3QY6Zi9hSIVIB5nLm19IeQH6\n\tB+x181of4t2q207mbPPBwhduz5MCZw8NHwa2yR5zE6VrK/4+Pc62f33Y3d2fxqHN/kAl\n\tVwPQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1750149001; x=1750753801;\n\th=content-transfer-encoding:mime-version:references:in-reply-to\n\t:message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=YX87qigBl9t1AU9Ees2mQoY/o3WFqTSoOY5Nua1k+Lo=;\n\tb=rS9gcDKhlDv5L1OctinwseWhIVpod2ETzxPc6IGeM0h1h66o/nf4sRhL48Z9OvqmUK\n\tgwpqI7RuSGVrwHvWVBMGeD0zea3SawhjsQYyey8+m8+vEPiN8UCGM0M7vitHHA5ufUDG\n\thH/FWcV6bS5TBdY+SHvOCzVIGQo5HzB/XD1NgOi+idcafaKOIBc9v8N6AK3icxqtUKBP\n\t8Hv3rHCd7Hks21WhAQJXwt1L8PmYlRC1z70R1xw48tDNkp1dZz+cXQjmQyvAHdeIuhZJ\n\tuxmcTCNXWRntOD+v5oEJ8UAncd5ii8wbQDm/tdeZYj9gUVBmsrxyKt6vCRzSr1HziGOO\n\ts1og==","X-Gm-Message-State":"AOJu0YzLDs2dk7oPybtSZQQq2DHlPExPUQLzYubNvOxfNyMad54Svsgb\n\tVg7u9N3wDBoLWOqF86SXHippyqZfsTdVqeSQXiqR+vXBeY1eqcvkpBZdYbB065hzLvuGzZbSOYI\n\tUBRt0","X-Gm-Gg":"ASbGnctugxpTNAOoHg8LwUgFjm5jDju81I7gXC7C+JZWBw0nU9zZ1CPKSuSVz7mHaET\n\tSR+lJyBewtMi130JCN3DS9kqR6yHX00wcEihwqfOULrJ5gHvnlHMlG38WvtA85Pj97CXdLhUUrz\n\t+IkT60Ckmd3OOHWAF3w7NvUWTLLJ94jjWgtUwXvieO23+Zi+dxntYMWWu038wkdyODFGZ0mvx3V\n\titONAF1L0nnpUIDFfaHFflMK3JtGjz8qyK++nVPdeP8pCSd9vxw1Kz+oTko/3wGVyMZXNWW6Q5y\n\tDm+8Fuc37OiUqm9hEIr9QlxcuoXaXrBCXAd+yon493rIOEhvrDcuyfORZYHo2RYnCagc30phtwS\n\tn5B4S2GIdSwFov3qzxA==","X-Google-Smtp-Source":"AGHT+IFl0870O7TU4HjOJPCG6wKq5uqNDWzeJa757q0mtwBdAe3XWCFxZc5UyMSKxzb1CMQrHcbOJQ==","X-Received":"by 2002:a05:600c:314c:b0:442:f4a3:a2c0 with SMTP id\n\t5b1f17b1804b1-4533c95bc97mr128678745e9.13.1750149001005; \n\tTue, 17 Jun 2025 01:30:01 -0700 (PDT)","From":"David Plowman ","To":"libcamera-devel@lists.libcamera.org","Cc":"David Plowman ","Subject":"[PATCH 1/7] ipa: rpi: agc: Change handling of colour gains less\n\tthan 1","Date":"Tue, 17 Jun 2025 09:29:49 +0100","Message-Id":"<20250617082956.5699-2-david.plowman@raspberrypi.com>","X-Mailer":"git-send-email 2.39.5","In-Reply-To":"<20250617082956.5699-1-david.plowman@raspberrypi.com>","References":"<20250617082956.5699-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","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\" "},"content":"Previously these were handled in the AGC/AEC exposure update\ncalculations by explicitly driving a higher digital gain to \"cancel\nout\" any colour gains that were less than 1.\n\nNow we're ignoring this in the AGC and leaving it to the IPA code to\nnormalise all the gains so that the smallest is 1. We don't regard\nthis as a \"real\" increase because one of the colour channels (just not\nnecessarily the green one) still gets the minimum gain possible.\n\nWe do, however, update the statistics calculations so that they\nreflect any such digital gain increase, so that images are driven to\nthe correct level.\n\nSigned-off-by: David Plowman \n---\n src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++---------------\n src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++----\n src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++---\n 3 files changed, 54 insertions(+), 39 deletions(-)","diff":"diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\nindex a5562760..a87dc194 100644\n--- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n+++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n@@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode,\n \n \tDuration fixedExposureTime = limitExposureTime(fixedExposureTime_);\n \tif (fixedExposureTime && fixedAnalogueGain_) {\n-\t\t/* We're going to reset the algorithm here with these fixed values. */\n-\t\tfetchAwbStatus(metadata);\n-\t\tdouble minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n-\t\tASSERT(minColourGain != 0.0);\n-\n \t\t/* This is the equivalent of computeTargetExposure and applyDigitalGain. */\n \t\ttarget_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_;\n-\t\ttarget_.totalExposure = target_.totalExposureNoDG / minColourGain;\n+\t\ttarget_.totalExposure = target_.totalExposureNoDG;\n \n \t\t/* Equivalent of filterExposure. This resets any \"history\". */\n \t\tfiltered_ = target_;\n@@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode,\n \t\t */\n \n \t\tdouble ratio = lastSensitivity / cameraMode.sensitivity;\n-\t\ttarget_.totalExposureNoDG *= ratio;\n \t\ttarget_.totalExposure *= ratio;\n-\t\tfiltered_.totalExposureNoDG *= ratio;\n+\t\ttarget_.totalExposureNoDG = target_.totalExposure;\n \t\tfiltered_.totalExposure *= ratio;\n+\t\tfiltered_.totalExposureNoDG = filtered_.totalExposure;\n \n \t\tdivideUpExposure();\n \t} else {\n@@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n \t}\n \n \t/* Factor in the AWB correction if needed. */\n-\tif (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb)\n-\t\tsum *= RGB{ { awb.gainR, awb.gainR, awb.gainB } };\n+\tif (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {\n+\t\tdouble minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 });\n+\t\tminColourGain = std::max(minColourGain, 1.0);\n+\t\tRGB colourGains{ { awb.gainR, awb.gainR, awb.gainB } };\n+\t\tcolourGains /= minColourGain;\n+\t\tsum *= colourGains;\n+\t}\n \n \tdouble ySum = ipa::rec601LuminanceFromRGB(sum);\n \n@@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n void AgcChannel::computeTargetExposure(double gain)\n {\n \tif (status_.fixedExposureTime && status_.fixedAnalogueGain) {\n-\t\t/*\n-\t\t * When analogue gain and exposure time are both fixed, we need\n-\t\t * to drive the total exposure so that we end up with a digital\n-\t\t * gain of at least 1/minColourGain. Otherwise we'd desaturate\n-\t\t * channels causing white to go cyan or magenta.\n-\t\t */\n-\t\tdouble minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n-\t\tASSERT(minColourGain != 0.0);\n \t\ttarget_.totalExposure =\n-\t\t\tstatus_.fixedExposureTime * status_.fixedAnalogueGain / minColourGain;\n+\t\t\tstatus_.fixedExposureTime * status_.fixedAnalogueGain;\n \t} else {\n \t\t/*\n \t\t * The statistics reflect the image without digital gain, so the final\n@@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel\n \n bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound)\n {\n-\tdouble minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n-\tASSERT(minColourGain != 0.0);\n-\tdouble dg = 1.0 / minColourGain;\n-\t/*\n-\t * I think this pipeline subtracts black level and rescales before we\n-\t * get the stats, so no need to worry about it.\n-\t */\n-\tLOG(RPiAgc, Debug) << \"after AWB, target dg \" << dg << \" gain \" << gain\n-\t\t\t << \" target_Y \" << targetY;\n+\tdouble dg = 1.0;\n+\n \t/*\n \t * Finally, if we're trying to reduce exposure but the target_Y is\n \t * \"close\" to 1.0, then the gain computed for that constraint will be\ndiff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp\nindex bb50a9e0..e1a804f5 100644\n--- a/src/ipa/rpi/pisp/pisp.cpp\n+++ b/src/ipa/rpi/pisp/pisp.cpp\n@@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr\n \tpisp_wbg_config wbg;\n \tpisp_fe_rgby_config rgby = {};\n \tdouble dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0;\n+\tdouble minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 });\n+\t/* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */\n+\tdouble extraGain = 1.0 / std::max({ minColourGain, 0.1 });\n \n-\twbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10);\n-\twbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10);\n-\twbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10);\n+\t/*\n+\t * Apply an extra gain of 1 / minColourGain so as not to apply < 1 gains to any\n+\t * channels (which would cause saturated pixels to go cyan or magenta).\n+\t * Doing this doesn't really apply more gain than necessary, because one of the\n+\t * channels is always getting the minimum gain possible. For this reason we also\n+\t * don't change the values that we report externally.\n+\t */\n+\tdouble gainR = awbStatus->gainR * extraGain;\n+\tdouble gainG = awbStatus->gainG * extraGain;\n+\tdouble gainB = awbStatus->gainB * extraGain;\n+\n+\twbg.gain_r = clampField(dg * gainR, 14, 10);\n+\twbg.gain_g = clampField(dg * gainG, 14, 10);\n+\twbg.gain_b = clampField(dg * gainB, 14, 10);\n \n \t/*\n \t * The YCbCr conversion block should contain the appropriate YCbCr\n@@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr\n \tbe_->GetYcbcr(csc);\n \n \t/* The CSC coefficients already have the << 10 scaling applied. */\n-\trgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14);\n-\trgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14);\n-\trgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14);\n+\trgby.gain_r = clampField(csc.coeffs[0] * gainR, 14);\n+\trgby.gain_g = clampField(csc.coeffs[1] * gainG, 14);\n+\trgby.gain_b = clampField(csc.coeffs[2] * gainB, 14);\n \n \tLOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n \t\t\t << awbStatus->gainB;\ndiff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\nindex ba43e474..8a7a37c8 100644\n--- a/src/ipa/rpi/vc4/vc4.cpp\n+++ b/src/ipa/rpi/vc4/vc4.cpp\n@@ -63,7 +63,8 @@ private:\n \tbool validateIspControls();\n \n \tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n-\tvoid applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls);\n+\tvoid applyDG(const struct AgcPrepareStatus *dgStatus,\n+\t\t const struct AwbStatus *awbStatus, ControlList &ctrls);\n \tvoid applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);\n \tvoid applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n \tvoid applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n@@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n \t\tapplyCCM(ccmStatus, ctrls);\n \n \tAgcPrepareStatus *dgStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n-\tif (dgStatus)\n-\t\tapplyDG(dgStatus, ctrls);\n+\tapplyDG(dgStatus, awbStatus, ctrls);\n \n \tAlscStatus *lsStatus = rpiMetadata.getLocked(\"alsc.status\");\n \tif (lsStatus)\n@@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n \t\t static_cast(awbStatus->gainB * 1000));\n }\n \n-void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls)\n+void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus,\n+\t\t const struct AwbStatus *awbStatus, ControlList &ctrls)\n {\n+\tdouble digitalGain = dgStatus ? dgStatus->digitalGain : 1.0;\n+\n+\tif (awbStatus) {\n+\t\t/*\n+\t\t * We must apply sufficient extra digital gain to stop any of the channel gains being\n+\t\t * less than 1, which would cause saturation artifacts. Note that one of the colour\n+\t\t * channels is still getting the minimum possible gain, so it's not a \"real\" gain\n+\t\t * increase.\n+\t\t */\n+\t\tdouble minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 });\n+\t\t/* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */\n+\t\tdouble extraGain = 1.0 / std::max({ minColourGain, 0.1 });\n+\t\tdigitalGain *= extraGain;\n+\t}\n+\n \tctrls.set(V4L2_CID_DIGITAL_GAIN,\n-\t\t static_cast(dgStatus->digitalGain * 1000));\n+\t\t static_cast(digitalGain * 1000));\n }\n \n void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n","prefixes":["1/7"]}