[v1,25/35] pipeline: rkisp1: Add SequenceSyncHelper class

24791 diff mbox series

On a V4L2 buffer the assigned sequence is not known until the buffer is
dequeued. But for per frame controls we have to prepare other data like
sensor controls and ISP params in advance. So we try to anticipate the
sequence number a given buffer will be. In a perfect world this works
well as long as the initial sequence is assigned correctly. But it
breaks as soon as things like running out of buffers or incomplete
images happen. To make things even more complicated, in most cases
more than one buffer is queued to the kernel at a time.

So as soon as a sequence number doesn't match the expected one after
dequeuing, most likely all the already queued buffers will be dequeued
with the same error.  It is not sufficient to simply add the correction
after dequeuing because the error on all queued frames would accumulate
and the whole system starts to oscillate. To work around that add a
SequenceSyncHelper class that tracks the expected error and allows to
easily query the necessary correction when queuing new buffers.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +
 .../pipeline/rkisp1/sequence_sync_helper.h    | 69 +++++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 src/libcamera/pipeline/rkisp1/sequence_sync_helper.h

Hi

2025. 10. 24. 10:50 keltezéssel, Stefan Klug írta:
> On a V4L2 buffer the assigned sequence is not known until the buffer is
> dequeued. But for per frame controls we have to prepare other data like
> sensor controls and ISP params in advance. So we try to anticipate the
> sequence number a given buffer will be. In a perfect world this works
> well as long as the initial sequence is assigned correctly. But it
> breaks as soon as things like running out of buffers or incomplete
> images happen. To make things even more complicated, in most cases
> more than one buffer is queued to the kernel at a time.
> 
> So as soon as a sequence number doesn't match the expected one after
> dequeuing, most likely all the already queued buffers will be dequeued
> with the same error.  It is not sufficient to simply add the correction
> after dequeuing because the error on all queued frames would accumulate
> and the whole system starts to oscillate. To work around that add a
> SequenceSyncHelper class that tracks the expected error and allows to
> easily query the necessary correction when queuing new buffers.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +
>   .../pipeline/rkisp1/sequence_sync_helper.h    | 69 +++++++++++++++++++
>   2 files changed, 71 insertions(+)
>   create mode 100644 src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7a4957d7e535..d83f7d787892 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -51,10 +51,12 @@
>   #include "libcamera/internal/yaml_parser.h"
> 
>   #include "rkisp1_path.h"
> +#include "sequence_sync_helper.h"
> 
>   namespace libcamera {
> 
>   LOG_DEFINE_CATEGORY(RkISP1)
> +LOG_DEFINE_CATEGORY(RkISP1Schedule)
> 
>   class PipelineHandlerRkISP1;
>   class RkISP1CameraData;
> diff --git a/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
> new file mode 100644
> index 000000000000..c3f91dbed45f
> --- /dev/null
> +++ b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas on Board
> + *
> + * Sequence sync helper
> + */
> +
> +#pragma once
> +
> +#include <queue>
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {

This looks like a non-rkisp1 specific class. In that case it could
be moved to a common area. But if it stays here, I would expect it
to the in the `libcamera::ipa::rkisp1` namespace.

> +
> +LOG_DECLARE_CATEGORY(RkISP1Schedule)
> +
> +class SequenceSyncHelper
> +{
> +public:
> +	int gotFrame(size_t expectedSequence, size_t actualSequence)
> +	{
> +		ASSERT(!corrections_.empty());
> +		int diff = actualSequence - expectedSequence;

Can this be negative? When?


> +		int corr = corrections_.front();
> +		corrections_.pop();
> +		expectedOffset_ -= corr;
> +		int necessaryCorrection = diff - expectedOffset_;
> +		correctionToApply_ += necessaryCorrection;
> +
> +		LOG(RkISP1Schedule, Debug) << "Sync frame "
> +					   << "expected: " << expectedSequence
> +					   << " actual: " << actualSequence
> +					   << " correction: " << corr
> +					   << " expectedOffset: " << expectedOffset_
> +					   << " correctionToApply " << correctionToApply_;
> +
> +		expectedOffset_ += necessaryCorrection;
> +		return necessaryCorrection;
> +	}
> +
> +	/* Value to be added to the source sequence */
> +	int correction()

   ...() const


> +	{
> +		return correctionToApply_;
> +	}
> +
> +	void pushCorrection(int correction)
> +	{
> +		corrections_.push(correction);
> +		correctionToApply_ -= correction;
> +		LOG(RkISP1Schedule, Debug)
> +			<< "Push correction " << correction
> +			<< " correctionToApply " << correctionToApply_;
> +	}
> +
> +	void reset()
> +	{
> +		corrections_ = {};
> +		correctionToApply_ = 0;
> +		expectedOffset_ = 0;
> +	}
> +
> +	std::queue<int> corrections_;
> +	int correctionToApply_ = 0;
> +	int expectedOffset_ = 0;

I suppose these could (should?) be private?

Also, did you plan on adding a bit more documentation?
For example, as far as I can tell the general mode of operation is:

   * (query correction and) apply correction
   * pushCorrection() the applied correction
   * ...
   * gotFrame() when the corresponding frame arrives

So `pushCorrection()` and `gotFrame()` calls should be strictly paired
as far as I can see.

I think some explanation about the algorithm would also be appreciated.

As far as I understand `correctionToApply_` is the cumulative correction to
be applied to "new" frames. And generally it is expected that `pushCorrection()`
will set `correctionToApply_` to 0 because it is just called with whatever
`correction()` returns.

And `expectedOffset_` stores the expected difference for the "in-progress" frames.
So for example if you queue two frames in succession (e.g. with correction of 0),
and the first one runs into a frame skip, then `expectedOffset_` is there to account
for that for when the second frame arrives.

So for a particular frame after `expectedOffset_ -= corr`, `expectedOffset_` is number
of skipped frames expected from earlier frames. If that is different from `diff`, then
the current frame also "has" skipped frames: `necessaryCorrection`.

Is this any close to what this is supposed to be doing?


Regards,
Barnabás Pőcze


> +};
> +
> +} /* namespace libcamera */
> --
> 2.48.1
>

Hi Stefan,

   let me start by the smallest pieces in this series

On Fri, Oct 24, 2025 at 10:50:49AM +0200, Stefan Klug wrote:
> On a V4L2 buffer the assigned sequence is not known until the buffer is
> dequeued. But for per frame controls we have to prepare other data like
> sensor controls and ISP params in advance. So we try to anticipate the
> sequence number a given buffer will be. In a perfect world this works
> well as long as the initial sequence is assigned correctly. But it
> breaks as soon as things like running out of buffers or incomplete
> images happen. To make things even more complicated, in most cases
> more than one buffer is queued to the kernel at a time.
>
> So as soon as a sequence number doesn't match the expected one after
> dequeuing, most likely all the already queued buffers will be dequeued
> with the same error.  It is not sufficient to simply add the correction
> after dequeuing because the error on all queued frames would accumulate
> and the whole system starts to oscillate. To work around that add a

Uh, I didn't get this part...

If I'm expecting frame number X and I get frame X + 2, isn't enough to
add 2 to the expect frame counter and re-align things ?

> SequenceSyncHelper class that tracks the expected error and allows to
> easily query the necessary correction when queuing new buffers.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +
>  .../pipeline/rkisp1/sequence_sync_helper.h    | 69 +++++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7a4957d7e535..d83f7d787892 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -51,10 +51,12 @@
>  #include "libcamera/internal/yaml_parser.h"
>
>  #include "rkisp1_path.h"
> +#include "sequence_sync_helper.h"
>
>  namespace libcamera {
>
>  LOG_DEFINE_CATEGORY(RkISP1)
> +LOG_DEFINE_CATEGORY(RkISP1Schedule)
>
>  class PipelineHandlerRkISP1;
>  class RkISP1CameraData;
> diff --git a/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
> new file mode 100644
> index 000000000000..c3f91dbed45f
> --- /dev/null
> +++ b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas on Board
> + *
> + * Sequence sync helper
> + */
> +
> +#pragma once
> +
> +#include <queue>
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(RkISP1Schedule)
> +
> +class SequenceSyncHelper
> +{
> +public:
> +	int gotFrame(size_t expectedSequence, size_t actualSequence)
> +	{
> +		ASSERT(!corrections_.empty());
> +		int diff = actualSequence - expectedSequence;
> +		int corr = corrections_.front();
> +		corrections_.pop();
> +		expectedOffset_ -= corr;
> +		int necessaryCorrection = diff - expectedOffset_;
> +		correctionToApply_ += necessaryCorrection;
> +
> +		LOG(RkISP1Schedule, Debug) << "Sync frame "
> +					   << "expected: " << expectedSequence
> +					   << " actual: " << actualSequence
> +					   << " correction: " << corr
> +					   << " expectedOffset: " << expectedOffset_
> +					   << " correctionToApply " << correctionToApply_;
> +
> +		expectedOffset_ += necessaryCorrection;
> +		return necessaryCorrection;
> +	}
> +
> +	/* Value to be added to the source sequence */
> +	int correction()
> +	{
> +		return correctionToApply_;
> +	}
> +
> +	void pushCorrection(int correction)
> +	{
> +		corrections_.push(correction);
> +		correctionToApply_ -= correction;
> +		LOG(RkISP1Schedule, Debug)
> +			<< "Push correction " << correction
> +			<< " correctionToApply " << correctionToApply_;
> +	}

I think you need to explain how this class should be used.

I'm looking at paramSyncHelper in rkisp1

1) paramsSyncHelper_.pushCorrection(0);

isn't this the default construction status ?
Why users should call this function ?

2) 		int correction = paramsSyncHelper_.correction();
		if (correction != 0)
			LOG(RkISP1Schedule, Warning)
				<< "Correcting params sequence "
				<< correction;

		uint32_t paramsSequence;
		if (correction >= 0) {
			nextParamsSequence_ += correction;
			paramsSyncHelper_.pushCorrection(correction);
			paramsSequence = nextParamsSequence_++;
		} else {
			/*
			 * Inject the same sequence multiple times, to correct
			 * for the offset.
			 */
			paramsSyncHelper_.pushCorrection(-1);
			paramsSequence = nextParamsSequence_;
		}

The call pattern seems to be for all users

        int correction = syncHelper.correction();
        ...
        syncHelper.pushCorrection(correction);

which suggests me the call to pushCorrection() could be handled
internally if it has to be called unconditionally.

Another question is what does pushing -1 means, and why the
corrections here are signed integers. Is there anything like negative
corrections ?


> +
> +	void reset()
> +	{
> +		corrections_ = {};
> +		correctionToApply_ = 0;
> +		expectedOffset_ = 0;
> +	}
> +
> +	std::queue<int> corrections_;

Why a queue ? Aren't the corrections cumulative ?

Maybe once clarified my question on the commit message I'll get a
better understanding of this class...

> +	int correctionToApply_ = 0;
> +	int expectedOffset_ = 0;
> +};
> +
> +} /* namespace libcamera */
> --
> 2.48.1
>

