[libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests setting AF controls to the AF algorithm
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 15 13:19:36 CEST 2022
On Fri, Jul 15, 2022 at 11:49:53AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-07-15 01:38:50)
> > 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 ? :-)
>
> Aha - Yes! I hadn't seen these patches yet.
>
> I'll head over and find those now. Merging that would help IPU3 already
> too I believe (and RKISP here of course).
I'll prioritize the series.
> > > 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.
>
> Yes, I think I can see workable solutions with that in mind.
And will do this on top if/when needed.
> > > 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.
>
> 'A really big list' terrifies me of ending up like the Android HAL layer
> request processing. I would want to avoid that too.
>
> > > 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