From patchwork Tue Jun 17 08:29:49 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23584 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 9151EBDE6B for ; Tue, 17 Jun 2025 08:30:10 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 4E02E68DD2; Tue, 17 Jun 2025 10:30:05 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="A6HjDcxG"; dkim-atps=neutral Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C00168DB4 for ; Tue, 17 Jun 2025 10:30:02 +0200 (CEST) Received: by mail-wm1-x333.google.com with SMTP id 5b1f17b1804b1-451e2f0d9c2so4698825e9.1 for ; Tue, 17 Jun 2025 01:30:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149001; x=1750753801; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=YX87qigBl9t1AU9Ees2mQoY/o3WFqTSoOY5Nua1k+Lo=; b=A6HjDcxGiMOTyyB+SQMImepGUcxk9VE7qQpnqNE7rHpODLlxVyqiBpez0uaPpuvsIg 0f7vBc+1oDjVFIpmMc/7WAT/aPwx39tsSIcWv2agmvzJwbvDizjp2z/ciJbxZWYCyMpU xUEcrxHbcQBHiuZ3s6lU0BvRxUG+stGVSaWQZl4WoJuFky1scChEkf0l71yqyGuYtlcf zx7RSg2LjmoPP8nP5UcD9s2VP8+QX6lx/NF+1XgDH7ri3QY6Zi9hSIVIB5nLm19IeQH6 B+x181of4t2q207mbPPBwhduz5MCZw8NHwa2yR5zE6VrK/4+Pc62f33Y3d2fxqHN/kAl VwPQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149001; x=1750753801; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=YX87qigBl9t1AU9Ees2mQoY/o3WFqTSoOY5Nua1k+Lo=; b=rS9gcDKhlDv5L1OctinwseWhIVpod2ETzxPc6IGeM0h1h66o/nf4sRhL48Z9OvqmUK gwpqI7RuSGVrwHvWVBMGeD0zea3SawhjsQYyey8+m8+vEPiN8UCGM0M7vitHHA5ufUDG hH/FWcV6bS5TBdY+SHvOCzVIGQo5HzB/XD1NgOi+idcafaKOIBc9v8N6AK3icxqtUKBP 8Hv3rHCd7Hks21WhAQJXwt1L8PmYlRC1z70R1xw48tDNkp1dZz+cXQjmQyvAHdeIuhZJ uxmcTCNXWRntOD+v5oEJ8UAncd5ii8wbQDm/tdeZYj9gUVBmsrxyKt6vCRzSr1HziGOO s1og== X-Gm-Message-State: AOJu0YzLDs2dk7oPybtSZQQq2DHlPExPUQLzYubNvOxfNyMad54Svsgb Vg7u9N3wDBoLWOqF86SXHippyqZfsTdVqeSQXiqR+vXBeY1eqcvkpBZdYbB065hzLvuGzZbSOYI UBRt0 X-Gm-Gg: ASbGnctugxpTNAOoHg8LwUgFjm5jDju81I7gXC7C+JZWBw0nU9zZ1CPKSuSVz7mHaET SR+lJyBewtMi130JCN3DS9kqR6yHX00wcEihwqfOULrJ5gHvnlHMlG38WvtA85Pj97CXdLhUUrz +IkT60Ckmd3OOHWAF3w7NvUWTLLJ94jjWgtUwXvieO23+Zi+dxntYMWWu038wkdyODFGZ0mvx3V itONAF1L0nnpUIDFfaHFflMK3JtGjz8qyK++nVPdeP8pCSd9vxw1Kz+oTko/3wGVyMZXNWW6Q5y Dm+8Fuc37OiUqm9hEIr9QlxcuoXaXrBCXAd+yon493rIOEhvrDcuyfORZYHo2RYnCagc30phtwS n5B4S2GIdSwFov3qzxA== X-Google-Smtp-Source: AGHT+IFl0870O7TU4HjOJPCG6wKq5uqNDWzeJa757q0mtwBdAe3XWCFxZc5UyMSKxzb1CMQrHcbOJQ== X-Received: by 2002:a05:600c:314c:b0:442:f4a3:a2c0 with SMTP id 5b1f17b1804b1-4533c95bc97mr128678745e9.13.1750149001005; Tue, 17 Jun 2025 01:30:01 -0700 (PDT) Received: from raspberrypi.pitowers.org ([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:00 -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 than 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 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Previously these were handled in the AGC/AEC exposure update calculations by explicitly driving a higher digital gain to "cancel out" any colour gains that were less than 1. Now we're ignoring this in the AGC and leaving it to the IPA code to normalise all the gains so that the smallest is 1. We don't regard this as a "real" increase because one of the colour channels (just not necessarily the green one) still gets the minimum gain possible. We do, however, update the statistics calculations so that they reflect any such digital gain increase, so that images are driven to the correct level. Signed-off-by: David Plowman --- src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++--------------- src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++---- src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++--- 3 files changed, 54 insertions(+), 39 deletions(-) diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index a5562760..a87dc194 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); if (fixedExposureTime && fixedAnalogueGain_) { - /* We're going to reset the algorithm here with these fixed values. */ - fetchAwbStatus(metadata); - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); - /* This is the equivalent of computeTargetExposure and applyDigitalGain. */ target_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_; - target_.totalExposure = target_.totalExposureNoDG / minColourGain; + target_.totalExposure = target_.totalExposureNoDG; /* Equivalent of filterExposure. This resets any "history". */ filtered_ = target_; @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, */ double ratio = lastSensitivity / cameraMode.sensitivity; - target_.totalExposureNoDG *= ratio; target_.totalExposure *= ratio; - filtered_.totalExposureNoDG *= ratio; + target_.totalExposureNoDG = target_.totalExposure; filtered_.totalExposure *= ratio; + filtered_.totalExposureNoDG = filtered_.totalExposure; divideUpExposure(); } else { @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, } /* Factor in the AWB correction if needed. */ - if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) - sum *= RGB{ { awb.gainR, awb.gainR, awb.gainB } }; + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) { + double minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 }); + minColourGain = std::max(minColourGain, 1.0); + RGB colourGains{ { awb.gainR, awb.gainR, awb.gainB } }; + colourGains /= minColourGain; + sum *= colourGains; + } double ySum = ipa::rec601LuminanceFromRGB(sum); @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, void AgcChannel::computeTargetExposure(double gain) { if (status_.fixedExposureTime && status_.fixedAnalogueGain) { - /* - * When analogue gain and exposure time are both fixed, we need - * to drive the total exposure so that we end up with a digital - * gain of at least 1/minColourGain. Otherwise we'd desaturate - * channels causing white to go cyan or magenta. - */ - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); target_.totalExposure = - status_.fixedExposureTime * status_.fixedAnalogueGain / minColourGain; + status_.fixedExposureTime * status_.fixedAnalogueGain; } else { /* * The statistics reflect the image without digital gain, so the final @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound) { - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); - double dg = 1.0 / minColourGain; - /* - * I think this pipeline subtracts black level and rescales before we - * get the stats, so no need to worry about it. - */ - LOG(RPiAgc, Debug) << "after AWB, target dg " << dg << " gain " << gain - << " target_Y " << targetY; + double dg = 1.0; + /* * Finally, if we're trying to reduce exposure but the target_Y is * "close" to 1.0, then the gain computed for that constraint will be diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp index bb50a9e0..e1a804f5 100644 --- a/src/ipa/rpi/pisp/pisp.cpp +++ b/src/ipa/rpi/pisp/pisp.cpp @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr pisp_wbg_config wbg; pisp_fe_rgby_config rgby = {}; double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0; + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); - wbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10); - wbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10); - wbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10); + /* + * Apply an extra gain of 1 / minColourGain so as not to apply < 1 gains to any + * channels (which would cause saturated pixels to go cyan or magenta). + * Doing this doesn't really apply more gain than necessary, because one of the + * channels is always getting the minimum gain possible. For this reason we also + * don't change the values that we report externally. + */ + double gainR = awbStatus->gainR * extraGain; + double gainG = awbStatus->gainG * extraGain; + double gainB = awbStatus->gainB * extraGain; + + wbg.gain_r = clampField(dg * gainR, 14, 10); + wbg.gain_g = clampField(dg * gainG, 14, 10); + wbg.gain_b = clampField(dg * gainB, 14, 10); /* * The YCbCr conversion block should contain the appropriate YCbCr @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr be_->GetYcbcr(csc); /* The CSC coefficients already have the << 10 scaling applied. */ - rgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14); - rgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14); - rgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14); + rgby.gain_r = clampField(csc.coeffs[0] * gainR, 14); + rgby.gain_g = clampField(csc.coeffs[1] * gainG, 14); + rgby.gain_b = clampField(csc.coeffs[2] * gainB, 14); LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: " << awbStatus->gainB; diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp index ba43e474..8a7a37c8 100644 --- a/src/ipa/rpi/vc4/vc4.cpp +++ b/src/ipa/rpi/vc4/vc4.cpp @@ -63,7 +63,8 @@ private: bool validateIspControls(); void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); - void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls); + void applyDG(const struct AgcPrepareStatus *dgStatus, + const struct AwbStatus *awbStatus, ControlList &ctrls); void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, applyCCM(ccmStatus, ctrls); AgcPrepareStatus *dgStatus = rpiMetadata.getLocked("agc.prepare_status"); - if (dgStatus) - applyDG(dgStatus, ctrls); + applyDG(dgStatus, awbStatus, ctrls); AlscStatus *lsStatus = rpiMetadata.getLocked("alsc.status"); if (lsStatus) @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) static_cast(awbStatus->gainB * 1000)); } -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls) +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, + const struct AwbStatus *awbStatus, ControlList &ctrls) { + double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0; + + if (awbStatus) { + /* + * We must apply sufficient extra digital gain to stop any of the channel gains being + * less than 1, which would cause saturation artifacts. Note that one of the colour + * channels is still getting the minimum possible gain, so it's not a "real" gain + * increase. + */ + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); + digitalGain *= extraGain; + } + ctrls.set(V4L2_CID_DIGITAL_GAIN, - static_cast(dgStatus->digitalGain * 1000)); + static_cast(digitalGain * 1000)); } void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)