[v1,04/35] libcamera: delayed_controls: Queue noop when needed, not before

24770 diff mbox series

A no-op is queued when DelayedControls runs out of controls after a
successful applyControls(). But after applyControls() there is still
some time left for new controls to be pushed until the next call to
applyControls(). To fix that, move the no-op push to the beginning of
applyContols().

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/delayed_controls.cpp | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

2025. 10. 24. 10:50 keltezéssel, Stefan Klug írta:
> A no-op is queued when DelayedControls runs out of controls after a
> successful applyControls(). But after applyControls() there is still
> some time left for new controls to be pushed until the next call to
> applyControls(). To fix that, move the no-op push to the beginning of
> applyContols().
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   src/libcamera/delayed_controls.cpp | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 044c6c7325e7..67213fe87d53 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -272,6 +272,14 @@ void DelayedControls::applyControls(uint32_t sequence)
>   {
>   	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> 
> +	while (queueCount_ - 1 < sequence) {
> +		LOG(DelayedControls, Warning)
> +			<< "Queue is empty, auto queue no-op." << queueCount_;

Missing space or some kind of separator in the log message.


> +		push(queueCount_, {});
> +	}
> +
> +	writeCount_ = sequence;

I must be missing something. The commit message says

   there is still some time left for new controls to be pushed until the next call

but the loop above does the same number of `push()` that the removed loop did.
So I don't understand what the commit message refers to, how could the user do
any "valid" `push()` that wasn't previously possible?


> +
>   	/*
>   	 * Create control list peeking ahead in the value queue to ensure
>   	 * values are set in time to satisfy the sensor delay.
> @@ -312,12 +320,6 @@ void DelayedControls::applyControls(uint32_t sequence)
> 
>   	writeCount_ = sequence + 1;
> 
> -	while (writeCount_ > queueCount_) {
> -		LOG(DelayedControls, Warning)
> -			<< "Queue is empty, auto queue no-op.";
> -		push(queueCount_, {});
> -	}
> -
>   	device_->setControls(&out);
>   }
> 
> --
> 2.48.1
>

Hi Barnabás,

Thank you for the review.

Quoting Barnabás Pőcze (2025-10-24 13:39:52)
> 2025. 10. 24. 10:50 keltezéssel, Stefan Klug írta:
> > A no-op is queued when DelayedControls runs out of controls after a
> > successful applyControls(). But after applyControls() there is still
> > some time left for new controls to be pushed until the next call to
> > applyControls(). To fix that, move the no-op push to the beginning of
> > applyContols().
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >   src/libcamera/delayed_controls.cpp | 14 ++++++++------
> >   1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> > index 044c6c7325e7..67213fe87d53 100644
> > --- a/src/libcamera/delayed_controls.cpp
> > +++ b/src/libcamera/delayed_controls.cpp
> > @@ -272,6 +272,14 @@ void DelayedControls::applyControls(uint32_t sequence)
> >   {
> >       LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> > 
> > +     while (queueCount_ - 1 < sequence) {
> > +             LOG(DelayedControls, Warning)
> > +                     << "Queue is empty, auto queue no-op." << queueCount_;
> 
> Missing space or some kind of separator in the log message.

Thanks, fixed for v2.

> 
> 
> > +             push(queueCount_, {});
> > +     }
> > +
> > +     writeCount_ = sequence;
> 
> I must be missing something. The commit message says
> 
>    there is still some time left for new controls to be pushed until the next call
> 
> but the loop above does the same number of `push()` that the removed loop did.
> So I don't understand what the commit message refers to, how could the user do
> any "valid" `push()` that wasn't previously possible?

As there are different sequence numbers at play here, (this gets
adressed in "libcamera: delayed_controls: Change semantics of sequence
numbers") I have a hard time explaining this precisely.

The old code basically does:
- Collect controls for sequence seq
- Check if there is data for seq+1, push no-op if not

The issue is that at the time of the check, the data for seq+1 is not
needed, but only on the next call to applyControls. So I turned the
logic upside down, so that it follows
- Check if all data for seq is available and push no-op if not
- Collect controls for sequence seq

So after applyingControls for sequence seq, there is still a timeslot to
push data for seq+1 before a no-op gets pushed.

Does that make sense?

Best regards,
Stefan

> 
> 
> > +
> >       /*
> >        * Create control list peeking ahead in the value queue to ensure
> >        * values are set in time to satisfy the sensor delay.
> > @@ -312,12 +320,6 @@ void DelayedControls::applyControls(uint32_t sequence)
> > 
> >       writeCount_ = sequence + 1;
> > 
> > -     while (writeCount_ > queueCount_) {
> > -             LOG(DelayedControls, Warning)
> > -                     << "Queue is empty, auto queue no-op.";
> > -             push(queueCount_, {});
> > -     }
> > -
> >       device_->setControls(&out);
> >   }
> > 
> > --
> > 2.48.1
> > 
>