| Message ID | 20220811150219.62066-5-jacopo@jmondi.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@jmondi.org> Reviewed-by: Paul Elder <paul.elder@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 >
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@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@jmondi.org> > > Reviewed-by: Paul Elder <paul.elder@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 > >
Hi David On Thu, Aug 18, 2022 at 03:25:35PM +0100, David Plowman wrote: > 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. > Yes, we discussed about having a single big button to get into full manual mode. In the same way we can have an helper to implement "aperture" and "shutter" priorities. My understanding is those belong to the upper layers. In your case the libcamera-apps or the python bindings. I wouldn't be against having those helpers as part of libcamera itself, the design is not clear to me yet but I wouldn't mind > 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". Yes you can send both of them as part of the same Request. True that without precise per-frame control the AECG machinery would be switched to manual mode immediately and the desired exposure time/gain would be realized only a few frames later. The IPA can be made smart and handle this case with some ad-hoc code. > > 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. > If there's a way to trigger such behaviour I can help > 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@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@jmondi.org> > > > > Reviewed-by: Paul Elder <paul.elder@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 > > >
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));
The newly introduced controls to driver the AEGC algorithm allow to control the computation of the exposure time and analogue gain 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 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@jmondi.org> --- .../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(-)