| Message ID | 20200828115507.305323-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On 8/28/20 5:25 PM, Kieran Bingham wrote: > 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@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. > */ > > /** Well, I just read clock_gettime manpage a while ago, so that context helps me inferring this documentation more clearly. ;-) Would it help pointing to that manpage as a "See also" for reference? Anyways, Reviewed-by: Umang Jain <email@uajain.com>
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@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 ? > */ > > /**
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On 30/08/2020 09:11, Niklas Söderlund wrote: > 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@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 > :-) I think perhaps we should be setting the clock explicitly if there are options (which does leave the possibility of it being configurable, but only if all underlying kernel layers support that, not 'just uvc'. ). I think it's important that all pipeline handlers treat the time stamps in the same consistent way though. Niklas, I do mostly agree with you on the fact that the timestamps need to be consistent (and monotonic) for performance measurements, but how will we handle the need for encoding the captured time stamp when it comes to the reprocessing API? Will there be any differences there? Or perhaps it's not a problem, as the layer injecting the frames is also the layer doing the encoding - so the timestamps are just simply a passthrough at that point. -- Kieran > >> >>> */ >>> >>> /** >> >> -- >> Regards, >> >> Laurent Pinchart >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
On Mon, Aug 31, 2020 at 10:41:12AM +0100, Kieran Bingham wrote: > On 30/08/2020 09:11, Niklas Söderlund wrote: > > On 2020-08-28 17:59:41 +0300, Laurent Pinchart wrote: > >>> 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@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. CLOCK_MONOTONIC is subject to NTP adjustments. You want CLOCK_MONOTONIC_RAW to avoid that. > > 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 > > :-) > > I think perhaps we should be setting the clock explicitly if there are > options (which does leave the possibility of it being configurable, but > only if all underlying kernel layers support that, not 'just uvc'. ). > > I think it's important that all pipeline handlers treat the time stamps > in the same consistent way though. I believe we'll have to support multiple timestamp sources. Timestamps are mostly useful to synchronize events in the system, and to do so, clocks need to match. We don't control what clock sources other subsystems use. CLOCK_BOOTTIME is another common alternative to CLOCK_MONOTONIC. It will likely be best to just allow the user to select a timestamp source (we'll need a corresponding kernel API). > Niklas, I do mostly agree with you on the fact that the timestamps need > to be consistent (and monotonic) for performance measurements, but how > will we handle the need for encoding the captured time stamp when it > comes to the reprocessing API? > > Will there be any differences there? Or perhaps it's not a problem, as > the layer injecting the frames is also the layer doing the encoding - so > the timestamps are just simply a passthrough at that point. Yes, timestamps should be passed-through I think. > >>> */ > >>> > >>> /**
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. */ /**
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@ideasonboard.com> --- src/libcamera/buffer.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)