| Message ID | 20241206142742.7931-4-david.plowman@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting David Plowman (2024-12-06 14:27:40) > FrameMetadata is extended to include a wallclock timestamp. > > When a frame is dequeued, we use the ClockRecovery class to generate a > good estimate of a wallclock timestamp to correspond to the frame's > SensorTimestamp. > > Pipeline handlers must enable wallclocks for chosen devices by passing > an appropriate ClockRecovery object. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/framebuffer.h | 1 + > include/libcamera/internal/v4l2_videodevice.h | 5 +++ > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++- > 3 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h > index ff839243..4579d0c6 100644 > --- a/include/libcamera/framebuffer.h > +++ b/include/libcamera/framebuffer.h > @@ -35,6 +35,7 @@ struct FrameMetadata { > Status status; > unsigned int sequence; > uint64_t timestamp; > + uint64_t wallClock; This breaks in the CI build I'm afraid: https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569 [481/650] Generating Documentation/doxygen-internal with a custom command FAILED: Documentation/internal-api-html /usr/bin/doxygen Documentation/Doxyfile-internal /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) Either we need to add the doc (which is what I'd do) - or potentially depending on discussions below, remove the addition... > > Span<Plane> planes() { return planes_; } > Span<const Plane> planes() const { return planes_; } > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index f021c2a0..5327c112 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -37,6 +37,7 @@ > > namespace libcamera { > > +class ClockRecovery; > class EventNotifier; > class MediaDevice; > class MediaEntity; > @@ -232,6 +233,8 @@ public: > > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const; > > + void enableWallClock(ClockRecovery *wallClockRecovery); > + > protected: > std::string logPrefix() const override; > > @@ -290,6 +293,8 @@ private: > > Timer watchdog_; > utils::Duration watchdogDuration_; > + > + ClockRecovery *wallClockRecovery_; > }; > > class V4L2M2MDevice > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index a5cf6784..2007dffc 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -26,6 +26,7 @@ > #include <libcamera/base/unique_fd.h> > #include <libcamera/base/utils.h> > > +#include "libcamera/internal/clock_recovery.h" > #include "libcamera/internal/formats.h" > #include "libcamera/internal/framebuffer.h" > #include "libcamera/internal/media_device.h" > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f) > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr), > fdBufferNotifier_(nullptr), state_(State::Stopped), > - watchdogDuration_(0.0) > + watchdogDuration_(0.0), wallClockRecovery_(nullptr) > { > /* > * We default to an MMAP based CAPTURE video device, however this will > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL > + buf.timestamp.tv_usec * 1000ULL; > > + metadata.wallClock = 0; > + if (wallClockRecovery_) { I know Laurent discussed passing in the 'ClockRecovery' class as a way of giving the responsibility to the Pipeline handler - but I'm not particularly fond of it. (I won't object if this is preferred all round). To me - especially if there is an addition of uint64_t wallClock in the FrameMetadata, then this should just always be enabled. But if it's /really/ optional and passed in, - then I don't think we should store a metadata.wallClock below. > + /* > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and > + * update the clock recovery model. Then derive the wallclock estimate for the > + * frame timestamp. > + */ > + wallClockRecovery_->addSample(); > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000); As that could be called in the pipeline handler. And to me - then I would ask why are we getting the V4L2VideoDevice to manage the clockRecovery object.. (And I think that it should 'just' be in the V4L2VideoDevice in this instance if we're going to provide wall clock timestamps of buffers). > + } > + > if (V4L2_TYPE_IS_OUTPUT(buf.type)) > return buffer; > > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn() > return ret; > } > > + if (wallClockRecovery_) { > + /* A good moment to sample the clocks to improve the clock recovery model. */ > + wallClockRecovery_->addSample(); > + } > + > state_ = State::Streaming; > if (watchdogDuration_ && !queuedBuffers_.empty()) > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma > return {}; > } > > +/** > + * \brief Enable wall clock timestamps for this device > + * \param[in] wallClockRecovery an appropriately configured ClockRecovery, or > + * nullptr to disable wall clocks Is there a reason we can think of for /why/ we would disable the wall clocks? It doesn't seem like a 'desired' feature to me - unless the costs of doing the clock recovery are prohibitively expensive? Which I wouldn't expect it to be ? > + * > + * When buffers are dequeued, wall clock timestamps will be generated that > + * correspond to the frame's timestamp, as returned by V4l2. > + */ > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery) > +{ > + wallClockRecovery_ = wallClockRecovery; > + > + /* Also a reasonable moment to sample the two clocks. */ > + if (wallClockRecovery_) > + wallClockRecovery_->addSample(); > +} > + > /** > * \class V4L2M2MDevice > * \brief Memory-to-Memory video device > -- > 2.39.5 >
Hi Kieran Thanks for the review! On Fri, 6 Dec 2024 at 15:37, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting David Plowman (2024-12-06 14:27:40) > > FrameMetadata is extended to include a wallclock timestamp. > > > > When a frame is dequeued, we use the ClockRecovery class to generate a > > good estimate of a wallclock timestamp to correspond to the frame's > > SensorTimestamp. > > > > Pipeline handlers must enable wallclocks for chosen devices by passing > > an appropriate ClockRecovery object. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/framebuffer.h | 1 + > > include/libcamera/internal/v4l2_videodevice.h | 5 +++ > > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++- > > 3 files changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h > > index ff839243..4579d0c6 100644 > > --- a/include/libcamera/framebuffer.h > > +++ b/include/libcamera/framebuffer.h > > @@ -35,6 +35,7 @@ struct FrameMetadata { > > Status status; > > unsigned int sequence; > > uint64_t timestamp; > > + uint64_t wallClock; > > This breaks in the CI build I'm afraid: > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569 > > > [481/650] Generating Documentation/doxygen-internal with a custom command > FAILED: Documentation/internal-api-html > /usr/bin/doxygen Documentation/Doxyfile-internal > /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) > > Either we need to add the doc (which is what I'd do) - or potentially > depending on discussions below, remove the addition... Yep, well I think this is a bit above my pay grade. The "wall clock" sure feels like it belongs with the regular timestamp. If that's a problem, I'm happy to put it somewhere else, though I don't think I would know where currently. Perhaps some others can offer an opinion here? > > > > > Span<Plane> planes() { return planes_; } > > Span<const Plane> planes() const { return planes_; } > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index f021c2a0..5327c112 100644 > > --- a/include/libcamera/internal/v4l2_videodevice.h > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > @@ -37,6 +37,7 @@ > > > > namespace libcamera { > > > > +class ClockRecovery; > > class EventNotifier; > > class MediaDevice; > > class MediaEntity; > > @@ -232,6 +233,8 @@ public: > > > > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const; > > > > + void enableWallClock(ClockRecovery *wallClockRecovery); > > + > > protected: > > std::string logPrefix() const override; > > > > @@ -290,6 +293,8 @@ private: > > > > Timer watchdog_; > > utils::Duration watchdogDuration_; > > + > > + ClockRecovery *wallClockRecovery_; > > }; > > > > class V4L2M2MDevice > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index a5cf6784..2007dffc 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -26,6 +26,7 @@ > > #include <libcamera/base/unique_fd.h> > > #include <libcamera/base/utils.h> > > > > +#include "libcamera/internal/clock_recovery.h" > > #include "libcamera/internal/formats.h" > > #include "libcamera/internal/framebuffer.h" > > #include "libcamera/internal/media_device.h" > > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f) > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr), > > fdBufferNotifier_(nullptr), state_(State::Stopped), > > - watchdogDuration_(0.0) > > + watchdogDuration_(0.0), wallClockRecovery_(nullptr) > > { > > /* > > * We default to an MMAP based CAPTURE video device, however this will > > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL > > + buf.timestamp.tv_usec * 1000ULL; > > > > + metadata.wallClock = 0; > > + if (wallClockRecovery_) { > > I know Laurent discussed passing in the 'ClockRecovery' class as a way > of giving the responsibility to the Pipeline handler - but I'm not > particularly fond of it. (I won't object if this is preferred all > round). > > To me - especially if there is an addition of uint64_t wallClock in the > FrameMetadata, then this should just always be enabled. > > But if it's /really/ optional and passed in, - then I don't think we > should store a metadata.wallClock below. Indeed, I don't actually feel terribly strongly either, I'd obviously prefer to reach a consensus ahead of time rather than produce different implementations that we then don't like! I'd certainly be happy always to enable wall clocks, it depends whether we think the burden of a bit of floating point arithmetic every time we add a sample is a problem. Probably not. To be fair, I do wonder a little bit whether a single ClockRecovery for the entire system might be the right thing to do. Though I'm not sure where it would go, or how you would add sporadic samples to it. But you could easily rate-limit the model updates, if you wanted to. On the downside, that would certainly make it more difficult for anyone to configure it differently, but maybe no one would really need to? Questions, questions... > > > + /* > > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and > > + * update the clock recovery model. Then derive the wallclock estimate for the > > + * frame timestamp. > > + */ > > + wallClockRecovery_->addSample(); > > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000); > > As that could be called in the pipeline handler. And to me - then I > would ask why are we getting the V4L2VideoDevice to manage the > clockRecovery object.. > > (And I think that it should 'just' be in the V4L2VideoDevice in this > instance if we're going to provide wall clock timestamps of buffers). I would agree that I think I'm against moving more of it into the PH. After all, the whole point was to put it here so that everyone can get the new feature easily. But I don't super-love the pass-the-ClockRecovery-in either. I can certainly see that argument for a single "global" ClockRecovery. I dunno... > > > > + } > > + > > if (V4L2_TYPE_IS_OUTPUT(buf.type)) > > return buffer; > > > > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn() > > return ret; > > } > > > > + if (wallClockRecovery_) { > > + /* A good moment to sample the clocks to improve the clock recovery model. */ > > + wallClockRecovery_->addSample(); > > + } > > + > > state_ = State::Streaming; > > if (watchdogDuration_ && !queuedBuffers_.empty()) > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma > > return {}; > > } > > > > +/** > > + * \brief Enable wall clock timestamps for this device > > + * \param[in] wallClockRecovery an appropriately configured ClockRecovery, or > > + * nullptr to disable wall clocks > > Is there a reason we can think of for /why/ we would disable the wall > clocks? > > It doesn't seem like a 'desired' feature to me - unless the costs of > doing the clock recovery are prohibitively expensive? Which I wouldn't > expect it to be ? Yeah, no reason... just no compelling reason not to allow it either. Having them "always enabled" feels nice in many ways... Thanks! David > > > > > + * > > + * When buffers are dequeued, wall clock timestamps will be generated that > > + * correspond to the frame's timestamp, as returned by V4l2. > > + */ > > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery) > > +{ > > + wallClockRecovery_ = wallClockRecovery; > > + > > + /* Also a reasonable moment to sample the two clocks. */ > > + if (wallClockRecovery_) > > + wallClockRecovery_->addSample(); > > +} > > + > > /** > > * \class V4L2M2MDevice > > * \brief Memory-to-Memory video device > > -- > > 2.39.5 > >
Hi, On Fri, 6 Dec 2024 at 16:53, David Plowman <david.plowman@raspberrypi.com> wrote: > > Hi Kieran > > Thanks for the review! > > On Fri, 6 Dec 2024 at 15:37, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting David Plowman (2024-12-06 14:27:40) > > > FrameMetadata is extended to include a wallclock timestamp. > > > > > > When a frame is dequeued, we use the ClockRecovery class to generate a > > > good estimate of a wallclock timestamp to correspond to the frame's > > > SensorTimestamp. > > > > > > Pipeline handlers must enable wallclocks for chosen devices by passing > > > an appropriate ClockRecovery object. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/framebuffer.h | 1 + > > > include/libcamera/internal/v4l2_videodevice.h | 5 +++ > > > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++- > > > 3 files changed, 41 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h > > > index ff839243..4579d0c6 100644 > > > --- a/include/libcamera/framebuffer.h > > > +++ b/include/libcamera/framebuffer.h > > > @@ -35,6 +35,7 @@ struct FrameMetadata { > > > Status status; > > > unsigned int sequence; > > > uint64_t timestamp; > > > + uint64_t wallClock; > > > > This breaks in the CI build I'm afraid: > > > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569 > > > > > > [481/650] Generating Documentation/doxygen-internal with a custom command > > FAILED: Documentation/internal-api-html > > /usr/bin/doxygen Documentation/Doxyfile-internal > > /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) > > > > Either we need to add the doc (which is what I'd do) - or potentially > > depending on discussions below, remove the addition... > > Yep, well I think this is a bit above my pay grade. The "wall clock" > sure feels like it belongs with the regular timestamp. If that's a > problem, I'm happy to put it somewhere else, though I don't think I > would know where currently. Perhaps some others can offer an opinion > here? > > > > > > > > > Span<Plane> planes() { return planes_; } > > > Span<const Plane> planes() const { return planes_; } > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > index f021c2a0..5327c112 100644 > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > @@ -37,6 +37,7 @@ > > > > > > namespace libcamera { > > > > > > +class ClockRecovery; > > > class EventNotifier; > > > class MediaDevice; > > > class MediaEntity; > > > @@ -232,6 +233,8 @@ public: > > > > > > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const; > > > > > > + void enableWallClock(ClockRecovery *wallClockRecovery); > > > + > > > protected: > > > std::string logPrefix() const override; > > > > > > @@ -290,6 +293,8 @@ private: > > > > > > Timer watchdog_; > > > utils::Duration watchdogDuration_; > > > + > > > + ClockRecovery *wallClockRecovery_; > > > }; > > > > > > class V4L2M2MDevice > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index a5cf6784..2007dffc 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -26,6 +26,7 @@ > > > #include <libcamera/base/unique_fd.h> > > > #include <libcamera/base/utils.h> > > > > > > +#include "libcamera/internal/clock_recovery.h" > > > #include "libcamera/internal/formats.h" > > > #include "libcamera/internal/framebuffer.h" > > > #include "libcamera/internal/media_device.h" > > > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f) > > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr), > > > fdBufferNotifier_(nullptr), state_(State::Stopped), > > > - watchdogDuration_(0.0) > > > + watchdogDuration_(0.0), wallClockRecovery_(nullptr) > > > { > > > /* > > > * We default to an MMAP based CAPTURE video device, however this will > > > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL > > > + buf.timestamp.tv_usec * 1000ULL; > > > > > > + metadata.wallClock = 0; > > > + if (wallClockRecovery_) { > > > > I know Laurent discussed passing in the 'ClockRecovery' class as a way > > of giving the responsibility to the Pipeline handler - but I'm not > > particularly fond of it. (I won't object if this is preferred all > > round). > > > > To me - especially if there is an addition of uint64_t wallClock in the > > FrameMetadata, then this should just always be enabled. > > > > But if it's /really/ optional and passed in, - then I don't think we > > should store a metadata.wallClock below. > > Indeed, I don't actually feel terribly strongly either, I'd obviously > prefer to reach a consensus ahead of time rather than produce > different implementations that we then don't like! > > I'd certainly be happy always to enable wall clocks, it depends > whether we think the burden of a bit of floating point arithmetic > every time we add a sample is a problem. Probably not. > > To be fair, I do wonder a little bit whether a single ClockRecovery > for the entire system might be the right thing to do. Though I'm not > sure where it would go, or how you would add sporadic samples to it. > But you could easily rate-limit the model updates, if you wanted to. > > On the downside, that would certainly make it more difficult for > anyone to configure it differently, but maybe no one would really need > to? Questions, questions... > > > > > > + /* > > > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and > > > + * update the clock recovery model. Then derive the wallclock estimate for the > > > + * frame timestamp. > > > + */ > > > + wallClockRecovery_->addSample(); > > > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000); > > > > As that could be called in the pipeline handler. And to me - then I > > would ask why are we getting the V4L2VideoDevice to manage the > > clockRecovery object.. > > > > (And I think that it should 'just' be in the V4L2VideoDevice in this > > instance if we're going to provide wall clock timestamps of buffers). > > I would agree that I think I'm against moving more of it into the PH. > After all, the whole point was to put it here so that everyone can get > the new feature easily. But I don't super-love the > pass-the-ClockRecovery-in either. I can certainly see that argument > for a single "global" ClockRecovery. I dunno... I tend to agree with Kieran that perhaps the wall clock recovery becomes a standard feature of the V4L2VideoDevice and the framebuffer metadata.wallClock field will be unconditionally valid. The amount of additional computation per frame (per device) is quite tiny, and I doubt it would make any measurable impact. As such I would suggest we move the instantiation of the ClockRecovery object into V4L2VideoDevice. As for specifying custom clock recovery config, perhaps we just ignore this problem (if it is indeed a problem) until we have a definitive need to change it. Since this is all in libcamera internals, we have some leeway with changing this in the future. Any objections to this? Naush > > > > > > > > + } > > > + > > > if (V4L2_TYPE_IS_OUTPUT(buf.type)) > > > return buffer; > > > > > > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn() > > > return ret; > > > } > > > > > > + if (wallClockRecovery_) { > > > + /* A good moment to sample the clocks to improve the clock recovery model. */ > > > + wallClockRecovery_->addSample(); > > > + } > > > + > > > state_ = State::Streaming; > > > if (watchdogDuration_ && !queuedBuffers_.empty()) > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma > > > return {}; > > > } > > > > > > +/** > > > + * \brief Enable wall clock timestamps for this device > > > + * \param[in] wallClockRecovery an appropriately configured ClockRecovery, or > > > + * nullptr to disable wall clocks > > > > Is there a reason we can think of for /why/ we would disable the wall > > clocks? > > > > It doesn't seem like a 'desired' feature to me - unless the costs of > > doing the clock recovery are prohibitively expensive? Which I wouldn't > > expect it to be ? > > Yeah, no reason... just no compelling reason not to allow it either. > Having them "always enabled" feels nice in many ways... > > Thanks! > David > > > > > > > > > > + * > > > + * When buffers are dequeued, wall clock timestamps will be generated that > > > + * correspond to the frame's timestamp, as returned by V4l2. > > > + */ > > > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery) > > > +{ > > > + wallClockRecovery_ = wallClockRecovery; > > > + > > > + /* Also a reasonable moment to sample the two clocks. */ > > > + if (wallClockRecovery_) > > > + wallClockRecovery_->addSample(); > > > +} > > > + > > > /** > > > * \class V4L2M2MDevice > > > * \brief Memory-to-Memory video device > > > -- > > > 2.39.5 > > >
On Thu, Dec 12, 2024 at 01:04:11PM +0000, Naushir Patuck wrote: > On Fri, 6 Dec 2024 at 16:53, David Plowman wrote: > > On Fri, 6 Dec 2024 at 15:37, Kieran Bingham wrote: > > > Quoting David Plowman (2024-12-06 14:27:40) > > > > FrameMetadata is extended to include a wallclock timestamp. > > > > > > > > When a frame is dequeued, we use the ClockRecovery class to generate a > > > > good estimate of a wallclock timestamp to correspond to the frame's > > > > SensorTimestamp. > > > > > > > > Pipeline handlers must enable wallclocks for chosen devices by passing > > > > an appropriate ClockRecovery object. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > include/libcamera/framebuffer.h | 1 + > > > > include/libcamera/internal/v4l2_videodevice.h | 5 +++ > > > > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++- > > > > 3 files changed, 41 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h > > > > index ff839243..4579d0c6 100644 > > > > --- a/include/libcamera/framebuffer.h > > > > +++ b/include/libcamera/framebuffer.h > > > > @@ -35,6 +35,7 @@ struct FrameMetadata { > > > > Status status; > > > > unsigned int sequence; > > > > uint64_t timestamp; > > > > + uint64_t wallClock; > > > > > > This breaks in the CI build I'm afraid: > > > > > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569 > > > > > > > > > [481/650] Generating Documentation/doxygen-internal with a custom command > > > FAILED: Documentation/internal-api-html > > > /usr/bin/doxygen Documentation/Doxyfile-internal > > > /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) > > > > > > Either we need to add the doc (which is what I'd do) - or potentially > > > depending on discussions below, remove the addition... > > > > Yep, well I think this is a bit above my pay grade. The "wall clock" > > sure feels like it belongs with the regular timestamp. If that's a > > problem, I'm happy to put it somewhere else, though I don't think I > > would know where currently. Perhaps some others can offer an opinion > > here? Even if I'm still considering whether or not we should remove timestamp from FrameMetadata, I think it makes sense to have either none or both, so it's fine to keep it here. What's missing is documentation. > > > > > > > > Span<Plane> planes() { return planes_; } > > > > Span<const Plane> planes() const { return planes_; } > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > index f021c2a0..5327c112 100644 > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > @@ -37,6 +37,7 @@ > > > > > > > > namespace libcamera { > > > > > > > > +class ClockRecovery; > > > > class EventNotifier; > > > > class MediaDevice; > > > > class MediaEntity; > > > > @@ -232,6 +233,8 @@ public: > > > > > > > > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const; > > > > > > > > + void enableWallClock(ClockRecovery *wallClockRecovery); > > > > + > > > > protected: > > > > std::string logPrefix() const override; > > > > > > > > @@ -290,6 +293,8 @@ private: > > > > > > > > Timer watchdog_; > > > > utils::Duration watchdogDuration_; > > > > + > > > > + ClockRecovery *wallClockRecovery_; > > > > }; > > > > > > > > class V4L2M2MDevice > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > index a5cf6784..2007dffc 100644 > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > @@ -26,6 +26,7 @@ > > > > #include <libcamera/base/unique_fd.h> > > > > #include <libcamera/base/utils.h> > > > > > > > > +#include "libcamera/internal/clock_recovery.h" > > > > #include "libcamera/internal/formats.h" > > > > #include "libcamera/internal/framebuffer.h" > > > > #include "libcamera/internal/media_device.h" > > > > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f) > > > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > > > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr), > > > > fdBufferNotifier_(nullptr), state_(State::Stopped), > > > > - watchdogDuration_(0.0) > > > > + watchdogDuration_(0.0), wallClockRecovery_(nullptr) > > > > { > > > > /* > > > > * We default to an MMAP based CAPTURE video device, however this will > > > > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL > > > > + buf.timestamp.tv_usec * 1000ULL; > > > > > > > > + metadata.wallClock = 0; > > > > + if (wallClockRecovery_) { > > > > > > I know Laurent discussed passing in the 'ClockRecovery' class as a way > > > of giving the responsibility to the Pipeline handler - but I'm not > > > particularly fond of it. (I won't object if this is preferred all > > > round). > > > > > > To me - especially if there is an addition of uint64_t wallClock in the > > > FrameMetadata, then this should just always be enabled. > > > > > > But if it's /really/ optional and passed in, - then I don't think we > > > should store a metadata.wallClock below. > > > > Indeed, I don't actually feel terribly strongly either, I'd obviously > > prefer to reach a consensus ahead of time rather than produce > > different implementations that we then don't like! > > > > I'd certainly be happy always to enable wall clocks, it depends > > whether we think the burden of a bit of floating point arithmetic > > every time we add a sample is a problem. Probably not. > > > > To be fair, I do wonder a little bit whether a single ClockRecovery > > for the entire system might be the right thing to do. Though I'm not > > sure where it would go, or how you would add sporadic samples to it. > > But you could easily rate-limit the model updates, if you wanted to. > > > > On the downside, that would certainly make it more difficult for > > anyone to configure it differently, but maybe no one would really need > > to? Questions, questions... > > > > > > + /* > > > > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and > > > > + * update the clock recovery model. Then derive the wallclock estimate for the > > > > + * frame timestamp. > > > > + */ > > > > + wallClockRecovery_->addSample(); > > > > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000); > > > > > > As that could be called in the pipeline handler. And to me - then I > > > would ask why are we getting the V4L2VideoDevice to manage the > > > clockRecovery object.. > > > > > > (And I think that it should 'just' be in the V4L2VideoDevice in this > > > instance if we're going to provide wall clock timestamps of buffers). > > > > I would agree that I think I'm against moving more of it into the PH. > > After all, the whole point was to put it here so that everyone can get > > the new feature easily. But I don't super-love the > > pass-the-ClockRecovery-in either. I can certainly see that argument > > for a single "global" ClockRecovery. I dunno... > > I tend to agree with Kieran that perhaps the wall clock recovery > becomes a standard feature of the V4L2VideoDevice and the framebuffer > metadata.wallClock field will be unconditionally valid. The amount of > additional computation per frame (per device) is quite tiny, and I > doubt it would make any measurable impact. As such I would suggest we > move the instantiation of the ClockRecovery object into > V4L2VideoDevice. > > As for specifying custom clock recovery config, perhaps we just ignore > this problem (if it is indeed a problem) until we have a definitive > need to change it. Since this is all in libcamera internals, we have > some leeway with changing this in the future. > > Any objections to this? > > > > > + } > > > > + > > > > if (V4L2_TYPE_IS_OUTPUT(buf.type)) > > > > return buffer; > > > > > > > > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn() > > > > return ret; > > > > } > > > > > > > > + if (wallClockRecovery_) { > > > > + /* A good moment to sample the clocks to improve the clock recovery model. */ > > > > + wallClockRecovery_->addSample(); > > > > + } > > > > + > > > > state_ = State::Streaming; > > > > if (watchdogDuration_ && !queuedBuffers_.empty()) > > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma > > > > return {}; > > > > } > > > > > > > > +/** > > > > + * \brief Enable wall clock timestamps for this device > > > > + * \param[in] wallClockRecovery an appropriately configured ClockRecovery, or > > > > + * nullptr to disable wall clocks > > > > > > Is there a reason we can think of for /why/ we would disable the wall > > > clocks? > > > > > > It doesn't seem like a 'desired' feature to me - unless the costs of > > > doing the clock recovery are prohibitively expensive? Which I wouldn't > > > expect it to be ? > > > > Yeah, no reason... just no compelling reason not to allow it either. > > Having them "always enabled" feels nice in many ways... Isn't it just a waste of CPU cycles on the video devices that correspond to the input and output of the ISP, or to the embedded data capture ? That's why I don't think this belongs to the V4L2VideoDevice class. The need is to timestamp requests, not frame buffers individually. I'd rather try to make this simple to wire up in pipeline handlers. > > > > + * > > > > + * When buffers are dequeued, wall clock timestamps will be generated that > > > > + * correspond to the frame's timestamp, as returned by V4l2. > > > > + */ > > > > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery) > > > > +{ > > > > + wallClockRecovery_ = wallClockRecovery; > > > > + > > > > + /* Also a reasonable moment to sample the two clocks. */ > > > > + if (wallClockRecovery_) > > > > + wallClockRecovery_->addSample(); > > > > +} > > > > + > > > > /** > > > > * \class V4L2M2MDevice > > > > * \brief Memory-to-Memory video device
Hi Laurent Thanks for the comments! On Wed, 18 Dec 2024 at 02:35, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Dec 12, 2024 at 01:04:11PM +0000, Naushir Patuck wrote: > > On Fri, 6 Dec 2024 at 16:53, David Plowman wrote: > > > On Fri, 6 Dec 2024 at 15:37, Kieran Bingham wrote: > > > > Quoting David Plowman (2024-12-06 14:27:40) > > > > > FrameMetadata is extended to include a wallclock timestamp. > > > > > > > > > > When a frame is dequeued, we use the ClockRecovery class to generate a > > > > > good estimate of a wallclock timestamp to correspond to the frame's > > > > > SensorTimestamp. > > > > > > > > > > Pipeline handlers must enable wallclocks for chosen devices by passing > > > > > an appropriate ClockRecovery object. > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > include/libcamera/framebuffer.h | 1 + > > > > > include/libcamera/internal/v4l2_videodevice.h | 5 +++ > > > > > src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++- > > > > > 3 files changed, 41 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h > > > > > index ff839243..4579d0c6 100644 > > > > > --- a/include/libcamera/framebuffer.h > > > > > +++ b/include/libcamera/framebuffer.h > > > > > @@ -35,6 +35,7 @@ struct FrameMetadata { > > > > > Status status; > > > > > unsigned int sequence; > > > > > uint64_t timestamp; > > > > > + uint64_t wallClock; > > > > > > > > This breaks in the CI build I'm afraid: > > > > > > > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67848569 > > > > > > > > > > > > [481/650] Generating Documentation/doxygen-internal with a custom command > > > > FAILED: Documentation/internal-api-html > > > > /usr/bin/doxygen Documentation/Doxyfile-internal > > > > /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) > > > > > > > > Either we need to add the doc (which is what I'd do) - or potentially > > > > depending on discussions below, remove the addition... > > > > > > Yep, well I think this is a bit above my pay grade. The "wall clock" > > > sure feels like it belongs with the regular timestamp. If that's a > > > problem, I'm happy to put it somewhere else, though I don't think I > > > would know where currently. Perhaps some others can offer an opinion > > > here? > > Even if I'm still considering whether or not we should remove timestamp > from FrameMetadata, I think it makes sense to have either none or both, > so it's fine to keep it here. What's missing is documentation. > > > > > > > > > > > Span<Plane> planes() { return planes_; } > > > > > Span<const Plane> planes() const { return planes_; } > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > > index f021c2a0..5327c112 100644 > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > > @@ -37,6 +37,7 @@ > > > > > > > > > > namespace libcamera { > > > > > > > > > > +class ClockRecovery; > > > > > class EventNotifier; > > > > > class MediaDevice; > > > > > class MediaEntity; > > > > > @@ -232,6 +233,8 @@ public: > > > > > > > > > > V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const; > > > > > > > > > > + void enableWallClock(ClockRecovery *wallClockRecovery); > > > > > + > > > > > protected: > > > > > std::string logPrefix() const override; > > > > > > > > > > @@ -290,6 +293,8 @@ private: > > > > > > > > > > Timer watchdog_; > > > > > utils::Duration watchdogDuration_; > > > > > + > > > > > + ClockRecovery *wallClockRecovery_; > > > > > }; > > > > > > > > > > class V4L2M2MDevice > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > > index a5cf6784..2007dffc 100644 > > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > > @@ -26,6 +26,7 @@ > > > > > #include <libcamera/base/unique_fd.h> > > > > > #include <libcamera/base/utils.h> > > > > > > > > > > +#include "libcamera/internal/clock_recovery.h" > > > > > #include "libcamera/internal/formats.h" > > > > > #include "libcamera/internal/framebuffer.h" > > > > > #include "libcamera/internal/media_device.h" > > > > > @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f) > > > > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > > > > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr), > > > > > fdBufferNotifier_(nullptr), state_(State::Stopped), > > > > > - watchdogDuration_(0.0) > > > > > + watchdogDuration_(0.0), wallClockRecovery_(nullptr) > > > > > { > > > > > /* > > > > > * We default to an MMAP based CAPTURE video device, however this will > > > > > @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > > > metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL > > > > > + buf.timestamp.tv_usec * 1000ULL; > > > > > > > > > > + metadata.wallClock = 0; > > > > > + if (wallClockRecovery_) { > > > > > > > > I know Laurent discussed passing in the 'ClockRecovery' class as a way > > > > of giving the responsibility to the Pipeline handler - but I'm not > > > > particularly fond of it. (I won't object if this is preferred all > > > > round). > > > > > > > > To me - especially if there is an addition of uint64_t wallClock in the > > > > FrameMetadata, then this should just always be enabled. > > > > > > > > But if it's /really/ optional and passed in, - then I don't think we > > > > should store a metadata.wallClock below. > > > > > > Indeed, I don't actually feel terribly strongly either, I'd obviously > > > prefer to reach a consensus ahead of time rather than produce > > > different implementations that we then don't like! > > > > > > I'd certainly be happy always to enable wall clocks, it depends > > > whether we think the burden of a bit of floating point arithmetic > > > every time we add a sample is a problem. Probably not. > > > > > > To be fair, I do wonder a little bit whether a single ClockRecovery > > > for the entire system might be the right thing to do. Though I'm not > > > sure where it would go, or how you would add sporadic samples to it. > > > But you could easily rate-limit the model updates, if you wanted to. > > > > > > On the downside, that would certainly make it more difficult for > > > anyone to configure it differently, but maybe no one would really need > > > to? Questions, questions... > > > > > > > > + /* > > > > > + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and > > > > > + * update the clock recovery model. Then derive the wallclock estimate for the > > > > > + * frame timestamp. > > > > > + */ > > > > > + wallClockRecovery_->addSample(); > > > > > + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000); > > > > > > > > As that could be called in the pipeline handler. And to me - then I > > > > would ask why are we getting the V4L2VideoDevice to manage the > > > > clockRecovery object.. > > > > > > > > (And I think that it should 'just' be in the V4L2VideoDevice in this > > > > instance if we're going to provide wall clock timestamps of buffers). > > > > > > I would agree that I think I'm against moving more of it into the PH. > > > After all, the whole point was to put it here so that everyone can get > > > the new feature easily. But I don't super-love the > > > pass-the-ClockRecovery-in either. I can certainly see that argument > > > for a single "global" ClockRecovery. I dunno... > > > > I tend to agree with Kieran that perhaps the wall clock recovery > > becomes a standard feature of the V4L2VideoDevice and the framebuffer > > metadata.wallClock field will be unconditionally valid. The amount of > > additional computation per frame (per device) is quite tiny, and I > > doubt it would make any measurable impact. As such I would suggest we > > move the instantiation of the ClockRecovery object into > > V4L2VideoDevice. > > > > As for specifying custom clock recovery config, perhaps we just ignore > > this problem (if it is indeed a problem) until we have a definitive > > need to change it. Since this is all in libcamera internals, we have > > some leeway with changing this in the future. > > > > Any objections to this? > > > > > > > + } > > > > > + > > > > > if (V4L2_TYPE_IS_OUTPUT(buf.type)) > > > > > return buffer; > > > > > > > > > > @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn() > > > > > return ret; > > > > > } > > > > > > > > > > + if (wallClockRecovery_) { > > > > > + /* A good moment to sample the clocks to improve the clock recovery model. */ > > > > > + wallClockRecovery_->addSample(); > > > > > + } > > > > > + > > > > > state_ = State::Streaming; > > > > > if (watchdogDuration_ && !queuedBuffers_.empty()) > > > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > > > @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma > > > > > return {}; > > > > > } > > > > > > > > > > +/** > > > > > + * \brief Enable wall clock timestamps for this device > > > > > + * \param[in] wallClockRecovery an appropriately configured ClockRecovery, or > > > > > + * nullptr to disable wall clocks > > > > > > > > Is there a reason we can think of for /why/ we would disable the wall > > > > clocks? > > > > > > > > It doesn't seem like a 'desired' feature to me - unless the costs of > > > > doing the clock recovery are prohibitively expensive? Which I wouldn't > > > > expect it to be ? > > > > > > Yeah, no reason... just no compelling reason not to allow it either. > > > Having them "always enabled" feels nice in many ways... > > Isn't it just a waste of CPU cycles on the video devices that correspond > to the input and output of the ISP, or to the embedded data capture ? > That's why I don't think this belongs to the V4L2VideoDevice class. The > need is to timestamp requests, not frame buffers individually. I'd > rather try to make this simple to wire up in pipeline handlers. Just to be sure I've understood here, what you're saying is that you'd rather have this handled completely in the pipeline handlers, and have nothing at all in the V4L2VideoDevice? I'm fine with that, just wanted to clear. At that point, it doesn't feel like there's any need to have it in the metadata either, and it can just be a control. Does that sound fair? Thanks! David > > > > > > + * > > > > > + * When buffers are dequeued, wall clock timestamps will be generated that > > > > > + * correspond to the frame's timestamp, as returned by V4l2. > > > > > + */ > > > > > +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery) > > > > > +{ > > > > > + wallClockRecovery_ = wallClockRecovery; > > > > > + > > > > > + /* Also a reasonable moment to sample the two clocks. */ > > > > > + if (wallClockRecovery_) > > > > > + wallClockRecovery_->addSample(); > > > > > +} > > > > > + > > > > > /** > > > > > * \class V4L2M2MDevice > > > > > * \brief Memory-to-Memory video device > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h index ff839243..4579d0c6 100644 --- a/include/libcamera/framebuffer.h +++ b/include/libcamera/framebuffer.h @@ -35,6 +35,7 @@ struct FrameMetadata { Status status; unsigned int sequence; uint64_t timestamp; + uint64_t wallClock; Span<Plane> planes() { return planes_; } Span<const Plane> planes() const { return planes_; } diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h index f021c2a0..5327c112 100644 --- a/include/libcamera/internal/v4l2_videodevice.h +++ b/include/libcamera/internal/v4l2_videodevice.h @@ -37,6 +37,7 @@ namespace libcamera { +class ClockRecovery; class EventNotifier; class MediaDevice; class MediaEntity; @@ -232,6 +233,8 @@ public: V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const; + void enableWallClock(ClockRecovery *wallClockRecovery); + protected: std::string logPrefix() const override; @@ -290,6 +293,8 @@ private: Timer watchdog_; utils::Duration watchdogDuration_; + + ClockRecovery *wallClockRecovery_; }; class V4L2M2MDevice diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index a5cf6784..2007dffc 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -26,6 +26,7 @@ #include <libcamera/base/unique_fd.h> #include <libcamera/base/utils.h> +#include "libcamera/internal/clock_recovery.h" #include "libcamera/internal/formats.h" #include "libcamera/internal/framebuffer.h" #include "libcamera/internal/media_device.h" @@ -534,7 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f) V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr), fdBufferNotifier_(nullptr), state_(State::Stopped), - watchdogDuration_(0.0) + watchdogDuration_(0.0), wallClockRecovery_(nullptr) { /* * We default to an MMAP based CAPTURE video device, however this will @@ -1865,6 +1866,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL + buf.timestamp.tv_usec * 1000ULL; + metadata.wallClock = 0; + if (wallClockRecovery_) { + /* + * Sample the internal (CLOCK_BOOTTIME) and realtime (CLOCK_REALTIME) clocks and + * update the clock recovery model. Then derive the wallclock estimate for the + * frame timestamp. + */ + wallClockRecovery_->addSample(); + metadata.wallClock = wallClockRecovery_->getOutput(metadata.timestamp / 1000); + } + if (V4L2_TYPE_IS_OUTPUT(buf.type)) return buffer; @@ -1965,6 +1977,11 @@ int V4L2VideoDevice::streamOn() return ret; } + if (wallClockRecovery_) { + /* A good moment to sample the clocks to improve the clock recovery model. */ + wallClockRecovery_->addSample(); + } + state_ = State::Streaming; if (watchdogDuration_ && !queuedBuffers_.empty()) watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); @@ -2120,6 +2137,23 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma return {}; } +/** + * \brief Enable wall clock timestamps for this device + * \param[in] wallClockRecovery an appropriately configured ClockRecovery, or + * nullptr to disable wall clocks + * + * When buffers are dequeued, wall clock timestamps will be generated that + * correspond to the frame's timestamp, as returned by V4l2. + */ +void V4L2VideoDevice::enableWallClock(ClockRecovery *wallClockRecovery) +{ + wallClockRecovery_ = wallClockRecovery; + + /* Also a reasonable moment to sample the two clocks. */ + if (wallClockRecovery_) + wallClockRecovery_->addSample(); +} + /** * \class V4L2M2MDevice * \brief Memory-to-Memory video device
FrameMetadata is extended to include a wallclock timestamp. When a frame is dequeued, we use the ClockRecovery class to generate a good estimate of a wallclock timestamp to correspond to the frame's SensorTimestamp. Pipeline handlers must enable wallclocks for chosen devices by passing an appropriate ClockRecovery object. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- include/libcamera/framebuffer.h | 1 + include/libcamera/internal/v4l2_videodevice.h | 5 +++ src/libcamera/v4l2_videodevice.cpp | 36 ++++++++++++++++++- 3 files changed, 41 insertions(+), 1 deletion(-)