[libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Handle AEC/AGC flicker controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 3 11:54:32 CEST 2023
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 at 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 at raspberrypi.com>
> > >
> > > > +
> > > > case controls::AWB_ENABLE: {
> > > > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > > controller_.getAlgorithm("awb"));
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list