[libcamera-devel,RFC,2/4] libcamera: pipeline: raspberrypi: Support the new AE controls
diff mbox series

Message ID 20210928074959.3489544-3-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Fix pipelines for the new AE-related controls
Related show

Commit Message

Paul Elder Sept. 28, 2021, 7:49 a.m. UTC 
Add support for the new AE controls in the raspberrypi pipeline handler,
and in the IPA.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
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 ++++++++++++++++----
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 ++++----
 5 files changed, 67 insertions(+), 18 deletions(-)

Comments

David Plowman Sept. 28, 2021, 9:04 a.m. UTC | #1 
Hi Paul

Thanks for this patch. I think it looks good mostly, there might just
be a couple of small tweaks that I would suggest.

On Tue, 28 Sept 2021 at 08:50, Paul Elder <paul.elder@ideasonboard.com> wrote:
>
> Add support for the new AE controls in the raspberrypi pipeline handler,
> and in the IPA.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> 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 ++++++++++++++++----
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 ++++----
>  5 files changed, 67 insertions(+), 18 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) },

I guess no global control just yet... :)

> +               { &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()
>  {
>         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;
>  }

Clearly we need these new Pause/Resume Exposure/Gain methods, but of
course we're still left with the old ones (just Pause/Resume) as our
Algorithm interface requires them. Maybe they should still do what
they used to do? (perhaps where each calls the two new functions) This
might support a global enable/disable at some point, should we ever
have one!

>
> 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();
> +

I think I'd like to see these go as pure virtual functions into
agc_algorithm.hpp, and then we override them here. The idea is that
our controller will work with any AGC algorithm that implements the
interface in agc_algorithm.hpp (so Foo Corporation could replace ours
with their own) and I think this would break that.

>  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);

I really want to replace "locked" by "converged" here, but that's a
patch for another day!

> +               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);

Ah yes, here it is. We should probably cast to an
RPiController::AgcAlgorithm here. Once those new Pause/Resume
functions have been moved to that class then a cast to AgcAlgorithm
should be fine.

>                         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);

Same thing here.

> +                       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"));
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index c227d885..3a9c3b8d 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -309,25 +309,25 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>                 /* \todo Make this nicer. */
>                 int32_t ivalue;
>                 if (id == controls::ExposureTimeMode && exposureSet) {
> -                       int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);
> -                       ivalue = value.get<int32_t>() == ExposureTimeModeAuto
> +                       int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
> +                       ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
>                                ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY
>                                   ? V4L2_EXPOSURE_AUTO
>                                   : V4L2_EXPOSURE_APERTURE_PRIORITY)
>                                : V4L2_EXPOSURE_MANUAL;
>                 } else if (id == controls::ExposureTimeMode && !exposureSet) {
> -                       ivalue = value.get<int32_t>() == ExposureTimeModeAuto
> +                       ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
>                                ? V4L2_EXPOSURE_APERTURE_PRIORITY
>                                : V4L2_EXPOSURE_MANUAL;
>                 } else if (id == controls::AnalogueGainMode && exposureSet) {
> -                       int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);
> -                       ivalue = value.get<int32_t>() == AnalogueGainModeAuto
> +                       int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
> +                       ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
>                                ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY
>                                   ? V4L2_EXPOSURE_AUTO
>                                   : V4L2_EXPOSURE_SHUTTER_PRIORITY)
>                                : V4L2_EXPOSURE_MANUAL;
>                 } else if (id == controls::AnalogueGainMode && !exposureSet) {
> -                       ivalue = value.get<int32_t>() == AnalogueGainModeAuto
> +                       ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
>                                ? V4L2_EXPOSURE_SHUTTER_PRIORITY
>                                : V4L2_EXPOSURE_MANUAL;
>                 }
> @@ -636,8 +636,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>                 break;
>
>         case V4L2_CID_EXPOSURE_AUTO:
> -               info = ControlInfo{ { ExposureTimeModeAuto, ExposureTimeModeDisabled },
> -                                   ExposureTimeModeDisabled };
> +               info = ControlInfo{ { controls::ExposureTimeModeAuto,
> +                                     controls::ExposureTimeModeDisabled },
> +                                   controls::ExposureTimeModeDisabled };
>                 break;
>
>         case V4L2_CID_EXPOSURE_ABSOLUTE:
> --
> 2.27.0
>

(Was this UVC stuff meant to be in the same patch?)

Apart from those small things this is looking pretty close to me!

Thanks

David
Paul Elder Sept. 28, 2021, 9:51 a.m. UTC | #2 
Hi David,

On Tue, Sep 28, 2021 at 10:04:20AM +0100, David Plowman wrote:
> Hi Paul
> 
> Thanks for this patch. I think it looks good mostly, there might just
> be a couple of small tweaks that I would suggest.

Thank you for the review!

> 
> On Tue, 28 Sept 2021 at 08:50, Paul Elder <paul.elder@ideasonboard.com> wrote:
> >
> > Add support for the new AE controls in the raspberrypi pipeline handler,
> > and in the IPA.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > 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 ++++++++++++++++----
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 ++++----
> >  5 files changed, 67 insertions(+), 18 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) },
> 
> I guess no global control just yet... :)

That's a \todo :D

