[libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control algorithms are missing; do not fail
Dave Stevenson
dave.stevenson at raspberrypi.com
Wed Jan 27 11:59:52 CET 2021
Hi David
On Wed, 27 Jan 2021 at 10:44, David Plowman
<david.plowman at 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 at 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 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.
>
> 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list