[libcamera-devel] [PATCH v2 3/7] libcamera: pipeline: raspberrypi: Support the new AE controls

[Naushir Patuck]

Hi Paul,

Thank you for your work.

On Fri, 1 Oct 2021 at 11:33, Paul Elder <paul.elder at ideasonboard.com> wrote:

> Add support for the new AE controls in the raspberrypi pipeline handler,
> and in the IPA.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v2:
> - fix the rebase error where some uvc stuff was in rasberrypi
> - i haven't yet taken in the comments to move the new Pause/Resume
>   functions
>
> Initial versoin:
> This is very hacky. I wasn't sure what the best way was to plumb it into
> the raspberrypi IPA as it was a bit hairy...
> ---
>  include/libcamera/ipa/raspberrypi.h        |  3 +-
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 +++++++++-
>  src/ipa/raspberrypi/controller/rpi/agc.hpp |  5 +++
>  src/ipa/raspberrypi/raspberrypi.cpp        | 42 +++++++++++++++++-----
>  4 files changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> index 521eaecd..363ea038 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -28,8 +28,9 @@ namespace RPi {
>   * unsupported control is encountered.
>   */
>  static const ControlInfoMap Controls({
> -               { &controls::AeEnable, ControlInfo(false, true) },
> +               { &controls::ExposureTimeMode,
> ControlInfo(controls::ExposureTimeModeValues) },
>                 { &controls::ExposureTime, ControlInfo(0, 999999) },
> +               { &controls::AnalogueGainMode,
> ControlInfo(controls::AnalogueGainModeValues) },
>                 { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
>                 { &controls::AeMeteringMode,
> ControlInfo(controls::AeMeteringModeValues) },
>                 { &controls::AeConstraintMode,
> ControlInfo(controls::AeConstraintModeValues) },
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 289c1fce..b45ea454 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -203,14 +203,30 @@ bool Agc::IsPaused() const
>  }
>
>  void Agc::Pause()
> +{
> +}
> +
> +void Agc::Resume()
> +{
> +}
> +
> +void Agc::PauseExposure()
>

One small suggestion, could this be called PauseShutter() instead?  Helps
clarify what part
of the exposure is paused.

Thanks,
Naush


>  {
>         fixed_shutter_ = status_.shutter_time;
> +}
> +
> +void Agc::PauseGain()
> +{
>         fixed_analogue_gain_ = status_.analogue_gain;
>  }
>
> -void Agc::Resume()
> +void Agc::ResumeExposure()
>  {
>         fixed_shutter_ = 0s;
> +}
> +
> +void Agc::ResumeGain()
> +{
>         fixed_analogue_gain_ = 0;
>  }
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 82063636..7ca3ca2f 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -92,6 +92,11 @@ public:
>         void Prepare(Metadata *image_metadata) override;
>         void Process(StatisticsPtr &stats, Metadata *image_metadata)
> override;
>
> +       void PauseExposure();
> +       void PauseGain();
> +       void ResumeExposure();
> +       void ResumeGain();
> +
>  private:
>         void updateLockStatus(DeviceStatus const &device_status);
>         AgcConfig config_;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 047123ab..99935515 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -53,6 +53,8 @@
>  #include "sharpen_algorithm.hpp"
>  #include "sharpen_status.h"
>
> +#include "controller/rpi/agc.hpp"
> +
>  namespace libcamera {
>
>  using namespace std::literals::chrono_literals;
> @@ -478,7 +480,10 @@ void IPARPi::reportMetadata()
>
>         AgcStatus *agcStatus =
> rpiMetadata_.GetLocked<AgcStatus>("agc.status");
>         if (agcStatus) {
> -               libcameraMetadata_.set(controls::AeLocked,
> agcStatus->locked);
> +               libcameraMetadata_.set(controls::AeState,
> +                                      agcStatus->locked ?
> +                                      controls::AeStateConverged :
> +                                      controls::AeStateSearching);
>                 libcameraMetadata_.set(controls::DigitalGain,
> agcStatus->digital_gain);
>         }
>
> @@ -623,20 +628,22 @@ void IPARPi::queueRequest(const ControlList
> &controls)
>                                   << " = " << ctrl.second.toString();
>
>                 switch (ctrl.first) {
> -               case controls::AE_ENABLE: {
> -                       RPiController::Algorithm *agc =
> controller_.GetAlgorithm("agc");
> +               case controls::EXPOSURE_TIME_MODE: {
> +                       RPiController::Algorithm *algo =
> controller_.GetAlgorithm("agc");
> +                       RPiController::Agc *agc =
> reinterpret_cast<RPiController::Agc *>(algo);
>                         if (!agc) {
>                                 LOG(IPARPI, Warning)
> -                                       << "Could not set AE_ENABLE - no
> AGC algorithm";
> +                                       << "Could not set
> EXPOSURE_TIME_MODE - no AGC algorithm";
>                                 break;
>                         }
>
> -                       if (ctrl.second.get<bool>() == false)
> -                               agc->Pause();
> +                       if (ctrl.second.get<int32_t>() ==
> controls::ExposureTimeModeDisabled)
> +                               agc->PauseExposure();
>                         else
> -                               agc->Resume();
> +                               agc->ResumeExposure();
>
> -                       libcameraMetadata_.set(controls::AeEnable,
> ctrl.second.get<bool>());
> +                       libcameraMetadata_.set(controls::ExposureTimeMode,
> +                                              ctrl.second.get<int32_t>());
>                         break;
>                 }
>
> @@ -656,6 +663,25 @@ void IPARPi::queueRequest(const ControlList &controls)
>                         break;
>                 }
>
> +               case controls::ANALOGUE_GAIN_MODE: {
> +                       RPiController::Algorithm *algo =
> controller_.GetAlgorithm("agc");
> +                       RPiController::Agc *agc =
> reinterpret_cast<RPiController::Agc *>(algo);
> +                       if (!agc) {
> +                               LOG(IPARPI, Warning)
> +                                       << "Could not set
> ANALOGUE_GAIN_MODE - no AGC algorithm";
> +                               break;
> +                       }
> +
> +                       if (ctrl.second.get<int32_t>() ==
> controls::AnalogueGainModeDisabled)
> +                               agc->PauseGain();
> +                       else
> +                               agc->ResumeGain();
> +
> +                       libcameraMetadata_.set(controls::AnalogueGainMode,
> +                                              ctrl.second.get<int32_t>());
> +                       break;
> +               }
> +
>                 case controls::ANALOGUE_GAIN: {
>                         RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
>                                 controller_.GetAlgorithm("agc"));
> --
> 2.27.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211004/2fd20a79/attachment.htm>