On Tue, 12 Dec 2017 14:43:11 +0200 Alexandros Frantzis <[email protected]> wrote:
> On Tue, Dec 12, 2017 at 12:09:59PM +0200, Pekka Paalanen wrote: > > 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, > > 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. > > Ack. > > > > > > > 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.) > > I am not opposed to introducing a struct timespec_proto in general, but > using it in only one of the two functions feels inconsistent. Ok. I was thinking of saving in the number of local variables on every call site of timespec_to_proto(). > > > 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. > > We can only do this safely if tv_sec is non-negative (due to > int32_t->uint64_t sign extension when time_t is 32 bits), which > I agree is a sensible pre-condition/assertion to have (see below). Why would the sign-extension be unwanted? I would expect it. > > I think it would be better to assert() that the timespec is already: > > > > - normalized, because otherwise nsec might overflow in a computation, > > and > > If we make normalized input a pre-condition of timespec_to_proto, we are > essentially moving the burden to the caller, which in most cases should > call timespec_normalize themselves before calling timespec_to_proto. I > am not sure if we should burden the callers with this and introduce the > possibility of runtime errors. There would be no burden if we ensure our timestamps are always normalized, which is a precondition for the timespec sub/add etc. functions to operate without overflows. Therefore I'd push the normalize calls to not just the callers but the sites where the timestamps originate. > Also, I like the idea that the callers don't need to care about protocol > encoding details, and just let timespec_to_proto do the conversion > properly. I think there are two different "normalized" here. One is struct timespec, another is what the protocol carries. Compared to struct timespec, the protocol has an additional requirement of non-negativity. I don't see that as leaking protocol encoding details - OTOH, we must not even attempt to send values the protocol cannot carry. > > - non-negative, because the protocol cannot carry negative > > timestamps. > > Makes sense. > > > > > > + *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 good question indeed. Overflow shouldn't be a problem for well > behaving clients (if we are talking about real-time timestamps then not > until 2038 for 32-bits and not ever for 64-bits). The question is how to > deal with misbehaving and/or malicious clients. How does client behaviour factor into it? I wouldn't expect anyone to handle timespec wrapping around, but for input events this can be fixed in the compositor by shifting the time base if even necessary. > There are two aspects to this. The first is 64-bit to 32-bit overflow on > 32-bit systems, which is currently handled by implicitly dropping the > high 32-bits. The second is unsigned to signed overflow, which currently > produces negative timestamps. One way to deal with this is to clamp to > the largest positive value if we ever get a negative value. Another way > is to force the sign bit to 0, essentially making negative values cycle > back to non-negative. I wonder where time_t would still be 32-bit. I suppose we'll ignore the issue for now. > > > + 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. > > Sure, I will split this in v2. Thanks, pq
pgpjzw4GrI3Iq.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
