<div dir="ltr"><div dir="ltr">Hi Nick,<div><br></div><div>Thank you for your patch.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 21 Nov 2022 at 14:47, Nick Hollinghurst via libcamera-devel <<a href="mailto:libcamera-devel@lists.libcamera.org">libcamera-devel@lists.libcamera.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">No existing Algorithm used the base pause(), resume() functions<br>
or the paused_ flag, nor is there a need or a generic pause API.<br>
Remove these. The AGC and AWB algorithms now have methods named<br>
disableAuto(), enableAuto() which better describe their functionality.<br>
<br>
Signed-off-by: Nick Hollinghurst <<a href="mailto:nick.hollinghurst@raspberrypi.com" target="_blank">nick.hollinghurst@raspberrypi.com</a>><br></blockquote><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
---<br>
 src/ipa/raspberrypi/controller/agc_algorithm.h |  2 ++<br>
 src/ipa/raspberrypi/controller/algorithm.h     |  6 +-----<br>
 src/ipa/raspberrypi/controller/awb_algorithm.h |  2 ++<br>
 src/ipa/raspberrypi/controller/controller.cpp  |  6 ++----<br>
 src/ipa/raspberrypi/controller/rpi/agc.cpp     | 13 ++++---------<br>
 src/ipa/raspberrypi/controller/rpi/agc.h       |  6 ++----<br>
 src/ipa/raspberrypi/controller/rpi/awb.cpp     | 11 +++--------<br>
 src/ipa/raspberrypi/controller/rpi/awb.h       |  6 ++----<br>
 src/ipa/raspberrypi/raspberrypi.cpp            | 14 ++++++++------<br>
 9 files changed, 26 insertions(+), 40 deletions(-)<br>
<br>
diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.h b/src/ipa/raspberrypi/controller/agc_algorithm.h<br>
index 3a91444c..36e6c110 100644<br>
--- a/src/ipa/raspberrypi/controller/agc_algorithm.h<br>
+++ b/src/ipa/raspberrypi/controller/agc_algorithm.h<br>
@@ -26,6 +26,8 @@ public:<br>
        virtual void setMeteringMode(std::string const &meteringModeName) = 0;<br>
        virtual void setExposureMode(std::string const &exposureModeName) = 0;<br>
        virtual void setConstraintMode(std::string const &contraintModeName) = 0;<br>
+       virtual void enableAuto() = 0;<br>
+       virtual void disableAuto() = 0;<br>
 };<br>
<br>
 } /* namespace RPiController */<br>
diff --git a/src/ipa/raspberrypi/controller/algorithm.h b/src/ipa/raspberrypi/controller/algorithm.h<br>
index cbbb13ba..4f327598 100644<br>
--- a/src/ipa/raspberrypi/controller/algorithm.h<br>
+++ b/src/ipa/raspberrypi/controller/algorithm.h<br>
@@ -27,14 +27,11 @@ class Algorithm<br>
 {<br>
 public:<br>
        Algorithm(Controller *controller)<br>
-               : controller_(controller), paused_(false)<br>
+               : controller_(controller)<br>
        {<br>
        }<br>
        virtual ~Algorithm() = default;<br>
        virtual char const *name() const = 0;<br>
-       virtual bool isPaused() const { return paused_; }<br>
-       virtual void pause() { paused_ = true; }<br>
-       virtual void resume() { paused_ = false; }<br>
        virtual int read(const libcamera::YamlObject &params);<br>
        virtual void initialise();<br>
        virtual void switchMode(CameraMode const &cameraMode, Metadata *metadata);<br>
@@ -47,7 +44,6 @@ public:<br>
<br>
 private:<br>
        Controller *controller_;<br>
-       bool paused_;<br>
 };<br>
