[libcamera-devel] ipa: raspberrypi: Warn when control algorithms are missing; do not fail

11008 diff mbox series

Users are free to avoid loading certain control algorithms from the
json tuning file if they wish. Under such circumstances, failing
completely when an application tries to set parameters for that
algorithm is unhelpful, and it is better just to issue a warning.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
 1 file changed, 72 insertions(+), 14 deletions(-)

Hi David,

On 26/01/2021 15:47, David Plowman wrote:
> Users are free to avoid loading certain control algorithms from the
> json tuning file if they wish. Under such circumstances, failing
> completely when an application tries to set parameters for that
> algorithm is unhelpful, and it is better just to issue a warning.

Aha this looks familiar from some review comments recently.

Certainly if the user can disable these, making sure we don't assert is
a good thing.

I wondered reading this pathc if it would help if the ControlList would
be passed into the algorithms with a dedicated operation so each
algorithm could parse the controls relevant to it, but then we'd lose
the ability to report a warning if controls were not processed.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
>  1 file changed, 72 insertions(+), 14 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 5ccc7a65..b57d77e9 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		switch (ctrl.first) {
>  		case controls::AE_ENABLE: {
>  			RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AE_ENABLE - no AGC algorithm";
> +				break;
> +			}
> +
>  			if (ctrl.second.get<bool>() == false)
>  				agc->Pause();
>  			else
> @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::EXPOSURE_TIME: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set EXPOSURE_TIME - no AGC algorithm";
> +				break;
> +			}
>  
>  			/* This expects units of micro-seconds. */
>  			agc->SetFixedShutter(ctrl.second.get<int32_t>());
> @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::ANALOGUE_GAIN: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set ANALOGUE_GAIN - no AGC algorithm";
> +				break;
> +			}
> +
>  			agc->SetFixedAnalogueGain(ctrl.second.get<float>());
>  
>  			libcameraMetadata_.set(controls::AnalogueGain,
> @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AE_METERING_MODE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AE_METERING_MODE - no AGC algorithm";
> +				break;
> +			}
>  
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (MeteringModeTable.count(idx)) {
> @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AE_CONSTRAINT_MODE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> +				break;
> +			}
>  
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (ConstraintModeTable.count(idx)) {
> @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AE_EXPOSURE_MODE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> +				break;
> +			}
>  
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (ExposureModeTable.count(idx)) {
> @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::EXPOSURE_VALUE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set EXPOSURE_VALUE - no AGC algorithm";
> +				break;
> +			}
>  
>  			/*
>  			 * The SetEv() method takes in a direct exposure multiplier.
> @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  
>  		case controls::AWB_ENABLE: {
>  			RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> -			ASSERT(awb);
> +			if (!awb) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AWB_ENABLE - no AWB algorithm";
> +				break;
> +			}
>  
>  			if (ctrl.second.get<bool>() == false)
>  				awb->Pause();
> @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AWB_MODE: {
>  			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>  				controller_.GetAlgorithm("awb"));
> -			ASSERT(awb);
> +			if (!awb) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AWB_MODE - no AWB algorithm";
> +				break;
> +			}
>  
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (AwbModeTable.count(idx)) {
> @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			auto gains = ctrl.second.get<Span<const float>>();
>  			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>  				controller_.GetAlgorithm("awb"));
> -			ASSERT(awb);
> +			if (!awb) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set COLOUR_GAINS - no AWB algorithm";
> +				break;
> +			}
>  
>  			awb->SetManualGains(gains[0], gains[1]);
>  			if (gains[0] != 0.0f && gains[1] != 0.0f)
> @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::BRIGHTNESS: {
>  			RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
>  				controller_.GetAlgorithm("contrast"));
> -			ASSERT(contrast);
> +			if (!contrast) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set BRIGHTNESS - no contrast algorithm";
> +				break;
> +			}
>  
>  			contrast->SetBrightness(ctrl.second.get<float>() * 65536);
>  			libcameraMetadata_.set(controls::Brightness,
> @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::CONTRAST: {
>  			RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
>  				controller_.GetAlgorithm("contrast"));
> -			ASSERT(contrast);
> +			if (!contrast) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set CONTRAST - no contrast algorithm";
> +				break;
> +			}
>  
>  			contrast->SetContrast(ctrl.second.get<float>());
>  			libcameraMetadata_.set(controls::Contrast,
> @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::SATURATION: {
>  			RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
>  				controller_.GetAlgorithm("ccm"));
> -			ASSERT(ccm);
> +			if (!ccm) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set SATURATION - no ccm algorithm";
> +				break;
> +			}
>  
>  			ccm->SetSaturation(ctrl.second.get<float>());
>  			libcameraMetadata_.set(controls::Saturation,
> @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::SHARPNESS: {
>  			RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
>  				controller_.GetAlgorithm("sharpen"));
> -			ASSERT(sharpen);
> +			if (!sharpen) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set SHARPNESS - no sharpen algorithm";
> +				break;
> +			}
>  
>  			sharpen->SetStrength(ctrl.second.get<float>());
>  			libcameraMetadata_.set(controls::Sharpness,
>

Hi David,

Thank you for the patch.

On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:
> Users are free to avoid loading certain control algorithms from the
> json tuning file if they wish. Under such circumstances, failing
> completely when an application tries to set parameters for that
> algorithm is unhelpful, and it is better just to issue a warning.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
>  1 file changed, 72 insertions(+), 14 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 5ccc7a65..b57d77e9 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		switch (ctrl.first) {
>  		case controls::AE_ENABLE: {
>  			RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AE_ENABLE - no AGC algorithm";
> +				break;
> +			}

This is a better behaviour indeed. I wonder if we need some kind of
LOG_ONCE() to avoid flooding the log, but it would be difficult to do so
per-camera.

Not aborting when a control is set without a corresponding algorithm is
good, but I think we need to go one step further and not report the
control to the application in the first place. This will require
replacing the static ControlInfoMap in
include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We
should then extend the libcamera core to verify that all controls in a
request are supported and either reject the request or strip the
unsupported controls, and we won't need this patch anymore.

I'm fine merging this patch as-is, given that the ongoing IPA IPC work
should make it easier to handle controls dynamically when it will land,
but it should then probably be reverted.

> +
>  			if (ctrl.second.get<bool>() == false)
>  				agc->Pause();
>  			else
> @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::EXPOSURE_TIME: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set EXPOSURE_TIME - no AGC algorithm";
> +				break;
> +			}
>  
>  			/* This expects units of micro-seconds. */
>  			agc->SetFixedShutter(ctrl.second.get<int32_t>());
> @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::ANALOGUE_GAIN: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set ANALOGUE_GAIN - no AGC algorithm";
> +				break;
> +			}
> +
>  			agc->SetFixedAnalogueGain(ctrl.second.get<float>());
>  
>  			libcameraMetadata_.set(controls::AnalogueGain,
> @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AE_METERING_MODE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AE_METERING_MODE - no AGC algorithm";
> +				break;
> +			}
>  
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (MeteringModeTable.count(idx)) {
> @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AE_CONSTRAINT_MODE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> +				break;
> +			}
>  
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (ConstraintModeTable.count(idx)) {
> @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AE_EXPOSURE_MODE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> +				break;
> +			}
>  
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (ExposureModeTable.count(idx)) {
> @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::EXPOSURE_VALUE: {
>  			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  				controller_.GetAlgorithm("agc"));
> -			ASSERT(agc);
> +			if (!agc) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set EXPOSURE_VALUE - no AGC algorithm";
> +				break;
> +			}
>  
>  			/*
>  			 * The SetEv() method takes in a direct exposure multiplier.
> @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  
>  		case controls::AWB_ENABLE: {
>  			RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> -			ASSERT(awb);
> +			if (!awb) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AWB_ENABLE - no AWB algorithm";
> +				break;
> +			}
>  
>  			if (ctrl.second.get<bool>() == false)
>  				awb->Pause();
> @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AWB_MODE: {
>  			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>  				controller_.GetAlgorithm("awb"));
> -			ASSERT(awb);
> +			if (!awb) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set AWB_MODE - no AWB algorithm";
> +				break;
> +			}
>  
>  			int32_t idx = ctrl.second.get<int32_t>();
>  			if (AwbModeTable.count(idx)) {
> @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			auto gains = ctrl.second.get<Span<const float>>();
>  			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>  				controller_.GetAlgorithm("awb"));
> -			ASSERT(awb);
> +			if (!awb) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set COLOUR_GAINS - no AWB algorithm";
> +				break;
> +			}
>  
>  			awb->SetManualGains(gains[0], gains[1]);
>  			if (gains[0] != 0.0f && gains[1] != 0.0f)
> @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::BRIGHTNESS: {
>  			RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
>  				controller_.GetAlgorithm("contrast"));
> -			ASSERT(contrast);
> +			if (!contrast) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set BRIGHTNESS - no contrast algorithm";
> +				break;
> +			}
>  
>  			contrast->SetBrightness(ctrl.second.get<float>() * 65536);
>  			libcameraMetadata_.set(controls::Brightness,
> @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::CONTRAST: {
>  			RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
>  				controller_.GetAlgorithm("contrast"));
> -			ASSERT(contrast);
> +			if (!contrast) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set CONTRAST - no contrast algorithm";
> +				break;
> +			}
>  
>  			contrast->SetContrast(ctrl.second.get<float>());
>  			libcameraMetadata_.set(controls::Contrast,
> @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::SATURATION: {
>  			RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
>  				controller_.GetAlgorithm("ccm"));
> -			ASSERT(ccm);
> +			if (!ccm) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set SATURATION - no ccm algorithm";
> +				break;
> +			}
>  
>  			ccm->SetSaturation(ctrl.second.get<float>());
>  			libcameraMetadata_.set(controls::Saturation,
> @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::SHARPNESS: {
>  			RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
>  				controller_.GetAlgorithm("sharpen"));
> -			ASSERT(sharpen);
> +			if (!sharpen) {
> +				LOG(IPARPI, Warning)
> +					<< "Could not set SHARPNESS - no sharpen algorithm";
> +				break;
> +			}
>  
>  			sharpen->SetStrength(ctrl.second.get<float>());
>  			libcameraMetadata_.set(controls::Sharpness,

Hi Laurent

Thanks for the comments. Let me just add a few more words for the
situation I'm thinking of.

On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:
> > Users are free to avoid loading certain control algorithms from the
> > json tuning file if they wish. Under such circumstances, failing
> > completely when an application tries to set parameters for that
> > algorithm is unhelpful, and it is better just to issue a warning.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
> >  1 file changed, 72 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 5ccc7a65..b57d77e9 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               switch (ctrl.first) {
> >               case controls::AE_ENABLE: {
> >                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> > -                     ASSERT(agc);
> > +                     if (!agc) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set AE_ENABLE - no AGC algorithm";
> > +                             break;
> > +                     }
>
> This is a better behaviour indeed. I wonder if we need some kind of
> LOG_ONCE() to avoid flooding the log, but it would be difficult to do so
> per-camera.
>
> Not aborting when a control is set without a corresponding algorithm is
> good, but I think we need to go one step further and not report the
> control to the application in the first place. This will require
> replacing the static ControlInfoMap in
> include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We
> should then extend the libcamera core to verify that all controls in a
> request are supported and either reject the request or strip the
> unsupported controls, and we won't need this patch anymore.
>
> I'm fine merging this patch as-is, given that the ongoing IPA IPC work
> should make it easier to handle controls dynamically when it will land,
> but it should then probably be reverted.

So the specific thing that led to the change was the following.

Normally I can run a libcamera application of my choice, such as qcam
or the Raspberry Pi libcamera-hello. Now, I might decide that I don't
want the control algorithms to touch (for example) the gamma curve any
more. So I comment the "rpi.contrast" algorithm out of the json file.

Now, qcam will still run quite happily (I think it sets almost no
controls) but libcamera-hello (prior to this change) would assert and
fail. The reason is that it tries to set the brightness to the value
chosen by the user (or its default), and if there's no contrast
algorithm, it can't do this. So the idea is that omitting certain
algorithm(s) doesn't mean you have to start editing your application
code.

However, I actually quite like the fact that the warning nags quite a
lot. It may well be that a single warning would go by during start-up
and is relatively easy to miss... how about a LOG_N_TIMES version?

Thanks!
David

>
> > +
> >                       if (ctrl.second.get<bool>() == false)
> >                               agc->Pause();
> >                       else
> > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::EXPOSURE_TIME: {
> >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> >                               controller_.GetAlgorithm("agc"));
> > -                     ASSERT(agc);
> > +                     if (!agc) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set EXPOSURE_TIME - no AGC algorithm";
> > +                             break;
> > +                     }
> >
> >                       /* This expects units of micro-seconds. */
> >                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::ANALOGUE_GAIN: {
> >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> >                               controller_.GetAlgorithm("agc"));
> > -                     ASSERT(agc);
> > +                     if (!agc) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set ANALOGUE_GAIN - no AGC algorithm";
> > +                             break;
> > +                     }
> > +
> >                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
> >
> >                       libcameraMetadata_.set(controls::AnalogueGain,
> > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::AE_METERING_MODE: {
> >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> >                               controller_.GetAlgorithm("agc"));
> > -                     ASSERT(agc);
> > +                     if (!agc) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set AE_METERING_MODE - no AGC algorithm";
> > +                             break;
> > +                     }
> >
> >                       int32_t idx = ctrl.second.get<int32_t>();
> >                       if (MeteringModeTable.count(idx)) {
> > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::AE_CONSTRAINT_MODE: {
> >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> >                               controller_.GetAlgorithm("agc"));
> > -                     ASSERT(agc);
> > +                     if (!agc) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> > +                             break;
> > +                     }
> >
> >                       int32_t idx = ctrl.second.get<int32_t>();
> >                       if (ConstraintModeTable.count(idx)) {
> > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::AE_EXPOSURE_MODE: {
> >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> >                               controller_.GetAlgorithm("agc"));
> > -                     ASSERT(agc);
> > +                     if (!agc) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> > +                             break;
> > +                     }
> >
> >                       int32_t idx = ctrl.second.get<int32_t>();
> >                       if (ExposureModeTable.count(idx)) {
> > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::EXPOSURE_VALUE: {
> >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> >                               controller_.GetAlgorithm("agc"));
> > -                     ASSERT(agc);
> > +                     if (!agc) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set EXPOSURE_VALUE - no AGC algorithm";
> > +                             break;
> > +                     }
> >
> >                       /*
> >                        * The SetEv() method takes in a direct exposure multiplier.
> > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >
> >               case controls::AWB_ENABLE: {
> >                       RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> > -                     ASSERT(awb);
> > +                     if (!awb) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set AWB_ENABLE - no AWB algorithm";
> > +                             break;
> > +                     }
> >
> >                       if (ctrl.second.get<bool>() == false)
> >                               awb->Pause();
> > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::AWB_MODE: {
> >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> >                               controller_.GetAlgorithm("awb"));
> > -                     ASSERT(awb);
> > +                     if (!awb) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set AWB_MODE - no AWB algorithm";
> > +                             break;
> > +                     }
> >
> >                       int32_t idx = ctrl.second.get<int32_t>();
> >                       if (AwbModeTable.count(idx)) {
> > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >                       auto gains = ctrl.second.get<Span<const float>>();
> >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> >                               controller_.GetAlgorithm("awb"));
> > -                     ASSERT(awb);
> > +                     if (!awb) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set COLOUR_GAINS - no AWB algorithm";
> > +                             break;
> > +                     }
> >
> >                       awb->SetManualGains(gains[0], gains[1]);
> >                       if (gains[0] != 0.0f && gains[1] != 0.0f)
> > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::BRIGHTNESS: {
> >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> >                               controller_.GetAlgorithm("contrast"));
> > -                     ASSERT(contrast);
> > +                     if (!contrast) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set BRIGHTNESS - no contrast algorithm";
> > +                             break;
> > +                     }
> >
> >                       contrast->SetBrightness(ctrl.second.get<float>() * 65536);
> >                       libcameraMetadata_.set(controls::Brightness,
> > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::CONTRAST: {
> >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> >                               controller_.GetAlgorithm("contrast"));
> > -                     ASSERT(contrast);
> > +                     if (!contrast) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set CONTRAST - no contrast algorithm";
> > +                             break;
> > +                     }
> >
> >                       contrast->SetContrast(ctrl.second.get<float>());
> >                       libcameraMetadata_.set(controls::Contrast,
> > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::SATURATION: {
> >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
> >                               controller_.GetAlgorithm("ccm"));
> > -                     ASSERT(ccm);
> > +                     if (!ccm) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set SATURATION - no ccm algorithm";
> > +                             break;
> > +                     }
> >
> >                       ccm->SetSaturation(ctrl.second.get<float>());
> >                       libcameraMetadata_.set(controls::Saturation,
> > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::SHARPNESS: {
> >                       RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
> >                               controller_.GetAlgorithm("sharpen"));
> > -                     ASSERT(sharpen);
> > +                     if (!sharpen) {
> > +                             LOG(IPARPI, Warning)
> > +                                     << "Could not set SHARPNESS - no sharpen algorithm";
> > +                             break;
> > +                     }
> >
> >                       sharpen->SetStrength(ctrl.second.get<float>());
> >                       libcameraMetadata_.set(controls::Sharpness,
>
> --
> Regards,
>
> Laurent Pinchart

Hi David

On Wed, 27 Jan 2021 at 10:44, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi Laurent
>
> Thanks for the comments. Let me just add a few more words for the
> situation I'm thinking of.
>
> On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi David,
> >
> > Thank you for the patch.
> >
> > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:
> > > Users are free to avoid loading certain control algorithms from the
> > > json tuning file if they wish. Under such circumstances, failing
> > > completely when an application tries to set parameters for that
> > > algorithm is unhelpful, and it is better just to issue a warning.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
> > >  1 file changed, 72 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 5ccc7a65..b57d77e9 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               switch (ctrl.first) {
> > >               case controls::AE_ENABLE: {
> > >                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AE_ENABLE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> >
> > This is a better behaviour indeed. I wonder if we need some kind of
> > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so
> > per-camera.
> >
> > Not aborting when a control is set without a corresponding algorithm is
> > good, but I think we need to go one step further and not report the
> > control to the application in the first place. This will require
> > replacing the static ControlInfoMap in
> > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We
> > should then extend the libcamera core to verify that all controls in a
> > request are supported and either reject the request or strip the
> > unsupported controls, and we won't need this patch anymore.
> >
> > I'm fine merging this patch as-is, given that the ongoing IPA IPC work
> > should make it easier to handle controls dynamically when it will land,
> > but it should then probably be reverted.
>
> So the specific thing that led to the change was the following.
>
> Normally I can run a libcamera application of my choice, such as qcam
> or the Raspberry Pi libcamera-hello. Now, I might decide that I don't
> want the control algorithms to touch (for example) the gamma curve any
> more. So I comment the "rpi.contrast" algorithm out of the json file.
>
> Now, qcam will still run quite happily (I think it sets almost no
> controls) but libcamera-hello (prior to this change) would assert and
> fail. The reason is that it tries to set the brightness to the value
> chosen by the user (or its default), and if there's no contrast
> algorithm, it can't do this. So the idea is that omitting certain
> algorithm(s) doesn't mean you have to start editing your application
> code.
>
> However, I actually quite like the fact that the warning nags quite a
> lot. It may well be that a single warning would go by during start-up
> and is relatively easy to miss... how about a LOG_N_TIMES version?

Unless I've misunderstood, won't this also apply for monochrome
sensors where you won't want AWB to run as there is no colour? Logging
any form of error or warning there is really unwanted.

I have an increasing pile of mono sensors with working drivers that I
can pass on if you wanted to try it out! (OV9281, OV7251, and OV2311
for starters).

  Dave

> Thanks!
> David
>
> >
> > > +
> > >                       if (ctrl.second.get<bool>() == false)
> > >                               agc->Pause();
> > >                       else
> > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::EXPOSURE_TIME: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set EXPOSURE_TIME - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       /* This expects units of micro-seconds. */
> > >                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::ANALOGUE_GAIN: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set ANALOGUE_GAIN - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > > +
> > >                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
> > >
> > >                       libcameraMetadata_.set(controls::AnalogueGain,
> > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::AE_METERING_MODE: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AE_METERING_MODE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       int32_t idx = ctrl.second.get<int32_t>();
> > >                       if (MeteringModeTable.count(idx)) {
> > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::AE_CONSTRAINT_MODE: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       int32_t idx = ctrl.second.get<int32_t>();
> > >                       if (ConstraintModeTable.count(idx)) {
> > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::AE_EXPOSURE_MODE: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       int32_t idx = ctrl.second.get<int32_t>();
> > >                       if (ExposureModeTable.count(idx)) {
> > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::EXPOSURE_VALUE: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set EXPOSURE_VALUE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       /*
> > >                        * The SetEv() method takes in a direct exposure multiplier.
> > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >
> > >               case controls::AWB_ENABLE: {
> > >                       RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> > > -                     ASSERT(awb);
> > > +                     if (!awb) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AWB_ENABLE - no AWB algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       if (ctrl.second.get<bool>() == false)
> > >                               awb->Pause();
> > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::AWB_MODE: {
> > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > >                               controller_.GetAlgorithm("awb"));
> > > -                     ASSERT(awb);
> > > +                     if (!awb) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AWB_MODE - no AWB algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       int32_t idx = ctrl.second.get<int32_t>();
> > >                       if (AwbModeTable.count(idx)) {
> > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >                       auto gains = ctrl.second.get<Span<const float>>();
> > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > >                               controller_.GetAlgorithm("awb"));
> > > -                     ASSERT(awb);
> > > +                     if (!awb) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set COLOUR_GAINS - no AWB algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       awb->SetManualGains(gains[0], gains[1]);
> > >                       if (gains[0] != 0.0f && gains[1] != 0.0f)
> > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::BRIGHTNESS: {
> > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > >                               controller_.GetAlgorithm("contrast"));
> > > -                     ASSERT(contrast);
> > > +                     if (!contrast) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set BRIGHTNESS - no contrast algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       contrast->SetBrightness(ctrl.second.get<float>() * 65536);
> > >                       libcameraMetadata_.set(controls::Brightness,
> > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::CONTRAST: {
> > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > >                               controller_.GetAlgorithm("contrast"));
> > > -                     ASSERT(contrast);
> > > +                     if (!contrast) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set CONTRAST - no contrast algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       contrast->SetContrast(ctrl.second.get<float>());
> > >                       libcameraMetadata_.set(controls::Contrast,
> > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::SATURATION: {
> > >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
> > >                               controller_.GetAlgorithm("ccm"));
> > > -                     ASSERT(ccm);
> > > +                     if (!ccm) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set SATURATION - no ccm algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       ccm->SetSaturation(ctrl.second.get<float>());
> > >                       libcameraMetadata_.set(controls::Saturation,
> > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::SHARPNESS: {
> > >                       RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
> > >                               controller_.GetAlgorithm("sharpen"));
> > > -                     ASSERT(sharpen);
> > > +                     if (!sharpen) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set SHARPNESS - no sharpen algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       sharpen->SetStrength(ctrl.second.get<float>());
> > >                       libcameraMetadata_.set(controls::Sharpness,
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi David,

On Wed, Jan 27, 2021 at 10:44:19AM +0000, David Plowman wrote:
> Hi Laurent
> 
> Thanks for the comments. Let me just add a few more words for the
> situation I'm thinking of.
> 
> On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart wrote:
> > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:
> > > Users are free to avoid loading certain control algorithms from the
> > > json tuning file if they wish. Under such circumstances, failing
> > > completely when an application tries to set parameters for that
> > > algorithm is unhelpful, and it is better just to issue a warning.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
> > >  1 file changed, 72 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 5ccc7a65..b57d77e9 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               switch (ctrl.first) {
> > >               case controls::AE_ENABLE: {
> > >                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AE_ENABLE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> >
> > This is a better behaviour indeed. I wonder if we need some kind of
> > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so
> > per-camera.
> >
> > Not aborting when a control is set without a corresponding algorithm is
> > good, but I think we need to go one step further and not report the
> > control to the application in the first place. This will require
> > replacing the static ControlInfoMap in
> > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We
> > should then extend the libcamera core to verify that all controls in a
> > request are supported and either reject the request or strip the
> > unsupported controls, and we won't need this patch anymore.
> >
> > I'm fine merging this patch as-is, given that the ongoing IPA IPC work
> > should make it easier to handle controls dynamically when it will land,
> > but it should then probably be reverted.
> 
> So the specific thing that led to the change was the following.
> 
> Normally I can run a libcamera application of my choice, such as qcam
> or the Raspberry Pi libcamera-hello. Now, I might decide that I don't
> want the control algorithms to touch (for example) the gamma curve any
> more. So I comment the "rpi.contrast" algorithm out of the json file.
> 
> Now, qcam will still run quite happily (I think it sets almost no
> controls) but libcamera-hello (prior to this change) would assert and
> fail. The reason is that it tries to set the brightness to the value
> chosen by the user (or its default), and if there's no contrast
> algorithm, it can't do this. So the idea is that omitting certain
> algorithm(s) doesn't mean you have to start editing your application
> code.

The API concept is that applications should only set controls that are
reported by the camera as supported. That puts a bit of an extra burden
on the application, but for applications that don't care about controls
being ignored if unsupported, it would be easy to implement a small
control set wrapper function that would skip controls that are not
advertised.

> However, I actually quite like the fact that the warning nags quite a
> lot. It may well be that a single warning would go by during start-up
> and is relatively easy to miss... how about a LOG_N_TIMES version?
> 
> > > +
> > >                       if (ctrl.second.get<bool>() == false)
> > >                               agc->Pause();
> > >                       else
> > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::EXPOSURE_TIME: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set EXPOSURE_TIME - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       /* This expects units of micro-seconds. */
> > >                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::ANALOGUE_GAIN: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set ANALOGUE_GAIN - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > > +
> > >                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
> > >
> > >                       libcameraMetadata_.set(controls::AnalogueGain,
> > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::AE_METERING_MODE: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AE_METERING_MODE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       int32_t idx = ctrl.second.get<int32_t>();
> > >                       if (MeteringModeTable.count(idx)) {
> > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::AE_CONSTRAINT_MODE: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       int32_t idx = ctrl.second.get<int32_t>();
> > >                       if (ConstraintModeTable.count(idx)) {
> > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::AE_EXPOSURE_MODE: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       int32_t idx = ctrl.second.get<int32_t>();
> > >                       if (ExposureModeTable.count(idx)) {
> > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::EXPOSURE_VALUE: {
> > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > >                               controller_.GetAlgorithm("agc"));
> > > -                     ASSERT(agc);
> > > +                     if (!agc) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set EXPOSURE_VALUE - no AGC algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       /*
> > >                        * The SetEv() method takes in a direct exposure multiplier.
> > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >
> > >               case controls::AWB_ENABLE: {
> > >                       RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> > > -                     ASSERT(awb);
> > > +                     if (!awb) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AWB_ENABLE - no AWB algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       if (ctrl.second.get<bool>() == false)
> > >                               awb->Pause();
> > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::AWB_MODE: {
> > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > >                               controller_.GetAlgorithm("awb"));
> > > -                     ASSERT(awb);
> > > +                     if (!awb) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set AWB_MODE - no AWB algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       int32_t idx = ctrl.second.get<int32_t>();
> > >                       if (AwbModeTable.count(idx)) {
> > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >                       auto gains = ctrl.second.get<Span<const float>>();
> > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > >                               controller_.GetAlgorithm("awb"));
> > > -                     ASSERT(awb);
> > > +                     if (!awb) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set COLOUR_GAINS - no AWB algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       awb->SetManualGains(gains[0], gains[1]);
> > >                       if (gains[0] != 0.0f && gains[1] != 0.0f)
> > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::BRIGHTNESS: {
> > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > >                               controller_.GetAlgorithm("contrast"));
> > > -                     ASSERT(contrast);
> > > +                     if (!contrast) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set BRIGHTNESS - no contrast algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       contrast->SetBrightness(ctrl.second.get<float>() * 65536);
> > >                       libcameraMetadata_.set(controls::Brightness,
> > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::CONTRAST: {
> > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > >                               controller_.GetAlgorithm("contrast"));
> > > -                     ASSERT(contrast);
> > > +                     if (!contrast) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set CONTRAST - no contrast algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       contrast->SetContrast(ctrl.second.get<float>());
> > >                       libcameraMetadata_.set(controls::Contrast,
> > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::SATURATION: {
> > >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
> > >                               controller_.GetAlgorithm("ccm"));
> > > -                     ASSERT(ccm);
> > > +                     if (!ccm) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set SATURATION - no ccm algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       ccm->SetSaturation(ctrl.second.get<float>());
> > >                       libcameraMetadata_.set(controls::Saturation,
> > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >               case controls::SHARPNESS: {
> > >                       RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
> > >                               controller_.GetAlgorithm("sharpen"));
> > > -                     ASSERT(sharpen);
> > > +                     if (!sharpen) {
> > > +                             LOG(IPARPI, Warning)
> > > +                                     << "Could not set SHARPNESS - no sharpen algorithm";
> > > +                             break;
> > > +                     }
> > >
> > >                       sharpen->SetStrength(ctrl.second.get<float>());
> > >                       libcameraMetadata_.set(controls::Sharpness,

Hi Dave

Yes, I'm a bit in several minds over monochrome sensors. Should
algorithms know whether sensors are colour or monochrome? Should
certain algorithms just be deleted from the tuning file? Probably, but
it does want some thinking about.

As regards this patch, I don't think anything gets worse (actually you
can delete the AWB and CCM algorithms and your camera will still run).
There should be just one grumble at startup when it tries to set the
AWB mode. AGC is actually more annoying, though it's not affected
either way by this patch, as it complains repeatedly if it has no AWB
metadata. Either the algorithm would have to know not to care, or
perhaps that warning isn't really very useful these days and could be
demoted.

Certainly someone (e.g. me) needs to spend some time with these
monochrome sensors, calibrating them and making sure everything works.
The tuning tool will have issues with them too, but it would be great
to have more supported sensors. Do they all have fully functional V4L2
drivers for libcamera?

Thanks!
David

On Wed, 27 Jan 2021 at 11:00, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi David
>
> On Wed, 27 Jan 2021 at 10:44, David Plowman
> <david.plowman@raspberrypi.com> wrote:
> >
> > Hi Laurent
> >
> > Thanks for the comments. Let me just add a few more words for the
> > situation I'm thinking of.
> >
> > On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi David,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:
> > > > Users are free to avoid loading certain control algorithms from the
> > > > json tuning file if they wish. Under such circumstances, failing
> > > > completely when an application tries to set parameters for that
> > > > algorithm is unhelpful, and it is better just to issue a warning.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > ---
> > > >  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
> > > >  1 file changed, 72 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index 5ccc7a65..b57d77e9 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               switch (ctrl.first) {
> > > >               case controls::AE_ENABLE: {
> > > >                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AE_ENABLE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > >
> > > This is a better behaviour indeed. I wonder if we need some kind of
> > > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so
> > > per-camera.
> > >
> > > Not aborting when a control is set without a corresponding algorithm is
> > > good, but I think we need to go one step further and not report the
> > > control to the application in the first place. This will require
> > > replacing the static ControlInfoMap in
> > > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We
> > > should then extend the libcamera core to verify that all controls in a
> > > request are supported and either reject the request or strip the
> > > unsupported controls, and we won't need this patch anymore.
> > >
> > > I'm fine merging this patch as-is, given that the ongoing IPA IPC work
> > > should make it easier to handle controls dynamically when it will land,
> > > but it should then probably be reverted.
> >
> > So the specific thing that led to the change was the following.
> >
> > Normally I can run a libcamera application of my choice, such as qcam
> > or the Raspberry Pi libcamera-hello. Now, I might decide that I don't
> > want the control algorithms to touch (for example) the gamma curve any
> > more. So I comment the "rpi.contrast" algorithm out of the json file.
> >
> > Now, qcam will still run quite happily (I think it sets almost no
> > controls) but libcamera-hello (prior to this change) would assert and
> > fail. The reason is that it tries to set the brightness to the value
> > chosen by the user (or its default), and if there's no contrast
> > algorithm, it can't do this. So the idea is that omitting certain
> > algorithm(s) doesn't mean you have to start editing your application
> > code.
> >
> > However, I actually quite like the fact that the warning nags quite a
> > lot. It may well be that a single warning would go by during start-up
> > and is relatively easy to miss... how about a LOG_N_TIMES version?
>
> Unless I've misunderstood, won't this also apply for monochrome
> sensors where you won't want AWB to run as there is no colour? Logging
> any form of error or warning there is really unwanted.
>
> I have an increasing pile of mono sensors with working drivers that I
> can pass on if you wanted to try it out! (OV9281, OV7251, and OV2311
> for starters).
>
>   Dave
>
> > Thanks!
> > David
> >
> > >
> > > > +
> > > >                       if (ctrl.second.get<bool>() == false)
> > > >                               agc->Pause();
> > > >                       else
> > > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::EXPOSURE_TIME: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set EXPOSURE_TIME - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       /* This expects units of micro-seconds. */
> > > >                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> > > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::ANALOGUE_GAIN: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set ANALOGUE_GAIN - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > > +
> > > >                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
> > > >
> > > >                       libcameraMetadata_.set(controls::AnalogueGain,
> > > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::AE_METERING_MODE: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AE_METERING_MODE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > >                       if (MeteringModeTable.count(idx)) {
> > > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::AE_CONSTRAINT_MODE: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > >                       if (ConstraintModeTable.count(idx)) {
> > > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::AE_EXPOSURE_MODE: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > >                       if (ExposureModeTable.count(idx)) {
> > > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::EXPOSURE_VALUE: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set EXPOSURE_VALUE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       /*
> > > >                        * The SetEv() method takes in a direct exposure multiplier.
> > > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >
> > > >               case controls::AWB_ENABLE: {
> > > >                       RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> > > > -                     ASSERT(awb);
> > > > +                     if (!awb) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AWB_ENABLE - no AWB algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       if (ctrl.second.get<bool>() == false)
> > > >                               awb->Pause();
> > > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::AWB_MODE: {
> > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > >                               controller_.GetAlgorithm("awb"));
> > > > -                     ASSERT(awb);
> > > > +                     if (!awb) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AWB_MODE - no AWB algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > >                       if (AwbModeTable.count(idx)) {
> > > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >                       auto gains = ctrl.second.get<Span<const float>>();
> > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > >                               controller_.GetAlgorithm("awb"));
> > > > -                     ASSERT(awb);
> > > > +                     if (!awb) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set COLOUR_GAINS - no AWB algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       awb->SetManualGains(gains[0], gains[1]);
> > > >                       if (gains[0] != 0.0f && gains[1] != 0.0f)
> > > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::BRIGHTNESS: {
> > > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > > >                               controller_.GetAlgorithm("contrast"));
> > > > -                     ASSERT(contrast);
> > > > +                     if (!contrast) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set BRIGHTNESS - no contrast algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       contrast->SetBrightness(ctrl.second.get<float>() * 65536);
> > > >                       libcameraMetadata_.set(controls::Brightness,
> > > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::CONTRAST: {
> > > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > > >                               controller_.GetAlgorithm("contrast"));
> > > > -                     ASSERT(contrast);
> > > > +                     if (!contrast) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set CONTRAST - no contrast algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       contrast->SetContrast(ctrl.second.get<float>());
> > > >                       libcameraMetadata_.set(controls::Contrast,
> > > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::SATURATION: {
> > > >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
> > > >                               controller_.GetAlgorithm("ccm"));
> > > > -                     ASSERT(ccm);
> > > > +                     if (!ccm) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set SATURATION - no ccm algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       ccm->SetSaturation(ctrl.second.get<float>());
> > > >                       libcameraMetadata_.set(controls::Saturation,
> > > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::SHARPNESS: {
> > > >                       RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
> > > >                               controller_.GetAlgorithm("sharpen"));
> > > > -                     ASSERT(sharpen);
> > > > +                     if (!sharpen) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set SHARPNESS - no sharpen algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       sharpen->SetStrength(ctrl.second.get<float>());
> > > >                       libcameraMetadata_.set(controls::Sharpness,
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent

On Wed, 27 Jan 2021 at 11:17, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> On Wed, Jan 27, 2021 at 10:44:19AM +0000, David Plowman wrote:
> > Hi Laurent
> >
> > Thanks for the comments. Let me just add a few more words for the
> > situation I'm thinking of.
> >
> > On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart wrote:
> > > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:
> > > > Users are free to avoid loading certain control algorithms from the
> > > > json tuning file if they wish. Under such circumstances, failing
> > > > completely when an application tries to set parameters for that
> > > > algorithm is unhelpful, and it is better just to issue a warning.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > ---
> > > >  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
> > > >  1 file changed, 72 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index 5ccc7a65..b57d77e9 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               switch (ctrl.first) {
> > > >               case controls::AE_ENABLE: {
> > > >                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AE_ENABLE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > >
> > > This is a better behaviour indeed. I wonder if we need some kind of
> > > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so
> > > per-camera.
> > >
> > > Not aborting when a control is set without a corresponding algorithm is
> > > good, but I think we need to go one step further and not report the
> > > control to the application in the first place. This will require
> > > replacing the static ControlInfoMap in
> > > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We
> > > should then extend the libcamera core to verify that all controls in a
> > > request are supported and either reject the request or strip the
> > > unsupported controls, and we won't need this patch anymore.
> > >
> > > I'm fine merging this patch as-is, given that the ongoing IPA IPC work
> > > should make it easier to handle controls dynamically when it will land,
> > > but it should then probably be reverted.
> >
> > So the specific thing that led to the change was the following.
> >
> > Normally I can run a libcamera application of my choice, such as qcam
> > or the Raspberry Pi libcamera-hello. Now, I might decide that I don't
> > want the control algorithms to touch (for example) the gamma curve any
> > more. So I comment the "rpi.contrast" algorithm out of the json file.
> >
> > Now, qcam will still run quite happily (I think it sets almost no
> > controls) but libcamera-hello (prior to this change) would assert and
> > fail. The reason is that it tries to set the brightness to the value
> > chosen by the user (or its default), and if there's no contrast
> > algorithm, it can't do this. So the idea is that omitting certain
> > algorithm(s) doesn't mean you have to start editing your application
> > code.
>
> The API concept is that applications should only set controls that are
> reported by the camera as supported. That puts a bit of an extra burden
> on the application, but for applications that don't care about controls
> being ignored if unsupported, it would be easy to implement a small
> control set wrapper function that would skip controls that are not
> advertised.

Yes, I understand you now. We need "dynamic controls" which will
depend in our case probably on the control algorithms that have been
loaded. Checking whether controls are actually supported is perhaps a
little tiresome, but I don't particularly object to our applications
doing that.

Thanks for the clarifications!
Davd

>
> > However, I actually quite like the fact that the warning nags quite a
> > lot. It may well be that a single warning would go by during start-up
> > and is relatively easy to miss... how about a LOG_N_TIMES version?
> >
> > > > +
> > > >                       if (ctrl.second.get<bool>() == false)
> > > >                               agc->Pause();
> > > >                       else
> > > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::EXPOSURE_TIME: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set EXPOSURE_TIME - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       /* This expects units of micro-seconds. */
> > > >                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> > > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::ANALOGUE_GAIN: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set ANALOGUE_GAIN - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > > +
> > > >                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
> > > >
> > > >                       libcameraMetadata_.set(controls::AnalogueGain,
> > > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::AE_METERING_MODE: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AE_METERING_MODE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > >                       if (MeteringModeTable.count(idx)) {
> > > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::AE_CONSTRAINT_MODE: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > >                       if (ConstraintModeTable.count(idx)) {
> > > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::AE_EXPOSURE_MODE: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > >                       if (ExposureModeTable.count(idx)) {
> > > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::EXPOSURE_VALUE: {
> > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > >                               controller_.GetAlgorithm("agc"));
> > > > -                     ASSERT(agc);
> > > > +                     if (!agc) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set EXPOSURE_VALUE - no AGC algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       /*
> > > >                        * The SetEv() method takes in a direct exposure multiplier.
> > > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >
> > > >               case controls::AWB_ENABLE: {
> > > >                       RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> > > > -                     ASSERT(awb);
> > > > +                     if (!awb) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AWB_ENABLE - no AWB algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       if (ctrl.second.get<bool>() == false)
> > > >                               awb->Pause();
> > > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::AWB_MODE: {
> > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > >                               controller_.GetAlgorithm("awb"));
> > > > -                     ASSERT(awb);
> > > > +                     if (!awb) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set AWB_MODE - no AWB algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > >                       if (AwbModeTable.count(idx)) {
> > > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >                       auto gains = ctrl.second.get<Span<const float>>();
> > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > >                               controller_.GetAlgorithm("awb"));
> > > > -                     ASSERT(awb);
> > > > +                     if (!awb) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set COLOUR_GAINS - no AWB algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       awb->SetManualGains(gains[0], gains[1]);
> > > >                       if (gains[0] != 0.0f && gains[1] != 0.0f)
> > > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::BRIGHTNESS: {
> > > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > > >                               controller_.GetAlgorithm("contrast"));
> > > > -                     ASSERT(contrast);
> > > > +                     if (!contrast) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set BRIGHTNESS - no contrast algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       contrast->SetBrightness(ctrl.second.get<float>() * 65536);
> > > >                       libcameraMetadata_.set(controls::Brightness,
> > > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::CONTRAST: {
> > > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > > >                               controller_.GetAlgorithm("contrast"));
> > > > -                     ASSERT(contrast);
> > > > +                     if (!contrast) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set CONTRAST - no contrast algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       contrast->SetContrast(ctrl.second.get<float>());
> > > >                       libcameraMetadata_.set(controls::Contrast,
> > > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::SATURATION: {
> > > >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
> > > >                               controller_.GetAlgorithm("ccm"));
> > > > -                     ASSERT(ccm);
> > > > +                     if (!ccm) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set SATURATION - no ccm algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       ccm->SetSaturation(ctrl.second.get<float>());
> > > >                       libcameraMetadata_.set(controls::Saturation,
> > > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >               case controls::SHARPNESS: {
> > > >                       RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
> > > >                               controller_.GetAlgorithm("sharpen"));
> > > > -                     ASSERT(sharpen);
> > > > +                     if (!sharpen) {
> > > > +                             LOG(IPARPI, Warning)
> > > > +                                     << "Could not set SHARPNESS - no sharpen algorithm";
> > > > +                             break;
> > > > +                     }
> > > >
> > > >                       sharpen->SetStrength(ctrl.second.get<float>());
> > > >                       libcameraMetadata_.set(controls::Sharpness,
>
> --
> Regards,
>
> Laurent Pinchart

On Wed, 27 Jan 2021 at 11:33, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi Dave
>
> Yes, I'm a bit in several minds over monochrome sensors. Should
> algorithms know whether sensors are colour or monochrome? Should
> certain algorithms just be deleted from the tuning file? Probably, but
> it does want some thinking about.
>
> As regards this patch, I don't think anything gets worse (actually you
> can delete the AWB and CCM algorithms and your camera will still run).
> There should be just one grumble at startup when it tries to set the
> AWB mode. AGC is actually more annoying, though it's not affected
> either way by this patch, as it complains repeatedly if it has no AWB
> metadata. Either the algorithm would have to know not to care, or
> perhaps that warning isn't really very useful these days and could be
> demoted.
>
> Certainly someone (e.g. me) needs to spend some time with these
> monochrome sensors, calibrating them and making sure everything works.
> The tuning tool will have issues with them too, but it would be great
> to have more supported sensors. Do they all have fully functional V4L2
> drivers for libcamera?

Functional V4L2 sensor drivers and dtoverlays - yes.
They'll need cam_helper implementations in order to work with libcamera.

OV2311 isn't merged to rpi-5.10.y as yet as I only found a published
register set about a week ago.
https://github.com/6by9/linux/tree/ov2311 is streaming images and has
the normal controls (calibration needs checking).

OV7251 (0.3MP global shutter) and OV9281 (1MPix global shutter) are
both in rpi-5.10.y. I believe they all have the required V4L2 controls
for libcamera, but haven't double checked.

  Dave

> Thanks!
> David
>
> On Wed, 27 Jan 2021 at 11:00, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi David
> >
> > On Wed, 27 Jan 2021 at 10:44, David Plowman
> > <david.plowman@raspberrypi.com> wrote:
> > >
> > > Hi Laurent
> > >
> > > Thanks for the comments. Let me just add a few more words for the
> > > situation I'm thinking of.
> > >
> > > On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com> wrote:
> > > >
> > > > Hi David,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:
> > > > > Users are free to avoid loading certain control algorithms from the
> > > > > json tuning file if they wish. Under such circumstances, failing
> > > > > completely when an application tries to set parameters for that
> > > > > algorithm is unhelpful, and it is better just to issue a warning.
> > > > >
> > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > ---
> > > > >  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
> > > > >  1 file changed, 72 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index 5ccc7a65..b57d77e9 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               switch (ctrl.first) {
> > > > >               case controls::AE_ENABLE: {
> > > > >                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AE_ENABLE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > >
> > > > This is a better behaviour indeed. I wonder if we need some kind of
> > > > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so
> > > > per-camera.
> > > >
> > > > Not aborting when a control is set without a corresponding algorithm is
> > > > good, but I think we need to go one step further and not report the
> > > > control to the application in the first place. This will require
> > > > replacing the static ControlInfoMap in
> > > > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We
> > > > should then extend the libcamera core to verify that all controls in a
> > > > request are supported and either reject the request or strip the
> > > > unsupported controls, and we won't need this patch anymore.
> > > >
> > > > I'm fine merging this patch as-is, given that the ongoing IPA IPC work
> > > > should make it easier to handle controls dynamically when it will land,
> > > > but it should then probably be reverted.
> > >
> > > So the specific thing that led to the change was the following.
> > >
> > > Normally I can run a libcamera application of my choice, such as qcam
> > > or the Raspberry Pi libcamera-hello. Now, I might decide that I don't
> > > want the control algorithms to touch (for example) the gamma curve any
> > > more. So I comment the "rpi.contrast" algorithm out of the json file.
> > >
> > > Now, qcam will still run quite happily (I think it sets almost no
> > > controls) but libcamera-hello (prior to this change) would assert and
> > > fail. The reason is that it tries to set the brightness to the value
> > > chosen by the user (or its default), and if there's no contrast
> > > algorithm, it can't do this. So the idea is that omitting certain
> > > algorithm(s) doesn't mean you have to start editing your application
> > > code.
> > >
> > > However, I actually quite like the fact that the warning nags quite a
> > > lot. It may well be that a single warning would go by during start-up
> > > and is relatively easy to miss... how about a LOG_N_TIMES version?
> >
> > Unless I've misunderstood, won't this also apply for monochrome
> > sensors where you won't want AWB to run as there is no colour? Logging
> > any form of error or warning there is really unwanted.
> >
> > I have an increasing pile of mono sensors with working drivers that I
> > can pass on if you wanted to try it out! (OV9281, OV7251, and OV2311
> > for starters).
> >
> >   Dave
> >
> > > Thanks!
> > > David
> > >
> > > >
> > > > > +
> > > > >                       if (ctrl.second.get<bool>() == false)
> > > > >                               agc->Pause();
> > > > >                       else
> > > > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::EXPOSURE_TIME: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set EXPOSURE_TIME - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       /* This expects units of micro-seconds. */
> > > > >                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> > > > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::ANALOGUE_GAIN: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set ANALOGUE_GAIN - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > > +
> > > > >                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
> > > > >
> > > > >                       libcameraMetadata_.set(controls::AnalogueGain,
> > > > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::AE_METERING_MODE: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AE_METERING_MODE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > > >                       if (MeteringModeTable.count(idx)) {
> > > > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::AE_CONSTRAINT_MODE: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > > >                       if (ConstraintModeTable.count(idx)) {
> > > > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::AE_EXPOSURE_MODE: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > > >                       if (ExposureModeTable.count(idx)) {
> > > > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::EXPOSURE_VALUE: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set EXPOSURE_VALUE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       /*
> > > > >                        * The SetEv() method takes in a direct exposure multiplier.
> > > > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >
> > > > >               case controls::AWB_ENABLE: {
> > > > >                       RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> > > > > -                     ASSERT(awb);
> > > > > +                     if (!awb) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AWB_ENABLE - no AWB algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       if (ctrl.second.get<bool>() == false)
> > > > >                               awb->Pause();
> > > > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::AWB_MODE: {
> > > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("awb"));
> > > > > -                     ASSERT(awb);
> > > > > +                     if (!awb) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AWB_MODE - no AWB algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > > >                       if (AwbModeTable.count(idx)) {
> > > > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >                       auto gains = ctrl.second.get<Span<const float>>();
> > > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("awb"));
> > > > > -                     ASSERT(awb);
> > > > > +                     if (!awb) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set COLOUR_GAINS - no AWB algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       awb->SetManualGains(gains[0], gains[1]);
> > > > >                       if (gains[0] != 0.0f && gains[1] != 0.0f)
> > > > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::BRIGHTNESS: {
> > > > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("contrast"));
> > > > > -                     ASSERT(contrast);
> > > > > +                     if (!contrast) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set BRIGHTNESS - no contrast algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       contrast->SetBrightness(ctrl.second.get<float>() * 65536);
> > > > >                       libcameraMetadata_.set(controls::Brightness,
> > > > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::CONTRAST: {
> > > > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("contrast"));
> > > > > -                     ASSERT(contrast);
> > > > > +                     if (!contrast) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set CONTRAST - no contrast algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       contrast->SetContrast(ctrl.second.get<float>());
> > > > >                       libcameraMetadata_.set(controls::Contrast,
> > > > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::SATURATION: {
> > > > >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("ccm"));
> > > > > -                     ASSERT(ccm);
> > > > > +                     if (!ccm) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set SATURATION - no ccm algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       ccm->SetSaturation(ctrl.second.get<float>());
> > > > >                       libcameraMetadata_.set(controls::Saturation,
> > > > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::SHARPNESS: {
> > > > >                       RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("sharpen"));
> > > > > -                     ASSERT(sharpen);
> > > > > +                     if (!sharpen) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set SHARPNESS - no sharpen algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       sharpen->SetStrength(ctrl.second.get<float>());
> > > > >                       libcameraMetadata_.set(controls::Sharpness,
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Laurent Pinchart
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel

Hi everyone

I was wondering if I might give this one another little nudge. I've
been working with a different sensor for a few days using the
"uncalibrated.json" tuning... only the Raspberry Pi libcamera-apps all
fail without the patch because that json file only loads a skeleton
set of control algorithms.

I'm fine to change this back once we have dynamic controls - perhaps
that wants a "\todo" in the commit message or the source file? I'd be
happy to add that and re-submit.

Thanks!
David

On Wed, 27 Jan 2021 at 11:43, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi Laurent
>
> On Wed, 27 Jan 2021 at 11:17, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi David,
> >
> > On Wed, Jan 27, 2021 at 10:44:19AM +0000, David Plowman wrote:
> > > Hi Laurent
> > >
> > > Thanks for the comments. Let me just add a few more words for the
> > > situation I'm thinking of.
> > >
> > > On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart wrote:
> > > > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:
> > > > > Users are free to avoid loading certain control algorithms from the
> > > > > json tuning file if they wish. Under such circumstances, failing
> > > > > completely when an application tries to set parameters for that
> > > > > algorithm is unhelpful, and it is better just to issue a warning.
> > > > >
> > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > ---
> > > > >  src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----
> > > > >  1 file changed, 72 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index 5ccc7a65..b57d77e9 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               switch (ctrl.first) {
> > > > >               case controls::AE_ENABLE: {
> > > > >                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AE_ENABLE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > >
> > > > This is a better behaviour indeed. I wonder if we need some kind of
> > > > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so
> > > > per-camera.
> > > >
> > > > Not aborting when a control is set without a corresponding algorithm is
> > > > good, but I think we need to go one step further and not report the
> > > > control to the application in the first place. This will require
> > > > replacing the static ControlInfoMap in
> > > > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We
> > > > should then extend the libcamera core to verify that all controls in a
> > > > request are supported and either reject the request or strip the
> > > > unsupported controls, and we won't need this patch anymore.
> > > >
> > > > I'm fine merging this patch as-is, given that the ongoing IPA IPC work
> > > > should make it easier to handle controls dynamically when it will land,
> > > > but it should then probably be reverted.
> > >
> > > So the specific thing that led to the change was the following.
> > >
> > > Normally I can run a libcamera application of my choice, such as qcam
> > > or the Raspberry Pi libcamera-hello. Now, I might decide that I don't
> > > want the control algorithms to touch (for example) the gamma curve any
> > > more. So I comment the "rpi.contrast" algorithm out of the json file.
> > >
> > > Now, qcam will still run quite happily (I think it sets almost no
> > > controls) but libcamera-hello (prior to this change) would assert and
> > > fail. The reason is that it tries to set the brightness to the value
> > > chosen by the user (or its default), and if there's no contrast
> > > algorithm, it can't do this. So the idea is that omitting certain
> > > algorithm(s) doesn't mean you have to start editing your application
> > > code.
> >
> > The API concept is that applications should only set controls that are
> > reported by the camera as supported. That puts a bit of an extra burden
> > on the application, but for applications that don't care about controls
> > being ignored if unsupported, it would be easy to implement a small
> > control set wrapper function that would skip controls that are not
> > advertised.
>
> Yes, I understand you now. We need "dynamic controls" which will
> depend in our case probably on the control algorithms that have been
> loaded. Checking whether controls are actually supported is perhaps a
> little tiresome, but I don't particularly object to our applications
> doing that.
>
> Thanks for the clarifications!
> Davd
>
> >
> > > However, I actually quite like the fact that the warning nags quite a
> > > lot. It may well be that a single warning would go by during start-up
> > > and is relatively easy to miss... how about a LOG_N_TIMES version?
> > >
> > > > > +
> > > > >                       if (ctrl.second.get<bool>() == false)
> > > > >                               agc->Pause();
> > > > >                       else
> > > > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::EXPOSURE_TIME: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set EXPOSURE_TIME - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       /* This expects units of micro-seconds. */
> > > > >                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> > > > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::ANALOGUE_GAIN: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set ANALOGUE_GAIN - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > > +
> > > > >                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
> > > > >
> > > > >                       libcameraMetadata_.set(controls::AnalogueGain,
> > > > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::AE_METERING_MODE: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AE_METERING_MODE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > > >                       if (MeteringModeTable.count(idx)) {
> > > > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::AE_CONSTRAINT_MODE: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AE_CONSTRAINT_MODE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > > >                       if (ConstraintModeTable.count(idx)) {
> > > > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::AE_EXPOSURE_MODE: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AE_EXPOSURE_MODE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > > >                       if (ExposureModeTable.count(idx)) {
> > > > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::EXPOSURE_VALUE: {
> > > > >                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("agc"));
> > > > > -                     ASSERT(agc);
> > > > > +                     if (!agc) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set EXPOSURE_VALUE - no AGC algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       /*
> > > > >                        * The SetEv() method takes in a direct exposure multiplier.
> > > > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >
> > > > >               case controls::AWB_ENABLE: {
> > > > >                       RPiController::Algorithm *awb = controller_.GetAlgorithm("awb");
> > > > > -                     ASSERT(awb);
> > > > > +                     if (!awb) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AWB_ENABLE - no AWB algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       if (ctrl.second.get<bool>() == false)
> > > > >                               awb->Pause();
> > > > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::AWB_MODE: {
> > > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("awb"));
> > > > > -                     ASSERT(awb);
> > > > > +                     if (!awb) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set AWB_MODE - no AWB algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       int32_t idx = ctrl.second.get<int32_t>();
> > > > >                       if (AwbModeTable.count(idx)) {
> > > > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >                       auto gains = ctrl.second.get<Span<const float>>();
> > > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("awb"));
> > > > > -                     ASSERT(awb);
> > > > > +                     if (!awb) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set COLOUR_GAINS - no AWB algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       awb->SetManualGains(gains[0], gains[1]);
> > > > >                       if (gains[0] != 0.0f && gains[1] != 0.0f)
> > > > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::BRIGHTNESS: {
> > > > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("contrast"));
> > > > > -                     ASSERT(contrast);
> > > > > +                     if (!contrast) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set BRIGHTNESS - no contrast algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       contrast->SetBrightness(ctrl.second.get<float>() * 65536);
> > > > >                       libcameraMetadata_.set(controls::Brightness,
> > > > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::CONTRAST: {
> > > > >                       RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("contrast"));
> > > > > -                     ASSERT(contrast);
> > > > > +                     if (!contrast) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set CONTRAST - no contrast algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       contrast->SetContrast(ctrl.second.get<float>());
> > > > >                       libcameraMetadata_.set(controls::Contrast,
> > > > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::SATURATION: {
> > > > >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("ccm"));
> > > > > -                     ASSERT(ccm);
> > > > > +                     if (!ccm) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set SATURATION - no ccm algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       ccm->SetSaturation(ctrl.second.get<float>());
> > > > >                       libcameraMetadata_.set(controls::Saturation,
> > > > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >               case controls::SHARPNESS: {
> > > > >                       RPiController::SharpenAlgorithm *sharpen = dynamic_cast<RPiController::SharpenAlgorithm *>(
> > > > >                               controller_.GetAlgorithm("sharpen"));
> > > > > -                     ASSERT(sharpen);
> > > > > +                     if (!sharpen) {
> > > > > +                             LOG(IPARPI, Warning)
> > > > > +                                     << "Could not set SHARPNESS - no sharpen algorithm";
> > > > > +                             break;
> > > > > +                     }
> > > > >
> > > > >                       sharpen->SetStrength(ctrl.second.get<float>());
> > > > >                       libcameraMetadata_.set(controls::Sharpness,
> >
> > --
> > Regards,
> >
> > Laurent Pinchart