> 
> > +               { &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()
> >  {
> >         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;
> >  }
> 
> Clearly we need these new Pause/Resume Exposure/Gain methods, but of

Phew, glad that was the right direction.

> course we're still left with the old ones (just Pause/Resume) as our
> Algorithm interface requires them. Maybe they should still do what
> they used to do? (perhaps where each calls the two new functions) This

Ah, good idea.

> might support a global enable/disable at some point, should we ever
> have one!
> 
> >
> > 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();
> > +
> 
> I think I'd like to see these go as pure virtual functions into
> agc_algorithm.hpp, and then we override them here. The idea is that
> our controller will work with any AGC algorithm that implements the
> interface in agc_algorithm.hpp (so Foo Corporation could replace ours
> with their own) and I think this would break that.

I see; I'll move them there then.

> 
> >  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);
> 
> I really want to replace "locked" by "converged" here, but that's a
> patch for another day!
> 
> > +               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);
> 
> Ah yes, here it is. We should probably cast to an
> RPiController::AgcAlgorithm here. Once those new Pause/Resume
> functions have been moved to that class then a cast to AgcAlgorithm
> should be fine.

Aah okay.

> 
> >                         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);
> 
> Same thing here.
> 
> > +                       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"));
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index c227d885..3a9c3b8d 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -309,25 +309,25 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >                 /* \todo Make this nicer. */
> >                 int32_t ivalue;
> >                 if (id == controls::ExposureTimeMode && exposureSet) {
> > -                       int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);
> > -                       ivalue = value.get<int32_t>() == ExposureTimeModeAuto
> > +                       int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
> > +                       ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
> >                                ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY
> >                                   ? V4L2_EXPOSURE_AUTO
> >                                   : V4L2_EXPOSURE_APERTURE_PRIORITY)
> >                                : V4L2_EXPOSURE_MANUAL;
> >                 } else if (id == controls::ExposureTimeMode && !exposureSet) {
> > -                       ivalue = value.get<int32_t>() == ExposureTimeModeAuto
> > +                       ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
> >                                ? V4L2_EXPOSURE_APERTURE_PRIORITY
> >                                : V4L2_EXPOSURE_MANUAL;
> >                 } else if (id == controls::AnalogueGainMode && exposureSet) {
> > -                       int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);
> > -                       ivalue = value.get<int32_t>() == AnalogueGainModeAuto
> > +                       int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
> > +                       ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
> >                                ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY
> >                                   ? V4L2_EXPOSURE_AUTO
> >                                   : V4L2_EXPOSURE_SHUTTER_PRIORITY)
> >                                : V4L2_EXPOSURE_MANUAL;
> >                 } else if (id == controls::AnalogueGainMode && !exposureSet) {
> > -                       ivalue = value.get<int32_t>() == AnalogueGainModeAuto
> > +                       ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
> >                                ? V4L2_EXPOSURE_SHUTTER_PRIORITY
> >                                : V4L2_EXPOSURE_MANUAL;
> >                 }
> > @@ -636,8 +636,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >                 break;
> >
> >         case V4L2_CID_EXPOSURE_AUTO:
> > -               info = ControlInfo{ { ExposureTimeModeAuto, ExposureTimeModeDisabled },
> > -                                   ExposureTimeModeDisabled };
> > +               info = ControlInfo{ { controls::ExposureTimeModeAuto,
> > +                                     controls::ExposureTimeModeDisabled },
> > +                                   controls::ExposureTimeModeDisabled };
> >                 break;
> >
> >         case V4L2_CID_EXPOSURE_ABSOLUTE:
> > --
> > 2.27.0
> >
> 
> (Was this UVC stuff meant to be in the same patch?)

Oops, must be a rebase mistake :S

> 
> Apart from those small things this is looking pretty close to me!

Glad to hear that!

(Just wondering, have you seen the patch that adds these new controls
yet?)


Thanks,

Paul

Patch
diff mbox series 

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()
 {
 	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"));
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index c227d885..3a9c3b8d 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -309,25 +309,25 @@  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
 		/* \todo Make this nicer. */
 		int32_t ivalue;
 		if (id == controls::ExposureTimeMode && exposureSet) {
-			int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);
-			ivalue = value.get<int32_t>() == ExposureTimeModeAuto
+			int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
+			ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
 			       ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY
 				  ? V4L2_EXPOSURE_AUTO
 				  : V4L2_EXPOSURE_APERTURE_PRIORITY)
 			       : V4L2_EXPOSURE_MANUAL;
 		} else if (id == controls::ExposureTimeMode && !exposureSet) {
-			ivalue = value.get<int32_t>() == ExposureTimeModeAuto
+			ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
 			       ? V4L2_EXPOSURE_APERTURE_PRIORITY
 			       : V4L2_EXPOSURE_MANUAL;
 		} else if (id == controls::AnalogueGainMode && exposureSet) {
-			int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);
-			ivalue = value.get<int32_t>() == AnalogueGainModeAuto
+			int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
+			ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
 			       ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY
 				  ? V4L2_EXPOSURE_AUTO
 				  : V4L2_EXPOSURE_SHUTTER_PRIORITY)
 			       : V4L2_EXPOSURE_MANUAL;
 		} else if (id == controls::AnalogueGainMode && !exposureSet) {
-			ivalue = value.get<int32_t>() == AnalogueGainModeAuto
+			ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
 			       ? V4L2_EXPOSURE_SHUTTER_PRIORITY
 			       : V4L2_EXPOSURE_MANUAL;
 		}
@@ -636,8 +636,9 @@  void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
 		break;
 
 	case V4L2_CID_EXPOSURE_AUTO:
-		info = ControlInfo{ { ExposureTimeModeAuto, ExposureTimeModeDisabled },
-				    ExposureTimeModeDisabled };
+		info = ControlInfo{ { controls::ExposureTimeModeAuto,
+				      controls::ExposureTimeModeDisabled },
+				    controls::ExposureTimeModeDisabled };
 		break;
 
 	case V4L2_CID_EXPOSURE_ABSOLUTE: