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

[Daniel Stone]

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.
>
> I recall some comments about using timespec over int64_t nanoseconds
> being over-engineering, what do you think? Should we rather just
> convert everything to int64_t nsec from the start and only deal with
> them?
>
> Personally I'm favour of timespec, even though int64_t nsec range is
> +/- 290 years or so. :-)

Honestly, I have no opinion whatsoever. I was just trying to stick to
what was already in the repaint calculations, which seemed to be input
timestamps and intermediate calculations using timespecs, normalised
to a millisecond value at the end.

>> @@ -1307,6 +1308,15 @@ config_parser_test_CFLAGS =                    \
>>       $(AM_CFLAGS)                            \
>>       -I$(top_srcdir)/tools/zunitc/inc
>>
>> +timespec_test_SOURCES = tests/timespec-test.c
>> +timespec_test_LDADD =        \
>> +     libshared.la            \
>> +     libzunitc.la            \
>> +     libzunitcmain.la
>> +timespec_test_CFLAGS =                       \
>> +     $(AM_CFLAGS)                            \
>> +     -I$(top_srcdir)/tools/zunitc/inc
>> +
>
> Ooh, a zuc test!

Yeah, I thought the test suite might be enhanced by having something
to test other than itself.

>> +/* 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).

> 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.

>> +#define NSEC_PER_SEC 1000000000
>
> NSEC_PER_SEC should come from timespec-util.h.

Sure, will fix.

>> +ZUC_TEST(timespec_test, timespec_add_nsec)
>> +{
>> +     struct timespec a, r;
>
> An array and a loop to ease readability? :-)

Fair enough.

>> +     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.

>> +     a.tv_nsec = -1;
>
> Invalid timestamp.

I can bin the test.

Cheers,
Daniel