[libcamera-devel] [PATCH v2 4/6] ipa: raspberry: Port to the new AEGC controls
David Plowman
david.plowman at raspberrypi.com
Thu Aug 18 16:25:35 CEST 2022
Hi Jacopo
Thanks for doing this work! I think mostly this seemed reasonable to
me, there were just a few things I wanted to clarify about the desired
behaviour - it seems such a long time ago that we first discussed
this.
1. How to turn AEGC on/off
As I understand it now, you have to pause both the "shutter" and
"gain" explicitly. I think we talked about having some way to "turn
everything on/off", given that an application might not know exactly
how many different things there are that it has to worry about.
Has there been any further thought about this? Actually I'm less
interested in the C++ API these days, so maybe it's up to me to make
this work nicely in Python-land.
2 Setting manual values
My reading of this is now that manual values are ignored unless you're
in the "manual" mode. I guess this is OK so long as you can send
"manual mode" along with the "manual value".
Actually I wasn't quite sure if the Raspberry Pi implementation would
behave correctly in this respect, there's maybe an issue related to
processing controls in the correct order. So perhaps something to look
at there.
Here too I wonder about doing something to make the interface a bit
easier for our Python users, but that sounds like my problem.
I think that was pretty much everything that came to mind, so thanks
for doing this work!
Best regards
David
On Tue, 16 Aug 2022 at 06:31, Paul Elder via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Hi Jacopo,
>
> On Thu, Aug 11, 2022 at 05:02:17PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > The newly introduced controls to driver the AEGC algorithm allow
>
> s/driver/drive/
>
> > to control the computation of the exposure time and analogue gain
>
> s/to control/controlling/
>
> > separately.
> >
> > The RPi AEGC implementation already computes the shutter and gain
> > values separately but does not expose separate functions to control
> > them.
> >
> > Augment the AgcAlgorithm interface to allow to pause/resume the shutter
>
> s#to pause/resume#pausing/resuming#
>
> > and gain automatic computations separately and plumb them to the newly
> > introduced controls.
> >
> > Add safety checks to ignore ExposureTime and AnalogueGain values if the
> > algorithms are not paused, and report the correct AeState value by
> > checking if both algorithms have been paused or if they have converged.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
> > ---
> > .../raspberrypi/controller/agc_algorithm.h | 6 ++
> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 +++++-
> > src/ipa/raspberrypi/controller/rpi/agc.h | 8 +-
> > src/ipa/raspberrypi/raspberrypi.cpp | 76 ++++++++++++++++---
> > 4 files changed, 99 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.h b/src/ipa/raspberrypi/controller/agc_algorithm.h
> > index 3a91444c3a61..bf9c501db553 100644
> > --- a/src/ipa/raspberrypi/controller/agc_algorithm.h
> > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.h
> > @@ -17,6 +17,12 @@ class AgcAlgorithm : public Algorithm
> > public:
> > AgcAlgorithm(Controller *controller) : Algorithm(controller) {}
> > /* An AGC algorithm must provide the following: */
> > + virtual void pauseShutter();
> > + virtual void resumeShutter();
> > + virtual bool isShutterPaused() const;
> > + virtual void pauseGain();
> > + virtual void resumeGain();
> > + virtual bool isGainPaused() const;
> > virtual unsigned int getConvergenceFrames() const = 0;
> > virtual void setEv(double ev) = 0;
> > virtual void setFlickerPeriod(libcamera::utils::Duration flickerPeriod) = 0;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index bd54a639d637..3d04724349f7 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -275,18 +275,36 @@ bool Agc::isPaused() const
> > return false;
> > }
> >
> > -void Agc::pause()
> > +void Agc::pauseShutter()
> > {
> > fixedShutter_ = status_.shutterTime;
> > - fixedAnalogueGain_ = status_.analogueGain;
> > }
> >
> > -void Agc::resume()
> > +void Agc::resumeShutter()
> > {
> > fixedShutter_ = 0s;
> > +}
> > +
> > +bool Agc::isShutterPaused() const
> > +{
> > + return fixedShutter_ != 0s;
> > +}
> > +
> > +void Agc::pauseGain()
> > +{
> > + fixedAnalogueGain_ = status_.analogueGain;
> > +}
> > +
> > +void Agc::resumeGain()
> > +{
> > fixedAnalogueGain_ = 0;
> > }
> >
> > +bool Agc::isGainPaused() const
> > +{
> > + return fixedAnalogueGain_ != 0;
> > +}
> > +
> > unsigned int Agc::getConvergenceFrames() const
> > {
> > /*
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h
> > index 6d6b0e5ad857..cfb57f41848b 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.h
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.h
> > @@ -77,8 +77,12 @@ public:
> > int read(const libcamera::YamlObject ¶ms) override;
> > /* AGC handles "pausing" for itself. */
> > bool isPaused() const override;
> > - void pause() override;
> > - void resume() override;
> > + void pauseShutter() override;
> > + void resumeShutter() override;
> > + bool isShutterPaused() const override;
> > + void pauseGain() override;
> > + void resumeGain() override;
> > + bool isGainPaused() const override;
> > unsigned int getConvergenceFrames() const override;
> > void setEv(double ev) override;
> > void setFlickerPeriod(libcamera::utils::Duration flickerPeriod) override;
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 69c73f8c780a..c041aac008eb 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -74,8 +74,13 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
> >
> > /* List of controls handled by the Raspberry Pi IPA */
> > static const ControlInfoMap::Map ipaControls{
> > - { &controls::AeEnable, ControlInfo(false, true) },
> > + { &controls::ExposureTimeMode,
> > + ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> > + static_cast<int32_t>(controls::ExposureTimeModeManual)) },
> > { &controls::ExposureTime, ControlInfo(0, 66666) },
> > + { &controls::AnalogueGainMode,
> > + ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> > + static_cast<int32_t>(controls::AnalogueGainModeManual)) },
> > { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
> > { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > @@ -556,9 +561,18 @@ void IPARPi::reportMetadata()
> > }
> >
> > AgcStatus *agcStatus = rpiMetadata_.getLocked<AgcStatus>("agc.status");
> > - if (agcStatus) {
> > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);
> > + if (agcStatus)
> > libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);
> > +
> > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > + controller_.getAlgorithm("agc"));
> > + if (agc) {
> > + if (agc->isShutterPaused() && agc->isGainPaused())
> > + libcameraMetadata_.set(controls::AeState, controls::AeStateIdle);
> > + else if (agcStatus)
> > + libcameraMetadata_.set(controls::AeState,
> > + agcStatus->locked ? controls::AeStateConverged
> > + : controls::AeStateSearching);
> > }
> >
> > LuxStatus *luxStatus = rpiMetadata_.getLocked<LuxStatus>("lux.status");
> > @@ -703,20 +717,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::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > + controller_.getAlgorithm("agc"));
> > 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::ExposureTimeModeManual)
> > + agc->pauseShutter();
> > else
> > - agc->resume();
> > + agc->resumeShutter();
> >
> > - libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
> > + libcameraMetadata_.set(controls::ExposureTimeMode,
> > + ctrl.second.get<int32_t>());
> > break;
> > }
> >
> > @@ -729,6 +745,13 @@ void IPARPi::queueRequest(const ControlList &controls)
> > break;
> > }
> >
> > + /*
> > + * Ignore manual exposure time when the auto exposure
> > + * algorithm is running.
> > + */
> > + if (!agc->isShutterPaused())
> > + break;
> > +
> > /* The control provides units of microseconds. */
> > agc->setFixedShutter(ctrl.second.get<int32_t>() * 1.0us);
> >
> > @@ -736,6 +759,25 @@ void IPARPi::queueRequest(const ControlList &controls)
> > break;
> > }
> >
> > + case controls::ANALOGUE_GAIN_MODE: {
> > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > + controller_.getAlgorithm("agc"));
> > + if (!agc) {
> > + LOG(IPARPI, Warning)
> > + << "Could not set ANALOGUE_GAIN_MODE - no AGC algorithm";
> > + break;
> > + }
> > +
> > + if (ctrl.second.get<int32_t>() == controls::AnalogueGainModeManual)
> > + 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"));
> > @@ -745,6 +787,13 @@ void IPARPi::queueRequest(const ControlList &controls)
> > break;
> > }
> >
> > + /*
> > + * Ignore manual analogue gain value when the auto gain
> > + * algorithm is running.
> > + */
> > + if (!agc->isGainPaused())
> > + break;
> > +
> > agc->setFixedAnalogueGain(ctrl.second.get<float>());
> >
> > libcameraMetadata_.set(controls::AnalogueGain,
> > @@ -801,6 +850,13 @@ void IPARPi::queueRequest(const ControlList &controls)
> > break;
> > }
> >
> > + /*
> > + * Ignore AE_EXPOSURE_MODE if the shutter or the gain
> > + * are in auto mode.
> > + */
> > + if (!agc->isShutterPaused() || !agc->isGainPaused())
> > + break;
> > +
> > int32_t idx = ctrl.second.get<int32_t>();
> > if (ExposureModeTable.count(idx)) {
> > agc->setExposureMode(ExposureModeTable.at(idx));
> > --
> > 2.37.1
> >
More information about the libcamera-devel
mailing list