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

[Kieran Bingham]

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 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 
> :-)

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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran