[libcamera-devel] [PATCH 2/3] src: ipa: raspberrypi: Improve behaviour when AE disabled

[Naushir Patuck]

Hi David,

Thank you for the patch.

On Wed, 25 Nov 2020 at 11:36, David Plowman <david.plowman at raspberrypi.com>
wrote:

> AE/AGC "disabled" is now implemented by leaving it running but fixing
> the exposure/gain to the last values we requested. This behaves better
> when, for example, a fixed shutter or gain is then specified - the
> other value remains fixed and doesn't start floating again.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 48 ++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343..30516665 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -67,7 +67,8 @@ public:
>         IPARPi()
>                 : lastMode_({}), controller_(), controllerInit_(false),
>                   frameCount_(0), checkCount_(0), mistrustCount_(0),
> -                 lsTable_(nullptr)
> +                 lsTable_(nullptr),
> +                 lastShutter_(0), lastGain_(0)
>         {
>         }
>
> @@ -145,6 +146,10 @@ private:
>         /* LS table allocation passed in from the pipeline handler. */
>         FileDescriptor lsTableHandle_;
>         void *lsTable_;
> +
> +       /* For enabling/disabling the AEC/AGC algorithm. */
> +       uint32_t lastShutter_;
> +       float lastGain_;
>  };
>
>  int IPARPi::init(const IPASettings &settings)
> @@ -493,12 +498,22 @@ void IPARPi::queueRequest(const ControlList
> &controls)
>
>                 switch (ctrl.first) {
>                 case controls::AE_ENABLE: {
> -                       RPiController::Algorithm *agc =
> controller_.GetAlgorithm("agc");
> +                       RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> +                               controller_.GetAlgorithm("agc"));
>                         ASSERT(agc);
> -                       if (ctrl.second.get<bool>() == false)
> -                               agc->Pause();
> -                       else
> -                               agc->Resume();
> +
> +                       /*
> +                        * We always run the AGC even if it's "disabled".
> +                        * Both exposure and gain are "fixed" to the last
> +                        * values it produced.
> +                        */
> +                       if (ctrl.second.get<bool>() == false) {
> +                               agc->SetFixedShutter(lastShutter_);
> +                               agc->SetFixedAnalogueGain(lastGain_);
> +                       } else {
> +                               agc->SetFixedShutter(0);
> +                               agc->SetFixedAnalogueGain(0);
> +                       }
>

I wonder if we could do this within the AGC controller itself?  So we keep
the existing Pause()/Resume() logic in the IPA, and have the AGC do the
above block on the appropriate API call.  This way the IPA would not need
to track the last used exposure/gain values that are already stored within
the AGC block?

The rest of the changes would still be valid with this tweak.

Regards,
Naush



>
>                         libcameraMetadata_.set(controls::AeEnable,
> ctrl.second.get<bool>());
>                         break;
> @@ -510,13 +525,10 @@ void IPARPi::queueRequest(const ControlList
> &controls)
>                         ASSERT(agc);
>
>                         /* This expects units of micro-seconds. */
> -                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> +                       int32_t shutter = ctrl.second.get<int32_t>();
> +                       agc->SetFixedShutter(shutter);
>
> -                       /* For the manual values to take effect, AGC must
> be unpaused. */
> -                       if (agc->IsPaused())
> -                               agc->Resume();
> -
> -                       libcameraMetadata_.set(controls::ExposureTime,
> ctrl.second.get<int32_t>());
> +                       libcameraMetadata_.set(controls::ExposureTime,
> shutter);
>                         break;
>                 }
>
> @@ -524,14 +536,11 @@ void IPARPi::queueRequest(const ControlList
> &controls)
>                         RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
>                                 controller_.GetAlgorithm("agc"));
>                         ASSERT(agc);
> -
>  agc->SetFixedAnalogueGain(ctrl.second.get<float>());
>
> -                       /* For the manual values to take effect, AGC must
> be unpaused. */
> -                       if (agc->IsPaused())
> -                               agc->Resume();
> +                       float gain = ctrl.second.get<float>();
> +                       agc->SetFixedAnalogueGain(gain);
>
> -                       libcameraMetadata_.set(controls::AnalogueGain,
> -                                              ctrl.second.get<float>());
> +                       libcameraMetadata_.set(controls::AnalogueGain,
> gain);
>                         break;
>                 }
>
> @@ -861,6 +870,9 @@ void IPARPi::applyAWB(const struct AwbStatus
> *awbStatus, ControlList &ctrls)
>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls)
>  {
> +       lastShutter_ = agcStatus->shutter_time;
> +       lastGain_ = agcStatus->analogue_gain;
> +
>         int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
>         int32_t exposureLines =
> helper_->ExposureLines(agcStatus->shutter_time);
>
> --
> 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/20201126/337fa52b/attachment.htm>