Converting between OscTime and SystemTime

[ooesili]

Hello. I wrote this function in some code that used this library and thought that it would be useful for other people. I could not find an easy way to create an OscTime from SystemTime in this library.

use rosc::OscTime;
use std::time::{Duration, SystemTime};

fn system_time_to_osc(time: SystemTime) -> OscTime {
    const UNIX_OFFSET: u64 = 2_208_988_800; // From RFC 5905
    const TWO_POW_32: f64 = 4294967296.0;

    let unix_time = time.duration_since(SystemTime::UNIX_EPOCH).unwrap(); // Safe absent time machines
    let unix_offset = Duration::new(UNIX_OFFSET, 0);
    let epoch_time = unix_offset + unix_time;
    let seconds = epoch_time.as_secs() as u32;
    let nanos = epoch_time.subsec_nanos() as f64;
    let fractional = ((nanos * TWO_POW_32) / 1.0e9).round() as u32;
    (seconds, fractional)
}

I based the function from some code I found in Cloudflare's cfnts project which uses NTP timestamps which appear to be the same format used in OSC.

I was going to make a PR to add it but there appeared to be a few different ways to go about implementing it and I wanted some input on the approach. I thought of three alternatives. Let me know if this is something you want to include in the library and if you have a preference between one of these.

1. Add this function as-is to the crate

This is the easiest option and is the only non-breaking change. Unfortunately, because OscTime is currently a type alias to (u8, u8), we can not add implementations to OscTime, such as impl From<SystemTime> for OscTime, which would provide the nicest experience to performing this conversion in my opinion.

2. Change OscTime into a struct

If we changed OscTime into a struct we could implement the conversion from SystemTime like this:

#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct OscTime {
    pub seconds: u32,
    pub fractional: u32,
}

// Now we can do this.
impl From<SystemTime> for OscTime {
    fn from(time: SystemTime) -> OscTime {
        // ...
    }
}

This provides a better experience in my opinion, but would be a breaking change. Than the previous one.

3. Completely Replace OscTime with SystemTime

I haven't quite thought though the ramifications of this one. SystemTime does appear able to represent times as far back as 1900-01-01 00:00:00 UTC so in theory we should be able to represent every OSC time as a SystemTime.

[ooesili]

I dug into it a little bit more, and found out that the conversions are a little bit lossy. This means that option 3 as I mentioned above is probably not a good idea because decoded and re-encoding an OSC timestamp might result in a different value than the original.

I liked option 2 the most so I put together pull request #14. I think the breaking change is OK because this library is still not at 1.0 and the conversions I added to and from (u32, u32) should hopefully allow users to fix their code with a handful of simple .into() calls and such.

[klingtnet]

Option two would have been my favorite as well, so I am happy that you have implemented that in #14 . When this is merged I plan to do a 0.5.0 release due to the, even though pretty minor, breaking change.

[klingtnet]

This is now released with rosc@0.5.0. Thanks again for providing the implementation 👍🏻

[nicocti]

Hello ! There is an issue in the current implementation for 32 bits systems here: https://github.com/klingtnet/rosc/blob/e30112c512d44ec0426a3e5ee9b07cb096eeb395/src/types.rs#L58

To reproduce, you can run the following 32-bits Dockerfile:

FROM i386/rust:buster
ENV RUST_BACKTRACE=1
COPY src/main.rs .
RUN rustc main.rs
CMD ./main

with main.rs being:

#![allow(unused)]
fn main() {
    use std::time::{Duration, UNIX_EPOCH};
    const UNIX_OFFSET: u64 = 2_208_988_800;
    UNIX_EPOCH - Duration::from_secs(UNIX_OFFSET);
}

It returns:

thread 'main' panicked at 'overflow when subtracting duration from instant', library/std/src/time.rs:555:31
stack backtrace:
   0: rust_begin_unwind
             at ./rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at ./rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0/library/core/src/panicking.rs:92:14
   2: core::option::expect_failed
             at ./rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0/library/core/src/option.rs:1292:5
   3: core::option::Option<T>::expect
             at ./rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0/library/core/src/option.rs:349:21
   4: <std::time::SystemTime as core::ops::arith::Sub<core::time::Duration>>::sub
             at ./rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0/library/std/src/time.rs:555:9
   5: main::main
   6: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[klingtnet]

Thanks for reporting, I haven't thought about 32-bit systems when reviewing this. I will try to reproduce and fix this later today. Is it okay for you @nicocti to create a separate issue for tracking this problem?

[ooesili]

@klingtnet @nicocti I just pushed up #17 to address this. Let me know what you think.