[libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests setting AF controls to the AF algorithm

[Laurent Pinchart]

Hello,

On Thu, Jul 14, 2022 at 09:47:55PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)
> > On Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > Pass the controls set by top level API to the AF algorithm if it
> > > was enabled.
> > >
> > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 50 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 01bb54fb..53b53f12 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -28,6 +28,7 @@
> > >  #include "libcamera/internal/mapped_framebuffer.h"
> > >  #include "libcamera/internal/yaml_parser.h"
> > >
> > > +#include "algorithms/af.h"
> > >  #include "algorithms/agc.h"
> > >  #include "algorithms/algorithm.h"
> > >  #include "algorithms/awb.h"
> > > @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> > >  }
> > >
> > >  void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,
> > > -                          [[maybe_unused]] const ControlList &controls)
> > > +                          const ControlList &controls)
> > >  {
> > > -     /* \todo Start processing for 'frame' based on 'controls'. */
> > > +     using namespace algorithms;
> > > +
> > > +     for (auto const &ctrl : controls) {
> > 
> > You can use structured bindings, they're nicer :)
> > 
> >         for (auto const &[id, val] : controls) {
> > 
> >         }
> > 
> > > +             unsigned int ctrlEnum = ctrl.first;
> > > +             const ControlValue &ctrlValue = ctrl.second;
> > 
> > And drop these
> > > +
> > > +             LOG(IPARkISP1, Debug) << "Request ctrl: "
> > > +                                   << controls::controls.at(ctrlEnum)->name()
> > > +                                   << " = " << ctrlValue.toString();
> > > +
> > > +             switch (ctrlEnum) {
> > > +             case controls::AF_MODE: {
> > > +                     Af *af = getAlgorithm<Af>();
> > > +                     if (!af) {
> > > +                             LOG(IPARkISP1, Warning) << "Could not set AF_MODE - no AF algorithm";
> > > +                             break;
> > > +                     }
> > 
> > You can get *af once outside of the switch.
> > 
> > Also, as the failure in getting *af is not related to the control,
> > there's not much value in duplicating the error message, should
> > getAlgorithm<>() be made loud on failure so that the caller can skip
> > doing the same, if not required ?
> 
> I had actually envisaged handling controls differently, adding a hook to
> each algorithm called either queueRequest() or processControls() or
> such, and let each algorithm handle only the controls it uses.

Do you mean https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031931.html
and https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031932.html ? :-)

> The drawback there is that if a control goes unhandled, it would be
> difficult for us to detect or report that, so I think I like this
> approach too.

We could let the Algorithm::queueRequest() function indicate which
control(s) it has handled, and verify at the end that all controls have
been handled.

> Especially as you cover that exact condition in the default case below!
> 
> My only worry is that switch table could get really large though, and
> the algorithms will have to have a public API to handle all each control
> specifically which could get extensive.

That's what the RPi IPA module does, and I'm not a big fan of the end
result.

> The only alternative I can think of off the top of my head to consider
> could be:
> 
> ControlList afControls;
> ControlList agcControls;
> 
> for (auto const control : controls) {
> 	case controls::AF_TRIGGER:
> 	case controls::AF_PAUSE:
> 		afControls.emplace(control);
> 		break;
> 
> 	case controls::AE_ENABLE:
> 	case controls::AE_METERING_MODE:
> 		agcControls.emplace(control);
> 		break;
> 
> 	default:
> 		// unhandled control error
> 		break;
> }
> 
> 
> if (afControls.size()) {
> 	// I would probably store all the instantiated algorithms
> 	// as a named private pointer variable in the IPARkISP1 class.
> 
> 	if (!af_) {
> 		LOG(IPARkISP1, Warning) << "Unhandled AF controls";
> 	} else {
> 		af_->setControls(afControls);
> 	}
> }
> 
> if (agcControls.size()) {
> 
> 	if (!agc_) {
> 		LOG(IPARkISP1, Warning) << "Unhandled AGC controls";
> 	} else {
> 		agc_->setControls(agcControls);
> 	}
> }
> 
> Perhaps some of that boilerplate for each algorithm could get mapped
> down in a template. And also perhaps this introduces more iteration and
> copying than would be desired too - it's only sketching out an idea to
> see if it's easier to keep the definition of how controls are handled by
> algorithms managed by the algorithms themselves.
> 
> > > +
> > > +                     af->setMode(static_cast<controls::AfModeEnum>(ctrlValue.get<int32_t>()));
> > > +                     break;
> > > +             }
> > > +             case controls::AF_TRIGGER: {
> > > +                     Af *af = getAlgorithm<Af>();
> > > +                     if (!af) {
> > > +                             LOG(IPARkISP1, Warning) << "Could not set AF_TRIGGER - no AF algorithm";
> > > +                             break;
> > > +                     }
> > > +
> > > +                     af->setTrigger(static_cast<controls::AfTriggerEnum>(ctrlValue.get<int32_t>()));
> > > +                     break;
> > > +             }
> > > +             case controls::AF_PAUSE: {
> > > +                     Af *af = getAlgorithm<Af>();
> > > +                     if (!af) {
> > > +                             LOG(IPARkISP1, Warning) << "Could not set AF_TRIGGER - no AF algorithm";
> > > +                             break;
> > > +                     }
> > > +
> > > +                     af->setPause(static_cast<controls::AfPauseEnum>(ctrlValue.get<int32_t>()));
> > > +                     break;
> > > +             }
> > > +             default:
> > > +                     LOG(IPARkISP1, Warning)
> > > +                             << "Ctrl " << controls::controls.at(ctrlEnum)->name()
> > > +                             << " is not handled.";
> > > +                     break;
> > > +             }
> > > +     }
> > >  }
> > >
> > >  void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)

-- 
Regards,

Laurent Pinchart