| Message ID | 20230328085521.7409-3-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for this work. On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> 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 > the "AeFlickerDetected" metadata, are unsupported for now. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++ > 1 file changed, 80 insertions(+) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 13757955..d485b851 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -91,6 +91,8 @@ static 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::AwbEnable, ControlInfo(false, true) }, > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > @@ -230,6 +232,12 @@ private: > /* Track the frame length times over FrameLengthsQueueSize frames. */ > std::deque<Duration> frameLengths_; > Duration lastTimeout_; > + > + /* The current state of flicker avoidance. */ > + struct FlickerState { > + int32_t mode; > + Duration customPeriod; > + } flickerState_; > }; > > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) > @@ -962,6 +970,78 @@ void IPARPi::queueRequest(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::FlickerFreq50Hz: { > + agc->setFlickerPeriod(10000 * 1.0us); > + > + break; > + } > + > + case controls::FlickerFreq60Hz: { > + agc->setFlickerPeriod(8333.333 * 1.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; > + } Perhaps remove the blank lines above the break statements above? > + > + 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; > + } This is not something to do for this change, but for controls like AeFlickerMode::FlickerCustom which require a value set in AeFlickerCustom, perhaps we can mandate that both controls *must* be set in the same control list to be valid? This would avoid having mostly useless state information stored in the IPA class. Ditto for some of the focus, AGC and AWB controls as well. What do folks think? Is that too restrictive on the application? Other than that: Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > + > case controls::AWB_ENABLE: { > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > controller_.getAlgorithm("awb")); > -- > 2.30.2 >
Hi Naush On Mon, Apr 03, 2023 at 09:06:35AM +0100, Naushir Patuck via libcamera-devel wrote: > Hi David, > > Thank you for this work. > > On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel > <libcamera-devel@lists.libcamera.org> 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 > > the "AeFlickerDetected" metadata, are unsupported for now. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++ > > 1 file changed, 80 insertions(+) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 13757955..d485b851 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -91,6 +91,8 @@ static 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::AwbEnable, ControlInfo(false, true) }, > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > > @@ -230,6 +232,12 @@ private: > > /* Track the frame length times over FrameLengthsQueueSize frames. */ > > std::deque<Duration> frameLengths_; > > Duration lastTimeout_; > > + > > + /* The current state of flicker avoidance. */ > > + struct FlickerState { > > + int32_t mode; > > + Duration customPeriod; > > + } flickerState_; > > }; > > > > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) > > @@ -962,6 +970,78 @@ void IPARPi::queueRequest(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::FlickerFreq50Hz: { > > + agc->setFlickerPeriod(10000 * 1.0us); > > + > > + break; > > + } > > + > > + case controls::FlickerFreq60Hz: { > > + agc->setFlickerPeriod(8333.333 * 1.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; > > + } > > Perhaps remove the blank lines above the break statements above? > > > + > > + 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; > > + } > > This is not something to do for this change, but for controls like > AeFlickerMode::FlickerCustom which require a value set in AeFlickerCustom, > perhaps we can mandate that both controls *must* be set in the same control list > to be valid? This would avoid having mostly useless state information stored in > the IPA class. Ditto for some of the focus, AGC and AWB controls as well. What > do folks think? Is that too restrictive on the application? > This won't work well for AEGC. We have a pending rework of AEGC controls where we had separated ExposureTime and AnalogueGain. In this context is valid to set ExposureTimeMode to Manual and not send a new value so that the exposure time is fixed and the gain is adjusted to compensate it to implement "Aperture priority" (or "Shutter priority" for the other way around) modes. Does this make sense ? > Other than that: > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > + > > case controls::AWB_ENABLE: { > > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > > controller_.getAlgorithm("awb")); > > -- > > 2.30.2 > >
Hello, On Mon, Apr 03, 2023 at 04:36:02PM +0200, Jacopo Mondi via libcamera-devel wrote: > On Mon, Apr 03, 2023 at 09:06:35AM +0100, Naushir Patuck via libcamera-devel wrote: > > On Tue, 28 Mar 2023 at 09:55, 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 > > > the "AeFlickerDetected" metadata, are unsupported for now. David, do you have plans to implement those ? > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++ > > > 1 file changed, 80 insertions(+) > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index 13757955..d485b851 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -91,6 +91,8 @@ static 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::AwbEnable, ControlInfo(false, true) }, > > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > > > @@ -230,6 +232,12 @@ private: > > > /* Track the frame length times over FrameLengthsQueueSize frames. */ > > > std::deque<Duration> frameLengths_; > > > Duration lastTimeout_; > > > + > > > + /* The current state of flicker avoidance. */ > > > + struct FlickerState { > > > + int32_t mode; > > > + Duration customPeriod; > > > + } flickerState_; > > > }; > > > > > > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) > > > @@ -962,6 +970,78 @@ void IPARPi::queueRequest(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); > > > + No blank line needed. You may store the period in a local variable and call agc->setFlickerPeriod() once after the switch. Up to you. > > > + break; > > > + } > > > + > > > + case controls::FlickerFreq50Hz: { > > > + agc->setFlickerPeriod(10000 * 1.0us); > > > + > > > + break; > > > + } > > > + > > > + case controls::FlickerFreq60Hz: { > > > + agc->setFlickerPeriod(8333.333 * 1.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; > > > + } > > > > Perhaps remove the blank lines above the break statements above? I should read through before reviewing :-) > > > + > > > + 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; > > > + } > > > > This is not something to do for this change, but for controls like > > AeFlickerMode::FlickerCustom which require a value set in AeFlickerCustom, > > perhaps we can mandate that both controls *must* be set in the same control list > > to be valid? This would avoid having mostly useless state information stored in > > the IPA class. Ditto for some of the focus, AGC and AWB controls as well. What > > do folks think? Is that too restrictive on the application? The best option is to design controls that can't be used inconsistently. That's a goal that can never be fully reached, so we need to decide on a consistent strategy for cases where the user could get it wrong. There can be lots of dependencies between controls. If we decide to start enforcing this kind of rules, it will need to be done globally for consistency. I'm worried it could quickly get out of control, which is why, so far, inconsistent control values are silently ignored. I'm willing to reconsider this though. > This won't work well for AEGC. We have a pending rework of AEGC > controls where we had separated ExposureTime and AnalogueGain. In this > context is valid to set ExposureTimeMode to Manual and not send a new > value so that the exposure time is fixed and the gain is adjusted to > compensate it to implement "Aperture priority" (or "Shutter priority" for the > other way around) modes. > > Does this make sense ? > > > Other than that: > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > + > > > case controls::AWB_ENABLE: { > > > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > > > controller_.getAlgorithm("awb"));
Resending this to reply to the correct mail thread for patchworks sake: Content added below is from Maarten in Message-ID <022b6380-7d78-8d1c-20ee-0ae211a65653@lankhorst.se> Quoting David Plowman via libcamera-devel (2023-03-28 09:55:21) > 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. > This series removes the power line flickering on my pi v3 camera when using libcamerasrc (with some modifications there). Tested-by: Maarten Lankhorst <dev@lankhorst.se> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++ > 1 file changed, 80 insertions(+) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 13757955..d485b851 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -91,6 +91,8 @@ static 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::AwbEnable, ControlInfo(false, true) }, > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > @@ -230,6 +232,12 @@ private: > /* Track the frame length times over FrameLengthsQueueSize frames. */ > std::deque<Duration> frameLengths_; > Duration lastTimeout_; > + > + /* The current state of flicker avoidance. */ > + struct FlickerState { > + int32_t mode; > + Duration customPeriod; > + } flickerState_; > }; > > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) > @@ -962,6 +970,78 @@ void IPARPi::queueRequest(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::FlickerFreq50Hz: { > + agc->setFlickerPeriod(10000 * 1.0us); > + > + break; > + } > + > + case controls::FlickerFreq60Hz: { > + agc->setFlickerPeriod(8333.333 * 1.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: { > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > controller_.getAlgorithm("awb")); > -- > 2.30.2 >
On Tue, Apr 04, 2023 at 10:37:10AM +0300, Laurent Pinchart via libcamera-devel wrote: > On Mon, Apr 03, 2023 at 04:36:02PM +0200, Jacopo Mondi via libcamera-devel wrote: > > On Mon, Apr 03, 2023 at 09:06:35AM +0100, Naushir Patuck via libcamera-devel wrote: > > > On Tue, 28 Mar 2023 at 09:55, 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 > > > > the "AeFlickerDetected" metadata, are unsupported for now. > > David, do you have plans to implement those ? And what would it take to implement it ? > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++ > > > > 1 file changed, 80 insertions(+) > > > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index 13757955..d485b851 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -91,6 +91,8 @@ static 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::AwbEnable, ControlInfo(false, true) }, > > > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > > > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > > > > @@ -230,6 +232,12 @@ private: > > > > /* Track the frame length times over FrameLengthsQueueSize frames. */ > > > > std::deque<Duration> frameLengths_; > > > > Duration lastTimeout_; > > > > + > > > > + /* The current state of flicker avoidance. */ > > > > + struct FlickerState { > > > > + int32_t mode; > > > > + Duration customPeriod; > > > > + } flickerState_; > > > > }; > > > > > > > > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) > > > > @@ -962,6 +970,78 @@ void IPARPi::queueRequest(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); > > > > + > > No blank line needed. > > You may store the period in a local variable and call > agc->setFlickerPeriod() once after the switch. Up to you. > > > > > + break; > > > > + } > > > > + > > > > + case controls::FlickerFreq50Hz: { > > > > + agc->setFlickerPeriod(10000 * 1.0us); > > > > + > > > > + break; > > > > + } > > > > + > > > > + case controls::FlickerFreq60Hz: { > > > > + agc->setFlickerPeriod(8333.333 * 1.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; > > > > + } > > > > > > Perhaps remove the blank lines above the break statements above? > > I should read through before reviewing :-) > > > > > + > > > > + 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; > > > > + } > > > > > > This is not something to do for this change, but for controls like > > > AeFlickerMode::FlickerCustom which require a value set in AeFlickerCustom, > > > perhaps we can mandate that both controls *must* be set in the same control list > > > to be valid? This would avoid having mostly useless state information stored in > > > the IPA class. Ditto for some of the focus, AGC and AWB controls as well. What > > > do folks think? Is that too restrictive on the application? > > The best option is to design controls that can't be used inconsistently. > That's a goal that can never be fully reached, so we need to decide on a > consistent strategy for cases where the user could get it wrong. > > There can be lots of dependencies between controls. If we decide to > start enforcing this kind of rules, it will need to be done globally for > consistency. I'm worried it could quickly get out of control, which is > why, so far, inconsistent control values are silently ignored. I'm > willing to reconsider this though. > > > This won't work well for AEGC. We have a pending rework of AEGC > > controls where we had separated ExposureTime and AnalogueGain. In this > > context is valid to set ExposureTimeMode to Manual and not send a new > > value so that the exposure time is fixed and the gain is adjusted to > > compensate it to implement "Aperture priority" (or "Shutter priority" for the > > other way around) modes. > > > > Does this make sense ? > > > > > Other than that: > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > + > > > > case controls::AWB_ENABLE: { > > > > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > > > > controller_.getAlgorithm("awb"));
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 13757955..d485b851 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -91,6 +91,8 @@ static 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::AwbEnable, ControlInfo(false, true) }, { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, @@ -230,6 +232,12 @@ private: /* Track the frame length times over FrameLengthsQueueSize frames. */ std::deque<Duration> frameLengths_; Duration lastTimeout_; + + /* The current state of flicker avoidance. */ + struct FlickerState { + int32_t mode; + Duration customPeriod; + } flickerState_; }; int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) @@ -962,6 +970,78 @@ void IPARPi::queueRequest(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::FlickerFreq50Hz: { + agc->setFlickerPeriod(10000 * 1.0us); + + break; + } + + case controls::FlickerFreq60Hz: { + agc->setFlickerPeriod(8333.333 * 1.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: { RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( controller_.getAlgorithm("awb"));
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/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)