[libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests setting AF controls to the AF algorithm
Daniel Semkowicz
dse at thaumatec.com
Mon Jul 18 17:45:53 CEST 2022
Hi,
On Fri, Jul 15, 2022 at 1:20 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.
I really like the idea of passing controls to each algorithm! It would
be great if at the end We could control the algorithms, by just enabling
them in the tuning file, without additional changes in the source code.
Waiting for these changes to be merged!
Best regards
Daniel
>
> > > > 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