[libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc: Make AGC handle Pause/Resume for itself

[David Plowman]

Hi Laurent

Yes, good comments.

On Thu, 26 Nov 2020 at 12:56, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Thu, Nov 26, 2020 at 12:32:01PM +0000, David Plowman wrote:
> > AGC, when paused, sets the last exposure/gain it wrote to be its
> > "fixed" values and will therefore continue to return them. When
> > resumed, we clear them so that both will float again.
> >
> > This approach is better because AGC can be paused and we can
> > subsequently change (for example) the exposure and the gain won't
> > float again.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp |  5 +++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 30a1c1c1..9da18c31 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -184,6 +184,26 @@ void Agc::Read(boost::property_tree::ptree const &params)
> >       exposure_mode_ = &config_.exposure_modes[exposure_mode_name_];
> >       constraint_mode_name_ = config_.default_constraint_mode;
> >       constraint_mode_ = &config_.constraint_modes[constraint_mode_name_];
> > +     // Set up the "last shutter/gain" values, in case AGC starts "disabled".
> > +     status_.shutter_time = config_.default_exposure_time;
> > +     status_.analogue_gain = config_.default_analogue_gain;
> > +}
> > +
> > +bool Agc::IsPaused() const
> > +{
> > +     return false;
> > +}
> > +
> > +void Agc::Pause()
> > +{
> > +     fixed_shutter_ = status_.shutter_time;
> > +     fixed_analogue_gain_ = status_.analogue_gain;
> > +}
> > +
> > +void Agc::Resume()
> > +{
> > +     fixed_shutter_ = 0;
> > +     fixed_analogue_gain_ = 0;
> >  }
> >
> >  void Agc::SetEv(double ev)
> > @@ -199,11 +219,15 @@ void Agc::SetFlickerPeriod(double flicker_period)
> >  void Agc::SetFixedShutter(double fixed_shutter)
> >  {
> >       fixed_shutter_ = fixed_shutter;
>
> Maybe there's something I'm missing, but when a request is queued with
> an exposure time, Agc::SetFixedShutter is called regardless of whether
> AGC is enabled or disabled. fixed_shutter_ is thus set to a non-zero
> value, which will be copied to status.fixed_shutter in
> Agc::housekeepConfig(), called in Agc::Process(). As the core code
> doesn't see the AGC being paused anymore, it will call Process() for
> every frame. Won't the fixed shutter take precedence, even when AGC is
> enabled ?

The AGC really has 4 modes of operation, depending on the various
combinations of fixed_shutter_ and fixed_analogue_gain_ being zero or
not. For example, fixed ISO (fixed gain, really) exposures are
accomplished by fixing the analogue gain, and the exposure time can
still float.

In all cases except where both are non-zero, the AGC has to run. But
even in this last case we still leave AGC running so that things work
in the same way, but it actually does some useful things for us too:

1. If you ask for a fixed shutter and gain, but the gain is bigger
than the sensor can deliver, the AGC will make up the rest with
digital gain.

2. There's always this headache case if any of the colour gains go
less than 1. The AGC takes care of that too by forcing the digital
gain up to 1 / minimum_colour_gain.

So in short, yes, Prepare and Process keep running every frame - and
those "fixed" values take precedence if they are non-zero.

I hope that makes it clearer... (but I'm not sure!)

>
> > +     // Set this in case someone calls Pause() straight after.
> > +     status_.shutter_time = fixed_shutter;
> >  }
> >
> >  void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
> >  {
> >       fixed_analogue_gain_ = fixed_analogue_gain;
> > +     // Set this in case someone calls Pause() straight after.
> > +     status_.analogue_gain = fixed_analogue_gain;
> >  }
> >
> >  void Agc::SetMeteringMode(std::string const &metering_mode_name)
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 47ebb324..8a1a20e6 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -70,6 +70,11 @@ public:
> >       Agc(Controller *controller);
> >       char const *Name() const override;
> >       void Read(boost::property_tree::ptree const &params) override;
> > +     // AGC handles "pausing" for itself.
> > +     bool IsPaused() const;
> > +     void Pause();
> > +     void Resume();
>
> Could you add "override" for those three functions ?

Oops, of course. Sorry about that!

Best regards
David

>
> > +     unsigned int GetFrameDrops() const override;
> >       void SetEv(double ev) override;
> >       void SetFlickerPeriod(double flicker_period) override;
> >       void SetFixedShutter(double fixed_shutter) override; // microseconds
>
> --
> Regards,
>
> Laurent Pinchart