[libcamera-devel] [PATCH v4 2/2] ipa: raspberrypi: AGC: handle modes with different sensitivities

[Kieran Bingham]

Hi David,

On 18/07/2021 19:51, David Plowman wrote:
> When the sensor is switched to a mode with a different sensitivity,
> the target exposure values need to be adjusted proportionately to
> maintain the same image brightness.

Great, looks like my review comment is handled so:

> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>

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

and I think these are ready for merge.

--
Kieran


> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 27 ++++++++++++++++++----
>  src/ipa/raspberrypi/controller/rpi/agc.hpp |  1 +
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index f57783f8..1cfdd3bb 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -163,7 +163,7 @@ Agc::Agc(Controller *controller)
>  	: AgcAlgorithm(controller), metering_mode_(nullptr),
>  	  exposure_mode_(nullptr), constraint_mode_(nullptr),
>  	  frame_count_(0), lock_count_(0),
> -	  last_target_exposure_(0s),
> +	  last_target_exposure_(0s), last_sensitivity_(0.0),
>  	  ev_(1.0), flicker_period_(0s),
>  	  max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)
>  {
> @@ -268,9 +268,12 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)
>  	constraint_mode_name_ = constraint_mode_name;
>  }
>  
> -void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
> +void Agc::SwitchMode(CameraMode const &camera_mode,
>  		     Metadata *metadata)
>  {
> +	/* AGC expects the mode sensitivity always to be non-zero. */
> +	ASSERT(camera_mode.sensitivity);
> +
>  	housekeepConfig();
>  
>  	Duration fixed_shutter = clipShutter(fixed_shutter_);
> @@ -292,9 +295,20 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
>  		filtered_.shutter = fixed_shutter;
>  		filtered_.analogue_gain = fixed_analogue_gain_;
>  	} else if (status_.total_exposure_value) {
> -		// On a mode switch, it's possible the exposure profile could change,
> -		// or a fixed exposure/gain might be set so we divide up the exposure/
> -		// gain again, but we don't change any target values.
> +		// On a mode switch, various things could happen:
> +		// - the exposure profile might change
> +		// - a fixed exposure or gain might be set
> +		// - the new mode's sensitivity might be different
> +		// We cope with the last of these by scaling the target values. After
> +		// that we just need to re-divide the exposure/gain according to the
> +		// current exposure profile, which takes care of everything else.
> +
> +		double ratio = last_sensitivity_ / camera_mode.sensitivity;
> +		target_.total_exposure_no_dg *= ratio;
> +		target_.total_exposure *= ratio;
> +		filtered_.total_exposure_no_dg *= ratio;
> +		filtered_.total_exposure *= ratio;
> +
>  		divideUpExposure();
>  	} else {
>  		// We come through here on startup, when at least one of the shutter
> @@ -308,6 +322,9 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
>  	}
>  
>  	writeAndFinish(metadata, false);
> +
> +	// We must remember the sensitivity of this mode for the next SwitchMode.
> +	last_sensitivity_ = camera_mode.sensitivity;
>  }
>  
>  void Agc::Prepare(Metadata *image_metadata)
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 85067dc6..82063636 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -126,6 +126,7 @@ private:
>  	int lock_count_;
>  	DeviceStatus last_device_status_;
>  	libcamera::utils::Duration last_target_exposure_;
> +	double last_sensitivity_; // sensitivity of the previous camera mode
>  	// Below here the "settings" that applications can change.
>  	std::string metering_mode_name_;
>  	std::string exposure_mode_name_;
>