| Message ID | 20220713084317.24268-7-dse@thaumatec.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > + > + 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 >
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@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 > >
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@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)
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@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
Quoting Laurent Pinchart (2022-07-15 01:38:50) > 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@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). > > 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. > > 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. -- Kieran > > 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
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@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)
Hi, On Fri, Jul 15, 2022 at 1:20 PM Laurent Pinchart <laurent.pinchart@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@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
Hi Daniel, On Mon, Jul 18, 2022 at 05:45:53PM +0200, Daniel Semkowicz wrote: > On Fri, Jul 15, 2022 at 1:20 PM Laurent Pinchart 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@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! Done :-) I've pushed the two patches that add the Algorithm::queueRequest() function and call it from the top level of the IPA module. Could you give it a try to see if it solves your problem ? > > > > > 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)
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) { + unsigned int ctrlEnum = ctrl.first; + const ControlValue &ctrlValue = ctrl.second; + + 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; + } + + 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)
Pass the controls set by top level API to the AF algorithm if it was enabled. Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> --- src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-)