From patchwork Mon Jul 21 07:47:25 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Naushir Patuck X-Patchwork-Id: 23864 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 A02B8C3323 for ; Mon, 21 Jul 2025 07:49:10 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 18C4468FDE; Mon, 21 Jul 2025 09:49:09 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="Wu/Nv8zC"; dkim-atps=neutral Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 61E2768FCE for ; Mon, 21 Jul 2025 09:48:59 +0200 (CEST) Received: by mail-wr1-x42a.google.com with SMTP id ffacd0b85a97d-3a4e749d7b2so660864f8f.0 for ; Mon, 21 Jul 2025 00:48:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1753084138; x=1753688938; 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=BGJsTePNOrgIui+pOXQicmAtP5q9l21Oj649LL5oFl8=; b=Wu/Nv8zCOgGamHseKgJDBKNiNolkId9fAU7UrQAMoOVa3dn8m+NE9UZ/BkIIHMCfLH x17wAEmmL4jnd/f0B0iJsYgOkm0DdI/ZcjPy7CApNx+nHvpkaIkF6SOXcq0RyHx9d/O7 y1z33Rv2qVehiSF3dcBovdecFVBFdUwtkETXbTCznETJECcJmfpIe228IFWUlHJpSEJK Z/e7Elk4xoA8rSRB9+lVpVFrZo7NQk6uRfIa3NdaVn8e+bsbKob20lFg7PgBC6Nwcna4 ZeSKjOhNzh30uPEIVdx3OrxW9Df72f7O6ZjXxxQRCBkwzlOPKpZu0P5V+lm4xV3SO3Yd OYvA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1753084138; x=1753688938; 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=BGJsTePNOrgIui+pOXQicmAtP5q9l21Oj649LL5oFl8=; b=b9XbiK1ydYbK4lixVeCF9YSpiyR7p+sRaN2HsjzigGAC3Ej/omy1geLl2EAjpW712b RlNPhP7eNW2y1WvdnlPrjZGOocge8wBYwbBFYwL2r8UWeIyfSUZtkq/FbYcVLA7a7elA HY2gk32K2EWqBTP5KYdZX7NgbYHphdrxJjHihsyAYGP0jC0um+FrItDghcVVwQuk1toQ c9T/UjpBkzReIyotGj6yVL517lsKisCXMatir6ydVKaRZvRgw/49+v874LjtqJ+YRcZX Oncl+M2bdApGSTP5S3wfpgJvfLYmWgIHNEPr6/8UYcuJo7N657v65aHFwJmE7yLPxcDj lhGA== X-Gm-Message-State: AOJu0YyFRiW409MWaGM4jptQ1MZ7F2f8rSWil/3GYza2keoOqgtVRsdI gJbiGuRFQ945N3PZ1Bknsz5IpdrhtxPKi7u0tLpvIzMFTzoe890anZfcTNYgLQRycwjdv57z1wl vm5wl2VE= X-Gm-Gg: ASbGncuxnoO65IuJeDT//XiWZedqUSVXLy/32qkJ7TUhyfUm8VJrc8+qCSZgcOUA4aG p5dYxxtibC6d6J9ZIPBu+c2bAOS6cPQajTK7JcN8SW3oC2RcHI5aoEd0XqVEtqNu/nCtowpzYAX oq3i3TZvdYE9e0BJOmc/8rxTR8xzSd6G3mTFd4szktl/MixwkzPy96yZFEj68IpT633osAPri/H 1hkLsZQJJ69MoKPzPgaZgVgwjYFBCm+jTSBG01PQIUQlNm3WHb3LRGSZ+8a9iUBib6KB8Xoxq2H 2fzsXxkwyHcRGKNu4RamxoyZAIfzOIfiLMJiTjDtP8miCX8RZmyubF6ueWuupo+q14izmITzPjd qXP7BJc01coM5KVb0cUADtkOzn3Mh56Wfd+ltCsGEDw== X-Google-Smtp-Source: AGHT+IHSjNsYTMgVxEuC/RBdJ23Pfo9VGVH4d83MFxJ1R/fuA+VxwgGT1hXql0gklPeNPavgW+kv1Q== X-Received: by 2002:adf:eec7:0:b0:3a4:dbdf:7152 with SMTP id ffacd0b85a97d-3b60dd87cc9mr5469220f8f.14.1753084138335; Mon, 21 Jul 2025 00:48:58 -0700 (PDT) Received: from NAUSH-P-DELL.pitowers.org ([93.93.133.154]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4562e802afasm151101765e9.12.2025.07.21.00.48.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 21 Jul 2025 00:48:58 -0700 (PDT) From: Naushir Patuck To: libcamera-devel@lists.libcamera.org Cc: David Plowman , Naushir Patuck Subject: [PATCH v2 5/7] ipa: rpi: agc: Calculate digital gain in process() Date: Mon, 21 Jul 2025 08:47:25 +0100 Message-ID: <20250721074853.1463358-6-naush@raspberrypi.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20250721074853.1463358-1-naush@raspberrypi.com> References: <20250721074853.1463358-1-naush@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" From: David Plowman 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 Signed-off-by: Naushir Patuck Reviewed-by: Naushir Patuck --- 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 d4cedcf49c3c..956d6abf398c 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 9e4661616051..7da1c6dbbec7 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 93229128abf1..42d85ec15e8d 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 */ };