<br>
 /*<br>
diff --git a/src/ipa/raspberrypi/controller/awb_algorithm.h b/src/ipa/raspberrypi/controller/awb_algorithm.h<br>
index 48e08b60..8462c4db 100644<br>
--- a/src/ipa/raspberrypi/controller/awb_algorithm.h<br>
+++ b/src/ipa/raspberrypi/controller/awb_algorithm.h<br>
@@ -18,6 +18,8 @@ public:<br>
        virtual unsigned int getConvergenceFrames() const = 0;<br>
        virtual void setMode(std::string const &modeName) = 0;<br>
        virtual void setManualGains(double manualR, double manualB) = 0;<br>
+       virtual void enableAuto() = 0;<br>
+       virtual void disableAuto() = 0;<br>
 };<br>
<br>
 } /* namespace RPiController */<br>
diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp<br>
index f4892786..e9156785 100644<br>
--- a/src/ipa/raspberrypi/controller/controller.cpp<br>
+++ b/src/ipa/raspberrypi/controller/controller.cpp<br>
@@ -108,16 +108,14 @@ void Controller::prepare(Metadata *imageMetadata)<br>
 {<br>
        assert(switchModeCalled_);<br>
        for (auto &algo : algorithms_)<br>
-               if (!algo->isPaused())<br>
-                       algo->prepare(imageMetadata);<br>
+               algo->prepare(imageMetadata);<br>
 }<br>
<br>
 void Controller::process(StatisticsPtr stats, Metadata *imageMetadata)<br>
 {<br>
        assert(switchModeCalled_);<br>
        for (auto &algo : algorithms_)<br>
-               if (!algo->isPaused())<br>
-                       algo->process(stats, imageMetadata);<br>
+               algo->process(stats, imageMetadata);<br>
 }<br>
<br>
 Metadata &Controller::getGlobalMetadata()<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
index bd54a639..a30e50c1 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
@@ -270,18 +270,13 @@ int Agc::read(const libcamera::YamlObject &params)<br>
        return 0;<br>
 }<br>
<br>
-bool Agc::isPaused() const<br>
-{<br>
-       return false;<br>
-}<br>
-<br>
-void Agc::pause()<br>
+void Agc::disableAuto()<br>
 {<br>
        fixedShutter_ = status_.shutterTime;<br>
        fixedAnalogueGain_ = status_.analogueGain;<br>
 }<br>
<br>
-void Agc::resume()<br>
+void Agc::enableAuto()<br>
 {<br>
        fixedShutter_ = 0s;<br>
        fixedAnalogueGain_ = 0;<br>
@@ -317,14 +312,14 @@ void Agc::setMaxShutter(Duration maxShutter)<br>
 void Agc::setFixedShutter(Duration fixedShutter)<br>
 {<br>
        fixedShutter_ = fixedShutter;<br>
-       /* Set this in case someone calls Pause() straight after. */<br>
+       /* Set this in case someone calls disableAuto() straight after. */<br>
        status_.shutterTime = clipShutter(fixedShutter_);<br>
 }<br>
<br>
 void Agc::setFixedAnalogueGain(double fixedAnalogueGain)<br>
 {<br>
        fixedAnalogueGain_ = fixedAnalogueGain;<br>
-       /* Set this in case someone calls Pause() straight after. */<br>
+       /* Set this in case someone calls disableAuto() straight after. */<br>
        status_.analogueGain = fixedAnalogueGain;<br>
 }<br>
<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h<br>
index 6d6b0e5a..cf04da19 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/agc.h<br>
+++ b/src/ipa/raspberrypi/controller/rpi/agc.h<br>
@@ -75,10 +75,6 @@ public:<br>
        Agc(Controller *controller);<br>
        char const *name() const override;<br>
        int read(const libcamera::YamlObject &params) override;<br>
-       /* AGC handles "pausing" for itself. */<br>
-       bool isPaused() const override;<br>
-       void pause() override;<br>
-       void resume() override;<br>
        unsigned int getConvergenceFrames() const override;<br>
        void setEv(double ev) override;<br>
        void setFlickerPeriod(libcamera::utils::Duration flickerPeriod) override;<br>
@@ -88,6 +84,8 @@ public:<br>
        void setMeteringMode(std::string const &meteringModeName) override;<br>
        void setExposureMode(std::string const &exposureModeName) override;<br>
        void setConstraintMode(std::string const &contraintModeName) override;<br>
+       void enableAuto() override;<br>
+       void disableAuto() override;<br>
        void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;<br>
        void prepare(Metadata *imageMetadata) override;<br>
        void process(StatisticsPtr &stats, Metadata *imageMetadata) override;<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
index 8d8ddf09..4f6af4ba 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
@@ -222,21 +222,16 @@ void Awb::initialise()<br>
        asyncResults_ = syncResults_;<br>
 }<br>
