| Message ID | 20201126123203.19105-3-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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 ¶ms) > 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 ¶ms) 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
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 ¶ms) > > 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 ¶ms) 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
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 ¶ms) >>> 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 ¶ms) 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 >
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 ¶ms) > > > 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 ¶ms) 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
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 ¶ms) > > > > 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 ¶ms) 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
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 ¶ms) 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 ¶ms) 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
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(+)