[v5,5/8] ipa: raspberry: Port to the new AEGC controls

22336 diff mbox series

From: Jacopo Mondi <jacopo@jmondi.org>

The newly introduced controls to drive the AEGC algorithm allow
to control the computation of the exposure time and analogue gain
separately.

The RPi AEGC implementation already computes the shutter and gain
values separately but does not expose separate functions to control
them.

Augment the AgcAlgorithm interface to allow pausing/resuming the shutter
and gain automatic computations separately and plumb them to the newly
introduced controls.

Add safety checks to ignore ExposureTime and AnalogueGain values if the
algorithms are not paused, and report the correct AeState value by
checking if both algorithms have been paused or if they have converged.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

---
No change in v5

Changes in v4:
- s/shutter/exposure/

Changes in v3:
- recovered from 2-year-old bitrot
---
 src/ipa/rpi/common/ipa_base.cpp            | 74 +++++++++++++++++++---
 src/ipa/rpi/controller/agc_algorithm.h     |  8 ++-
 src/ipa/rpi/controller/rpi/agc.cpp         | 52 +++++++++++++--
 src/ipa/rpi/controller/rpi/agc.h           |  8 ++-
 src/ipa/rpi/controller/rpi/agc_channel.cpp | 24 ++++++-
 src/ipa/rpi/controller/rpi/agc_channel.h   |  8 ++-
 6 files changed, 150 insertions(+), 24 deletions(-)

Hi Paul,

Thank you for the patch.

As this patch touches the Rapsberry Pi IPA, Naush, David, could you
review it ?

