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

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

Commit Message

Naushir Patuck June 2, 2023, 1:23 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 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@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

Comments

Laurent Pinchart June 2, 2023, 3:51 p.m. UTC | #1 
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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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_;
Naushir Patuck June 5, 2023, 7:20 a.m. UTC | #2 
Hi Laurent,

On Fri, 2 Jun 2023 at 16:51, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 ?

Yes this is true.  I've always felt that the ControlInfoMap should not be
advertised at all during ipa->init(), since some controls limits (e.g.
AnalogueGain
and ExposureTime) have invalid min/max/default values.  They only get correct
values set after ipa->configure().

Rather than passing IPASensorInfo to init(), I would probably prefer to entirely
remove setting up the ControlInfoMap there.  What are your thoughts?

Regards,
Naush

>
> >       /* 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
Laurent Pinchart June 5, 2023, 7:43 a.m. UTC | #3 
Hi Naush,

On Mon, Jun 05, 2023 at 08:20:28AM +0100, Naushir Patuck wrote:
> On Fri, 2 Jun 2023 at 16:51, Laurent Pinchart wrote:
> > 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@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 ?
> 
> Yes this is true.  I've always felt that the ControlInfoMap should not be
> advertised at all during ipa->init(), since some controls limits (e.g.
> AnalogueGain and ExposureTime) have invalid min/max/default values.
> They only get correct values set after ipa->configure().
>
> Rather than passing IPASensorInfo to init(), I would probably prefer to entirely
> remove setting up the ControlInfoMap there.  What are your thoughts?

This is an issue we'll need to tackle, and I don't know yet how to do
so. Not reporting properties and controls until after configure() is an
easy option, but it would make it more difficult for applications to
figure out if a camera is suitable for their needs, or just adapt their
behaviour based on the camera features. I don't want to drop reporting
supported controls at init time completely until we have a proper,
well-thought replacement.

> > >       /* 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_;
Naushir Patuck June 5, 2023, 7:52 a.m. UTC | #4 
Hi Laurent,

On Mon, 5 Jun 2023 at 08:43, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Mon, Jun 05, 2023 at 08:20:28AM +0100, Naushir Patuck wrote:
> > On Fri, 2 Jun 2023 at 16:51, Laurent Pinchart wrote:
> > > 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@raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 ?
> >
> > Yes this is true.  I've always felt that the ControlInfoMap should not be
> > advertised at all during ipa->init(), since some controls limits (e.g.
> > AnalogueGain and ExposureTime) have invalid min/max/default values.
> > They only get correct values set after ipa->configure().
> >
> > Rather than passing IPASensorInfo to init(), I would probably prefer to entirely
> > remove setting up the ControlInfoMap there.  What are your thoughts?
>
> This is an issue we'll need to tackle, and I don't know yet how to do
> so. Not reporting properties and controls until after configure() is an
> easy option, but it would make it more difficult for applications to
> figure out if a camera is suitable for their needs, or just adapt their
> behaviour based on the camera features. I don't want to drop reporting
> supported controls at init time completely until we have a proper,
> well-thought replacement.

Ok, I'll post an update with the IPASensorInfo passed into init() for now until
we have a better solution.

Regards,
Naush

>
> > > >       /* 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

Patch
diff mbox series 

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));
+
 	/* 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_;