[{"id":32594,"web_url":"https://patchwork.libcamera.org/comment/32594/","msgid":"<173350021613.3135963.7261573657809888292@ping.linuxembedded.co.uk>","date":"2024-12-06T15:50:16","subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2024-12-06 14:27:39)\n> The ClockRecovery class takes pairs of timestamps from two different\n> clocks, and models the second (\"output\") clock from the first (\"input\")\n> clock.\n> \n> We can use it, in particular, to get a good wallclock estimate for a\n> frame's SensorTimestamp.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/clock_recovery.h | 72 +++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/clock_recovery.cpp | 207 ++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 281 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..c874574e\n> --- /dev/null\n> +++ b/include/libcamera/internal/clock_recovery.h\n> @@ -0,0 +1,72 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Raspberry Pi Ltd\n> + *\n> + * Camera recovery algorithm\n\nRecovering cameras ? ;-)\n\n> + */\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class ClockRecovery\n> +{\n> +public:\n> + ClockRecovery();\n> +\n> + /* Set configuration parameters. */\n> + void configure(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> + /*\n> + * Add a new input clock / output clock sample, taking the input from the Linux\n> + * CLOCK_BOOTTIME and the output from the CLOCK_REALTIME.\n> + */\n> + void addSample();\n> + /*\n> + * Add a new input clock / output clock sample, specifying the clock times exactly. Use this\n> + * when you want to use clocks other than the ones described above.\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> + 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> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 7d6aa8b7..41500636 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..966599ee\n> --- /dev/null\n> +++ b/src/libcamera/clock_recovery.cpp\n> @@ -0,0 +1,207 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Raspberry Pi Ltd\n> + *\n> + * Clock recovery algorithm\n> + */\n> +\n> +#include \"libcamera/internal/clock_recovery.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file clock_recovery.h\n> + * \\brief Clock recovery - deriving one clock from another independent clock\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(ClockRec)\n> +\n> +/**\n> + * \\class ClockRecovery\n> + * \\brief Recover an output clock from an input clock\n> + *\n> + * The ClockRecovery class derives an output clock from an input clock,\n> + * modelling the output clock as being linearly related to the input clock.\n> + * For example, we may use it to derive wall clock timestamps from timestamps\n> + * measured by the internal system clock which counts local time since boot.\n> + *\n> + * When pairs of corresponding input and output timestamps are available,\n> + * they should be submitted to the model with addSample(). The model will\n> + * update, and output clock values for known input clock values can be\n> + * obtained using getOutput().\n> + *\n> + * As a convenience, if the input clock is indeed the time since boot, and the\n> + * output clock represents a real wallclock time, then addSample() can be\n> + * called with no arguments, and a pair of timestamps will be captured at\n> + * that moment.\n> + *\n> + * The configure() function accepts some configuration parameters to control\n> + * the linear fitting process.\n> + */\n> +\n> +/**\n> + * \\brief Construct a ClockRecovery\n> + */\n> +ClockRecovery::ClockRecovery()\n> +{\n> + configure();\n> + reset();\n> +}\n> +\n> +/**\n> + * \\brief Set configuration parameters\n> + * \\param[in] numPts The approximate duration for which the state of the model\n> + * is persistent, measured in samples\n> + * \\param[in] maxJitter New output samples are clamped to no more than this\n> + * amount of jitter, to prevent sudden swings from having a large effect\n> + * \\param[in] minPts The fitted clock model is not used to generate outputs\n> + * until this many samples have been received\n> + * \\param[in] errorThreshold If the accumulated differences between input and\n> + * output clocks reaches this amount over a few frames, the model is reset\n> + */\n> +void ClockRecovery::configure(unsigned int numPts, unsigned int maxJitter, unsigned int minPts,\n> + unsigned int errorThreshold)\n> +{\n> + LOG(ClockRec, Debug)\n> + << \"configure \" << numPts << \" \" << maxJitter << \" \" << minPts << \" \" << errorThreshold;\n> +\n> + numPts_ = numPts;\n> + maxJitter_ = maxJitter;\n> + minPts_ = minPts;\n> + errorThreshold_ = errorThreshold;\n> +}\n> +\n> +/**\n> + * \\brief Reset the clock recovery model and start again from scratch\n> + */\n> +void ClockRecovery::reset()\n> +{\n> + LOG(ClockRec, Debug) << \"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\nShould slope be initialised to 1.0 or anything 'normalised' for any\nstartup conditions?\n\n> + offset_ = 0.0;\n> + error_ = 0.0;\n> +}\n> +\n> +/**\n> + * \\brief Add a sample point to the clock recovery model, for recovering a wall\n> + * clock value from the internal system time since boot\n> + *\n> + * This is a convenience function to make it easy to derive a wall clock value\n> + * (using the Linux CLOCK_REALTIME) from the time since the system started\n> + * (measured by CLOCK_BOOTTIME).\n> + */\n> +void ClockRecovery::addSample()\n> +{\n> + LOG(ClockRec, Debug) << \"addSample\";\n> +\n> + struct timespec bootTime;\n> + struct timespec wallTime;\n> +\n> + /* Get boot and wall clocks in microseconds. */\n> + clock_gettime(CLOCK_BOOTTIME, &bootTime);\n> + clock_gettime(CLOCK_REALTIME, &wallTime);\n> + uint64_t boot = bootTime.tv_sec * 1000000ULL + bootTime.tv_nsec / 1000;\n> + uint64_t wall = wallTime.tv_sec * 1000000ULL + wallTime.tv_nsec / 1000;\n> +\n> + addSample(boot, wall);\n> +}\n> +\n> +/**\n> + * \\brief Add a sample point to the clock recovery model, specifying the exact\n> + * input and output clock values\n> + *\n> + * This function should be used for corresponding clocks other than the Linux\n> + * BOOTTIME and REALTIME clocks.\n\nI like that this object has both an 'easy addSample()' and the paired\nversion.\n\nI think having the addSample() can define that it pairs BOOTTIME and\nREALTIME clocks, but this addSample(uint64_t input, uint64_t output)\nitself isn't actually tied to those clocks.\n\nIf this ClockRecovery object is used elsewhere, the 'only' reason I\ncould imagine this function being used is if it were to use a clock\nspecifically different to the defaults - so I'd probably drop the text\nhere, and keep the specification of BOOTTIME/REALTIME to the addSample()\n- or specific to the usage in V4L2VideoDevice.\n\n> +void ClockRecovery::addSample(uint64_t input, uint64_t output)\n\nAs an example that I'm not sure if it's crazy or not yet - I could\nimagine setting up a ClockRecovery object that maps sequence number and\ntimestamp so that you could determine the frame sequence number (output)\nfrom timestamp (input) which would give you quantised sequence numbers\ndetermined from the timestamp - so you could spot frame drops if a CSI\nreceiver always supplies monotonic sequence numbers. (which I think the\ni.MX8MP does... ?)\n\n\n\n\n> +{\n> + LOG(ClockRec, Debug) << \"addSample \" << input << \" \" << 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\nThroughout the series, there's lots of text that could easily be wrapped\nto 80 chars. It shows up a lot in my email because I have an 92 char\nterminal for my email which shows lots of lines wrapping, but it's not a\nhard requirement.\n\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 = static_cast(input - inputBase_);\n> + double y = static_cast(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\nWhat's the output from getOuput before this happens ? Anything\nproblematic? Or just things that can be ignored for the first few\nframes?\n\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> +/**\n> + * \\brief Calculate the output clock value according to the model from an input\n> + * clock value\n> + *\n> + * \\return Output clock value\n> + */\n> +uint64_t ClockRecovery::getOutput(uint64_t input)\n> +{\n> + double x = static_cast(input - inputBase_);\n> + double y = slope_ * x + offset_;\n> + uint64_t output = y + x + outputBase_;\n> +\n> + LOG(ClockRec, Debug) << \"getOutput \" << input << \" \" << output;\n> +\n> + return output;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 57fde8a8..4eaa1c8e 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 B2A56BE173\n\tfor ;\n\tFri, 6 Dec 2024 15:50:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9A5B67E17;\n\tFri, 6 Dec 2024 16:50:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 34AC466176\n\tfor ;\n\tFri, 6 Dec 2024 16:50:19 +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 A68A774C;\n\tFri, 6 Dec 2024 16:49:49 +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=\"ZFA/leDt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733500189;\n\tbh=lZx5AUw3wbkKTb5S2V96RrmrLuvv5+ZHnDuelfPvLW0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZFA/leDtGIm78cnFMSJoiiTpYXWbQIlapQfvAAvcXmyxX9aii3J34bQehDd+DLNW3\n\tYHzryygrT8tl0+4L9dRhtF3UgqI4PtwseJg7hYkkcb3CO+w0D7QVtOHgcdP4hvaweT\n\tc73dIjvwBdlaMsDspGChx9jKZNwHpD8Q7jO2cl+c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241206142742.7931-3-david.plowman@raspberrypi.com>","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-3-david.plowman@raspberrypi.com>","Subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","From":"Kieran Bingham ","Cc":"David Plowman ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 06 Dec 2024 15:50:16 +0000","Message-ID":"<173350021613.3135963.7261573657809888292@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\" "}},{"id":32597,"web_url":"https://patchwork.libcamera.org/comment/32597/","msgid":"","date":"2024-12-06T16:29:02","subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nThanks for the review!\n\nOn Fri, 6 Dec 2024 at 15:50, Kieran Bingham\n wrote:\n>\n> Quoting David Plowman (2024-12-06 14:27:39)\n> > The ClockRecovery class takes pairs of timestamps from two different\n> > clocks, and models the second (\"output\") clock from the first (\"input\")\n> > clock.\n> >\n> > We can use it, in particular, to get a good wallclock estimate for a\n> > frame's SensorTimestamp.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/clock_recovery.h | 72 +++++++\n> > include/libcamera/internal/meson.build | 1 +\n> > src/libcamera/clock_recovery.cpp | 207 ++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 281 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..c874574e\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/clock_recovery.h\n> > @@ -0,0 +1,72 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > + *\n> > + * Camera recovery algorithm\n>\n> Recovering cameras ? ;-)\n\nHehe. I'll fix that!!\n\n>\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class ClockRecovery\n> > +{\n> > +public:\n> > + ClockRecovery();\n> > +\n> > + /* Set configuration parameters. */\n> > + void configure(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> > + /*\n> > + * Add a new input clock / output clock sample, taking the input from the Linux\n> > + * CLOCK_BOOTTIME and the output from the CLOCK_REALTIME.\n> > + */\n> > + void addSample();\n> > + /*\n> > + * Add a new input clock / output clock sample, specifying the clock times exactly. Use this\n> > + * when you want to use clocks other than the ones described above.\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> > + 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> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 7d6aa8b7..41500636 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..966599ee\n> > --- /dev/null\n> > +++ b/src/libcamera/clock_recovery.cpp\n> > @@ -0,0 +1,207 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > + *\n> > + * Clock recovery algorithm\n> > + */\n> > +\n> > +#include \"libcamera/internal/clock_recovery.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file clock_recovery.h\n> > + * \\brief Clock recovery - deriving one clock from another independent clock\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(ClockRec)\n> > +\n> > +/**\n> > + * \\class ClockRecovery\n> > + * \\brief Recover an output clock from an input clock\n> > + *\n> > + * The ClockRecovery class derives an output clock from an input clock,\n> > + * modelling the output clock as being linearly related to the input clock.\n> > + * For example, we may use it to derive wall clock timestamps from timestamps\n> > + * measured by the internal system clock which counts local time since boot.\n> > + *\n> > + * When pairs of corresponding input and output timestamps are available,\n> > + * they should be submitted to the model with addSample(). The model will\n> > + * update, and output clock values for known input clock values can be\n> > + * obtained using getOutput().\n> > + *\n> > + * As a convenience, if the input clock is indeed the time since boot, and the\n> > + * output clock represents a real wallclock time, then addSample() can be\n> > + * called with no arguments, and a pair of timestamps will be captured at\n> > + * that moment.\n> > + *\n> > + * The configure() function accepts some configuration parameters to control\n> > + * the linear fitting process.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a ClockRecovery\n> > + */\n> > +ClockRecovery::ClockRecovery()\n> > +{\n> > + configure();\n> > + reset();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Set configuration parameters\n> > + * \\param[in] numPts The approximate duration for which the state of the model\n> > + * is persistent, measured in samples\n> > + * \\param[in] maxJitter New output samples are clamped to no more than this\n> > + * amount of jitter, to prevent sudden swings from having a large effect\n> > + * \\param[in] minPts The fitted clock model is not used to generate outputs\n> > + * until this many samples have been received\n> > + * \\param[in] errorThreshold If the accumulated differences between input and\n> > + * output clocks reaches this amount over a few frames, the model is reset\n> > + */\n> > +void ClockRecovery::configure(unsigned int numPts, unsigned int maxJitter, unsigned int minPts,\n> > + unsigned int errorThreshold)\n> > +{\n> > + LOG(ClockRec, Debug)\n> > + << \"configure \" << numPts << \" \" << maxJitter << \" \" << minPts << \" \" << errorThreshold;\n> > +\n> > + numPts_ = numPts;\n> > + maxJitter_ = maxJitter;\n> > + minPts_ = minPts;\n> > + errorThreshold_ = errorThreshold;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Reset the clock recovery model and start again from scratch\n> > + */\n> > +void ClockRecovery::reset()\n> > +{\n> > + LOG(ClockRec, Debug) << \"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>\n> Should slope be initialised to 1.0 or anything 'normalised' for any\n> startup conditions?\n\nActually, 0 is the value that ensures \"output = input\" (allowing for\nthe different starting offset).\nMaybe worth adding a comment to that end?\n\n>\n> > + offset_ = 0.0;\n> > + error_ = 0.0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Add a sample point to the clock recovery model, for recovering a wall\n> > + * clock value from the internal system time since boot\n> > + *\n> > + * This is a convenience function to make it easy to derive a wall clock value\n> > + * (using the Linux CLOCK_REALTIME) from the time since the system started\n> > + * (measured by CLOCK_BOOTTIME).\n> > + */\n> > +void ClockRecovery::addSample()\n> > +{\n> > + LOG(ClockRec, Debug) << \"addSample\";\n> > +\n> > + struct timespec bootTime;\n> > + struct timespec wallTime;\n> > +\n> > + /* Get boot and wall clocks in microseconds. */\n> > + clock_gettime(CLOCK_BOOTTIME, &bootTime);\n> > + clock_gettime(CLOCK_REALTIME, &wallTime);\n> > + uint64_t boot = bootTime.tv_sec * 1000000ULL + bootTime.tv_nsec / 1000;\n> > + uint64_t wall = wallTime.tv_sec * 1000000ULL + wallTime.tv_nsec / 1000;\n> > +\n> > + addSample(boot, wall);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Add a sample point to the clock recovery model, specifying the exact\n> > + * input and output clock values\n> > + *\n> > + * This function should be used for corresponding clocks other than the Linux\n> > + * BOOTTIME and REALTIME clocks.\n>\n> I like that this object has both an 'easy addSample()' and the paired\n> version.\n>\n> I think having the addSample() can define that it pairs BOOTTIME and\n> REALTIME clocks, but this addSample(uint64_t input, uint64_t output)\n> itself isn't actually tied to those clocks.\n>\n> If this ClockRecovery object is used elsewhere, the 'only' reason I\n> could imagine this function being used is if it were to use a clock\n> specifically different to the defaults - so I'd probably drop the text\n> here, and keep the specification of BOOTTIME/REALTIME to the addSample()\n> - or specific to the usage in V4L2VideoDevice.\n>\n> > +void ClockRecovery::addSample(uint64_t input, uint64_t output)\n>\n> As an example that I'm not sure if it's crazy or not yet - I could\n> imagine setting up a ClockRecovery object that maps sequence number and\n> timestamp so that you could determine the frame sequence number (output)\n> from timestamp (input) which would give you quantised sequence numbers\n> determined from the timestamp - so you could spot frame drops if a CSI\n> receiver always supplies monotonic sequence numbers. (which I think the\n> i.MX8MP does... ?)\n>\n\nInteresting thought, though I think I'd prefer to keep the basic\nClockRecovery restricted to, well, clocks. If someone wants to spot\nframe drops... obviously they're welcome to do that, but maybe that\ngets implemented on top? Remember of course that frame times may be\nvariable.\n\n>\n>\n>\n> > +{\n> > + LOG(ClockRec, Debug) << \"addSample \" << input << \" \" << 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>\n> Throughout the series, there's lots of text that could easily be wrapped\n> to 80 chars. It shows up a lot in my email because I have an 92 char\n> terminal for my email which shows lots of lines wrapping, but it's not a\n> hard requirement.\n\nI guess I'm not clear how strict we are about line lengths... mostly I\ndon't worry if the style checker doesn't complain. But I'm happy to do\na bit of reflowing if that helps, though some slightly more \"official\"\nguidance on when to do that would be helpful.\n\n>\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 = static_cast(input - inputBase_);\n> > + double y = static_cast(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>\n> What's the output from getOuput before this happens ? Anything\n> problematic? Or just things that can be ignored for the first few\n> frames?\n\nNo, you just get \"output = input\" allowing for the different starting\noffsets. Again, maybe worth a comment?\n\nThanks!\nDavid\n\n>\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> > +/**\n> > + * \\brief Calculate the output clock value according to the model from an input\n> > + * clock value\n> > + *\n> > + * \\return Output clock value\n> > + */\n> > +uint64_t ClockRecovery::getOutput(uint64_t input)\n> > +{\n> > + double x = static_cast(input - inputBase_);\n> > + double y = slope_ * x + offset_;\n> > + uint64_t output = y + x + outputBase_;\n> > +\n> > + LOG(ClockRec, Debug) << \"getOutput \" << input << \" \" << output;\n> > +\n> > + return output;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 57fde8a8..4eaa1c8e 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 60A81BF415\n\tfor ;\n\tFri, 6 Dec 2024 16:29:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2ACD467E23;\n\tFri, 6 Dec 2024 17:29:16 +0100 (CET)","from mail-qv1-xf2d.google.com (mail-qv1-xf2d.google.com\n\t[IPv6:2607:f8b0:4864:20::f2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D119367E1C\n\tfor ;\n\tFri, 6 Dec 2024 17:29:14 +0100 (CET)","by mail-qv1-xf2d.google.com with SMTP id\n\t6a1803df08f44-6d8edbd1a3bso4929676d6.3\n\tfor ;\n\tFri, 06 Dec 2024 08:29:14 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"IZhri292\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1733502554; x=1734107354;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=tne9ifNaG8e04emAT6BXg7gL/uVN8tfJnj15e56RikI=;\n\tb=IZhri292ZIYXFK2+eS8nk9JKKnSgJeQlpLuN1VLRgOYa2t99nsBz/bG2AuylNMrSkI\n\t9wpWwZrL3JSALkZbcWajBUEZwjhxrwdfnY8JS0/cTuWsyK2QAX74NGLpjWSgJzInb/x6\n\tK7C3kVjBMH0PICYbkUvMC0Wi4FLZrLpGolk04h2Gkuz5kEh+EsqlJgzspd3fpcpGG3Ez\n\tUsyr5caIYaIa6tSr0p3jChnEli+fqK2yvXuXSn+qoZ6PjUMpuevfaCRoDDPnkvv2fJrN\n\tEAc1PHuI6KEbrXoY4DzsVX3POmZbn88hY29xN4ooo7+xKY0rtU2oOJxD6cwjBJ4HUBJk\n\taoxg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733502554; x=1734107354;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=tne9ifNaG8e04emAT6BXg7gL/uVN8tfJnj15e56RikI=;\n\tb=BqcFKA4tgNEqxKR+fY9TAxOgFAy2tSrx9MjVW9JtrA6fnDFiFtLlERSDnMBhGIKcQX\n\t6RIio/YQaAYMWGVIqS5QvYKTt47Y/UdxY+IeBhz4naQNWINtH/t/78XmfA6jMrl1kSUB\n\tQiYdg9VmTvOYJweJESaLtQPxt+qvJae5eFlWDXi6UABWHWmDkyGqJcLndBiWUyey0X9Q\n\tryQvhcRv0k1PG5VpWCp4nQVWizzj91kSQyYm7R52cOXcaO41SoxdRYQ6mRrxXM4c3HSW\n\tDHecOpwEofhE9x6M8NyUKduLM2+Hdl3UjVJAHbM5NLEb/ZHGgV7AaiF2AZVNMQQCkETb\n\tHzkQ==","X-Gm-Message-State":"AOJu0YxNpwp0L4XRQllVJFfw13cay0bDeCJkVJrCJgMFNAUe40qeuTP5\n\tv/hrA7AzH/GpT4D36tdHxoaos67rZj5LEEoPPanB+lH+cSeJwflJihohYCWo5Tpwvu2Abv01OnM\n\tUOGDB4ypl5mBiMjbdVqg6pGUMP1HFn2MeyAboasN9el02Norq","X-Gm-Gg":"ASbGnctMzJ9bqMgREm0oTgJjGG22jL77ULvsNObqNBiwyVgQ2PrjqcMMp0TuhrK5ipk\n\tjH7pj44IrI+Mjw20DvREOPmbKs5MqH2E=","X-Google-Smtp-Source":"AGHT+IGaRHcnXgkB8DF+OYKO867kVIQsng/UvrwpSt7iShgLAq9wb8Yjt4yxFaFmOG9ui7Vc5ha3FkPqYYktO5Zbjjw=","X-Received":"by 2002:a05:6214:5299:b0:6d4:2910:7f13 with SMTP id\n\t6a1803df08f44-6d8e71dc135mr45005306d6.32.1733502553638;\n\tFri, 06 Dec 2024 08:29:13 -0800 (PST)","MIME-Version":"1.0","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-3-david.plowman@raspberrypi.com>\n\t<173350021613.3135963.7261573657809888292@ping.linuxembedded.co.uk>","In-Reply-To":"<173350021613.3135963.7261573657809888292@ping.linuxembedded.co.uk>","From":"David Plowman ","Date":"Fri, 6 Dec 2024 16:29:02 +0000","Message-ID":"","Subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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":32600,"web_url":"https://patchwork.libcamera.org/comment/32600/","msgid":"<173350342329.3135963.11307611673106150546@ping.linuxembedded.co.uk>","date":"2024-12-06T16:43:43","subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2024-12-06 16:29:02)\n> Hi Kieran\n> \n> Thanks for the review!\n> \n> On Fri, 6 Dec 2024 at 15:50, Kieran Bingham\n> wrote:\n> >\n> > Quoting David Plowman (2024-12-06 14:27:39)\n> > > The ClockRecovery class takes pairs of timestamps from two different\n> > > clocks, and models the second (\"output\") clock from the first (\"input\")\n> > > clock.\n> > >\n> > > We can use it, in particular, to get a good wallclock estimate for a\n> > > frame's SensorTimestamp.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/clock_recovery.h | 72 +++++++\n> > > include/libcamera/internal/meson.build | 1 +\n> > > src/libcamera/clock_recovery.cpp | 207 ++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 4 files changed, 281 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..c874574e\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/clock_recovery.h\n> > > @@ -0,0 +1,72 @@\n> > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > +/*\n> > > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > > + *\n> > > + * Camera recovery algorithm\n> >\n> > Recovering cameras ? ;-)\n> \n> Hehe. I'll fix that!!\n> \n> >\n> > > + */\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class ClockRecovery\n> > > +{\n> > > +public:\n> > > + ClockRecovery();\n> > > +\n> > > + /* Set configuration parameters. */\n> > > + void configure(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> > > + /*\n> > > + * Add a new input clock / output clock sample, taking the input from the Linux\n> > > + * CLOCK_BOOTTIME and the output from the CLOCK_REALTIME.\n> > > + */\n> > > + void addSample();\n> > > + /*\n> > > + * Add a new input clock / output clock sample, specifying the clock times exactly. Use this\n> > > + * when you want to use clocks other than the ones described above.\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> > > + 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> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > index 7d6aa8b7..41500636 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..966599ee\n> > > --- /dev/null\n> > > +++ b/src/libcamera/clock_recovery.cpp\n> > > @@ -0,0 +1,207 @@\n> > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > +/*\n> > > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > > + *\n> > > + * Clock recovery algorithm\n> > > + */\n> > > +\n> > > +#include \"libcamera/internal/clock_recovery.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +/**\n> > > + * \\file clock_recovery.h\n> > > + * \\brief Clock recovery - deriving one clock from another independent clock\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(ClockRec)\n> > > +\n> > > +/**\n> > > + * \\class ClockRecovery\n> > > + * \\brief Recover an output clock from an input clock\n> > > + *\n> > > + * The ClockRecovery class derives an output clock from an input clock,\n> > > + * modelling the output clock as being linearly related to the input clock.\n> > > + * For example, we may use it to derive wall clock timestamps from timestamps\n> > > + * measured by the internal system clock which counts local time since boot.\n> > > + *\n> > > + * When pairs of corresponding input and output timestamps are available,\n> > > + * they should be submitted to the model with addSample(). The model will\n> > > + * update, and output clock values for known input clock values can be\n> > > + * obtained using getOutput().\n> > > + *\n> > > + * As a convenience, if the input clock is indeed the time since boot, and the\n> > > + * output clock represents a real wallclock time, then addSample() can be\n> > > + * called with no arguments, and a pair of timestamps will be captured at\n> > > + * that moment.\n> > > + *\n> > > + * The configure() function accepts some configuration parameters to control\n> > > + * the linear fitting process.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct a ClockRecovery\n> > > + */\n> > > +ClockRecovery::ClockRecovery()\n> > > +{\n> > > + configure();\n> > > + reset();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Set configuration parameters\n> > > + * \\param[in] numPts The approximate duration for which the state of the model\n> > > + * is persistent, measured in samples\n> > > + * \\param[in] maxJitter New output samples are clamped to no more than this\n> > > + * amount of jitter, to prevent sudden swings from having a large effect\n> > > + * \\param[in] minPts The fitted clock model is not used to generate outputs\n> > > + * until this many samples have been received\n> > > + * \\param[in] errorThreshold If the accumulated differences between input and\n> > > + * output clocks reaches this amount over a few frames, the model is reset\n> > > + */\n> > > +void ClockRecovery::configure(unsigned int numPts, unsigned int maxJitter, unsigned int minPts,\n> > > + unsigned int errorThreshold)\n> > > +{\n> > > + LOG(ClockRec, Debug)\n> > > + << \"configure \" << numPts << \" \" << maxJitter << \" \" << minPts << \" \" << errorThreshold;\n> > > +\n> > > + numPts_ = numPts;\n> > > + maxJitter_ = maxJitter;\n> > > + minPts_ = minPts;\n> > > + errorThreshold_ = errorThreshold;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Reset the clock recovery model and start again from scratch\n> > > + */\n> > > +void ClockRecovery::reset()\n> > > +{\n> > > + LOG(ClockRec, Debug) << \"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> >\n> > Should slope be initialised to 1.0 or anything 'normalised' for any\n> > startup conditions?\n> \n> Actually, 0 is the value that ensures \"output = input\" (allowing for\n> the different starting offset).\n> Maybe worth adding a comment to that end?\n\naha - no - that's probably fine - it's just me misunderstanding. For\nsome reason I thought '1' would be the more expected slope for\nundefined/uninitialised, but I see '0' just removes the slope which is\n(until samples are ready...) undefined ;-)\n\n\n\n> \n> >\n> > > + offset_ = 0.0;\n> > > + error_ = 0.0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Add a sample point to the clock recovery model, for recovering a wall\n> > > + * clock value from the internal system time since boot\n> > > + *\n> > > + * This is a convenience function to make it easy to derive a wall clock value\n> > > + * (using the Linux CLOCK_REALTIME) from the time since the system started\n> > > + * (measured by CLOCK_BOOTTIME).\n> > > + */\n> > > +void ClockRecovery::addSample()\n> > > +{\n> > > + LOG(ClockRec, Debug) << \"addSample\";\n> > > +\n> > > + struct timespec bootTime;\n> > > + struct timespec wallTime;\n> > > +\n> > > + /* Get boot and wall clocks in microseconds. */\n> > > + clock_gettime(CLOCK_BOOTTIME, &bootTime);\n> > > + clock_gettime(CLOCK_REALTIME, &wallTime);\n> > > + uint64_t boot = bootTime.tv_sec * 1000000ULL + bootTime.tv_nsec / 1000;\n> > > + uint64_t wall = wallTime.tv_sec * 1000000ULL + wallTime.tv_nsec / 1000;\n> > > +\n> > > + addSample(boot, wall);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Add a sample point to the clock recovery model, specifying the exact\n> > > + * input and output clock values\n> > > + *\n> > > + * This function should be used for corresponding clocks other than the Linux\n> > > + * BOOTTIME and REALTIME clocks.\n> >\n> > I like that this object has both an 'easy addSample()' and the paired\n> > version.\n> >\n> > I think having the addSample() can define that it pairs BOOTTIME and\n> > REALTIME clocks, but this addSample(uint64_t input, uint64_t output)\n> > itself isn't actually tied to those clocks.\n> >\n> > If this ClockRecovery object is used elsewhere, the 'only' reason I\n> > could imagine this function being used is if it were to use a clock\n> > specifically different to the defaults - so I'd probably drop the text\n> > here, and keep the specification of BOOTTIME/REALTIME to the addSample()\n> > - or specific to the usage in V4L2VideoDevice.\n> >\n> > > +void ClockRecovery::addSample(uint64_t input, uint64_t output)\n> >\n> > As an example that I'm not sure if it's crazy or not yet - I could\n> > imagine setting up a ClockRecovery object that maps sequence number and\n> > timestamp so that you could determine the frame sequence number (output)\n> > from timestamp (input) which would give you quantised sequence numbers\n> > determined from the timestamp - so you could spot frame drops if a CSI\n> > receiver always supplies monotonic sequence numbers. (which I think the\n> > i.MX8MP does... ?)\n> >\n> \n> Interesting thought, though I think I'd prefer to keep the basic\n> ClockRecovery restricted to, well, clocks. If someone wants to spot\n> frame drops... obviously they're welcome to do that, but maybe that\n> gets implemented on top? Remember of course that frame times may be\n> variable.\n\nOh absolutly - I'm not expecting anything here - I'm just speculating on\nwhat crazy things we could match between input/output to get the linear\nregression on ;-)\n\nAnd that's why I think \"ClockRecovery::addSample(uint64_t input, uint64_t\noutput)\" as a function with two parameters isn't itself tied to a\nspecific clock. (but the usage in V4L2VideoDevice, and addSample() is).\n\n\n> \n> >\n> >\n> >\n> > > +{\n> > > + LOG(ClockRec, Debug) << \"addSample \" << input << \" \" << 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> >\n> > Throughout the series, there's lots of text that could easily be wrapped\n> > to 80 chars. It shows up a lot in my email because I have an 92 char\n> > terminal for my email which shows lots of lines wrapping, but it's not a\n> > hard requirement.\n> \n> I guess I'm not clear how strict we are about line lengths... mostly I\n> don't worry if the style checker doesn't complain. But I'm happy to do\n> a bit of reflowing if that helps, though some slightly more \"official\"\n> guidance on when to do that would be helpful.\n\nI think we normally aim for blocks of 80 - but the exceptions are up to\n120 if it helps/makes sense in that instance.\n\nIt only shows up in this series, as I think everything is targetting\n~100/120ish instead.\n\n\n\n> \n> >\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 = static_cast(input - inputBase_);\n> > > + double y = static_cast(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> >\n> > What's the output from getOuput before this happens ? Anything\n> > problematic? Or just things that can be ignored for the first few\n> > frames?\n> \n> No, you just get \"output = input\" allowing for the different starting\n> offsets. Again, maybe worth a comment?\n\nHrm, ok so maybe it might be useful to state 'somewhere' that the system\nwill produce output values matching the input value until sufficient\nsampling points have been collected.\n\nAnd following that back, I see minPts_ is configurable (which is a good\nthing :D), so it's up to the use case to determine what the minimum\nrequirements are.\n\nSeems pretty good to me.\n\n> \n> Thanks!\n> David\n> \n> >\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> > > +/**\n> > > + * \\brief Calculate the output clock value according to the model from an input\n> > > + * clock value\n> > > + *\n> > > + * \\return Output clock value\n> > > + */\n> > > +uint64_t ClockRecovery::getOutput(uint64_t input)\n> > > +{\n> > > + double x = static_cast(input - inputBase_);\n> > > + double y = slope_ * x + offset_;\n> > > + uint64_t output = y + x + outputBase_;\n> > > +\n> > > + LOG(ClockRec, Debug) << \"getOutput \" << input << \" \" << output;\n> > > +\n> > > + return output;\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 57fde8a8..4eaa1c8e 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 DF2E4BF415\n\tfor ;\n\tFri, 6 Dec 2024 16:43:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2005667E22;\n\tFri, 6 Dec 2024 17:43:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C2B0618B3\n\tfor ;\n\tFri, 6 Dec 2024 17:43:46 +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 67FB9641;\n\tFri, 6 Dec 2024 17:43:16 +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=\"JpvfxvMK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733503396;\n\tbh=XA8N4RnY0QFgzBcowKQe8ozZtZh0uubm8mlEw3BOx2U=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JpvfxvMKYw5yyNwxM1Ocx1nPoTqkJ7NEKJOhLqLWEtf4htw7XAdekNfKk0dUL91fL\n\tszZyC+JgIYRToJAeb4E/SraoRN8yEaGd0D8HIZED2PbVacDboml5Elg7g2h1hcB0+s\n\tC1I+UefI9IWFmJMdCUc9P6bxia6eplwTjHyapYDw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-3-david.plowman@raspberrypi.com>\n\t<173350021613.3135963.7261573657809888292@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"David Plowman ","Date":"Fri, 06 Dec 2024 16:43:43 +0000","Message-ID":"<173350342329.3135963.11307611673106150546@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\" "}},{"id":32709,"web_url":"https://patchwork.libcamera.org/comment/32709/","msgid":"","date":"2024-12-12T17:56:11","subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. december 6., péntek 15:27 keltezéssel, David Plowman írta:\n\n> The ClockRecovery class takes pairs of timestamps from two different\n> clocks, and models the second (\"output\") clock from the first (\"input\")\n> clock.\n> \n> We can use it, in particular, to get a good wallclock estimate for a\n> frame's SensorTimestamp.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/clock_recovery.h | 72 +++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/clock_recovery.cpp | 207 ++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 281 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..c874574e\n> --- /dev/null\n> +++ b/include/libcamera/internal/clock_recovery.h\n> @@ -0,0 +1,72 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Raspberry Pi Ltd\n> + *\n> + * Camera recovery algorithm\n> + */\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class ClockRecovery\n> +{\n> +public:\n> +\tClockRecovery();\n> +\n> +\t/* Set configuration parameters. */\n> +\tvoid configure(unsigned int numPts = 100, unsigned int maxJitter = 2000, unsigned int minPts = 10,\n> +\t\t unsigned int errorThreshold = 50000);\n> +\t/* Erase all history and restart the fitting process. */\n> +\tvoid reset();\n> +\n> +\t/*\n> +\t * Add a new input clock / output clock sample, taking the input from the Linux\n> +\t * CLOCK_BOOTTIME and the output from the CLOCK_REALTIME.\n> +\t */\n> +\tvoid addSample();\n> +\t/*\n> +\t * Add a new input clock / output clock sample, specifying the clock times exactly. Use this\n> +\t * when you want to use clocks other than the ones described above.\n> +\t */\n> +\tvoid addSample(uint64_t input, uint64_t output);\n> +\t/* Calculate the output clock value for this input. */\n> +\tuint64_t getOutput(uint64_t input);\n> +\n> +private:\n> +\tunsigned int numPts_; /* how many samples contribute to the history */\n> +\tunsigned int maxJitter_; /* smooth out any jitter larger than this immediately */\n> +\tunsigned int minPts_; /* number of samples below which we treat clocks as 1:1 */\n> +\tunsigned int errorThreshold_; /* reset everything when the error exceeds this */\n> +\n> +\tunsigned int count_; /* how many samples seen (up to numPts_) */\n> +\tuint64_t inputBase_; /* subtract this from all input values, just to make the numbers easier */\n> +\tuint64_t outputBase_; /* as above, for the output */\n> +\n> +\tuint64_t lastInput_; /* the previous input sample */\n> +\tuint64_t lastOutput_; /* the previous output sample */\n> +\n> +\t/*\n> +\t * We do a linear regression of y against x, where:\n> +\t * x is the value input - inputBase_, and\n> +\t * y is the value output - outputBase_ - x.\n> +\t * We additionally subtract x from y so that y \"should\" be zero, again making the numnbers easier.\n> +\t */\n> +\tdouble xAve_; /* average x value seen so far */\n> +\tdouble yAve_; /* average y value seen so far */\n> +\tdouble x2Ave_; /* average x^2 value seen so far */\n> +\tdouble xyAve_; /* average x*y value seen so far */\n> +\n> +\t/*\n> +\t * Once we've seen more than minPts_ samples, we recalculate the slope and offset according\n> +\t * to the linear regression normal equations.\n> +\t */\n> +\tdouble slope_; /* latest slope value */\n> +\tdouble offset_; /* latest offset value */\n> +\n> +\t/* We use this cumulative error to monitor spontaneous system clock updates. */\n> +\tdouble error_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 7d6aa8b7..41500636 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..966599ee\n> --- /dev/null\n> +++ b/src/libcamera/clock_recovery.cpp\n> @@ -0,0 +1,207 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Raspberry Pi Ltd\n> + *\n> + * Clock recovery algorithm\n> + */\n> +\n> +#include \"libcamera/internal/clock_recovery.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file clock_recovery.h\n> + * \\brief Clock recovery - deriving one clock from another independent clock\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(ClockRec)\n> +\n> +/**\n> + * \\class ClockRecovery\n> + * \\brief Recover an output clock from an input clock\n> + *\n> + * The ClockRecovery class derives an output clock from an input clock,\n> + * modelling the output clock as being linearly related to the input clock.\n> + * For example, we may use it to derive wall clock timestamps from timestamps\n> + * measured by the internal system clock which counts local time since boot.\n> + *\n> + * When pairs of corresponding input and output timestamps are available,\n> + * they should be submitted to the model with addSample(). The model will\n> + * update, and output clock values for known input clock values can be\n> + * obtained using getOutput().\n> + *\n> + * As a convenience, if the input clock is indeed the time since boot, and the\n> + * output clock represents a real wallclock time, then addSample() can be\n> + * called with no arguments, and a pair of timestamps will be captured at\n> + * that moment.\n> + *\n> + * The configure() function accepts some configuration parameters to control\n> + * the linear fitting process.\n> + */\n> +\n> +/**\n> + * \\brief Construct a ClockRecovery\n> + */\n> +ClockRecovery::ClockRecovery()\n> +{\n> +\tconfigure();\n> +\treset();\n> +}\n> +\n> +/**\n> + * \\brief Set configuration parameters\n> + * \\param[in] numPts The approximate duration for which the state of the model\n> + * is persistent, measured in samples\n> + * \\param[in] maxJitter New output samples are clamped to no more than this\n> + * amount of jitter, to prevent sudden swings from having a large effect\n> + * \\param[in] minPts The fitted clock model is not used to generate outputs\n> + * until this many samples have been received\n> + * \\param[in] errorThreshold If the accumulated differences between input and\n> + * output clocks reaches this amount over a few frames, the model is reset\n> + */\n> +void ClockRecovery::configure(unsigned int numPts, unsigned int maxJitter, unsigned int minPts,\n> +\t\t\t unsigned int errorThreshold)\n> +{\n> +\tLOG(ClockRec, Debug)\n> +\t\t<< \"configure \" << numPts << \" \" << maxJitter << \" \" << minPts << \" \" << errorThreshold;\n> +\n> +\tnumPts_ = numPts;\n> +\tmaxJitter_ = maxJitter;\n> +\tminPts_ = minPts;\n> +\terrorThreshold_ = errorThreshold;\n> +}\n> +\n> +/**\n> + * \\brief Reset the clock recovery model and start again from scratch\n> + */\n> +void ClockRecovery::reset()\n> +{\n> +\tLOG(ClockRec, Debug) << \"reset\";\n> +\n> +\tlastInput_ = 0;\n> +\tlastOutput_ = 0;\n> +\txAve_ = 0;\n> +\tyAve_ = 0;\n> +\tx2Ave_ = 0;\n> +\txyAve_ = 0;\n> +\tcount_ = 0;\n> +\tslope_ = 0.0;\n> +\toffset_ = 0.0;\n> +\terror_ = 0.0;\n> +}\n> +\n> +/**\n> + * \\brief Add a sample point to the clock recovery model, for recovering a wall\n> + * clock value from the internal system time since boot\n> + *\n> + * This is a convenience function to make it easy to derive a wall clock value\n> + * (using the Linux CLOCK_REALTIME) from the time since the system started\n> + * (measured by CLOCK_BOOTTIME).\n> + */\n> +void ClockRecovery::addSample()\n> +{\n> +\tLOG(ClockRec, Debug) << \"addSample\";\n> +\n> +\tstruct timespec bootTime;\n> +\tstruct timespec wallTime;\n> +\n> +\t/* Get boot and wall clocks in microseconds. */\n> +\tclock_gettime(CLOCK_BOOTTIME, &bootTime);\n> +\tclock_gettime(CLOCK_REALTIME, &wallTime);\n> +\tuint64_t boot = bootTime.tv_sec * 1000000ULL + bootTime.tv_nsec / 1000;\n> +\tuint64_t wall = wallTime.tv_sec * 1000000ULL + wallTime.tv_nsec / 1000;\n\nIt could be that I am missing something that accounts for this, but I am wondering\nif it would make sense to sample one of the clocks twice, and average the two samples.\ni.e.\n\n x1 = clock_boottime()\n y = clock_realtime()\n x2 = clock_boottime()\n\n addSample(midpoint(x1, x2), y)\n\nOtherwise I'd expect a constant offset to be present in the mapping, although\nI am not sure if that makes an appreciable difference.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> +\n> +\taddSample(boot, wall);\n> +}\n> +\n> +/**\n> + * \\brief Add a sample point to the clock recovery model, specifying the exact\n> + * input and output clock values\n> + *\n> + * This function should be used for corresponding clocks other than the Linux\n> + * BOOTTIME and REALTIME clocks.\n> + */\n> +void ClockRecovery::addSample(uint64_t input, uint64_t output)\n> +{\n> +\tLOG(ClockRec, Debug) << \"addSample \" << input << \" \" << output;\n> +\n> +\tif (count_ == 0) {\n> +\t\tinputBase_ = input;\n> +\t\toutputBase_ = output;\n> +\t}\n> +\n> +\t/*\n> +\t * We keep an eye on cumulative drift over the last several frames. If this exceeds a\n> +\t * threshold, then probably the system clock has been updated and we're going to have to\n> +\t * reset everything and start over.\n> +\t */\n> +\tif (lastOutput_) {\n> +\t\tint64_t inputDiff = getOutput(input) - getOutput(lastInput_);\n> +\t\tint64_t outputDiff = output - lastOutput_;\n> +\t\terror_ = error_ * 0.95 + (outputDiff - inputDiff);\n> +\t\tif (std::abs(error_) > errorThreshold_) {\n> +\t\t\treset();\n> +\t\t\tinputBase_ = input;\n> +\t\t\toutputBase_ = output;\n> +\t\t}\n> +\t}\n> +\tlastInput_ = input;\n> +\tlastOutput_ = output;\n> +\n> +\t/*\n> +\t * Never let the new output value be more than maxJitter_ away from what we would have expected.\n> +\t * This is just to reduce the effect of sudden large delays in the measured output.\n> +\t */\n> +\tuint64_t expectedOutput = getOutput(input);\n> +\toutput = std::clamp(output, expectedOutput - maxJitter_, expectedOutput + maxJitter_);\n> +\n> +\t/*\n> +\t * We use x, y, x^2 and x*y sums to calculate the best fit line. Here we update them by\n> +\t * pretending we have count_ samples at the previous fit, and now one new one. Gradually\n> +\t * the effect of the older values gets lost. This is a very simple way of updating the\n> +\t * fit (there are much more complicated ones!), but it works well enough. Using averages\n> +\t * instead of sums makes the relative effect of old values and the new sample clearer.\n> +\t */\n> +\tdouble x = static_cast(input - inputBase_);\n> +\tdouble y = static_cast(output - outputBase_) - x;\n> +\tunsigned int count1 = count_ + 1;\n> +\txAve_ = (count_ * xAve_ + x) / count1;\n> +\tyAve_ = (count_ * yAve_ + y) / count1;\n> +\tx2Ave_ = (count_ * x2Ave_ + x * x) / count1;\n> +\txyAve_ = (count_ * xyAve_ + x * y) / count1;\n> +\n> +\t/* Don't update slope and offset until we've seen \"enough\" sample points. */\n> +\tif (count_ > minPts_) {\n> +\t\t/* These are the standard equations for least squares linear regression. */\n> +\t\tslope_ = (count1 * count1 * xyAve_ - count1 * xAve_ * count1 * yAve_) /\n> +\t\t\t (count1 * count1 * x2Ave_ - count1 * xAve_ * count1 * xAve_);\n> +\t\toffset_ = yAve_ - slope_ * xAve_;\n> +\t}\n> +\n> +\t/* Don't increase count_ above numPts_, as this controls the long-term amount of the residual fit. */\n> +\tif (count1 < numPts_)\n> +\t\tcount_++;\n> +}\n> +\n> +/**\n> + * \\brief Calculate the output clock value according to the model from an input\n> + * clock value\n> + *\n> + * \\return Output clock value\n> + */\n> +uint64_t ClockRecovery::getOutput(uint64_t input)\n> +{\n> +\tdouble x = static_cast(input - inputBase_);\n> +\tdouble y = slope_ * x + offset_;\n> +\tuint64_t output = y + x + outputBase_;\n> +\n> +\tLOG(ClockRec, Debug) << \"getOutput \" << input << \" \" << output;\n> +\n> +\treturn output;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 57fde8a8..4eaa1c8e 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","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 7E171C3260\n\tfor ;\n\tThu, 12 Dec 2024 17:56:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B61F267EDA;\n\tThu, 12 Dec 2024 18:56:16 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC8A1608B6\n\tfor ;\n\tThu, 12 Dec 2024 18:56:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"WnNhaRGJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1734026174; x=1734285374;\n\tbh=HUU+d71TvTEsxabOs4KpzUZvmae+VHiGD8rdk4CVXA0=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=WnNhaRGJ6PBPmD5W97qB8gfYsbGIUVik8WizB05QEFtYHmgAzSezCTip+6JuiEzYn\n\t0d89LscOjkdICNKlwmeGMfOp4KLLDo4oWTGfdvWYbXmClkhxwyHiEQTOYhLatNjESs\n\tWQ8qhe4VFvyE9K4ZvESl2j7AnTzGdQmdcdZQbuviNTj0Sl7CLprYrAYO9BpeQSIoxh\n\t1sHR9lh1tbUpm9kRrwOd5xAvcQxwyHHOJ152z3V6xQqWGWkWCZFP5u0lMTpH1uwnK4\n\tWC4sFBQhXCFYK2hR/HOuvW89f10I9+E7HCyQBL2a/W3QvXj6oRdEO8yhkFjSt1bNkm\n\te+E9iLKYl6leQ==","Date":"Thu, 12 Dec 2024 17:56:11 +0000","To":"David Plowman ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","Message-ID":"","In-Reply-To":"<20241206142742.7931-3-david.plowman@raspberrypi.com>","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-3-david.plowman@raspberrypi.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"368bcccbdca3545a26e9ece4081b2bbc011c9709","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":32723,"web_url":"https://patchwork.libcamera.org/comment/32723/","msgid":"","date":"2024-12-13T09:21:32","subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Barnabas\n\nThanks for looking at this!\n\nOn Thu, 12 Dec 2024 at 17:56, Barnabás Pőcze wrote:\n>\n> Hi\n>\n>\n> 2024. december 6., péntek 15:27 keltezéssel, David Plowman írta:\n>\n> > The ClockRecovery class takes pairs of timestamps from two different\n> > clocks, and models the second (\"output\") clock from the first (\"input\")\n> > clock.\n> >\n> > We can use it, in particular, to get a good wallclock estimate for a\n> > frame's SensorTimestamp.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/clock_recovery.h | 72 +++++++\n> > include/libcamera/internal/meson.build | 1 +\n> > src/libcamera/clock_recovery.cpp | 207 ++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 281 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..c874574e\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/clock_recovery.h\n> > @@ -0,0 +1,72 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > + *\n> > + * Camera recovery algorithm\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class ClockRecovery\n> > +{\n> > +public:\n> > + ClockRecovery();\n> > +\n> > + /* Set configuration parameters. */\n> > + void configure(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> > + /*\n> > + * Add a new input clock / output clock sample, taking the input from the Linux\n> > + * CLOCK_BOOTTIME and the output from the CLOCK_REALTIME.\n> > + */\n> > + void addSample();\n> > + /*\n> > + * Add a new input clock / output clock sample, specifying the clock times exactly. Use this\n> > + * when you want to use clocks other than the ones described above.\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> > + 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> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 7d6aa8b7..41500636 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..966599ee\n> > --- /dev/null\n> > +++ b/src/libcamera/clock_recovery.cpp\n> > @@ -0,0 +1,207 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > + *\n> > + * Clock recovery algorithm\n> > + */\n> > +\n> > +#include \"libcamera/internal/clock_recovery.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file clock_recovery.h\n> > + * \\brief Clock recovery - deriving one clock from another independent clock\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(ClockRec)\n> > +\n> > +/**\n> > + * \\class ClockRecovery\n> > + * \\brief Recover an output clock from an input clock\n> > + *\n> > + * The ClockRecovery class derives an output clock from an input clock,\n> > + * modelling the output clock as being linearly related to the input clock.\n> > + * For example, we may use it to derive wall clock timestamps from timestamps\n> > + * measured by the internal system clock which counts local time since boot.\n> > + *\n> > + * When pairs of corresponding input and output timestamps are available,\n> > + * they should be submitted to the model with addSample(). The model will\n> > + * update, and output clock values for known input clock values can be\n> > + * obtained using getOutput().\n> > + *\n> > + * As a convenience, if the input clock is indeed the time since boot, and the\n> > + * output clock represents a real wallclock time, then addSample() can be\n> > + * called with no arguments, and a pair of timestamps will be captured at\n> > + * that moment.\n> > + *\n> > + * The configure() function accepts some configuration parameters to control\n> > + * the linear fitting process.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a ClockRecovery\n> > + */\n> > +ClockRecovery::ClockRecovery()\n> > +{\n> > + configure();\n> > + reset();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Set configuration parameters\n> > + * \\param[in] numPts The approximate duration for which the state of the model\n> > + * is persistent, measured in samples\n> > + * \\param[in] maxJitter New output samples are clamped to no more than this\n> > + * amount of jitter, to prevent sudden swings from having a large effect\n> > + * \\param[in] minPts The fitted clock model is not used to generate outputs\n> > + * until this many samples have been received\n> > + * \\param[in] errorThreshold If the accumulated differences between input and\n> > + * output clocks reaches this amount over a few frames, the model is reset\n> > + */\n> > +void ClockRecovery::configure(unsigned int numPts, unsigned int maxJitter, unsigned int minPts,\n> > + unsigned int errorThreshold)\n> > +{\n> > + LOG(ClockRec, Debug)\n> > + << \"configure \" << numPts << \" \" << maxJitter << \" \" << minPts << \" \" << errorThreshold;\n> > +\n> > + numPts_ = numPts;\n> > + maxJitter_ = maxJitter;\n> > + minPts_ = minPts;\n> > + errorThreshold_ = errorThreshold;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Reset the clock recovery model and start again from scratch\n> > + */\n> > +void ClockRecovery::reset()\n> > +{\n> > + LOG(ClockRec, Debug) << \"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> > +/**\n> > + * \\brief Add a sample point to the clock recovery model, for recovering a wall\n> > + * clock value from the internal system time since boot\n> > + *\n> > + * This is a convenience function to make it easy to derive a wall clock value\n> > + * (using the Linux CLOCK_REALTIME) from the time since the system started\n> > + * (measured by CLOCK_BOOTTIME).\n> > + */\n> > +void ClockRecovery::addSample()\n> > +{\n> > + LOG(ClockRec, Debug) << \"addSample\";\n> > +\n> > + struct timespec bootTime;\n> > + struct timespec wallTime;\n> > +\n> > + /* Get boot and wall clocks in microseconds. */\n> > + clock_gettime(CLOCK_BOOTTIME, &bootTime);\n> > + clock_gettime(CLOCK_REALTIME, &wallTime);\n> > + uint64_t boot = bootTime.tv_sec * 1000000ULL + bootTime.tv_nsec / 1000;\n> > + uint64_t wall = wallTime.tv_sec * 1000000ULL + wallTime.tv_nsec / 1000;\n>\n> It could be that I am missing something that accounts for this, but I am wondering\n> if it would make sense to sample one of the clocks twice, and average the two samples.\n> i.e.\n>\n> x1 = clock_boottime()\n> y = clock_realtime()\n> x2 = clock_boottime()\n>\n> addSample(midpoint(x1, x2), y)\n>\n> Otherwise I'd expect a constant offset to be present in the mapping, although\n> I am not sure if that makes an appreciable difference.\n>\n>\n> Regards,\n> Barnabás Pőcze\n\nThat's a good suggestion, so thanks for that, I'll give it a try.\n\nI think you're also right that, in reality, there will be no\nmeaningful difference - we're only counting microseconds - and for the\npurposes of camera synchronisation, fixed offsets don't matter. But\nit's still a good thought, so I'll try it!\n\nThanks\nDavid\n\n>\n>\n> > +\n> > + addSample(boot, wall);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Add a sample point to the clock recovery model, specifying the exact\n> > + * input and output clock values\n> > + *\n> > + * This function should be used for corresponding clocks other than the Linux\n> > + * BOOTTIME and REALTIME clocks.\n> > + */\n> > +void ClockRecovery::addSample(uint64_t input, uint64_t output)\n> > +{\n> > + LOG(ClockRec, Debug) << \"addSample \" << input << \" \" << 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 = static_cast(input - inputBase_);\n> > + double y = static_cast(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> > +/**\n> > + * \\brief Calculate the output clock value according to the model from an input\n> > + * clock value\n> > + *\n> > + * \\return Output clock value\n> > + */\n> > +uint64_t ClockRecovery::getOutput(uint64_t input)\n> > +{\n> > + double x = static_cast(input - inputBase_);\n> > + double y = slope_ * x + offset_;\n> > + uint64_t output = y + x + outputBase_;\n> > +\n> > + LOG(ClockRec, Debug) << \"getOutput \" << input << \" \" << output;\n> > +\n> > + return output;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 57fde8a8..4eaa1c8e 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","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 B5DC6C326B\n\tfor ;\n\tFri, 13 Dec 2024 20:51:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 716A967EFD;\n\tFri, 13 Dec 2024 21:51:19 +0100 (CET)","from mail-oi1-x22c.google.com (mail-oi1-x22c.google.com\n\t[IPv6:2607:f8b0:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94EF36189C\n\tfor ;\n\tFri, 13 Dec 2024 21:51:16 +0100 (CET)","by mail-oi1-x22c.google.com with SMTP id\n\t5614622812f47-3eb98ad8caaso1042636b6e.0\n\tfor ;\n\tFri, 13 Dec 2024 12:51:16 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"V186Oj53\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1734123075; x=1734727875;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=aBRBbmvzE1d67M6GwqPjskG5tpGnJlpOGhMTJ9A8r2U=;\n\tb=V186Oj53WKkaz81d6bkt+EnigZ95Gw2zRFGm92vzSWKqVIKbD6BZq4Mq0X/DmoiJg1\n\tKVk5Uz25x8U7qlXKRm3SId6fzJFKSkPHyj5MaAqOG5+Y/FQM3Bgd4HQrIOONXDHnQPl5\n\tPOzPswO3wIy6oubycV3asxzXXY8rV6YV5G0v4H3Gfa76w+k9JpeIOmFZVFWRp9zjZhvB\n\t2sSXG9pM985vRp/Ml9GPYjFJ5KXugnJQCW/fDzQYuWVbtXXtax/4pMOHqj7Lqyi4H1ZL\n\tvZIwt7KGBOsoM6ZmbNt3mbbYePJcipPlkRYeuCXjLJDkGI3+j6KzFpIHV5DvryZ3CMss\n\ttPwQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1734123075; x=1734727875;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=aBRBbmvzE1d67M6GwqPjskG5tpGnJlpOGhMTJ9A8r2U=;\n\tb=TKFaZNgLiizHXO7aFCiZZ+XM5h0fwutSHAf8JmU7nLaezV9YgS+hqm5FfLJElZhpI6\n\t6/gsT29y4ziaQr3srAHYXSdnHeE+PU/6Uty9QSL1kEx7WtfLXizawK144pT3OBmdBpiK\n\t//FAeT5vslVl761O8sT5Rvf+ZULgyTugXEKBftievQa/vCxGXvh2ACbfqqHfW+dHDkAR\n\tyKdvUR/6e4ssSyZPDal8IY4goIOOX6qJojNLvUmRNFDdvDiELryNzLrNrpxbmxHtzq2u\n\tqYCJheEUFh0w07ppMK8YYu7lhak9DUduL06GafSrDL88ZvMDi/eLm1pMwsSgO8rdGi4Y\n\tesTA==","X-Gm-Message-State":"AOJu0YxQiiIne5O1fAYEJyBAXcpn4bxpmDF7br90JsxQ5eqi5PUTdllq\n\tSVU4g4FSxXY7TFfQ5lGNz/7psbzB50U5mQVFnhUmsw6hr8X9osShQ9uEja+e/HMi4+1hUYThKsN\n\tkCrlVBQO5nyhAFIcxGf9eJskFVe4x10dioLWXN/7qpfGw+tmH","X-Gm-Gg":"ASbGncszI5Bz7WlKeDciosBGHKLGH5DqmuIFUop0AaB1reiGUmo5S+yXQSRbDq1bOiv\n\t+/SneyDRxi71eVGE7XO4ZepUGK9/XdoY+zXxS4A==","X-Google-Smtp-Source":"AGHT+IEpPMC0bp2HqY8UxLnne5C7SuzsI6pQQfyjztuLTqnhS1mXaKNU0N6tx9E4QnWTuG69p1RwLumxQAclUE0vUxU=","X-Received":"by 2002:ac8:7f82:0:b0:467:68a3:4c44 with SMTP id\n\td75a77b69052e-467a581d9bemr45638931cf.39.1734081703155;\n\tFri, 13 Dec 2024 01:21:43 -0800 (PST)","MIME-Version":"1.0","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-3-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 13 Dec 2024 09:21:32 +0000","Message-ID":"","Subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":32860,"web_url":"https://patchwork.libcamera.org/comment/32860/","msgid":"<20241218022135.GY23470@pendragon.ideasonboard.com>","date":"2024-12-18T02:21:35","subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Dec 06, 2024 at 04:43:43PM +0000, Kieran Bingham wrote:\n> Quoting David Plowman (2024-12-06 16:29:02)\n> > On Fri, 6 Dec 2024 at 15:50, Kieran Bingham wrote:\n> > > Quoting David Plowman (2024-12-06 14:27:39)\n> > > > The ClockRecovery class takes pairs of timestamps from two different\n> > > > clocks, and models the second (\"output\") clock from the first (\"input\")\n> > > > clock.\n> > > >\n> > > > We can use it, in particular, to get a good wallclock estimate for a\n> > > > frame's SensorTimestamp.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > include/libcamera/internal/clock_recovery.h | 72 +++++++\n> > > > include/libcamera/internal/meson.build | 1 +\n> > > > src/libcamera/clock_recovery.cpp | 207 ++++++++++++++++++++\n> > > > src/libcamera/meson.build | 1 +\n> > > > 4 files changed, 281 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..c874574e\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/internal/clock_recovery.h\n> > > > @@ -0,0 +1,72 @@\n> > > > +/* SPDX-License-Identifier: BSD-2-Clause */\n\nAll headers are LGPL-licensed, can we do the same here ?\n\n> > > > +/*\n> > > > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > > > + *\n> > > > + * Camera recovery algorithm\n> > >\n> > > Recovering cameras ? ;-)\n> > \n> > Hehe. I'll fix that!!\n> > \n> > > > + */\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +class ClockRecovery\n> > > > +{\n> > > > +public:\n> > > > + ClockRecovery();\n> > > > +\n> > > > + /* Set configuration parameters. */\n\nAll documentation goes to the .cpp file.\n\n> > > > + void configure(unsigned int numPts = 100, unsigned int maxJitter = 2000, unsigned int minPts = 10,\n> > > > + unsigned int errorThreshold = 50000);\n\n\tvoid configure(unsigned int numPts = 100, unsigned int maxJitter = 2000,\n\t\t unsigned int minPts = 10, unsigned int errorThreshold = 50000);\n\n> > > > + /* Erase all history and restart the fitting process. */\n> > > > + void reset();\n> > > > +\n> > > > + /*\n> > > > + * Add a new input clock / output clock sample, taking the input from the Linux\n> > > > + * CLOCK_BOOTTIME and the output from the CLOCK_REALTIME.\n> > > > + */\n> > > > + void addSample();\n> > > > + /*\n> > > > + * Add a new input clock / output clock sample, specifying the clock times exactly. Use this\n> > > > + * when you want to use clocks other than the ones described above.\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> > > > + 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> > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > > index 7d6aa8b7..41500636 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..966599ee\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/clock_recovery.cpp\n> > > > @@ -0,0 +1,207 @@\n> > > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > > > + *\n> > > > + * Clock recovery algorithm\n> > > > + */\n> > > > +\n> > > > +#include \"libcamera/internal/clock_recovery.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +/**\n> > > > + * \\file clock_recovery.h\n> > > > + * \\brief Clock recovery - deriving one clock from another independent clock\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(ClockRec)\n> > > > +\n> > > > +/**\n> > > > + * \\class ClockRecovery\n> > > > + * \\brief Recover an output clock from an input clock\n> > > > + *\n> > > > + * The ClockRecovery class derives an output clock from an input clock,\n> > > > + * modelling the output clock as being linearly related to the input clock.\n> > > > + * For example, we may use it to derive wall clock timestamps from timestamps\n> > > > + * measured by the internal system clock which counts local time since boot.\n> > > > + *\n> > > > + * When pairs of corresponding input and output timestamps are available,\n> > > > + * they should be submitted to the model with addSample(). The model will\n> > > > + * update, and output clock values for known input clock values can be\n> > > > + * obtained using getOutput().\n> > > > + *\n> > > > + * As a convenience, if the input clock is indeed the time since boot, and the\n> > > > + * output clock represents a real wallclock time, then addSample() can be\n> > > > + * called with no arguments, and a pair of timestamps will be captured at\n> > > > + * that moment.\n> > > > + *\n> > > > + * The configure() function accepts some configuration parameters to control\n> > > > + * the linear fitting process.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Construct a ClockRecovery\n> > > > + */\n> > > > +ClockRecovery::ClockRecovery()\n> > > > +{\n> > > > + configure();\n> > > > + reset();\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Set configuration parameters\n> > > > + * \\param[in] numPts The approximate duration for which the state of the model\n> > > > + * is persistent, measured in samples\n\nInstead of saying \"measured in samples\" you could rename the variable to\nnumSamples. Same elsewhere where applicable.\n\n> > > > + * \\param[in] maxJitter New output samples are clamped to no more than this\n> > > > + * amount of jitter, to prevent sudden swings from having a large effect\n> > > > + * \\param[in] minPts The fitted clock model is not used to generate outputs\n> > > > + * until this many samples have been received\n> > > > + * \\param[in] errorThreshold If the accumulated differences between input and\n> > > > + * output clocks reaches this amount over a few frames, the model is reset\n> > > > + */\n> > > > +void ClockRecovery::configure(unsigned int numPts, unsigned int maxJitter, unsigned int minPts,\n> > > > + unsigned int errorThreshold)\n> > > > +{\n> > > > + LOG(ClockRec, Debug)\n> > > > + << \"configure \" << numPts << \" \" << maxJitter << \" \" << minPts << \" \" << errorThreshold;\n> > > > +\n> > > > + numPts_ = numPts;\n> > > > + maxJitter_ = maxJitter;\n> > > > + minPts_ = minPts;\n> > > > + errorThreshold_ = errorThreshold;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Reset the clock recovery model and start again from scratch\n> > > > + */\n> > > > +void ClockRecovery::reset()\n> > > > +{\n> > > > + LOG(ClockRec, Debug) << \"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> > >\n> > > Should slope be initialised to 1.0 or anything 'normalised' for any\n> > > startup conditions?\n> > \n> > Actually, 0 is the value that ensures \"output = input\" (allowing for\n> > the different starting offset).\n> > Maybe worth adding a comment to that end?\n> \n> aha - no - that's probably fine - it's just me misunderstanding. For\n> some reason I thought '1' would be the more expected slope for\n> undefined/uninitialised, but I see '0' just removes the slope which is\n> (until samples are ready...) undefined ;-)\n> \n> > > > + offset_ = 0.0;\n> > > > + error_ = 0.0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Add a sample point to the clock recovery model, for recovering a wall\n> > > > + * clock value from the internal system time since boot\n> > > > + *\n> > > > + * This is a convenience function to make it easy to derive a wall clock value\n> > > > + * (using the Linux CLOCK_REALTIME) from the time since the system started\n> > > > + * (measured by CLOCK_BOOTTIME).\n> > > > + */\n> > > > +void ClockRecovery::addSample()\n> > > > +{\n> > > > + LOG(ClockRec, Debug) << \"addSample\";\n> > > > +\n> > > > + struct timespec bootTime;\n> > > > + struct timespec wallTime;\n> > > > +\n> > > > + /* Get boot and wall clocks in microseconds. */\n> > > > + clock_gettime(CLOCK_BOOTTIME, &bootTime);\n> > > > + clock_gettime(CLOCK_REALTIME, &wallTime);\n> > > > + uint64_t boot = bootTime.tv_sec * 1000000ULL + bootTime.tv_nsec / 1000;\n> > > > + uint64_t wall = wallTime.tv_sec * 1000000ULL + wallTime.tv_nsec / 1000;\n\nApart from this, there's nothing in this class that makes it specific to\nclocks. I wonder if we could pass a sampler function to the constructor\n(or maybe to the configure function, or use a template parameter ...),\nand make the class handle linear regressions in a more generic way.\n\n> > > > +\n> > > > + addSample(boot, wall);\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Add a sample point to the clock recovery model, specifying the exact\n> > > > + * input and output clock values\n\nMissing \\param\n\n> > > > + *\n> > > > + * This function should be used for corresponding clocks other than the Linux\n> > > > + * BOOTTIME and REALTIME clocks.\n> > >\n> > > I like that this object has both an 'easy addSample()' and the paired\n> > > version.\n> > >\n> > > I think having the addSample() can define that it pairs BOOTTIME and\n> > > REALTIME clocks, but this addSample(uint64_t input, uint64_t output)\n> > > itself isn't actually tied to those clocks.\n> > >\n> > > If this ClockRecovery object is used elsewhere, the 'only' reason I\n> > > could imagine this function being used is if it were to use a clock\n> > > specifically different to the defaults - so I'd probably drop the text\n> > > here, and keep the specification of BOOTTIME/REALTIME to the addSample()\n> > > - or specific to the usage in V4L2VideoDevice.\n\nThe class could be useful in the uvcvideo pipeline handler, to convert\nfrom the hardware clock to the system clock (we'll need two instances\nthough, as we need to go through the USB clock in the middle).\n\n> > > > +void ClockRecovery::addSample(uint64_t input, uint64_t output)\n> > >\n> > > As an example that I'm not sure if it's crazy or not yet - I could\n> > > imagine setting up a ClockRecovery object that maps sequence number and\n> > > timestamp so that you could determine the frame sequence number (output)\n> > > from timestamp (input) which would give you quantised sequence numbers\n> > > determined from the timestamp - so you could spot frame drops if a CSI\n> > > receiver always supplies monotonic sequence numbers. (which I think the\n> > > i.MX8MP does... ?)\n> > \n> > Interesting thought, though I think I'd prefer to keep the basic\n> > ClockRecovery restricted to, well, clocks. If someone wants to spot\n> > frame drops... obviously they're welcome to do that, but maybe that\n> > gets implemented on top? Remember of course that frame times may be\n> > variable.\n> \n> Oh absolutly - I'm not expecting anything here - I'm just speculating on\n> what crazy things we could match between input/output to get the linear\n> regression on ;-)\n> \n> And that's why I think \"ClockRecovery::addSample(uint64_t input, uint64_t\n> output)\" as a function with two parameters isn't itself tied to a\n> specific clock. (but the usage in V4L2VideoDevice, and addSample() is).\n\nA point that bothers me a bit with the two functions is that it opens\nthe door to incorrect usage. I wonder if we could improve that. Not\nsomething to look at right now, let's focus on the rest of the series\nfirst. I'll see if I can find a good idea to improve the next version in\nthis regards.\n\n> > > > +{\n> > > > + LOG(ClockRec, Debug) << \"addSample \" << input << \" \" << 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> > >\n> > > Throughout the series, there's lots of text that could easily be wrapped\n> > > to 80 chars. It shows up a lot in my email because I have an 92 char\n> > > terminal for my email which shows lots of lines wrapping, but it's not a\n> > > hard requirement.\n> > \n> > I guess I'm not clear how strict we are about line lengths... mostly I\n> > don't worry if the style checker doesn't complain. But I'm happy to do\n> > a bit of reflowing if that helps, though some slightly more \"official\"\n> > guidance on when to do that would be helpful.\n> \n> I think we normally aim for blocks of 80 - but the exceptions are up to\n> 120 if it helps/makes sense in that instance.\n\nThat's right, we have a 80 columns soft limit and a 120 columns harder\nlimit. For comment blocks there are very few reasons to go over 80\ncolumns.\n\n> It only shows up in this series, as I think everything is targetting\n> ~100/120ish instead.\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 = static_cast(input - inputBase_);\n> > > > + double y = static_cast(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> > >\n> > > What's the output from getOuput before this happens ? Anything\n> > > problematic? Or just things that can be ignored for the first few\n> > > frames?\n> > \n> > No, you just get \"output = input\" allowing for the different starting\n> > offsets. Again, maybe worth a comment?\n> \n> Hrm, ok so maybe it might be useful to state 'somewhere' that the system\n> will produce output values matching the input value until sufficient\n> sampling points have been collected.\n\nWhat should be the behaviour of the FrameWallClock control during that\ntime ? That should be documented in the control definition. I'm tempted\nto say it should not be generated until it becomes stable.\n\n> And following that back, I see minPts_ is configurable (which is a good\n> thing :D), so it's up to the use case to determine what the minimum\n> requirements are.\n> \n> Seems pretty good to me.\n> \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> > > > +/**\n> > > > + * \\brief Calculate the output clock value according to the model from an input\n> > > > + * clock value\n\nMissing \\param\n\n> > > > + *\n> > > > + * \\return Output clock value\n> > > > + */\n> > > > +uint64_t ClockRecovery::getOutput(uint64_t input)\n> > > > +{\n> > > > + double x = static_cast(input - inputBase_);\n> > > > + double y = slope_ * x + offset_;\n> > > > + uint64_t output = y + x + outputBase_;\n> > > > +\n> > > > + LOG(ClockRec, Debug) << \"getOutput \" << input << \" \" << output;\n> > > > +\n> > > > + return output;\n> > > > +}\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index 57fde8a8..4eaa1c8e 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',","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 3CD22BD1F1\n\tfor ;\n\tWed, 18 Dec 2024 02:21:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 788F468073;\n\tWed, 18 Dec 2024 03:21:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D64761898\n\tfor ;\n\tWed, 18 Dec 2024 03:21:38 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C4F4670;\n\tWed, 18 Dec 2024 03:21:00 +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=\"KOsCGOuZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734488460;\n\tbh=u6bpLyOMw48KGLx0CF8XTDNoYGYBj5TdICUcQarb8WY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KOsCGOuZWNgRTKExfcfjDmzFAwXimSD3FS420rFBX3i8jp5mogdUkNBJbLhUB75HZ\n\tt4mjXTdIuK0dlNGKzCIQnvNResAGc0x1IOsxmzDKkc8bUePgHgQAfaxlvd4uJwmxP4\n\tKt/mT5Q2mHmA7zbrzDvCGAN1HeJWVcagBcuDftDg=","Date":"Wed, 18 Dec 2024 04:21:35 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","Message-ID":"<20241218022135.GY23470@pendragon.ideasonboard.com>","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-3-david.plowman@raspberrypi.com>\n\t<173350021613.3135963.7261573657809888292@ping.linuxembedded.co.uk>\n\t\n\t<173350342329.3135963.11307611673106150546@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173350342329.3135963.11307611673106150546@ping.linuxembedded.co.uk>","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":32873,"web_url":"https://patchwork.libcamera.org/comment/32873/","msgid":"","date":"2024-12-18T14:10:48","subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the review and comments.\n\nOn Wed, 18 Dec 2024 at 02:21, Laurent Pinchart\n wrote:\n>\n> On Fri, Dec 06, 2024 at 04:43:43PM +0000, Kieran Bingham wrote:\n> > Quoting David Plowman (2024-12-06 16:29:02)\n> > > On Fri, 6 Dec 2024 at 15:50, Kieran Bingham wrote:\n> > > > Quoting David Plowman (2024-12-06 14:27:39)\n> > > > > The ClockRecovery class takes pairs of timestamps from two different\n> > > > > clocks, and models the second (\"output\") clock from the first (\"input\")\n> > > > > clock.\n> > > > >\n> > > > > We can use it, in particular, to get a good wallclock estimate for a\n> > > > > frame's SensorTimestamp.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/internal/clock_recovery.h | 72 +++++++\n> > > > > include/libcamera/internal/meson.build | 1 +\n> > > > > src/libcamera/clock_recovery.cpp | 207 ++++++++++++++++++++\n> > > > > src/libcamera/meson.build | 1 +\n> > > > > 4 files changed, 281 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..c874574e\n> > > > > --- /dev/null\n> > > > > +++ b/include/libcamera/internal/clock_recovery.h\n> > > > > @@ -0,0 +1,72 @@\n> > > > > +/* SPDX-License-Identifier: BSD-2-Clause */\n>\n> All headers are LGPL-licensed, can we do the same here ?\n\nWill do.\n\n>\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > > > > + *\n> > > > > + * Camera recovery algorithm\n> > > >\n> > > > Recovering cameras ? ;-)\n> > >\n> > > Hehe. I'll fix that!!\n> > >\n> > > > > + */\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +class ClockRecovery\n> > > > > +{\n> > > > > +public:\n> > > > > + ClockRecovery();\n> > > > > +\n> > > > > + /* Set configuration parameters. */\n>\n> All documentation goes to the .cpp file.\n\nI'll delete the comments here, presumably they're still OK when\ndocumenting private members (?) so I'll leave those.\n\n>\n> > > > > + void configure(unsigned int numPts = 100, unsigned int maxJitter = 2000, unsigned int minPts = 10,\n> > > > > + unsigned int errorThreshold = 50000);\n>\n> void configure(unsigned int numPts = 100, unsigned int maxJitter = 2000,\n> unsigned int minPts = 10, unsigned int errorThreshold = 50000);\n>\n> > > > > + /* Erase all history and restart the fitting process. */\n> > > > > + void reset();\n> > > > > +\n> > > > > + /*\n> > > > > + * Add a new input clock / output clock sample, taking the input from the Linux\n> > > > > + * CLOCK_BOOTTIME and the output from the CLOCK_REALTIME.\n> > > > > + */\n> > > > > + void addSample();\n> > > > > + /*\n> > > > > + * Add a new input clock / output clock sample, specifying the clock times exactly. Use this\n> > > > > + * when you want to use clocks other than the ones described above.\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> > > > > + 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> > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > > > index 7d6aa8b7..41500636 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..966599ee\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/clock_recovery.cpp\n> > > > > @@ -0,0 +1,207 @@\n> > > > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > > > > + *\n> > > > > + * Clock recovery algorithm\n> > > > > + */\n> > > > > +\n> > > > > +#include \"libcamera/internal/clock_recovery.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +/**\n> > > > > + * \\file clock_recovery.h\n> > > > > + * \\brief Clock recovery - deriving one clock from another independent clock\n> > > > > + */\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(ClockRec)\n> > > > > +\n> > > > > +/**\n> > > > > + * \\class ClockRecovery\n> > > > > + * \\brief Recover an output clock from an input clock\n> > > > > + *\n> > > > > + * The ClockRecovery class derives an output clock from an input clock,\n> > > > > + * modelling the output clock as being linearly related to the input clock.\n> > > > > + * For example, we may use it to derive wall clock timestamps from timestamps\n> > > > > + * measured by the internal system clock which counts local time since boot.\n> > > > > + *\n> > > > > + * When pairs of corresponding input and output timestamps are available,\n> > > > > + * they should be submitted to the model with addSample(). The model will\n> > > > > + * update, and output clock values for known input clock values can be\n> > > > > + * obtained using getOutput().\n> > > > > + *\n> > > > > + * As a convenience, if the input clock is indeed the time since boot, and the\n> > > > > + * output clock represents a real wallclock time, then addSample() can be\n> > > > > + * called with no arguments, and a pair of timestamps will be captured at\n> > > > > + * that moment.\n> > > > > + *\n> > > > > + * The configure() function accepts some configuration parameters to control\n> > > > > + * the linear fitting process.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Construct a ClockRecovery\n> > > > > + */\n> > > > > +ClockRecovery::ClockRecovery()\n> > > > > +{\n> > > > > + configure();\n> > > > > + reset();\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Set configuration parameters\n> > > > > + * \\param[in] numPts The approximate duration for which the state of the model\n> > > > > + * is persistent, measured in samples\n>\n> Instead of saying \"measured in samples\" you could rename the variable to\n> numSamples. Same elsewhere where applicable.\n\nYes, a name change is probably helpful.\n\n>\n> > > > > + * \\param[in] maxJitter New output samples are clamped to no more than this\n> > > > > + * amount of jitter, to prevent sudden swings from having a large effect\n> > > > > + * \\param[in] minPts The fitted clock model is not used to generate outputs\n> > > > > + * until this many samples have been received\n> > > > > + * \\param[in] errorThreshold If the accumulated differences between input and\n> > > > > + * output clocks reaches this amount over a few frames, the model is reset\n> > > > > + */\n> > > > > +void ClockRecovery::configure(unsigned int numPts, unsigned int maxJitter, unsigned int minPts,\n> > > > > + unsigned int errorThreshold)\n> > > > > +{\n> > > > > + LOG(ClockRec, Debug)\n> > > > > + << \"configure \" << numPts << \" \" << maxJitter << \" \" << minPts << \" \" << errorThreshold;\n> > > > > +\n> > > > > + numPts_ = numPts;\n> > > > > + maxJitter_ = maxJitter;\n> > > > > + minPts_ = minPts;\n> > > > > + errorThreshold_ = errorThreshold;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Reset the clock recovery model and start again from scratch\n> > > > > + */\n> > > > > +void ClockRecovery::reset()\n> > > > > +{\n> > > > > + LOG(ClockRec, Debug) << \"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> > > >\n> > > > Should slope be initialised to 1.0 or anything 'normalised' for any\n> > > > startup conditions?\n> > >\n> > > Actually, 0 is the value that ensures \"output = input\" (allowing for\n> > > the different starting offset).\n> > > Maybe worth adding a comment to that end?\n> >\n> > aha - no - that's probably fine - it's just me misunderstanding. For\n> > some reason I thought '1' would be the more expected slope for\n> > undefined/uninitialised, but I see '0' just removes the slope which is\n> > (until samples are ready...) undefined ;-)\n> >\n> > > > > + offset_ = 0.0;\n> > > > > + error_ = 0.0;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Add a sample point to the clock recovery model, for recovering a wall\n> > > > > + * clock value from the internal system time since boot\n> > > > > + *\n> > > > > + * This is a convenience function to make it easy to derive a wall clock value\n> > > > > + * (using the Linux CLOCK_REALTIME) from the time since the system started\n> > > > > + * (measured by CLOCK_BOOTTIME).\n> > > > > + */\n> > > > > +void ClockRecovery::addSample()\n> > > > > +{\n> > > > > + LOG(ClockRec, Debug) << \"addSample\";\n> > > > > +\n> > > > > + struct timespec bootTime;\n> > > > > + struct timespec wallTime;\n> > > > > +\n> > > > > + /* Get boot and wall clocks in microseconds. */\n> > > > > + clock_gettime(CLOCK_BOOTTIME, &bootTime);\n> > > > > + clock_gettime(CLOCK_REALTIME, &wallTime);\n> > > > > + uint64_t boot = bootTime.tv_sec * 1000000ULL + bootTime.tv_nsec / 1000;\n> > > > > + uint64_t wall = wallTime.tv_sec * 1000000ULL + wallTime.tv_nsec / 1000;\n>\n> Apart from this, there's nothing in this class that makes it specific to\n> clocks. I wonder if we could pass a sampler function to the constructor\n> (or maybe to the configure function, or use a template parameter ...),\n> and make the class handle linear regressions in a more generic way.\n\nThat's an interesting thought. Mostly I'm dead nervous about\ngeneralising stuff when I have only a single use case, but I certainly\nsee the idea here. Folks could supply a \"sampler\", or maybe derive\nfrom the base class, or something. Maybe the version of addSample with\ntwo parameters becomes \"protected\"?\n\nOn the other hand, as soon as you're doing that, then maybe the\nconfiguration parameters become something you might want to tweak as\nwell. Maybe making intialise() and addSample() virtual is enough?\nThere's also a bit of an assumption about the input-to-output rate\ndefaulting to 1-to-1, and whether that's transferable. The fact that\nyou know this is pretty helpful - it means you get good output\nestimates right off the bat. So... don't know. Maybe? Maybe wait for\nanother use case to turn up?\n\n>\n> > > > > +\n> > > > > + addSample(boot, wall);\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Add a sample point to the clock recovery model, specifying the exact\n> > > > > + * input and output clock values\n>\n> Missing \\param\n\nWill add!\n\n>\n> > > > > + *\n> > > > > + * This function should be used for corresponding clocks other than the Linux\n> > > > > + * BOOTTIME and REALTIME clocks.\n> > > >\n> > > > I like that this object has both an 'easy addSample()' and the paired\n> > > > version.\n> > > >\n> > > > I think having the addSample() can define that it pairs BOOTTIME and\n> > > > REALTIME clocks, but this addSample(uint64_t input, uint64_t output)\n> > > > itself isn't actually tied to those clocks.\n> > > >\n> > > > If this ClockRecovery object is used elsewhere, the 'only' reason I\n> > > > could imagine this function being used is if it were to use a clock\n> > > > specifically different to the defaults - so I'd probably drop the text\n> > > > here, and keep the specification of BOOTTIME/REALTIME to the addSample()\n> > > > - or specific to the usage in V4L2VideoDevice.\n>\n> The class could be useful in the uvcvideo pipeline handler, to convert\n> from the hardware clock to the system clock (we'll need two instances\n> though, as we need to go through the USB clock in the middle).\n>\n> > > > > +void ClockRecovery::addSample(uint64_t input, uint64_t output)\n> > > >\n> > > > As an example that I'm not sure if it's crazy or not yet - I could\n> > > > imagine setting up a ClockRecovery object that maps sequence number and\n> > > > timestamp so that you could determine the frame sequence number (output)\n> > > > from timestamp (input) which would give you quantised sequence numbers\n> > > > determined from the timestamp - so you could spot frame drops if a CSI\n> > > > receiver always supplies monotonic sequence numbers. (which I think the\n> > > > i.MX8MP does... ?)\n> > >\n> > > Interesting thought, though I think I'd prefer to keep the basic\n> > > ClockRecovery restricted to, well, clocks. If someone wants to spot\n> > > frame drops... obviously they're welcome to do that, but maybe that\n> > > gets implemented on top? Remember of course that frame times may be\n> > > variable.\n> >\n> > Oh absolutly - I'm not expecting anything here - I'm just speculating on\n> > what crazy things we could match between input/output to get the linear\n> > regression on ;-)\n> >\n> > And that's why I think \"ClockRecovery::addSample(uint64_t input, uint64_t\n> > output)\" as a function with two parameters isn't itself tied to a\n> > specific clock. (but the usage in V4L2VideoDevice, and addSample() is).\n>\n> A point that bothers me a bit with the two functions is that it opens\n> the door to incorrect usage. I wonder if we could improve that. Not\n> something to look at right now, let's focus on the rest of the series\n> first. I'll see if I can find a good idea to improve the next version in\n> this regards.\n\nOK, we'll wait and see. I'll probably post an update in the meantime\njust to try and settle at least some other things where we can!\n\n>\n> > > > > +{\n> > > > > + LOG(ClockRec, Debug) << \"addSample \" << input << \" \" << 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> > > >\n> > > > Throughout the series, there's lots of text that could easily be wrapped\n> > > > to 80 chars. It shows up a lot in my email because I have an 92 char\n> > > > terminal for my email which shows lots of lines wrapping, but it's not a\n> > > > hard requirement.\n> > >\n> > > I guess I'm not clear how strict we are about line lengths... mostly I\n> > > don't worry if the style checker doesn't complain. But I'm happy to do\n> > > a bit of reflowing if that helps, though some slightly more \"official\"\n> > > guidance on when to do that would be helpful.\n> >\n> > I think we normally aim for blocks of 80 - but the exceptions are up to\n> > 120 if it helps/makes sense in that instance.\n>\n> That's right, we have a 80 columns soft limit and a 120 columns harder\n> limit. For comment blocks there are very few reasons to go over 80\n> columns.\n>\n> > It only shows up in this series, as I think everything is targetting\n> > ~100/120ish instead.\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 = static_cast(input - inputBase_);\n> > > > > + double y = static_cast(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> > > >\n> > > > What's the output from getOuput before this happens ? Anything\n> > > > problematic? Or just things that can be ignored for the first few\n> > > > frames?\n> > >\n> > > No, you just get \"output = input\" allowing for the different starting\n> > > offsets. Again, maybe worth a comment?\n> >\n> > Hrm, ok so maybe it might be useful to state 'somewhere' that the system\n> > will produce output values matching the input value until sufficient\n> > sampling points have been collected.\n>\n> What should be the behaviour of the FrameWallClock control during that\n> time ? That should be documented in the control definition. I'm tempted\n> to say it should not be generated until it becomes stable.\n\nWell, actually I find that I get pretty good estimates right from the\nstart, in large part because of the 1-to-1 initial assumption. Of\ncourse, there is a risk that, if you're unlucky, you'll end up with an\noffset that's slightly skewed, and this would last for several\nsamples.\n\nIf that initial sample is a worry, we could take multiple readings and\naverage them. Another slightly interesting variation is that, with\nBarnabas's modification, you could actually generate a passable bound\nfor this error (namely bootTime2 - bootTime1). So you could take the\nsample where this is smallest.\n\nIn fact, there's a lot we could do with something like an error bound,\nthe question in my mind is really... should we? Or are we\n(prematurely) over-complicating everything?\n\n>\n> > And following that back, I see minPts_ is configurable (which is a good\n> > thing :D), so it's up to the use case to determine what the minimum\n> > requirements are.\n> >\n> > Seems pretty good to me.\n> >\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> > > > > +/**\n> > > > > + * \\brief Calculate the output clock value according to the model from an input\n> > > > > + * clock value\n>\n> Missing \\param\n\nYep.\n\nThanks!\nDavid\n\n>\n> > > > > + *\n> > > > > + * \\return Output clock value\n> > > > > + */\n> > > > > +uint64_t ClockRecovery::getOutput(uint64_t input)\n> > > > > +{\n> > > > > + double x = static_cast(input - inputBase_);\n> > > > > + double y = slope_ * x + offset_;\n> > > > > + uint64_t output = y + x + outputBase_;\n> > > > > +\n> > > > > + LOG(ClockRec, Debug) << \"getOutput \" << input << \" \" << output;\n> > > > > +\n> > > > > + return output;\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > > index 57fde8a8..4eaa1c8e 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> --\n> Regards,\n>\n> Laurent Pinchart","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 54C38C32FE\n\tfor ;\n\tWed, 18 Dec 2024 14:11:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F4736809F;\n\tWed, 18 Dec 2024 15:11:03 +0100 (CET)","from mail-qt1-x82d.google.com (mail-qt1-x82d.google.com\n\t[IPv6:2607:f8b0:4864:20::82d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFDA267F59\n\tfor ;\n\tWed, 18 Dec 2024 15:11:01 +0100 (CET)","by mail-qt1-x82d.google.com with SMTP id\n\td75a77b69052e-467a17055e6so66120991cf.3\n\tfor ;\n\tWed, 18 Dec 2024 06:11:01 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"SRATVnAW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1734531060; x=1735135860;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=jJG9Qdm3elvooCVYsfykYDVLi7+jCIicekvJIv4znWU=;\n\tb=SRATVnAWXfjGBQScw3cA4cX4pS7kETPswpH2uVaVUqS2Spsvgir+jyRUi3kqthlz/c\n\tj3kIsxzPiEaSUk8MX/yKsLae7crNOQXaYHfGxgQduIeRsVsQiviL/fDDKi3LKGV7l3Um\n\tS9cW89mSgPPknb4Y8teyM7lxC2mhXJQNtRQ5BAYeZwwg87MkBsBcSyd7GezxeCtqVDPt\n\twlVejSv1JQ90uK+GM19oj7R65hlFRSJdVgfNkYwoQJsaHCiApn/+VV69rNhW2WZnTWdO\n\t+8VPkKlpz0iLvSkRnyqhQ5f1tDcOlVrkHsxwxsXq7SwELwdoIpNbdi1Sl6JOll6zcgrT\n\tm0tw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1734531060; x=1735135860;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=jJG9Qdm3elvooCVYsfykYDVLi7+jCIicekvJIv4znWU=;\n\tb=FXqt9Nl77pFKOySik6XtTQWySvbju0RLUqaRsIYjmPBr5gEPjqyOkrOT87JcDio30+\n\tZvXKTmz0DDpz/OuAwffDaMcIiUOSidKyELyppp3VHNraRiZYPSB3a9auuW6tIQZ6c9jM\n\tJl4OUYoVejuK588UXtL3QJXp62vqZBLMNw6FGAhhjEQKDdAoFoH/ACJWuNep6pSswVHO\n\t0MTzQXPzeLy2ew8cldLkCRsnv1VarMIP4B7mqfLSzaZi5YnLtK0+aBOtahZtwHEZLfbq\n\tDOL6YTlpe8zPRCxnj4FnpBwpgZLVg53ik+COSIIlpsijdZ7tFRgK46gcrThbJu408CIk\n\tUVsg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXcNODPRaz9fa685R6AqDuv/WmIC1Xigr6+Cy4w3vy2o99gsx6BdhJ75yvpWYYqvviVG+bo/vtnWoGcT9q4veM=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxSpYHS4KwDaVzm9wYWqnGzaAVdTCTGIlpnuSPg++MyvY3dOuAQ\n\tBhjfDS0gdVPy7Y9ZK1KeWcRjzBdaAVnOZmo3JoaEkztd4F6CrBGPgfJufjAQb8YUrK3/NZAUM1W\n\tc0OVzJfyd6GBl87nkLwyZIiPSjZjGm1yMImSzsA==","X-Gm-Gg":"ASbGncvSVmzMfme3k4yOaQw5W9lBKA8NvASn+WX9dLrWmxS77vR/mhhWsFk7GhqMjaN\n\teESaNpQ+zuRCnowbpajFXNDxvUttWVGk+6wRlAqkuUiufeFsw1YWnfCkbKfzGRPdSd3dlZQ==","X-Google-Smtp-Source":"AGHT+IEd1nVMZYH+Drs8dTTFqVG7VlbvRcQdfldO8lo+WLtL7qoQeQesWV0waK8toum/CFRszSFbSm/bySC107OWBJk=","X-Received":"by 2002:a05:622a:94:b0:467:7701:a604 with SMTP id\n\td75a77b69052e-46908ef03acmr57241951cf.56.1734531060353;\n\tWed, 18 Dec 2024 06:11:00 -0800 (PST)","MIME-Version":"1.0","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-3-david.plowman@raspberrypi.com>\n\t<173350021613.3135963.7261573657809888292@ping.linuxembedded.co.uk>\n\t\n\t<173350342329.3135963.11307611673106150546@ping.linuxembedded.co.uk>\n\t<20241218022135.GY23470@pendragon.ideasonboard.com>","In-Reply-To":"<20241218022135.GY23470@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Wed, 18 Dec 2024 14:10:48 +0000","Message-ID":"","Subject":"Re: [PATCH 2/5] libcamera: Add ClockRecovery class to generate\n\twallclock timestamps","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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\" "}}]