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

[Kieran Bingham]

Hi Daniel,

Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)
> Hi Daniel
> 
> 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.

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.

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.


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.

--
Kieran



> > +
> > +                     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)
> > --
> > 2.34.1
> >