[v3,2/4] ipa: rkisp1: lux: Properly handle frame context and active state

25082 diff mbox series

WIth the upcoming regulation rework the processing order in the IPA is
updated a bit so that process() updates the active state with new
measurements and fills the metadata with the data from the corresponding
frame context. In prepare() all parameters for one frame are tied
together using the most up to date values from active state.

Change the lux algorithm to support that order of events. Also prepare
for cases where stats can be null which can happen with the upcoming
regulation rework.

While at it fix a formatting issue reported by checkstyle and drop a
unnecessary local variable.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v3:
- Added this patch in response to the comment from Kieran in
  https://patchwork.libcamera.org/patch/24982/#36740
---
 src/ipa/rkisp1/algorithms/lux.cpp | 23 ++++++++++++++++++-----
 src/ipa/rkisp1/algorithms/lux.h   |  3 +++
 src/ipa/rkisp1/ipa_context.h      |  4 ++++
 3 files changed, 25 insertions(+), 5 deletions(-)

Quoting Stefan Klug (2025-11-19 13:22:11)
> WIth the upcoming regulation rework the processing order in the IPA is

s/WIth/With/

> updated a bit so that process() updates the active state with new
> measurements and fills the metadata with the data from the corresponding
> frame context. In prepare() all parameters for one frame are tied
> together using the most up to date values from active state.
> 
> Change the lux algorithm to support that order of events. Also prepare
> for cases where stats can be null which can happen with the upcoming
> regulation rework.
> 
> While at it fix a formatting issue reported by checkstyle and drop a
> unnecessary local variable.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v3:
> - Added this patch in response to the comment from Kieran in
>   https://patchwork.libcamera.org/patch/24982/#36740
> ---
>  src/ipa/rkisp1/algorithms/lux.cpp | 23 ++++++++++++++++++-----
>  src/ipa/rkisp1/algorithms/lux.h   |  3 +++
>  src/ipa/rkisp1/ipa_context.h      |  4 ++++
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/lux.cpp b/src/ipa/rkisp1/algorithms/lux.cpp
> index e8da69810008..bc714b7a82c6 100644
> --- a/src/ipa/rkisp1/algorithms/lux.cpp
> +++ b/src/ipa/rkisp1/algorithms/lux.cpp
> @@ -46,6 +46,16 @@ int Lux::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
>         return lux_.parseTuningData(tuningData);
>  }
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::prepare
> + */
> +void Lux::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> +                 IPAFrameContext &frameContext,
> +                 [[maybe_unused]] RkISP1Params *params)
> +{
> +       frameContext.lux.lux = context.activeState.lux.lux;

I'm worried that this is predicting the physics of the future ;-)
But that's because my mental model of how all of this works is out of
date.

What I would do to get a slideshow presentation with animations of how
all the pieces move around for the IPA in general ;-)

Do we know any good animators?



> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::process
>   */
> @@ -55,8 +65,13 @@ void Lux::process(IPAContext &context,
>                   const rkisp1_stat_buffer *stats,
>                   ControlList &metadata)
>  {
> -       utils::Duration exposureTime = context.configuration.sensor.lineDuration
> -                                    * frameContext.sensor.exposure;
> +       metadata.set(controls::Lux, frameContext.lux.lux);

I think if this line stays here - we need a banner comment saying "This
is reporting the lux level that was used at the time of
preparing the algorithms. Otherwise brains will break in the time
dilation distortion....

Perhaps just:

	/*
	 * Report the lux level used by algorithms to prepare this frame
	 * not the lux level *of* this frame.
	 */

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
> +       if (!stats)
> +               return;
> +
> +       utils::Duration exposureTime = context.configuration.sensor.lineDuration *
> +                                      frameContext.sensor.exposure;
>         double gain = frameContext.sensor.gain;
>  
>         /* \todo Deduplicate the histogram calculation from AGC */
> @@ -64,9 +79,7 @@ void Lux::process(IPAContext &context,
>         Histogram yHist({ params->hist.hist_bins, context.hw.numHistogramBins },
>                         [](uint32_t x) { return x >> 4; });
>  
> -       double lux = lux_.estimateLux(exposureTime, gain, 1.0, yHist);
> -       frameContext.lux.lux = lux;
> -       metadata.set(controls::Lux, lux);
> +       context.activeState.lux.lux = lux_.estimateLux(exposureTime, gain, 1.0, yHist);
>  }
>  
>  REGISTER_IPA_ALGORITHM(Lux, "Lux")
> diff --git a/src/ipa/rkisp1/algorithms/lux.h b/src/ipa/rkisp1/algorithms/lux.h
> index 8dcadc284a84..e0239848e252 100644
> --- a/src/ipa/rkisp1/algorithms/lux.h
> +++ b/src/ipa/rkisp1/algorithms/lux.h
> @@ -23,6 +23,9 @@ public:
>         Lux();
>  
>         int init(IPAContext &context, const YamlObject &tuningData) override;
> +       void prepare(IPAContext &context, const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    RkISP1Params *params) override;
>         void process(IPAContext &context, const uint32_t frame,
>                      IPAFrameContext &frameContext,
>                      const rkisp1_stat_buffer *stats,
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index f85a130d9c23..b257cee55379 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -133,6 +133,10 @@ struct IPAActiveState {
>                 double gamma;
>         } goc;
>  
> +       struct {
> +               double lux;
> +       } lux;
> +
>         struct {
>                 controls::WdrModeEnum mode;
>                 AgcMeanLuminance::AgcConstraint constraint;
> -- 
> 2.51.0
>