[libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control algorithms are missing; do not fail
David Plowman
david.plowman at raspberrypi.com
Wed Jan 27 12:33:28 CET 2021
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 at raspberrypi.com> wrote:
>
> 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