[libcamera-devel] [PATCH] libcamera: buffer: Improve timestamp documentation

[Niklas Söderlund]

Hello,

On 2020-08-28 17:59:41 +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> > The Buffer timestamp documentation is short and does not explain that
> > the timestamp is based on a monotonic time source, as opposed to the
> > real/wall-time clocks available in the system.
> > 
> > Expand upon the statements to detail that it can not be used directly as
> > a wall-clock for the captured date/time without a further reference
> > point, and that by taking a reference point - the user will be bringing
> > in potential inconsistencies which must be considered.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/libcamera/buffer.cpp | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> > index a094737d0392..c08ca4234b29 100644
> > --- a/src/libcamera/buffer.cpp
> > +++ b/src/libcamera/buffer.cpp
> > @@ -92,7 +92,13 @@ LOG_DEFINE_CATEGORY(Buffer)
> >   * The timestamp is expressed as a number of nanoseconds relative to the system
> >   * clock since an unspecified time point.
> >   *
> > - * \todo Be more precise on what timestamps refer to.
> > + * This timestamp is monotonic and not affected by changes in system time,
> > + * however it is still susceptible to small adjustments made by NTP.
> > + *
> > + * This clock has no specification on its time base, so can not be directly be
> > + * converted to a 'real' wall clock time without a comparable reference point.
> > + * However, continued conversions from the reference point, may bring in
> > + * inaccuracies due to changes on the real time clock which must be considered.
> 
> I'd keep a todo here, as we still haven't decided what clock source is
> the most suitable. Describing what is being done today is of course a
> good idea, but one issue is that we get the timestamp directly from
> V4L2, and V4L2 doesn't guarantee what timestamp source is used. I've had
> a look at Linus' master branch, and we have, in drivers/media/,
> 
>     103 ktime_get
>       6 ktime_get_boottime
>       6 ktime_get_real
> 
> ktime_get_boottime is only used in DVB, so we're fine there.
> ktime_get_real is used by
> 
> drivers/media/pci/ttpci/av7110.c:       ktime_get_real_ts64(&ts);
> drivers/media/rc/streamzap.c:           sz->signal_start = ktime_get_real();
> drivers/media/rc/streamzap.c:   sz->signal_start = ktime_get_real();
> drivers/media/test-drivers/vivid/vivid-rds-gen.c:                       time64_to_tm(ktime_get_real_seconds(), 0, &tm);
> drivers/media/test-drivers/vivid/vivid-vbi-gen.c:       time64_to_tm(ktime_get_real_seconds(), 0, &tm);
> drivers/media/usb/uvc/uvc_video.c:              return ktime_get_real();
> 
> UVC supports selecting either the monotonic or real clock, with the
> monotonic clock being the default, so I think we're fine too.
> 
> We can thus assume for the time being that we only get CLOCK_MONOTONIC
> timestamp. Should we simplify the doc by just telling we use
> CLOCK_MONOTONIC, with a todo comment to revisit the topic and possibly
> select a different clock, or make the clock source configurable ?

I think we should always use the CLOCK_MONOTONIC for buffer/request 
timestamps. For other clocks we might have clock drift or adjustment 
(ntp) messing with the values creating subtle bugs. For applications 
wanting to have a stamp to write to file I think they need to create 
that themself, taking all the localization and timezones into account 
:-)

> 
> >   */
> >  
> >  /**
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund