| Message ID | 20230713151218.26045-3-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David On Thu, Jul 13, 2023 at 04:12:18PM +0100, David Plowman via libcamera-devel wrote: > We handle the flicker modes by passing the correct period to the > AEC/AGC algorithm which already contains the necessary code. > > The "Auto" mode, as well as reporting the detected flicker period via If so you shouldn't register { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, otherwise AeFlickerMode::FlickerAuto will be exposed to applications ? > the "AeFlickerDetected" metadata, are unsupported for now. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++- > src/ipa/rpi/common/ipa_base.h | 6 ++++ > 2 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index f40f2e71..81d65ea5 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{ > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) }, > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > @@ -97,7 +99,7 @@ namespace ipa::RPi { > > IpaBase::IpaBase() > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), > - firstStart_(true) > + firstStart_(true), flickerState_({ 0, 0s }) > { > } > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls) > break; > } > > + case controls::AE_FLICKER_MODE: { > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > + controller_.getAlgorithm("agc")); > + if (!agc) { > + LOG(IPARPI, Warning) > + << "Could not set AeFlickerMode - no AGC algorithm"; > + break; > + } > + > + int32_t mode = ctrl.second.get<int32_t>(); > + bool modeValid = true; > + > + switch (mode) { > + case controls::FlickerOff: { > + agc->setFlickerPeriod(0us); > + > + break; > + } Do you need braces ? > + > + case controls::FlickerCustom: { > + agc->setFlickerPeriod(flickerState_.customPeriod); > + > + break; > + } > + > + default: > + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported"; > + modeValid = false; > + > + break; > + } > + > + if (modeValid) > + flickerState_.mode = mode; > + > + break; > + } > + > + case controls::AE_FLICKER_CUSTOM: { > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > + controller_.getAlgorithm("agc")); > + if (!agc) { > + LOG(IPARPI, Warning) > + << "Could not set AeFlickerCustom - no AGC algorithm"; > + break; > + } > + > + uint32_t customPeriod = ctrl.second.get<int32_t>(); > + flickerState_.customPeriod = customPeriod * 1.0us; > + > + /* > + * We note that it makes no difference if the mode gets set to "custom" > + * first, and the period updated after, or vice versa. > + */ True, but what if the app never provides a value for controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to some sensible default ? > + if (flickerState_.mode == controls::FlickerCustom) > + agc->setFlickerPeriod(flickerState_.customPeriod); > + > + break; > + } > + > case controls::AWB_ENABLE: { > /* Silently ignore this control for a mono sensor. */ > if (monoSensor_) > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > index 39d00760..22823176 100644 > --- a/src/ipa/rpi/common/ipa_base.h > +++ b/src/ipa/rpi/common/ipa_base.h > @@ -116,6 +116,12 @@ private: > /* Frame duration (1/fps) limits. */ > utils::Duration minFrameDuration_; > utils::Duration maxFrameDuration_; > + > + /* The current state of flicker avoidance. */ > + struct FlickerState { > + int32_t mode; > + utils::Duration customPeriod; > + } flickerState_; > }; > > } /* namespace ipa::RPi */ > -- > 2.30.2 >
Hi Jacopo Thanks for the comments. On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Thu, Jul 13, 2023 at 04:12:18PM +0100, David Plowman via libcamera-devel wrote: > > We handle the flicker modes by passing the correct period to the > > AEC/AGC algorithm which already contains the necessary code. > > > > The "Auto" mode, as well as reporting the detected flicker period via > > If so you shouldn't register > > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ? Indeed, it might be better not to expose the auto value! > > > > the "AeFlickerDetected" metadata, are unsupported for now. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++- > > src/ipa/rpi/common/ipa_base.h | 6 ++++ > > 2 files changed, 69 insertions(+), 1 deletion(-) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index f40f2e71..81d65ea5 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{ > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) }, > > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > @@ -97,7 +99,7 @@ namespace ipa::RPi { > > > > IpaBase::IpaBase() > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), > > - firstStart_(true) > > + firstStart_(true), flickerState_({ 0, 0s }) > > { > > } > > > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls) > > break; > > } > > > > + case controls::AE_FLICKER_MODE: { > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > + controller_.getAlgorithm("agc")); > > + if (!agc) { > > + LOG(IPARPI, Warning) > > + << "Could not set AeFlickerMode - no AGC algorithm"; > > + break; > > + } > > + > > + int32_t mode = ctrl.second.get<int32_t>(); > > + bool modeValid = true; > > + > > + switch (mode) { > > + case controls::FlickerOff: { > > + agc->setFlickerPeriod(0us); > > + > > + break; > > + } > > Do you need braces ? Sure, I can remove them! > > > + > > + case controls::FlickerCustom: { > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > + > > + break; > > + } > > + > > + default: > > + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported"; > > + modeValid = false; > > + > > + break; > > + } > > + > > + if (modeValid) > > + flickerState_.mode = mode; > > + > > + break; > > + } > > + > > + case controls::AE_FLICKER_CUSTOM: { > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > + controller_.getAlgorithm("agc")); > > + if (!agc) { > > + LOG(IPARPI, Warning) > > + << "Could not set AeFlickerCustom - no AGC algorithm"; > > + break; > > + } > > + > > + uint32_t customPeriod = ctrl.second.get<int32_t>(); > > + flickerState_.customPeriod = customPeriod * 1.0us; > > + > > + /* > > + * We note that it makes no difference if the mode gets set to "custom" > > + * first, and the period updated after, or vice versa. > > + */ > > True, but what if the app never provides a value for > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to > some sensible default ? In our implementation we use the value 0 to mean "no flicker cancellection", so setting the mode to custom/manual without a flicker period will cause it to do nothing (until you set one). I'm OK with this behaviour unless anyone objects, though I don't feel strongly! I'd rather document that setting custom/manual mode without a period leads to undefined behaviour rather than mandating a particular value, I think, as not every PH may wish to do the same. What do you think? Thanks! David > > > + if (flickerState_.mode == controls::FlickerCustom) > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > + > > + break; > > + } > > + > > case controls::AWB_ENABLE: { > > /* Silently ignore this control for a mono sensor. */ > > if (monoSensor_) > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > index 39d00760..22823176 100644 > > --- a/src/ipa/rpi/common/ipa_base.h > > +++ b/src/ipa/rpi/common/ipa_base.h > > @@ -116,6 +116,12 @@ private: > > /* Frame duration (1/fps) limits. */ > > utils::Duration minFrameDuration_; > > utils::Duration maxFrameDuration_; > > + > > + /* The current state of flicker avoidance. */ > > + struct FlickerState { > > + int32_t mode; > > + utils::Duration customPeriod; > > + } flickerState_; > > }; > > > > } /* namespace ipa::RPi */ > > -- > > 2.30.2 > >
Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24) > Hi Jacopo > > Thanks for the comments. > > On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi David > > > > On Thu, Jul 13, 2023 at 04:12:18PM +0100, David Plowman via libcamera-devel wrote: > > > We handle the flicker modes by passing the correct period to the > > > AEC/AGC algorithm which already contains the necessary code. > > > > > > The "Auto" mode, as well as reporting the detected flicker period via > > > > If so you shouldn't register > > > > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ? > > Indeed, it might be better not to expose the auto value! > > > > > > > > the "AeFlickerDetected" metadata, are unsupported for now. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++- > > > src/ipa/rpi/common/ipa_base.h | 6 ++++ > > > 2 files changed, 69 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > index f40f2e71..81d65ea5 100644 > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{ > > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) }, > > > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > > @@ -97,7 +99,7 @@ namespace ipa::RPi { > > > > > > IpaBase::IpaBase() > > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), > > > - firstStart_(true) > > > + firstStart_(true), flickerState_({ 0, 0s }) > > > { > > > } > > > > > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls) > > > break; > > > } > > > > > > + case controls::AE_FLICKER_MODE: { > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > + controller_.getAlgorithm("agc")); > > > + if (!agc) { > > > + LOG(IPARPI, Warning) > > > + << "Could not set AeFlickerMode - no AGC algorithm"; > > > + break; > > > + } > > > + > > > + int32_t mode = ctrl.second.get<int32_t>(); > > > + bool modeValid = true; > > > + > > > + switch (mode) { > > > + case controls::FlickerOff: { > > > + agc->setFlickerPeriod(0us); > > > + > > > + break; > > > + } > > > > Do you need braces ? > > Sure, I can remove them! > > > > > > + > > > + case controls::FlickerCustom: { > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > + > > > + break; > > > + } > > > + > > > + default: > > > + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported"; > > > + modeValid = false; > > > + > > > + break; > > > + } > > > + > > > + if (modeValid) > > > + flickerState_.mode = mode; > > > + > > > + break; > > > + } > > > + > > > + case controls::AE_FLICKER_CUSTOM: { > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > + controller_.getAlgorithm("agc")); > > > + if (!agc) { > > > + LOG(IPARPI, Warning) > > > + << "Could not set AeFlickerCustom - no AGC algorithm"; > > > + break; > > > + } > > > + > > > + uint32_t customPeriod = ctrl.second.get<int32_t>(); > > > + flickerState_.customPeriod = customPeriod * 1.0us; > > > + > > > + /* > > > + * We note that it makes no difference if the mode gets set to "custom" > > > + * first, and the period updated after, or vice versa. > > > + */ > > > > True, but what if the app never provides a value for > > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to > > some sensible default ? > > In our implementation we use the value 0 to mean "no flicker > cancellection", so setting the mode to custom/manual without a flicker > period will cause it to do nothing (until you set one). I'm OK with > this behaviour unless anyone objects, though I don't feel strongly! > I'd rather document that setting custom/manual mode without a period > leads to undefined behaviour rather than mandating a particular value, > I think, as not every PH may wish to do the same. What do you think? I had wondered if we could get this down to a single control by having '0' as no flicker cancellation - but we'd have no way to express 'auto' so I think we do still need two, and it lets us report in the metadata if the value was determined automatically or explicitly I guess? (Though the app should know that anyway as it would know if it set the manual value?) Throwing paint on the bike shed I feel like 'AeFlickerPeriod' would be better suited for the control name as then it would be usable to report the period back in the metadata when it's detected with 'Auto' mode. What limits will be imposed for the period ? I think setting AeFlickerMode to 'Custom' (or 'Manual'?) and not setting an {AeFlickerCustom,AeFlickerPeriod} control should be the same as not changing anything, but I expect that maybe the ControlInfo might have limits that would preclude setting a period of '0' ? It might be reasonable to say setting AeFlickerMode to Manual/Custom will only take effect when the corresponding AeFlickerPeriod control is applied ? -- Kieran > > Thanks! > David > > > > > > + if (flickerState_.mode == controls::FlickerCustom) > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > + > > > + break; > > > + } > > > + > > > case controls::AWB_ENABLE: { > > > /* Silently ignore this control for a mono sensor. */ > > > if (monoSensor_) > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > index 39d00760..22823176 100644 > > > --- a/src/ipa/rpi/common/ipa_base.h > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > @@ -116,6 +116,12 @@ private: > > > /* Frame duration (1/fps) limits. */ > > > utils::Duration minFrameDuration_; > > > utils::Duration maxFrameDuration_; > > > + > > > + /* The current state of flicker avoidance. */ > > > + struct FlickerState { > > > + int32_t mode; > > > + utils::Duration customPeriod; > > > + } flickerState_; > > > }; > > > > > > } /* namespace ipa::RPi */ > > > -- > > > 2.30.2 > > >
Hi Kieran On Tue, 18 Jul 2023 at 09:48, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24) > > Hi Jacopo > > > > Thanks for the comments. > > > > On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi David > > > > > > On Thu, Jul 13, 2023 at 04:12:18PM +0100, David Plowman via libcamera-devel wrote: > > > > We handle the flicker modes by passing the correct period to the > > > > AEC/AGC algorithm which already contains the necessary code. > > > > > > > > The "Auto" mode, as well as reporting the detected flicker period via > > > > > > If so you shouldn't register > > > > > > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > > > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ? > > > > Indeed, it might be better not to expose the auto value! > > > > > > > > > > > > the "AeFlickerDetected" metadata, are unsupported for now. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++- > > > > src/ipa/rpi/common/ipa_base.h | 6 ++++ > > > > 2 files changed, 69 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > index f40f2e71..81d65ea5 100644 > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{ > > > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > > > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > > > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > > > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) }, > > > > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > > > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, > > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > > > @@ -97,7 +99,7 @@ namespace ipa::RPi { > > > > > > > > IpaBase::IpaBase() > > > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), > > > > - firstStart_(true) > > > > + firstStart_(true), flickerState_({ 0, 0s }) > > > > { > > > > } > > > > > > > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls) > > > > break; > > > > } > > > > > > > > + case controls::AE_FLICKER_MODE: { > > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > > + controller_.getAlgorithm("agc")); > > > > + if (!agc) { > > > > + LOG(IPARPI, Warning) > > > > + << "Could not set AeFlickerMode - no AGC algorithm"; > > > > + break; > > > > + } > > > > + > > > > + int32_t mode = ctrl.second.get<int32_t>(); > > > > + bool modeValid = true; > > > > + > > > > + switch (mode) { > > > > + case controls::FlickerOff: { > > > > + agc->setFlickerPeriod(0us); > > > > + > > > > + break; > > > > + } > > > > > > Do you need braces ? > > > > Sure, I can remove them! > > > > > > > > > + > > > > + case controls::FlickerCustom: { > > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > > + > > > > + break; > > > > + } > > > > + > > > > + default: > > > > + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported"; > > > > + modeValid = false; > > > > + > > > > + break; > > > > + } > > > > + > > > > + if (modeValid) > > > > + flickerState_.mode = mode; > > > > + > > > > + break; > > > > + } > > > > + > > > > + case controls::AE_FLICKER_CUSTOM: { > > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > > + controller_.getAlgorithm("agc")); > > > > + if (!agc) { > > > > + LOG(IPARPI, Warning) > > > > + << "Could not set AeFlickerCustom - no AGC algorithm"; > > > > + break; > > > > + } > > > > + > > > > + uint32_t customPeriod = ctrl.second.get<int32_t>(); > > > > + flickerState_.customPeriod = customPeriod * 1.0us; > > > > + > > > > + /* > > > > + * We note that it makes no difference if the mode gets set to "custom" > > > > + * first, and the period updated after, or vice versa. > > > > + */ > > > > > > True, but what if the app never provides a value for > > > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to > > > some sensible default ? > > > > In our implementation we use the value 0 to mean "no flicker > > cancellection", so setting the mode to custom/manual without a flicker > > period will cause it to do nothing (until you set one). I'm OK with > > this behaviour unless anyone objects, though I don't feel strongly! > > I'd rather document that setting custom/manual mode without a period > > leads to undefined behaviour rather than mandating a particular value, > > I think, as not every PH may wish to do the same. What do you think? > > > I had wondered if we could get this down to a single control by having > '0' as no flicker cancellation - but we'd have no way to express 'auto' > so I think we do still need two, and it lets us report in the metadata > if the value was determined automatically or explicitly I guess? (Though > the app should know that anyway as it would know if it set the manual > value?) Yes, I think we do still want a separate control. > > Throwing paint on the bike shed I feel like 'AeFlickerPeriod' would be > better suited for the control name as then it would be usable to report > the period back in the metadata when it's detected with 'Auto' mode. I agree that 'AeFlickerPeriod' is a better name, particularly if the 'custom' mode becomes the 'manual' mode. > > What limits will be imposed for the period ? I think setting > AeFlickerMode to 'Custom' (or 'Manual'?) and not setting an > {AeFlickerCustom,AeFlickerPeriod} control should be the same as not > changing anything, but I expect that maybe the ControlInfo might have > limits that would preclude setting a period of '0' ? I guess implementations get to choose their own limits though, for the most part, I see no reason not to allow them to be very wide. Extremely short or extremely long periods are going to be increasingly unlikely IRL, of course. > > It might be reasonable to say setting AeFlickerMode to Manual/Custom > will only take effect when the corresponding AeFlickerPeriod control is > applied ? Yes, we could say that setting the mode to manual has no effect until a period is also set. Though of course I assume the period is "sticky", so once you've set it, you can go to and fro to manual mode without re-sending the said period. I guess we have to ask what happens if you go from auto to manual mode, having never set a period. What happens then? TBH, I think I'd be quite happy to say that selecting manual mode, without ever setting a period, is the same as being "off". Thoughts? David > > > -- > Kieran > > > > > > Thanks! > > David > > > > > > > > > + if (flickerState_.mode == controls::FlickerCustom) > > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > > + > > > > + break; > > > > + } > > > > + > > > > case controls::AWB_ENABLE: { > > > > /* Silently ignore this control for a mono sensor. */ > > > > if (monoSensor_) > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > > index 39d00760..22823176 100644 > > > > --- a/src/ipa/rpi/common/ipa_base.h > > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > > @@ -116,6 +116,12 @@ private: > > > > /* Frame duration (1/fps) limits. */ > > > > utils::Duration minFrameDuration_; > > > > utils::Duration maxFrameDuration_; > > > > + > > > > + /* The current state of flicker avoidance. */ > > > > + struct FlickerState { > > > > + int32_t mode; > > > > + utils::Duration customPeriod; > > > > + } flickerState_; > > > > }; > > > > > > > > } /* namespace ipa::RPi */ > > > > -- > > > > 2.30.2 > > > >
Quoting David Plowman (2023-07-18 10:13:42) > Hi Kieran > > On Tue, 18 Jul 2023 at 09:48, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24) > > > Hi Jacopo > > > > > > Thanks for the comments. > > > > > > On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > Hi David > > > > > > > > On Thu, Jul 13, 2023 at 04:12:18PM +0100, David Plowman via libcamera-devel wrote: > > > > > We handle the flicker modes by passing the correct period to the > > > > > AEC/AGC algorithm which already contains the necessary code. > > > > > > > > > > The "Auto" mode, as well as reporting the detected flicker period via > > > > > > > > If so you shouldn't register > > > > > > > > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > > > > > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ? > > > > > > Indeed, it might be better not to expose the auto value! > > > > > > > > > > > > > > > > the "AeFlickerDetected" metadata, are unsupported for now. > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++- > > > > > src/ipa/rpi/common/ipa_base.h | 6 ++++ > > > > > 2 files changed, 69 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > index f40f2e71..81d65ea5 100644 > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{ > > > > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > > > > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > > > > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > > > > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) }, > > > > > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > > > > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, > > > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > > > > @@ -97,7 +99,7 @@ namespace ipa::RPi { > > > > > > > > > > IpaBase::IpaBase() > > > > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), > > > > > - firstStart_(true) > > > > > + firstStart_(true), flickerState_({ 0, 0s }) > > > > > { > > > > > } > > > > > > > > > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls) > > > > > break; > > > > > } > > > > > > > > > > + case controls::AE_FLICKER_MODE: { > > > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > > > + controller_.getAlgorithm("agc")); > > > > > + if (!agc) { > > > > > + LOG(IPARPI, Warning) > > > > > + << "Could not set AeFlickerMode - no AGC algorithm"; > > > > > + break; > > > > > + } > > > > > + > > > > > + int32_t mode = ctrl.second.get<int32_t>(); > > > > > + bool modeValid = true; > > > > > + > > > > > + switch (mode) { > > > > > + case controls::FlickerOff: { > > > > > + agc->setFlickerPeriod(0us); > > > > > + > > > > > + break; > > > > > + } > > > > > > > > Do you need braces ? > > > > > > Sure, I can remove them! > > > > > > > > > > > > + > > > > > + case controls::FlickerCustom: { > > > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > > > + > > > > > + break; > > > > > + } > > > > > + > > > > > + default: > > > > > + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported"; > > > > > + modeValid = false; > > > > > + > > > > > + break; > > > > > + } > > > > > + > > > > > + if (modeValid) > > > > > + flickerState_.mode = mode; > > > > > + > > > > > + break; > > > > > + } > > > > > + > > > > > + case controls::AE_FLICKER_CUSTOM: { > > > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > > > + controller_.getAlgorithm("agc")); > > > > > + if (!agc) { > > > > > + LOG(IPARPI, Warning) > > > > > + << "Could not set AeFlickerCustom - no AGC algorithm"; > > > > > + break; > > > > > + } > > > > > + > > > > > + uint32_t customPeriod = ctrl.second.get<int32_t>(); > > > > > + flickerState_.customPeriod = customPeriod * 1.0us; > > > > > + > > > > > + /* > > > > > + * We note that it makes no difference if the mode gets set to "custom" > > > > > + * first, and the period updated after, or vice versa. > > > > > + */ > > > > > > > > True, but what if the app never provides a value for > > > > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to > > > > some sensible default ? > > > > > > In our implementation we use the value 0 to mean "no flicker > > > cancellection", so setting the mode to custom/manual without a flicker > > > period will cause it to do nothing (until you set one). I'm OK with > > > this behaviour unless anyone objects, though I don't feel strongly! > > > I'd rather document that setting custom/manual mode without a period > > > leads to undefined behaviour rather than mandating a particular value, > > > I think, as not every PH may wish to do the same. What do you think? > > > > > > I had wondered if we could get this down to a single control by having > > '0' as no flicker cancellation - but we'd have no way to express 'auto' > > so I think we do still need two, and it lets us report in the metadata > > if the value was determined automatically or explicitly I guess? (Though > > the app should know that anyway as it would know if it set the manual > > value?) > > Yes, I think we do still want a separate control. > > > > > Throwing paint on the bike shed I feel like 'AeFlickerPeriod' would be > > better suited for the control name as then it would be usable to report > > the period back in the metadata when it's detected with 'Auto' mode. > > I agree that 'AeFlickerPeriod' is a better name, particularly if the > 'custom' mode becomes the 'manual' mode. > > > > > What limits will be imposed for the period ? I think setting > > AeFlickerMode to 'Custom' (or 'Manual'?) and not setting an > > {AeFlickerCustom,AeFlickerPeriod} control should be the same as not > > changing anything, but I expect that maybe the ControlInfo might have > > limits that would preclude setting a period of '0' ? > > I guess implementations get to choose their own limits though, for the > most part, I see no reason not to allow them to be very wide. > Extremely short or extremely long periods are going to be increasingly > unlikely IRL, of course. The part I was wondering about is if we 'defined' 0 as being equivalent to controls::FlickerOff, but that could make it harder to validate the control if it means any value from 0...MAX is 'acceptable'. But yes, I think this can all be handled by the pipeline handler/IPA. > > It might be reasonable to say setting AeFlickerMode to Manual/Custom > > will only take effect when the corresponding AeFlickerPeriod control is > > applied ? > > Yes, we could say that setting the mode to manual has no effect until > a period is also set. Though of course I assume the period is > "sticky", so once you've set it, you can go to and fro to manual mode > without re-sending the said period. <Time jump, now that I've writen the below> Does this mean you expect to go from AeFlickerMode=Manaul(20000) to AeFlickerMode=Auto(Detected 30000) and then go to AeFlickerMode=Off(0) and when you finally go back to AeFlickerMode=Manual(no period applied) you expect period to be 'remembered' as 20000? > > I guess we have to ask what happens if you go from auto to manual > mode, having never set a period. What happens then? TBH, I think I'd > be quite happy to say that selecting manual mode, without ever setting > a period, is the same as being "off". Thoughts? I suspect this might be something we should consider for all 'multi-control' settings, for how they react if they are only partially set. As long as the behaviour is documented I think it's fine though. I think I would consider setting the period to be 'sticky' as well so if you did: 1) AeFlickerMode = Off 2) AeFlickerPeriod = 10000 3) AeFlickerMode = Manual The Period would be defined at 10000 I think for switching from Auto to Manual it would remain at whatever the autodetected value was... 1) AeFlickerMode = Off 2) AeFlickerMode = Auto #Metadata reports AeFlickerPeriod as 20000 3) AeFlickerMode = Manual # AeFlickerPeriod now 'locked' at manual 20000 Hrm... but that seems to differ with your suggestion above of setting it to 'off' To me 'sticky' to last actual value (where off = 0, auto == detected value) seems easiest to codify and explain? But maybe I'm missing something obvious. And now I feel like we should have helper classes in libipa to codify the behaviour of all these controls in a common way for each PH!? (later) -- Kieran > > David > > > > > > > -- > > Kieran > > > > > > > > > > Thanks! > > > David > > > > > > > > > > > > + if (flickerState_.mode == controls::FlickerCustom) > > > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > > > + > > > > > + break; > > > > > + } > > > > > + > > > > > case controls::AWB_ENABLE: { > > > > > /* Silently ignore this control for a mono sensor. */ > > > > > if (monoSensor_) > > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > > > index 39d00760..22823176 100644 > > > > > --- a/src/ipa/rpi/common/ipa_base.h > > > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > > > @@ -116,6 +116,12 @@ private: > > > > > /* Frame duration (1/fps) limits. */ > > > > > utils::Duration minFrameDuration_; > > > > > utils::Duration maxFrameDuration_; > > > > > + > > > > > + /* The current state of flicker avoidance. */ > > > > > + struct FlickerState { > > > > > + int32_t mode; > > > > > + utils::Duration customPeriod; > > > > > + } flickerState_; > > > > > }; > > > > > > > > > > } /* namespace ipa::RPi */ > > > > > -- > > > > > 2.30.2 > > > > >
Just to throw more confusion to the mix On Tue, Jul 18, 2023 at 12:16:02PM +0100, Kieran Bingham via libcamera-devel wrote: > Quoting David Plowman (2023-07-18 10:13:42) > > Hi Kieran > > > > On Tue, 18 Jul 2023 at 09:48, Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24) > > > > Hi Jacopo > > > > > > > > Thanks for the comments. > > > > > > > > On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi > > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > > > Hi David > > > > > > > > > > On Thu, Jul 13, 2023 at 04:12:18PM +0100, David Plowman via libcamera-devel wrote: > > > > > > We handle the flicker modes by passing the correct period to the > > > > > > AEC/AGC algorithm which already contains the necessary code. > > > > > > > > > > > > The "Auto" mode, as well as reporting the detected flicker period via > > > > > > > > > > If so you shouldn't register > > > > > > > > > > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > > > > > > > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ? > > > > > > > > Indeed, it might be better not to expose the auto value! > > > > > > > > > > > > > > > > > > > > the "AeFlickerDetected" metadata, are unsupported for now. > > > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > --- > > > > > > src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++- > > > > > > src/ipa/rpi/common/ipa_base.h | 6 ++++ > > > > > > 2 files changed, 69 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > > index f40f2e71..81d65ea5 100644 > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > > @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{ > > > > > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > > > > > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > > > > > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > > > > > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) }, > > > > > > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > > > > > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, > > > > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > > > > > @@ -97,7 +99,7 @@ namespace ipa::RPi { > > > > > > > > > > > > IpaBase::IpaBase() > > > > > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), > > > > > > - firstStart_(true) > > > > > > + firstStart_(true), flickerState_({ 0, 0s }) > > > > > > { > > > > > > } > > > > > > > > > > > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls) > > > > > > break; > > > > > > } > > > > > > > > > > > > + case controls::AE_FLICKER_MODE: { > > > > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > > > > + controller_.getAlgorithm("agc")); > > > > > > + if (!agc) { > > > > > > + LOG(IPARPI, Warning) > > > > > > + << "Could not set AeFlickerMode - no AGC algorithm"; > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + int32_t mode = ctrl.second.get<int32_t>(); > > > > > > + bool modeValid = true; > > > > > > + > > > > > > + switch (mode) { > > > > > > + case controls::FlickerOff: { > > > > > > + agc->setFlickerPeriod(0us); > > > > > > + > > > > > > + break; > > > > > > + } > > > > > > > > > > Do you need braces ? > > > > > > > > Sure, I can remove them! > > > > > > > > > > > > > > > + > > > > > > + case controls::FlickerCustom: { > > > > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > > > > + > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + default: > > > > > > + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported"; > > > > > > + modeValid = false; > > > > > > + > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + if (modeValid) > > > > > > + flickerState_.mode = mode; > > > > > > + > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + case controls::AE_FLICKER_CUSTOM: { > > > > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > > > > + controller_.getAlgorithm("agc")); > > > > > > + if (!agc) { > > > > > > + LOG(IPARPI, Warning) > > > > > > + << "Could not set AeFlickerCustom - no AGC algorithm"; > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + uint32_t customPeriod = ctrl.second.get<int32_t>(); > > > > > > + flickerState_.customPeriod = customPeriod * 1.0us; > > > > > > + > > > > > > + /* > > > > > > + * We note that it makes no difference if the mode gets set to "custom" > > > > > > + * first, and the period updated after, or vice versa. > > > > > > + */ > > > > > > > > > > True, but what if the app never provides a value for > > > > > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to > > > > > some sensible default ? > > > > > > > > In our implementation we use the value 0 to mean "no flicker > > > > cancellection", so setting the mode to custom/manual without a flicker > > > > period will cause it to do nothing (until you set one). I'm OK with > > > > this behaviour unless anyone objects, though I don't feel strongly! > > > > I'd rather document that setting custom/manual mode without a period > > > > leads to undefined behaviour rather than mandating a particular value, > > > > I think, as not every PH may wish to do the same. What do you think? > > > > > > > > > I had wondered if we could get this down to a single control by having > > > '0' as no flicker cancellation - but we'd have no way to express 'auto' > > > so I think we do still need two, and it lets us report in the metadata > > > if the value was determined automatically or explicitly I guess? (Though > > > the app should know that anyway as it would know if it set the manual > > > value?) > > > > Yes, I think we do still want a separate control. > > > > > > > > Throwing paint on the bike shed I feel like 'AeFlickerPeriod' would be > > > better suited for the control name as then it would be usable to report > > > the period back in the metadata when it's detected with 'Auto' mode. > > > > I agree that 'AeFlickerPeriod' is a better name, particularly if the > > 'custom' mode becomes the 'manual' mode. > > > > > > > > What limits will be imposed for the period ? I think setting > > > AeFlickerMode to 'Custom' (or 'Manual'?) and not setting an > > > {AeFlickerCustom,AeFlickerPeriod} control should be the same as not > > > changing anything, but I expect that maybe the ControlInfo might have > > > limits that would preclude setting a period of '0' ? > > > > I guess implementations get to choose their own limits though, for the > > most part, I see no reason not to allow them to be very wide. > > Extremely short or extremely long periods are going to be increasingly > > unlikely IRL, of course. > > The part I was wondering about is if we 'defined' 0 as being equivalent > to controls::FlickerOff, but that could make it harder to validate the > control if it means any value from 0...MAX is 'acceptable'. > > But yes, I think this can all be handled by the pipeline handler/IPA. > > > > > It might be reasonable to say setting AeFlickerMode to Manual/Custom > > > will only take effect when the corresponding AeFlickerPeriod control is > > > applied ? > > > > Yes, we could say that setting the mode to manual has no effect until > > a period is also set. Though of course I assume the period is > > "sticky", so once you've set it, you can go to and fro to manual mode > > without re-sending the said period. > > <Time jump, now that I've writen the below> Does this mean you expect to > go from AeFlickerMode=Manaul(20000) to AeFlickerMode=Auto(Detected > 30000) and then go to AeFlickerMode=Off(0) and when you finally go back > to AeFlickerMode=Manual(no period applied) you expect period to be > 'remembered' as 20000? > > > > > > > I guess we have to ask what happens if you go from auto to manual > > mode, having never set a period. What happens then? TBH, I think I'd > > be quite happy to say that selecting manual mode, without ever setting > > a period, is the same as being "off". Thoughts? > > I suspect this might be something we should consider for all > 'multi-control' settings, for how they react if they are only partially > set. As long as the behaviour is documented I think it's fine though. > > I think I would consider setting the period to be 'sticky' as well so if > you did: > > 1) AeFlickerMode = Off > 2) AeFlickerPeriod = 10000 > 3) AeFlickerMode = Manual I guess it depends if 1) 2) and 3) are separate requests ? If that's the case, we discussed in the past how this should work for the AEGC controls that I still wish someone review: https://patchwork.libcamera.org/patch/17077/ A similar case is the one represented by 'ExposureTime' which is only meaningful when the mode is set to Manual. From the description of the control we drafted: - ExposureTime: This control will only take effect if ExposureTimeMode is Manual. If this control is set when ExposureTimeMode is Auto, the value will be ignored and will not be retained. I guess the situation here is similar ? > > The Period would be defined at 10000 > > I think for switching from Auto to Manual it would remain at whatever > the autodetected value was... > > 1) AeFlickerMode = Off > 2) AeFlickerMode = Auto #Metadata reports AeFlickerPeriod as 20000 > 3) AeFlickerMode = Manual # AeFlickerPeriod now 'locked' at manual 20000 Looking at the behaviour we defined for AEGC, this seems to match + When transitioning from Auto to Manual mode and no ExposureTime control + is provided by the application, the last value computed by the AE + algorithm when the mode was Auto will be used. > > Hrm... but that seems to differ with your suggestion above of setting it > to 'off' > > To me 'sticky' to last actual value (where off = 0, auto == detected > value) seems easiest to codify and explain? But maybe I'm missing > something obvious. > > And now I feel like we should have helper classes in libipa to codify > the behaviour of all these controls in a common way for each PH!? > (later) I wish > > -- > Kieran > > > > > David > > > > > > > > > > > -- > > > Kieran > > > > > > > > > > > > > > Thanks! > > > > David > > > > > > > > > > > > > > > + if (flickerState_.mode == controls::FlickerCustom) > > > > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > > > > + > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > case controls::AWB_ENABLE: { > > > > > > /* Silently ignore this control for a mono sensor. */ > > > > > > if (monoSensor_) > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > > > > index 39d00760..22823176 100644 > > > > > > --- a/src/ipa/rpi/common/ipa_base.h > > > > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > > > > @@ -116,6 +116,12 @@ private: > > > > > > /* Frame duration (1/fps) limits. */ > > > > > > utils::Duration minFrameDuration_; > > > > > > utils::Duration maxFrameDuration_; > > > > > > + > > > > > > + /* The current state of flicker avoidance. */ > > > > > > + struct FlickerState { > > > > > > + int32_t mode; > > > > > > + utils::Duration customPeriod; > > > > > > + } flickerState_; > > > > > > }; > > > > > > > > > > > > } /* namespace ipa::RPi */ > > > > > > -- > > > > > > 2.30.2 > > > > > >
Thanks for the extra dose of confusion!! Actually I worry slightly that we're over-thinking a bit here. The AE/AGC analogy is indeed interesting because there's a specific reason why you would change from auto to manual. Because the exposure is always changing and you just want to fix it at whatever it is for a bit while you (for example) take some pictures. And for this to work you need manual mode to adopt whatever values auto mode had at the time Flicker isn't really like that. In auto mode it's not changing all over the place, so I don't see any benefit in, for example, setting the manual period to what the auto was doing when switching to manual mode. Indeed, maybe you have some a priori guess as to what the flicker period might be and that you want to use if the auto algorithm fails to detect an exact value for itself. So it might just be more helpful to applications to leave the manual period completely alone, unless the application changes it. As always, it's hard to predict. So my gut reaction is simply not to couple these things together, it has the virtue of simplicity, if nothing else. Perhaps let me post a revised version with some of the name changes and improved wording, and then we can take up the discussion again! Thanks David On Tue, 18 Jul 2023 at 13:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Just to throw more confusion to the mix > > On Tue, Jul 18, 2023 at 12:16:02PM +0100, Kieran Bingham via libcamera-devel wrote: > > Quoting David Plowman (2023-07-18 10:13:42) > > > Hi Kieran > > > > > > On Tue, 18 Jul 2023 at 09:48, Kieran Bingham > > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > > > Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24) > > > > > Hi Jacopo > > > > > > > > > > Thanks for the comments. > > > > > > > > > > On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi > > > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > > > > > Hi David > > > > > > > > > > > > On Thu, Jul 13, 2023 at 04:12:18PM +0100, David Plowman via libcamera-devel wrote: > > > > > > > We handle the flicker modes by passing the correct period to the > > > > > > > AEC/AGC algorithm which already contains the necessary code. > > > > > > > > > > > > > > The "Auto" mode, as well as reporting the detected flicker period via > > > > > > > > > > > > If so you shouldn't register > > > > > > > > > > > > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > > > > > > > > > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ? > > > > > > > > > > Indeed, it might be better not to expose the auto value! > > > > > > > > > > > > > > > > > > > > > > > > the "AeFlickerDetected" metadata, are unsupported for now. > > > > > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > > --- > > > > > > > src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++- > > > > > > > src/ipa/rpi/common/ipa_base.h | 6 ++++ > > > > > > > 2 files changed, 69 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > > > index f40f2e71..81d65ea5 100644 > > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > > > @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{ > > > > > > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > > > > > > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > > > > > > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > > > > > > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, > > > > > > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) }, > > > > > > > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > > > > > > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, > > > > > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > > > > > > @@ -97,7 +99,7 @@ namespace ipa::RPi { > > > > > > > > > > > > > > IpaBase::IpaBase() > > > > > > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), > > > > > > > - firstStart_(true) > > > > > > > + firstStart_(true), flickerState_({ 0, 0s }) > > > > > > > { > > > > > > > } > > > > > > > > > > > > > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls) > > > > > > > break; > > > > > > > } > > > > > > > > > > > > > > + case controls::AE_FLICKER_MODE: { > > > > > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > > > > > + controller_.getAlgorithm("agc")); > > > > > > > + if (!agc) { > > > > > > > + LOG(IPARPI, Warning) > > > > > > > + << "Could not set AeFlickerMode - no AGC algorithm"; > > > > > > > + break; > > > > > > > + } > > > > > > > + > > > > > > > + int32_t mode = ctrl.second.get<int32_t>(); > > > > > > > + bool modeValid = true; > > > > > > > + > > > > > > > + switch (mode) { > > > > > > > + case controls::FlickerOff: { > > > > > > > + agc->setFlickerPeriod(0us); > > > > > > > + > > > > > > > + break; > > > > > > > + } > > > > > > > > > > > > Do you need braces ? > > > > > > > > > > Sure, I can remove them! > > > > > > > > > > > > > > > > > > + > > > > > > > + case controls::FlickerCustom: { > > > > > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > > > > > + > > > > > > > + break; > > > > > > > + } > > > > > > > + > > > > > > > + default: > > > > > > > + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported"; > > > > > > > + modeValid = false; > > > > > > > + > > > > > > > + break; > > > > > > > + } > > > > > > > + > > > > > > > + if (modeValid) > > > > > > > + flickerState_.mode = mode; > > > > > > > + > > > > > > > + break; > > > > > > > + } > > > > > > > + > > > > > > > + case controls::AE_FLICKER_CUSTOM: { > > > > > > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > > > > > > > + controller_.getAlgorithm("agc")); > > > > > > > + if (!agc) { > > > > > > > + LOG(IPARPI, Warning) > > > > > > > + << "Could not set AeFlickerCustom - no AGC algorithm"; > > > > > > > + break; > > > > > > > + } > > > > > > > + > > > > > > > + uint32_t customPeriod = ctrl.second.get<int32_t>(); > > > > > > > + flickerState_.customPeriod = customPeriod * 1.0us; > > > > > > > + > > > > > > > + /* > > > > > > > + * We note that it makes no difference if the mode gets set to "custom" > > > > > > > + * first, and the period updated after, or vice versa. > > > > > > > + */ > > > > > > > > > > > > True, but what if the app never provides a value for > > > > > > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to > > > > > > some sensible default ? > > > > > > > > > > In our implementation we use the value 0 to mean "no flicker > > > > > cancellection", so setting the mode to custom/manual without a flicker > > > > > period will cause it to do nothing (until you set one). I'm OK with > > > > > this behaviour unless anyone objects, though I don't feel strongly! > > > > > I'd rather document that setting custom/manual mode without a period > > > > > leads to undefined behaviour rather than mandating a particular value, > > > > > I think, as not every PH may wish to do the same. What do you think? > > > > > > > > > > > > I had wondered if we could get this down to a single control by having > > > > '0' as no flicker cancellation - but we'd have no way to express 'auto' > > > > so I think we do still need two, and it lets us report in the metadata > > > > if the value was determined automatically or explicitly I guess? (Though > > > > the app should know that anyway as it would know if it set the manual > > > > value?) > > > > > > Yes, I think we do still want a separate control. > > > > > > > > > > > Throwing paint on the bike shed I feel like 'AeFlickerPeriod' would be > > > > better suited for the control name as then it would be usable to report > > > > the period back in the metadata when it's detected with 'Auto' mode. > > > > > > I agree that 'AeFlickerPeriod' is a better name, particularly if the > > > 'custom' mode becomes the 'manual' mode. > > > > > > > > > > > What limits will be imposed for the period ? I think setting > > > > AeFlickerMode to 'Custom' (or 'Manual'?) and not setting an > > > > {AeFlickerCustom,AeFlickerPeriod} control should be the same as not > > > > changing anything, but I expect that maybe the ControlInfo might have > > > > limits that would preclude setting a period of '0' ? > > > > > > I guess implementations get to choose their own limits though, for the > > > most part, I see no reason not to allow them to be very wide. > > > Extremely short or extremely long periods are going to be increasingly > > > unlikely IRL, of course. > > > > The part I was wondering about is if we 'defined' 0 as being equivalent > > to controls::FlickerOff, but that could make it harder to validate the > > control if it means any value from 0...MAX is 'acceptable'. > > > > But yes, I think this can all be handled by the pipeline handler/IPA. > > > > > > > > It might be reasonable to say setting AeFlickerMode to Manual/Custom > > > > will only take effect when the corresponding AeFlickerPeriod control is > > > > applied ? > > > > > > Yes, we could say that setting the mode to manual has no effect until > > > a period is also set. Though of course I assume the period is > > > "sticky", so once you've set it, you can go to and fro to manual mode > > > without re-sending the said period. > > > > <Time jump, now that I've writen the below> Does this mean you expect to > > go from AeFlickerMode=Manaul(20000) to AeFlickerMode=Auto(Detected > > 30000) and then go to AeFlickerMode=Off(0) and when you finally go back > > to AeFlickerMode=Manual(no period applied) you expect period to be > > 'remembered' as 20000? > > > > > > > > > > > > I guess we have to ask what happens if you go from auto to manual > > > mode, having never set a period. What happens then? TBH, I think I'd > > > be quite happy to say that selecting manual mode, without ever setting > > > a period, is the same as being "off". Thoughts? > > > > I suspect this might be something we should consider for all > > 'multi-control' settings, for how they react if they are only partially > > set. As long as the behaviour is documented I think it's fine though. > > > > I think I would consider setting the period to be 'sticky' as well so if > > you did: > > > > 1) AeFlickerMode = Off > > 2) AeFlickerPeriod = 10000 > > 3) AeFlickerMode = Manual > > I guess it depends if 1) 2) and 3) are separate requests ? If that's > the case, we discussed in the past how this should work for the AEGC > controls that I still wish someone review: > https://patchwork.libcamera.org/patch/17077/ > > A similar case is the one represented by 'ExposureTime' which is only > meaningful when the mode is set to Manual. From the description of the > control we drafted: > > - ExposureTime: > This control will only take effect if ExposureTimeMode is Manual. If > this control is set when ExposureTimeMode is Auto, the value will be > ignored and will not be retained. > > I guess the situation here is similar ? > > > > > > The Period would be defined at 10000 > > > > I think for switching from Auto to Manual it would remain at whatever > > the autodetected value was... > > > > 1) AeFlickerMode = Off > > 2) AeFlickerMode = Auto #Metadata reports AeFlickerPeriod as 20000 > > 3) AeFlickerMode = Manual # AeFlickerPeriod now 'locked' at manual 20000 > > Looking at the behaviour we defined for AEGC, this seems to match > > + When transitioning from Auto to Manual mode and no ExposureTime control > + is provided by the application, the last value computed by the AE > + algorithm when the mode was Auto will be used. > > > > > > Hrm... but that seems to differ with your suggestion above of setting it > > to 'off' > > > > To me 'sticky' to last actual value (where off = 0, auto == detected > > value) seems easiest to codify and explain? But maybe I'm missing > > something obvious. > > > > And now I feel like we should have helper classes in libipa to codify > > the behaviour of all these controls in a common way for each PH!? > > (later) > > I wish > > > > > -- > > Kieran > > > > > > > > David > > > > > > > > > > > > > > > -- > > > > Kieran > > > > > > > > > > > > > > > > > > Thanks! > > > > > David > > > > > > > > > > > > > > > > > > + if (flickerState_.mode == controls::FlickerCustom) > > > > > > > + agc->setFlickerPeriod(flickerState_.customPeriod); > > > > > > > + > > > > > > > + break; > > > > > > > + } > > > > > > > + > > > > > > > case controls::AWB_ENABLE: { > > > > > > > /* Silently ignore this control for a mono sensor. */ > > > > > > > if (monoSensor_) > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > > > > > index 39d00760..22823176 100644 > > > > > > > --- a/src/ipa/rpi/common/ipa_base.h > > > > > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > > > > > @@ -116,6 +116,12 @@ private: > > > > > > > /* Frame duration (1/fps) limits. */ > > > > > > > utils::Duration minFrameDuration_; > > > > > > > utils::Duration maxFrameDuration_; > > > > > > > + > > > > > > > + /* The current state of flicker avoidance. */ > > > > > > > + struct FlickerState { > > > > > > > + int32_t mode; > > > > > > > + utils::Duration customPeriod; > > > > > > > + } flickerState_; > > > > > > > }; > > > > > > > > > > > > > > } /* namespace ipa::RPi */ > > > > > > > -- > > > > > > > 2.30.2 > > > > > > >
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index f40f2e71..81d65ea5 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{ { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) }, + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) }, { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, @@ -97,7 +99,7 @@ namespace ipa::RPi { IpaBase::IpaBase() : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), - firstStart_(true) + firstStart_(true), flickerState_({ 0, 0s }) { } @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls) break; } + case controls::AE_FLICKER_MODE: { + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( + controller_.getAlgorithm("agc")); + if (!agc) { + LOG(IPARPI, Warning) + << "Could not set AeFlickerMode - no AGC algorithm"; + break; + } + + int32_t mode = ctrl.second.get<int32_t>(); + bool modeValid = true; + + switch (mode) { + case controls::FlickerOff: { + agc->setFlickerPeriod(0us); + + break; + } + + case controls::FlickerCustom: { + agc->setFlickerPeriod(flickerState_.customPeriod); + + break; + } + + default: + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported"; + modeValid = false; + + break; + } + + if (modeValid) + flickerState_.mode = mode; + + break; + } + + case controls::AE_FLICKER_CUSTOM: { + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( + controller_.getAlgorithm("agc")); + if (!agc) { + LOG(IPARPI, Warning) + << "Could not set AeFlickerCustom - no AGC algorithm"; + break; + } + + uint32_t customPeriod = ctrl.second.get<int32_t>(); + flickerState_.customPeriod = customPeriod * 1.0us; + + /* + * We note that it makes no difference if the mode gets set to "custom" + * first, and the period updated after, or vice versa. + */ + if (flickerState_.mode == controls::FlickerCustom) + agc->setFlickerPeriod(flickerState_.customPeriod); + + break; + } + case controls::AWB_ENABLE: { /* Silently ignore this control for a mono sensor. */ if (monoSensor_) diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h index 39d00760..22823176 100644 --- a/src/ipa/rpi/common/ipa_base.h +++ b/src/ipa/rpi/common/ipa_base.h @@ -116,6 +116,12 @@ private: /* Frame duration (1/fps) limits. */ utils::Duration minFrameDuration_; utils::Duration maxFrameDuration_; + + /* The current state of flicker avoidance. */ + struct FlickerState { + int32_t mode; + utils::Duration customPeriod; + } flickerState_; }; } /* namespace ipa::RPi */
We handle the flicker modes by passing the correct period to the AEC/AGC algorithm which already contains the necessary code. The "Auto" mode, as well as reporting the detected flicker period via the "AeFlickerDetected" metadata, are unsupported for now. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++- src/ipa/rpi/common/ipa_base.h | 6 ++++ 2 files changed, 69 insertions(+), 1 deletion(-)