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

[Jacopo Mondi]

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 at 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 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);
> > +
> > +                               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 at raspberrypi.com>
>
> > +
> >                 case controls::AWB_ENABLE: {
> >                         RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> >                                 controller_.getAlgorithm("awb"));
> > --
> > 2.30.2
> >