On Mon, Dec 16, 2024 at 01:39:51PM +0900, Paul Elder wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> The newly introduced controls to drive the AEGC algorithm allow
> to control the computation of the exposure time and analogue gain
> separately.
> 
> The RPi AEGC implementation already computes the shutter and gain
> values separately but does not expose separate functions to control
> them.
> 
> Augment the AgcAlgorithm interface to allow pausing/resuming the shutter
> and gain automatic computations separately and plumb them to the newly
> introduced controls.
> 
> Add safety checks to ignore ExposureTime and AnalogueGain values if the
> algorithms are not paused, and report the correct AeState value by
> checking if both algorithms have been paused or if they have converged.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> No change in v5
> 
> Changes in v4:
> - s/shutter/exposure/
> 
> Changes in v3:
> - recovered from 2-year-old bitrot
> ---
>  src/ipa/rpi/common/ipa_base.cpp            | 74 +++++++++++++++++++---
>  src/ipa/rpi/controller/agc_algorithm.h     |  8 ++-
>  src/ipa/rpi/controller/rpi/agc.cpp         | 52 +++++++++++++--
>  src/ipa/rpi/controller/rpi/agc.h           |  8 ++-
>  src/ipa/rpi/controller/rpi/agc_channel.cpp | 24 ++++++-
>  src/ipa/rpi/controller/rpi/agc_channel.h   |  8 ++-
>  6 files changed, 150 insertions(+), 24 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 5fce17e67bd6..8924c3a52c26 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -55,8 +55,13 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>  
>  /* List of controls handled by the Raspberry Pi IPA */
>  const ControlInfoMap::Map ipaControls{
> -	{ &controls::AeEnable, ControlInfo(false, true) },
> +	{ &controls::ExposureTimeMode,
> +	  ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> +		      static_cast<int32_t>(controls::ExposureTimeModeManual)) },
>  	{ &controls::ExposureTime, ControlInfo(0, 66666) },
> +	{ &controls::AnalogueGainMode,
> +	  ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> +		      static_cast<int32_t>(controls::AnalogueGainModeManual)) },
>  	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
>  	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>  	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> @@ -764,21 +769,22 @@ void IpaBase::applyControls(const ControlList &controls)
>  				   << " = " << ctrl.second.toString();
>  
>  		switch (ctrl.first) {
> -		case controls::AE_ENABLE: {
> +		case controls::EXPOSURE_TIME_MODE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.getAlgorithm("agc"));
>  			if (!agc) {
>  				LOG(IPARPI, Warning)
> -					<< "Could not set AE_ENABLE - no AGC algorithm";
> +					<< "Could not set EXPOSURE_TIME_MODE - no AGC algorithm";
>  				break;
>  			}
>  
> -			if (ctrl.second.get<bool>() == false)
> -				agc->disableAuto();
> +			if (ctrl.second.get<int32_t>() == controls::ExposureTimeModeManual)
> +				agc->disableAutoExposure();
>  			else
> -				agc->enableAuto();
> +				agc->enableAutoExposure();
>  
> -			libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
> +			libcameraMetadata_.set(controls::ExposureTimeMode,
> +					       ctrl.second.get<int32_t>());
>  			break;
>  		}
>  
> @@ -791,6 +797,13 @@ void IpaBase::applyControls(const ControlList &controls)
>  				break;
>  			}
>  
> +			/*
> +			 * Ignore manual exposure time when the auto exposure
> +			 * algorithm is running.
> +			 */
> +			if (agc->autoExposureEnabled())
> +				break;
> +
>  			/* The control provides units of microseconds. */
>  			agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us);
>  
> @@ -798,6 +811,25 @@ void IpaBase::applyControls(const ControlList &controls)
>  			break;
>  		}
>  
> +		case controls::ANALOGUE_GAIN_MODE: {
> +			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +				controller_.getAlgorithm("agc"));
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set ANALOGUE_GAIN_MODE - no AGC algorithm";
> +				break;
> +			}
> +
> +			if (ctrl.second.get<int32_t>() == controls::AnalogueGainModeManual)
> +				agc->disableAutoGain();
> +			else
> +				agc->enableAutoGain();
> +
> +			libcameraMetadata_.set(controls::AnalogueGainMode,
> +					       ctrl.second.get<int32_t>());
> +			break;
> +		}
> +
>  		case controls::ANALOGUE_GAIN: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.getAlgorithm("agc"));
> @@ -807,6 +839,13 @@ void IpaBase::applyControls(const ControlList &controls)
>  				break;
>  			}
>  
> +			/*
> +			 * Ignore manual analogue gain value when the auto gain
> +			 * algorithm is running.
> +			 */
> +			if (agc->autoGainEnabled())
> +				break;
> +
>  			agc->setFixedAnalogueGain(0, ctrl.second.get<float>());
>  
>  			libcameraMetadata_.set(controls::AnalogueGain,
> @@ -863,6 +902,13 @@ void IpaBase::applyControls(const ControlList &controls)
>  				break;
>  			}
>  
> +			/*
> +			 * Ignore AE_EXPOSURE_MODE if the shutter or the gain
> +			 * are in auto mode.
> +			 */
> +			if (agc->autoExposureEnabled() || agc->autoGainEnabled())
> +				break;
> +
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (ExposureModeTable.count(idx)) {
>  				agc->setExposureMode(ExposureModeTable.at(idx));
> @@ -1323,9 +1369,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
>  	}
>  
>  	AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
> -	if (agcPrepareStatus) {
> -		libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);
> +	if (agcPrepareStatus)
>  		libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);
> +
> +	RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +		controller_.getAlgorithm("agc"));
> +	if (agc) {
> +		if (!agc->autoExposureEnabled() && !agc->autoGainEnabled())
> +			libcameraMetadata_.set(controls::AeState, controls::AeStateIdle);
> +		else if (agcPrepareStatus)
> +			libcameraMetadata_.set(controls::AeState,
> +					       agcPrepareStatus->locked
> +						       ? controls::AeStateConverged
> +						       : controls::AeStateSearching);
>  	}
>  
>  	LuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>("lux.status");
> diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h
> index c97828577602..fdaa10e6c176 100644
> --- a/src/ipa/rpi/controller/agc_algorithm.h
> +++ b/src/ipa/rpi/controller/agc_algorithm.h
> @@ -30,8 +30,12 @@ public:
>  	virtual void setMeteringMode(std::string const &meteringModeName) = 0;
>  	virtual void setExposureMode(std::string const &exposureModeName) = 0;
>  	virtual void setConstraintMode(std::string const &contraintModeName) = 0;
> -	virtual void enableAuto() = 0;
> -	virtual void disableAuto() = 0;
> +	virtual void enableAutoExposure() = 0;
> +	virtual void disableAutoExposure() = 0;
> +	virtual bool autoExposureEnabled() const = 0;
> +	virtual void enableAutoGain() = 0;
> +	virtual void disableAutoGain() = 0;
> +	virtual bool autoGainEnabled() const = 0;
>  	virtual void setActiveChannels(const std::vector<unsigned int> &activeChannels) = 0;
>  };
>  
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index c48fdf156591..02bfdb4a5e22 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -74,22 +74,62 @@ int Agc::checkChannel(unsigned int channelIndex) const
>  	return 0;
>  }
>  
> -void Agc::disableAuto()
> +void Agc::disableAutoExposure()
>  {
> -	LOG(RPiAgc, Debug) << "disableAuto";
> +	LOG(RPiAgc, Debug) << "disableAutoExposure";
>  
>  	/* All channels are enabled/disabled together. */
>  	for (auto &data : channelData_)
> -		data.channel.disableAuto();
> +		data.channel.disableAutoExposure();
>  }
>  
> -void Agc::enableAuto()
> +void Agc::enableAutoExposure()
>  {
> -	LOG(RPiAgc, Debug) << "enableAuto";
> +	LOG(RPiAgc, Debug) << "enableAutoExposure";
>  
>  	/* All channels are enabled/disabled together. */
>  	for (auto &data : channelData_)
> -		data.channel.enableAuto();
> +		data.channel.enableAutoExposure();
> +}
> +
> +bool Agc::autoExposureEnabled() const
> +{
> +	LOG(RPiAgc, Debug) << "autoExposureEnabled";
> +
> +	/*
> +	 * We always have at least one channel, and since all channels are
> +	 * enabled and disabled together we can simply check the first one.
> +	 */
> +	return channelData_[0].channel.autoExposureEnabled();
> +}
> +
> +void Agc::disableAutoGain()
> +{
> +	LOG(RPiAgc, Debug) << "disableAutoGain";
> +
> +	/* All channels are enabled/disabled together. */
> +	for (auto &data : channelData_)
> +		data.channel.disableAutoGain();
> +}
> +
> +void Agc::enableAutoGain()
> +{
> +	LOG(RPiAgc, Debug) << "enableAutoGain";
> +
> +	/* All channels are enabled/disabled together. */
> +	for (auto &data : channelData_)
> +		data.channel.enableAutoGain();
> +}
> +
> +bool Agc::autoGainEnabled() const
> +{
> +	LOG(RPiAgc, Debug) << "autoGainEnabled";
> +
> +	/*
> +	 * We always have at least one channel, and since all channels are
> +	 * enabled and disabled together we can simply check the first one.
> +	 */
> +	return channelData_[0].channel.autoGainEnabled();
>  }
>  
>  unsigned int Agc::getConvergenceFrames() const
> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> index 3aca000bb4c2..c3a940bf697a 100644
> --- a/src/ipa/rpi/controller/rpi/agc.h
> +++ b/src/ipa/rpi/controller/rpi/agc.h
> @@ -40,8 +40,12 @@ public:
>  	void setMeteringMode(std::string const &meteringModeName) override;
>  	void setExposureMode(std::string const &exposureModeName) override;
>  	void setConstraintMode(std::string const &contraintModeName) override;
> -	void enableAuto() override;
> -	void disableAuto() override;
> +	void enableAutoExposure() override;
> +	void disableAutoExposure() override;
> +	bool autoExposureEnabled() const override;
> +	void enableAutoGain() override;
> +	void disableAutoGain() override;
> +	bool autoGainEnabled() const override;
>  	void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
>  	void prepare(Metadata *imageMetadata) override;
>  	void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index 79c459735dfd..e79184b7ac74 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -319,18 +319,36 @@ int AgcChannel::read(const libcamera::YamlObject &params,
>  	return 0;
>  }
>  
> -void AgcChannel::disableAuto()
> +void AgcChannel::disableAutoExposure()
>  {
>  	fixedExposureTime_ = status_.exposureTime;
> -	fixedAnalogueGain_ = status_.analogueGain;
>  }
>  
> -void AgcChannel::enableAuto()
> +void AgcChannel::enableAutoExposure()
>  {
>  	fixedExposureTime_ = 0s;
> +}
> +
> +bool AgcChannel::autoExposureEnabled() const
> +{
> +	return fixedExposureTime_ == 0s;
> +}
> +
> +void AgcChannel::disableAutoGain()
> +{
> +	fixedAnalogueGain_ = status_.analogueGain;
> +}
> +
> +void AgcChannel::enableAutoGain()
> +{
>  	fixedAnalogueGain_ = 0;
>  }
>  
> +bool AgcChannel::autoGainEnabled() const
> +{
> +	return fixedAnalogueGain_ == 0;
> +}
> +
>  unsigned int AgcChannel::getConvergenceFrames() const
>  {
>  	/*
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
> index 734e5efd3266..fa697e6fa57d 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.h
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h
> @@ -96,8 +96,12 @@ public:
>  	void setMeteringMode(std::string const &meteringModeName);
>  	void setExposureMode(std::string const &exposureModeName);
>  	void setConstraintMode(std::string const &contraintModeName);
> -	void enableAuto();
> -	void disableAuto();
> +	void enableAutoExposure();
> +	void disableAutoExposure();
> +	bool autoExposureEnabled() const;
> +	void enableAutoGain();
> +	void disableAutoGain();
> +	bool autoGainEnabled() const;
>  	void switchMode(CameraMode const &cameraMode, Metadata *metadata);
>  	void prepare(Metadata *imageMetadata);
>  	void process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, Metadata *imageMetadata,

Hi Paul

Thanks for the patch!

Maybe s/raspberry/rpi/ in the subject line?

Before getting into the details, perhaps a few remarks would help
everyone to understand where I'm coming from. We have a lot of users,
and mostly I'm just a bit worried about the many Python users could
get quite confused by some of these changes. So I'm interested in
mitigations, such as:
- limiting behaviour changes
- providing obvious warnings when things are not behaving as before
- maybe maintaining "legacy" controls for a period, maybe with warnings
- as a last resort I can hide changes in our Python layers, though the
thought of (effectively) "reverting" some parts of this patch up there
sounds quite unappealing!

On Mon, 16 Dec 2024 at 04:40, Paul Elder <paul.elder@ideasonboard.com> wrote:
>
> From: Jacopo Mondi <jacopo@jmondi.org>
>
> The newly introduced controls to drive the AEGC algorithm allow
> to control the computation of the exposure time and analogue gain
> separately.
>
> The RPi AEGC implementation already computes the shutter and gain
> values separately but does not expose separate functions to control
> them.
>
> Augment the AgcAlgorithm interface to allow pausing/resuming the shutter
> and gain automatic computations separately and plumb them to the newly
> introduced controls.
>
> Add safety checks to ignore ExposureTime and AnalogueGain values if the
> algorithms are not paused, and report the correct AeState value by
> checking if both algorithms have been paused or if they have converged.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> No change in v5
>
> Changes in v4:
> - s/shutter/exposure/
>
> Changes in v3:
> - recovered from 2-year-old bitrot
> ---
>  src/ipa/rpi/common/ipa_base.cpp            | 74 +++++++++++++++++++---
>  src/ipa/rpi/controller/agc_algorithm.h     |  8 ++-
>  src/ipa/rpi/controller/rpi/agc.cpp         | 52 +++++++++++++--
>  src/ipa/rpi/controller/rpi/agc.h           |  8 ++-
>  src/ipa/rpi/controller/rpi/agc_channel.cpp | 24 ++++++-
>  src/ipa/rpi/controller/rpi/agc_channel.h   |  8 ++-
>  6 files changed, 150 insertions(+), 24 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 5fce17e67bd6..8924c3a52c26 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -55,8 +55,13 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>
>  /* List of controls handled by the Raspberry Pi IPA */
>  const ControlInfoMap::Map ipaControls{
> -       { &controls::AeEnable, ControlInfo(false, true) },

This feels like a common operation, perhaps almost the single most
common one in practice. Is there an argument for keeping it?

> +       { &controls::ExposureTimeMode,
> +         ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> +                     static_cast<int32_t>(controls::ExposureTimeModeManual)) },

Previously we had 0 = off (therefore manual), 1 = on (therefore auto),
so this looks to be the opposite way round. Quite a lot of Python
users try to avoid doing the
"libcamera.controls.ExposureTimeModeEnum.Auto/Manual" thing because
they can't remember it, so these folks could get caught out. Could we
avoid that just by swapping these round?

>         { &controls::ExposureTime, ControlInfo(0, 66666) },
> +       { &controls::AnalogueGainMode,
> +         ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> +                     static_cast<int32_t>(controls::AnalogueGainModeManual)) },

