[PATCH] pipeline: rkisp1: cproc: Fix default value handling

[Jacopo Mondi]

On Wed, Jun 05, 2024 at 08:48:25AM GMT, Stefan Klug wrote:
> Default control values where not applied to activeState. This had no negative
> side effects in first place, as the hardware defaults where used. The issue
> became visible, when only one of the controls was set at runtime. In that case
> the params for the other values where overwritten with 0 (reset value of
> activeState) resulting in a black image.
>
> While at it, only add the controls to the controls map if the algorithm is
> contained in the tuning file.
>
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>
> ---
>
> This patch depends on https://patchwork.libcamera.org/patch/20194/ which needs
> to be merged first.
>
>
>  src/ipa/rkisp1/algorithms/cproc.cpp | 59 +++++++++++++++++++++++++++--
>  src/ipa/rkisp1/algorithms/cproc.h   |  3 ++
>  src/ipa/rkisp1/rkisp1.cpp           |  3 --
>  3 files changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index 68bb8180..0f122504 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -33,6 +33,56 @@ namespace ipa::rkisp1::algorithms {
>
>  LOG_DEFINE_CATEGORY(RkISP1CProc)
>
> +static int convertBrightness(const float v)
> +{
> +	return std::clamp<int>(std::lround(v * 128), -128, 127);
> +}
> +
> +static int convertContrast(const float v)
> +{
> +	return std::clamp<int>(std::lround(v * 128), 0, 255);
> +}
> +
> +static int convertSaturation(const float v)
> +{
> +	return std::clamp<int>(std::lround(v * 128), 0, 255);
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::init
> + */
> +int ColorProcessing::init([[maybe_unused]] IPAContext &context,
> +			  [[maybe_unused]] const YamlObject &tuningData)
> +{
> +	context.ctrlMap[&controls::Brightness] = ControlInfo(-1.0f, 0.993f, 0.0f);
> +	context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, 1.0f);
> +	context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, 1.0f);
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Configure the Gamma given a configInfo
> + * \param[in] context The shared IPA context
> + * \param[in] configInfo The IPA configuration data
> + *
> + * \return 0
> + */
> +int ColorProcessing::configure([[maybe_unused]] IPAContext &context,
> +			       [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> +{
> +	auto &cproc = context.activeState.cproc;
> +
> +	cproc.brightness = convertBrightness(
> +		context.ctrlMap[&controls::Brightness].def().get<float>());
> +	cproc.contrast = convertContrast(
> +		context.ctrlMap[&controls::Contrast].def().get<float>());
> +	cproc.saturation = convertSaturation(
> +		context.ctrlMap[&controls::Saturation].def().get<float>());
> +
> +	return 0;
> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::queueRequest
>   */
> @@ -44,9 +94,12 @@ void ColorProcessing::queueRequest(IPAContext &context,
>  	auto &cproc = context.activeState.cproc;
>  	bool update = false;
>
> +	if (frame == 0)
> +		update = true;
> +
>  	const auto &brightness = controls.get(controls::Brightness);
>  	if (brightness) {
> -		int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127);
> +		int value = convertBrightness(*brightness);
>  		if (cproc.brightness != value) {
>  			cproc.brightness = value;
>  			update = true;
> @@ -57,7 +110,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
>
>  	const auto &contrast = controls.get(controls::Contrast);
>  	if (contrast) {
> -		int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255);
> +		int value = convertContrast(*contrast);
>  		if (cproc.contrast != value) {
>  			cproc.contrast = value;
>  			update = true;
> @@ -68,7 +121,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
>
>  	const auto saturation = controls.get(controls::Saturation);
>  	if (saturation) {
> -		int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255);
> +		int value = convertSaturation(*saturation);
>  		if (cproc.saturation != value) {
>  			cproc.saturation = value;
>  			update = true;
> diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h
> index bafba5cc..e50e7200 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.h
> +++ b/src/ipa/rkisp1/algorithms/cproc.h
> @@ -21,6 +21,9 @@ public:
>  	ColorProcessing() = default;
>  	~ColorProcessing() = default;
>
> +	int init(IPAContext &context, const YamlObject &tuningData) override;
> +	int configure(IPAContext &context,
> +		      const IPACameraSensorInfo &configInfo) override;
>  	void queueRequest(IPAContext &context, const uint32_t frame,
>  			  IPAFrameContext &frameContext,
>  			  const ControlList &controls) override;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 17474408..62d56a3a 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -109,9 +109,6 @@ const ControlInfoMap::Map rkisp1Controls{
>  	{ &controls::AeEnable, ControlInfo(false, true) },
>  	{ &controls::AwbEnable, ControlInfo(false, true) },
>  	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> -	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
> -	{ &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) },
> -	{ &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
>  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>  };
> --
> 2.43.0
>