[libcamera-devel] [PATCH v4 21/32] ipa: rkisp1: cproc: Store per-frame information in frame context

[Jacopo Mondi]

Hi Laurent,

On Thu, Sep 08, 2022 at 04:41:49AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Rework the algorithm's usage of the active state, to store the value of
> controls for the last queued request in the queueRequest() function, and
> store a copy of the values in the corresponding frame context. The
> latter is used in the prepare() function to populate the ISP parameters
> with values corresponding to the right frame.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

This one is easier indeed

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> ---
>  src/ipa/rkisp1/algorithms/cproc.cpp | 51 ++++++++++++++++++-----------
>  src/ipa/rkisp1/ipa_context.cpp      | 21 ++++++++++--
>  src/ipa/rkisp1/ipa_context.h        |  8 ++++-
>  3 files changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index 22a70e0b70c7..9c442cd56b3f 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -38,55 +38,66 @@ LOG_DEFINE_CATEGORY(RkISP1CProc)
>   */
>  void ColorProcessing::queueRequest(IPAContext &context,
>  				   [[maybe_unused]] const uint32_t frame,
> -				   [[maybe_unused]] RkISP1FrameContext &frameContext,
> +				   RkISP1FrameContext &frameContext,
>  				   const ControlList &controls)
>  {
>  	auto &cproc = context.activeState.cproc;
> +	bool update = false;
>
>  	const auto &brightness = controls.get(controls::Brightness);
>  	if (brightness) {
> -		cproc.brightness = std::clamp<int>(std::lround(*brightness * 128), -128, 127);
> -		cproc.updateParams = true;
> +		int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127);
> +		if (cproc.brightness != value) {
> +			cproc.brightness = value;
> +			update = true;
> +		}
>
> -		LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness;
> +		LOG(RkISP1CProc, Debug) << "Set brightness to " << value;
>  	}
>
>  	const auto &contrast = controls.get(controls::Contrast);
>  	if (contrast) {
> -		cproc.contrast = std::clamp<int>(std::lround(*contrast * 128), 0, 255);
> -		cproc.updateParams = true;
> +		int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255);
> +		if (cproc.contrast != value) {
> +			cproc.contrast = value;
> +			update = true;
> +		}
>
> -		LOG(RkISP1CProc, Debug) << "Set contrast to " << *contrast;
> +		LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
>  	}
>
>  	const auto saturation = controls.get(controls::Saturation);
>  	if (saturation) {
> -		cproc.saturation = std::clamp<int>(std::lround(*saturation * 128), 0, 255);
> -		cproc.updateParams = true;
> +		int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255);
> +		if (cproc.saturation != value) {
> +			cproc.saturation = value;
> +			update = true;
> +		}
>
> -		LOG(RkISP1CProc, Debug) << "Set saturation to " << *saturation;
> +		LOG(RkISP1CProc, Debug) << "Set saturation to " << value;
>  	}
> +
> +	frameContext.cproc.brightness = cproc.brightness;
> +	frameContext.cproc.contrast = cproc.contrast;
> +	frameContext.cproc.saturation = cproc.saturation;
> +	frameContext.cproc.update = update;
>  }
>
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void ColorProcessing::prepare(IPAContext &context,
> +void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>  			      [[maybe_unused]] const uint32_t frame,
> -			      [[maybe_unused]] RkISP1FrameContext &frameContext,
> +			      RkISP1FrameContext &frameContext,
>  			      rkisp1_params_cfg *params)
>  {
> -	auto &cproc = context.activeState.cproc;
> -
>  	/* Check if the algorithm configuration has been updated. */
> -	if (!cproc.updateParams)
> +	if (!frameContext.cproc.update)
>  		return;
>
> -	cproc.updateParams = false;
> -
> -	params->others.cproc_config.brightness = cproc.brightness;
> -	params->others.cproc_config.contrast = cproc.contrast;
> -	params->others.cproc_config.sat = cproc.saturation;
> +	params->others.cproc_config.brightness = frameContext.cproc.brightness;
> +	params->others.cproc_config.contrast = frameContext.cproc.contrast;
> +	params->others.cproc_config.sat = frameContext.cproc.saturation;
>
>  	params->module_en_update |= RKISP1_CIF_ISP_MODULE_CPROC;
>  	params->module_ens |= RKISP1_CIF_ISP_MODULE_CPROC;
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index cd1443e1ed46..936b77709417 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -160,9 +160,6 @@ namespace libcamera::ipa::rkisp1 {
>   *
>   * \var IPAActiveState::cproc.saturation
>   * \brief Saturation level
> - *
> - * \var IPAActiveState::cproc.updateParams
> - * \brief Indicates if ISP parameters need to be updated
>   */
>
>  /**
> @@ -236,6 +233,24 @@ namespace libcamera::ipa::rkisp1 {
>   * \brief Whether the Auto White Balance algorithm is enabled
>   */
>
> +/**
> + * \var RkISP1FrameContext::cproc
> + * \brief Color Processing parameters for this frame
> + *
> + * \struct RkISP1FrameContext::cproc.brightness
> + * \brief Brightness level
> + *
> + * \var RkISP1FrameContext::cproc.contrast
> + * \brief Contrast level
> + *
> + * \var RkISP1FrameContext::cproc.saturation
> + * \brief Saturation level
> + *
> + * \var RkISP1FrameContext::cproc.update
> + * \brief Indicates if the color processing parameters have been updated
> + * compared to the previous frame
> + */
> +
>  /**
>   * \var RkISP1FrameContext::sensor
>   * \brief Sensor configuration that used been used for this frame
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index d97aae9a97b4..78edb607d039 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -75,7 +75,6 @@ struct IPAActiveState {
>  		int8_t brightness;
>  		uint8_t contrast;
>  		uint8_t saturation;
> -		bool updateParams;
>  	} cproc;
>
>  	struct {
> @@ -107,6 +106,13 @@ struct RkISP1FrameContext : public FrameContext {
>  		bool autoEnabled;
>  	} awb;
>
> +	struct {
> +		int8_t brightness;
> +		uint8_t contrast;
> +		uint8_t saturation;
> +		bool update;
> +	} cproc;
> +
>  	struct {
>  		uint32_t exposure;
>  		double gain;
> --
> Regards,
>
> Laurent Pinchart
>