[libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control algorithms are missing; do not fail
Dave Stevenson
dave.stevenson at raspberrypi.com
Wed Jan 27 13:02:45 CET 2021
On Wed, 27 Jan 2021 at 11:33, David Plowman
<david.plowman at 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 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