[libcamera-devel,v5,2/2] ipa: rpi: common: Handle AEC/AGC flicker controls
diff mbox series

Message ID 20230718144301.3612-3-david.plowman@raspberrypi.com
State Accepted
Commit a2eadc40a78f73410d2dcf76a24313a83a1c51c8
Headers show
Series
  • Add flicker avoidance controls
Related show

Commit Message

David Plowman July 18, 2023, 2:43 p.m. UTC 
We handle the flicker modes by passing the correct period to the
AEC/AGC algorithm which already contains the necessary code.

The "Auto" mode, as well as reporting the detected flicker period via
the "AeFlickerDetected" metadata, are unsupported for now.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++-
 src/ipa/rpi/common/ipa_base.h   |  6 ++++
 2 files changed, 69 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi July 19, 2023, 12:36 p.m. UTC | #1 
Hi David

On Tue, Jul 18, 2023 at 03:43:01PM +0100, David Plowman via libcamera-devel wrote:
> We handle the flicker modes by passing the correct period to the
> AEC/AGC algorithm which already contains the necessary code.
>
> The "Auto" mode, as well as reporting the detected flicker period via
> the "AeFlickerDetected" metadata, are unsupported for now.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++-
>  src/ipa/rpi/common/ipa_base.h   |  6 ++++
>  2 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index f40f2e71..4438ecd9 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -61,6 +61,10 @@ const ControlInfoMap::Map ipaControls{
>  	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>  	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>  	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> +	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
> +						static_cast<int>(controls::FlickerManual),
> +						static_cast<int>(controls::FlickerOff)) },
> +	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
>  	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>  	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> @@ -97,7 +101,7 @@ namespace ipa::RPi {
>
>  IpaBase::IpaBase()
>  	: controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),
> -	  firstStart_(true)
> +	  firstStart_(true), flickerState_({ 0, 0s })
>  {
>  }
>
> @@ -812,6 +816,64 @@ void IpaBase::applyControls(const ControlList &controls)
>  			break;
>  		}
>
> +		case controls::AE_FLICKER_MODE: {
> +			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +				controller_.getAlgorithm("agc"));
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AeFlickerMode - no AGC algorithm";
> +				break;
> +			}
> +
> +			int32_t mode = ctrl.second.get<int32_t>();
> +			bool modeValid = true;
> +
> +			switch (mode) {
> +			case controls::FlickerOff:
> +				agc->setFlickerPeriod(0us);
> +
> +				break;
> +
> +			case controls::FlickerManual:
> +				agc->setFlickerPeriod(flickerState_.manualPeriod);
> +
> +				break;
> +
> +			default:
> +				LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported";
> +				modeValid = false;
> +
> +				break;
> +			}
> +
> +			if (modeValid)
> +				flickerState_.mode = mode;
> +
> +			break;
> +		}
> +
> +		case controls::AE_FLICKER_PERIOD: {
> +			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +				controller_.getAlgorithm("agc"));
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AeFlickerPeriod - no AGC algorithm";
> +				break;
> +			}
> +
> +			uint32_t manualPeriod = ctrl.second.get<int32_t>();
> +			flickerState_.manualPeriod = manualPeriod * 1.0us;
> +
> +			/*
> +			 * We note that it makes no difference if the mode gets set to "manual"
> +			 * first, and the period updated after, or vice versa.

I guss this is fine, as you said, we're probably overthinking this,
flicker correction is meant to be updated less frequently than
exposure/gain

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> +			 */
> +			if (flickerState_.mode == controls::FlickerManual)
> +				agc->setFlickerPeriod(flickerState_.manualPeriod);
> +
> +			break;
> +		}
> +
>  		case controls::AWB_ENABLE: {
>  			/* Silently ignore this control for a mono sensor. */
>  			if (monoSensor_)
> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> index 39d00760..097f436a 100644
> --- a/src/ipa/rpi/common/ipa_base.h
> +++ b/src/ipa/rpi/common/ipa_base.h
> @@ -116,6 +116,12 @@ private:
>  	/* Frame duration (1/fps) limits. */
>  	utils::Duration minFrameDuration_;
>  	utils::Duration maxFrameDuration_;
> +
> +	/* The current state of flicker avoidance. */
> +	struct FlickerState {
> +		int32_t mode;
> +		utils::Duration manualPeriod;
> +	} flickerState_;
>  };
>
>  } /* namespace ipa::RPi */
> --
> 2.30.2
>
Umang Jain July 21, 2023, 4:23 a.m. UTC | #2 
Hi David

