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

[David Plowman]

Hi Laurent

On Mon, 30 Nov 2020 at 18:25, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.

Yes, setting them to zero makes them float again but we should
document that and I'll do so.

>
> 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 :-)) ?

I think there's quite a lot to think about here. The camera world
tends to talk about AE being on or off, fixed ISO, aperture or shutter
priority, those kinds of terms. But I think computer people think
about 3 degrees of freedom (shutter, gain and aperture where it
exists), and what combinations you can have in which they are fixed or
floating.

But actually it's worse than that because as soon as you have at least
two of your variables that float, you can start to talk about the
order and at which values they change.

Now there is a solution to this, namely exposure profiles. In our
implementation we have only shutter and gain, but you could add
aperture and define exposure/aperture/gain profiles for all the
situations you could imagine.

Unfortunately, I don't know if many vendors would do this, and you
could easily end up with vast numbers of seemingly arbitrary profiles.
Therefore, I think I'm drawn to the conclusion that one is better off
going with the "camera" view of the world. It's a bit of a lowest
common denominator, but I think it would be broadly acceptable to
everyone.

I think this means you'd have, essentially, user-specified shutter or
gain or aperture. Whether you can fix only one, or two would be up to
the implementation. Fixing all 3 would probably be known as "AE off".
Anyway, that's my initial reaction, but there's definitely something
to sleep on there!

Best regards
David

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