[libcamera-devel,v2,6/7] android: Plumb all AE-related controls
diff mbox series

Message ID 20211001103325.1077590-7-paul.elder@ideasonboard.com
State New
Headers show
Series
  • The Great AE Changes
Related show

Commit Message

Paul Elder Oct. 1, 2021, 10:33 a.m. UTC
With the AE-related controls reorganized in libcamera, with separate
value and lever controls for exposure and gain, plumb these through the
HAL layer (in order of appearence in the patch):
- static metadata: available AE modes, AE lock available
- manual template: add AE off (the other templates already have AE on)
- request metadata: HAL -> libcamera controls conversion
- result metadata: libcamera -> HAL controls conversion
- result metadata: AE state

We add class variables to CameraDevice to save the last set android AE
controls, as that is how android controls function (no need to resubmit
controls if they are unchanged).

We also save libcamera's AE state in the request descriptor, as
otherwise there is insufficient information from libcamera's result
metadata alone to tell if the state is locked or manual (as they are an
internal state in libcamera).

Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
New in v2
---
 src/android/camera_capabilities.cpp | 63 ++++++++++++++++---
 src/android/camera_device.cpp       | 98 +++++++++++++++++++++++++++--
 src/android/camera_device.h         | 16 +++++
 3 files changed, 165 insertions(+), 12 deletions(-)

Comments

Jacopo Mondi Oct. 1, 2021, 5:32 p.m. UTC | #1
Hi Paul,

On Fri, Oct 01, 2021 at 07:33:24PM +0900, Paul Elder wrote:
> With the AE-related controls reorganized in libcamera, with separate
> value and lever controls for exposure and gain, plumb these through the
> HAL layer (in order of appearence in the patch):
> - static metadata: available AE modes, AE lock available
> - manual template: add AE off (the other templates already have AE on)
> - request metadata: HAL -> libcamera controls conversion
> - result metadata: libcamera -> HAL controls conversion
> - result metadata: AE state

So I think they should go in separate patches maybe

>
> We add class variables to CameraDevice to save the last set android AE
> controls, as that is how android controls function (no need to resubmit
> controls if they are unchanged).

Do we work any differently ? It's an honest question, I always assumed
we don't.

>
> We also save libcamera's AE state in the request descriptor, as
> otherwise there is insufficient information from libcamera's result
> metadata alone to tell if the state is locked or manual (as they are an
> internal state in libcamera).
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> New in v2
> ---
>  src/android/camera_capabilities.cpp | 63 ++++++++++++++++---
>  src/android/camera_device.cpp       | 98 +++++++++++++++++++++++++++--
>  src/android/camera_device.h         | 16 +++++
>  3 files changed, 165 insertions(+), 12 deletions(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index f7a6cda9..3fed3f83 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -830,12 +830,62 @@ int CameraCapabilities::initializeStaticMetadata()
>  	staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
>  				  aeAvailableAntiBandingModes);
>
> -	std::vector<uint8_t> aeAvailableModes = {
> -		ANDROID_CONTROL_AE_MODE_ON,
> -	};
> +	std::vector<uint8_t> aeAvailableModes;
> +	/* This will cause problems if one supports only on and the other only off */
> +	bool aeAddedOn = false;
> +	bool aeAddedOff = false;
> +
> +	const auto &analogGainModeInfo = controlsInfo.find(&controls::AnalogueGainMode);
> +	if (analogGainModeInfo != controlsInfo.end()) {
> +		for (const auto &value : analogGainModeInfo->second.values()) {
> +			switch (value.get<int32_t>()) {
> +			case controls::AnalogueGainModeAuto:
> +				if (!aeAddedOn)

How can aeAddedOn be true if it's initialized to false and set to true
just here below, unless you expect the same value to be specified
twice in the info.values() ?

Should you just add the value unconditionally here and below ?

> +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
> +				aeAddedOn = true;
> +				break;
> +			case controls::AnalogueGainModeDisabled:
> +				if (!aeAddedOff)
> +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);
> +				aeAddedOff = true;
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +	}
> +
> +	const auto &exposureTimeModeInfo = controlsInfo.find(&controls::ExposureTimeMode);
> +	if (exposureTimeModeInfo != controlsInfo.end()) {
> +		for (const auto &value : exposureTimeModeInfo->second.values()) {
> +			switch (value.get<int32_t>()) {
> +			case controls::ExposureTimeModeAuto:
> +				if (!aeAddedOn)
> +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
> +				aeAddedOn = true;

And setting the aeAdded to true here has no purpose, but that's not an
issue.

What is the policy we want to have here ? Android has a single AE
control, we have two. I don't think we can allow situation where we
have an asymmetric ExposureTimeMode and AnalogueGainMode as matching
on which one to add would become painful.

I would cut it short and requires libcamera implementation that aims
to HAL full level to have both gain and exposure time controllable.

The code can thus be restructured as

        availableModes = { AE_MODE_ON };
        if (exposureTimeInfo != end && analogGainInfo != end) {
                bool manualExp = false;
                bool manualGain = false;

                for (modes : exposureTimeInfo.values())
                        if (mode == Disabled)
                                manualExp = true;
                                break;

               for (modes : analogGainInfo.values())
                        if (mode == Disabled)
                                manualGain = true;
                                break

               if (manualExp && manualGain)
                        availableModes.push_back(AE_MODE_OFF);
        }
> +				break;
> +			case controls::ExposureTimeModeDisabled:
> +				if (!aeAddedOff)
> +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);
> +				aeAddedOff = true;


> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (aeAvailableModes.empty())
> +		aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
>  	staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,
>  				  aeAvailableModes);
>
> +	/* In libcamera, turning off AE is equivalient to locking. */
> +	uint8_t aeLockAvailable = aeAddedOff ? ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE
> +					     : ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;

This really depends on what we want to do with AeState.
I think if we want to report locking from there we should make sure
that among the supported AeState values there is locked. Bummer, we
don't have the Camera::controls() equivalent for metadata, hence we
don't have a way to report the values supported for metadata-only
controls as AeState is

> +	staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> +				  aeLockAvailable);
> +
>  	int64_t minFrameDurationNsec = -1;
>  	int64_t maxFrameDurationNsec = -1;
>  	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);
> @@ -946,10 +996,6 @@ int CameraCapabilities::initializeStaticMetadata()
>  	staticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
>  				  sceneModesOverride);
>
> -	uint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;
> -	staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> -				  aeLockAvailable);
> -
>  	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
>  	staticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
>  				  awbLockAvailable);
> @@ -1358,6 +1404,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons
>  	if (!manualTemplate)
>  		return nullptr;
>
> +	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> +	manualTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);

Only if supported, or unconditionally ?

