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

Message ID 20201126123203.19105-3-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Raspberry Pi AGC improvements
Related show

Commit Message

David Plowman Nov. 26, 2020, 12:32 p.m. UTC
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@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++
 src/ipa/raspberrypi/controller/rpi/agc.hpp |  5 +++++
 2 files changed, 29 insertions(+)

Comments

Laurent Pinchart Nov. 26, 2020, 12:56 p.m. UTC | #1
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@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 ?

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

> +	unsigned int GetFrameDrops() const override;
>  	void SetEv(double ev) override;
>  	void SetFlickerPeriod(double flicker_period) override;
>  	void SetFixedShutter(double fixed_shutter) override; // microseconds
David Plowman Nov. 26, 2020, 2:10 p.m. UTC | #2
Hi Laurent

Yes, good comments.

On Thu, 26 Nov 2020 at 12:56, Laurent Pinchart
<laurent.pinchart@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@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
Kieran Bingham Nov. 30, 2020, 10:56 a.m. UTC | #3
Hi David,

On 26/11/2020 14:10, David Plowman wrote:
> Hi Laurent
> 
> Yes, good comments.
> 
> On Thu, 26 Nov 2020 at 12:56, Laurent Pinchart
> <laurent.pinchart@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@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;

there won't be any side effects here if the requests are specifying
fixed values will there?

I.e. something calling 'Resume' isn't going to remove the users requests...


>>>  }
>>>
>>>  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.

Baha, Ok that answers my question above.

So this is fine with me.

With the overrides added in the next version,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

We're in src/ipa/raspberrypi so it would be helpful to have some RB or
TB tags from Naush on this series.



>>> +     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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Nov. 30, 2020, 6:25 p.m. UTC | #4
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@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
David Plowman Nov. 30, 2020, 8:21 p.m. UTC | #5
Hi Laurent

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

Patch
diff mbox series

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;
+	// 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();
+	unsigned int GetFrameDrops() const override;
 	void SetEv(double ev) override;
 	void SetFlickerPeriod(double flicker_period) override;
 	void SetFixedShutter(double fixed_shutter) override; // microseconds