[PATCH 3/5] libcamera: v4l2: Add wallclock metadata to video devices
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 18 03:35:47 CET 2024
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 at 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list