klingtnet / rosc

An OSC library for Rust.
Apache License 2.0
173 stars 25 forks source link

Base OscTime/SystemTime conversions on UNIX_EPOCH #17

Closed ooesili closed 3 years ago

ooesili commented 3 years ago

This allows the math to work on 32 bit systems where SystemTime cannot represent times as far back as the OSC epoch. As a consequence times before the unix epoch can not be converted between, even on 64-bit systems. This has been documented.

klingtnet commented 3 years ago

First, the implementation looks fine to me, kudos for doing the hard work @ooesili :) But I will need to have a second look, generally it should be fine to deny timestamps from before the Unix epoch but I am still a bit on the fence with this since we then diverge from the OSC specification. For the majority of cases this should be fine, hence adjusting the logic based on register width is probably not worth the hassle.

ooesili commented 3 years ago

I don't think it's technically diverging from the specification, because users can still create OscTime values as far back as the OSC epoch, they just won't be able to do so by converting from a SystemTime.

klingtnet commented 3 years ago

Here's a proposal that uses conditional compilation based on the target systems pointer width to allow the full conversion range on 64-bit systems and the epoch-time limitation for others. See commit edaf5caf0413a52fe0307c782ddc47bbbd8b1800 for the implementation. @ooesili You are free to cherry pick this change and to give feedback on the implemenation of course.

klingtnet commented 3 years ago

@ooesili Did you take a look at edaf5caf0413a52fe0307c782ddc47bbbd8b1800 ?

ooesili commented 3 years ago

Hey there! Sorry for the delay. I was dealing with some quarantine blues and haven't really been coding for the last couple weeks. I looked into this a little bit more today and it seems that the rules for determining the range of times a SystemTime can represent on any given platform are more complex than just looking at target_pointer_width. Here are the sources for libc::time_t on a few platforms to show you what I mean (time_t is used to hold the seconds portion of SystemTime).

Considering the complexity of figuring this out, and the fact that in practice OSC timestamps are primarily used (based on my experience and understanding of the OSC spec) for specifying times close to or slightly ahead of the current time, I feel pretty confident in the current implementation in this PR. It seems unlikely to me that users would try to create OSC timestamps before 1970, and this implementation does not prevent users from doing so, they would just have to construct an OscTime directly instead of converting to one from a SystemTime.

TL;DR I strongly agree with what you already said :)

For the majority of cases this should be fine, hence adjusting the logic based on register width is probably not worth the hassle.

klingtnet commented 3 years ago

Hey there! Sorry for the delay. I was dealing with some quarantine blues and haven't really been coding for the last couple weeks.

No worries, took me also quite some time to reply, hope you're doing well now.

I looked into this a little bit more today and it seems that the rules for determining the range of times a SystemTime can represent on any given platform are more complex than just looking at target_pointer_width.

Thanks for having a deeper look and, yes, it's more complicated than I initially thought. Actually I'm happy to skip edaf5caf0413a52fe0307c782ddc47bbbd8b1800 since it would have introduced additional complexity without benefit, hence I will go with the initial approach and merge this PR as-is. Thanks again for the fix!