[libcamera-devel] [PATCH v2 6/7] android: Plumb all AE-related controls
Jacopo Mondi
jacopo at jmondi.org
Fri Oct 1 19:32:48 CEST 2021
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 at 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
>
More information about the libcamera-devel
mailing list