[libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Handle AEC/AGC flicker controls

[Laurent Pinchart]

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 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