[libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for mono variant sensors

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Jun 02, 2023 at 02:23:58PM +0100, Naushir Patuck 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 ignore 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 at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp | 35 ++++++++++++++++++++++++++++-----
>  src/ipa/rpi/common/ipa_base.h   |  1 +
>  2 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index db7a0eb3a1ca..813e01fa5cfd 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -12,6 +12,7 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/span.h>
>  #include <libcamera/control_ids.h>
> +#include <libcamera/property_ids.h>
>  
>  #include "controller/af_algorithm.h"
>  #include "controller/af_status.h"
> @@ -60,19 +61,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) },
> @@ -220,6 +224,11 @@ 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>()));
>  
> +	/* Declare colour processing related controls for non-mono sensors. */
> +	monoSensor_ = sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;
> +	if (!monoSensor_)
> +		ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
> +

This means that the camera won't report the colour-related controls
until it gets configured. Could we fix that by passing the IPASensorInfo
to the init() function as well ?

>  	/* Declare Autofocus controls, only if we have a controllable lens */
>  	if (lensPresent_)
>  		ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
> @@ -780,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) {
> @@ -799,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) {
> @@ -819,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"));
> @@ -867,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) {
> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> index 6f9c46bb16b1..39d00760d012 100644
> --- a/src/ipa/rpi/common/ipa_base.h
> +++ b/src/ipa/rpi/common/ipa_base.h
> @@ -87,6 +87,7 @@ private:
>  	std::map<unsigned int, MappedFrameBuffer> buffers_;
>  
>  	bool lensPresent_;
> +	bool monoSensor_;
>  	ControlList libcameraMetadata_;
>  
>  	std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;

-- 
Regards,

Laurent Pinchart