> +
>  	return manualTemplate;
>  }
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ef4fbab8..d5027ec5 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -263,7 +263,9 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>
>  CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
>  	: id_(id), state_(State::Stopped), camera_(std::move(camera)),
> -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> +	  facing_(CAMERA_FACING_FRONT), orientation_(0),
> +	  aeOn_(true), aeLocked_(false), lastExposureTime_(0),
> +	  lastAnalogueGain_(1.0f), lastDigitalGain_(1.0f)
>  {
>  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
>
> @@ -857,6 +859,62 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  		controls.set(controls::draft::TestPatternMode, testPatternMode);
>  	}
>
> +	/*
> +	 * \todo Can we trust that we won't receive values that we didn't
> +	 * report supporting?
> +	 */
> +	if (settings.getEntry(ANDROID_CONTROL_AE_LOCK, &entry))
> +		aeLocked_ = *entry.data.u8 != ANDROID_CONTROL_AE_LOCK_OFF;
> +
> +	if (settings.getEntry(ANDROID_CONTROL_AE_MODE, &entry))
> +		aeOn_ = *entry.data.u8 != ANDROID_CONTROL_AE_MODE_OFF;
> +
> +	AeMode aeMode;
> +	if (aeLocked_ && aeOn_)
> +		aeMode = AeMode::Lock;
> +	else if (aeLocked_ && !aeOn_)
> +		aeMode = AeMode::Manual;
> +	else if (!aeLocked_ && aeOn_)
> +		aeMode = AeMode::Auto;
> +	else /* !aeLocked_ && !aeOn_ */
> +		aeMode = AeMode::Manual;

        if (!aeOn_)
                aeMode = Manual;
        else if (aeLocked)
                aeMode = Lock;
        else
                aeMode = Auto
> +
> +	/* Save this so that we can recover it in the result */
> +	descriptor->aeMode_ = aeMode;
> +
> +	const auto &eInfo = camera_->controls().find(&controls::ExposureTimeMode);

Doing so at every request is not nasty, but if we could avoid it it
would nice. Don't we have a list of capabilities to rely on ?

> +	if (eInfo != camera_->controls().end()) {
> +		controls.set(controls::ExposureTimeMode,
> +				aeMode == AeMode::Auto ?
> +				controls::ExposureTimeModeAuto :
> +				controls::ExposureTimeModeDisabled);

I'll ask again, do we expect the same controls to be in every request
even if they do not change ?

> +	}
> +
> +	const auto &gInfo = camera_->controls().find(&controls::AnalogueGainMode);
> +	if (gInfo != camera_->controls().end()) {
> +		controls.set(controls::AnalogueGainMode,
> +				aeMode == AeMode::Auto ?
> +				controls::AnalogueGainModeAuto :
> +				controls::AnalogueGainModeDisabled);
> +	}
> +
> +	if (settings.getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry)) {
> +		const auto &info = camera_->controls().find(&controls::ExposureTime);

Should we make sure at capabilities contruction time that if
ExposureTimeMode supports Disabled, then ExposureTime is present ?
If we assume so, and make it a requisite to claim we support
AE_MODE_OFF then these controls should only be handled if
capabilities.contain(MANUAL_SENSOR)

> +		if (info != camera_->controls().end()) {
> +			lastExposureTime_ = (*entry.data.i64) / 1000;
> +			/* Don't disable libcamera's internal AeMode::Lock */
> +			if (aeMode != AeMode::Lock)

Do we expect a request to contain both AE_LOCK and a sensor exposure
time ?

> +				controls.set(controls::ExposureTime, lastExposureTime_);
> +		}
> +	}
> +
> +	/* Trigger libcamera's locked -> manual state change */
> +	if (aeMode == AeMode::Manual && !settings.hasEntry(ANDROID_SENSOR_EXPOSURE_TIME)) {
> +		const auto &info = camera_->controls().find(&controls::ExposureTime);
> +		if (info != camera_->controls().end())
> +			controls.set(controls::ExposureTime, lastExposureTime_);

If we don't need to repeat the control values in every request, is
this necessary ? Or are you afraid we receive a AE_MODE_OFF and no
SENSOR_EXPOSURE_TIME ?

> +	}
> +
>  	return 0;
>  }
>
> @@ -1345,11 +1403,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
>  	resultMetadata->addEntry(ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
>  				 value32);
>
> -	value = ANDROID_CONTROL_AE_LOCK_OFF;
> -	resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, value);
> +	uint8_t aeLock = (descriptor.aeMode_ == AeMode::Lock)
> +		       ? ANDROID_CONTROL_AE_LOCK_ON
> +		       : ANDROID_CONTROL_AE_LOCK_OFF;
> +	resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, aeLock);
>
> -	value = ANDROID_CONTROL_AE_MODE_ON;
> -	resultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, value);
> +	/* Locked means auto + locked in android */
> +	uint8_t aeMode = (descriptor.aeMode_ != AeMode::Manual)
> +		       ? ANDROID_CONTROL_AE_MODE_ON
> +		       : ANDROID_CONTROL_AE_MODE_OFF;
> +	resultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
>
>  	if (settings.getEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, &entry))
>  		/*
> @@ -1366,6 +1429,31 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
>  	resultMetadata->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, value);
>
>  	value = ANDROID_CONTROL_AE_STATE_CONVERGED;
> +	if (metadata.contains(controls::AeState)) {
> +		switch (metadata.get(controls::AeState)) {
> +		case controls::AeStateInactive:
> +			value = ANDROID_CONTROL_AE_STATE_INACTIVE;
> +			break;
> +		case controls::AeStateSearching:
> +			value = ANDROID_CONTROL_AE_STATE_SEARCHING;
> +			break;
> +		case controls::AeStateConverged:
> +			value = ANDROID_CONTROL_AE_STATE_CONVERGED;
> +			break;
> +		case controls::AeStateFlashRequired:
> +			value = ANDROID_CONTROL_AE_STATE_FLASH_REQUIRED;
> +			break;
> +		case controls::AeStatePrecapture:
> +			value = ANDROID_CONTROL_AE_STATE_PRECAPTURE;
> +			break;
> +		default:
> +			if (descriptor.aeMode_ == AeMode::Lock) {
> +				value = ANDROID_CONTROL_AE_STATE_LOCKED;
> +				break;
> +			}
> +			LOG(HAL, Error) << "Invalid AeState, setting converged";
> +		}
> +	}
>  	resultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, value);
>
>  	value = ANDROID_CONTROL_AF_MODE_OFF;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index b7d774fe..f693cdbc 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -73,6 +73,12 @@ private:
>
>  	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
>
> +	enum class AeMode {
> +		Auto,
> +		Lock,
> +		Manual,
> +	};
> +
>  	struct Camera3RequestDescriptor {
>  		enum class Status {
>  			Pending,
> @@ -96,6 +102,9 @@ private:
>
>  		camera3_capture_result_t captureResult_ = {};
>  		Status status_ = Status::Pending;
> +
> +		/* The libcamera internal AE state for this request */
> +		AeMode aeMode_ = AeMode::Auto;
>  	};
>
>  	enum class State {
> @@ -146,6 +155,13 @@ private:
>  	int facing_;
>  	int orientation_;
>
> +	/* Track the last-set android AE controls */
> +	bool aeOn_;
> +	bool aeLocked_;

I guess this is again about if we have to repeat controls every
request, otherwise they don't seem to be required as class members..

Thanks
  j

> +	int32_t lastExposureTime_;
> +	float lastAnalogueGain_;
> +	float lastDigitalGain_;
> +
>  	CameraMetadata lastSettings_;
>  };
>
> --
> 2.27.0
>
Paul Elder Oct. 4, 2021, 8:52 a.m. UTC | #2
Hi Jacopo,

