| Message ID | CAPhyPA6Wm-kdAm8LVoO3Mz+88=P3-0FiAa0VZi9qLVmNei9KBQ@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nick, Thank you for the patch. On Mon, Nov 21, 2022 at 02:21:05PM +0000, Nick Hollinghurst via libcamera-devel wrote: > No existing Algorithm used the base pause(), resume() functions > or the paused_ flag, nor is there a need or a generic pause API. > Remove these. The AGC and AWB algorithms now have methods named > disableAuto(), enableAuto() which better describe their functionality. > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/agc_algorithm.h | 2 ++ > src/ipa/raspberrypi/controller/algorithm.h | 6 +----- > src/ipa/raspberrypi/controller/awb_algorithm.h | 2 ++ > src/ipa/raspberrypi/controller/controller.cpp | 6 ++---- > src/ipa/raspberrypi/controller/rpi/agc.cpp | 13 ++++--------- > src/ipa/raspberrypi/controller/rpi/agc.h | 6 ++---- > src/ipa/raspberrypi/controller/rpi/awb.cpp | 11 +++-------- > src/ipa/raspberrypi/controller/rpi/awb.h | 6 ++---- > src/ipa/raspberrypi/raspberrypi.cpp | 14 ++++++++------ > 9 files changed, 26 insertions(+), 40 deletions(-) > > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.h > b/src/ipa/raspberrypi/controller/agc_algorithm.h > index 3a91444c..36e6c110 100644 > --- a/src/ipa/raspberrypi/controller/agc_algorithm.h > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.h > @@ -26,6 +26,8 @@ public: > virtual void setMeteringMode(std::string const &meteringModeName) = 0; > virtual void setExposureMode(std::string const &exposureModeName) = 0; > virtual void setConstraintMode(std::string const &contraintModeName) = 0; > + virtual void enableAuto() = 0; > + virtual void disableAuto() = 0; It looks like your mail client and/or server mess up with white space. I recommend using git-send-email to send patches, you will avoid lots of trouble. > }; > > } /* namespace RPiController */ > diff --git a/src/ipa/raspberrypi/controller/algorithm.h > b/src/ipa/raspberrypi/controller/algorithm.h > index cbbb13ba..4f327598 100644 > --- a/src/ipa/raspberrypi/controller/algorithm.h > +++ b/src/ipa/raspberrypi/controller/algorithm.h > @@ -27,14 +27,11 @@ class Algorithm > { > public: > Algorithm(Controller *controller) > - : controller_(controller), paused_(false) > + : controller_(controller) > { > } > virtual ~Algorithm() = default; > virtual char const *name() const = 0; > - virtual bool isPaused() const { return paused_; } > - virtual void pause() { paused_ = true; } > - virtual void resume() { paused_ = false; } > virtual int read(const libcamera::YamlObject ¶ms); > virtual void initialise(); > virtual void switchMode(CameraMode const &cameraMode, Metadata *metadata); > @@ -47,7 +44,6 @@ public: > > private: > Controller *controller_; > - bool paused_; > }; > > /* > diff --git a/src/ipa/raspberrypi/controller/awb_algorithm.h > b/src/ipa/raspberrypi/controller/awb_algorithm.h > index 48e08b60..8462c4db 100644 > --- a/src/ipa/raspberrypi/controller/awb_algorithm.h > +++ b/src/ipa/raspberrypi/controller/awb_algorithm.h > @@ -18,6 +18,8 @@ public: > virtual unsigned int getConvergenceFrames() const = 0; > virtual void setMode(std::string const &modeName) = 0; > virtual void setManualGains(double manualR, double manualB) = 0; > + virtual void enableAuto() = 0; > + virtual void disableAuto() = 0; > }; > > } /* namespace RPiController */ > diff --git a/src/ipa/raspberrypi/controller/controller.cpp > b/src/ipa/raspberrypi/controller/controller.cpp > index f4892786..e9156785 100644 > --- a/src/ipa/raspberrypi/controller/controller.cpp > +++ b/src/ipa/raspberrypi/controller/controller.cpp > @@ -108,16 +108,14 @@ void Controller::prepare(Metadata *imageMetadata) > { > assert(switchModeCalled_); > for (auto &algo : algorithms_) > - if (!algo->isPaused()) > - algo->prepare(imageMetadata); > + algo->prepare(imageMetadata); > } > > void Controller::process(StatisticsPtr stats, Metadata *imageMetadata) > { > assert(switchModeCalled_); > for (auto &algo : algorithms_) > - if (!algo->isPaused()) > - algo->process(stats, imageMetadata); > + algo->process(stats, imageMetadata); > } > > Metadata &Controller::getGlobalMetadata() > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp > b/src/ipa/raspberrypi/controller/rpi/agc.cpp > index bd54a639..a30e50c1 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > @@ -270,18 +270,13 @@ int Agc::read(const libcamera::YamlObject ¶ms) > return 0; > } > > -bool Agc::isPaused() const > -{ > - return false; > -} > - > -void Agc::pause() > +void Agc::disableAuto() > { > fixedShutter_ = status_.shutterTime; > fixedAnalogueGain_ = status_.analogueGain; > } > > -void Agc::resume() > +void Agc::enableAuto() > { > fixedShutter_ = 0s; > fixedAnalogueGain_ = 0; > @@ -317,14 +312,14 @@ void Agc::setMaxShutter(Duration maxShutter) > void Agc::setFixedShutter(Duration fixedShutter) > { > fixedShutter_ = fixedShutter; > - /* Set this in case someone calls Pause() straight after. */ > + /* Set this in case someone calls disableAuto() straight after. */ > status_.shutterTime = clipShutter(fixedShutter_); > } > > void Agc::setFixedAnalogueGain(double fixedAnalogueGain) > { > fixedAnalogueGain_ = fixedAnalogueGain; > - /* Set this in case someone calls Pause() straight after. */ > + /* Set this in case someone calls disableAuto() straight after. */ > status_.analogueGain = fixedAnalogueGain; > } > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h > b/src/ipa/raspberrypi/controller/rpi/agc.h > index 6d6b0e5a..cf04da19 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.h > +++ b/src/ipa/raspberrypi/controller/rpi/agc.h > @@ -75,10 +75,6 @@ public: > Agc(Controller *controller); > char const *name() const override; > int read(const libcamera::YamlObject ¶ms) override; > - /* AGC handles "pausing" for itself. */ > - bool isPaused() const override; > - void pause() override; > - void resume() override; > unsigned int getConvergenceFrames() const override; > void setEv(double ev) override; > void setFlickerPeriod(libcamera::utils::Duration flickerPeriod) override; > @@ -88,6 +84,8 @@ public: > void setMeteringMode(std::string const &meteringModeName) override; > void setExposureMode(std::string const &exposureModeName) override; > void setConstraintMode(std::string const &contraintModeName) override; > + void enableAuto() override; > + void disableAuto() override; > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > void prepare(Metadata *imageMetadata) override; > void process(StatisticsPtr &stats, Metadata *imageMetadata) override; > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp > b/src/ipa/raspberrypi/controller/rpi/awb.cpp > index 8d8ddf09..4f6af4ba 100644 > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp > @@ -222,21 +222,16 @@ void Awb::initialise() > asyncResults_ = syncResults_; > } > > -bool Awb::isPaused() const > +void Awb::disableAuto() > { > - return false; > -} > - > -void Awb::pause() > -{ > - /* "Pause" by fixing everything to the most recent values. */ > + /* Freeze the most recent values, and treat them as manual gains */ > manualR_ = syncResults_.gainR = prevSyncResults_.gainR; > manualB_ = syncResults_.gainB = prevSyncResults_.gainB; > syncResults_.gainG = prevSyncResults_.gainG; > syncResults_.temperatureK = prevSyncResults_.temperatureK; > } > > -void Awb::resume() > +void Awb::enableAuto() > { > manualR_ = 0.0; > manualB_ = 0.0; > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h > b/src/ipa/raspberrypi/controller/rpi/awb.h > index 30acd89d..2254c3ed 100644 > --- a/src/ipa/raspberrypi/controller/rpi/awb.h > +++ b/src/ipa/raspberrypi/controller/rpi/awb.h > @@ -93,13 +93,11 @@ public: > char const *name() const override; > void initialise() override; > int read(const libcamera::YamlObject ¶ms) override; > - /* AWB handles "pausing" for itself. */ > - bool isPaused() const override; > - void pause() override; > - void resume() override; > unsigned int getConvergenceFrames() const override; > void setMode(std::string const &name) override; > void setManualGains(double manualR, double manualB) override; > + void enableAuto() override; > + void disableAuto() override; > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > void prepare(Metadata *imageMetadata) override; > void process(StatisticsPtr &stats, Metadata *imageMetadata) override; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > index b74f1ecf..beb076dc 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -706,7 +706,8 @@ 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")); > if (!agc) { > LOG(IPARPI, Warning) > << "Could not set AE_ENABLE - no AGC algorithm"; > @@ -714,9 +715,9 @@ void IPARPi::queueRequest(const ControlList &controls) > } > > if (ctrl.second.get<bool>() == false) > - agc->pause(); > + agc->disableAuto(); > else > - agc->resume(); > + agc->enableAuto(); > > libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>()); > break; > @@ -835,7 +836,8 @@ void IPARPi::queueRequest(const ControlList &controls) > } > > case controls::AWB_ENABLE: { > - RPiController::Algorithm *awb = controller_.getAlgorithm("awb"); > + RPiController::AwbAlgorithm *awb = > dynamic_cast<RPiController::AwbAlgorithm *>( > + controller_.getAlgorithm("awb")); > if (!awb) { > LOG(IPARPI, Warning) > << "Could not set AWB_ENABLE - no AWB algorithm"; > @@ -843,9 +845,9 @@ void IPARPi::queueRequest(const ControlList &controls) > } > > if (ctrl.second.get<bool>() == false) > - awb->pause(); > + awb->disableAuto(); > else > - awb->resume(); > + awb->enableAuto(); > > libcameraMetadata_.set(controls::AwbEnable, > ctrl.second.get<bool>());
Hi Nick Thanks for the patch! Just a couple of nit-picks... (sorry!) Firstly, the first line of the commit message would normally be "ipa: raspberrypi: ..." to follow the established pattern. Secondly, the indentation doesn't look the same as other patches. Not sure what's happened there or if it's a problem? But the content all looks fine to me. Thanks! David On Mon, 21 Nov 2022 at 14:21, Nick Hollinghurst via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > No existing Algorithm used the base pause(), resume() functions > or the paused_ flag, nor is there a need or a generic pause API. > Remove these. The AGC and AWB algorithms now have methods named > disableAuto(), enableAuto() which better describe their functionality. > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/agc_algorithm.h | 2 ++ > src/ipa/raspberrypi/controller/algorithm.h | 6 +----- > src/ipa/raspberrypi/controller/awb_algorithm.h | 2 ++ > src/ipa/raspberrypi/controller/controller.cpp | 6 ++---- > src/ipa/raspberrypi/controller/rpi/agc.cpp | 13 ++++--------- > src/ipa/raspberrypi/controller/rpi/agc.h | 6 ++---- > src/ipa/raspberrypi/controller/rpi/awb.cpp | 11 +++-------- > src/ipa/raspberrypi/controller/rpi/awb.h | 6 ++---- > src/ipa/raspberrypi/raspberrypi.cpp | 14 ++++++++------ > 9 files changed, 26 insertions(+), 40 deletions(-) > > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.h > b/src/ipa/raspberrypi/controller/agc_algorithm.h > index 3a91444c..36e6c110 100644 > --- a/src/ipa/raspberrypi/controller/agc_algorithm.h > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.h > @@ -26,6 +26,8 @@ public: > virtual void setMeteringMode(std::string const &meteringModeName) = 0; > virtual void setExposureMode(std::string const &exposureModeName) = 0; > virtual void setConstraintMode(std::string const &contraintModeName) = 0; > + virtual void enableAuto() = 0; > + virtual void disableAuto() = 0; > }; > > } /* namespace RPiController */ > diff --git a/src/ipa/raspberrypi/controller/algorithm.h > b/src/ipa/raspberrypi/controller/algorithm.h > index cbbb13ba..4f327598 100644 > --- a/src/ipa/raspberrypi/controller/algorithm.h > +++ b/src/ipa/raspberrypi/controller/algorithm.h > @@ -27,14 +27,11 @@ class Algorithm > { > public: > Algorithm(Controller *controller) > - : controller_(controller), paused_(false) > + : controller_(controller) > { > } > virtual ~Algorithm() = default; > virtual char const *name() const = 0; > - virtual bool isPaused() const { return paused_; } > - virtual void pause() { paused_ = true; } > - virtual void resume() { paused_ = false; } > virtual int read(const libcamera::YamlObject ¶ms); > virtual void initialise(); > virtual void switchMode(CameraMode const &cameraMode, Metadata *metadata); > @@ -47,7 +44,6 @@ public: > > private: > Controller *controller_; > - bool paused_; > }; > > /* > diff --git a/src/ipa/raspberrypi/controller/awb_algorithm.h > b/src/ipa/raspberrypi/controller/awb_algorithm.h > index 48e08b60..8462c4db 100644 > --- a/src/ipa/raspberrypi/controller/awb_algorithm.h > +++ b/src/ipa/raspberrypi/controller/awb_algorithm.h > @@ -18,6 +18,8 @@ public: > virtual unsigned int getConvergenceFrames() const = 0; > virtual void setMode(std::string const &modeName) = 0; > virtual void setManualGains(double manualR, double manualB) = 0; > + virtual void enableAuto() = 0; > + virtual void disableAuto() = 0; > }; > > } /* namespace RPiController */ > diff --git a/src/ipa/raspberrypi/controller/controller.cpp > b/src/ipa/raspberrypi/controller/controller.cpp > index f4892786..e9156785 100644 > --- a/src/ipa/raspberrypi/controller/controller.cpp > +++ b/src/ipa/raspberrypi/controller/controller.cpp > @@ -108,16 +108,14 @@ void Controller::prepare(Metadata *imageMetadata) > { > assert(switchModeCalled_); > for (auto &algo : algorithms_) > - if (!algo->isPaused()) > - algo->prepare(imageMetadata); > + algo->prepare(imageMetadata); > } > > void Controller::process(StatisticsPtr stats, Metadata *imageMetadata) > { > assert(switchModeCalled_); > for (auto &algo : algorithms_) > - if (!algo->isPaused()) > - algo->process(stats, imageMetadata); > + algo->process(stats, imageMetadata); > } > > Metadata &Controller::getGlobalMetadata() > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp > b/src/ipa/raspberrypi/controller/rpi/agc.cpp > index bd54a639..a30e50c1 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > @@ -270,18 +270,13 @@ int Agc::read(const libcamera::YamlObject ¶ms) > return 0; > } > > -bool Agc::isPaused() const > -{ > - return false; > -} > - > -void Agc::pause() > +void Agc::disableAuto() > { > fixedShutter_ = status_.shutterTime; > fixedAnalogueGain_ = status_.analogueGain; > } > > -void Agc::resume() > +void Agc::enableAuto() > { > fixedShutter_ = 0s; > fixedAnalogueGain_ = 0; > @@ -317,14 +312,14 @@ void Agc::setMaxShutter(Duration maxShutter) > void Agc::setFixedShutter(Duration fixedShutter) > { > fixedShutter_ = fixedShutter; > - /* Set this in case someone calls Pause() straight after. */ > + /* Set this in case someone calls disableAuto() straight after. */ > status_.shutterTime = clipShutter(fixedShutter_); > } > > void Agc::setFixedAnalogueGain(double fixedAnalogueGain) > { > fixedAnalogueGain_ = fixedAnalogueGain; > - /* Set this in case someone calls Pause() straight after. */ > + /* Set this in case someone calls disableAuto() straight after. */ > status_.analogueGain = fixedAnalogueGain; > } > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h > b/src/ipa/raspberrypi/controller/rpi/agc.h > index 6d6b0e5a..cf04da19 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.h > +++ b/src/ipa/raspberrypi/controller/rpi/agc.h > @@ -75,10 +75,6 @@ public: > Agc(Controller *controller); > char const *name() const override; > int read(const libcamera::YamlObject ¶ms) override; > - /* AGC handles "pausing" for itself. */ > - bool isPaused() const override; > - void pause() override; > - void resume() override; > unsigned int getConvergenceFrames() const override; > void setEv(double ev) override; > void setFlickerPeriod(libcamera::utils::Duration flickerPeriod) override; > @@ -88,6 +84,8 @@ public: > void setMeteringMode(std::string const &meteringModeName) override; > void setExposureMode(std::string const &exposureModeName) override; > void setConstraintMode(std::string const &contraintModeName) override; > + void enableAuto() override; > + void disableAuto() override; > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > void prepare(Metadata *imageMetadata) override; > void process(StatisticsPtr &stats, Metadata *imageMetadata) override; > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp > b/src/ipa/raspberrypi/controller/rpi/awb.cpp > index 8d8ddf09..4f6af4ba 100644 > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp > @@ -222,21 +222,16 @@ void Awb::initialise() > asyncResults_ = syncResults_; > } > > -bool Awb::isPaused() const > +void Awb::disableAuto() > { > - return false; > -} > - > -void Awb::pause() > -{ > - /* "Pause" by fixing everything to the most recent values. */ > + /* Freeze the most recent values, and treat them as manual gains */ > manualR_ = syncResults_.gainR = prevSyncResults_.gainR; > manualB_ = syncResults_.gainB = prevSyncResults_.gainB; > syncResults_.gainG = prevSyncResults_.gainG; > syncResults_.temperatureK = prevSyncResults_.temperatureK; > } > > -void Awb::resume() > +void Awb::enableAuto() > { > manualR_ = 0.0; > manualB_ = 0.0; > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h > b/src/ipa/raspberrypi/controller/rpi/awb.h > index 30acd89d..2254c3ed 100644 > --- a/src/ipa/raspberrypi/controller/rpi/awb.h > +++ b/src/ipa/raspberrypi/controller/rpi/awb.h > @@ -93,13 +93,11 @@ public: > char const *name() const override; > void initialise() override; > int read(const libcamera::YamlObject ¶ms) override; > - /* AWB handles "pausing" for itself. */ > - bool isPaused() const override; > - void pause() override; > - void resume() override; > unsigned int getConvergenceFrames() const override; > void setMode(std::string const &name) override; > void setManualGains(double manualR, double manualB) override; > + void enableAuto() override; > + void disableAuto() override; > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > void prepare(Metadata *imageMetadata) override; > void process(StatisticsPtr &stats, Metadata *imageMetadata) override; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > index b74f1ecf..beb076dc 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -706,7 +706,8 @@ 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")); > if (!agc) { > LOG(IPARPI, Warning) > << "Could not set AE_ENABLE - no AGC algorithm"; > @@ -714,9 +715,9 @@ void IPARPi::queueRequest(const ControlList &controls) > } > > if (ctrl.second.get<bool>() == false) > - agc->pause(); > + agc->disableAuto(); > else > - agc->resume(); > + agc->enableAuto(); > > libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>()); > break; > @@ -835,7 +836,8 @@ void IPARPi::queueRequest(const ControlList &controls) > } > > case controls::AWB_ENABLE: { > - RPiController::Algorithm *awb = controller_.getAlgorithm("awb"); > + RPiController::AwbAlgorithm *awb = > dynamic_cast<RPiController::AwbAlgorithm *>( > + controller_.getAlgorithm("awb")); > if (!awb) { > LOG(IPARPI, Warning) > << "Could not set AWB_ENABLE - no AWB algorithm"; > @@ -843,9 +845,9 @@ void IPARPi::queueRequest(const ControlList &controls) > } > > if (ctrl.second.get<bool>() == false) > - awb->pause(); > + awb->disableAuto(); > else > - awb->resume(); > + awb->enableAuto(); > > libcameraMetadata_.set(controls::AwbEnable, > ctrl.second.get<bool>()); > -- > 2.20.1
Whoops. I will try that again... Thanks, Nick On Mon, 21 Nov 2022 at 14:30, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Nick, > > Thank you for the patch. > > On Mon, Nov 21, 2022 at 02:21:05PM +0000, Nick Hollinghurst via libcamera-devel wrote: > > No existing Algorithm used the base pause(), resume() functions > > or the paused_ flag, nor is there a need or a generic pause API. > > Remove these. The AGC and AWB algorithms now have methods named > > disableAuto(), enableAuto() which better describe their functionality. > > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/agc_algorithm.h | 2 ++ > > src/ipa/raspberrypi/controller/algorithm.h | 6 +----- > > src/ipa/raspberrypi/controller/awb_algorithm.h | 2 ++ > > src/ipa/raspberrypi/controller/controller.cpp | 6 ++---- > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 13 ++++--------- > > src/ipa/raspberrypi/controller/rpi/agc.h | 6 ++---- > > src/ipa/raspberrypi/controller/rpi/awb.cpp | 11 +++-------- > > src/ipa/raspberrypi/controller/rpi/awb.h | 6 ++---- > > src/ipa/raspberrypi/raspberrypi.cpp | 14 ++++++++------ > > 9 files changed, 26 insertions(+), 40 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.h > > b/src/ipa/raspberrypi/controller/agc_algorithm.h > > index 3a91444c..36e6c110 100644 > > --- a/src/ipa/raspberrypi/controller/agc_algorithm.h > > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.h > > @@ -26,6 +26,8 @@ public: > > virtual void setMeteringMode(std::string const &meteringModeName) = 0; > > virtual void setExposureMode(std::string const &exposureModeName) = 0; > > virtual void setConstraintMode(std::string const &contraintModeName) = 0; > > + virtual void enableAuto() = 0; > > + virtual void disableAuto() = 0; > > It looks like your mail client and/or server mess up with white space. I > recommend using git-send-email to send patches, you will avoid lots of > trouble. > > > }; > > > > } /* namespace RPiController */ > > diff --git a/src/ipa/raspberrypi/controller/algorithm.h > > b/src/ipa/raspberrypi/controller/algorithm.h > > index cbbb13ba..4f327598 100644 > > --- a/src/ipa/raspberrypi/controller/algorithm.h > > +++ b/src/ipa/raspberrypi/controller/algorithm.h > > @@ -27,14 +27,11 @@ class Algorithm > > { > > public: > > Algorithm(Controller *controller) > > - : controller_(controller), paused_(false) > > + : controller_(controller) > > { > > } > > virtual ~Algorithm() = default; > > virtual char const *name() const = 0; > > - virtual bool isPaused() const { return paused_; } > > - virtual void pause() { paused_ = true; } > > - virtual void resume() { paused_ = false; } > > virtual int read(const libcamera::YamlObject ¶ms); > > virtual void initialise(); > > virtual void switchMode(CameraMode const &cameraMode, Metadata *metadata); > > @@ -47,7 +44,6 @@ public: > > > > private: > > Controller *controller_; > > - bool paused_; > > }; > > > > /* > > diff --git a/src/ipa/raspberrypi/controller/awb_algorithm.h > > b/src/ipa/raspberrypi/controller/awb_algorithm.h > > index 48e08b60..8462c4db 100644 > > --- a/src/ipa/raspberrypi/controller/awb_algorithm.h > > +++ b/src/ipa/raspberrypi/controller/awb_algorithm.h > > @@ -18,6 +18,8 @@ public: > > virtual unsigned int getConvergenceFrames() const = 0; > > virtual void setMode(std::string const &modeName) = 0; > > virtual void setManualGains(double manualR, double manualB) = 0; > > + virtual void enableAuto() = 0; > > + virtual void disableAuto() = 0; > > }; > > > > } /* namespace RPiController */ > > diff --git a/src/ipa/raspberrypi/controller/controller.cpp > > b/src/ipa/raspberrypi/controller/controller.cpp > > index f4892786..e9156785 100644 > > --- a/src/ipa/raspberrypi/controller/controller.cpp > > +++ b/src/ipa/raspberrypi/controller/controller.cpp > > @@ -108,16 +108,14 @@ void Controller::prepare(Metadata *imageMetadata) > > { > > assert(switchModeCalled_); > > for (auto &algo : algorithms_) > > - if (!algo->isPaused()) > > - algo->prepare(imageMetadata); > > + algo->prepare(imageMetadata); > > } > > > > void Controller::process(StatisticsPtr stats, Metadata *imageMetadata) > > { > > assert(switchModeCalled_); > > for (auto &algo : algorithms_) > > - if (!algo->isPaused()) > > - algo->process(stats, imageMetadata); > > + algo->process(stats, imageMetadata); > > } > > > > Metadata &Controller::getGlobalMetadata() > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > index bd54a639..a30e50c1 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > @@ -270,18 +270,13 @@ int Agc::read(const libcamera::YamlObject ¶ms) > > return 0; > > } > > > > -bool Agc::isPaused() const > > -{ > > - return false; > > -} > > - > > -void Agc::pause() > > +void Agc::disableAuto() > > { > > fixedShutter_ = status_.shutterTime; > > fixedAnalogueGain_ = status_.analogueGain; > > } > > > > -void Agc::resume() > > +void Agc::enableAuto() > > { > > fixedShutter_ = 0s; > > fixedAnalogueGain_ = 0; > > @@ -317,14 +312,14 @@ void Agc::setMaxShutter(Duration maxShutter) > > void Agc::setFixedShutter(Duration fixedShutter) > > { > > fixedShutter_ = fixedShutter; > > - /* Set this in case someone calls Pause() straight after. */ > > + /* Set this in case someone calls disableAuto() straight after. */ > > status_.shutterTime = clipShutter(fixedShutter_); > > } > > > > void Agc::setFixedAnalogueGain(double fixedAnalogueGain) > > { > > fixedAnalogueGain_ = fixedAnalogueGain; > > - /* Set this in case someone calls Pause() straight after. */ > > + /* Set this in case someone calls disableAuto() straight after. */ > > status_.analogueGain = fixedAnalogueGain; > > } > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h > > b/src/ipa/raspberrypi/controller/rpi/agc.h > > index 6d6b0e5a..cf04da19 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/agc.h > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.h > > @@ -75,10 +75,6 @@ public: > > Agc(Controller *controller); > > char const *name() const override; > > int read(const libcamera::YamlObject ¶ms) override; > > - /* AGC handles "pausing" for itself. */ > > - bool isPaused() const override; > > - void pause() override; > > - void resume() override; > > unsigned int getConvergenceFrames() const override; > > void setEv(double ev) override; > > void setFlickerPeriod(libcamera::utils::Duration flickerPeriod) override; > > @@ -88,6 +84,8 @@ public: > > void setMeteringMode(std::string const &meteringModeName) override; > > void setExposureMode(std::string const &exposureModeName) override; > > void setConstraintMode(std::string const &contraintModeName) override; > > + void enableAuto() override; > > + void disableAuto() override; > > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > > void prepare(Metadata *imageMetadata) override; > > void process(StatisticsPtr &stats, Metadata *imageMetadata) override; > > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp > > b/src/ipa/raspberrypi/controller/rpi/awb.cpp > > index 8d8ddf09..4f6af4ba 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp > > @@ -222,21 +222,16 @@ void Awb::initialise() > > asyncResults_ = syncResults_; > > } > > > > -bool Awb::isPaused() const > > +void Awb::disableAuto() > > { > > - return false; > > -} > > - > > -void Awb::pause() > > -{ > > - /* "Pause" by fixing everything to the most recent values. */ > > + /* Freeze the most recent values, and treat them as manual gains */ > > manualR_ = syncResults_.gainR = prevSyncResults_.gainR; > > manualB_ = syncResults_.gainB = prevSyncResults_.gainB; > > syncResults_.gainG = prevSyncResults_.gainG; > > syncResults_.temperatureK = prevSyncResults_.temperatureK; > > } > > > > -void Awb::resume() > > +void Awb::enableAuto() > > { > > manualR_ = 0.0; > > manualB_ = 0.0; > > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h > > b/src/ipa/raspberrypi/controller/rpi/awb.h > > index 30acd89d..2254c3ed 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/awb.h > > +++ b/src/ipa/raspberrypi/controller/rpi/awb.h > > @@ -93,13 +93,11 @@ public: > > char const *name() const override; > > void initialise() override; > > int read(const libcamera::YamlObject ¶ms) override; > > - /* AWB handles "pausing" for itself. */ > > - bool isPaused() const override; > > - void pause() override; > > - void resume() override; > > unsigned int getConvergenceFrames() const override; > > void setMode(std::string const &name) override; > > void setManualGains(double manualR, double manualB) override; > > + void enableAuto() override; > > + void disableAuto() override; > > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > > void prepare(Metadata *imageMetadata) override; > > void process(StatisticsPtr &stats, Metadata *imageMetadata) override; > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > > b/src/ipa/raspberrypi/raspberrypi.cpp > > index b74f1ecf..beb076dc 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -706,7 +706,8 @@ 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")); > > if (!agc) { > > LOG(IPARPI, Warning) > > << "Could not set AE_ENABLE - no AGC algorithm"; > > @@ -714,9 +715,9 @@ void IPARPi::queueRequest(const ControlList &controls) > > } > > > > if (ctrl.second.get<bool>() == false) > > - agc->pause(); > > + agc->disableAuto(); > > else > > - agc->resume(); > > + agc->enableAuto(); > > > > libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>()); > > break; > > @@ -835,7 +836,8 @@ void IPARPi::queueRequest(const ControlList &controls) > > } > > > > case controls::AWB_ENABLE: { > > - RPiController::Algorithm *awb = controller_.getAlgorithm("awb"); > > + RPiController::AwbAlgorithm *awb = > > dynamic_cast<RPiController::AwbAlgorithm *>( > > + controller_.getAlgorithm("awb")); > > if (!awb) { > > LOG(IPARPI, Warning) > > << "Could not set AWB_ENABLE - no AWB algorithm"; > > @@ -843,9 +845,9 @@ void IPARPi::queueRequest(const ControlList &controls) > > } > > > > if (ctrl.second.get<bool>() == false) > > - awb->pause(); > > + awb->disableAuto(); > > else > > - awb->resume(); > > + awb->enableAuto(); > > > > libcameraMetadata_.set(controls::AwbEnable, > > ctrl.second.get<bool>()); > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.h b/src/ipa/raspberrypi/controller/agc_algorithm.h index 3a91444c..36e6c110 100644 --- a/src/ipa/raspberrypi/controller/agc_algorithm.h +++ b/src/ipa/raspberrypi/controller/agc_algorithm.h @@ -26,6 +26,8 @@ public: virtual void setMeteringMode(std::string const &meteringModeName) = 0; virtual void setExposureMode(std::string const &exposureModeName) = 0; virtual void setConstraintMode(std::string const &contraintModeName) = 0; + virtual void enableAuto() = 0; + virtual void disableAuto() = 0; }; } /* namespace RPiController */ diff --git a/src/ipa/raspberrypi/controller/algorithm.h b/src/ipa/raspberrypi/controller/algorithm.h index cbbb13ba..4f327598 100644 --- a/src/ipa/raspberrypi/controller/algorithm.h +++ b/src/ipa/raspberrypi/controller/algorithm.h @@ -27,14 +27,11 @@ class Algorithm { public: Algorithm(Controller *controller) - : controller_(controller), paused_(false) + : controller_(controller) { } virtual ~Algorithm() = default; virtual char const *name() const = 0; - virtual bool isPaused() const { return paused_; } - virtual void pause() { paused_ = true; } - virtual void resume() { paused_ = false; } virtual int read(const libcamera::YamlObject ¶ms); virtual void initialise(); virtual void switchMode(CameraMode const &cameraMode, Metadata *metadata); @@ -47,7 +44,6 @@ public: private: Controller *controller_; - bool paused_; }; /* diff --git a/src/ipa/raspberrypi/controller/awb_algorithm.h b/src/ipa/raspberrypi/controller/awb_algorithm.h index 48e08b60..8462c4db 100644 --- a/src/ipa/raspberrypi/controller/awb_algorithm.h +++ b/src/ipa/raspberrypi/controller/awb_algorithm.h @@ -18,6 +18,8 @@ public: virtual unsigned int getConvergenceFrames() const = 0; virtual void setMode(std::string const &modeName) = 0; virtual void setManualGains(double manualR, double manualB) = 0; + virtual void enableAuto() = 0; + virtual void disableAuto() = 0; }; } /* namespace RPiController */ diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp index f4892786..e9156785 100644 --- a/src/ipa/raspberrypi/controller/controller.cpp +++ b/src/ipa/raspberrypi/controller/controller.cpp @@ -108,16 +108,14 @@ void Controller::prepare(Metadata *imageMetadata) { assert(switchModeCalled_); for (auto &algo : algorithms_) - if (!algo->isPaused()) - algo->prepare(imageMetadata); + algo->prepare(imageMetadata); } void Controller::process(StatisticsPtr stats, Metadata *imageMetadata) { assert(switchModeCalled_); for (auto &algo : algorithms_) - if (!algo->isPaused()) - algo->process(stats, imageMetadata); + algo->process(stats, imageMetadata); } Metadata &Controller::getGlobalMetadata() diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp index bd54a639..a30e50c1 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp @@ -270,18 +270,13 @@ int Agc::read(const libcamera::YamlObject ¶ms) return 0; } -bool Agc::isPaused() const -{ - return false; -} - -void Agc::pause() +void Agc::disableAuto() { fixedShutter_ = status_.shutterTime; fixedAnalogueGain_ = status_.analogueGain; } -void Agc::resume() +void Agc::enableAuto() { fixedShutter_ = 0s; fixedAnalogueGain_ = 0; @@ -317,14 +312,14 @@ void Agc::setMaxShutter(Duration maxShutter) void Agc::setFixedShutter(Duration fixedShutter) { fixedShutter_ = fixedShutter; - /* Set this in case someone calls Pause() straight after. */ + /* Set this in case someone calls disableAuto() straight after. */ status_.shutterTime = clipShutter(fixedShutter_); } void Agc::setFixedAnalogueGain(double fixedAnalogueGain) { fixedAnalogueGain_ = fixedAnalogueGain; - /* Set this in case someone calls Pause() straight after. */ + /* Set this in case someone calls disableAuto() straight after. */ status_.analogueGain = fixedAnalogueGain; } diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h index 6d6b0e5a..cf04da19 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.h +++ b/src/ipa/raspberrypi/controller/rpi/agc.h @@ -75,10 +75,6 @@ public: Agc(Controller *controller); char const *name() const override; int read(const libcamera::YamlObject ¶ms) override; - /* AGC handles "pausing" for itself. */ - bool isPaused() const override; - void pause() override; - void resume() override; unsigned int getConvergenceFrames() const override; void setEv(double ev) override; void setFlickerPeriod(libcamera::utils::Duration flickerPeriod) override; @@ -88,6 +84,8 @@ public: void setMeteringMode(std::string const &meteringModeName) override; void setExposureMode(std::string const &exposureModeName) override; void setConstraintMode(std::string const &contraintModeName) override; + void enableAuto() override; + void disableAuto() override; void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; void prepare(Metadata *imageMetadata) override; void process(StatisticsPtr &stats, Metadata *imageMetadata) override; diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp index 8d8ddf09..4f6af4ba 100644 --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp @@ -222,21 +222,16 @@ void Awb::initialise() asyncResults_ = syncResults_; } -bool Awb::isPaused() const +void Awb::disableAuto() { - return false; -} - -void Awb::pause() -{ - /* "Pause" by fixing everything to the most recent values. */ + /* Freeze the most recent values, and treat them as manual gains */ manualR_ = syncResults_.gainR = prevSyncResults_.gainR; manualB_ = syncResults_.gainB = prevSyncResults_.gainB; syncResults_.gainG = prevSyncResults_.gainG; syncResults_.temperatureK = prevSyncResults_.temperatureK; } -void Awb::resume() +void Awb::enableAuto() { manualR_ = 0.0; manualB_ = 0.0; diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h b/src/ipa/raspberrypi/controller/rpi/awb.h index 30acd89d..2254c3ed 100644 --- a/src/ipa/raspberrypi/controller/rpi/awb.h +++ b/src/ipa/raspberrypi/controller/rpi/awb.h @@ -93,13 +93,11 @@ public: char const *name() const override; void initialise() override; int read(const libcamera::YamlObject ¶ms) override; - /* AWB handles "pausing" for itself. */ - bool isPaused() const override; - void pause() override; - void resume() override; unsigned int getConvergenceFrames() const override; void setMode(std::string const &name) override; void setManualGains(double manualR, double manualB) override; + void enableAuto() override; + void disableAuto() override; void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; void prepare(Metadata *imageMetadata) override; void process(StatisticsPtr &stats, Metadata *imageMetadata) override; diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index b74f1ecf..beb076dc 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -706,7 +706,8 @@ 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")); if (!agc) { LOG(IPARPI, Warning)
No existing Algorithm used the base pause(), resume() functions or the paused_ flag, nor is there a need or a generic pause API. Remove these. The AGC and AWB algorithms now have methods named disableAuto(), enableAuto() which better describe their functionality. Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> --- src/ipa/raspberrypi/controller/agc_algorithm.h | 2 ++ src/ipa/raspberrypi/controller/algorithm.h | 6 +----- src/ipa/raspberrypi/controller/awb_algorithm.h | 2 ++ src/ipa/raspberrypi/controller/controller.cpp | 6 ++---- src/ipa/raspberrypi/controller/rpi/agc.cpp | 13 ++++--------- src/ipa/raspberrypi/controller/rpi/agc.h | 6 ++---- src/ipa/raspberrypi/controller/rpi/awb.cpp | 11 +++-------- src/ipa/raspberrypi/controller/rpi/awb.h | 6 ++---- src/ipa/raspberrypi/raspberrypi.cpp | 14 ++++++++------ 9 files changed, 26 insertions(+), 40 deletions(-) << "Could not set AE_ENABLE - no AGC algorithm"; @@ -714,9 +715,9 @@ void IPARPi::queueRequest(const ControlList &controls) } if (ctrl.second.get<bool>() == false) - agc->pause(); + agc->disableAuto(); else - agc->resume(); + agc->enableAuto(); libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>()); break; @@ -835,7 +836,8 @@ void IPARPi::queueRequest(const ControlList &controls) } case controls::AWB_ENABLE: { - RPiController::Algorithm *awb = controller_.getAlgorithm("awb"); + RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( + controller_.getAlgorithm("awb")); if (!awb) { LOG(IPARPI, Warning) << "Could not set AWB_ENABLE - no AWB algorithm"; @@ -843,9 +845,9 @@ void IPARPi::queueRequest(const ControlList &controls) } if (ctrl.second.get<bool>() == false) - awb->pause(); + awb->disableAuto(); else - awb->resume(); + awb->enableAuto(); libcameraMetadata_.set(controls::AwbEnable, ctrl.second.get<bool>());