[libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control algorithms are missing; do not fail
David Plowman
david.plowman at raspberrypi.com
Mon Feb 1 16:27:34 CET 2021
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 at raspberrypi.com> wrote:
>
> Hi Laurent
>
> On Wed, 27 Jan 2021 at 11:17, Laurent Pinchart
> <laurent.pinchart at 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 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.
>
> 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
More information about the libcamera-devel
mailing list