<br>
-bool Awb::isPaused() const<br>
+void Awb::disableAuto()<br>
 {<br>
-       return false;<br>
-}<br>
-<br>
-void Awb::pause()<br>
-{<br>
-       /* "Pause" by fixing everything to the most recent values. */<br>
+       /* Freeze the most recent values, and treat them as manual gains */<br>
        manualR_ = syncResults_.gainR = prevSyncResults_.gainR;<br>
        manualB_ = syncResults_.gainB = prevSyncResults_.gainB;<br>
        syncResults_.gainG = prevSyncResults_.gainG;<br>
        syncResults_.temperatureK = prevSyncResults_.temperatureK;<br>
 }<br>
<br>
-void Awb::resume()<br>
+void Awb::enableAuto()<br>
 {<br>
        manualR_ = 0.0;<br>
        manualB_ = 0.0;<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h b/src/ipa/raspberrypi/controller/rpi/awb.h<br>
index 30acd89d..2254c3ed 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/awb.h<br>
+++ b/src/ipa/raspberrypi/controller/rpi/awb.h<br>
@@ -93,13 +93,11 @@ public:<br>
        char const *name() const override;<br>
        void initialise() override;<br>
        int read(const libcamera::YamlObject &params) override;<br>
-       /* AWB handles "pausing" for itself. */<br>
-       bool isPaused() const override;<br>
-       void pause() override;<br>
-       void resume() override;<br>
        unsigned int getConvergenceFrames() const override;<br>
        void setMode(std::string const &name) override;<br>
        void setManualGains(double manualR, double manualB) override;<br>
+       void enableAuto() override;<br>
+       void disableAuto() override;<br>
        void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;<br>
        void prepare(Metadata *imageMetadata) override;<br>
        void process(StatisticsPtr &stats, Metadata *imageMetadata) override;<br>
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
index b74f1ecf..beb076dc 100644<br>
--- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
+++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
@@ -706,7 +706,8 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
<br>
                switch (ctrl.first) {<br>
                case controls::AE_ENABLE: {<br>
-                       RPiController::Algorithm *agc = controller_.getAlgorithm("agc");<br>
+                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(<br>
+                               controller_.getAlgorithm("agc"));<br>
                        if (!agc) {<br>
                                LOG(IPARPI, Warning)<br>
                                        << "Could not set AE_ENABLE - no AGC algorithm";<br>
@@ -714,9 +715,9 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
                        }<br>
<br>
                        if (ctrl.second.get<bool>() == false)<br>
-                               agc->pause();<br>
+                               agc->disableAuto();<br>
                        else<br>
-                               agc->resume();<br>
+                               agc->enableAuto();<br>
<br>
                        libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());<br>
                        break;<br>
@@ -835,7 +836,8 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
                }<br>
<br>
                case controls::AWB_ENABLE: {<br>
-                       RPiController::Algorithm *awb = controller_.getAlgorithm("awb");<br>
+                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(<br>
+                               controller_.getAlgorithm("awb"));<br>
                        if (!awb) {<br>
                                LOG(IPARPI, Warning)<br>
                                        << "Could not set AWB_ENABLE - no AWB algorithm";<br>
@@ -843,9 +845,9 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
                        }<br>
<br>
                        if (ctrl.second.get<bool>() == false)<br>
-                               awb->pause();<br>
+                               awb->disableAuto();<br>
                        else<br>
-                               awb->resume();<br>
+                               awb->enableAuto();<br>
<br>
                        libcameraMetadata_.set(controls::AwbEnable,<br>
                                               ctrl.second.get<bool>());<br>
-- <br>
2.20.1<br>
<br>
</blockquote></div></div>