Hi Jacopo,

Thanks for digging deep into this.

Quoting Jacopo Mondi (2025-11-06 18:06:24)
> Hi Stefan,
> 
>    let me start by the smallest pieces in this series
> 
> On Fri, Oct 24, 2025 at 10:50:49AM +0200, Stefan Klug wrote:
> > On a V4L2 buffer the assigned sequence is not known until the buffer is
> > dequeued. But for per frame controls we have to prepare other data like
> > sensor controls and ISP params in advance. So we try to anticipate the
> > sequence number a given buffer will be. In a perfect world this works
> > well as long as the initial sequence is assigned correctly. But it
> > breaks as soon as things like running out of buffers or incomplete
> > images happen. To make things even more complicated, in most cases
> > more than one buffer is queued to the kernel at a time.
> >
> > So as soon as a sequence number doesn't match the expected one after
> > dequeuing, most likely all the already queued buffers will be dequeued
> > with the same error.  It is not sufficient to simply add the correction
> > after dequeuing because the error on all queued frames would accumulate
> > and the whole system starts to oscillate. To work around that add a
> 
> Uh, I didn't get this part...
> 
> If I'm expecting frame number X and I get frame X + 2, isn't enough to
> add 2 to the expect frame counter and re-align things ?

Well, roughly. The "re-align things" is the issue. In the system we use
the sensor sequence number as key into to different parts (FCQueue,
delayed controls etc.).

