[RFC,v3,15/19] ipa: libipa: agc: Rework frame duration limit calculation

28522 diff mbox series

The frame duration is calculated as `frameHeight * lineDuration`, but
it was effectively repeating the line duration calculation, but with
microsecond precision. Instead, simply reuse the already calculated
line duration, and use the already calculated min/max values in
the provided in `IPACameraSensorInfo`.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

Hi Barnabás

On Mon, Oct 19, 2026 at 08:45:22PM +0200, Barnabás Pőcze wrote:
> The frame duration is calculated as `frameHeight * lineDuration`, but
> it was effectively repeating the line duration calculation, but with
> microsecond precision. Instead, simply reuse the already calculated
> line duration, and use the already calculated min/max values in
> the provided in `IPACameraSensorInfo`.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

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

Thanks
  j

> ---
>  src/ipa/libipa/agc.cpp | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 7fbaeefb3d..ac81e0f601 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -388,25 +388,6 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  		<< "line-duration: " << session.lineDuration << ", "
>  		<< "sensor-output: " << session.sensor.outputSize;
>
> -	/*
> -	 * Compute the frame duration limits.
> -	 *
> -	 * The frame length is computed assuming a fixed line length combined
> -	 * with the vertical frame sizes.
> -	 */
> -
> -	const ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;
> -	std::array<uint32_t, 3> frameHeights{
> -		v4l2VBlank.min().get<int32_t>() + config.sensorInfo.outputSize.height,
> -		v4l2VBlank.max().get<int32_t>() + config.sensorInfo.outputSize.height,
> -		v4l2VBlank.def().get<int32_t>() + config.sensorInfo.outputSize.height,
> -	};
> -
> -	std::array<int64_t, 3> frameDurations;
> -	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> -		uint64_t frameSize = static_cast<uint64_t>(lineLength) * frameHeights[i];
> -		frameDurations[i] = frameSize * 1000000U / config.sensorInfo.pixelRate;
> -	}
>
>  	/*
>  	 * When the AGC computes the new exposure values for a frame, it needs
> @@ -420,8 +401,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	session.minAnalogueGain = minGain;
>  	session.maxAnalogueGain = maxGain;
>  	session.defAnalogueGain = defGain;
> -	session.minFrameDuration = std::chrono::microseconds(frameDurations[0]);
> -	session.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);
> +	session.minFrameDuration = config.sensorInfo.minFrameLength * session.lineDuration;
> +	session.maxFrameDuration = config.sensorInfo.maxFrameLength * session.lineDuration;
>
>  	/* Configure the default exposure and gain. */
>  	state = {};
> @@ -460,8 +441,12 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  		static_cast<int32_t>(defExposure * lineDurationUs),
>  	};
>  	config.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{
> -		frameDurations[0], frameDurations[1],
> -		std::span<const int64_t, 2>{ { frameDurations[0], frameDurations[1] } },
> +		static_cast<int64_t>(session.minFrameDuration.get<std::micro>()),
> +		static_cast<int64_t>(session.maxFrameDuration.get<std::micro>()),
> +		std::span<const int64_t, 2>{ {
> +			static_cast<int64_t>(state.minFrameDuration.get<std::micro>()),
> +			static_cast<int64_t>(state.maxFrameDuration.get<std::micro>()),
> +		} },
>  	};
>
>  	const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {
> --
> 2.56.0
>

Hello François,

I tested v2 on a Dell Latitude 7340 (Debian 13, kernel 7.1.13+deb13-amd64,
libcamera 0.7.1, Intel IPU6 + IVSC, sensor "ov02c10 15-0036", SGRBG10_1X10,
1928x1092) with the simple pipeline and the software ISP. Your values match
what this machine reports.

Without the helper:

   IPASoft: Exposure 4-2320, gain 16-248 (1)
   IPASoft: Failed to create camera sensor helper for ov02c10

and the sensor stays at the driver's power-on defaults: exposure pinned at
its maximum (2320) and analogue_gain at its minimum (16), so AGC never
raises gain and the image sits at ~30/255 mean luminance with a green cast
from the un-corrected black level.

With the helper applied:

   IPASoft: Exposure 4-2320, gain 1-15.5 (0.145)

gain now climbs while streaming (measured 16 -> 163 -> 215 -> 248 as the
scene demands), mean luminance goes to 133-150/255 and the cast is gone.
Black level 4096 matches this sensor too.

One remark from the same debugging session, in case it is worth a warning
somewhere: a camera can end up with a hand-written
/usr/share/libcamera/ipa/simple/<sensor>.yaml that lists only
"BlackLevel:" and "Awb:". Such a file replaces the uncalibrated.yaml
fallback, so Agc and Adjust are never instantiated, and the image stays dark
even with the sensor helper present — the log gives no hint at all. A
warning when the tuning file instantiates no exposure algorithm (or a
mention in the documentation) would have saved me an hour of suspecting the
driver.

Thanks for the patch,
myke
ps : sorry if there is some misspeling, using partialy a translator...

Is it me only or this reply ended up in the wrong thread ?

Checking the header
In-Reply-To: <ateI9wz4VLEXQ4cZ@zed>

it seems it's intentional.

Myke, have you manually mangled the email header or something got
wrong somewhere ?

On Tue, Oct 20, 2026 at 05:36:26PM +0200, myke bourgeoisat wrote:
> Hello François,
>
> I tested v2 on a Dell Latitude 7340 (Debian 13, kernel 7.1.13+deb13-amd64,
> libcamera 0.7.1, Intel IPU6 + IVSC, sensor "ov02c10 15-0036", SGRBG10_1X10,
> 1928x1092) with the simple pipeline and the software ISP. Your values match
> what this machine reports.
>
> Without the helper:
>
>   IPASoft: Exposure 4-2320, gain 16-248 (1)
>   IPASoft: Failed to create camera sensor helper for ov02c10
>
> and the sensor stays at the driver's power-on defaults: exposure pinned at
> its maximum (2320) and analogue_gain at its minimum (16), so AGC never
> raises gain and the image sits at ~30/255 mean luminance with a green cast
> from the un-corrected black level.
>
> With the helper applied:
>
>   IPASoft: Exposure 4-2320, gain 1-15.5 (0.145)
>
> gain now climbs while streaming (measured 16 -> 163 -> 215 -> 248 as the
> scene demands), mean luminance goes to 133-150/255 and the cast is gone.
> Black level 4096 matches this sensor too.
>
> One remark from the same debugging session, in case it is worth a warning
> somewhere: a camera can end up with a hand-written
> /usr/share/libcamera/ipa/simple/<sensor>.yaml that lists only
> "BlackLevel:" and "Awb:". Such a file replaces the uncalibrated.yaml
> fallback, so Agc and Adjust are never instantiated, and the image stays dark
> even with the sensor helper present — the log gives no hint at all. A
> warning when the tuning file instantiates no exposure algorithm (or a
> mention in the documentation) would have saved me an hour of suspecting the
> driver.
>
> Thanks for the patch,
> myke
> ps : sorry if there is some misspeling, using partialy a translator...

Yes it's possible, sorry, first post on this mailing list ( haven't use 
that for decades...) 
  
  
  
  
  
  
  
  
  
  Is it me only or this reply ended up in the wrong thread ? 
  
  
  
  
  Checking the header 
  
  In-Reply-To: <ateI9wz4VLEXQ4cZ@zed> 
  
  
  
  
  it seems it's intentional. 
  
  
  
  
  Myke, have you manually mangled the email header or something got 
  
  wrong somewhere ? 
  
  
  
  
  On Tue, Oct 20, 2026 at 05:36:26PM +0200, myke bourgeoisat wrote: