[libcamera-devel,v1,2/2] ipa: rpi: Handle controls for mono variant sensors
diff mbox series

Message ID 20230531143946.23571-3-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Mono sensor improvements
Related show

Commit Message

Naushir Patuck May 31, 2023, 2:39 p.m. UTC 
Do not advertise colour related controls (i.e. [A]WB, colour saturation)
in the ControlInfoMap of available controls returned out to the
application.

Silently fail these controls in the control handler in case applications
don't use the advertised ControlInfoMap to validate controls.

As a drive-by, don't advertise controls::ColourCorrectionMatrix in the
ControlInfoMap as it is not handled by the IPA.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 34 ++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

Comments

David Plowman May 31, 2023, 3:09 p.m. UTC | #1 
Hi Naush

Thanks for the patch!

On Wed, 31 May 2023 at 15:39, Naushir Patuck via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Do not advertise colour related controls (i.e. [A]WB, colour saturation)
> in the ControlInfoMap of available controls returned out to the
> application.
>
> Silently fail these controls in the control handler in case applications
> don't use the advertised ControlInfoMap to validate controls.
>
> As a drive-by, don't advertise controls::ColourCorrectionMatrix in the
> ControlInfoMap as it is not handled by the IPA.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

I think silently ignoring the colour controls in this situation seems
reasonable.

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks
David

> ---
>  src/ipa/rpi/common/ipa_base.cpp | 34 ++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 150fe433d0df..8de454637cfe 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -60,19 +60,22 @@ const ControlInfoMap::Map ipaControls{
>         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>         { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>         { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> -       { &controls::AwbEnable, ControlInfo(false, true) },
> -       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> -       { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> -       { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> -       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>         { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
>  };
>
> +/* IPA controls handled conditionally, if the sensor is not mono */
> +const ControlInfoMap::Map ipaColourControls{
> +       { &controls::AwbEnable, ControlInfo(false, true) },
> +       { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> +       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> +       { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> +};
> +
>  /* IPA controls handled conditionally, if the lens has a focus control */
>  const ControlInfoMap::Map ipaAfControls{
>         { &controls::AfMode, ControlInfo(controls::AfModeValues) },
> @@ -145,6 +148,8 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
>
>         /* Return the controls handled by the IPA */
>         ControlInfoMap::Map ctrlMap = ipaControls;
> +       if (!monoSensor_)
> +               ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
>         if (lensPresent_)
>                 ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
>         result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
> @@ -221,6 +226,9 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa
>                 ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),
>                             static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));
>
> +       if (!monoSensor_)
> +               ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
> +
>         /* Declare Autofocus controls, only if we have a controllable lens */
>         if (lensPresent_)
>                 ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
> @@ -781,6 +789,10 @@ void IpaBase::applyControls(const ControlList &controls)
>                 }
>
>                 case controls::AWB_ENABLE: {
> +                       /* Silently ignore this control for a mono sensor. */
> +                       if (monoSensor_)
> +                               break;
> +
>                         RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>                                 controller_.getAlgorithm("awb"));
>                         if (!awb) {
> @@ -800,6 +812,10 @@ void IpaBase::applyControls(const ControlList &controls)
>                 }
>
>                 case controls::AWB_MODE: {
> +                       /* Silently ignore this control for a mono sensor. */
> +                       if (monoSensor_)
> +                               break;
> +
>                         RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>                                 controller_.getAlgorithm("awb"));
>                         if (!awb) {
> @@ -820,6 +836,10 @@ void IpaBase::applyControls(const ControlList &controls)
>                 }
>
>                 case controls::COLOUR_GAINS: {
> +                       /* Silently ignore this control for a mono sensor. */
> +                       if (monoSensor_)
> +                               break;
> +
>                         auto gains = ctrl.second.get<Span<const float>>();
>                         RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>                                 controller_.getAlgorithm("awb"));
> @@ -868,6 +888,10 @@ void IpaBase::applyControls(const ControlList &controls)
>                 }
>
>                 case controls::SATURATION: {
> +                       /* Silently ignore this control for a mono sensor. */
> +                       if (monoSensor_)
> +                               break;
> +
>                         RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
>                                 controller_.getAlgorithm("ccm"));
>                         if (!ccm) {
> --
> 2.34.1
>
Laurent Pinchart June 2, 2023, 7:50 a.m. UTC | #2 
Hi Naush,

Thank you for the patch.

On Wed, May 31, 2023 at 03:39:46PM +0100, Naushir Patuck via libcamera-devel wrote:
> Do not advertise colour related controls (i.e. [A]WB, colour saturation)
> in the ControlInfoMap of available controls returned out to the
> application.
> 
> Silently fail these controls in the control handler in case applications

Did you mean "ignore" instead of "fail" ?

> don't use the advertised ControlInfoMap to validate controls.
> 
> As a drive-by, don't advertise controls::ColourCorrectionMatrix in the
> ControlInfoMap as it is not handled by the IPA.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I will wait until we finish the discussion regarding 1/2 before applying
the series.

> ---
>  src/ipa/rpi/common/ipa_base.cpp | 34 ++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 150fe433d0df..8de454637cfe 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -60,19 +60,22 @@ const ControlInfoMap::Map ipaControls{
>  	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>  	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>  	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> -	{ &controls::AwbEnable, ControlInfo(false, true) },
> -	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> -	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
>  	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>  	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> -	{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> -	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>  	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>  	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
>  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
>  };
>  
> +/* IPA controls handled conditionally, if the sensor is not mono */
> +const ControlInfoMap::Map ipaColourControls{
> +	{ &controls::AwbEnable, ControlInfo(false, true) },
> +	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> +	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> +	{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> +};
> +
>  /* IPA controls handled conditionally, if the lens has a focus control */
>  const ControlInfoMap::Map ipaAfControls{
>  	{ &controls::AfMode, ControlInfo(controls::AfModeValues) },
> @@ -145,6 +148,8 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
>  
>  	/* Return the controls handled by the IPA */
>  	ControlInfoMap::Map ctrlMap = ipaControls;
> +	if (!monoSensor_)
> +		ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
>  	if (lensPresent_)
>  		ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
>  	result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
> @@ -221,6 +226,9 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa
>  		ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),
>  			    static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));
>  
> +	if (!monoSensor_)
> +		ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
> +
>  	/* Declare Autofocus controls, only if we have a controllable lens */
>  	if (lensPresent_)
>  		ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
> @@ -781,6 +789,10 @@ void IpaBase::applyControls(const ControlList &controls)
>  		}
>  
>  		case controls::AWB_ENABLE: {
> +			/* Silently ignore this control for a mono sensor. */
> +			if (monoSensor_)
> +				break;
> +
>  			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>  				controller_.getAlgorithm("awb"));
>  			if (!awb) {
> @@ -800,6 +812,10 @@ void IpaBase::applyControls(const ControlList &controls)
>  		}
>  
>  		case controls::AWB_MODE: {
> +			/* Silently ignore this control for a mono sensor. */
> +			if (monoSensor_)
> +				break;
> +
>  			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>  				controller_.getAlgorithm("awb"));
>  			if (!awb) {
> @@ -820,6 +836,10 @@ void IpaBase::applyControls(const ControlList &controls)
>  		}
>  
>  		case controls::COLOUR_GAINS: {
> +			/* Silently ignore this control for a mono sensor. */
> +			if (monoSensor_)
> +				break;
> +
>  			auto gains = ctrl.second.get<Span<const float>>();
>  			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>  				controller_.getAlgorithm("awb"));
> @@ -868,6 +888,10 @@ void IpaBase::applyControls(const ControlList &controls)
>  		}
>  
>  		case controls::SATURATION: {
> +			/* Silently ignore this control for a mono sensor. */
> +			if (monoSensor_)
> +				break;
> +
>  			RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
>  				controller_.getAlgorithm("ccm"));
>  			if (!ccm) {

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index 150fe433d0df..8de454637cfe 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -60,19 +60,22 @@  const ControlInfoMap::Map ipaControls{
 	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
 	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
 	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
-	{ &controls::AwbEnable, ControlInfo(false, true) },
-	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
-	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
 	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
 	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
-	{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
 	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
-	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
 	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
 	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
 	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
 };
 
+/* IPA controls handled conditionally, if the sensor is not mono */
+const ControlInfoMap::Map ipaColourControls{
+	{ &controls::AwbEnable, ControlInfo(false, true) },
+	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
+	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
+	{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
+};
+
 /* IPA controls handled conditionally, if the lens has a focus control */
 const ControlInfoMap::Map ipaAfControls{
 	{ &controls::AfMode, ControlInfo(controls::AfModeValues) },
@@ -145,6 +148,8 @@  int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
 
 	/* Return the controls handled by the IPA */
 	ControlInfoMap::Map ctrlMap = ipaControls;
+	if (!monoSensor_)
+		ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
 	if (lensPresent_)
 		ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
 	result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
@@ -221,6 +226,9 @@  int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa
 		ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),
 			    static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));
 
+	if (!monoSensor_)
+		ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
+
 	/* Declare Autofocus controls, only if we have a controllable lens */
 	if (lensPresent_)
 		ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
@@ -781,6 +789,10 @@  void IpaBase::applyControls(const ControlList &controls)
 		}
 
 		case controls::AWB_ENABLE: {
+			/* Silently ignore this control for a mono sensor. */
+			if (monoSensor_)
+				break;
+
 			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
 				controller_.getAlgorithm("awb"));
 			if (!awb) {
@@ -800,6 +812,10 @@  void IpaBase::applyControls(const ControlList &controls)
 		}
 
 		case controls::AWB_MODE: {
+			/* Silently ignore this control for a mono sensor. */
+			if (monoSensor_)
+				break;
+
 			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
 				controller_.getAlgorithm("awb"));
 			if (!awb) {
@@ -820,6 +836,10 @@  void IpaBase::applyControls(const ControlList &controls)
 		}
 
 		case controls::COLOUR_GAINS: {
+			/* Silently ignore this control for a mono sensor. */
+			if (monoSensor_)
+				break;
+
 			auto gains = ctrl.second.get<Span<const float>>();
 			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
 				controller_.getAlgorithm("awb"));
@@ -868,6 +888,10 @@  void IpaBase::applyControls(const ControlList &controls)
 		}
 
 		case controls::SATURATION: {
+			/* Silently ignore this control for a mono sensor. */
+			if (monoSensor_)
+				break;
+
 			RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
 				controller_.getAlgorithm("ccm"));
 			if (!ccm) {