So roughly at computeParams() time we prepare a set of data
(params/sensor controls/imagebuffer) and create an assumption on the
sensor frame number these belong to. This happens for every request that
is queued to the kernel. 

When we now detect a error, we could indeed update the assumptions on
all elements/requests that were tied to a frame and the related
structures (everything that is past the above mentioned computeParams()
point). But the bookkeeping for that (e.g. Do you forward the error to
the IPA?, What happens to a params buffer that is in-flight to the IPA?)
is error-prone and difficult to track in log files and in debugging.

So with the addition of this class we don't have to do that complex
bookkeeping and only correct for the error early in the processing (e.g.
queueRequest()). The class tracks the errors that were accounted for so
that we do not over-correct.

I also added a few numbers for visualization at the bottom of this
email. Maybe worth reading first.

> 
> > SequenceSyncHelper class that tracks the expected error and allows to
> > easily query the necessary correction when queuing new buffers.
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +
> >  .../pipeline/rkisp1/sequence_sync_helper.h    | 69 +++++++++++++++++++
> >  2 files changed, 71 insertions(+)
> >  create mode 100644 src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 7a4957d7e535..d83f7d787892 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -51,10 +51,12 @@
> >  #include "libcamera/internal/yaml_parser.h"
> >
> >  #include "rkisp1_path.h"
> > +#include "sequence_sync_helper.h"
> >
> >  namespace libcamera {
> >
> >  LOG_DEFINE_CATEGORY(RkISP1)
> > +LOG_DEFINE_CATEGORY(RkISP1Schedule)
> >
> >  class PipelineHandlerRkISP1;
> >  class RkISP1CameraData;
> > diff --git a/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
> > new file mode 100644
> > index 000000000000..c3f91dbed45f
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h
> > @@ -0,0 +1,69 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2025, Ideas on Board
> > + *
> > + * Sequence sync helper
> > + */
> > +
> > +#pragma once
> > +
> > +#include <queue>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(RkISP1Schedule)
> > +
> > +class SequenceSyncHelper
> > +{
> > +public:
> > +     int gotFrame(size_t expectedSequence, size_t actualSequence)
> > +     {
> > +             ASSERT(!corrections_.empty());
> > +             int diff = actualSequence - expectedSequence;
> > +             int corr = corrections_.front();
> > +             corrections_.pop();
> > +             expectedOffset_ -= corr;
> > +             int necessaryCorrection = diff - expectedOffset_;
> > +             correctionToApply_ += necessaryCorrection;
> > +
> > +             LOG(RkISP1Schedule, Debug) << "Sync frame "
> > +                                        << "expected: " << expectedSequence
> > +                                        << " actual: " << actualSequence
> > +                                        << " correction: " << corr
> > +                                        << " expectedOffset: " << expectedOffset_
> > +                                        << " correctionToApply " << correctionToApply_;
> > +
> > +             expectedOffset_ += necessaryCorrection;
> > +             return necessaryCorrection;
> > +     }
> > +
> > +     /* Value to be added to the source sequence */
> > +     int correction()
> > +     {
> > +             return correctionToApply_;
> > +     }
> > +
> > +     void pushCorrection(int correction)
> > +     {
> > +             corrections_.push(correction);
> > +             correctionToApply_ -= correction;
> > +             LOG(RkISP1Schedule, Debug)
> > +                     << "Push correction " << correction
> > +                     << " correctionToApply " << correctionToApply_;
> > +     }
> 
> I think you need to explain how this class should be used.
> 
> I'm looking at paramSyncHelper in rkisp1
> 
> 1) paramsSyncHelper_.pushCorrection(0);
> 
> isn't this the default construction status ?
> Why users should call this function ?

No, it indicates that one buffer is in flight, and for that buffer we
didn't apply any sequence correction. There must be one pushCorrection()
call per QBUF and one gotFrame() per DQBUF.

> 
> 2)              int correction = paramsSyncHelper_.correction();
>                 if (correction != 0)
>                         LOG(RkISP1Schedule, Warning)
>                                 << "Correcting params sequence "
>                                 << correction;
> 
>                 uint32_t paramsSequence;
>                 if (correction >= 0) {
>                         nextParamsSequence_ += correction;
>                         paramsSyncHelper_.pushCorrection(correction);
>                         paramsSequence = nextParamsSequence_++;
>                 } else {
>                         /*
>                          * Inject the same sequence multiple times, to correct
>                          * for the offset.
>                          */
>                         paramsSyncHelper_.pushCorrection(-1);
>                         paramsSequence = nextParamsSequence_;
>                 }
> 
> The call pattern seems to be for all users
> 
>         int correction = syncHelper.correction();
>         ...
>         syncHelper.pushCorrection(correction);
> 
> which suggests me the call to pushCorrection() could be handled
> internally if it has to be called unconditionally.