On Fri, Oct 01, 2021 at 07:32:48PM +0200, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Fri, Oct 01, 2021 at 07:33:24PM +0900, Paul Elder wrote:
> > With the AE-related controls reorganized in libcamera, with separate
> > value and lever controls for exposure and gain, plumb these through the
> > HAL layer (in order of appearence in the patch):
> > - static metadata: available AE modes, AE lock available
> > - manual template: add AE off (the other templates already have AE on)
> > - request metadata: HAL -> libcamera controls conversion
> > - result metadata: libcamera -> HAL controls conversion
> > - result metadata: AE state
> 
> So I think they should go in separate patches maybe

Hmmm... they might be easily separatable... they're all related though.

> 
> >
> > We add class variables to CameraDevice to save the last set android AE
> > controls, as that is how android controls function (no need to resubmit
> > controls if they are unchanged).
> 
> Do we work any differently ? It's an honest question, I always assumed
> we don't.

That is how we should work afaik, but we currently don't (and I'm pretty
sure pipeline handlers don't act that way either...).

> 
> >
> > We also save libcamera's AE state in the request descriptor, as
> > otherwise there is insufficient information from libcamera's result
> > metadata alone to tell if the state is locked or manual (as they are an
> > internal state in libcamera).
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > New in v2
> > ---
> >  src/android/camera_capabilities.cpp | 63 ++++++++++++++++---
> >  src/android/camera_device.cpp       | 98 +++++++++++++++++++++++++++--
> >  src/android/camera_device.h         | 16 +++++
> >  3 files changed, 165 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index f7a6cda9..3fed3f83 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -830,12 +830,62 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> >  				  aeAvailableAntiBandingModes);
> >
> > -	std::vector<uint8_t> aeAvailableModes = {
> > -		ANDROID_CONTROL_AE_MODE_ON,
> > -	};
> > +	std::vector<uint8_t> aeAvailableModes;
> > +	/* This will cause problems if one supports only on and the other only off */
> > +	bool aeAddedOn = false;
> > +	bool aeAddedOff = false;
> > +
> > +	const auto &analogGainModeInfo = controlsInfo.find(&controls::AnalogueGainMode);
> > +	if (analogGainModeInfo != controlsInfo.end()) {
> > +		for (const auto &value : analogGainModeInfo->second.values()) {
> > +			switch (value.get<int32_t>()) {
> > +			case controls::AnalogueGainModeAuto:
> > +				if (!aeAddedOn)
> 
> How can aeAddedOn be true if it's initialized to false and set to true
> just here below, unless you expect the same value to be specified
> twice in the info.values() ?

Oh it was for consistency. Same code same shape :D

> 
> Should you just add the value unconditionally here and below ?

Yeah.

> 
> > +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
> > +				aeAddedOn = true;
> > +				break;
> > +			case controls::AnalogueGainModeDisabled:
> > +				if (!aeAddedOff)
> > +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);
> > +				aeAddedOff = true;
> > +				break;
> > +			default:
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	const auto &exposureTimeModeInfo = controlsInfo.find(&controls::ExposureTimeMode);
> > +	if (exposureTimeModeInfo != controlsInfo.end()) {
> > +		for (const auto &value : exposureTimeModeInfo->second.values()) {
> > +			switch (value.get<int32_t>()) {
> > +			case controls::ExposureTimeModeAuto:
> > +				if (!aeAddedOn)
> > +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
> > +				aeAddedOn = true;
> 
> And setting the aeAdded to true here has no purpose, but that's not an
> issue.
> 
> What is the policy we want to have here ? Android has a single AE
> control, we have two. I don't think we can allow situation where we

Yeah that's exactly what I was wondering. What happens if the libcamera
camera only has one of the two (is that possible?)? What if one is
manual-able but the other isn't? Even worse, what if one is auto-only
and the other is manual-only?

So I wasn't sure what to do. I decided that at least one should be set
to auto to enable auto, and at least one should be manual-able to
consider AE manual-able, and at least one should be auto-able to
consider AE auto-able.

> have an asymmetric ExposureTimeMode and AnalogueGainMode as matching
> on which one to add would become painful.

Yeah it's painful. Does anybody know what we can mandate to simplify
this? Can we require that if a camera supports one manual-able it must
also support the other manualable? And the same for auto?

> 
> I would cut it short and requires libcamera implementation that aims
> to HAL full level to have both gain and exposure time controllable.
> 
> The code can thus be restructured as
> 
>         availableModes = { AE_MODE_ON };
>         if (exposureTimeInfo != end && analogGainInfo != end) {
>                 bool manualExp = false;
>                 bool manualGain = false;
> 
>                 for (modes : exposureTimeInfo.values())
>                         if (mode == Disabled)
>                                 manualExp = true;
>                                 break;
> 
>                for (modes : analogGainInfo.values())
>                         if (mode == Disabled)
>                                 manualGain = true;
>                                 break
> 
>                if (manualExp && manualGain)
>                         availableModes.push_back(AE_MODE_OFF);
>         }

Yeah it would be nice, but we need to know that pipelines+IPAs *will*
follow this behavior.

> > +				break;
> > +			case controls::ExposureTimeModeDisabled:
> > +				if (!aeAddedOff)
> > +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);
> > +				aeAddedOff = true;
> 
> 
> > +				break;
> > +			default:
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	if (aeAvailableModes.empty())
> > +		aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
> >  	staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,
> >  				  aeAvailableModes);
> >
> > +	/* In libcamera, turning off AE is equivalient to locking. */
> > +	uint8_t aeLockAvailable = aeAddedOff ? ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE
> > +					     : ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;
> 
> This really depends on what we want to do with AeState.
> I think if we want to report locking from there we should make sure

I don't want to report locking from AeState. Locking is already reported
from mode, though it's merged with manual.

> that among the supported AeState values there is locked. Bummer, we
> don't have the Camera::controls() equivalent for metadata, hence we
> don't have a way to report the values supported for metadata-only
> controls as AeState is
> 
> > +	staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > +				  aeLockAvailable);
> > +
> >  	int64_t minFrameDurationNsec = -1;
> >  	int64_t maxFrameDurationNsec = -1;
> >  	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);
> > @@ -946,10 +996,6 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	staticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
> >  				  sceneModesOverride);
> >
> > -	uint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;
> > -	staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > -				  aeLockAvailable);
> > -
> >  	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> >  	staticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> >  				  awbLockAvailable);
> > @@ -1358,6 +1404,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons
> >  	if (!manualTemplate)
> >  		return nullptr;
> >
> > +	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> > +	manualTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
> 
> Only if supported, or unconditionally ?

There's already a check for this. The manual sensor capability requires
AE off, and requestTemplateManual() checks that the manual sensor
capability is available.

