| Message ID | 20211001103325.1077590-7-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 > >
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 > > >
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_; };
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(-)