But that wouldn't allow for smaller corrections. In the above code only
corrections > 0 are handled at once. The others are handled one by one.

> 
> Another question is what does pushing -1 means, and why the
> corrections here are signed integers. Is there anything like negative
> corrections ?

Yes, I've seen these. I saw situations where the correction is
flip-flopping over multiple frames which lead e.g. to 
https://lore.kernel.org/linux-media/20250922182003.3712101-2-stefan.klug@ideasonboard.com/

> 
> 
> > +
> > +     void reset()
> > +     {
> > +             corrections_ = {};
> > +             correctionToApply_ = 0;
> > +             expectedOffset_ = 0;
> > +     }
> > +
> > +     std::queue<int> corrections_;
> 
> Why a queue ? Aren't the corrections cumulative ?

They are, but only if we update all the bookkeeping data so we don't
measure the same error on multiple frames.

To visualize the issue I tried to write down a few frames. In all
cases the kernel drops frame 2 and 4. So the output sequence is 
1, 3, 5, 6, 7, 8, ...

On the Q: line I noted what we expect without updating already queued
expectations and the error is accounted for when queueing a new item.

On the D: line I noted what we dequeued and the detected error.

Q: 1, 2, 3    E: 0
D: 1          E: 0
Q: 2, 3, 4    E: 0
D: 3          E: 1
Q: 3, 4, 6    E: 0
D: 5          E: 2
Q: 4, 6, 9    E: 0
D: 6          E: 2
Q: 6, 9, 11   E: 0
D: 7          E: 1
Q: 9, 11, 13  E: 1
D: 8          E: -1
Q: 11, 13, 13 E: 0
D: 9          E: -2
Q: 13, 13, 12 E: 0
D: 10         E: -3
... oscillation continues ...

