[v4,29/49] ipa: libipa: agc: Calculate vblank and frame duration sooner

27702 diff mbox series

Calculating the vblank and frame duration is problematic in `process()`
because at the moment it is calculated for an already finished frame
based on the new suggested exposure time.

Instead, move the calculation to `prepare()` where the frame's
exposure and gain are finalized.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp | 29 ++++++++++++++---------------
 src/ipa/libipa/agc.h   |  3 ++-
 2 files changed, 16 insertions(+), 16 deletions(-)

Hi Barnabás
   sorry, I let the discussion drop on the previous version

On Mon, Aug 10, 2026 at 12:38:25PM +0200, Barnabás Pőcze wrote:
> Calculating the vblank and frame duration is problematic in `process()`
> because at the moment it is calculated for an already finished frame
> based on the new suggested exposure time.
>
> Instead, move the calculation to `prepare()` where the frame's
> exposure and gain are finalized.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

As said in the review of the previous version, at least we compute
gain, exposure and frame duration at the same time.

To get back to your point:

--------------------------------------------------------------------------------
> indeed, if you move the vblank calculation to prepare() at least frame
> duration, exposure and gain will all come from the same state, but
> isn't it the previous one ?

I suppose you could say that, but for sensor parameters specifically,
there is not much else one can do. One cannot use the statistics from
frame X to configure the sensor parameters for frame X, so it has to
be some function of all the results from previous frames. Or maybe
I misunderstand what you mean?
--------------------------------------------------------------------------------

In order to calculate the desired exposure for frame X we would need
to 'look ahead' in the request queue by a number of frames equal to
the sensor delays. We don't do that at the moment. So my point was
about the fact that keeping the vblank calculation in process(), after
the call to calculateNewWV()) meant
that we're computing vblank according to the newest calculated
exposure, meaning that we first enlarge the blankings then, at the
next frame prepare() we set the exposure and gains.

Now, DelayedControls handles priorities at the v4l2 level, by writing
VBLANK before anything else, so we're safe from the point of view of
the kernel interface.

My concern is about the AGC algorithm limits and what vblank is used
to clamp the new calculated exposure.

However this might not be real issue, as setLimits() doesn't take
frame duration/vblank into account, but rather, we adjust vblank
according to the required exposure.

At the contrary, the current implementation seems to be problematic
because it might end up shrinking vblank too early.

In example
- frame X: exposure = 33,333msec
- frame X+1: exposure = 16,666msec

The current implementation does:

        prepare(X) {
                frameContext.exposure = 33,333;
        }

        process(X) {
                activeState.exposure = 16,666
                frameContext.vblank = 16,666 - height;
        }

        setControls(EXPOSURE) = 33,333
        setControls(VBLANK) = 16,666 - height

        prepare(X + 1) {
                frameContext.exposure = 16,666;
        }

        process(X + 1) {
                activeState.exposure = xx,xxx
                frameContext.vblank = xx,xxx - height;
        }

        setControls(EXPOSURE) = 16,666
        setControls(VBLANK) = xx,xxx - height

With your patch

        prepare(X) {
                frameContext.exposure = 33,333;
                frameContext.vblank = 33,333 - height;
        }

        process(X) {
                activeState.exposure = 16,666
        }

        setControls(EXPOSURE) = 33,333
        setControls(VBLANK) = 33,333 - height

        prepare(X + 1) {
                frameContext.exposure = 16,666;
                frameContext.vblank = 16,666 - height;
        }

        process(X + 1) {
                activeState.exposure = xx,xxx
        }

        setControls(EXPOSURE) = 16,666
        setControls(VBLANK) = 16,666 - height

Does this match your understanding ?

If that's the case, how does it work right now ? Is it DelayedControls
applying vblank with 1 frame of delay compared to exposure to
compensate for the sensors delays ?

In other words, I wonder why the current implementation was the way it
used to be...


> ---
>  src/ipa/libipa/agc.cpp | 29 ++++++++++++++---------------
>  src/ipa/libipa/agc.h   |  3 ++-
>  2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index def60a8570..8e141a3a53 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -504,7 +504,7 @@ void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &s
>  /**
>   * \brief Handle a \a prepare operation
>   */
> -void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)
> +void AgcAlgorithm::prepare(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext)
>  {
>  	uint32_t activeAutoExposure = state.automatic.exposure;
>  	double activeAutoGain = state.automatic.gain;
> @@ -535,6 +535,19 @@ void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameCont
>  	}
>
>  	frameContext.yTarget = state.automatic.yTarget;
> +
> +	/*
> +	 * Expand the target frame duration so that we do not run faster than
> +	 * the minimum frame duration when we have short exposures.
> +	 */
> +	const auto frameDuration = std::max<uint32_t>(
> +		frameContext.minFrameDuration / session.lineDuration,
> +		frameContext.exposure);
> +	frameContext.vblank = frameDuration - session.sensor.outputSize.height;
> +
> +	/* Update frame duration accounting for line length quantization. */
> +	frameContext.frameDuration =
> +		(session.sensor.outputSize.height + frameContext.vblank) * session.lineDuration;
>  }
>
>  /**
> @@ -545,7 +558,6 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  			   ControlList &metadata)
>  {
>  	const utils::Duration &lineDuration = session.lineDuration;
> -	utils::Duration newExposureTime = {};
>
>  	if (params) {
>  		ASSERT(session.autoAllowed);
> @@ -605,8 +617,6 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  		state.automatic.digitalGain = newEv.digitalGain;
>  		state.automatic.yTarget = newEv.yTarget;
>
> -		newExposureTime = newEv.exposureTime;
> -
>  		LOG(Agc, Debug)
>  			<< "exposure-time:" << utils::Duration(state.automatic.exposure * lineDuration)
>  			<< " analogue-gain:" << state.automatic.gain
> @@ -614,17 +624,6 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  			<< " digital-gain:" << state.automatic.digitalGain;
>  	}
>
> -	/*
> -	 * Expand the target frame duration so that we do not run faster than
> -	 * the minimum frame duration when we have short exposures.
> -	 */
> -	const auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);
> -	frameContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;
> -
> -	/* Update frame duration accounting for line length quantization. */
> -	frameContext.frameDuration =
> -		(session.sensor.outputSize.height + frameContext.vblank) * lineDuration;
> -
>  	metadata.set(controls::AnalogueGain, frameContext.gain);
>  	metadata.set(controls::ExposureTime,
>  		     utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> index 728f25911c..d02dbff562 100644
> --- a/src/ipa/libipa/agc.h
> +++ b/src/ipa/libipa/agc.h
> @@ -129,7 +129,8 @@ public:
>  	void queueRequest(const agc::Session &session, agc::ActiveState &state,
>  			  agc::FrameContext &frameContext, const ControlList &controls);
>
> -	void prepare(agc::ActiveState &state, agc::FrameContext &frameContext);
> +	void prepare(const agc::Session &session, agc::ActiveState &state,
> +		     agc::FrameContext &frameContext);
>
>  	struct ProcessParams {
>  		const AgcMeanLuminance::Traits &traits;
> --
> 2.55.0
>