[{"id":32537,"web_url":"https://patchwork.libcamera.org/comment/32537/","msgid":"<173339876982.3135963.3796854513088041144@ping.linuxembedded.co.uk>","date":"2024-12-05T11:39:29","subject":"Re: [RFC PATCH 2/3] libcamera: clock: Add ClockRecovery class to\n\thelp generate wallclock timestamps","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nThanks for discussing this in real-time. I'm looking forward to merging\nthese.\n\n\nQuoting David Plowman (2024-11-26 12:17:05)\n> Sampling the system clock is susceptible to many milliseconds of\n> jitter, dependent on system load and other factors.\n> \n> The ClockRecovery class takes pairs of kernel timestamps (which\n> exhitbit much less jitter) and wallclock readings, and returns a\n> smoother version of the wallclock timestamps.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/clock_recovery.h | 64 ++++++++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/clock_recovery.cpp | 110 ++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 176 insertions(+)\n> create mode 100644 include/libcamera/internal/clock_recovery.h\n> create mode 100644 src/libcamera/clock_recovery.cpp\n> \n> diff --git a/include/libcamera/internal/clock_recovery.h b/include/libcamera/internal/clock_recovery.h\n> new file mode 100644\n> index 00000000..49747747\n> --- /dev/null\n> +++ b/include/libcamera/internal/clock_recovery.h\n> @@ -0,0 +1,64 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Raspberry Pi Ltd\n> + *\n> + * Camera sync control algorithm\n> + */\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class ClockRecovery\n> +{\n> +public:\n> + ClockRecovery();\n> +\n> + /* Initialise with configuration parameters and restart the fitting process. */\n> + void initialise(unsigned int numPts = 100, unsigned int maxJitter = 2000, unsigned int minPts = 10,\n> + unsigned int errorThreshold = 50000);\n> + /* Erase all history and restart the fitting process. */\n> + void reset();\n> +\n> + // Add a new input clock / output clock sample. */\n\n// */ needs fixing.\n\n> + void addSample(uint64_t input, uint64_t output);\n> + /* Calculate the output clock value for this input. */\n> + uint64_t getOutput(uint64_t input);\n> +\n> +private:\n> + unsigned int numPts_; /* how many samples contribute to the history */\n> + unsigned int maxJitter_; /* smooth out any jitter larger than this immediately */\n> + unsigned int minPts_; /* number of samples below which we treat clocks as 1:1 */\n> + unsigned int errorThreshold_; /* reset everything when the error exceeds this */\n> +\n> + unsigned int count_; /* how many samples seen (up to numPts_) */\n> + uint64_t inputBase_; /* subtract this from all input values, just to make the numbers easier */\n> + uint64_t outputBase_; /* as above, for the output */\n> +\n> + uint64_t lastInput_; /* the previous input sample */\n> + uint64_t lastOutput_; /* the previous output sample */\n> +\n> + /*\n> + * We do a linear regression of y against x, where:\n> + * x is the value input - inputBase_, and\n> + * y is the value output - outputBase_ - x.\n> + * We additionally subtract x from y so that y \"should\" be zero, again making the numnbers easier.\n\n\n> + */\n> + double xAve_; /* average x value seen so far */\n> + double yAve_; /* average y value seen so far */\n> + double x2Ave_; /* average x^2 value seen so far */\n> + double xyAve_; /* average x*y value seen so far */\n> +\n> + /*\n> + * Once we've seen more than minPts_ samples, we recalculate the slope and offset according\n> + * to the linear regression normal equations.\n> + */\n> + double slope_; /* latest slope value */\n> + double offset_; /* latest offset value */\n> +\n> + /* We use this cumulative error to monitor spontaneous system clock updates. */\n> + double error_;\n> +};\n> +\n> +} //namespace libcamera\n\nSmall nit here\n\n} /* namespace libcamera */\n\n\nSome line wrapping needed throughout to aim for 80 chars could help too,\n- but that's all I've got - so with that:\n\nReviewed-by: Kieran Bingham \n\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 1dddcd50..b6271ee1 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -11,6 +11,7 @@ libcamera_internal_headers = files([\n> 'camera_manager.h',\n> 'camera_sensor.h',\n> 'camera_sensor_properties.h',\n> + 'clock_recovery.h',\n> 'control_serializer.h',\n> 'control_validator.h',\n> 'converter.h',\n> diff --git a/src/libcamera/clock_recovery.cpp b/src/libcamera/clock_recovery.cpp\n> new file mode 100644\n> index 00000000..6dec8cb3\n> --- /dev/null\n> +++ b/src/libcamera/clock_recovery.cpp\n> @@ -0,0 +1,110 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Raspberry Pi Ltd\n> + *\n> + * Camera sync control algorithm\n> + */\n> +\n> +#include \"libcamera/internal/clock_recovery.h\"\n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(RPiClockRec)\n> +\n> +ClockRecovery::ClockRecovery()\n> +{\n> + initialise();\n> +}\n> +\n> +void ClockRecovery::initialise(unsigned int numPts, unsigned int maxJitter, unsigned int minPts,\n> + unsigned int errorThreshold)\n> +{\n> + numPts_ = numPts;\n> + maxJitter_ = maxJitter;\n> + minPts_ = minPts;\n> + errorThreshold_ = errorThreshold;\n> + reset();\n> +}\n> +\n> +void ClockRecovery::reset()\n> +{\n> + lastInput_ = 0;\n> + lastOutput_ = 0;\n> + xAve_ = 0;\n> + yAve_ = 0;\n> + x2Ave_ = 0;\n> + xyAve_ = 0;\n> + count_ = 0;\n> + slope_ = 0.0;\n> + offset_ = 0.0;\n> + error_ = 0.0;\n> +}\n> +\n> +void ClockRecovery::addSample(uint64_t input, uint64_t output)\n> +{\n> + if (count_ == 0) {\n> + inputBase_ = input;\n> + outputBase_ = output;\n> + }\n> +\n> + /*\n> + * We keep an eye on cumulative drift over the last several frames. If this exceeds a\n> + * threshold, then probably the system clock has been updated and we're going to have to\n> + * reset everything and start over.\n> + */\n> + if (lastOutput_) {\n> + int64_t inputDiff = getOutput(input) - getOutput(lastInput_);\n> + int64_t outputDiff = output - lastOutput_;\n> + error_ = error_ * 0.95 + (outputDiff - inputDiff);\n> + if (std::abs(error_) > errorThreshold_) {\n> + reset();\n> + inputBase_ = input;\n> + outputBase_ = output;\n> + }\n> + }\n> + lastInput_ = input;\n> + lastOutput_ = output;\n> +\n> + /*\n> + * Never let the new output value be more than maxJitter_ away from what we would have expected.\n> + * This is just to reduce the effect of sudden large delays in the measured output.\n> + */\n> + uint64_t expectedOutput = getOutput(input);\n> + output = std::clamp(output, expectedOutput - maxJitter_, expectedOutput + maxJitter_);\n> +\n> + /*\n> + * We use x, y, x^2 and x*y sums to calculate the best fit line. Here we update them by\n> + * pretending we have count_ samples at the previous fit, and now one new one. Gradually\n> + * the effect of the older values gets lost. This is a very simple way of updating the\n> + * fit (there are much more complicated ones!), but it works well enough. Using averages\n> + * instead of sums makes the relative effect of old values and the new sample clearer.\n> + */\n> + double x = input - inputBase_;\n> + double y = output - outputBase_ - x;\n> + unsigned int count1 = count_ + 1;\n> + xAve_ = (count_ * xAve_ + x) / count1;\n> + yAve_ = (count_ * yAve_ + y) / count1;\n> + x2Ave_ = (count_ * x2Ave_ + x * x) / count1;\n> + xyAve_ = (count_ * xyAve_ + x * y) / count1;\n> +\n> + /* Don't update slope and offset until we've seen \"enough\" sample points. */\n> + if (count_ > minPts_) {\n> + /* These are the standard equations for least squares linear regression. */\n> + slope_ = (count1 * count1 * xyAve_ - count1 * xAve_ * count1 * yAve_) /\n> + (count1 * count1 * x2Ave_ - count1 * xAve_ * count1 * xAve_);\n> + offset_ = yAve_ - slope_ * xAve_;\n> + }\n> +\n> + /* Don't increase count_ above numPts_, as this controls the long-term amount of the residual fit. */\n> + if (count1 < numPts_)\n> + count_++;\n> +}\n> +\n> +uint64_t ClockRecovery::getOutput(uint64_t input)\n> +{\n> + double x = input - inputBase_;\n> + double y = slope_ * x + offset_;\n> + return y + x + outputBase_;\n> +}\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 21cae117..f221590c 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -21,6 +21,7 @@ libcamera_internal_sources = files([\n> 'byte_stream_buffer.cpp',\n> 'camera_controls.cpp',\n> 'camera_lens.cpp',\n> + 'clock_recovery.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> 'converter.cpp',\n> -- \n> 2.39.5\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 93512C324E\n\tfor ;\n\tThu, 5 Dec 2024 11:39:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AA6D660F2;\n\tThu, 5 Dec 2024 12:39:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50EA76608C\n\tfor ;\n\tThu, 5 Dec 2024 12:39:33 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 80A8F2B3;\n\tThu, 5 Dec 2024 12:39:04 +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=\"XOBF8IH4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733398744;\n\tbh=ivkkC0+zw9BUESuGnX+PCpLXMbR0NO7ILmOM60fdhgk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=XOBF8IH40AyntJi5wdQ9rlihFffOkRao40zC7Cv4wmbtP5nCI2lIjLyO/Poqeib86\n\t8840CNknOVGOx0+bqzi74vIIsW/W7X6kNcOIIbmpRI65K7tdVCj66ArCaRqKQxyy1S\n\t9/o1CR29ho2BQLjncE5c3KO6LcshcvCdEmeq6J+c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241126121706.4350-3-david.plowman@raspberrypi.com>","References":"<20241126121706.4350-1-david.plowman@raspberrypi.com>\n\t<20241126121706.4350-3-david.plowman@raspberrypi.com>","Subject":"Re: [RFC PATCH 2/3] libcamera: clock: Add ClockRecovery class to\n\thelp generate wallclock timestamps","From":"Kieran Bingham ","Cc":"David Plowman ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 05 Dec 2024 11:39:29 +0000","Message-ID":"<173339876982.3135963.3796854513088041144@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}}]