As above.

>         { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
>         { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> @@ -764,21 +769,22 @@ void IpaBase::applyControls(const ControlList &controls)
>                                    << " = " << ctrl.second.toString();
>
>                 switch (ctrl.first) {
> -               case controls::AE_ENABLE: {
> +               case controls::EXPOSURE_TIME_MODE: {
>                         RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>                                 controller_.getAlgorithm("agc"));
>                         if (!agc) {
>                                 LOG(IPARPI, Warning)
> -                                       << "Could not set AE_ENABLE - no AGC algorithm";
> +                                       << "Could not set EXPOSURE_TIME_MODE - no AGC algorithm";
>                                 break;
>                         }
>
> -                       if (ctrl.second.get<bool>() == false)
> -                               agc->disableAuto();
> +                       if (ctrl.second.get<int32_t>() == controls::ExposureTimeModeManual)
> +                               agc->disableAutoExposure();
>                         else
> -                               agc->enableAuto();
> +                               agc->enableAutoExposure();
>
> -                       libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
> +                       libcameraMetadata_.set(controls::ExposureTimeMode,
> +                                              ctrl.second.get<int32_t>());

I'm wondering here whether we have introduced that race condition
where you can't reliably update the ExposureTimeMode and the
ExposureTime in the same set of controls. (The unordered nature of
controls means it could be random whether the exposure time gets
ignored.)

Being able to set manual mode and an exposure time atomically seems
like an obvious thing we (indeed everyone) should support.

The other case - setting an exposure time and re-enabling auto mode -
sounds plausible too, though less obvious. I'm not totally sure how
our implementation could support that, do we need to worry?

>                         break;
>                 }
>
> @@ -791,6 +797,13 @@ void IpaBase::applyControls(const ControlList &controls)
>                                 break;
>                         }
>
> +                       /*
> +                        * Ignore manual exposure time when the auto exposure
> +                        * algorithm is running.
> +                        */
> +                       if (agc->autoExposureEnabled())
> +                               break;
> +

Might it be preferable if setting the exposure time *automatically*
puts you into manual mode? Not sure. This would feel slightly more
familiar to our existing users, and also mirrors the behaviour of
autofocus (where setting lens position puts you into manual mode), so
it has a degree of consistency/precedent.

>                         /* The control provides units of microseconds. */
>                         agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us);
>
> @@ -798,6 +811,25 @@ void IpaBase::applyControls(const ControlList &controls)
>                         break;
>                 }
>
> +               case controls::ANALOGUE_GAIN_MODE: {
> +                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +                               controller_.getAlgorithm("agc"));
> +                       if (!agc) {
> +                               LOG(IPARPI, Warning)
> +                                       << "Could not set ANALOGUE_GAIN_MODE - no AGC algorithm";
> +                               break;
> +                       }
> +
> +                       if (ctrl.second.get<int32_t>() == controls::AnalogueGainModeManual)
> +                               agc->disableAutoGain();
> +                       else
> +                               agc->enableAutoGain();
> +
> +                       libcameraMetadata_.set(controls::AnalogueGainMode,
> +                                              ctrl.second.get<int32_t>());
> +                       break;
> +               }
> +

