[{"id":32535,"web_url":"https://patchwork.libcamera.org/comment/32535/","msgid":"<20241205100039.GA20214@pendragon.ideasonboard.com>","date":"2024-12-05T10:00:39","subject":"Re: [RFC PATCH 3/3] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Tue, Nov 26, 2024 at 12:17:06PM +0000, David Plowman wrote:\n> FrameMetadata is extended to include wallclock timestamps, both\n> \"raw\" (unsmoothed) and de-jittered versions.\n> \n> The \"raw\" wallclock timestamps are recorded when frames start, and at\n> frame end we generate a de-jittered version of this wallclock, handing\n> both out to pipeline handlers.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/framebuffer.h | 2 ++\n> include/libcamera/internal/v4l2_device.h | 4 +++\n> include/libcamera/internal/v4l2_videodevice.h | 3 ++\n> src/libcamera/v4l2_device.cpp | 22 ++++++++++++++\n> src/libcamera/v4l2_videodevice.cpp | 29 +++++++++++++++++++\n> 5 files changed, 60 insertions(+)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index ff839243..a181d97a 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -35,6 +35,8 @@ struct FrameMetadata {\n> \tStatus status;\n> \tunsigned int sequence;\n> \tuint64_t timestamp;\n> +\tuint64_t wallClockRaw;\n> +\tuint64_t wallClock;\n> \n> \tSpan planes() { return planes_; }\n> \tSpan planes() const { return planes_; }\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index f5aa5024..90dbc4a8 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -67,6 +68,9 @@ protected:\n> \ttemplate\n> \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> \n> +\tstd::queue> wallClockQueue_;\n> +\tbool frameStartEnabled() const { return frameStartEnabled_; }\n> +\n> private:\n> \tstatic ControlType v4l2CtrlType(uint32_t ctrlType);\n> \tstatic std::unique_ptr v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index f021c2a0..d6127928 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -31,6 +31,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/clock_recovery.h\"\n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/v4l2_device.h\"\n> #include \"libcamera/internal/v4l2_pixelformat.h\"\n> @@ -290,6 +291,8 @@ private:\n> \n> \tTimer watchdog_;\n> \tutils::Duration watchdogDuration_;\n> +\n> +\tClockRecovery wallClockRecovery_;\n> };\n> \n> class V4L2M2MDevice\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 7d21cf15..285527b4 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -474,6 +474,11 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> * \\return The V4L2 device file descriptor, -1 if the device node is not open\n> */\n> \n> +/**\n> + * \\fn V4L2Device::frameStartEnabled()\n> + * \\return True if frame start notifications are enabled, otherwise false\n> + */\n> +\n> /**\n> * \\brief Retrieve the libcamera control type associated with the V4L2 control\n> * \\param[in] ctrlType The V4L2 control type\n> @@ -761,6 +766,23 @@ void V4L2Device::eventAvailable()\n> \t\treturn;\n> \t}\n> \n> +\t/*\n> +\t * Record this frame (by its sequence number) and its corresponding wallclock value.\n> +\t * Use a queue as these two events may not interleave perfectly.\n\nWhat do you mean by \"may not interleave perfectly\" ?\n\n> +\t */\n> +\tauto now = std::chrono::system_clock::now();\n\nOK, so the wall clock control is a SOF timestamp. Is that an\nimplementation detail here, or something cameras will guarantee ?\n\nAnd is it actually the right value ? SensorTimestamp is taken directly\nfrom the buffer timestamp returned by V4L2. In theory drivers should\nreport if the timestamp corresponds to the start of exposure\n(V4L2_BUF_FLAG_TSTAMP_SRC_SOE) or end of frame\n(V4L2_BUF_FLAG_TSTAMP_SRC_EOF), but in practice only one driver does\n(beside vivid) -_-'. The V4L2 timestamp is therefore taken at an\nunspecified point of time.\n\nIf we want to correlate the wall clock time with the SensorTimestamp,\nwe'll need to do better than that, and sample both for the same event\n(SOE or EOF). Ideally that sampling should be performed in the kernel to\nbe as atomic as possible, but I'm OK implementing that separately (as\nlong as it gets done).\n\n> +\tuint64_t wallClock = std::chrono::duration_cast(now.time_since_epoch()).count();\n> +\n> +\twallClockQueue_.emplace(event.u.frame_sync.frame_sequence, wallClock);\n> +\n> +\t/*\n> +\t * Also avoid growing the queue indefiniteily. It seems highly unlikely that you could\n> +\t * get more than a few \"frame starts\" being processed without a \"frame end\", so the value\n> +\t * 5, while arbitrary, appears to be more than enough in practice.\n> +\t */\n> +\twhile (wallClockQueue_.size() > 5)\n> +\t\twallClockQueue_.pop();\n\nIs this efficient, or should we use some sort of ring buffer instead ?\n\n> +\n> \tframeStart.emit(event.u.frame_sync.frame_sequence);\n> }\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 14eba056..5c5dfc79 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1865,6 +1865,33 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \tmetadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> \t\t\t + buf.timestamp.tv_usec * 1000ULL;\n> \n> +\tif (frameStartEnabled()) {\n> +\t\t/*\n> +\t\t * Find the wallclock that should have been recorded for this frame, discarding any\n> +\t\t * stale frames on the way.\n> +\t\t */\n> +\t\twhile (!wallClockQueue_.empty() && wallClockQueue_.front().first < buf.sequence)\n> +\t\t\twallClockQueue_.pop();\n> +\n> +\t\tif (!wallClockQueue_.empty() && wallClockQueue_.front().first == buf.sequence) {\n> +\t\t\tmetadata.wallClockRaw = wallClockQueue_.front().second;\n> +\t\t\twallClockQueue_.pop();\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * At higher framerates it can happen that this gets handled before the frame\n> +\t\t\t * start event, meaning there's no wallclock time in the queue. So the best we\n> +\t\t\t * can do is sample the wallclock now. (The frame start will subsequently add\n> +\t\t\t * another wallclock timestamp, but this will get safely discarded.)\n> +\t\t\t */\n> +\t\t\tauto now = std::chrono::system_clock::now();\n> +\t\t\tmetadata.wallClockRaw = std::chrono::duration_cast(now.time_since_epoch()).count();\n> +\t\t}\n> +\n> +\t\t/* Now recover a de-jittered wallclock value. Everything must be in microseconds. */\n> +\t\twallClockRecovery_.addSample(metadata.timestamp / 1000, metadata.wallClockRaw);\n> +\t\tmetadata.wallClock = wallClockRecovery_.getOutput(metadata.timestamp / 1000);\n\nReading this, I realize we may have more leeway. As far as I understand,\nwe could sample the wall clock and monotonic clock periodicaly to feed\nthe clock recovery class with samples, and then convert the buffer\ntimestamp to wall time. It makes me wonder why we want to expose the raw\nwall clock. I also think we may want to change the definition of the\nsmoothed timestamp, it's not so much smoothed than recovered.\n\n> +\t}\n> +\n> \tif (V4L2_TYPE_IS_OUTPUT(buf.type))\n> \t\treturn buffer;\n> \n> @@ -1958,6 +1985,8 @@ int V4L2VideoDevice::streamOn()\n> \n> \tfirstFrame_.reset();\n> \n> +\twallClockQueue_ = {};\n> +\n> \tret = ioctl(VIDIOC_STREAMON, &bufferType_);\n> \tif (ret < 0) {\n> \t\tLOG(V4L2, Error)","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 08539BDB1C\n\tfor ;\n\tThu, 5 Dec 2024 10:00:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4AD18618B3;\n\tThu, 5 Dec 2024 11:00:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63B27618B3\n\tfor ;\n\tThu, 5 Dec 2024 11:00:51 +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 86F422B3;\n\tThu, 5 Dec 2024 11:00:22 +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=\"n41Qt39g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733392822;\n\tbh=iRbrGJ99kyVC5tbkVZK9SHWYgaTqQnHmiuNyhG1e+nM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n41Qt39gIDle/tqacNxVlYsOQNdz3A+NisQpzXaQC8/wtmhOciZShwHIYLXqKPae5\n\tlqbaDQqGPtzPnadFnhLSq52bIwJSsZgXk2ZIiAtbgO0coycqeTl0A/M70vr0HjjXKy\n\tMnYwrk7bQio55m7T2Ffc5BLrH0dsmfoxrZ/EJ058=","Date":"Thu, 5 Dec 2024 12:00:39 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 3/3] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","Message-ID":"<20241205100039.GA20214@pendragon.ideasonboard.com>","References":"<20241126121706.4350-1-david.plowman@raspberrypi.com>\n\t<20241126121706.4350-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241126121706.4350-4-david.plowman@raspberrypi.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]