ipa: rpi: agc: Ignore stable region when exposure/gain set manually
diff mbox series

Message ID 20240909132204.14147-1-david.plowman@raspberrypi.com
State Accepted
Commit 5c5bc85082fce87d66964207f4e0292807f591a3
Headers show
Series
  • ipa: rpi: agc: Ignore stable region when exposure/gain set manually
Related show

Commit Message

David Plowman Sept. 9, 2024, 1:22 p.m. UTC
When a user is taking control of exposure and gain, setting them
manually, we set the AGC "stable region" to zero. This means that any
user changes, however small, will be applied, and they won't be
regarded as "too small to bother with".

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/rpi/controller/rpi/agc_channel.cpp | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Naushir Patuck Sept. 9, 2024, 1:40 p.m. UTC | #1
Hi David,

Thank you for this fix.


On Mon, 9 Sept 2024 at 14:22, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> When a user is taking control of exposure and gain, setting them
> manually, we set the AGC "stable region" to zero. This means that any
> user changes, however small, will be applied, and they won't be
> regarded as "too small to bother with".
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  src/ipa/rpi/controller/rpi/agc_channel.cpp | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index cf2565a8..c9df9b5b 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -883,11 +883,14 @@ void AgcChannel::filterExposure()
>
>         /*
>          * AGC adapts instantly if both shutter and gain are directly specified
> -        * or we're in the startup phase.
> +        * or we're in the startup phase. Also disable the stable region, because we want
> +        * to reflect any user exposure/gain updates, however small.
>          */
>         if ((status_.fixedShutter && status_.fixedAnalogueGain) ||
> -           frameCount_ <= config_.startupFrames)
> +           frameCount_ <= config_.startupFrames) {
>                 speed = 1.0;
> +               stableRegion = 0.0;
> +       }
>         if (!filtered_.totalExposure) {
>                 filtered_.totalExposure = target_.totalExposure;
>         } else if (filtered_.totalExposure * (1.0 - stableRegion) < target_.totalExposure &&
> --
> 2.34.1
>
Kieran Bingham Sept. 9, 2024, 1:48 p.m. UTC | #2
Quoting David Plowman (2024-09-09 14:22:04)
> When a user is taking control of exposure and gain, setting them
> manually, we set the AGC "stable region" to zero. This means that any
> user changes, however small, will be applied, and they won't be
> regarded as "too small to bother with".
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

This sounds/looks sane to me, and this is the only function that
stableRegion is used from the config file.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/ipa/rpi/controller/rpi/agc_channel.cpp | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index cf2565a8..c9df9b5b 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -883,11 +883,14 @@ void AgcChannel::filterExposure()
>  
>         /*
>          * AGC adapts instantly if both shutter and gain are directly specified
> -        * or we're in the startup phase.
> +        * or we're in the startup phase. Also disable the stable region, because we want
> +        * to reflect any user exposure/gain updates, however small.
>          */
>         if ((status_.fixedShutter && status_.fixedAnalogueGain) ||
> -           frameCount_ <= config_.startupFrames)
> +           frameCount_ <= config_.startupFrames) {
>                 speed = 1.0;
> +               stableRegion = 0.0;
> +       }
>         if (!filtered_.totalExposure) {
>                 filtered_.totalExposure = target_.totalExposure;
>         } else if (filtered_.totalExposure * (1.0 - stableRegion) < target_.totalExposure &&
> -- 
> 2.34.1
>

Patch
diff mbox series

diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
index cf2565a8..c9df9b5b 100644
--- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
+++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
@@ -883,11 +883,14 @@  void AgcChannel::filterExposure()
 
 	/*
 	 * AGC adapts instantly if both shutter and gain are directly specified
-	 * or we're in the startup phase.
+	 * or we're in the startup phase. Also disable the stable region, because we want
+	 * to reflect any user exposure/gain updates, however small.
 	 */
 	if ((status_.fixedShutter && status_.fixedAnalogueGain) ||
-	    frameCount_ <= config_.startupFrames)
+	    frameCount_ <= config_.startupFrames) {
 		speed = 1.0;
+		stableRegion = 0.0;
+	}
 	if (!filtered_.totalExposure) {
 		filtered_.totalExposure = target_.totalExposure;
 	} else if (filtered_.totalExposure * (1.0 - stableRegion) < target_.totalExposure &&