[libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control algorithms are missing; do not fail
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 27 12:17:37 CET 2021
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 at 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,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list