[{"id":36426,"web_url":"https://patchwork.libcamera.org/comment/36426/","msgid":"","date":"2025-10-24T10:38:34","subject":"Re: [PATCH v1 25/35] pipeline: rkisp1: Add SequenceSyncHelper class","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 10. 24. 10:50 keltezéssel, Stefan Klug írta:\n> On a V4L2 buffer the assigned sequence is not known until the buffer is\n> dequeued. But for per frame controls we have to prepare other data like\n> sensor controls and ISP params in advance. So we try to anticipate the\n> sequence number a given buffer will be. In a perfect world this works\n> well as long as the initial sequence is assigned correctly. But it\n> breaks as soon as things like running out of buffers or incomplete\n> images happen. To make things even more complicated, in most cases\n> more than one buffer is queued to the kernel at a time.\n> \n> So as soon as a sequence number doesn't match the expected one after\n> dequeuing, most likely all the already queued buffers will be dequeued\n> with the same error. It is not sufficient to simply add the correction\n> after dequeuing because the error on all queued frames would accumulate\n> and the whole system starts to oscillate. To work around that add a\n> SequenceSyncHelper class that tracks the expected error and allows to\n> easily query the necessary correction when queuing new buffers.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> .../pipeline/rkisp1/sequence_sync_helper.h | 69 +++++++++++++++++++\n> 2 files changed, 71 insertions(+)\n> create mode 100644 src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 7a4957d7e535..d83f7d787892 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -51,10 +51,12 @@\n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> #include \"rkisp1_path.h\"\n> +#include \"sequence_sync_helper.h\"\n> \n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(RkISP1)\n> +LOG_DEFINE_CATEGORY(RkISP1Schedule)\n> \n> class PipelineHandlerRkISP1;\n> class RkISP1CameraData;\n> diff --git a/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> new file mode 100644\n> index 000000000000..c3f91dbed45f\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> @@ -0,0 +1,69 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas on Board\n> + *\n> + * Sequence sync helper\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n\nThis looks like a non-rkisp1 specific class. In that case it could\nbe moved to a common area. But if it stays here, I would expect it\nto the in the `libcamera::ipa::rkisp1` namespace.\n\n> +\n> +LOG_DECLARE_CATEGORY(RkISP1Schedule)\n> +\n> +class SequenceSyncHelper\n> +{\n> +public:\n> +\tint gotFrame(size_t expectedSequence, size_t actualSequence)\n> +\t{\n> +\t\tASSERT(!corrections_.empty());\n> +\t\tint diff = actualSequence - expectedSequence;\n\nCan this be negative? When?\n\n\n> +\t\tint corr = corrections_.front();\n> +\t\tcorrections_.pop();\n> +\t\texpectedOffset_ -= corr;\n> +\t\tint necessaryCorrection = diff - expectedOffset_;\n> +\t\tcorrectionToApply_ += necessaryCorrection;\n> +\n> +\t\tLOG(RkISP1Schedule, Debug) << \"Sync frame \"\n> +\t\t\t\t\t << \"expected: \" << expectedSequence\n> +\t\t\t\t\t << \" actual: \" << actualSequence\n> +\t\t\t\t\t << \" correction: \" << corr\n> +\t\t\t\t\t << \" expectedOffset: \" << expectedOffset_\n> +\t\t\t\t\t << \" correctionToApply \" << correctionToApply_;\n> +\n> +\t\texpectedOffset_ += necessaryCorrection;\n> +\t\treturn necessaryCorrection;\n> +\t}\n> +\n> +\t/* Value to be added to the source sequence */\n> +\tint correction()\n\n ...() const\n\n\n> +\t{\n> +\t\treturn correctionToApply_;\n> +\t}\n> +\n> +\tvoid pushCorrection(int correction)\n> +\t{\n> +\t\tcorrections_.push(correction);\n> +\t\tcorrectionToApply_ -= correction;\n> +\t\tLOG(RkISP1Schedule, Debug)\n> +\t\t\t<< \"Push correction \" << correction\n> +\t\t\t<< \" correctionToApply \" << correctionToApply_;\n> +\t}\n> +\n> +\tvoid reset()\n> +\t{\n> +\t\tcorrections_ = {};\n> +\t\tcorrectionToApply_ = 0;\n> +\t\texpectedOffset_ = 0;\n> +\t}\n> +\n> +\tstd::queue corrections_;\n> +\tint correctionToApply_ = 0;\n> +\tint expectedOffset_ = 0;\n\nI suppose these could (should?) be private?\n\nAlso, did you plan on adding a bit more documentation?\nFor example, as far as I can tell the general mode of operation is:\n\n * (query correction and) apply correction\n * pushCorrection() the applied correction\n * ...\n * gotFrame() when the corresponding frame arrives\n\nSo `pushCorrection()` and `gotFrame()` calls should be strictly paired\nas far as I can see.\n\nI think some explanation about the algorithm would also be appreciated.\n\nAs far as I understand `correctionToApply_` is the cumulative correction to\nbe applied to \"new\" frames. And generally it is expected that `pushCorrection()`\nwill set `correctionToApply_` to 0 because it is just called with whatever\n`correction()` returns.\n\nAnd `expectedOffset_` stores the expected difference for the \"in-progress\" frames.\nSo for example if you queue two frames in succession (e.g. with correction of 0),\nand the first one runs into a frame skip, then `expectedOffset_` is there to account\nfor that for when the second frame arrives.\n\nSo for a particular frame after `expectedOffset_ -= corr`, `expectedOffset_` is number\nof skipped frames expected from earlier frames. If that is different from `diff`, then\nthe current frame also \"has\" skipped frames: `necessaryCorrection`.\n\nIs this any close to what this is supposed to be doing?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> +};\n> +\n> +} /* namespace libcamera */\n> --\n> 2.48.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8E293C3259\n\tfor ;\n\tFri, 24 Oct 2025 10:38:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE6CA6087D;\n\tFri, 24 Oct 2025 12:38:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39BA46087D\n\tfor ;\n\tFri, 24 Oct 2025 12:38:38 +0200 (CEST)","from [192.168.33.13] (185.221.141.231.nat.pool.zt.hu\n\t[185.221.141.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D1CF591;\n\tFri, 24 Oct 2025 12:36:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Df/peUTQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761302212;\n\tbh=AJdSWq9HRpXf+hxWZTwgnDt9pEuDsesyegRYAfyO/lc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Df/peUTQxhDrrsJGx0XqUWm8KkZj9SJEdBXND8zy/OCxOu05uwXAvRvaHxD/DWVJ/\n\t/b2DtB4vC+hwEcb7h4Xe4eTNZ7ZebY+HGN6QTMxj6lCC2Vwr24IgNLV56oc8pHKN3D\n\tBAACvAywRlTKkx4X9SfT+2g+ohL4k6JpYdmrsHyo=","Message-ID":"","Date":"Fri, 24 Oct 2025 12:38:34 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 25/35] pipeline: rkisp1: Add SequenceSyncHelper class","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t\n\t<20251024085130.995967-26-stefan.klug@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251024085130.995967-26-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36737,"web_url":"https://patchwork.libcamera.org/comment/36737/","msgid":"<2ymjc52yhtr6kgvt2negatpzgbzaqpqig3y5wtl4mmv3ubl4ha@e4pxa4ls3gox>","date":"2025-11-06T17:06:24","subject":"Re: [PATCH v1 25/35] pipeline: rkisp1: Add SequenceSyncHelper class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan,\n\n let me start by the smallest pieces in this series\n\nOn Fri, Oct 24, 2025 at 10:50:49AM +0200, Stefan Klug wrote:\n> On a V4L2 buffer the assigned sequence is not known until the buffer is\n> dequeued. But for per frame controls we have to prepare other data like\n> sensor controls and ISP params in advance. So we try to anticipate the\n> sequence number a given buffer will be. In a perfect world this works\n> well as long as the initial sequence is assigned correctly. But it\n> breaks as soon as things like running out of buffers or incomplete\n> images happen. To make things even more complicated, in most cases\n> more than one buffer is queued to the kernel at a time.\n>\n> So as soon as a sequence number doesn't match the expected one after\n> dequeuing, most likely all the already queued buffers will be dequeued\n> with the same error. It is not sufficient to simply add the correction\n> after dequeuing because the error on all queued frames would accumulate\n> and the whole system starts to oscillate. To work around that add a\n\nUh, I didn't get this part...\n\nIf I'm expecting frame number X and I get frame X + 2, isn't enough to\nadd 2 to the expect frame counter and re-align things ?\n\n> SequenceSyncHelper class that tracks the expected error and allows to\n> easily query the necessary correction when queuing new buffers.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> .../pipeline/rkisp1/sequence_sync_helper.h | 69 +++++++++++++++++++\n> 2 files changed, 71 insertions(+)\n> create mode 100644 src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 7a4957d7e535..d83f7d787892 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -51,10 +51,12 @@\n> #include \"libcamera/internal/yaml_parser.h\"\n>\n> #include \"rkisp1_path.h\"\n> +#include \"sequence_sync_helper.h\"\n>\n> namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(RkISP1)\n> +LOG_DEFINE_CATEGORY(RkISP1Schedule)\n>\n> class PipelineHandlerRkISP1;\n> class RkISP1CameraData;\n> diff --git a/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> new file mode 100644\n> index 000000000000..c3f91dbed45f\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> @@ -0,0 +1,69 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas on Board\n> + *\n> + * Sequence sync helper\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(RkISP1Schedule)\n> +\n> +class SequenceSyncHelper\n> +{\n> +public:\n> +\tint gotFrame(size_t expectedSequence, size_t actualSequence)\n> +\t{\n> +\t\tASSERT(!corrections_.empty());\n> +\t\tint diff = actualSequence - expectedSequence;\n> +\t\tint corr = corrections_.front();\n> +\t\tcorrections_.pop();\n> +\t\texpectedOffset_ -= corr;\n> +\t\tint necessaryCorrection = diff - expectedOffset_;\n> +\t\tcorrectionToApply_ += necessaryCorrection;\n> +\n> +\t\tLOG(RkISP1Schedule, Debug) << \"Sync frame \"\n> +\t\t\t\t\t << \"expected: \" << expectedSequence\n> +\t\t\t\t\t << \" actual: \" << actualSequence\n> +\t\t\t\t\t << \" correction: \" << corr\n> +\t\t\t\t\t << \" expectedOffset: \" << expectedOffset_\n> +\t\t\t\t\t << \" correctionToApply \" << correctionToApply_;\n> +\n> +\t\texpectedOffset_ += necessaryCorrection;\n> +\t\treturn necessaryCorrection;\n> +\t}\n> +\n> +\t/* Value to be added to the source sequence */\n> +\tint correction()\n> +\t{\n> +\t\treturn correctionToApply_;\n> +\t}\n> +\n> +\tvoid pushCorrection(int correction)\n> +\t{\n> +\t\tcorrections_.push(correction);\n> +\t\tcorrectionToApply_ -= correction;\n> +\t\tLOG(RkISP1Schedule, Debug)\n> +\t\t\t<< \"Push correction \" << correction\n> +\t\t\t<< \" correctionToApply \" << correctionToApply_;\n> +\t}\n\nI think you need to explain how this class should be used.\n\nI'm looking at paramSyncHelper in rkisp1\n\n1) paramsSyncHelper_.pushCorrection(0);\n\nisn't this the default construction status ?\nWhy users should call this function ?\n\n2) \t\tint correction = paramsSyncHelper_.correction();\n\t\tif (correction != 0)\n\t\t\tLOG(RkISP1Schedule, Warning)\n\t\t\t\t<< \"Correcting params sequence \"\n\t\t\t\t<< correction;\n\n\t\tuint32_t paramsSequence;\n\t\tif (correction >= 0) {\n\t\t\tnextParamsSequence_ += correction;\n\t\t\tparamsSyncHelper_.pushCorrection(correction);\n\t\t\tparamsSequence = nextParamsSequence_++;\n\t\t} else {\n\t\t\t/*\n\t\t\t * Inject the same sequence multiple times, to correct\n\t\t\t * for the offset.\n\t\t\t */\n\t\t\tparamsSyncHelper_.pushCorrection(-1);\n\t\t\tparamsSequence = nextParamsSequence_;\n\t\t}\n\nThe call pattern seems to be for all users\n\n int correction = syncHelper.correction();\n ...\n syncHelper.pushCorrection(correction);\n\nwhich suggests me the call to pushCorrection() could be handled\ninternally if it has to be called unconditionally.\n\nAnother question is what does pushing -1 means, and why the\ncorrections here are signed integers. Is there anything like negative\ncorrections ?\n\n\n> +\n> +\tvoid reset()\n> +\t{\n> +\t\tcorrections_ = {};\n> +\t\tcorrectionToApply_ = 0;\n> +\t\texpectedOffset_ = 0;\n> +\t}\n> +\n> +\tstd::queue corrections_;\n\nWhy a queue ? Aren't the corrections cumulative ?\n\nMaybe once clarified my question on the commit message I'll get a\nbetter understanding of this class...\n\n> +\tint correctionToApply_ = 0;\n> +\tint expectedOffset_ = 0;\n> +};\n> +\n> +} /* namespace libcamera */\n> --\n> 2.48.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9F138C3241\n\tfor ;\n\tThu, 6 Nov 2025 17:06:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0FB2609D8;\n\tThu, 6 Nov 2025 18:06:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01156606E6\n\tfor ;\n\tThu, 6 Nov 2025 18:06:28 +0100 (CET)","from ideasonboard.com (mob-5-90-141-71.net.vodafone.it\n\t[5.90.141.71])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 619D922A;\n\tThu, 6 Nov 2025 18:04:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hxYOUeoB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762448673;\n\tbh=sp/m7+t6Y9cYCaN6z225aMUUWKVsNYr0Z9otevGAwK8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hxYOUeoBiR/r8G4ltUm9KofxTAhs1z8NjwpIA1LrYr/OqyLP16OBYjnCyv0Hkva+e\n\tUF4oWc4zWg4h2g3UnDBVA2zgeFuTm6p5wbbji1cweCpZM/1odyuxOksXV251VqXA1d\n\thqdT0TbHIxmAWmCoc0UZXpEAjUu0vdQWKL5/WhSw=","Date":"Thu, 6 Nov 2025 18:06:24 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 25/35] pipeline: rkisp1: Add SequenceSyncHelper class","Message-ID":"<2ymjc52yhtr6kgvt2negatpzgbzaqpqig3y5wtl4mmv3ubl4ha@e4pxa4ls3gox>","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-26-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251024085130.995967-26-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36750,"web_url":"https://patchwork.libcamera.org/comment/36750/","msgid":"<176253105522.509848.11176855002155475459@localhost>","date":"2025-11-07T15:57:35","subject":"Re: [PATCH v1 25/35] pipeline: rkisp1: Add SequenceSyncHelper class","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for digging deep into this.\n\nQuoting Jacopo Mondi (2025-11-06 18:06:24)\n> Hi Stefan,\n> \n> let me start by the smallest pieces in this series\n> \n> On Fri, Oct 24, 2025 at 10:50:49AM +0200, Stefan Klug wrote:\n> > On a V4L2 buffer the assigned sequence is not known until the buffer is\n> > dequeued. But for per frame controls we have to prepare other data like\n> > sensor controls and ISP params in advance. So we try to anticipate the\n> > sequence number a given buffer will be. In a perfect world this works\n> > well as long as the initial sequence is assigned correctly. But it\n> > breaks as soon as things like running out of buffers or incomplete\n> > images happen. To make things even more complicated, in most cases\n> > more than one buffer is queued to the kernel at a time.\n> >\n> > So as soon as a sequence number doesn't match the expected one after\n> > dequeuing, most likely all the already queued buffers will be dequeued\n> > with the same error. It is not sufficient to simply add the correction\n> > after dequeuing because the error on all queued frames would accumulate\n> > and the whole system starts to oscillate. To work around that add a\n> \n> Uh, I didn't get this part...\n> \n> If I'm expecting frame number X and I get frame X + 2, isn't enough to\n> add 2 to the expect frame counter and re-align things ?\n\nWell, roughly. The \"re-align things\" is the issue. In the system we use\nthe sensor sequence number as key into to different parts (FCQueue,\ndelayed controls etc.).\n\nSo roughly at computeParams() time we prepare a set of data\n(params/sensor controls/imagebuffer) and create an assumption on the\nsensor frame number these belong to. This happens for every request that\nis queued to the kernel. \n\nWhen we now detect a error, we could indeed update the assumptions on\nall elements/requests that were tied to a frame and the related\nstructures (everything that is past the above mentioned computeParams()\npoint). But the bookkeeping for that (e.g. Do you forward the error to\nthe IPA?, What happens to a params buffer that is in-flight to the IPA?)\nis error-prone and difficult to track in log files and in debugging.\n\nSo with the addition of this class we don't have to do that complex\nbookkeeping and only correct for the error early in the processing (e.g.\nqueueRequest()). The class tracks the errors that were accounted for so\nthat we do not over-correct.\n\nI also added a few numbers for visualization at the bottom of this\nemail. Maybe worth reading first.\n\n> \n> > SequenceSyncHelper class that tracks the expected error and allows to\n> > easily query the necessary correction when queuing new buffers.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> > .../pipeline/rkisp1/sequence_sync_helper.h | 69 +++++++++++++++++++\n> > 2 files changed, 71 insertions(+)\n> > create mode 100644 src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 7a4957d7e535..d83f7d787892 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -51,10 +51,12 @@\n> > #include \"libcamera/internal/yaml_parser.h\"\n> >\n> > #include \"rkisp1_path.h\"\n> > +#include \"sequence_sync_helper.h\"\n> >\n> > namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(RkISP1)\n> > +LOG_DEFINE_CATEGORY(RkISP1Schedule)\n> >\n> > class PipelineHandlerRkISP1;\n> > class RkISP1CameraData;\n> > diff --git a/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> > new file mode 100644\n> > index 000000000000..c3f91dbed45f\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> > @@ -0,0 +1,69 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2025, Ideas on Board\n> > + *\n> > + * Sequence sync helper\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(RkISP1Schedule)\n> > +\n> > +class SequenceSyncHelper\n> > +{\n> > +public:\n> > + int gotFrame(size_t expectedSequence, size_t actualSequence)\n> > + {\n> > + ASSERT(!corrections_.empty());\n> > + int diff = actualSequence - expectedSequence;\n> > + int corr = corrections_.front();\n> > + corrections_.pop();\n> > + expectedOffset_ -= corr;\n> > + int necessaryCorrection = diff - expectedOffset_;\n> > + correctionToApply_ += necessaryCorrection;\n> > +\n> > + LOG(RkISP1Schedule, Debug) << \"Sync frame \"\n> > + << \"expected: \" << expectedSequence\n> > + << \" actual: \" << actualSequence\n> > + << \" correction: \" << corr\n> > + << \" expectedOffset: \" << expectedOffset_\n> > + << \" correctionToApply \" << correctionToApply_;\n> > +\n> > + expectedOffset_ += necessaryCorrection;\n> > + return necessaryCorrection;\n> > + }\n> > +\n> > + /* Value to be added to the source sequence */\n> > + int correction()\n> > + {\n> > + return correctionToApply_;\n> > + }\n> > +\n> > + void pushCorrection(int correction)\n> > + {\n> > + corrections_.push(correction);\n> > + correctionToApply_ -= correction;\n> > + LOG(RkISP1Schedule, Debug)\n> > + << \"Push correction \" << correction\n> > + << \" correctionToApply \" << correctionToApply_;\n> > + }\n> \n> I think you need to explain how this class should be used.\n> \n> I'm looking at paramSyncHelper in rkisp1\n> \n> 1) paramsSyncHelper_.pushCorrection(0);\n> \n> isn't this the default construction status ?\n> Why users should call this function ?\n\nNo, it indicates that one buffer is in flight, and for that buffer we\ndidn't apply any sequence correction. There must be one pushCorrection()\ncall per QBUF and one gotFrame() per DQBUF.\n\n> \n> 2) int correction = paramsSyncHelper_.correction();\n> if (correction != 0)\n> LOG(RkISP1Schedule, Warning)\n> << \"Correcting params sequence \"\n> << correction;\n> \n> uint32_t paramsSequence;\n> if (correction >= 0) {\n> nextParamsSequence_ += correction;\n> paramsSyncHelper_.pushCorrection(correction);\n> paramsSequence = nextParamsSequence_++;\n> } else {\n> /*\n> * Inject the same sequence multiple times, to correct\n> * for the offset.\n> */\n> paramsSyncHelper_.pushCorrection(-1);\n> paramsSequence = nextParamsSequence_;\n> }\n> \n> The call pattern seems to be for all users\n> \n> int correction = syncHelper.correction();\n> ...\n> syncHelper.pushCorrection(correction);\n> \n> which suggests me the call to pushCorrection() could be handled\n> internally if it has to be called unconditionally.\n\nBut that wouldn't allow for smaller corrections. In the above code only\ncorrections > 0 are handled at once. The others are handled one by one.\n\n> \n> Another question is what does pushing -1 means, and why the\n> corrections here are signed integers. Is there anything like negative\n> corrections ?\n\nYes, I've seen these. I saw situations where the correction is\nflip-flopping over multiple frames which lead e.g. to \nhttps://lore.kernel.org/linux-media/20250922182003.3712101-2-stefan.klug@ideasonboard.com/\n\n> \n> \n> > +\n> > + void reset()\n> > + {\n> > + corrections_ = {};\n> > + correctionToApply_ = 0;\n> > + expectedOffset_ = 0;\n> > + }\n> > +\n> > + std::queue corrections_;\n> \n> Why a queue ? Aren't the corrections cumulative ?\n\nThey are, but only if we update all the bookkeeping data so we don't\nmeasure the same error on multiple frames.\n\nTo visualize the issue I tried to write down a few frames. In all\ncases the kernel drops frame 2 and 4. So the output sequence is \n1, 3, 5, 6, 7, 8, ...\n\nOn the Q: line I noted what we expect without updating already queued\nexpectations and the error is accounted for when queueing a new item.\n\nOn the D: line I noted what we dequeued and the detected error.\n\nQ: 1, 2, 3 E: 0\nD: 1 E: 0\nQ: 2, 3, 4 E: 0\nD: 3 E: 1\nQ: 3, 4, 6 E: 0\nD: 5 E: 2\nQ: 4, 6, 9 E: 0\nD: 6 E: 2\nQ: 6, 9, 11 E: 0\nD: 7 E: 1\nQ: 9, 11, 13 E: 1\nD: 8 E: -1\nQ: 11, 13, 13 E: 0\nD: 9 E: -2\nQ: 13, 13, 12 E: 0\nD: 10 E: -3\n... oscillation continues ...\n\nNow without negative corrections:\n\nQ: 1, 2, 3 E: 0\nD: 1 E: 0\nQ: 2, 3, 4 E: 0\nD: 3 E: 1\nQ: 3, 4, 6 E: 0\nD: 5 E: 2\nQ: 4, 6, 9 E: 0\nD: 6 E: 2\nQ: 6, 9, 11 E: 0\nD: 7 E: 1\nQ: 9, 11, 13 E: 1\nD: 8 E: 0 (clamped)\nQ: 11, 13, 14 E: 0\nD: 9 E: 0 (clamped)\nQ: 13, 14, 15 E: 0\nD: 10 E: 0 (clamped)\n... now we are constantly off ...\n\n\nNow with a correction queue. The sum of the queue is subtracted from the\nmeasured error.\n\nQ: 1, 2, 3 E: 0 C: 0, 0, 0\nD: 1 E: 0 C: 0, 0\nQ: 2, 3, 4 E: 0 C: 0, 0, 0\nD: 3 E: 1 C: 0, 0\nQ: 3, 4, 6 E: 0 C: 0, 0, 1\nD: 5 E: 1 C: 0, 1\nQ: 4, 6, 8 E: 0 C: 0, 1, 1\nD: 6 E: 0 C: 1, 1\nQ: 6, 8, 9 E: 0 C: 1, 1, 0\nD: 7 E: 0 C: 1, 0\nQ: 8, 9, 10 E: 0 C: 1, 0, 0 \nD: 8 E: 0 C: 0, 0\nQ: 9, 10, 11 E: 0 C: 0, 0, 0\nD: 9 E: 0\n\n... in sync again ...\n\n\n\nOn our current master we have this code:\nif (data->frame_ <= buffer->metadata().sequence)\n data->frame_ = buffer->metadata().sequence + 1;\n\nThis code only starts to correct as soon as the error grew beyond the\nnumber of queued requests (where frame_ is incremented) but is not\nsuitable for synchronization.\n\nI hope that somehow explains the underlying thoughts.\n\nBest regards,\nStefan\n\n> \n> Maybe once clarified my question on the commit message I'll get a\n> better understanding of this class...\n> \n> > + int correctionToApply_ = 0;\n> > + int expectedOffset_ = 0;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.48.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1C0D4C3241\n\tfor ;\n\tFri, 7 Nov 2025 15:57:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4746960A80;\n\tFri, 7 Nov 2025 16:57:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7579F608CF\n\tfor ;\n\tFri, 7 Nov 2025 16:57:38 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:8809:eccd:216c:b9a0])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id E9F6322E;\n\tFri, 7 Nov 2025 16:55:41 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"up3RayWw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762530942;\n\tbh=GKdbeJMmKk6jHh6M2Sr4ednH+zhVTW4h1gKZ0d7WEoQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=up3RayWwqUeWxxPdLP4leBK7LocyKc7/QyvCAb775QZP7iaYQLKHWhX7PozOOX3mO\n\tH2VGcCf6Cv1YrU06xo3nkdg7dwZc20OyxhcfmYkbKtA8NLDY5SexckVrxrXU4epPYm\n\t6CB4E9TARNQRguG4BI+cOrycUT5yChe7W8hCfIik=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<2ymjc52yhtr6kgvt2negatpzgbzaqpqig3y5wtl4mmv3ubl4ha@e4pxa4ls3gox>","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-26-stefan.klug@ideasonboard.com>\n\t<2ymjc52yhtr6kgvt2negatpzgbzaqpqig3y5wtl4mmv3ubl4ha@e4pxa4ls3gox>","Subject":"Re: [PATCH v1 25/35] pipeline: rkisp1: Add SequenceSyncHelper class","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"Jacopo Mondi ","Date":"Fri, 07 Nov 2025 16:57:35 +0100","Message-ID":"<176253105522.509848.11176855002155475459@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37858,"web_url":"https://patchwork.libcamera.org/comment/37858/","msgid":"<176907343007.3882822.17295531075150243707@neptunite.rasen.tech>","date":"2026-01-22T09:17:10","subject":"Re: [PATCH v1 25/35] pipeline: rkisp1: Add SequenceSyncHelper class","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-10-24 17:50:49)\n> On a V4L2 buffer the assigned sequence is not known until the buffer is\n> dequeued. But for per frame controls we have to prepare other data like\n> sensor controls and ISP params in advance. So we try to anticipate the\n> sequence number a given buffer will be. In a perfect world this works\n> well as long as the initial sequence is assigned correctly. But it\n> breaks as soon as things like running out of buffers or incomplete\n> images happen. To make things even more complicated, in most cases\n> more than one buffer is queued to the kernel at a time.\n> \n> So as soon as a sequence number doesn't match the expected one after\n> dequeuing, most likely all the already queued buffers will be dequeued\n> with the same error. It is not sufficient to simply add the correction\n> after dequeuing because the error on all queued frames would accumulate\n> and the whole system starts to oscillate. To work around that add a\n> SequenceSyncHelper class that tracks the expected error and allows to\n> easily query the necessary correction when queuing new buffers.\n> \n> Signed-off-by: Stefan Klug \n\nAs others have said, I think we need to add documentation. The example\ntimelines (all three of them) that you wrote in reply to Jacopo I thnik are\nespecially helpful in understanding what's going on.\n\n\nPaul\n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> .../pipeline/rkisp1/sequence_sync_helper.h | 69 +++++++++++++++++++\n> 2 files changed, 71 insertions(+)\n> create mode 100644 src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 7a4957d7e535..d83f7d787892 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -51,10 +51,12 @@\n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> #include \"rkisp1_path.h\"\n> +#include \"sequence_sync_helper.h\"\n> \n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(RkISP1)\n> +LOG_DEFINE_CATEGORY(RkISP1Schedule)\n> \n> class PipelineHandlerRkISP1;\n> class RkISP1CameraData;\n> diff --git a/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> new file mode 100644\n> index 000000000000..c3f91dbed45f\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/sequence_sync_helper.h\n> @@ -0,0 +1,69 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas on Board\n> + *\n> + * Sequence sync helper\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(RkISP1Schedule)\n> +\n> +class SequenceSyncHelper\n> +{\n> +public:\n> + int gotFrame(size_t expectedSequence, size_t actualSequence)\n> + {\n> + ASSERT(!corrections_.empty());\n> + int diff = actualSequence - expectedSequence;\n> + int corr = corrections_.front();\n> + corrections_.pop();\n> + expectedOffset_ -= corr;\n> + int necessaryCorrection = diff - expectedOffset_;\n> + correctionToApply_ += necessaryCorrection;\n> +\n> + LOG(RkISP1Schedule, Debug) << \"Sync frame \"\n> + << \"expected: \" << expectedSequence\n> + << \" actual: \" << actualSequence\n> + << \" correction: \" << corr\n> + << \" expectedOffset: \" << expectedOffset_\n> + << \" correctionToApply \" << correctionToApply_;\n> +\n> + expectedOffset_ += necessaryCorrection;\n> + return necessaryCorrection;\n> + }\n> +\n> + /* Value to be added to the source sequence */\n> + int correction()\n> + {\n> + return correctionToApply_;\n> + }\n> +\n> + void pushCorrection(int correction)\n> + {\n> + corrections_.push(correction);\n> + correctionToApply_ -= correction;\n> + LOG(RkISP1Schedule, Debug)\n> + << \"Push correction \" << correction\n> + << \" correctionToApply \" << correctionToApply_;\n> + }\n> +\n> + void reset()\n> + {\n> + corrections_ = {};\n> + correctionToApply_ = 0;\n> + expectedOffset_ = 0;\n> + }\n> +\n> + std::queue corrections_;\n> + int correctionToApply_ = 0;\n> + int expectedOffset_ = 0;\n> +};\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.48.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F080EBDCBF\n\tfor ;\n\tThu, 22 Jan 2026 09:17:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9046C61FFB;\n\tThu, 22 Jan 2026 10:17:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07C0D61FC4\n\tfor ;\n\tThu, 22 Jan 2026 10:17:16 +0100 (CET)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:8816:a947:ebed:2ec7])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 210B0324;\n\tThu, 22 Jan 2026 10:16:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uqcSCyuV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1769073403;\n\tbh=BchOLaZDixYp/h9fkWB4syOvwkIVf4TAmx4W0Pwnr9o=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=uqcSCyuVRIG42acEVzlnaCu6fImxIMiJVOb8Twim3kR2JLXBW3MIUpNcJDN0AoFTG\n\tf47jgwFhykfl4XZKuJ2yvtpkX5OouhLt+7bdK3D142qhJs+wTDSQabXp7P/yJScR07\n\t8S/zOiwypcH7YRqJG0CTOFhw484dqRl+AQ6HRa9c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251024085130.995967-26-stefan.klug@ideasonboard.com>","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-26-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v1 25/35] pipeline: rkisp1: Add SequenceSyncHelper class","From":"Paul Elder ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 22 Jan 2026 18:17:10 +0900","Message-ID":"<176907343007.3882822.17295531075150243707@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]