[libcamera-devel] [PATCH v4 2/2] ipa: rpi: common: Handle AEC/AGC flicker controls

[David Plowman]

Hi Jacopo

Thanks for the comments.

On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi
<jacopo.mondi at 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 at 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
> >