[v1,18/35] libcamera: delayed_controls: Change semantics of sequence numbers

24784 diff mbox series

In the context of per frame controls I believe it is easier to think
about what needs to be done for a specific sequence number.

So (assuming a max sensor delay of 2) the actions would be:

- delayedControls.push(n) stores the controls that shall be active on
  frame n
- delayedControls.get(n) returns these controls again
- delayedControls.apply(n) applies the slowest control for frame n and
  does a look back on other controls. So when a frameStart for frame n
occurs, it is time to call delayedControls.apply(n + maxDelay)

Changing these semantics on delayed controls doesn't require much code
change and has the added benefit that we don't run in to clamping for
get() on frames < maxDelay.

ToDo: This breaks the delayed controls test at the moment. The tests
need to be fixed and other pipelines need to be adjusted accordingly.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/delayed_controls.cpp       | 15 ++++++++-------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  5 +++--
 test/meson.build                         |  2 +-
 3 files changed, 12 insertions(+), 10 deletions(-)

Hi Stefan

On Fri, Oct 24, 2025 at 10:50:42AM +0200, Stefan Klug wrote:
> In the context of per frame controls I believe it is easier to think
> about what needs to be done for a specific sequence number.
>
> So (assuming a max sensor delay of 2) the actions would be:
>
> - delayedControls.push(n) stores the controls that shall be active on
>   frame n
> - delayedControls.get(n) returns these controls again
> - delayedControls.apply(n) applies the slowest control for frame n and
>   does a look back on other controls. So when a frameStart for frame n
> occurs, it is time to call delayedControls.apply(n + maxDelay)

I see this being used as

void PipelineHandlerRkISP1::frameStart(uint32_t sequence)

        ...

	uint32_t sequenceToApply = sequence + data->delayedCtrls_->maxDelay();
	data->delayedCtrls_->applyControls(sequenceToApply);

and you suggest this as a call pattern.

Is this me or this is exactly what was happening before without the
trouble of having to query the maxDelay() everytime from the caller
side ? Or, in other words, do you expect applyControls to be called
with anything but 'sequence + maxDelay()' ?

>
> Changing these semantics on delayed controls doesn't require much code
> change and has the added benefit that we don't run in to clamping for
> get() on frames < maxDelay.
>
> ToDo: This breaks the delayed controls test at the moment. The tests
> need to be fixed and other pipelines need to be adjusted accordingly.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/libcamera/delayed_controls.cpp       | 15 ++++++++-------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  5 +++--
>  test/meson.build                         |  2 +-
>  3 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 35a624525b80..8239f3dcf347 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -241,7 +241,7 @@ bool DelayedControls::push(uint32_t sequence, const ControlList &controls)
>   */
>  ControlList DelayedControls::get(uint32_t sequence)
>  {
> -	unsigned int index = std::max<int>(0, sequence - maxDelay_);
> +	unsigned int index = sequence;
>
>  	ControlList out(device_->controls());
>  	for (const auto &ctrl : values_) {
> @@ -267,13 +267,14 @@ ControlList DelayedControls::get(uint32_t sequence)
>   */
>
>  /**
> - * \brief Inform DelayedControls of the start of a new frame
> - * \param[in] sequence Sequence number of the frame that started
> + * \brief Apply controls for a frame
> + * \param[in] sequence Sequence number of the frame to apply
>   *
> - * Inform the state machine that a new frame has started and of its sequence
> - * number. Any user of these helpers is responsible to inform the helper about
> - * the start of any frame. This can be connected with ease to the start of a
> - * exposure (SOE) V4L2 event.
> + * Apply controls for the frame \a sequence. This applies the controls with the
> + * largest delay. For controls with a smaller delay it does a looks back and
> + * applies the controls for the previous sequence. So usually this function is
> + * called in a start of exposure event as applyControls(startedSequence +
> + * maxDelay)
>   */
>  void DelayedControls::applyControls(uint32_t sequence)
>  {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index d937c94e351f..7a4957d7e535 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1624,10 +1624,11 @@ void PipelineHandlerRkISP1::frameStart(uint32_t sequence)
>  		return;
>
>  	RkISP1CameraData *data = cameraData(activeCamera_);
> -	data->delayedCtrls_->applyControls(sequence);
> +	uint32_t sequenceToApply = sequence + data->delayedCtrls_->maxDelay();
> +	data->delayedCtrls_->applyControls(sequenceToApply);
>
>  	if (isRaw_) {
> -		data->ipa_->computeParams(sequence + 1, 0);
> +		data->ipa_->computeParams(sequenceToApply + 1, 0);
>  	}
>  }
>
> diff --git a/test/meson.build b/test/meson.build
> index 52f04364e4fc..80fb543f2201 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -53,7 +53,7 @@ internal_tests = [
>      {'name': 'bayer-format', 'sources': ['bayer-format.cpp']},
>      {'name': 'byte-stream-buffer', 'sources': ['byte-stream-buffer.cpp']},
>      {'name': 'camera-sensor', 'sources': ['camera-sensor.cpp']},
> -    {'name': 'delayed_controls', 'sources': ['delayed_controls.cpp']},
> +    {'name': 'delayed_controls', 'sources': ['delayed_controls.cpp'], 'should_fail': true},
>      {'name': 'event', 'sources': ['event.cpp']},
>      {'name': 'event-dispatcher', 'sources': ['event-dispatcher.cpp']},
>      {'name': 'event-thread', 'sources': ['event-thread.cpp']},
> --
> 2.48.1
>