> 
> > +
> >  	return manualTemplate;
> >  }
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ef4fbab8..d5027ec5 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -263,7 +263,9 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> >
> >  CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> >  	: id_(id), state_(State::Stopped), camera_(std::move(camera)),
> > -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> > +	  facing_(CAMERA_FACING_FRONT), orientation_(0),
> > +	  aeOn_(true), aeLocked_(false), lastExposureTime_(0),
> > +	  lastAnalogueGain_(1.0f), lastDigitalGain_(1.0f)
> >  {
> >  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> >
> > @@ -857,6 +859,62 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> >  		controls.set(controls::draft::TestPatternMode, testPatternMode);
> >  	}
> >
> > +	/*
> > +	 * \todo Can we trust that we won't receive values that we didn't
> > +	 * report supporting?
> > +	 */
> > +	if (settings.getEntry(ANDROID_CONTROL_AE_LOCK, &entry))
> > +		aeLocked_ = *entry.data.u8 != ANDROID_CONTROL_AE_LOCK_OFF;
> > +
> > +	if (settings.getEntry(ANDROID_CONTROL_AE_MODE, &entry))
> > +		aeOn_ = *entry.data.u8 != ANDROID_CONTROL_AE_MODE_OFF;
> > +
> > +	AeMode aeMode;
> > +	if (aeLocked_ && aeOn_)
> > +		aeMode = AeMode::Lock;
> > +	else if (aeLocked_ && !aeOn_)
> > +		aeMode = AeMode::Manual;
> > +	else if (!aeLocked_ && aeOn_)
> > +		aeMode = AeMode::Auto;
> > +	else /* !aeLocked_ && !aeOn_ */
> > +		aeMode = AeMode::Manual;
> 
>         if (!aeOn_)
>                 aeMode = Manual;
>         else if (aeLocked)
>                 aeMode = Lock;
>         else
>                 aeMode = Auto

Oh yeah that's simpler.

> > +
> > +	/* Save this so that we can recover it in the result */
> > +	descriptor->aeMode_ = aeMode;
> > +
> > +	const auto &eInfo = camera_->controls().find(&controls::ExposureTimeMode);
> 
> Doing so at every request is not nasty, but if we could avoid it it
> would nice. Don't we have a list of capabilities to rely on ?

We do, but again, does supporting manual sensor capability (= supporting
AE off) guarantee that the camera has both ExposureTime/Mode and
AnalogueGain/Mode?

> 
> > +	if (eInfo != camera_->controls().end()) {
> > +		controls.set(controls::ExposureTimeMode,
> > +				aeMode == AeMode::Auto ?
> > +				controls::ExposureTimeModeAuto :
> > +				controls::ExposureTimeModeDisabled);
> 
> I'll ask again, do we expect the same controls to be in every request
> even if they do not change ?

If the control value does not change, it doesn't need to be set again in
a new request, at least with the current status quo. In android it's
definitely a requirement. In libcamera, a lot (all?) of things don't
behave like this yet, though. This patch makes all the AE-related
controls behave like this, and it's especially required since it's the
HAL layer. That's why there's the ae state variable in the request
descriptor, and why there are new class variables.

> 
> > +	}
> > +
> > +	const auto &gInfo = camera_->controls().find(&controls::AnalogueGainMode);
> > +	if (gInfo != camera_->controls().end()) {
> > +		controls.set(controls::AnalogueGainMode,
> > +				aeMode == AeMode::Auto ?
> > +				controls::AnalogueGainModeAuto :
> > +				controls::AnalogueGainModeDisabled);
> > +	}
> > +
> > +	if (settings.getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry)) {
> > +		const auto &info = camera_->controls().find(&controls::ExposureTime);
> 
> Should we make sure at capabilities contruction time that if
> ExposureTimeMode supports Disabled, then ExposureTime is present ?
> If we assume so, and make it a requisite to claim we support
> AE_MODE_OFF then these controls should only be handled if
> capabilities.contain(MANUAL_SENSOR)

Yeah I think we should. Actually I think it should be mandated on the
libcamera side. (Do we have such mechanism yet?)

> 
> > +		if (info != camera_->controls().end()) {
> > +			lastExposureTime_ = (*entry.data.i64) / 1000;
> > +			/* Don't disable libcamera's internal AeMode::Lock */
> > +			if (aeMode != AeMode::Lock)
> 
> Do we expect a request to contain both AE_LOCK and a sensor exposure
> time ?

No. But don't we have to prepare for (or at least not break) if the user
decides to do so anyway?

> 
> > +				controls.set(controls::ExposureTime, lastExposureTime_);
> > +		}
> > +	}
> > +
> > +	/* Trigger libcamera's locked -> manual state change */
> > +	if (aeMode == AeMode::Manual && !settings.hasEntry(ANDROID_SENSOR_EXPOSURE_TIME)) {
> > +		const auto &info = camera_->controls().find(&controls::ExposureTime);
> > +		if (info != camera_->controls().end())
> > +			controls.set(controls::ExposureTime, lastExposureTime_);
> 
> If we don't need to repeat the control values in every request, is
> this necessary ? Or are you afraid we receive a AE_MODE_OFF and no

It's necessary to translate android's lock control to libcamera's new
manual-locked hidden state. As I commented:
/* Trigger libcamera's locked -> manual state change */

Because the Mode controls' internal state causes and exception to the
"don't need to repeat the control values every requrest" rule. It's even
documented in the control :D

> SENSOR_EXPOSURE_TIME ?
> 
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > @@ -1345,11 +1403,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >  	resultMetadata->addEntry(ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> >  				 value32);
> >
> > -	value = ANDROID_CONTROL_AE_LOCK_OFF;
> > -	resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, value);
> > +	uint8_t aeLock = (descriptor.aeMode_ == AeMode::Lock)
> > +		       ? ANDROID_CONTROL_AE_LOCK_ON
> > +		       : ANDROID_CONTROL_AE_LOCK_OFF;
> > +	resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, aeLock);
> >
> > -	value = ANDROID_CONTROL_AE_MODE_ON;
> > -	resultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, value);
> > +	/* Locked means auto + locked in android */
> > +	uint8_t aeMode = (descriptor.aeMode_ != AeMode::Manual)
> > +		       ? ANDROID_CONTROL_AE_MODE_ON
> > +		       : ANDROID_CONTROL_AE_MODE_OFF;
> > +	resultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
> >
> >  	if (settings.getEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, &entry))
> >  		/*
> > @@ -1366,6 +1429,31 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >  	resultMetadata->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, value);
> >
> >  	value = ANDROID_CONTROL_AE_STATE_CONVERGED;
> > +	if (metadata.contains(controls::AeState)) {
> > +		switch (metadata.get(controls::AeState)) {
> > +		case controls::AeStateInactive:
> > +			value = ANDROID_CONTROL_AE_STATE_INACTIVE;
> > +			break;
> > +		case controls::AeStateSearching:
> > +			value = ANDROID_CONTROL_AE_STATE_SEARCHING;
> > +			break;
> > +		case controls::AeStateConverged:
> > +			value = ANDROID_CONTROL_AE_STATE_CONVERGED;
> > +			break;
> > +		case controls::AeStateFlashRequired:
> > +			value = ANDROID_CONTROL_AE_STATE_FLASH_REQUIRED;
> > +			break;
> > +		case controls::AeStatePrecapture:
> > +			value = ANDROID_CONTROL_AE_STATE_PRECAPTURE;
> > +			break;
> > +		default:
> > +			if (descriptor.aeMode_ == AeMode::Lock) {
> > +				value = ANDROID_CONTROL_AE_STATE_LOCKED;
> > +				break;
> > +			}
> > +			LOG(HAL, Error) << "Invalid AeState, setting converged";
> > +		}
> > +	}
> >  	resultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, value);
> >
> >  	value = ANDROID_CONTROL_AF_MODE_OFF;
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index b7d774fe..f693cdbc 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -73,6 +73,12 @@ private:
> >
> >  	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> >
> > +	enum class AeMode {
> > +		Auto,
> > +		Lock,
> > +		Manual,
> > +	};
> > +
> >  	struct Camera3RequestDescriptor {
> >  		enum class Status {
> >  			Pending,
> > @@ -96,6 +102,9 @@ private:
> >
> >  		camera3_capture_result_t captureResult_ = {};
> >  		Status status_ = Status::Pending;
> > +
> > +		/* The libcamera internal AE state for this request */
> > +		AeMode aeMode_ = AeMode::Auto;
> >  	};
> >
> >  	enum class State {
> > @@ -146,6 +155,13 @@ private:
> >  	int facing_;
> >  	int orientation_;
> >
> > +	/* Track the last-set android AE controls */
> > +	bool aeOn_;
> > +	bool aeLocked_;
> 
> I guess this is again about if we have to repeat controls every
> request, otherwise they don't seem to be required as class members..

Yeah, exactly. These two specifically are for the android controls so
we don't get to decide.


Thanks,

Paul

> 
> > +	int32_t lastExposureTime_;
> > +	float lastAnalogueGain_;
> > +	float lastDigitalGain_;
> > +
> >  	CameraMetadata lastSettings_;
> >  };
> >
> > --
> > 2.27.0
> >
Jacopo Mondi Oct. 11, 2021, 12:15 p.m. UTC | #3
Hi Paul,

