From patchwork Tue Jun 17 08:29:53 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23588 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 69DCABDE6B for ; Tue, 17 Jun 2025 08:30:18 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 7801968DDA; Tue, 17 Jun 2025 10:30:13 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="pMZU0uiu"; dkim-atps=neutral Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id BE94E68DD4 for ; Tue, 17 Jun 2025 10:30:05 +0200 (CEST) Received: by mail-wm1-x32a.google.com with SMTP id 5b1f17b1804b1-451d41e1ad1so46129455e9.1 for ; Tue, 17 Jun 2025 01:30:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149005; x=1750753805; 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=aYyKjOsdJLwWOcff0kElCz9r2+aJ89+xtpQUrkR4tfQ=; b=pMZU0uiuP8B4j9aPmVswXOxBxsrcYkGxvpY1kN87+pa2hbi9zT+F5JUulHFJ9bgvns crL3438fB7L8FfOvktxt7ldIxq2NiKNzvgTQUxkoMxjQJZ2F2Vf1IbatUxJfNb6eRhOy //B1T5Ol/NjmHbAuayFnBTwSRcutLPwtwqsY5VfqAwG9EasYEwJ8ZJqE05K0chHcqv3i +xWvYaHiZoL47/eygcyTOcuUShILLdxPMJ0io3a8NIYMu6uwwM3QX/4LYSI+mdRne73Z Y3otyyNcmzEuSdp5eZGDV0VUdiUtDmQlvcNXBVeWrodSQLZxNyS+FLvEcrLl7oIC79In RpZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149005; x=1750753805; 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=aYyKjOsdJLwWOcff0kElCz9r2+aJ89+xtpQUrkR4tfQ=; b=aMwvCRBU6cyS9ln7u/KZdkHXMsVTc5V0fYqdZkgUiydx5V4UA7XlfSGpQVb/vYzF4m BWu5R+ijhTswVJEyghdqSsmJVIU+W51EmCR1Nf+dgjwwzPZc37beJT4L5zRRYzeTmcq7 K+fUY9Aabj0omNCsnkt7Du5+cvykiEefCfoGAhQ4AFARFhK2Y68VtBW36B5rSGnyaXox ijoESv5iIVOKD0yJELU+DLGpFUWG9+vPzTA7WgpfSPif3jKioLcEIVcSrsgpGiR3IAFd aj2kBAELF0SVJ+L2iT0l0f3DwH7GhQiXtqtLoCCNMwsth5tEunZplAfDBcH4iuG0xShc CHoQ== X-Gm-Message-State: AOJu0YyVIC7L2gdSrkPyngCVMzN4oXodEofDrznyshPS7ORqhoOtGqud tF9Hkg1QIPWMih78fbx31NojJTP5auK/w0tERQ6CYUfkubiz2jFM33t/M0oqeihynhibEv/lyqB puA+f X-Gm-Gg: ASbGncsnDxGj/T4uMKHuBwskKOLHH2Azzw81XE86G8dnzP/u60BcTsvFmvfVfFUmPAD 2ge0czcVYsUzD9out5c/RHoSoyCt0EubhZbbQZcKa/r1Mq136bnkHXvqDDM5hqX/01yQLrqVG9f FZOlyFc0YZ/shxi7XEvUSERYt4i1Rjz8YhmNCv9UgCZ23wAYYs6KqjbePWFiiWhw/jRYtz7DcUr 3q1p9p1RPJIEBB2somKtDy8F0ArrBfCKVeC6fJa4Pf60WdZfkt052MHS6TjNOqIQfIYL1Li7z4p CdrlTgm0yYG7ZoHKt2mkKW1hjL7VuMgVckSATnyVkkjnEpUbhZbAgOh2Eny9esGGhY5tQu62Wmw IbRBnB7MJUZhW/LyApetTMXHkhtsJ X-Google-Smtp-Source: AGHT+IGvh/cZvHSWOsx54Fnh2Mk5IyMt4pToui5MLbb+fhDXDraeFy+FqWzwkVBje/pTDpKRkoaGUg== X-Received: by 2002:a05:600c:1c0a:b0:453:b44:eb71 with SMTP id 5b1f17b1804b1-4533caa2114mr110052405e9.19.1750149004779; Tue, 17 Jun 2025 01:30:04 -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.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:03 -0700 (PDT) From: David Plowman To: libcamera-devel@lists.libcamera.org Cc: David Plowman Subject: [PATCH 5/7] ipa: rpi: agc: Calculate digital gain in process() Date: Tue, 17 Jun 2025 09:29:53 +0100 Message-Id: <20250617082956.5699-6-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 we let prepare() do the work by comparing the desired total exposure against the shutter time and analogue gain. This can cause the image to "wink" at high framerates because we may skip running prepare() to get the new digital gain even when the delayed AGC status (which came out of an earlier call to process()) shows that a change was required. Now we're taking explicit control of the digital gain by calculating it ourselves so that we can output it in the standard AgcStatus object. This means that whenever the delayed AGC status changes, we have the correct digital gain to go with it. Signed-off-by: David Plowman --- src/ipa/rpi/controller/agc_status.h | 1 + src/ipa/rpi/controller/rpi/agc_channel.cpp | 163 +++++++++------------ src/ipa/rpi/controller/rpi/agc_channel.h | 1 + 3 files changed, 75 insertions(+), 90 deletions(-) diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h index d4cedcf4..956d6abf 100644 --- a/src/ipa/rpi/controller/agc_status.h +++ b/src/ipa/rpi/controller/agc_status.h @@ -30,6 +30,7 @@ struct AgcStatus { libcamera::utils::Duration targetExposureValue; /* (unfiltered) target total exposure AGC is aiming for */ libcamera::utils::Duration exposureTime; double analogueGain; + double digitalGain; std::string exposureMode; std::string constraintMode; std::string meteringMode; diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index 1028d82e..3829534c 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -266,7 +266,7 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms) } AgcChannel::ExposureValues::ExposureValues() - : exposureTime(0s), analogueGain(0), + : exposureTime(0s), analogueGain(0), digitalGain(0), totalExposure(0s), totalExposureNoDG(0s) { } @@ -434,17 +434,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, mode_ = cameraMode; Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); + double fixedGain = limitGain(fixedGain_); if (fixedExposureTime && fixedGain_) { - /* This is the equivalent of computeTargetExposure and applyDigitalGain. */ - target_.totalExposureNoDG = fixedExposureTime_ * fixedGain_; - target_.totalExposure = target_.totalExposureNoDG; - - /* Equivalent of filterExposure. This resets any "history". */ - filtered_ = target_; - - /* Equivalent of divideUpExposure. */ - filtered_.exposureTime = fixedExposureTime; - filtered_.analogueGain = fixedGain_; + filtered_.totalExposureNoDG = fixedExposureTime * fixedGain; + filtered_.totalExposure = filtered_.totalExposureNoDG; } else if (status_.totalExposureValue) { /* * On a mode switch, various things could happen: @@ -457,12 +450,8 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, */ double ratio = lastSensitivity / cameraMode.sensitivity; - target_.totalExposure *= ratio; - target_.totalExposureNoDG = target_.totalExposure; filtered_.totalExposure *= ratio; filtered_.totalExposureNoDG = filtered_.totalExposure; - - divideUpExposure(); } else { /* * We come through here on startup, when at least one of the @@ -472,55 +461,46 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, * weren't set. */ - /* Equivalent of divideUpExposure. */ - filtered_.exposureTime = fixedExposureTime ? fixedExposureTime : config_.defaultExposureTime; - filtered_.analogueGain = fixedGain_ ? fixedGain_ : config_.defaultAnalogueGain; + Duration exposureTime = fixedExposureTime ? fixedExposureTime : config_.defaultExposureTime; + double gain = fixedGain ? fixedGain : config_.defaultAnalogueGain; + filtered_.totalExposure = exposureTime * gain; + filtered_.totalExposureNoDG = filtered_.totalExposure; } + /* Setting target_ to filtered_ removes any history from before the mode switch. */ + target_ = filtered_; + divideUpExposure(); + writeAndFinish(metadata, false); } void AgcChannel::prepare(Metadata *imageMetadata) { - Duration totalExposureValue = status_.totalExposureValue; - AgcStatus delayedStatus; + DeviceStatus deviceStatus; AgcPrepareStatus prepareStatus; - /* Fetch the AWB status now because AWB also sets it in the prepare method. */ - fetchAwbStatus(imageMetadata); - - if (!imageMetadata->get("agc.delayed_status", delayedStatus)) - totalExposureValue = delayedStatus.totalExposureValue; - - prepareStatus.digitalGain = 1.0; prepareStatus.locked = false; + prepareStatus.digitalGain = 1.0; - if (status_.totalExposureValue) { - /* Process has run, so we have meaningful values. */ - DeviceStatus deviceStatus; - if (imageMetadata->get("device.status", deviceStatus) == 0) { - Duration actualExposure = deviceStatus.exposureTime * - deviceStatus.analogueGain; - if (actualExposure) { - double digitalGain = totalExposureValue / actualExposure; - LOG(RPiAgc, Debug) << "Want total exposure " << totalExposureValue; - /* - * Never ask for a gain < 1.0, and also impose - * some upper limit. Make it customisable? - */ - prepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, - config_.maxDigitalGain)); - LOG(RPiAgc, Debug) << "Actual exposure " << actualExposure; - LOG(RPiAgc, Debug) << "Use digitalGain " << prepareStatus.digitalGain; - LOG(RPiAgc, Debug) << "Effective exposure " - << actualExposure * prepareStatus.digitalGain; - /* Decide whether AEC/AGC has converged. */ - prepareStatus.locked = updateLockStatus(deviceStatus); - } - } else - LOG(RPiAgc, Warning) << "AgcChannel: no device metadata"; - imageMetadata->set("agc.prepare_status", prepareStatus); + if (!imageMetadata->get("device.status", deviceStatus)) { + prepareStatus.locked = updateLockStatus(deviceStatus); + + /* + * For now, the IPA code is still expecting the digital gain to come back in + * the prepare_status. To keep things happy, we'll just fill in the value that + * we calculated previously and put in the AgcStatus (which comes back as the + * "delayed" status). Once the rest of the IPA code is updated, we'll be able + * to remove this, and indeed remove the digitalGain from the AgcPrepareStatus. + */ + AgcStatus delayedStatus; + if (!imageMetadata->get("agc.delayed_status", delayedStatus)) + prepareStatus.digitalGain = delayedStatus.digitalGain; + else + /* After a mode switch, this must be correct until new values come through. */ + prepareStatus.digitalGain = status_.digitalGain; } + + imageMetadata->set("agc.prepare_status", prepareStatus); } void AgcChannel::process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, @@ -606,7 +586,7 @@ void AgcChannel::housekeepConfig() /* First fetch all the up-to-date settings, so no one else has to do it. */ status_.ev = ev_; status_.fixedExposureTime = limitExposureTime(fixedExposureTime_); - status_.fixedGain = fixedGain_; + status_.fixedGain = limitGain(fixedGain_); status_.flickerPeriod = flickerPeriod_; LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixedExposureTime " << status_.fixedExposureTime << " fixedGain " @@ -657,6 +637,9 @@ void AgcChannel::fetchCurrentExposure(DeviceStatus const &deviceStatus) current_.analogueGain = deviceStatus.analogueGain; current_.totalExposure = 0s; /* this value is unused */ current_.totalExposureNoDG = current_.exposureTime * current_.analogueGain; + LOG(RPiAgc, Debug) << "Current frame: exposure time " << current_.exposureTime + << " ag " << current_.analogueGain + << " (total " << current_.totalExposureNoDG << ")"; } void AgcChannel::fetchAwbStatus(Metadata *imageMetadata) @@ -808,14 +791,12 @@ void AgcChannel::computeTargetExposure(double gain) target_.totalExposure = current_.totalExposureNoDG * gain; /* The final target exposure is also limited to what the exposure mode allows. */ Duration maxExposureTime = status_.fixedExposureTime - ? status_.fixedExposureTime - : exposureMode_->exposureTime.back(); + ? status_.fixedExposureTime + : exposureMode_->exposureTime.back(); maxExposureTime = limitExposureTime(maxExposureTime); - Duration maxTotalExposure = - maxExposureTime * - (status_.fixedGain != 0.0 - ? status_.fixedGain - : exposureMode_->gain.back()); + double maxGain = status_.fixedGain ? status_.fixedGain : exposureMode_->gain.back(); + maxGain = limitGain(maxGain); + Duration maxTotalExposure = maxExposureTime * maxGain; target_.totalExposure = std::min(target_.totalExposure, maxTotalExposure); } LOG(RPiAgc, Debug) << "Target totalExposure " << target_.totalExposure; @@ -824,8 +805,6 @@ void AgcChannel::computeTargetExposure(double gain) bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channelTotalExposures) { bool channelBound = false; - LOG(RPiAgc, Debug) - << "Total exposure before channel constraints " << filtered_.totalExposure; for (const auto &constraint : config_.channelConstraints) { LOG(RPiAgc, Debug) @@ -860,7 +839,7 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound) { - double dg = 1.0; + filtered_.totalExposureNoDG = filtered_.totalExposure; /* * Finally, if we're trying to reduce exposure but the target_Y is @@ -871,15 +850,14 @@ bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound * quickly (and we then approach the correct value more quickly from * below). */ - bool desaturate = false; - if (config_.desaturate) - desaturate = !channelBound && - targetY > config_.fastReduceThreshold && gain < sqrt(targetY); - if (desaturate) - dg /= config_.fastReduceThreshold; - LOG(RPiAgc, Debug) << "Digital gain " << dg << " desaturate? " << desaturate; - filtered_.totalExposureNoDG = filtered_.totalExposure / dg; - LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << filtered_.totalExposureNoDG; + bool desaturate = config_.desaturate && !channelBound && + targetY > config_.fastReduceThreshold && gain < sqrt(targetY); + + if (desaturate) { + filtered_.totalExposureNoDG *= config_.fastReduceThreshold; + LOG(RPiAgc, Debug) << "Desaturating, exposure no dg " << filtered_.totalExposureNoDG; + } + return desaturate; } @@ -915,8 +893,7 @@ void AgcChannel::filterExposure() filtered_.totalExposure = speed * target_.totalExposure + filtered_.totalExposure * (1.0 - speed); } - LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure - << " no dg " << filtered_.totalExposureNoDG; + LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure; } void AgcChannel::divideUpExposure() @@ -957,9 +934,7 @@ void AgcChannel::divideUpExposure() } } } - LOG(RPiAgc, Debug) - << "Divided up exposure time and gain are " << exposureTime - << " and " << gain; + /* * Finally adjust exposure time for flicker avoidance (require both * exposure time and gain not to be fixed). @@ -970,22 +945,30 @@ void AgcChannel::divideUpExposure() if (flickerPeriods) { Duration newExposureTime = flickerPeriods * status_.flickerPeriod; gain *= exposureTime / newExposureTime; - /* - * We should still not allow the ag to go over the - * largest value in the exposure mode. Note that this - * may force more of the total exposure into the digital - * gain as a side-effect. - */ - gain = std::min(gain, exposureMode_->gain.back()); - gain = limitGain(gain); exposureTime = newExposureTime; } LOG(RPiAgc, Debug) << "After flicker avoidance, exposure time " << exposureTime << " gain " << gain; } + + /* Limit analogue gain to maximum allowed. */ + double analogueGain = std::min(gain, mode_.maxAnalogueGain); + + /* Finally work out the digital gain that we will need. */ + filtered_.totalExposureNoDG = analogueGain * exposureTime; + double digitalGain = filtered_.totalExposure / filtered_.totalExposureNoDG; + /* Limit dg by what is allowed. */ + digitalGain = std::min(digitalGain, config_.maxDigitalGain); + /* Update total exposure, in case the dg went down. */ + filtered_.totalExposure = filtered_.totalExposureNoDG * digitalGain; + filtered_.exposureTime = exposureTime; - /* We ask for all the gain as analogue gain; prepare() will be told what we got. */ - filtered_.analogueGain = gain; + filtered_.analogueGain = analogueGain; + filtered_.digitalGain = digitalGain; + LOG(RPiAgc, Debug) << "DivideUpExposure: total " << filtered_.totalExposure + << " no dg " << filtered_.totalExposureNoDG; + LOG(RPiAgc, Debug) << "DivideUpExposure: exp " << exposureTime + << " ag " << gain << " dg " << digitalGain; } void AgcChannel::writeAndFinish(Metadata *imageMetadata, bool desaturate) @@ -994,6 +977,7 @@ void AgcChannel::writeAndFinish(Metadata *imageMetadata, bool desaturate) status_.targetExposureValue = desaturate ? 0s : target_.totalExposure; status_.exposureTime = filtered_.exposureTime; status_.analogueGain = filtered_.analogueGain; + status_.digitalGain = filtered_.digitalGain; /* * Write to metadata as well, in case anyone wants to update the camera * immediately. @@ -1001,8 +985,6 @@ void AgcChannel::writeAndFinish(Metadata *imageMetadata, bool desaturate) imageMetadata->set("agc.status", status_); LOG(RPiAgc, Debug) << "Output written, total exposure requested is " << filtered_.totalExposure; - LOG(RPiAgc, Debug) << "Camera exposure update: exposure time " << filtered_.exposureTime - << " analogue gain " << filtered_.analogueGain; } Duration AgcChannel::limitExposureTime(Duration exposureTime) @@ -1031,6 +1013,7 @@ double AgcChannel::limitGain(double gain) const if (!gain) return gain; - gain = std::max(gain, mode_.minAnalogueGain); + gain = std::clamp(gain, mode_.minAnalogueGain, + mode_.maxAnalogueGain * config_.maxDigitalGain); return gain; } diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h index 93229128..42d85ec1 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.h +++ b/src/ipa/rpi/controller/rpi/agc_channel.h @@ -135,6 +135,7 @@ private: libcamera::utils::Duration exposureTime; double analogueGain; + double digitalGain; libcamera::utils::Duration totalExposure; libcamera::utils::Duration totalExposureNoDG; /* without digital gain */ };