Same as above re. control race condition.

>                 case controls::ANALOGUE_GAIN: {
>                         RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>                                 controller_.getAlgorithm("agc"));
> @@ -807,6 +839,13 @@ void IpaBase::applyControls(const ControlList &controls)
>                                 break;
>                         }
>
> +                       /*
> +                        * Ignore manual analogue gain value when the auto gain
> +                        * algorithm is running.
> +                        */
> +                       if (agc->autoGainEnabled())
> +                               break;
> +

Also same as before.

>                         agc->setFixedAnalogueGain(0, ctrl.second.get<float>());
>
>                         libcameraMetadata_.set(controls::AnalogueGain,
> @@ -863,6 +902,13 @@ void IpaBase::applyControls(const ControlList &controls)
>                                 break;
>                         }
>
> +                       /*
> +                        * Ignore AE_EXPOSURE_MODE if the shutter or the gain
> +                        * are in auto mode.
> +                        */
> +                       if (agc->autoExposureEnabled() || agc->autoGainEnabled())
> +                               break;
> +

I wasn't convinced here. We want to set exposure mode if both are
auto, right? Suddenly feeling a bit muddled...

I feel a bit funny about not allowing this to update if it will have
no (immediate) effect. All other "auto parameters" (constraint mode,
metering mode, ev, flicker...) can be updated in manual mode, and this
feels probably right to me. What do other folks think about this?

