[libcamera-devel] [PATCH 10/11] ipa: ipu3: tonemapping: Generate the LUT only on gamma change

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Sep 13, 2021 at 04:58:09PM +0200, Jean-Michel Hautbois wrote:
> The Tonemapping algorithm calculates the gamma curve for every frame,

s/Tonemapping/tone mapping/

> regardless of whether the gamma value has changed or not. This issue is
> exasperated as we currently hardcode the gamma to a single value.
> 
> Optimise the implementation to only recalculate the look up table when
> the gamma setting is changed, and store the gamma setting of the LUT
> curve as part of the IPA context.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/tone_mapping.cpp | 5 +++++
>  src/ipa/ipu3/ipa_context.h               | 1 +
>  src/ipa/ipu3/ipu3.cpp                    | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> index 3af96261..40337f9d 100644
> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> @@ -43,6 +43,9 @@ void ToneMapping::process([[maybe_unused]] IPAContext &context,
>  	 */
>  	gamma_ = 1.1;
>  
> +	if (context.frameContext.toneMapping.gamma == gamma_)
> +		return;

We currently clear the context in IPAIPU3::configure() with

	context_ = {};

This ensures that the check above will never be true for the first
frame, but it feels like it would probably be best to let algorithms
perform the initial configuration. Should we implement
ToneMapping::configure() and set context.frameContext.toneMapping.gamma
to 0.0 there ?

> +
>  	struct ipu3_uapi_gamma_corr_lut &lut =
>  		context.frameContext.toneMapping.gammaCorrection;
>  
> @@ -53,6 +56,8 @@ void ToneMapping::process([[maybe_unused]] IPAContext &context,
>  		/* The output value is expressed on 13 bits. */
>  		lut.lut[i] = gamma * 8191;
>  	}
> +
> +	context.frameContext.toneMapping.gamma = gamma_;
>  }
>  
>  } /* namespace ipa::ipu3::algorithms */
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 9d9444dc..bffa7634 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -38,6 +38,7 @@ struct IPAFrameContext {
>  	} awb;
>  
>  	struct {
> +		double gamma;
>  		struct ipu3_uapi_gamma_corr_lut gammaCorrection;
>  	} toneMapping;
>  };
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 5b487dcd..f76627f6 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -129,6 +129,9 @@
>   * \struct IPAFrameContext::toneMapping
>   * \brief Context for ToneMapping and Gamma control
>   *
> + * \var IPAFrameContext::toneMapping::gamma
> + * \brief Gamma value for the LUT
> + *
>   * \var IPAFrameContext::toneMapping::gammaCorrection
>   * \brief Per-pixel tone mapping implemented as a LUT
>   *

-- 
Regards,

Laurent Pinchart