On Mon, Oct 04, 2021 at 05:52:57PM +0900, paul.elder@ideasonboard.com wrote:
> Hi Jacopo,
>
> On Fri, Oct 01, 2021 at 07:32:48PM +0200, Jacopo Mondi wrote:
> > Hi Paul,
> >
> > On Fri, Oct 01, 2021 at 07:33:24PM +0900, Paul Elder wrote:
> > > With the AE-related controls reorganized in libcamera, with separate
> > > value and lever controls for exposure and gain, plumb these through the
> > > HAL layer (in order of appearence in the patch):
> > > - static metadata: available AE modes, AE lock available
> > > - manual template: add AE off (the other templates already have AE on)
> > > - request metadata: HAL -> libcamera controls conversion
> > > - result metadata: libcamera -> HAL controls conversion
> > > - result metadata: AE state
> >
> > So I think they should go in separate patches maybe
>
> Hmmm... they might be easily separatable... they're all related though.
>
> >
> > >
> > > We add class variables to CameraDevice to save the last set android AE
> > > controls, as that is how android controls function (no need to resubmit
> > > controls if they are unchanged).
> >
> > Do we work any differently ? It's an honest question, I always assumed
> > we don't.
>
> That is how we should work afaik, but we currently don't (and I'm pretty
> sure pipeline handlers don't act that way either...).
>
> >
> > >
> > > We also save libcamera's AE state in the request descriptor, as
> > > otherwise there is insufficient information from libcamera's result
> > > metadata alone to tell if the state is locked or manual (as they are an
> > > internal state in libcamera).
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > ---
> > > New in v2
> > > ---
> > >  src/android/camera_capabilities.cpp | 63 ++++++++++++++++---
> > >  src/android/camera_device.cpp       | 98 +++++++++++++++++++++++++++--
> > >  src/android/camera_device.h         | 16 +++++
> > >  3 files changed, 165 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index f7a6cda9..3fed3f83 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -830,12 +830,62 @@ int CameraCapabilities::initializeStaticMetadata()
> > >  	staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> > >  				  aeAvailableAntiBandingModes);
> > >
> > > -	std::vector<uint8_t> aeAvailableModes = {
> > > -		ANDROID_CONTROL_AE_MODE_ON,
> > > -	};
> > > +	std::vector<uint8_t> aeAvailableModes;
> > > +	/* This will cause problems if one supports only on and the other only off */
> > > +	bool aeAddedOn = false;
> > > +	bool aeAddedOff = false;
> > > +
> > > +	const auto &analogGainModeInfo = controlsInfo.find(&controls::AnalogueGainMode);
> > > +	if (analogGainModeInfo != controlsInfo.end()) {
> > > +		for (const auto &value : analogGainModeInfo->second.values()) {
> > > +			switch (value.get<int32_t>()) {
> > > +			case controls::AnalogueGainModeAuto:
> > > +				if (!aeAddedOn)
> >
> > How can aeAddedOn be true if it's initialized to false and set to true
> > just here below, unless you expect the same value to be specified
> > twice in the info.values() ?
>
> Oh it was for consistency. Same code same shape :D
>
> >
> > Should you just add the value unconditionally here and below ?
>
> Yeah.
>
> >
> > > +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
> > > +				aeAddedOn = true;
> > > +				break;
> > > +			case controls::AnalogueGainModeDisabled:
> > > +				if (!aeAddedOff)
> > > +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);
> > > +				aeAddedOff = true;
> > > +				break;
> > > +			default:
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	const auto &exposureTimeModeInfo = controlsInfo.find(&controls::ExposureTimeMode);
> > > +	if (exposureTimeModeInfo != controlsInfo.end()) {
> > > +		for (const auto &value : exposureTimeModeInfo->second.values()) {
> > > +			switch (value.get<int32_t>()) {
> > > +			case controls::ExposureTimeModeAuto:
> > > +				if (!aeAddedOn)
> > > +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
> > > +				aeAddedOn = true;
> >
> > And setting the aeAdded to true here has no purpose, but that's not an
> > issue.
> >
> > What is the policy we want to have here ? Android has a single AE
> > control, we have two. I don't think we can allow situation where we
>
> Yeah that's exactly what I was wondering. What happens if the libcamera
> camera only has one of the two (is that possible?)? What if one is
> manual-able but the other isn't? Even worse, what if one is auto-only
> and the other is manual-only?
>
> So I wasn't sure what to do. I decided that at least one should be set
> to auto to enable auto, and at least one should be manual-able to
> consider AE manual-able, and at least one should be auto-able to
> consider AE auto-able.
>
> > have an asymmetric ExposureTimeMode and AnalogueGainMode as matching
> > on which one to add would become painful.
>
> Yeah it's painful. Does anybody know what we can mandate to simplify
> this? Can we require that if a camera supports one manual-able it must
> also support the other manualable? And the same for auto?
>
> >
> > I would cut it short and requires libcamera implementation that aims
> > to HAL full level to have both gain and exposure time controllable.
> >
> > The code can thus be restructured as
> >
> >         availableModes = { AE_MODE_ON };
> >         if (exposureTimeInfo != end && analogGainInfo != end) {
> >                 bool manualExp = false;
> >                 bool manualGain = false;
> >
> >                 for (modes : exposureTimeInfo.values())
> >                         if (mode == Disabled)
> >                                 manualExp = true;
> >                                 break;
> >
> >                for (modes : analogGainInfo.values())
> >                         if (mode == Disabled)
> >                                 manualGain = true;
> >                                 break
> >
> >                if (manualExp && manualGain)
> >                         availableModes.push_back(AE_MODE_OFF);
> >         }
>
> Yeah it would be nice, but we need to know that pipelines+IPAs *will*
> follow this behavior.
>

The ones that want to support fully controllable manual mode, and
consequentially wants to support Android's ManualSensor capabilities.

I think we can assume to claim we support AE_MODE_OFF is fair to
require to the ph to allow control of both exposure time and gain.

> > > +				break;
> > > +			case controls::ExposureTimeModeDisabled:
> > > +				if (!aeAddedOff)
> > > +					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);
> > > +				aeAddedOff = true;
> >
> >
> > > +				break;
> > > +			default:
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	if (aeAvailableModes.empty())
> > > +		aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
> > >  	staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > >  				  aeAvailableModes);
> > >
> > > +	/* In libcamera, turning off AE is equivalient to locking. */
> > > +	uint8_t aeLockAvailable = aeAddedOff ? ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE
> > > +					     : ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;
> >
> > This really depends on what we want to do with AeState.
> > I think if we want to report locking from there we should make sure
>
> I don't want to report locking from AeState. Locking is already reported
> from mode, though it's merged with manual.
>
> > that among the supported AeState values there is locked. Bummer, we
> > don't have the Camera::controls() equivalent for metadata, hence we
> > don't have a way to report the values supported for metadata-only
> > controls as AeState is
> >
> > > +	staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > +				  aeLockAvailable);
> > > +
> > >  	int64_t minFrameDurationNsec = -1;
> > >  	int64_t maxFrameDurationNsec = -1;
> > >  	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);
> > > @@ -946,10 +996,6 @@ int CameraCapabilities::initializeStaticMetadata()
> > >  	staticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
> > >  				  sceneModesOverride);
> > >
> > > -	uint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;
> > > -	staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > -				  aeLockAvailable);
> > > -
> > >  	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> > >  	staticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > >  				  awbLockAvailable);
> > > @@ -1358,6 +1404,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons
> > >  	if (!manualTemplate)
> > >  		return nullptr;
> > >
> > > +	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> > > +	manualTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
> >
> > Only if supported, or unconditionally ?
>
> There's already a check for this. The manual sensor capability requires
> AE off, and requestTemplateManual() checks that the manual sensor
> capability is available.
>
> >
> > > +
> > >  	return manualTemplate;
> > >  }
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index ef4fbab8..d5027ec5 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -263,7 +263,9 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> > >
> > >  CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> > >  	: id_(id), state_(State::Stopped), camera_(std::move(camera)),
> > > -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> > > +	  facing_(CAMERA_FACING_FRONT), orientation_(0),
> > > +	  aeOn_(true), aeLocked_(false), lastExposureTime_(0),
> > > +	  lastAnalogueGain_(1.0f), lastDigitalGain_(1.0f)
> > >  {
> > >  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> > >
> > > @@ -857,6 +859,62 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> > >  		controls.set(controls::draft::TestPatternMode, testPatternMode);
> > >  	}
> > >
> > > +	/*
> > > +	 * \todo Can we trust that we won't receive values that we didn't
> > > +	 * report supporting?
> > > +	 */
> > > +	if (settings.getEntry(ANDROID_CONTROL_AE_LOCK, &entry))
> > > +		aeLocked_ = *entry.data.u8 != ANDROID_CONTROL_AE_LOCK_OFF;
> > > +
> > > +	if (settings.getEntry(ANDROID_CONTROL_AE_MODE, &entry))
> > > +		aeOn_ = *entry.data.u8 != ANDROID_CONTROL_AE_MODE_OFF;
> > > +
> > > +	AeMode aeMode;
> > > +	if (aeLocked_ && aeOn_)
> > > +		aeMode = AeMode::Lock;
> > > +	else if (aeLocked_ && !aeOn_)
> > > +		aeMode = AeMode::Manual;
> > > +	else if (!aeLocked_ && aeOn_)
> > > +		aeMode = AeMode::Auto;
> > > +	else /* !aeLocked_ && !aeOn_ */
> > > +		aeMode = AeMode::Manual;
> >
> >         if (!aeOn_)
> >                 aeMode = Manual;
> >         else if (aeLocked)
> >                 aeMode = Lock;
> >         else
> >                 aeMode = Auto
>
> Oh yeah that's simpler.
>
> > > +
> > > +	/* Save this so that we can recover it in the result */
> > > +	descriptor->aeMode_ = aeMode;
> > > +
> > > +	const auto &eInfo = camera_->controls().find(&controls::ExposureTimeMode);
> >
> > Doing so at every request is not nasty, but if we could avoid it it
> > would nice. Don't we have a list of capabilities to rely on ?
>
> We do, but again, does supporting manual sensor capability (= supporting
> AE off) guarantee that the camera has both ExposureTime/Mode and
> AnalogueGain/Mode?
>

