[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
>