On 7/18/23 8:13 PM, David Plowman via libcamera-devel wrote:
> We handle the flicker modes by passing the correct period to the
> AEC/AGC algorithm which already contains the necessary code.
>
> The "Auto" mode, as well as reporting the detected flicker period via
> the "AeFlickerDetected" metadata, are unsupported for now.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++-
>   src/ipa/rpi/common/ipa_base.h   |  6 ++++
>   2 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index f40f2e71..4438ecd9 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -61,6 +61,10 @@ const ControlInfoMap::Map ipaControls{
>   	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>   	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>   	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> +	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
> +						static_cast<int>(controls::FlickerManual),
> +						static_cast<int>(controls::FlickerOff)) },
> +	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
>   	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>   	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
>   	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> @@ -97,7 +101,7 @@ namespace ipa::RPi {
>   
>   IpaBase::IpaBase()
>   	: controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),
> -	  firstStart_(true)
> +	  firstStart_(true), flickerState_({ 0, 0s })
>   {
>   }
>   
> @@ -812,6 +816,64 @@ void IpaBase::applyControls(const ControlList &controls)
>   			break;
>   		}
>   
> +		case controls::AE_FLICKER_MODE: {
> +			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +				controller_.getAlgorithm("agc"));
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AeFlickerMode - no AGC algorithm";
> +				break;
> +			}
> +
> +			int32_t mode = ctrl.second.get<int32_t>();
> +			bool modeValid = true;
> +
> +			switch (mode) {
> +			case controls::FlickerOff:
> +				agc->setFlickerPeriod(0us);
> +
> +				break;
> +
> +			case controls::FlickerManual:
> +				agc->setFlickerPeriod(flickerState_.manualPeriod);
> +
> +				break;
> +
> +			default:
> +				LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported";
> +				modeValid = false;
> +
> +				break;
> +			}
> +
> +			if (modeValid)
> +				flickerState_.mode = mode;
> +
> +			break;
> +		}
> +
> +		case controls::AE_FLICKER_PERIOD: {
> +			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +				controller_.getAlgorithm("agc"));
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AeFlickerPeriod - no AGC algorithm";
> +				break;
> +			}
> +
> +			uint32_t manualPeriod = ctrl.second.get<int32_t>();
> +			flickerState_.manualPeriod = manualPeriod * 1.0us;
> +
> +			/*
> +			 * We note that it makes no difference if the mode gets set to "manual"
> +			 * first, and the period updated after, or vice versa.
> +			 */
> +			if (flickerState_.mode == controls::FlickerManual)
> +				agc->setFlickerPeriod(flickerState_.manualPeriod);
> +
> +			break;
> +		}
> +
>   		case controls::AWB_ENABLE: {
>   			/* Silently ignore this control for a mono sensor. */
>   			if (monoSensor_)
> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> index 39d00760..097f436a 100644
> --- a/src/ipa/rpi/common/ipa_base.h
> +++ b/src/ipa/rpi/common/ipa_base.h
> @@ -116,6 +116,12 @@ private:
>   	/* Frame duration (1/fps) limits. */
>   	utils::Duration minFrameDuration_;
>   	utils::Duration maxFrameDuration_;
> +
> +	/* The current state of flicker avoidance. */
> +	struct FlickerState {
> +		int32_t mode;
> +		utils::Duration manualPeriod;
> +	} flickerState_;
>   };
>   
>   } /* namespace ipa::RPi */

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index f40f2e71..4438ecd9 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -61,6 +61,10 @@  const ControlInfoMap::Map ipaControls{
 	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
 	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
 	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
+	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
+						static_cast<int>(controls::FlickerManual),
+						static_cast<int>(controls::FlickerOff)) },
+	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
 	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
 	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
 	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
@@ -97,7 +101,7 @@  namespace ipa::RPi {
 
 IpaBase::IpaBase()
 	: controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),
-	  firstStart_(true)
+	  firstStart_(true), flickerState_({ 0, 0s })
 {
 }
 
@@ -812,6 +816,64 @@  void IpaBase::applyControls(const ControlList &controls)
 			break;
 		}
 
+		case controls::AE_FLICKER_MODE: {
+			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
+				controller_.getAlgorithm("agc"));
+			if (!agc) {
+				LOG(IPARPI, Warning)
+					<< "Could not set AeFlickerMode - no AGC algorithm";
+				break;
+			}
+
+			int32_t mode = ctrl.second.get<int32_t>();
+			bool modeValid = true;
+
+			switch (mode) {
+			case controls::FlickerOff:
+				agc->setFlickerPeriod(0us);
+
+				break;
+
+			case controls::FlickerManual:
+				agc->setFlickerPeriod(flickerState_.manualPeriod);
+
+				break;
+
+			default:
+				LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported";
+				modeValid = false;
+
+				break;
+			}
+
+			if (modeValid)
+				flickerState_.mode = mode;
+
+			break;
+		}
+
+		case controls::AE_FLICKER_PERIOD: {
+			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
+				controller_.getAlgorithm("agc"));
+			if (!agc) {
+				LOG(IPARPI, Warning)
+					<< "Could not set AeFlickerPeriod - no AGC algorithm";
+				break;
+			}
+
+			uint32_t manualPeriod = ctrl.second.get<int32_t>();
+			flickerState_.manualPeriod = manualPeriod * 1.0us;
+
+			/*
+			 * We note that it makes no difference if the mode gets set to "manual"
+			 * first, and the period updated after, or vice versa.
+			 */
+			if (flickerState_.mode == controls::FlickerManual)
+				agc->setFlickerPeriod(flickerState_.manualPeriod);
+
+			break;
+		}
+
 		case controls::AWB_ENABLE: {
 			/* Silently ignore this control for a mono sensor. */
 			if (monoSensor_)
diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
index 39d00760..097f436a 100644
--- a/src/ipa/rpi/common/ipa_base.h
+++ b/src/ipa/rpi/common/ipa_base.h
@@ -116,6 +116,12 @@  private:
 	/* Frame duration (1/fps) limits. */
 	utils::Duration minFrameDuration_;
 	utils::Duration maxFrameDuration_;
+
+	/* The current state of flicker avoidance. */
+	struct FlickerState {
+		int32_t mode;
+		utils::Duration manualPeriod;
+	} flickerState_;
 };
 
 } /* namespace ipa::RPi */