On Mon, 4 Dec 2017 15:34:02 +0200 Alexandros Frantzis <[email protected]> wrote:
> Add helpers to safely convert between struct timespec values and > tv_sec_hi, tv_sec_lo, tv_nsec triplets used for sending high-resolution > timestamp data over the wayland protocol. Replace existing conversion > code with the helper functions. > > Signed-off-by: Alexandros Frantzis <[email protected]> > --- > clients/presentation-shm.c | 9 +------ > libweston/compositor.c | 9 ++++--- > shared/timespec-util.h | 39 +++++++++++++++++++++++++++ > tests/presentation-test.c | 9 +------ > tests/timespec-test.c | 66 > ++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 112 insertions(+), 20 deletions(-) Hi, I have questions below that will have implications to the protocol extension spec as well. I would like to require the time values in protocol to be normalized. > > diff --git a/clients/presentation-shm.c b/clients/presentation-shm.c > index c9fb66cc..d6a939e5 100644 > --- a/clients/presentation-shm.c > +++ b/clients/presentation-shm.c > @@ -39,6 +39,7 @@ > #include <wayland-client.h> > #include "shared/helpers.h" > #include "shared/zalloc.h" > +#include "shared/timespec-util.h" > #include "shared/os-compatibility.h" > #include "presentation-time-client-protocol.h" > > @@ -383,14 +384,6 @@ timespec_to_ms(const struct timespec *ts) > return (uint32_t)ts->tv_sec * 1000 + ts->tv_nsec / 1000000; > } > > -static void > -timespec_from_proto(struct timespec *tm, uint32_t tv_sec_hi, > - uint32_t tv_sec_lo, uint32_t tv_nsec) > -{ > - tm->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo; > - tm->tv_nsec = tv_nsec; > -} > - > static int > timespec_diff_to_usec(const struct timespec *a, const struct timespec *b) > { > diff --git a/libweston/compositor.c b/libweston/compositor.c > index 7d7a17ed..083664fd 100644 > --- a/libweston/compositor.c > +++ b/libweston/compositor.c > @@ -341,7 +341,9 @@ weston_presentation_feedback_present( > { > struct wl_client *client = wl_resource_get_client(feedback->resource); > struct wl_resource *o; > - uint64_t secs; > + uint32_t tv_sec_hi; > + uint32_t tv_sec_lo; > + uint32_t tv_nsec; A suggestion: how about introducing struct timespec_proto { uint32_t sec_hi; uint32_t sec_lo; uint32_t nsec; }; and using that in timespec_to_proto()? (Not useful for timespec_from_proto() because the three variables are already declared.) > > wl_resource_for_each(o, &output->resource_list) { > if (wl_resource_get_client(o) != client) > @@ -350,10 +352,9 @@ weston_presentation_feedback_present( > wp_presentation_feedback_send_sync_output(feedback->resource, > o); > } > > - secs = ts->tv_sec; > + timespec_to_proto(ts, &tv_sec_hi, &tv_sec_lo, &tv_nsec); > wp_presentation_feedback_send_presented(feedback->resource, > - secs >> 32, secs & 0xffffffff, > - ts->tv_nsec, > + tv_sec_hi, tv_sec_lo, tv_nsec, > refresh_nsec, > seq >> 32, seq & 0xffffffff, > flags | feedback->psf_flags); > diff --git a/shared/timespec-util.h b/shared/timespec-util.h > index a10edf5b..c734accd 100644 > --- a/shared/timespec-util.h > +++ b/shared/timespec-util.h > @@ -175,6 +175,30 @@ timespec_to_usec(const struct timespec *a) > return (int64_t)a->tv_sec * 1000000 + a->tv_nsec / 1000; > } > > +/* Convert timespec to protocol data > + * > + * \param a timespec > + * \param tv_sec_hi[out] the high bytes of the seconds part > + * \param tv_sec_lo[out] the low bytes of the seconds part > + * \param tv_nsec[out] the nanoseconds part > + * > + * The timespec is normalized before being converted to protocol data. > + */ > +static inline void > +timespec_to_proto(const struct timespec *a, uint32_t *tv_sec_hi, > + uint32_t *tv_sec_lo, uint32_t *tv_nsec) > +{ > + struct timespec r; > + > + timespec_normalize(&r, a); > + > + /* We check the size of tv_sec, so that we shift only if the size > + * is 64-bits, in order to avoid sign extension on 32-bit systems. */ > + *tv_sec_hi = sizeof(r.tv_sec) == 8 ? (int64_t)r.tv_sec >> 32 : 0; How about just casting tv_sec to uint64_t before shifting and masking it to 32-bit pieces? I think that would be more obvious to read. I think it would be better to assert() that the timespec is already: - normalized, because otherwise nsec might overflow in a computation, and - non-negative, because the protocol cannot carry negative timestamps. > + *tv_sec_lo = r.tv_sec; > + *tv_nsec = r.tv_nsec; > +} > + > /* Convert nanoseconds to timespec > * > * \param a timespec > @@ -209,6 +233,21 @@ timespec_from_msec(struct timespec *a, int64_t b) > timespec_from_nsec(a, b * 1000000); > } > > +/* Convert protocol data to timespec > + * > + * \param a[out] timespec > + * \param tv_sec_hi the high bytes of seconds part > + * \param tv_sec_lo the low bytes of seconds part > + * \param tv_nsec the nanoseconds part > + */ > +static inline void > +timespec_from_proto(struct timespec *a, uint32_t tv_sec_hi, > + uint32_t tv_sec_lo, uint32_t tv_nsec) > +{ > + a->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo; How to handle overflows in tv_sec would be a good question... > + a->tv_nsec = tv_nsec; > +} Btw. refactoring code to introduce timespec_from_proto() with no functional changes should probably be a patch of its own. Thanks, pq > + > /* Check if a timespec is zero > * > * \param a timespec > diff --git a/tests/presentation-test.c b/tests/presentation-test.c > index f12f8eef..f6ffe480 100644 > --- a/tests/presentation-test.c > +++ b/tests/presentation-test.c > @@ -34,6 +34,7 @@ > > #include "shared/helpers.h" > #include "shared/xalloc.h" > +#include "shared/timespec-util.h" > #include "weston-test-client-helper.h" > #include "presentation-time-client-protocol.h" > > @@ -85,14 +86,6 @@ struct feedback { > uint32_t flags; > }; > > -static void > -timespec_from_proto(struct timespec *tm, uint32_t tv_sec_hi, > - uint32_t tv_sec_lo, uint32_t tv_nsec) > -{ > - tm->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo; > - tm->tv_nsec = tv_nsec; > -} > - > static void > feedback_sync_output(void *data, > struct wp_presentation_feedback *presentation_feedback, > diff --git a/tests/timespec-test.c b/tests/timespec-test.c > index 8c2296d1..9000295a 100644 > --- a/tests/timespec-test.c > +++ b/tests/timespec-test.c > @@ -38,6 +38,12 @@ > #include "shared/helpers.h" > #include "zunitc/zunitc.h" > > +static uint32_t > +get_tv_sec_hi(const struct timespec *ts) > +{ > + return sizeof(ts->tv_sec) == 8 ? (int64_t)ts->tv_sec >> 32 : 0; > +} > + > ZUC_TEST(timespec_test, timespec_normalize) > { > struct timespec a, r; > @@ -143,6 +149,49 @@ ZUC_TEST(timespec_test, timespec_to_msec) > ZUC_ASSERT_EQ(timespec_to_msec(&a), (4000ULL) + 4); > } > > +ZUC_TEST(timespec_test, timespec_to_proto) > +{ > + struct timespec a; > + uint32_t tv_sec_hi; > + uint32_t tv_sec_lo; > + uint32_t tv_nsec; > + > + a.tv_sec = 0; > + a.tv_nsec = 0; > + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec); > + ZUC_ASSERT_EQ(0, tv_sec_hi); > + ZUC_ASSERT_EQ(0, tv_sec_lo); > + ZUC_ASSERT_EQ(0, tv_nsec); > + > + a.tv_sec = 1234; > + a.tv_nsec = NSEC_PER_SEC - 1; > + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec); > + ZUC_ASSERT_EQ(0, tv_sec_hi); > + ZUC_ASSERT_EQ(1234, tv_sec_lo); > + ZUC_ASSERT_EQ(NSEC_PER_SEC - 1, tv_nsec); > + > + a.tv_sec = 1234; > + a.tv_nsec = NSEC_PER_SEC + 1; > + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec); > + ZUC_ASSERT_EQ(0, tv_sec_hi); > + ZUC_ASSERT_EQ(1235, tv_sec_lo); > + ZUC_ASSERT_EQ(1, tv_nsec); > + > + a.tv_sec = (time_t)0x7000123470005678LL; > + a.tv_nsec = 1; > + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec); > + ZUC_ASSERT_EQ(get_tv_sec_hi(&a), tv_sec_hi); > + ZUC_ASSERT_EQ(0x70005678, tv_sec_lo); > + ZUC_ASSERT_EQ(1, tv_nsec); > + > + a.tv_sec = (time_t)0x8000123480005678LL; > + a.tv_nsec = -1; > + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec); > + ZUC_ASSERT_EQ(get_tv_sec_hi(&a), tv_sec_hi); > + ZUC_ASSERT_EQ(0x80005678, tv_sec_lo); > + ZUC_ASSERT_EQ((uint32_t)-1, tv_nsec); > +} > + > ZUC_TEST(timespec_test, millihz_to_nsec) > { > ZUC_ASSERT_EQ(millihz_to_nsec(60000), 16666666); > @@ -302,6 +351,23 @@ ZUC_TEST(timespec_test, timespec_from_msec) > ZUC_ASSERT_EQ(1000000, a.tv_nsec); > } > > +ZUC_TEST(timespec_test, timespec_from_proto) > +{ > + struct timespec a; > + > + timespec_from_proto(&a, 0, 0, 0); > + ZUC_ASSERT_EQ(0, a.tv_sec); > + ZUC_ASSERT_EQ(0, a.tv_nsec); > + > + timespec_from_proto(&a, 0, 1234, 9999); > + ZUC_ASSERT_EQ(1234, a.tv_sec); > + ZUC_ASSERT_EQ(9999, a.tv_nsec); > + > + timespec_from_proto(&a, 0x1234, 0x5678, 1); > + ZUC_ASSERT_EQ(0x0000123400005678LL, a.tv_sec); > + ZUC_ASSERT_EQ(1, a.tv_nsec); > +} > + > ZUC_TEST(timespec_test, timespec_is_zero) > { > struct timespec zero = { 0 };
pgpaeuAuh3Wnc.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