Just as an aside, I also feel a bit funny about the exposure time and
analogue gain being different in this respect. That is, with this
patch you can't update them while in auto mode, when perhaps you
should be able to - taking only effect when you go into manual mode.
Some of the confusion here is because of our implementation - we
conflate the "enable" and the "value" into just the "value", whereas
the controls for this are now split. I wonder if we should carefully
define the desired behaviour for all this, and then rationalise the
implementation. Feeling a bit uncertain about all this...!

Anyway, I think that's probably everything for now. Thanks again!

David

>                         int32_t idx = ctrl.second.get<int32_t>();
>                         if (ExposureModeTable.count(idx)) {
>                                 agc->setExposureMode(ExposureModeTable.at(idx));
> @@ -1323,9 +1369,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
>         }
>
>         AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
> -       if (agcPrepareStatus) {
> -               libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);
> +       if (agcPrepareStatus)
>                 libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);
> +
> +       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +               controller_.getAlgorithm("agc"));
> +       if (agc) {
> +               if (!agc->autoExposureEnabled() && !agc->autoGainEnabled())
> +                       libcameraMetadata_.set(controls::AeState, controls::AeStateIdle);
> +               else if (agcPrepareStatus)
> +                       libcameraMetadata_.set(controls::AeState,
> +                                              agcPrepareStatus->locked
> +                                                      ? controls::AeStateConverged
> +                                                      : controls::AeStateSearching);
>         }
>
>         LuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>("lux.status");
> diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h
> index c97828577602..fdaa10e6c176 100644
> --- a/src/ipa/rpi/controller/agc_algorithm.h
> +++ b/src/ipa/rpi/controller/agc_algorithm.h
> @@ -30,8 +30,12 @@ public:
>         virtual void setMeteringMode(std::string const &meteringModeName) = 0;
>         virtual void setExposureMode(std::string const &exposureModeName) = 0;
>         virtual void setConstraintMode(std::string const &contraintModeName) = 0;
> -       virtual void enableAuto() = 0;
> -       virtual void disableAuto() = 0;
> +       virtual void enableAutoExposure() = 0;
> +       virtual void disableAutoExposure() = 0;
> +       virtual bool autoExposureEnabled() const = 0;
> +       virtual void enableAutoGain() = 0;
> +       virtual void disableAutoGain() = 0;
> +       virtual bool autoGainEnabled() const = 0;
>         virtual void setActiveChannels(const std::vector<unsigned int> &activeChannels) = 0;
>  };
>
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index c48fdf156591..02bfdb4a5e22 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -74,22 +74,62 @@ int Agc::checkChannel(unsigned int channelIndex) const
>         return 0;
>  }
>
> -void Agc::disableAuto()
> +void Agc::disableAutoExposure()
>  {
> -       LOG(RPiAgc, Debug) << "disableAuto";
> +       LOG(RPiAgc, Debug) << "disableAutoExposure";
>
>         /* All channels are enabled/disabled together. */
>         for (auto &data : channelData_)
> -               data.channel.disableAuto();
> +               data.channel.disableAutoExposure();
>  }
>
> -void Agc::enableAuto()
> +void Agc::enableAutoExposure()
>  {
> -       LOG(RPiAgc, Debug) << "enableAuto";
> +       LOG(RPiAgc, Debug) << "enableAutoExposure";
>
>         /* All channels are enabled/disabled together. */
>         for (auto &data : channelData_)
> -               data.channel.enableAuto();
> +               data.channel.enableAutoExposure();
> +}
> +
> +bool Agc::autoExposureEnabled() const
> +{
> +       LOG(RPiAgc, Debug) << "autoExposureEnabled";
> +
> +       /*
> +        * We always have at least one channel, and since all channels are
> +        * enabled and disabled together we can simply check the first one.
> +        */
> +       return channelData_[0].channel.autoExposureEnabled();
> +}
> +
> +void Agc::disableAutoGain()
> +{
> +       LOG(RPiAgc, Debug) << "disableAutoGain";
> +
> +       /* All channels are enabled/disabled together. */
> +       for (auto &data : channelData_)
> +               data.channel.disableAutoGain();
> +}
> +
> +void Agc::enableAutoGain()
> +{
> +       LOG(RPiAgc, Debug) << "enableAutoGain";
> +
> +       /* All channels are enabled/disabled together. */
> +       for (auto &data : channelData_)
> +               data.channel.enableAutoGain();
> +}
> +
> +bool Agc::autoGainEnabled() const
> +{
> +       LOG(RPiAgc, Debug) << "autoGainEnabled";
> +
> +       /*
> +        * We always have at least one channel, and since all channels are
> +        * enabled and disabled together we can simply check the first one.
> +        */
> +       return channelData_[0].channel.autoGainEnabled();
>  }
>
>  unsigned int Agc::getConvergenceFrames() const
> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> index 3aca000bb4c2..c3a940bf697a 100644
> --- a/src/ipa/rpi/controller/rpi/agc.h
> +++ b/src/ipa/rpi/controller/rpi/agc.h
> @@ -40,8 +40,12 @@ public:
>         void setMeteringMode(std::string const &meteringModeName) override;
>         void setExposureMode(std::string const &exposureModeName) override;
>         void setConstraintMode(std::string const &contraintModeName) override;
> -       void enableAuto() override;
> -       void disableAuto() override;
> +       void enableAutoExposure() override;
> +       void disableAutoExposure() override;
> +       bool autoExposureEnabled() const override;
> +       void enableAutoGain() override;
> +       void disableAutoGain() override;
> +       bool autoGainEnabled() const override;
>         void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
>         void prepare(Metadata *imageMetadata) override;
>         void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index 79c459735dfd..e79184b7ac74 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -319,18 +319,36 @@ int AgcChannel::read(const libcamera::YamlObject &params,
>         return 0;
>  }
>
> -void AgcChannel::disableAuto()
> +void AgcChannel::disableAutoExposure()
>  {
>         fixedExposureTime_ = status_.exposureTime;
> -       fixedAnalogueGain_ = status_.analogueGain;
>  }
>
> -void AgcChannel::enableAuto()
> +void AgcChannel::enableAutoExposure()
>  {
>         fixedExposureTime_ = 0s;
> +}
> +
> +bool AgcChannel::autoExposureEnabled() const
> +{
> +       return fixedExposureTime_ == 0s;
> +}
> +
> +void AgcChannel::disableAutoGain()
> +{
> +       fixedAnalogueGain_ = status_.analogueGain;
> +}
> +
> +void AgcChannel::enableAutoGain()
> +{
>         fixedAnalogueGain_ = 0;
>  }
>
> +bool AgcChannel::autoGainEnabled() const
> +{
> +       return fixedAnalogueGain_ == 0;
> +}
> +
>  unsigned int AgcChannel::getConvergenceFrames() const
>  {
>         /*
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
> index 734e5efd3266..fa697e6fa57d 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.h
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h
> @@ -96,8 +96,12 @@ public:
>         void setMeteringMode(std::string const &meteringModeName);
>         void setExposureMode(std::string const &exposureModeName);
>         void setConstraintMode(std::string const &contraintModeName);
> -       void enableAuto();
> -       void disableAuto();
> +       void enableAutoExposure();
> +       void disableAutoExposure();
> +       bool autoExposureEnabled() const;
> +       void enableAutoGain();
> +       void disableAutoGain();
> +       bool autoGainEnabled() const;
>         void switchMode(CameraMode const &cameraMode, Metadata *metadata);
>         void prepare(Metadata *imageMetadata);
>         void process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, Metadata *imageMetadata,
> --
> 2.39.2
>