I think we should require it and simplify the whole procedure here
by assuming that if we list ManualSensor in the capabilities, the
camera supports both ExposureTimeMode = Disabled and AnalogueGainMode
= Disabled.


> >
> > > +	if (eInfo != camera_->controls().end()) {
> > > +		controls.set(controls::ExposureTimeMode,
> > > +				aeMode == AeMode::Auto ?
> > > +				controls::ExposureTimeModeAuto :
> > > +				controls::ExposureTimeModeDisabled);
> >
> > I'll ask again, do we expect the same controls to be in every request
> > even if they do not change ?
>
> If the control value does not change, it doesn't need to be set again in
> a new request, at least with the current status quo. In android it's
> definitely a requirement. In libcamera, a lot (all?) of things don't
> behave like this yet, though. This patch makes all the AE-related

Which controls assumes to be set at every request ?

> controls behave like this, and it's especially required since it's the
> HAL layer. That's why there's the ae state variable in the request
> descriptor, and why there are new class variables.
>
> >
> > > +	}
> > > +
> > > +	const auto &gInfo = camera_->controls().find(&controls::AnalogueGainMode);
> > > +	if (gInfo != camera_->controls().end()) {
> > > +		controls.set(controls::AnalogueGainMode,
> > > +				aeMode == AeMode::Auto ?
> > > +				controls::AnalogueGainModeAuto :
> > > +				controls::AnalogueGainModeDisabled);
> > > +	}
> > > +
> > > +	if (settings.getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry)) {
> > > +		const auto &info = camera_->controls().find(&controls::ExposureTime);
> >
> > Should we make sure at capabilities contruction time that if
> > ExposureTimeMode supports Disabled, then ExposureTime is present ?
> > If we assume so, and make it a requisite to claim we support
> > AE_MODE_OFF then these controls should only be handled if
> > capabilities.contain(MANUAL_SENSOR)
>
> Yeah I think we should. Actually I think it should be mandated on the
> libcamera side. (Do we have such mechanism yet?)
>

Not for libcamera, lc-compliance might help, but yes, this should be enforced.

> >
> > > +		if (info != camera_->controls().end()) {
> > > +			lastExposureTime_ = (*entry.data.i64) / 1000;
> > > +			/* Don't disable libcamera's internal AeMode::Lock */
> > > +			if (aeMode != AeMode::Lock)
> >
> > Do we expect a request to contain both AE_LOCK and a sensor exposure
> > time ?
>
> No. But don't we have to prepare for (or at least not break) if the user
> decides to do so anyway?
>

I think it would be very hard to protect against all the possible
mis-use of controls an application can submit to libcamera. Android
doesn't clearly define dependencies between control not precendences,
but I think it's fair to assume the combination is not valid and we
can decide to act as we please (ignore one or ignore both ?) as far as
CTS does not complain.

> >
> > > +				controls.set(controls::ExposureTime, lastExposureTime_);
> > > +		}
> > > +	}
> > > +
> > > +	/* Trigger libcamera's locked -> manual state change */
> > > +	if (aeMode == AeMode::Manual && !settings.hasEntry(ANDROID_SENSOR_EXPOSURE_TIME)) {
> > > +		const auto &info = camera_->controls().find(&controls::ExposureTime);
> > > +		if (info != camera_->controls().end())
> > > +			controls.set(controls::ExposureTime, lastExposureTime_);
> >
> > If we don't need to repeat the control values in every request, is
> > this necessary ? Or are you afraid we receive a AE_MODE_OFF and no
>
> It's necessary to translate android's lock control to libcamera's new
> manual-locked hidden state. As I commented:
> /* Trigger libcamera's locked -> manual state change */
>
> Because the Mode controls' internal state causes and exception to the
> "don't need to repeat the control values every requrest" rule. It's even
> documented in the control :D

What I mean is why you need to submit a cached exposure time if the
android request does not provide one. If it's not updated by the HAL
client (the app) we don't update it either. After all if the app
switches to manual mode but does not provide an exposure time, it's an
ill-formed request, right ? So I would rather keep the AEGC locked
instead of using a cached value.

Thanks
  j

>
> > SENSOR_EXPOSURE_TIME ?
> >
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -1345,11 +1403,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> > >  	resultMetadata->addEntry(ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > >  				 value32);
> > >
> > > -	value = ANDROID_CONTROL_AE_LOCK_OFF;
> > > -	resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, value);
> > > +	uint8_t aeLock = (descriptor.aeMode_ == AeMode::Lock)
> > > +		       ? ANDROID_CONTROL_AE_LOCK_ON
> > > +		       : ANDROID_CONTROL_AE_LOCK_OFF;
> > > +	resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, aeLock);
> > >
> > > -	value = ANDROID_CONTROL_AE_MODE_ON;
> > > -	resultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, value);
> > > +	/* Locked means auto + locked in android */
> > > +	uint8_t aeMode = (descriptor.aeMode_ != AeMode::Manual)
> > > +		       ? ANDROID_CONTROL_AE_MODE_ON
> > > +		       : ANDROID_CONTROL_AE_MODE_OFF;
> > > +	resultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
> > >
> > >  	if (settings.getEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, &entry))
> > >  		/*
> > > @@ -1366,6 +1429,31 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> > >  	resultMetadata->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, value);
> > >
> > >  	value = ANDROID_CONTROL_AE_STATE_CONVERGED;
> > > +	if (metadata.contains(controls::AeState)) {
> > > +		switch (metadata.get(controls::AeState)) {
> > > +		case controls::AeStateInactive:
> > > +			value = ANDROID_CONTROL_AE_STATE_INACTIVE;
> > > +			break;
> > > +		case controls::AeStateSearching:
> > > +			value = ANDROID_CONTROL_AE_STATE_SEARCHING;
> > > +			break;
> > > +		case controls::AeStateConverged:
> > > +			value = ANDROID_CONTROL_AE_STATE_CONVERGED;
> > > +			break;
> > > +		case controls::AeStateFlashRequired:
> > > +			value = ANDROID_CONTROL_AE_STATE_FLASH_REQUIRED;
> > > +			break;
> > > +		case controls::AeStatePrecapture:
> > > +			value = ANDROID_CONTROL_AE_STATE_PRECAPTURE;
> > > +			break;
> > > +		default:
> > > +			if (descriptor.aeMode_ == AeMode::Lock) {
> > > +				value = ANDROID_CONTROL_AE_STATE_LOCKED;
> > > +				break;
> > > +			}
> > > +			LOG(HAL, Error) << "Invalid AeState, setting converged";
> > > +		}
> > > +	}
> > >  	resultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, value);
> > >
> > >  	value = ANDROID_CONTROL_AF_MODE_OFF;
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index b7d774fe..f693cdbc 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -73,6 +73,12 @@ private:
> > >
> > >  	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> > >
> > > +	enum class AeMode {
> > > +		Auto,
> > > +		Lock,
> > > +		Manual,
> > > +	};
> > > +
> > >  	struct Camera3RequestDescriptor {
> > >  		enum class Status {
> > >  			Pending,
> > > @@ -96,6 +102,9 @@ private:
> > >
> > >  		camera3_capture_result_t captureResult_ = {};
> > >  		Status status_ = Status::Pending;
> > > +
> > > +		/* The libcamera internal AE state for this request */
> > > +		AeMode aeMode_ = AeMode::Auto;
> > >  	};
> > >
> > >  	enum class State {
> > > @@ -146,6 +155,13 @@ private:
> > >  	int facing_;
> > >  	int orientation_;
> > >
> > > +	/* Track the last-set android AE controls */
> > > +	bool aeOn_;
> > > +	bool aeLocked_;
> >
> > I guess this is again about if we have to repeat controls every
> > request, otherwise they don't seem to be required as class members..
>
> Yeah, exactly. These two specifically are for the android controls so
> we don't get to decide.
>
>
> Thanks,
>
> Paul
>
> >
> > > +	int32_t lastExposureTime_;
> > > +	float lastAnalogueGain_;
> > > +	float lastDigitalGain_;
> > > +
> > >  	CameraMetadata lastSettings_;
> > >  };
> > >
> > > --
> > > 2.27.0
> > >

