[PATCH weston 1/7] timespec: Add timespec_add_nsec helper

[Pekka Paalanen]

On Tue, 14 Feb 2017 14:21:17 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 14 February 2017 at 14:09, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Tue, 14 Feb 2017 13:18:01 +0000
> > Daniel Stone <daniels at collabora.com> wrote:  
> >> Add a (timespec) = (timespec) + (nsec) helper, to save intermediate
> >> conversions to nanoseconds in its users.  

> >> +/* Add a nanosecond value to a timespec
> >> + *
> >> + * \param r[out] result: a + b
> >> + * \param a[in] base operand as timespec
> >> + * \param b[in] operand in nanoseconds
> >> + */
> >> +static inline void
> >> +timespec_add_nsec(struct timespec *r, const struct timespec *a, int64_t b)
> >> +{
> >> +     r->tv_sec = a->tv_sec + (b / NSEC_PER_SEC);
> >> +     r->tv_nsec = a->tv_nsec + (b % NSEC_PER_SEC);
> >> +
> >> +     if (r->tv_nsec >= NSEC_PER_SEC) {
> >> +             r->tv_sec++;
> >> +             r->tv_nsec -= NSEC_PER_SEC;
> >> +     } else if (r->tv_nsec <= -NSEC_PER_SEC) {  
> >
> > IIRC tv_nsec cannot be negative, so it should be:
> >         r->tv_nsec < 0  
> 
> This is working on r->tv_nsec, i.e. (a->tv_nsec + signed input value).
> I did this to deliberately allow timespec_add_nsec(&r, &a, -12345).

Indeed r->tv_nsec, and you need to guarantee to not return a timespec
with an invalid tv_nsec. It has no relation to allowing negative b
which is fine.

> > I recall writing asserts for tv_nsec, but maybe it wasn't weston where
> > I did that.
> >
> > I realize mandating 0 <= nsec < NSEC_PER_SEC to be somewhat arbitrary,
> > but timespec_sub() already assumes that and it is what
> > presentation-time protocol requires.
> >
> > In wesgr where I needed the notion of invalid timestamps, I abused the
> > value tv_nsec = -1 for it.  
> 
> That's all perfectly reasonable. I thought about adding some
> assertions that timespec was in fact normalised, but decided I'd
> already wandered far enough off topic tbh.

Yup, totally fine.

> >> +     a.tv_sec = 0;
> >> +     a.tv_nsec = 1;
> >> +     timespec_add_nsec(&r, &a, -2);
> >> +     ZUC_ASSERT_EQ(r.tv_sec, 0);
> >> +     ZUC_ASSERT_EQ(r.tv_nsec, -1);  
> >
> > Incorrect, tv_nsec cannot be negative.  
> 
> So this would have to be { .tv_sec = -1, .tv_nsec = NSEC_PER_SEC - 1
> }? I was somewhat confused as to the relationship of the two.

Yes, nsec is always [0, 999999999] in normalized form.

I was also confused about how to interpret a timespec when I started
using it, but in the end, it is just tv_sec + tv_nsec / 1e9. I think
comparison functions might become annoying if tv_nsec was allowed to be
negative. Sticking to the normalized form makes things easier, IMO. At
least it pays off to stick to just one normalized form (see
presentation-time). :-)

The real confusion comes if one tries to think of it as integer
concatenated with fractional part ("%d.%09d"). It works if tv_sec and
tv_nsec are both positive (and normalized), but fails immediately when
either becomes negative.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170214/1c7cec01/attachment.sig>