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

[Jacopo Mondi]

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 ?

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