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

[David Plowman]

Hi Kieran

Thanks for the comments.

On Wed, 14 Jul 2021 at 15:02, Kieran Bingham
<kieran.bingham at 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 at raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush at 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