Now without negative corrections:

Q: 1, 2, 3    E: 0
D: 1          E: 0
Q: 2, 3, 4    E: 0
D: 3          E: 1
Q: 3, 4, 6    E: 0
D: 5          E: 2
Q: 4, 6, 9    E: 0
D: 6          E: 2
Q: 6, 9, 11   E: 0
D: 7          E: 1
Q: 9, 11, 13  E: 1
D: 8          E: 0 (clamped)
Q: 11, 13, 14 E: 0
D: 9          E: 0 (clamped)
Q: 13, 14, 15 E: 0
D: 10         E: 0 (clamped)
... now we are constantly off ...


Now with a correction queue. The sum of the queue is subtracted from the
measured error.

Q: 1, 2, 3    E: 0   C: 0, 0, 0
D: 1          E: 0   C: 0, 0
Q: 2, 3, 4    E: 0   C: 0, 0, 0
D: 3          E: 1   C: 0, 0
Q: 3, 4, 6    E: 0   C: 0, 0, 1
D: 5          E: 1   C: 0, 1
Q: 4, 6, 8    E: 0   C: 0, 1, 1
D: 6          E: 0   C: 1, 1
Q: 6, 8, 9    E: 0   C: 1, 1, 0
D: 7          E: 0   C: 1, 0
Q: 8, 9, 10   E: 0   C: 1, 0, 0   
D: 8          E: 0   C: 0, 0
Q: 9, 10, 11  E: 0   C: 0, 0, 0
D: 9          E: 0

... in sync again ...



On our current master we have this code:
if (data->frame_ <= buffer->metadata().sequence)
     data->frame_ = buffer->metadata().sequence + 1;

This code only starts to correct as soon as the error grew beyond the
number of queued requests (where frame_ is incremented) but is not
suitable for synchronization.

I hope that somehow explains the underlying thoughts.

Best regards,
Stefan

> 
> Maybe once clarified my question on the commit message I'll get a
> better understanding of this class...
> 
> > +     int correctionToApply_ = 0;
> > +     int expectedOffset_ = 0;
> > +};
> > +
> > +} /* namespace libcamera */
> > --
> > 2.48.1
> >