[libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add GetDropFrames method to AGC base class

[Naushir Patuck]

Hi David,

Thank you for your patch.

On Wed, 2 Dec 2020 at 11:53, David Plowman <david.plowman at raspberrypi.com>
wrote:

> We add a GetDropFrames method to the AgcAlgorithm class which can be
> called when the AGC is started from scratch. It suggests how many
> frames should be dropped before displaying any (while the AGC
> converges).
>
> The Raspberry Pi specific implementation makes this customisable from
> the tuning file.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/agc_algorithm.hpp |  1 +
>  src/ipa/raspberrypi/controller/rpi/agc.cpp       | 11 +++++++++++
>  src/ipa/raspberrypi/controller/rpi/agc.hpp       |  2 ++
>  3 files changed, 14 insertions(+)
>
> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> index b4ea54fb..bfc9743f 100644
> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> @@ -15,6 +15,7 @@ class AgcAlgorithm : public Algorithm
>  public:
>         AgcAlgorithm(Controller *controller) : Algorithm(controller) {}
>         // An AGC algorithm must provide the following:
> +       virtual unsigned int GetDropFrames() const = 0;
>         virtual void SetEv(double ev) = 0;
>         virtual void SetFlickerPeriod(double flicker_period) = 0;
>         virtual void SetFixedShutter(double fixed_shutter) = 0; //
> microseconds
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 9da18c31..94c02d47 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -142,6 +142,7 @@ void AgcConfig::Read(boost::property_tree::ptree const
> &params)
>         Y_target.Read(params.get_child("y_target"));
>         speed = params.get<double>("speed", 0.2);
>         startup_frames = params.get<uint16_t>("startup_frames", 10);
> +       drop_frames = params.get<unsigned int>("drop_frames", 6);
>         fast_reduce_threshold =
>                 params.get<double>("fast_reduce_threshold", 0.4);
>         base_ev = params.get<double>("base_ev", 1.0);
> @@ -206,6 +207,16 @@ void Agc::Resume()
>         fixed_analogue_gain_ = 0;
>  }
>
> +unsigned int Agc::GetDropFrames() const
> +{
> +       // If shutter and gain have been explicitly set, there is no
> +       // convergence to happen, so no need to drop any frames.
> +       if (fixed_shutter_ && fixed_analogue_gain_)
> +               return 0;
> +       else
> +               return config_.drop_frames;
> +}
> +
>

  One little nit on this one.  The method name,  GetDropFrames, doesn't sit
right with me.  Could we rename this to something like, say,
GetConvergenceFrames, or something.  I feel this would better convey what
this number is.  What do you think?

 void Agc::SetEv(double ev)
>  {
>         ev_ = ev;
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 95db1812..1de4d505 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -52,6 +52,7 @@ struct AgcConfig {
>         Pwl Y_target;
>         double speed;
>         uint16_t startup_frames;
> +       unsigned int drop_frames;
>

Similarly, instead of calling it drop_frames, we could call it
convergence_frames?

Regards,
Naush


>         double max_change;
>         double min_change;
>         double fast_reduce_threshold;
> @@ -74,6 +75,7 @@ public:
>         bool IsPaused() const override;
>         void Pause() override;
>         void Resume() override;
> +       unsigned int GetDropFrames() const override;
>         void SetEv(double ev) override;
>         void SetFlickerPeriod(double flicker_period) override;
>         void SetFixedShutter(double fixed_shutter) override; //
> microseconds
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201204/314b1d0a/attachment.htm>