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

[Naushir Patuck]

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?

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
>