[libcamera-devel,v2,3/7] libcamera: pipeline: raspberrypi: Support the new AE controls
diff mbox series

Message ID 20211001103325.1077590-4-paul.elder@ideasonboard.com
State Superseded
Delegated to: Paul Elder
Headers show
Series
  • The Great AE Changes
Related show

Commit Message

Paul Elder Oct. 1, 2021, 10:33 a.m. UTC 
Add support for the new AE controls in the raspberrypi pipeline handler,
and in the IPA.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- fix the rebase error where some uvc stuff was in rasberrypi
- i haven't yet taken in the comments to move the new Pause/Resume
  functions

Initial versoin:
This is very hacky. I wasn't sure what the best way was to plumb it into
the raspberrypi IPA as it was a bit hairy...
---
 include/libcamera/ipa/raspberrypi.h        |  3 +-
 src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 +++++++++-
 src/ipa/raspberrypi/controller/rpi/agc.hpp |  5 +++
 src/ipa/raspberrypi/raspberrypi.cpp        | 42 +++++++++++++++++-----
 4 files changed, 58 insertions(+), 10 deletions(-)

Comments

Naushir Patuck Oct. 4, 2021, 8:31 a.m. UTC | #1 
Hi Paul,

Thank you for your work.

On Fri, 1 Oct 2021 at 11:33, Paul Elder <paul.elder@ideasonboard.com> wrote:

