[{"id":32591,"web_url":"https://patchwork.libcamera.org/comment/32591/","msgid":"<173349944209.312267.11949763883994764728@ping.linuxembedded.co.uk>","date":"2024-12-06T15:37:22","subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","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:40)\n> FrameMetadata is extended to include a wallclock timestamp.\n> \n> When a frame is dequeued, we use the ClockRecovery class to generate a\n> good estimate of a wallclock timestamp to correspond to the frame's\n> SensorTimestamp.\n> \n> Pipeline handlers must enable wallclocks for chosen devices by passing\n> an appropriate ClockRecovery object.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/framebuffer.h | 1 +\n> include/libcamera/internal/v4l2_videodevice.h | 5 +++\n> src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++-\n> 3 files changed, 41 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index ff839243..4579d0c6 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -35,6 +35,7 @@ struct FrameMetadata {\n> Status status;\n> unsigned int sequence;\n> uint64_t timestamp;\n> + uint64_t wallClock;\n\nThis breaks in the CI build I'm afraid:\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569\n\n\n[481/650] Generating Documentation/doxygen-internal with a custom command\nFAILED: Documentation/internal-api-html\n/usr/bin/doxygen Documentation/Doxyfile-internal\n/builds/camera/libcamera/include/libcamera/framebuffer.h:38: error: Member wallClock (variable) of struct libcamera::FrameMetadata is not documented. (warning treated as error, aborting now)\n\nEither we need to add the doc (which is what I'd do) - or potentially\ndepending on discussions below, remove the addition...\n\n> \n> Span planes() { return planes_; }\n> Span planes() const { return planes_; }\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index f021c2a0..5327c112 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -37,6 +37,7 @@\n> \n> namespace libcamera {\n> \n> +class ClockRecovery;\n> class EventNotifier;\n> class MediaDevice;\n> class MediaEntity;\n> @@ -232,6 +233,8 @@ public:\n> \n> V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;\n> \n> + void enableWallClock(ClockRecovery *wallClockRecovery);\n> +\n> protected:\n> std::string logPrefix() const override;\n> \n> @@ -290,6 +293,8 @@ private:\n> \n> Timer watchdog_;\n> utils::Duration watchdogDuration_;\n> +\n> + ClockRecovery *wallClockRecovery_;\n> };\n> \n> class V4L2M2MDevice\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index a5cf6784..2007dffc 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -26,6 +26,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/clock_recovery.h\"\n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/media_device.h\"\n> @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f)\n> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> fdBufferNotifier_(nullptr), state_(State::Stopped),\n> - watchdogDuration_(0.0)\n> + watchdogDuration_(0.0), wallClockRecovery_(nullptr)\n> {\n> /*\n> * We default to an MMAP based CAPTURE video device, however this will\n> @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> + buf.timestamp.tv_usec * 1000ULL;\n> \n> + metadata.wallClock = 0;\n> + if (wallClockRecovery_) {\n\nI know Laurent discussed passing in the 'ClockRecovery' class as a way\nof giving the responsibility to the Pipeline handler - but I'm not\nparticularly fond of it. (I won't object if this is preferred all\nround).\n\nTo me - especially if there is an addition of uint64_t wallClock in the\nFrameMetadata, then this should just always be enabled.\n\nBut if it's /really/ optional and passed in, - then I don't think we\nshould store a metadata.wallClock below.\n\n> + /*\n> + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and\n> + * update the clock recovery model. Then derive the wallclock estimate for the\n> + * frame timestamp.\n> + */\n> + wallClockRecovery_->addSample();\n> + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000);\n\nAs that could be called in the pipeline handler. And to me - then I\nwould ask why are we getting the V4L2VideoDevice to manage the\nclockRecovery object..\n\n(And I think that it should 'just' be in the V4L2VideoDevice in this\ninstance if we're going to provide wall clock timestamps of buffers).\n\n\n> + }\n> +\n> if (V4L2_TYPE_IS_OUTPUT(buf.type))\n> return buffer;\n> \n> @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn()\n> return ret;\n> }\n> \n> + if (wallClockRecovery_) {\n> + /* A good moment to sample the clocks to improve the clock recovery model. */\n> + wallClockRecovery_->addSample();\n> + }\n> +\n> state_ = State::Streaming;\n> if (watchdogDuration_ && !queuedBuffers_.empty())\n> watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma\n> return {};\n> }\n> \n> +/**\n> + * \\brief Enable wall clock timestamps for this device\n> + * \\param[in] wallClockRecovery an appropriately configured ClockRecovery, or\n> + * nullptr to disable wall clocks\n\nIs there a reason we can think of for /why/ we would disable the wall\nclocks?\n\nIt doesn't seem like a 'desired' feature to me - unless the costs of\ndoing the clock recovery are prohibitively expensive? Which I wouldn't\nexpect it to be ?\n\n\n\n> + *\n> + * When buffers are dequeued, wall clock timestamps will be generated that\n> + * correspond to the frame's timestamp, as returned by V4l2.\n> + */\n> +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery)\n> +{\n> + wallClockRecovery_ = wallClockRecovery;\n> +\n> + /* Also a reasonable moment to sample the two clocks. */\n> + if (wallClockRecovery_)\n> + wallClockRecovery_->addSample();\n> +}\n> +\n> /**\n> * \\class V4L2M2MDevice\n> * \\brief Memory-to-Memory video device\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 ABB0DBE173\n\tfor ;\n\tFri, 6 Dec 2024 15:37:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 142F866175;\n\tFri, 6 Dec 2024 16:37:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 903B1618B3\n\tfor ;\n\tFri, 6 Dec 2024 16:37:24 +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 0879974C;\n\tFri, 6 Dec 2024 16:36:55 +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=\"bVBPGOWj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733499415;\n\tbh=5BqGtmxPUZao3GCE9FbKcnCEeSavmuKXfMyGJiv7YvA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=bVBPGOWjvGidLVAcwHE/uPf8AYcNsBLpCgHlT9ns3NDbLZdPjDxZmCnkh52Q2D0q6\n\tfVLAV7OgsPY6TDRiiRraD4nDpvBgAXpaduLpRYAF8cfKwgAT0dLT6OKfireXLhWL9U\n\tGCFRUlM1m5IycfPycIgX+uePZhII0rjluR5vgLwY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241206142742.7931-4-david.plowman@raspberrypi.com>","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-4-david.plowman@raspberrypi.com>","Subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","From":"Kieran Bingham ","Cc":"David Plowman ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 06 Dec 2024 15:37:22 +0000","Message-ID":"<173349944209.312267.11949763883994764728@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":32601,"web_url":"https://patchwork.libcamera.org/comment/32601/","msgid":"","date":"2024-12-06T16:53:04","subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","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:37, Kieran Bingham\n wrote:\n>\n> Quoting David Plowman (2024-12-06 14:27:40)\n> > FrameMetadata is extended to include a wallclock timestamp.\n> >\n> > When a frame is dequeued, we use the ClockRecovery class to generate a\n> > good estimate of a wallclock timestamp to correspond to the frame's\n> > SensorTimestamp.\n> >\n> > Pipeline handlers must enable wallclocks for chosen devices by passing\n> > an appropriate ClockRecovery object.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/framebuffer.h | 1 +\n> > include/libcamera/internal/v4l2_videodevice.h | 5 +++\n> > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++-\n> > 3 files changed, 41 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index ff839243..4579d0c6 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -35,6 +35,7 @@ struct FrameMetadata {\n> > Status status;\n> > unsigned int sequence;\n> > uint64_t timestamp;\n> > + uint64_t wallClock;\n>\n> This breaks in the CI build I'm afraid:\n>\n> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569\n>\n>\n> [481/650] Generating Documentation/doxygen-internal with a custom command\n> FAILED: Documentation/internal-api-html\n> /usr/bin/doxygen Documentation/Doxyfile-internal\n> /builds/camera/libcamera/include/libcamera/framebuffer.h:38: error: Member wallClock (variable) of struct libcamera::FrameMetadata is not documented. (warning treated as error, aborting now)\n>\n> Either we need to add the doc (which is what I'd do) - or potentially\n> depending on discussions below, remove the addition...\n\nYep, well I think this is a bit above my pay grade. The \"wall clock\"\nsure feels like it belongs with the regular timestamp. If that's a\nproblem, I'm happy to put it somewhere else, though I don't think I\nwould know where currently. Perhaps some others can offer an opinion\nhere?\n\n>\n> >\n> > Span planes() { return planes_; }\n> > Span planes() const { return planes_; }\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index f021c2a0..5327c112 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -37,6 +37,7 @@\n> >\n> > namespace libcamera {\n> >\n> > +class ClockRecovery;\n> > class EventNotifier;\n> > class MediaDevice;\n> > class MediaEntity;\n> > @@ -232,6 +233,8 @@ public:\n> >\n> > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;\n> >\n> > + void enableWallClock(ClockRecovery *wallClockRecovery);\n> > +\n> > protected:\n> > std::string logPrefix() const override;\n> >\n> > @@ -290,6 +293,8 @@ private:\n> >\n> > Timer watchdog_;\n> > utils::Duration watchdogDuration_;\n> > +\n> > + ClockRecovery *wallClockRecovery_;\n> > };\n> >\n> > class V4L2M2MDevice\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index a5cf6784..2007dffc 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -26,6 +26,7 @@\n> > #include \n> > #include \n> >\n> > +#include \"libcamera/internal/clock_recovery.h\"\n> > #include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/framebuffer.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f)\n> > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> > fdBufferNotifier_(nullptr), state_(State::Stopped),\n> > - watchdogDuration_(0.0)\n> > + watchdogDuration_(0.0), wallClockRecovery_(nullptr)\n> > {\n> > /*\n> > * We default to an MMAP based CAPTURE video device, however this will\n> > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> > + buf.timestamp.tv_usec * 1000ULL;\n> >\n> > + metadata.wallClock = 0;\n> > + if (wallClockRecovery_) {\n>\n> I know Laurent discussed passing in the 'ClockRecovery' class as a way\n> of giving the responsibility to the Pipeline handler - but I'm not\n> particularly fond of it. (I won't object if this is preferred all\n> round).\n>\n> To me - especially if there is an addition of uint64_t wallClock in the\n> FrameMetadata, then this should just always be enabled.\n>\n> But if it's /really/ optional and passed in, - then I don't think we\n> should store a metadata.wallClock below.\n\nIndeed, I don't actually feel terribly strongly either, I'd obviously\nprefer to reach a consensus ahead of time rather than produce\ndifferent implementations that we then don't like!\n\nI'd certainly be happy always to enable wall clocks, it depends\nwhether we think the burden of a bit of floating point arithmetic\nevery time we add a sample is a problem. Probably not.\n\nTo be fair, I do wonder a little bit whether a single ClockRecovery\nfor the entire system might be the right thing to do. Though I'm not\nsure where it would go, or how you would add sporadic samples to it.\nBut you could easily rate-limit the model updates, if you wanted to.\n\nOn the downside, that would certainly make it more difficult for\nanyone to configure it differently, but maybe no one would really need\nto? Questions, questions...\n\n>\n> > + /*\n> > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and\n> > + * update the clock recovery model. Then derive the wallclock estimate for the\n> > + * frame timestamp.\n> > + */\n> > + wallClockRecovery_->addSample();\n> > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000);\n>\n> As that could be called in the pipeline handler. And to me - then I\n> would ask why are we getting the V4L2VideoDevice to manage the\n> clockRecovery object..\n>\n> (And I think that it should 'just' be in the V4L2VideoDevice in this\n> instance if we're going to provide wall clock timestamps of buffers).\n\nI would agree that I think I'm against moving more of it into the PH.\nAfter all, the whole point was to put it here so that everyone can get\nthe new feature easily. But I don't super-love the\npass-the-ClockRecovery-in either. I can certainly see that argument\nfor a single \"global\" ClockRecovery. I dunno...\n\n>\n>\n> > + }\n> > +\n> > if (V4L2_TYPE_IS_OUTPUT(buf.type))\n> > return buffer;\n> >\n> > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn()\n> > return ret;\n> > }\n> >\n> > + if (wallClockRecovery_) {\n> > + /* A good moment to sample the clocks to improve the clock recovery model. */\n> > + wallClockRecovery_->addSample();\n> > + }\n> > +\n> > state_ = State::Streaming;\n> > if (watchdogDuration_ && !queuedBuffers_.empty())\n> > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma\n> > return {};\n> > }\n> >\n> > +/**\n> > + * \\brief Enable wall clock timestamps for this device\n> > + * \\param[in] wallClockRecovery an appropriately configured ClockRecovery, or\n> > + * nullptr to disable wall clocks\n>\n> Is there a reason we can think of for /why/ we would disable the wall\n> clocks?\n>\n> It doesn't seem like a 'desired' feature to me - unless the costs of\n> doing the clock recovery are prohibitively expensive? Which I wouldn't\n> expect it to be ?\n\nYeah, no reason... just no compelling reason not to allow it either.\nHaving them \"always enabled\" feels nice in many ways...\n\nThanks!\nDavid\n\n>\n>\n>\n> > + *\n> > + * When buffers are dequeued, wall clock timestamps will be generated that\n> > + * correspond to the frame's timestamp, as returned by V4l2.\n> > + */\n> > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery)\n> > +{\n> > + wallClockRecovery_ = wallClockRecovery;\n> > +\n> > + /* Also a reasonable moment to sample the two clocks. */\n> > + if (wallClockRecovery_)\n> > + wallClockRecovery_->addSample();\n> > +}\n> > +\n> > /**\n> > * \\class V4L2M2MDevice\n> > * \\brief Memory-to-Memory video device\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 3AB53BE173\n\tfor ;\n\tFri, 6 Dec 2024 16:53:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 675D567E23;\n\tFri, 6 Dec 2024 17:53:19 +0100 (CET)","from mail-qk1-x72c.google.com (mail-qk1-x72c.google.com\n\t[IPv6:2607:f8b0:4864:20::72c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 25618618B3\n\tfor ;\n\tFri, 6 Dec 2024 17:53:17 +0100 (CET)","by mail-qk1-x72c.google.com with SMTP id\n\taf79cd13be357-7b673aacf13so253834785a.0\n\tfor ;\n\tFri, 06 Dec 2024 08:53:17 -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=\"dNofbMJv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1733503996; x=1734108796;\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=LcfpfQrzbpLji9e8aEgZ+6GQiPOuINAbsOWEdZ7Vx6E=;\n\tb=dNofbMJv77n8Mnw0KNugW+zYtjWCr0NIBzaZZj3orBCgrn0pPWlXcesE+LlpiEloYr\n\tRWa3lenJtotszaqDPZm2gyZuDBZOQx5V1kjedS8I+Q5W69uZCT3uqicmI181+QLSUcqY\n\tDrTas3oEVTVKDdC3kQ4S8m81rOM238FRWbTYHdgGHissK6HDtyaEtyAIyXHI4Ll7cJZV\n\tLaa61sXfcp83cSUWbD/RvnD+WiCRtslKOduGKvsEAUDuEhZlgpiGB4hSkt8LlTAASyZS\n\t33rngjfZ4G04/QWFC52kwykGo99vMrjADq1vmAwoMH/Hb/4jEKkpP4co8k2nqjXhTlzz\n\tllQg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733503996; x=1734108796;\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=LcfpfQrzbpLji9e8aEgZ+6GQiPOuINAbsOWEdZ7Vx6E=;\n\tb=ms6kqq8vpvbkUiFEe3+z1FsMhIUplBXMcU5C/z+D6ZMY1aW5REKTqotOENRvCFJ8c/\n\t//rb9Rio7utfmr+txYWNNj/fH7BZrIhsrfq4XMUFpRomCHRsNlcR1Vox8b+7CaIxzQe4\n\ts2dw57CNvn+O6LypS+cFCH2JwsBoGNKiN7DjojPYp62ls0o7FypC2kinvA3AvqG4QHij\n\twT2PQphxFKl+OOAWzma2C/vlyFAlPsqYNdAKFio2KPNw2pNpzI9jcIIvaBGBfgx5IwvB\n\tMAqxE+CXzPJ4yvsBKHVt7I8PR5ZZeV/gS85S4CqmUxZkE1LkgvH5ViXesQKgCelmg7CT\n\t5ztA==","X-Gm-Message-State":"AOJu0Yyv8sVkya8/H4zEJ8zC1HQkJvG6xUO1WVVFkgVMPDCXEJMjnkS/\n\tIQvGscT6sU+TZhh8NztFaCJOji7vt5oWlTuMntWylNcdgEuiLZzdV2ACPkGEK7CQBp+zXFPp1G5\n\tdJ8YrrCfCTpLMVCQ/TkWzXU1z1XnlhDSSGe5RUw==","X-Gm-Gg":"ASbGnctKoykQlJ8kUq9e8D72SfVSIIaW8iSaZfGGhO+q95cHntcJilite0INIDiyOLg\n\tP4beGgFn+Xrvi1aRo7qTm7v9I6leKneE=","X-Google-Smtp-Source":"AGHT+IF/x6tjy4/fgIrn3ep4O7/m2Ctp9A9VeVZAfu+G/AIMAqINOgnFW0xVaUI8d/IVTGuhiMEMqLjJS/YS8i8EIJs=","X-Received":"by 2002:a05:620a:17a0:b0:7b1:7508:9f38 with SMTP id\n\taf79cd13be357-7b6b4187b0cmr1214772885a.16.1733503995905;\n\tFri, 06 Dec 2024 08:53:15 -0800 (PST)","MIME-Version":"1.0","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-4-david.plowman@raspberrypi.com>\n\t<173349944209.312267.11949763883994764728@ping.linuxembedded.co.uk>","In-Reply-To":"<173349944209.312267.11949763883994764728@ping.linuxembedded.co.uk>","From":"David Plowman ","Date":"Fri, 6 Dec 2024 16:53:04 +0000","Message-ID":"","Subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","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":32706,"web_url":"https://patchwork.libcamera.org/comment/32706/","msgid":"","date":"2024-12-12T13:04:11","subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\n\nOn Fri, 6 Dec 2024 at 16:53, David Plowman\n wrote:\n>\n> Hi Kieran\n>\n> Thanks for the review!\n>\n> On Fri, 6 Dec 2024 at 15:37, Kieran Bingham\n> wrote:\n> >\n> > Quoting David Plowman (2024-12-06 14:27:40)\n> > > FrameMetadata is extended to include a wallclock timestamp.\n> > >\n> > > When a frame is dequeued, we use the ClockRecovery class to generate a\n> > > good estimate of a wallclock timestamp to correspond to the frame's\n> > > SensorTimestamp.\n> > >\n> > > Pipeline handlers must enable wallclocks for chosen devices by passing\n> > > an appropriate ClockRecovery object.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/framebuffer.h | 1 +\n> > > include/libcamera/internal/v4l2_videodevice.h | 5 +++\n> > > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++-\n> > > 3 files changed, 41 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index ff839243..4579d0c6 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -35,6 +35,7 @@ struct FrameMetadata {\n> > > Status status;\n> > > unsigned int sequence;\n> > > uint64_t timestamp;\n> > > + uint64_t wallClock;\n> >\n> > This breaks in the CI build I'm afraid:\n> >\n> > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569\n> >\n> >\n> > [481/650] Generating Documentation/doxygen-internal with a custom command\n> > FAILED: Documentation/internal-api-html\n> > /usr/bin/doxygen Documentation/Doxyfile-internal\n> > /builds/camera/libcamera/include/libcamera/framebuffer.h:38: error: Member wallClock (variable) of struct libcamera::FrameMetadata is not documented. (warning treated as error, aborting now)\n> >\n> > Either we need to add the doc (which is what I'd do) - or potentially\n> > depending on discussions below, remove the addition...\n>\n> Yep, well I think this is a bit above my pay grade. The \"wall clock\"\n> sure feels like it belongs with the regular timestamp. If that's a\n> problem, I'm happy to put it somewhere else, though I don't think I\n> would know where currently. Perhaps some others can offer an opinion\n> here?\n>\n> >\n> > >\n> > > Span planes() { return planes_; }\n> > > Span planes() const { return planes_; }\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index f021c2a0..5327c112 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -37,6 +37,7 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > +class ClockRecovery;\n> > > class EventNotifier;\n> > > class MediaDevice;\n> > > class MediaEntity;\n> > > @@ -232,6 +233,8 @@ public:\n> > >\n> > > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;\n> > >\n> > > + void enableWallClock(ClockRecovery *wallClockRecovery);\n> > > +\n> > > protected:\n> > > std::string logPrefix() const override;\n> > >\n> > > @@ -290,6 +293,8 @@ private:\n> > >\n> > > Timer watchdog_;\n> > > utils::Duration watchdogDuration_;\n> > > +\n> > > + ClockRecovery *wallClockRecovery_;\n> > > };\n> > >\n> > > class V4L2M2MDevice\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index a5cf6784..2007dffc 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -26,6 +26,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \"libcamera/internal/clock_recovery.h\"\n> > > #include \"libcamera/internal/formats.h\"\n> > > #include \"libcamera/internal/framebuffer.h\"\n> > > #include \"libcamera/internal/media_device.h\"\n> > > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f)\n> > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> > > fdBufferNotifier_(nullptr), state_(State::Stopped),\n> > > - watchdogDuration_(0.0)\n> > > + watchdogDuration_(0.0), wallClockRecovery_(nullptr)\n> > > {\n> > > /*\n> > > * We default to an MMAP based CAPTURE video device, however this will\n> > > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> > > + buf.timestamp.tv_usec * 1000ULL;\n> > >\n> > > + metadata.wallClock = 0;\n> > > + if (wallClockRecovery_) {\n> >\n> > I know Laurent discussed passing in the 'ClockRecovery' class as a way\n> > of giving the responsibility to the Pipeline handler - but I'm not\n> > particularly fond of it. (I won't object if this is preferred all\n> > round).\n> >\n> > To me - especially if there is an addition of uint64_t wallClock in the\n> > FrameMetadata, then this should just always be enabled.\n> >\n> > But if it's /really/ optional and passed in, - then I don't think we\n> > should store a metadata.wallClock below.\n>\n> Indeed, I don't actually feel terribly strongly either, I'd obviously\n> prefer to reach a consensus ahead of time rather than produce\n> different implementations that we then don't like!\n>\n> I'd certainly be happy always to enable wall clocks, it depends\n> whether we think the burden of a bit of floating point arithmetic\n> every time we add a sample is a problem. Probably not.\n>\n> To be fair, I do wonder a little bit whether a single ClockRecovery\n> for the entire system might be the right thing to do. Though I'm not\n> sure where it would go, or how you would add sporadic samples to it.\n> But you could easily rate-limit the model updates, if you wanted to.\n>\n> On the downside, that would certainly make it more difficult for\n> anyone to configure it differently, but maybe no one would really need\n> to? Questions, questions...\n>\n> >\n> > > + /*\n> > > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and\n> > > + * update the clock recovery model. Then derive the wallclock estimate for the\n> > > + * frame timestamp.\n> > > + */\n> > > + wallClockRecovery_->addSample();\n> > > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000);\n> >\n> > As that could be called in the pipeline handler. And to me - then I\n> > would ask why are we getting the V4L2VideoDevice to manage the\n> > clockRecovery object..\n> >\n> > (And I think that it should 'just' be in the V4L2VideoDevice in this\n> > instance if we're going to provide wall clock timestamps of buffers).\n>\n> I would agree that I think I'm against moving more of it into the PH.\n> After all, the whole point was to put it here so that everyone can get\n> the new feature easily. But I don't super-love the\n> pass-the-ClockRecovery-in either. I can certainly see that argument\n> for a single \"global\" ClockRecovery. I dunno...\n\nI tend to agree with Kieran that perhaps the wall clock recovery\nbecomes a standard feature of the V4L2VideoDevice and the framebuffer\nmetadata.wallClock field will be unconditionally valid. The amount of\nadditional computation per frame (per device) is quite tiny, and I\ndoubt it would make any measurable impact. As such I would suggest we\nmove the instantiation of the ClockRecovery object into\nV4L2VideoDevice.\n\nAs for specifying custom clock recovery config, perhaps we just ignore\nthis problem (if it is indeed a problem) until we have a definitive\nneed to change it. Since this is all in libcamera internals, we have\nsome leeway with changing this in the future.\n\nAny objections to this?\n\nNaush\n\n>\n> >\n> >\n> > > + }\n> > > +\n> > > if (V4L2_TYPE_IS_OUTPUT(buf.type))\n> > > return buffer;\n> > >\n> > > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn()\n> > > return ret;\n> > > }\n> > >\n> > > + if (wallClockRecovery_) {\n> > > + /* A good moment to sample the clocks to improve the clock recovery model. */\n> > > + wallClockRecovery_->addSample();\n> > > + }\n> > > +\n> > > state_ = State::Streaming;\n> > > if (watchdogDuration_ && !queuedBuffers_.empty())\n> > > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma\n> > > return {};\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Enable wall clock timestamps for this device\n> > > + * \\param[in] wallClockRecovery an appropriately configured ClockRecovery, or\n> > > + * nullptr to disable wall clocks\n> >\n> > Is there a reason we can think of for /why/ we would disable the wall\n> > clocks?\n> >\n> > It doesn't seem like a 'desired' feature to me - unless the costs of\n> > doing the clock recovery are prohibitively expensive? Which I wouldn't\n> > expect it to be ?\n>\n> Yeah, no reason... just no compelling reason not to allow it either.\n> Having them \"always enabled\" feels nice in many ways...\n>\n> Thanks!\n> David\n>\n> >\n> >\n> >\n> > > + *\n> > > + * When buffers are dequeued, wall clock timestamps will be generated that\n> > > + * correspond to the frame's timestamp, as returned by V4l2.\n> > > + */\n> > > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery)\n> > > +{\n> > > + wallClockRecovery_ = wallClockRecovery;\n> > > +\n> > > + /* Also a reasonable moment to sample the two clocks. */\n> > > + if (wallClockRecovery_)\n> > > + wallClockRecovery_->addSample();\n> > > +}\n> > > +\n> > > /**\n> > > * \\class V4L2M2MDevice\n> > > * \\brief Memory-to-Memory video device\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 21BD2C3260\n\tfor ;\n\tThu, 12 Dec 2024 13:04:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 651B4608B6;\n\tThu, 12 Dec 2024 14:04:51 +0100 (CET)","from mail-yw1-x1131.google.com (mail-yw1-x1131.google.com\n\t[IPv6:2607:f8b0:4864:20::1131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE8CC608B6\n\tfor ;\n\tThu, 12 Dec 2024 14:04:48 +0100 (CET)","by mail-yw1-x1131.google.com with SMTP id\n\t00721157ae682-6f012c16cccso244767b3.1\n\tfor ;\n\tThu, 12 Dec 2024 05:04:48 -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=\"pqW5zazI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1734008687; x=1734613487;\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=64qxz/2Y+r/hPVfYDyPl/MmGvVImCeLumyNkgGINeKc=;\n\tb=pqW5zazIMDUFHVBZjz29zyynxXGfbVXwXTL3xNL70dKHk70S+9SC6xdMhx/oZYORhb\n\tAxkHgnTbzoJ2Algdr4CfMmrTTxjH339+H8Ql36Isgb/JNLKrownAnIwLe+uygFdnd1wg\n\tjRlW2o1ischVUXpHdDuigtfXYaz4j+dnsk/95NO/QvZCnQM83VdaYrl1AlMwjZCVRUld\n\tYog7bsgOVdqiAUEqaq9begknxkxIP8gwv1sj15dOb1gvlf90iLUxoKdo/OPJ+IwygqZk\n\t64coXkKiDW2ETdsFI4dUyQoweMFGlM81/nc5BA9EMgEY8j9g1Q6OabcQ2KgCWQbxpujt\n\tgaMA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1734008687; x=1734613487;\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=64qxz/2Y+r/hPVfYDyPl/MmGvVImCeLumyNkgGINeKc=;\n\tb=R4a8oUUOYcFimDT6J/yBFVmxIT3rc1FIzTl+pZN7p17c3jlO0lPoEIdW/rSM2ucAfK\n\taZuaYR3RD5IaHJotktzcvvj7/3yqGjk+gB1UGBa3Z6XHx0LqXWmTZVsutkB76q2ZYr1H\n\t/+o6gPvFzcM+hINKfC6Toh7GwAyT1M81V+7wKMaUtwaznB8XCuBc85VTh1a1zUIhNclr\n\tUzlWwvQSKZ9W+hPsVJMR0zn7loqCx1SYz67lIQbg/EOtmS/uJ6w4YYB+sKJZkp5KXRZr\n\tggzywIwjIpvmnHPEsGiOlMd/dOagHKHS7CCJrx9f5+7ZBq+plztwXApUrBmvKvSy4yRf\n\t+afg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWyU7r166myqjWlelL/Pv5qxij5YGHEN8N/ipUhLVXgslCxBWbiJ7nKF0hqMs2GcafFSCWyGX5l9TKUkwv1R5o=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyMgDcgihMVt4QzgjrL+BF7+ZQwvsLRGo3OcI33+MCeybkLnPNx\n\tp6omDjPDjtjOgeAKJdkQa/ELs9oyQQEmdyXjJuRup+3BKECBWMmXtk7q8QeWLrd9opcxOT2Nx75\n\tfTGJKOJ/1n6QRStYkFA9XGfOGMIe/iIZDLnwqZQ==","X-Gm-Gg":"ASbGnctL+16TLIsSx0uHH6BdVYFWEDHLYONiLC6DYddPbtxwd6VyUztUh0jZByzKvRu\n\tXkivXRwO0zFCwI4gMpjwAnejWLQgajJ1LREmahWPGW6SnSZxli1yKexA0+F1UpPXeqVUDZA==","X-Google-Smtp-Source":"AGHT+IGS4CHc9lb2b5ADfcs8M0rP6MTICqzMH7r4HsKONeFk8ZjwyttTHnZYuN6wa904T30CIzMiexpbfEy+gha86NU=","X-Received":"by 2002:a05:690c:39d:b0:6ef:606c:12e7 with SMTP id\n\t00721157ae682-6f27527c1ccmr1489377b3.3.1734008687296; Thu, 12 Dec 2024\n\t05:04:47 -0800 (PST)","MIME-Version":"1.0","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-4-david.plowman@raspberrypi.com>\n\t<173349944209.312267.11949763883994764728@ping.linuxembedded.co.uk>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 12 Dec 2024 13:04:11 +0000","Message-ID":"","Subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","To":"David Plowman ","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\" "}},{"id":32861,"web_url":"https://patchwork.libcamera.org/comment/32861/","msgid":"<20241218023547.GZ23470@pendragon.ideasonboard.com>","date":"2024-12-18T02:35:47","subject":"Re: [PATCH 3/5] 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":"On Thu, Dec 12, 2024 at 01:04:11PM +0000, Naushir Patuck wrote:\n> On Fri, 6 Dec 2024 at 16:53, David Plowman wrote:\n> > On Fri, 6 Dec 2024 at 15:37, Kieran Bingham wrote:\n> > > Quoting David Plowman (2024-12-06 14:27:40)\n> > > > FrameMetadata is extended to include a wallclock timestamp.\n> > > >\n> > > > When a frame is dequeued, we use the ClockRecovery class to generate a\n> > > > good estimate of a wallclock timestamp to correspond to the frame's\n> > > > SensorTimestamp.\n> > > >\n> > > > Pipeline handlers must enable wallclocks for chosen devices by passing\n> > > > an appropriate ClockRecovery object.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > include/libcamera/framebuffer.h | 1 +\n> > > > include/libcamera/internal/v4l2_videodevice.h | 5 +++\n> > > > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++-\n> > > > 3 files changed, 41 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > index ff839243..4579d0c6 100644\n> > > > --- a/include/libcamera/framebuffer.h\n> > > > +++ b/include/libcamera/framebuffer.h\n> > > > @@ -35,6 +35,7 @@ struct FrameMetadata {\n> > > > Status status;\n> > > > unsigned int sequence;\n> > > > uint64_t timestamp;\n> > > > + uint64_t wallClock;\n> > >\n> > > This breaks in the CI build I'm afraid:\n> > >\n> > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569\n> > >\n> > >\n> > > [481/650] Generating Documentation/doxygen-internal with a custom command\n> > > FAILED: Documentation/internal-api-html\n> > > /usr/bin/doxygen Documentation/Doxyfile-internal\n> > > /builds/camera/libcamera/include/libcamera/framebuffer.h:38: error: Member wallClock (variable) of struct libcamera::FrameMetadata is not documented. (warning treated as error, aborting now)\n> > >\n> > > Either we need to add the doc (which is what I'd do) - or potentially\n> > > depending on discussions below, remove the addition...\n> >\n> > Yep, well I think this is a bit above my pay grade. The \"wall clock\"\n> > sure feels like it belongs with the regular timestamp. If that's a\n> > problem, I'm happy to put it somewhere else, though I don't think I\n> > would know where currently. Perhaps some others can offer an opinion\n> > here?\n\nEven if I'm still considering whether or not we should remove timestamp\nfrom FrameMetadata, I think it makes sense to have either none or both,\nso it's fine to keep it here. What's missing is documentation.\n\n> > > >\n> > > > Span planes() { return planes_; }\n> > > > Span planes() const { return planes_; }\n> > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > > index f021c2a0..5327c112 100644\n> > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > @@ -37,6 +37,7 @@\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > +class ClockRecovery;\n> > > > class EventNotifier;\n> > > > class MediaDevice;\n> > > > class MediaEntity;\n> > > > @@ -232,6 +233,8 @@ public:\n> > > >\n> > > > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;\n> > > >\n> > > > + void enableWallClock(ClockRecovery *wallClockRecovery);\n> > > > +\n> > > > protected:\n> > > > std::string logPrefix() const override;\n> > > >\n> > > > @@ -290,6 +293,8 @@ private:\n> > > >\n> > > > Timer watchdog_;\n> > > > utils::Duration watchdogDuration_;\n> > > > +\n> > > > + ClockRecovery *wallClockRecovery_;\n> > > > };\n> > > >\n> > > > class V4L2M2MDevice\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index a5cf6784..2007dffc 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -26,6 +26,7 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/clock_recovery.h\"\n> > > > #include \"libcamera/internal/formats.h\"\n> > > > #include \"libcamera/internal/framebuffer.h\"\n> > > > #include \"libcamera/internal/media_device.h\"\n> > > > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f)\n> > > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> > > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> > > > fdBufferNotifier_(nullptr), state_(State::Stopped),\n> > > > - watchdogDuration_(0.0)\n> > > > + watchdogDuration_(0.0), wallClockRecovery_(nullptr)\n> > > > {\n> > > > /*\n> > > > * We default to an MMAP based CAPTURE video device, however this will\n> > > > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> > > > + buf.timestamp.tv_usec * 1000ULL;\n> > > >\n> > > > + metadata.wallClock = 0;\n> > > > + if (wallClockRecovery_) {\n> > >\n> > > I know Laurent discussed passing in the 'ClockRecovery' class as a way\n> > > of giving the responsibility to the Pipeline handler - but I'm not\n> > > particularly fond of it. (I won't object if this is preferred all\n> > > round).\n> > >\n> > > To me - especially if there is an addition of uint64_t wallClock in the\n> > > FrameMetadata, then this should just always be enabled.\n> > >\n> > > But if it's /really/ optional and passed in, - then I don't think we\n> > > should store a metadata.wallClock below.\n> >\n> > Indeed, I don't actually feel terribly strongly either, I'd obviously\n> > prefer to reach a consensus ahead of time rather than produce\n> > different implementations that we then don't like!\n> >\n> > I'd certainly be happy always to enable wall clocks, it depends\n> > whether we think the burden of a bit of floating point arithmetic\n> > every time we add a sample is a problem. Probably not.\n> >\n> > To be fair, I do wonder a little bit whether a single ClockRecovery\n> > for the entire system might be the right thing to do. Though I'm not\n> > sure where it would go, or how you would add sporadic samples to it.\n> > But you could easily rate-limit the model updates, if you wanted to.\n> >\n> > On the downside, that would certainly make it more difficult for\n> > anyone to configure it differently, but maybe no one would really need\n> > to? Questions, questions...\n> >\n> > > > + /*\n> > > > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and\n> > > > + * update the clock recovery model. Then derive the wallclock estimate for the\n> > > > + * frame timestamp.\n> > > > + */\n> > > > + wallClockRecovery_->addSample();\n> > > > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000);\n> > >\n> > > As that could be called in the pipeline handler. And to me - then I\n> > > would ask why are we getting the V4L2VideoDevice to manage the\n> > > clockRecovery object..\n> > >\n> > > (And I think that it should 'just' be in the V4L2VideoDevice in this\n> > > instance if we're going to provide wall clock timestamps of buffers).\n> >\n> > I would agree that I think I'm against moving more of it into the PH.\n> > After all, the whole point was to put it here so that everyone can get\n> > the new feature easily. But I don't super-love the\n> > pass-the-ClockRecovery-in either. I can certainly see that argument\n> > for a single \"global\" ClockRecovery. I dunno...\n> \n> I tend to agree with Kieran that perhaps the wall clock recovery\n> becomes a standard feature of the V4L2VideoDevice and the framebuffer\n> metadata.wallClock field will be unconditionally valid. The amount of\n> additional computation per frame (per device) is quite tiny, and I\n> doubt it would make any measurable impact. As such I would suggest we\n> move the instantiation of the ClockRecovery object into\n> V4L2VideoDevice.\n> \n> As for specifying custom clock recovery config, perhaps we just ignore\n> this problem (if it is indeed a problem) until we have a definitive\n> need to change it. Since this is all in libcamera internals, we have\n> some leeway with changing this in the future.\n> \n> Any objections to this?\n> \n> > > > + }\n> > > > +\n> > > > if (V4L2_TYPE_IS_OUTPUT(buf.type))\n> > > > return buffer;\n> > > >\n> > > > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn()\n> > > > return ret;\n> > > > }\n> > > >\n> > > > + if (wallClockRecovery_) {\n> > > > + /* A good moment to sample the clocks to improve the clock recovery model. */\n> > > > + wallClockRecovery_->addSample();\n> > > > + }\n> > > > +\n> > > > state_ = State::Streaming;\n> > > > if (watchdogDuration_ && !queuedBuffers_.empty())\n> > > > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > > > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma\n> > > > return {};\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Enable wall clock timestamps for this device\n> > > > + * \\param[in] wallClockRecovery an appropriately configured ClockRecovery, or\n> > > > + * nullptr to disable wall clocks\n> > >\n> > > Is there a reason we can think of for /why/ we would disable the wall\n> > > clocks?\n> > >\n> > > It doesn't seem like a 'desired' feature to me - unless the costs of\n> > > doing the clock recovery are prohibitively expensive? Which I wouldn't\n> > > expect it to be ?\n> >\n> > Yeah, no reason... just no compelling reason not to allow it either.\n> > Having them \"always enabled\" feels nice in many ways...\n\nIsn't it just a waste of CPU cycles on the video devices that correspond\nto the input and output of the ISP, or to the embedded data capture ?\nThat's why I don't think this belongs to the V4L2VideoDevice class. The\nneed is to timestamp requests, not frame buffers individually. I'd\nrather try to make this simple to wire up in pipeline handlers.\n\n> > > > + *\n> > > > + * When buffers are dequeued, wall clock timestamps will be generated that\n> > > > + * correspond to the frame's timestamp, as returned by V4l2.\n> > > > + */\n> > > > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery)\n> > > > +{\n> > > > + wallClockRecovery_ = wallClockRecovery;\n> > > > +\n> > > > + /* Also a reasonable moment to sample the two clocks. */\n> > > > + if (wallClockRecovery_)\n> > > > + wallClockRecovery_->addSample();\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\class V4L2M2MDevice\n> > > > * \\brief Memory-to-Memory video device","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 889C8C3301\n\tfor ;\n\tWed, 18 Dec 2024 02:35:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFAC268075;\n\tWed, 18 Dec 2024 03:35:51 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCCD261898\n\tfor ;\n\tWed, 18 Dec 2024 03:35:49 +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 13750670;\n\tWed, 18 Dec 2024 03:35:12 +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=\"ORkd0so5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734489312;\n\tbh=drOM6StNhBJZAqZ1AMe7TV3eybITSQBWVdpjM7L3Uug=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ORkd0so5fmY+E7gjldNz6v1GgpSUnUi8sf31aaH5WnlOEZhEQnCta5Frw75xvwdhK\n\t/+2MWQPOr6x9sb4sGpV8pu6CvbVhaUzQn55zU9ZYyokbLtmV0rUdMBdEtq5MXrS5yU\n\tdLn05xCsWbTXlrek2BZ2xXWR2LAPjskW0goI6xu4=","Date":"Wed, 18 Dec 2024 04:35:47 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"David Plowman ,\n\tKieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","Message-ID":"<20241218023547.GZ23470@pendragon.ideasonboard.com>","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-4-david.plowman@raspberrypi.com>\n\t<173349944209.312267.11949763883994764728@ping.linuxembedded.co.uk>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":32867,"web_url":"https://patchwork.libcamera.org/comment/32867/","msgid":"","date":"2024-12-18T10:19:51","subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","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 comments!\n\nOn Wed, 18 Dec 2024 at 02:35, Laurent Pinchart\n wrote:\n>\n> On Thu, Dec 12, 2024 at 01:04:11PM +0000, Naushir Patuck wrote:\n> > On Fri, 6 Dec 2024 at 16:53, David Plowman wrote:\n> > > On Fri, 6 Dec 2024 at 15:37, Kieran Bingham wrote:\n> > > > Quoting David Plowman (2024-12-06 14:27:40)\n> > > > > FrameMetadata is extended to include a wallclock timestamp.\n> > > > >\n> > > > > When a frame is dequeued, we use the ClockRecovery class to generate a\n> > > > > good estimate of a wallclock timestamp to correspond to the frame's\n> > > > > SensorTimestamp.\n> > > > >\n> > > > > Pipeline handlers must enable wallclocks for chosen devices by passing\n> > > > > an appropriate ClockRecovery object.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/framebuffer.h | 1 +\n> > > > > include/libcamera/internal/v4l2_videodevice.h | 5 +++\n> > > > > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++-\n> > > > > 3 files changed, 41 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > > index ff839243..4579d0c6 100644\n> > > > > --- a/include/libcamera/framebuffer.h\n> > > > > +++ b/include/libcamera/framebuffer.h\n> > > > > @@ -35,6 +35,7 @@ struct FrameMetadata {\n> > > > > Status status;\n> > > > > unsigned int sequence;\n> > > > > uint64_t timestamp;\n> > > > > + uint64_t wallClock;\n> > > >\n> > > > This breaks in the CI build I'm afraid:\n> > > >\n> > > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569\n> > > >\n> > > >\n> > > > [481/650] Generating Documentation/doxygen-internal with a custom command\n> > > > FAILED: Documentation/internal-api-html\n> > > > /usr/bin/doxygen Documentation/Doxyfile-internal\n> > > > /builds/camera/libcamera/include/libcamera/framebuffer.h:38: error: Member wallClock (variable) of struct libcamera::FrameMetadata is not documented. (warning treated as error, aborting now)\n> > > >\n> > > > Either we need to add the doc (which is what I'd do) - or potentially\n> > > > depending on discussions below, remove the addition...\n> > >\n> > > Yep, well I think this is a bit above my pay grade. The \"wall clock\"\n> > > sure feels like it belongs with the regular timestamp. If that's a\n> > > problem, I'm happy to put it somewhere else, though I don't think I\n> > > would know where currently. Perhaps some others can offer an opinion\n> > > here?\n>\n> Even if I'm still considering whether or not we should remove timestamp\n> from FrameMetadata, I think it makes sense to have either none or both,\n> so it's fine to keep it here. What's missing is documentation.\n>\n> > > > >\n> > > > > Span planes() { return planes_; }\n> > > > > Span planes() const { return planes_; }\n> > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > index f021c2a0..5327c112 100644\n> > > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > @@ -37,6 +37,7 @@\n> > > > >\n> > > > > namespace libcamera {\n> > > > >\n> > > > > +class ClockRecovery;\n> > > > > class EventNotifier;\n> > > > > class MediaDevice;\n> > > > > class MediaEntity;\n> > > > > @@ -232,6 +233,8 @@ public:\n> > > > >\n> > > > > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;\n> > > > >\n> > > > > + void enableWallClock(ClockRecovery *wallClockRecovery);\n> > > > > +\n> > > > > protected:\n> > > > > std::string logPrefix() const override;\n> > > > >\n> > > > > @@ -290,6 +293,8 @@ private:\n> > > > >\n> > > > > Timer watchdog_;\n> > > > > utils::Duration watchdogDuration_;\n> > > > > +\n> > > > > + ClockRecovery *wallClockRecovery_;\n> > > > > };\n> > > > >\n> > > > > class V4L2M2MDevice\n> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > > index a5cf6784..2007dffc 100644\n> > > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > > @@ -26,6 +26,7 @@\n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > +#include \"libcamera/internal/clock_recovery.h\"\n> > > > > #include \"libcamera/internal/formats.h\"\n> > > > > #include \"libcamera/internal/framebuffer.h\"\n> > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f)\n> > > > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> > > > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> > > > > fdBufferNotifier_(nullptr), state_(State::Stopped),\n> > > > > - watchdogDuration_(0.0)\n> > > > > + watchdogDuration_(0.0), wallClockRecovery_(nullptr)\n> > > > > {\n> > > > > /*\n> > > > > * We default to an MMAP based CAPTURE video device, however this will\n> > > > > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > > > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> > > > > + buf.timestamp.tv_usec * 1000ULL;\n> > > > >\n> > > > > + metadata.wallClock = 0;\n> > > > > + if (wallClockRecovery_) {\n> > > >\n> > > > I know Laurent discussed passing in the 'ClockRecovery' class as a way\n> > > > of giving the responsibility to the Pipeline handler - but I'm not\n> > > > particularly fond of it. (I won't object if this is preferred all\n> > > > round).\n> > > >\n> > > > To me - especially if there is an addition of uint64_t wallClock in the\n> > > > FrameMetadata, then this should just always be enabled.\n> > > >\n> > > > But if it's /really/ optional and passed in, - then I don't think we\n> > > > should store a metadata.wallClock below.\n> > >\n> > > Indeed, I don't actually feel terribly strongly either, I'd obviously\n> > > prefer to reach a consensus ahead of time rather than produce\n> > > different implementations that we then don't like!\n> > >\n> > > I'd certainly be happy always to enable wall clocks, it depends\n> > > whether we think the burden of a bit of floating point arithmetic\n> > > every time we add a sample is a problem. Probably not.\n> > >\n> > > To be fair, I do wonder a little bit whether a single ClockRecovery\n> > > for the entire system might be the right thing to do. Though I'm not\n> > > sure where it would go, or how you would add sporadic samples to it.\n> > > But you could easily rate-limit the model updates, if you wanted to.\n> > >\n> > > On the downside, that would certainly make it more difficult for\n> > > anyone to configure it differently, but maybe no one would really need\n> > > to? Questions, questions...\n> > >\n> > > > > + /*\n> > > > > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and\n> > > > > + * update the clock recovery model. Then derive the wallclock estimate for the\n> > > > > + * frame timestamp.\n> > > > > + */\n> > > > > + wallClockRecovery_->addSample();\n> > > > > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000);\n> > > >\n> > > > As that could be called in the pipeline handler. And to me - then I\n> > > > would ask why are we getting the V4L2VideoDevice to manage the\n> > > > clockRecovery object..\n> > > >\n> > > > (And I think that it should 'just' be in the V4L2VideoDevice in this\n> > > > instance if we're going to provide wall clock timestamps of buffers).\n> > >\n> > > I would agree that I think I'm against moving more of it into the PH.\n> > > After all, the whole point was to put it here so that everyone can get\n> > > the new feature easily. But I don't super-love the\n> > > pass-the-ClockRecovery-in either. I can certainly see that argument\n> > > for a single \"global\" ClockRecovery. I dunno...\n> >\n> > I tend to agree with Kieran that perhaps the wall clock recovery\n> > becomes a standard feature of the V4L2VideoDevice and the framebuffer\n> > metadata.wallClock field will be unconditionally valid. The amount of\n> > additional computation per frame (per device) is quite tiny, and I\n> > doubt it would make any measurable impact. As such I would suggest we\n> > move the instantiation of the ClockRecovery object into\n> > V4L2VideoDevice.\n> >\n> > As for specifying custom clock recovery config, perhaps we just ignore\n> > this problem (if it is indeed a problem) until we have a definitive\n> > need to change it. Since this is all in libcamera internals, we have\n> > some leeway with changing this in the future.\n> >\n> > Any objections to this?\n> >\n> > > > > + }\n> > > > > +\n> > > > > if (V4L2_TYPE_IS_OUTPUT(buf.type))\n> > > > > return buffer;\n> > > > >\n> > > > > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn()\n> > > > > return ret;\n> > > > > }\n> > > > >\n> > > > > + if (wallClockRecovery_) {\n> > > > > + /* A good moment to sample the clocks to improve the clock recovery model. */\n> > > > > + wallClockRecovery_->addSample();\n> > > > > + }\n> > > > > +\n> > > > > state_ = State::Streaming;\n> > > > > if (watchdogDuration_ && !queuedBuffers_.empty())\n> > > > > watchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> > > > > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma\n> > > > > return {};\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Enable wall clock timestamps for this device\n> > > > > + * \\param[in] wallClockRecovery an appropriately configured ClockRecovery, or\n> > > > > + * nullptr to disable wall clocks\n> > > >\n> > > > Is there a reason we can think of for /why/ we would disable the wall\n> > > > clocks?\n> > > >\n> > > > It doesn't seem like a 'desired' feature to me - unless the costs of\n> > > > doing the clock recovery are prohibitively expensive? Which I wouldn't\n> > > > expect it to be ?\n> > >\n> > > Yeah, no reason... just no compelling reason not to allow it either.\n> > > Having them \"always enabled\" feels nice in many ways...\n>\n> Isn't it just a waste of CPU cycles on the video devices that correspond\n> to the input and output of the ISP, or to the embedded data capture ?\n> That's why I don't think this belongs to the V4L2VideoDevice class. The\n> need is to timestamp requests, not frame buffers individually. I'd\n> rather try to make this simple to wire up in pipeline handlers.\n\nJust to be sure I've understood here, what you're saying is that you'd\nrather have this handled completely in the pipeline handlers, and have\nnothing at all in the V4L2VideoDevice? I'm fine with that, just wanted\nto clear. At that point, it doesn't feel like there's any need to have\nit in the metadata either, and it can just be a control. Does that\nsound fair?\n\nThanks!\nDavid\n\n>\n> > > > > + *\n> > > > > + * When buffers are dequeued, wall clock timestamps will be generated that\n> > > > > + * correspond to the frame's timestamp, as returned by V4l2.\n> > > > > + */\n> > > > > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery)\n> > > > > +{\n> > > > > + wallClockRecovery_ = wallClockRecovery;\n> > > > > +\n> > > > > + /* Also a reasonable moment to sample the two clocks. */\n> > > > > + if (wallClockRecovery_)\n> > > > > + wallClockRecovery_->addSample();\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\class V4L2M2MDevice\n> > > > > * \\brief Memory-to-Memory video device\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 C0A16C32FE\n\tfor ;\n\tWed, 18 Dec 2024 10:20:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE11268089;\n\tWed, 18 Dec 2024 11:20:05 +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 95E9567F24\n\tfor ;\n\tWed, 18 Dec 2024 11:20:03 +0100 (CET)","by mail-qv1-xf2d.google.com with SMTP id\n\t6a1803df08f44-6d8e8445219so54525556d6.0\n\tfor ;\n\tWed, 18 Dec 2024 02:20:03 -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=\"Ync1JVNc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1734517202; x=1735122002;\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=Y/rWqSa4LE7v0Y6oFggdA3ZXoYGROp2RWjji+lYal74=;\n\tb=Ync1JVNcE5OMKqg83WgsZhNziaBiAPhBP4//BrPM8fUDfE1oW9UeUd8bsmZYZQ2CkG\n\tCLq0yvC/SOqTng2V9xP5wnHO23L6pkgnl+wGd9470iCU9P/IRNozatZpzKVck7uHwY/n\n\t5s+gqbE3YV3/a7SH23sdYKCGud0wXMGlOfxPZg33Tby79rneTyzFcVkzy6uLgIzj2+5O\n\tkX3w/nQ21gC1aqJna4xr7l6CPUFg+yo2/YOZAFBu/YJT2+Lqo2bjlEmP//LQ4DM1/ATP\n\tY5IuCvIpwaL7fNyerhNFURtbTv+v+rk7nBNHCt3YKkKwUu9WEPls2pz/h5O6yF33oEs5\n\tuEmg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1734517202; x=1735122002;\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=Y/rWqSa4LE7v0Y6oFggdA3ZXoYGROp2RWjji+lYal74=;\n\tb=lNLrWTeybJYEVOpPuwhwgs11/7Cejdpl1aH/1AjLu/JCPembo5oAvzeqLol1RH286M\n\tbuob771TWxuk7JO2mujaSvNsCaIwtwTkn38L6Y/j6JpAoiFXF6Tryu1LQ5MMRKNnp4qS\n\t+IQP36pdjscFjAj9SpW/CpTQjVeCh7jcWdrohCOcB4P43R3iLkWmaOI4ia7cy2eYZRr/\n\tCaGnfuvap8eK7iofCZHY3cDgWWjIw+zTz/AVmql2xUWsr2A7nhcdSuse+Nj4KZz+Qf6M\n\t+d/H/AS97cFJ+yIvqbi9blXg2dRNgU1mjiTodWJVWzkRf366dAl9BCGceCoPt5MFSOpl\n\to3nA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCV1K8rwSTQLTdSsPGI+BtOWVxMtdaiBxrzX/lskpfGneCwtCfq8XrWHymytSGe+NF0KIxlbqJwpmpetWJQboIk=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxGgfr+GeTVT4lIF3fqtJURPSOAurxx8eKwRQeJi2nh5uQr/xKc\n\tO4sixnwi/h1k+OrqxYBHP4GiCq2E5cpsRYc/YF31i0nndkNJbw2RqVfwAz2YO8o1PTjGZK4Z1uB\n\tM86x1HyMo2JJK1Csy2cq9YeKrXIelCYsLnWR9cQ==","X-Gm-Gg":"ASbGncthA8uF5DfSE8y21KxrjLwZ3M6rJiwmxYCmFKK3H/UaSgwglXLClsAAZ6cYAi8\n\t/tNfFqRMJvAfdvbT+eNYXXqYg5t1s7NXnDYkzhlbymnnbOuOhRecM8Y8t30RqMwA9Y00HjQ==","X-Google-Smtp-Source":"AGHT+IHQsR9TNW8WecRCfmuLgFohuo/fHMIWse6X5AaKvZQu9A0Fk0qyltssonSoMBkmPx7e0s7tXOqqe6Sg1NWbcUg=","X-Received":"by 2002:a05:6214:2389:b0:6d8:86c8:c2a9 with SMTP id\n\t6a1803df08f44-6dd092408e3mr36226206d6.31.1734517202255;\n\tWed, 18 Dec 2024 02:20:02 -0800 (PST)","MIME-Version":"1.0","References":"<20241206142742.7931-1-david.plowman@raspberrypi.com>\n\t<20241206142742.7931-4-david.plowman@raspberrypi.com>\n\t<173349944209.312267.11949763883994764728@ping.linuxembedded.co.uk>\n\t\n\t\n\t<20241218023547.GZ23470@pendragon.ideasonboard.com>","In-Reply-To":"<20241218023547.GZ23470@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Wed, 18 Dec 2024 10:19:51 +0000","Message-ID":"","Subject":"Re: [PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video\n\tdevices","To":"Laurent Pinchart ","Cc":"Naushir Patuck ,\n\tKieran 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\" "}}]