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

[Laurent Pinchart]

Hi David,

On Thu, Nov 26, 2020 at 02:10:25PM +0000, David Plowman wrote:
> On Thu, 26 Nov 2020 at 12:56, Laurent Pinchart wrote:
> > 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.

That's interesting, I hadn't thought about that.

> 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!)

It does, but doesn't address my concern fully :-)

My understanding of the code is that, when AGC is on, if an application
sets a fixed shutter or fixed analog gain, those will take precedence.
There however seems to be no way to go back to dynamic shutter or analog
gain. And not that I've written this, I wonder if this can be achieved
by setting the shutter or gain to 0 explicitly. Is that the expected
usage of this API ? If so it should at least be documented in
controls_ids.yaml.

I however wonder if we should create an AeMode control that would
explicitly select between the four modes (manual, auto,
exposure-priority and gain-priority). And thinking about it, how about
devices that can also control the iris aperture ? Maybe the mode (or
should we call it AePriority ?) should be a bitmask with bits telling
which parameters are manual and which are controlled by the AEGC
algorithm. But then, maybe setting manual parameters to zero would be a
good enough API to state they should be automatic, but in that case,
what would the AeEnable control be used for if setting AeEnable to true
with non-zero value for ExposureTime and AnalogueGain effectively
disable auto-exposure ?

We don't have to solve this as part of this series, but I think there's
a bit of an inconsistency in the controls we have, which I'd like to
address. What would your preference be between the possible options
(something better than me naive proposals above will certainly be
welcome too :-)) ?

> > > +     // 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!
>
> > > +     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