> Add support for the new AE controls in the raspberrypi pipeline handler,
> and in the IPA.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v2:
> - fix the rebase error where some uvc stuff was in rasberrypi
> - i haven't yet taken in the comments to move the new Pause/Resume
>   functions
>
> Initial versoin:
> This is very hacky. I wasn't sure what the best way was to plumb it into
> the raspberrypi IPA as it was a bit hairy...
> ---
>  include/libcamera/ipa/raspberrypi.h        |  3 +-
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 +++++++++-
>  src/ipa/raspberrypi/controller/rpi/agc.hpp |  5 +++
>  src/ipa/raspberrypi/raspberrypi.cpp        | 42 +++++++++++++++++-----
>  4 files changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> index 521eaecd..363ea038 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -28,8 +28,9 @@ namespace RPi {
>   * unsupported control is encountered.
>   */
>  static const ControlInfoMap Controls({
> -               { &controls::AeEnable, ControlInfo(false, true) },
> +               { &controls::ExposureTimeMode,
> ControlInfo(controls::ExposureTimeModeValues) },
>                 { &controls::ExposureTime, ControlInfo(0, 999999) },
> +               { &controls::AnalogueGainMode,
> ControlInfo(controls::AnalogueGainModeValues) },
>                 { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
>                 { &controls::AeMeteringMode,
> ControlInfo(controls::AeMeteringModeValues) },
>                 { &controls::AeConstraintMode,
> ControlInfo(controls::AeConstraintModeValues) },
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 289c1fce..b45ea454 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -203,14 +203,30 @@ bool Agc::IsPaused() const
>  }
>
>  void Agc::Pause()
> +{
> +}
> +
> +void Agc::Resume()
> +{
> +}
> +
> +void Agc::PauseExposure()
>

One small suggestion, could this be called PauseShutter() instead?  Helps
clarify what part
of the exposure is paused.

Thanks,
Naush


>  {
>         fixed_shutter_ = status_.shutter_time;
> +}
> +
> +void Agc::PauseGain()
> +{
>         fixed_analogue_gain_ = status_.analogue_gain;
>  }
>
> -void Agc::Resume()
> +void Agc::ResumeExposure()
>  {
>         fixed_shutter_ = 0s;
> +}
> +
> +void Agc::ResumeGain()
> +{
>         fixed_analogue_gain_ = 0;
>  }
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 82063636..7ca3ca2f 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -92,6 +92,11 @@ public:
>         void Prepare(Metadata *image_metadata) override;
>         void Process(StatisticsPtr &stats, Metadata *image_metadata)
> override;
>
> +       void PauseExposure();
> +       void PauseGain();
> +       void ResumeExposure();
> +       void ResumeGain();
> +
>  private:
>         void updateLockStatus(DeviceStatus const &device_status);
>         AgcConfig config_;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 047123ab..99935515 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -53,6 +53,8 @@
>  #include "sharpen_algorithm.hpp"
>  #include "sharpen_status.h"
>
> +#include "controller/rpi/agc.hpp"
> +
>  namespace libcamera {
>
>  using namespace std::literals::chrono_literals;
> @@ -478,7 +480,10 @@ void IPARPi::reportMetadata()
>
>         AgcStatus *agcStatus =
> rpiMetadata_.GetLocked<AgcStatus>("agc.status");
>         if (agcStatus) {
> -               libcameraMetadata_.set(controls::AeLocked,
> agcStatus->locked);
> +               libcameraMetadata_.set(controls::AeState,
> +                                      agcStatus->locked ?
> +                                      controls::AeStateConverged :
> +                                      controls::AeStateSearching);
>                 libcameraMetadata_.set(controls::DigitalGain,
> agcStatus->digital_gain);
>         }
>
> @@ -623,20 +628,22 @@ void IPARPi::queueRequest(const ControlList
> &controls)
>                                   << " = " << ctrl.second.toString();
>
>                 switch (ctrl.first) {
> -               case controls::AE_ENABLE: {
> -                       RPiController::Algorithm *agc =
> controller_.GetAlgorithm("agc");
> +               case controls::EXPOSURE_TIME_MODE: {
> +                       RPiController::Algorithm *algo =
> controller_.GetAlgorithm("agc");
> +                       RPiController::Agc *agc =
> reinterpret_cast<RPiController::Agc *>(algo);
>                         if (!agc) {
>                                 LOG(IPARPI, Warning)
> -                                       << "Could not set AE_ENABLE - no
> AGC algorithm";
> +                                       << "Could not set
> EXPOSURE_TIME_MODE - no AGC algorithm";
>                                 break;
>                         }
>
> -                       if (ctrl.second.get<bool>() == false)
> -                               agc->Pause();
> +                       if (ctrl.second.get<int32_t>() ==
> controls::ExposureTimeModeDisabled)
> +                               agc->PauseExposure();
>                         else
> -                               agc->Resume();
> +                               agc->ResumeExposure();
>
> -                       libcameraMetadata_.set(controls::AeEnable,
> ctrl.second.get<bool>());
> +                       libcameraMetadata_.set(controls::ExposureTimeMode,
> +                                              ctrl.second.get<int32_t>());
>                         break;
>                 }
>
> @@ -656,6 +663,25 @@ void IPARPi::queueRequest(const ControlList &controls)
>                         break;
>                 }
>
> +               case controls::ANALOGUE_GAIN_MODE: {
> +                       RPiController::Algorithm *algo =
> controller_.GetAlgorithm("agc");
> +                       RPiController::Agc *agc =
> reinterpret_cast<RPiController::Agc *>(algo);
> +                       if (!agc) {
> +                               LOG(IPARPI, Warning)
> +                                       << "Could not set
> ANALOGUE_GAIN_MODE - no AGC algorithm";
> +                               break;
> +                       }
> +
> +                       if (ctrl.second.get<int32_t>() ==
> controls::AnalogueGainModeDisabled)
> +                               agc->PauseGain();
> +                       else
> +                               agc->ResumeGain();
> +
> +                       libcameraMetadata_.set(controls::AnalogueGainMode,
> +                                              ctrl.second.get<int32_t>());
> +                       break;
> +               }
> +
>                 case controls::ANALOGUE_GAIN: {
>                         RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
>                                 controller_.GetAlgorithm("agc"));
> --
> 2.27.0
>
>
Paul Elder Oct. 4, 2021, 9:08 a.m. UTC | #2 
Hi Naush,

Thanks for the feedback.

On Mon, Oct 04, 2021 at 09:31:06AM +0100, Naushir Patuck wrote:
> Hi Paul,
> 
> Thank you for your work.
> 
> On Fri, 1 Oct 2021 at 11:33, Paul Elder <paul.elder@ideasonboard.com> wrote:
> 
>     Add support for the new AE controls in the raspberrypi pipeline handler,
>     and in the IPA.
> 
>     Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
>     Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
>     Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
>     ---
>     Changes in v2:
>     - fix the rebase error where some uvc stuff was in rasberrypi
>     - i haven't yet taken in the comments to move the new Pause/Resume
>       functions
> 
>     Initial versoin:
>     This is very hacky. I wasn't sure what the best way was to plumb it into
>     the raspberrypi IPA as it was a bit hairy...
>     ---
>      include/libcamera/ipa/raspberrypi.h        |  3 +-
>      src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 +++++++++-
>      src/ipa/raspberrypi/controller/rpi/agc.hpp |  5 +++
>      src/ipa/raspberrypi/raspberrypi.cpp        | 42 +++++++++++++++++-----
>      4 files changed, 58 insertions(+), 10 deletions(-)
> 
>     diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/
>     raspberrypi.h
>     index 521eaecd..363ea038 100644
>     --- a/include/libcamera/ipa/raspberrypi.h
>     +++ b/include/libcamera/ipa/raspberrypi.h
>     @@ -28,8 +28,9 @@ namespace RPi {
>       * unsupported control is encountered.
>       */
>      static const ControlInfoMap Controls({
>     -               { &controls::AeEnable, ControlInfo(false, true) },
>     +               { &controls::ExposureTimeMode, ControlInfo
>     (controls::ExposureTimeModeValues) },
>                     { &controls::ExposureTime, ControlInfo(0, 999999) },
>     +               { &controls::AnalogueGainMode, ControlInfo
>     (controls::AnalogueGainModeValues) },
>                     { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
>                     { &controls::AeMeteringMode, ControlInfo
>     (controls::AeMeteringModeValues) },
>                     { &controls::AeConstraintMode, ControlInfo
>     (controls::AeConstraintModeValues) },
>     diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/
>     raspberrypi/controller/rpi/agc.cpp
>     index 289c1fce..b45ea454 100644
>     --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
>     +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
>     @@ -203,14 +203,30 @@ bool Agc::IsPaused() const
>      }
> 
>      void Agc::Pause()
>     +{
>     +}
>     +
>     +void Agc::Resume()
>     +{
>     +}
>     +
>     +void Agc::PauseExposure()
> 
> 
> One small suggestion, could this be called PauseShutter() instead?  Helps

We can call it whatever you want :D

I stil need to move these to AgcAlgorithm. Is really just that and
renaming these sufficient?


Thanks,

Paul

> clarify what part
> of the exposure is paused.
> 
> Thanks,
> Naush
>  
> 
>      {
>             fixed_shutter_ = status_.shutter_time;
>     +}
>     +
>     +void Agc::PauseGain()
>     +{
>             fixed_analogue_gain_ = status_.analogue_gain;
>      }
> 
>     -void Agc::Resume()
>     +void Agc::ResumeExposure()
>      {
>             fixed_shutter_ = 0s;
>     +}
>     +
>     +void Agc::ResumeGain()
>     +{
>             fixed_analogue_gain_ = 0;
>      }
> 
>     diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/
>     raspberrypi/controller/rpi/agc.hpp
>     index 82063636..7ca3ca2f 100644
>     --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
>     +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
>     @@ -92,6 +92,11 @@ public:
>             void Prepare(Metadata *image_metadata) override;
>             void Process(StatisticsPtr &stats, Metadata *image_metadata)
>     override;
> 
>     +       void PauseExposure();
>     +       void PauseGain();
>     +       void ResumeExposure();
>     +       void ResumeGain();
>     +
>      private:
>             void updateLockStatus(DeviceStatus const &device_status);
>             AgcConfig config_;
>     diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
>     raspberrypi.cpp
>     index 047123ab..99935515 100644
>     --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     @@ -53,6 +53,8 @@
>      #include "sharpen_algorithm.hpp"
>      #include "sharpen_status.h"
> 
>     +#include "controller/rpi/agc.hpp"
>     +
>      namespace libcamera {
> 
>      using namespace std::literals::chrono_literals;
>     @@ -478,7 +480,10 @@ void IPARPi::reportMetadata()
> 
>             AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>
>     ("agc.status");
>             if (agcStatus) {
>     -               libcameraMetadata_.set(controls::AeLocked, agcStatus->
>     locked);
>     +               libcameraMetadata_.set(controls::AeState,
>     +                                      agcStatus->locked ?
>     +                                      controls::AeStateConverged :
>     +                                      controls::AeStateSearching);
>                     libcameraMetadata_.set(controls::DigitalGain, agcStatus->
>     digital_gain);
>             }
> 
>     @@ -623,20 +628,22 @@ void IPARPi::queueRequest(const ControlList &
>     controls)
>                                       << " = " << ctrl.second.toString();
> 
>                     switch (ctrl.first) {
>     -               case controls::AE_ENABLE: {
>     -                       RPiController::Algorithm *agc =
>     controller_.GetAlgorithm("agc");
>     +               case controls::EXPOSURE_TIME_MODE: {
>     +                       RPiController::Algorithm *algo =
>     controller_.GetAlgorithm("agc");
>     +                       RPiController::Agc *agc = reinterpret_cast
>     <RPiController::Agc *>(algo);
>                             if (!agc) {
>                                     LOG(IPARPI, Warning)
>     -                                       << "Could not set AE_ENABLE - no
>     AGC algorithm";
>     +                                       << "Could not set
>     EXPOSURE_TIME_MODE - no AGC algorithm";
>                                     break;
>                             }
> 
>     -                       if (ctrl.second.get<bool>() == false)
>     -                               agc->Pause();
>     +                       if (ctrl.second.get<int32_t>() ==
>     controls::ExposureTimeModeDisabled)
>     +                               agc->PauseExposure();
>                             else
>     -                               agc->Resume();
>     +                               agc->ResumeExposure();
> 
>     -                       libcameraMetadata_.set(controls::AeEnable,
>     ctrl.second.get<bool>());
>     +                       libcameraMetadata_.set(controls::ExposureTimeMode,
>     +                                              ctrl.second.get<int32_t>());
>                             break;
>                     }
> 
>     @@ -656,6 +663,25 @@ void IPARPi::queueRequest(const ControlList &controls)
>                             break;
>                     }
> 
>     +               case controls::ANALOGUE_GAIN_MODE: {
>     +                       RPiController::Algorithm *algo =
>     controller_.GetAlgorithm("agc");
>     +                       RPiController::Agc *agc = reinterpret_cast
>     <RPiController::Agc *>(algo);
>     +                       if (!agc) {
>     +                               LOG(IPARPI, Warning)
>     +                                       << "Could not set
>     ANALOGUE_GAIN_MODE - no AGC algorithm";
>     +                               break;
>     +                       }
>     +
>     +                       if (ctrl.second.get<int32_t>() ==
>     controls::AnalogueGainModeDisabled)
>     +                               agc->PauseGain();
>     +                       else
>     +                               agc->ResumeGain();
>     +
>     +                       libcameraMetadata_.set(controls::AnalogueGainMode,
>     +                                              ctrl.second.get<int32_t>());
>     +                       break;
>     +               }
>     +
>                     case controls::ANALOGUE_GAIN: {
>                             RPiController::AgcAlgorithm *agc = dynamic_cast
>     <RPiController::AgcAlgorithm *>(
>                                     controller_.GetAlgorithm("agc"));
>     --
>     2.27.0
> 
>

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index 521eaecd..363ea038 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -28,8 +28,9 @@  namespace RPi {
  * unsupported control is encountered.
  */
 static const ControlInfoMap Controls({
-		{ &controls::AeEnable, ControlInfo(false, true) },
+		{ &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },
 		{ &controls::ExposureTime, ControlInfo(0, 999999) },
+		{ &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },
 		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
 		{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
 		{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
index 289c1fce..b45ea454 100644
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
@@ -203,14 +203,30 @@  bool Agc::IsPaused() const
 }
 
 void Agc::Pause()
+{
+}
+
+void Agc::Resume()
+{
+}
+
+void Agc::PauseExposure()
 {
 	fixed_shutter_ = status_.shutter_time;
+}
+
+void Agc::PauseGain()
+{
 	fixed_analogue_gain_ = status_.analogue_gain;
 }
 
-void Agc::Resume()
+void Agc::ResumeExposure()
 {
 	fixed_shutter_ = 0s;
+}
+
+void Agc::ResumeGain()
+{
 	fixed_analogue_gain_ = 0;
 }
 
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
index 82063636..7ca3ca2f 100644
--- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
+++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
@@ -92,6 +92,11 @@  public:
 	void Prepare(Metadata *image_metadata) override;
 	void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
 
+	void PauseExposure();
+	void PauseGain();
+	void ResumeExposure();
+	void ResumeGain();
+
 private:
 	void updateLockStatus(DeviceStatus const &device_status);
 	AgcConfig config_;
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 047123ab..99935515 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -53,6 +53,8 @@ 
 #include "sharpen_algorithm.hpp"
 #include "sharpen_status.h"
 
+#include "controller/rpi/agc.hpp"
+
 namespace libcamera {
 
 using namespace std::literals::chrono_literals;
@@ -478,7 +480,10 @@  void IPARPi::reportMetadata()
 
 	AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");
 	if (agcStatus) {
-		libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);
+		libcameraMetadata_.set(controls::AeState,
+				       agcStatus->locked ?
+				       controls::AeStateConverged :
+				       controls::AeStateSearching);
 		libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);
 	}
 
@@ -623,20 +628,22 @@  void IPARPi::queueRequest(const ControlList &controls)
 				  << " = " << ctrl.second.toString();
 
 		switch (ctrl.first) {
-		case controls::AE_ENABLE: {
-			RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
+		case controls::EXPOSURE_TIME_MODE: {
+			RPiController::Algorithm *algo = controller_.GetAlgorithm("agc");
+			RPiController::Agc *agc = reinterpret_cast<RPiController::Agc *>(algo);
 			if (!agc) {
 				LOG(IPARPI, Warning)
-					<< "Could not set AE_ENABLE - no AGC algorithm";
+					<< "Could not set EXPOSURE_TIME_MODE - no AGC algorithm";
 				break;
 			}
 
-			if (ctrl.second.get<bool>() == false)
-				agc->Pause();
+			if (ctrl.second.get<int32_t>() == controls::ExposureTimeModeDisabled)
+				agc->PauseExposure();
 			else
-				agc->Resume();
+				agc->ResumeExposure();
 
-			libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
+			libcameraMetadata_.set(controls::ExposureTimeMode,
+					       ctrl.second.get<int32_t>());
 			break;
 		}
 
@@ -656,6 +663,25 @@  void IPARPi::queueRequest(const ControlList &controls)
 			break;
 		}
 
+		case controls::ANALOGUE_GAIN_MODE: {
+			RPiController::Algorithm *algo = controller_.GetAlgorithm("agc");
+			RPiController::Agc *agc = reinterpret_cast<RPiController::Agc *>(algo);
+			if (!agc) {
+				LOG(IPARPI, Warning)
+					<< "Could not set ANALOGUE_GAIN_MODE - no AGC algorithm";
+				break;
+			}
+
+			if (ctrl.second.get<int32_t>() == controls::AnalogueGainModeDisabled)
+				agc->PauseGain();
+			else
+				agc->ResumeGain();
+
+			libcameraMetadata_.set(controls::AnalogueGainMode,
+					       ctrl.second.get<int32_t>());
+			break;
+		}
+
 		case controls::ANALOGUE_GAIN: {
 			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
 				controller_.GetAlgorithm("agc"));