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

12570 diff mbox series

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.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/rpi/agc.cpp | 25 +++++++++++++++++-----
 src/ipa/raspberrypi/controller/rpi/agc.hpp |  1 +
 2 files changed, 21 insertions(+), 5 deletions(-)

Hi David,

On 11/06/2021 11:07, 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.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 25 +++++++++++++++++-----
>  src/ipa/raspberrypi/controller/rpi/agc.hpp |  1 +
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 55e80ac7..431029d0 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)
>  {
> @@ -269,7 +269,7 @@ 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)
>  {
>  	housekeepConfig();
> @@ -293,9 +293,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;

Is it right that we ASSERT(camera_mode.sensitivity != 0) below here,
which is presumably after this statement, and therefore we've already
broken here?

Perhaps I'm missing some calling context that isn't visible in the patch.



> +		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
> @@ -309,6 +320,10 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
>  	}
>  
>  	writeAndFinish(metadata, false);
> +
> +	// We must remember the sensitivity of this mode for the next SwitchMode.
> +	ASSERT(camera_mode.sensitivity != 0.0);

Is this check to ensure that a cam helper doesn't go setting it to 0.0?


> +	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 75078948..0d08dcd7 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_;
>

Hi Kieran

Thanks for the comments.

On Wed, 14 Jul 2021 at 15:02, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi David,
>
> On 11/06/2021 11:07, 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.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 25 +++++++++++++++++-----
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp |  1 +
> >  2 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 55e80ac7..431029d0 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)
> >  {
> > @@ -269,7 +269,7 @@ 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)
> >  {
> >       housekeepConfig();
> > @@ -293,9 +293,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;
>
> Is it right that we ASSERT(camera_mode.sensitivity != 0) below here,
> which is presumably after this statement, and therefore we've already
> broken here?
>
> Perhaps I'm missing some calling context that isn't visible in the patch.

No, I think that's a fair cop actually. Probably I decided to be
super-cautious and added the ASSERT later without double-checking.
Sigh. Never mind, let me submit a version 4 (or whatever) and then
maybe we'll be good to go. But anyway, thanks for spotting that.

David

>
>
>
> > +             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
> > @@ -309,6 +320,10 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
> >       }
> >
> >       writeAndFinish(metadata, false);
> > +
> > +     // We must remember the sensitivity of this mode for the next SwitchMode.
> > +     ASSERT(camera_mode.sensitivity != 0.0);
>
> Is this check to ensure that a cam helper doesn't go setting it to 0.0?
>
>
> > +     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 75078948..0d08dcd7 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_;
> >
>
> --
> Regards
> --
> Kieran