Patch
diff mbox series

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index f7a6cda9..3fed3f83 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -830,12 +830,62 @@  int CameraCapabilities::initializeStaticMetadata()
 	staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
 				  aeAvailableAntiBandingModes);
 
-	std::vector<uint8_t> aeAvailableModes = {
-		ANDROID_CONTROL_AE_MODE_ON,
-	};
+	std::vector<uint8_t> aeAvailableModes;
+	/* This will cause problems if one supports only on and the other only off */
+	bool aeAddedOn = false;
+	bool aeAddedOff = false;
+
+	const auto &analogGainModeInfo = controlsInfo.find(&controls::AnalogueGainMode);
+	if (analogGainModeInfo != controlsInfo.end()) {
+		for (const auto &value : analogGainModeInfo->second.values()) {
+			switch (value.get<int32_t>()) {
+			case controls::AnalogueGainModeAuto:
+				if (!aeAddedOn)
+					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
+				aeAddedOn = true;
+				break;
+			case controls::AnalogueGainModeDisabled:
+				if (!aeAddedOff)
+					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);
+				aeAddedOff = true;
+				break;
+			default:
+				break;
+			}
+		}
+	}
+
+	const auto &exposureTimeModeInfo = controlsInfo.find(&controls::ExposureTimeMode);
+	if (exposureTimeModeInfo != controlsInfo.end()) {
+		for (const auto &value : exposureTimeModeInfo->second.values()) {
+			switch (value.get<int32_t>()) {
+			case controls::ExposureTimeModeAuto:
+				if (!aeAddedOn)
+					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
+				aeAddedOn = true;
+				break;
+			case controls::ExposureTimeModeDisabled:
+				if (!aeAddedOff)
+					aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);
+				aeAddedOff = true;
+				break;
+			default:
+				break;
+			}
+		}
+	}
+
+	if (aeAvailableModes.empty())
+		aeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);
 	staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,
 				  aeAvailableModes);
 
