[libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests setting AF controls to the AF algorithm
Jacopo Mondi
jacopo at jmondi.org
Fri Jul 15 08:28:31 CEST 2022
Hello,
On Fri, Jul 15, 2022 at 03:38:50AM +0300, Laurent Pinchart wrote:
> 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 ? :-)
>
Oh that's nice. I have never been in love with the switch(controls)
pattern but I didn't want to throw too many thins at Daniel, as after
all this is what the RPi IPA does. If we can find something different,
I'm all for it.
> > 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
More information about the libcamera-devel
mailing list