+	/* In libcamera, turning off AE is equivalient to locking. */
+	uint8_t aeLockAvailable = aeAddedOff ? ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE
+					     : ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;
+	staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
+				  aeLockAvailable);
+
 	int64_t minFrameDurationNsec = -1;
 	int64_t maxFrameDurationNsec = -1;
 	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);
@@ -946,10 +996,6 @@  int CameraCapabilities::initializeStaticMetadata()
 	staticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
 				  sceneModesOverride);
 
-	uint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;
-	staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
-				  aeLockAvailable);
-
 	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
 	staticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
 				  awbLockAvailable);
@@ -1358,6 +1404,9 @@  std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons
 	if (!manualTemplate)
 		return nullptr;
 
+	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
+	manualTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
+
 	return manualTemplate;
 }
 
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index ef4fbab8..d5027ec5 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -263,7 +263,9 @@  CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
 
 CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
 	: id_(id), state_(State::Stopped), camera_(std::move(camera)),
-	  facing_(CAMERA_FACING_FRONT), orientation_(0)
+	  facing_(CAMERA_FACING_FRONT), orientation_(0),
+	  aeOn_(true), aeLocked_(false), lastExposureTime_(0),
+	  lastAnalogueGain_(1.0f), lastDigitalGain_(1.0f)
 {
 	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
 
@@ -857,6 +859,62 @@  int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
 		controls.set(controls::draft::TestPatternMode, testPatternMode);
 	}
 
+	/*
+	 * \todo Can we trust that we won't receive values that we didn't
+	 * report supporting?
+	 */
+	if (settings.getEntry(ANDROID_CONTROL_AE_LOCK, &entry))
+		aeLocked_ = *entry.data.u8 != ANDROID_CONTROL_AE_LOCK_OFF;
+
+	if (settings.getEntry(ANDROID_CONTROL_AE_MODE, &entry))
+		aeOn_ = *entry.data.u8 != ANDROID_CONTROL_AE_MODE_OFF;
+
+	AeMode aeMode;
+	if (aeLocked_ && aeOn_)
+		aeMode = AeMode::Lock;
+	else if (aeLocked_ && !aeOn_)
+		aeMode = AeMode::Manual;
+	else if (!aeLocked_ && aeOn_)
+		aeMode = AeMode::Auto;
+	else /* !aeLocked_ && !aeOn_ */
+		aeMode = AeMode::Manual;
+
+	/* Save this so that we can recover it in the result */
+	descriptor->aeMode_ = aeMode;
+
+	const auto &eInfo = camera_->controls().find(&controls::ExposureTimeMode);
+	if (eInfo != camera_->controls().end()) {
+		controls.set(controls::ExposureTimeMode,
+				aeMode == AeMode::Auto ?
+				controls::ExposureTimeModeAuto :
+				controls::ExposureTimeModeDisabled);
+	}
+
+	const auto &gInfo = camera_->controls().find(&controls::AnalogueGainMode);
+	if (gInfo != camera_->controls().end()) {
+		controls.set(controls::AnalogueGainMode,
+				aeMode == AeMode::Auto ?
+				controls::AnalogueGainModeAuto :
+				controls::AnalogueGainModeDisabled);
+	}
+
+	if (settings.getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry)) {
+		const auto &info = camera_->controls().find(&controls::ExposureTime);
+		if (info != camera_->controls().end()) {
+			lastExposureTime_ = (*entry.data.i64) / 1000;
+			/* Don't disable libcamera's internal AeMode::Lock */
+			if (aeMode != AeMode::Lock)
+				controls.set(controls::ExposureTime, lastExposureTime_);
+		}
+	}
+
+	/* Trigger libcamera's locked -> manual state change */
+	if (aeMode == AeMode::Manual && !settings.hasEntry(ANDROID_SENSOR_EXPOSURE_TIME)) {
+		const auto &info = camera_->controls().find(&controls::ExposureTime);
+		if (info != camera_->controls().end())
+			controls.set(controls::ExposureTime, lastExposureTime_);
+	}
+
 	return 0;
 }
 
@@ -1345,11 +1403,16 @@  CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
 	resultMetadata->addEntry(ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
 				 value32);
 
-	value = ANDROID_CONTROL_AE_LOCK_OFF;
-	resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, value);
+	uint8_t aeLock = (descriptor.aeMode_ == AeMode::Lock)
+		       ? ANDROID_CONTROL_AE_LOCK_ON
+		       : ANDROID_CONTROL_AE_LOCK_OFF;
+	resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, aeLock);
 
-	value = ANDROID_CONTROL_AE_MODE_ON;
-	resultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, value);
+	/* Locked means auto + locked in android */
+	uint8_t aeMode = (descriptor.aeMode_ != AeMode::Manual)
+		       ? ANDROID_CONTROL_AE_MODE_ON
+		       : ANDROID_CONTROL_AE_MODE_OFF;
+	resultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
 
 	if (settings.getEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, &entry))
 		/*
@@ -1366,6 +1429,31 @@  CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
 	resultMetadata->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, value);
 
 	value = ANDROID_CONTROL_AE_STATE_CONVERGED;
+	if (metadata.contains(controls::AeState)) {
+		switch (metadata.get(controls::AeState)) {
+		case controls::AeStateInactive:
+			value = ANDROID_CONTROL_AE_STATE_INACTIVE;
+			break;
+		case controls::AeStateSearching:
+			value = ANDROID_CONTROL_AE_STATE_SEARCHING;
+			break;
+		case controls::AeStateConverged:
+			value = ANDROID_CONTROL_AE_STATE_CONVERGED;
+			break;
+		case controls::AeStateFlashRequired:
+			value = ANDROID_CONTROL_AE_STATE_FLASH_REQUIRED;
+			break;
+		case controls::AeStatePrecapture:
+			value = ANDROID_CONTROL_AE_STATE_PRECAPTURE;
+			break;
+		default:
+			if (descriptor.aeMode_ == AeMode::Lock) {
+				value = ANDROID_CONTROL_AE_STATE_LOCKED;
+				break;
+			}
+			LOG(HAL, Error) << "Invalid AeState, setting converged";
+		}
+	}
 	resultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, value);
 
 	value = ANDROID_CONTROL_AF_MODE_OFF;
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index b7d774fe..f693cdbc 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -73,6 +73,12 @@  private:
 
 	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
 
+	enum class AeMode {
+		Auto,
+		Lock,
+		Manual,
+	};
+
 	struct Camera3RequestDescriptor {
 		enum class Status {
 			Pending,
@@ -96,6 +102,9 @@  private:
 
 		camera3_capture_result_t captureResult_ = {};
 		Status status_ = Status::Pending;
+
+		/* The libcamera internal AE state for this request */
+		AeMode aeMode_ = AeMode::Auto;
 	};
 
 	enum class State {
@@ -146,6 +155,13 @@  private:
 	int facing_;
 	int orientation_;
 
+	/* Track the last-set android AE controls */
+	bool aeOn_;
+	bool aeLocked_;
+	int32_t lastExposureTime_;
+	float lastAnalogueGain_;
+	float lastDigitalGain_;
+
 	CameraMetadata lastSettings_;
 };