Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in timetag to time.Time conversion #56

Closed
tleb opened this issue Feb 23, 2022 · 4 comments · Fixed by #58
Closed

Error in timetag to time.Time conversion #56

tleb opened this issue Feb 23, 2022 · 4 comments · Fixed by #58

Comments

@tleb
Copy link

tleb commented Feb 23, 2022

Extract from the code:

// timetagToTime converts the given timetag to a time object.
func timetagToTime(timetag uint64) (t time.Time) {
	return time.Unix(int64((timetag>>32)-secondsFrom1900To1970), int64(timetag&0xffffffff))
}

This function is used on bundle reception (readBundle called by readPacket called by a few functions) to parse received timetags. It takes the lower 32 bits from the timetag and gives them as the second argument of time.Unix, that has the following signature and doc:

// Unix returns the local Time corresponding to the given Unix time,
// sec seconds and nsec nanoseconds since January 1, 1970 UTC.
// It is valid to pass nsec outside the range [0, 999999999].
// Not all sec values have a corresponding time value. One such
// value is 1<<63-1 (the largest int64 value).
func Unix(sec int64, nsec int64) Time

However, the OSC 1.0 spec describes time tags as:

Time tags are represented by a 64 bit fixed point number. The first 32 bits specify the number of seconds since midnight on January 1, 1900, and the last 32 bits specify fractional parts of a second to a precision of about 200 picoseconds. This is the representation used by Internet NTP timestamps.The time tag value consisting of 63 zero bits followed by a one in the least signifigant bit is a special case meaning “immediately.”
https://ccrma.stanford.edu/groups/osc/spec-1_0.html#timetags

As such, the lower 32 bits should be used as fractions of a second (1 second / 2^32 ~= 0.233 nano second per fraction) rather than whole nanoseconds.

If this analysis is right, this bug affects every bundle received by this library by a big amount: between 0s at the start of the second up to, at the end of a second, 2^32-1 nanoseconds - 1 second, or ~3.295 seconds! I have not run the code, but I assume people using the bundle feature would notice such big delays in dispatch?

@tleb
Copy link
Author

tleb commented Feb 23, 2022

This implementation seems OK to me:

const secondsFrom1900To1970 = 2208988800                       // Source: RFC 868
const nanosecondsPerFraction = float64(0.23283064365386962891) // 1e9/(2^32)

func timetagToTime(timetag uint64) time.Time {
	if timetag == 1 {
		// Means "immediately". It cannot occur otherwise as timetag == 0 gets
		// converted to January 1, 1900 while time.Time{} means year 1 in Go.
		// Use the IsZero method to detect it.
		return time.Time{}
	}

	return time.Unix(
		int64(timetag>>32)-secondsFrom1900To1970,
		int64(nanosecondsPerFraction*float64(timetag&(1<<32-1))),
	)
}

func timeToTimetag(v time.Time) uint64 {
	if v.IsZero() {
		return 1
	}

	seconds := uint64(v.Unix() + secondsFrom1900To1970)
	secondFraction := float64(v.Nanosecond()) / nanosecondsPerFraction
	return (seconds << 32) + uint64(uint32(secondFraction))
}

Here, we handle timetag == 1 as a special case that means "immediately", as described by the specification. It is up to the code above to handle t.IsZero to detect immediate action requests (might be dealt with differently compared to a date in the past). If not handled specially, it will be dealt with in the same way as previous timetags.

For the nanosecond part, we pre-calculate the number of nanoseconds per fraction so that its a simple floating point multiplication at runtime. We could round it to the nearest nanosecond rather than flooring it as the format has a precision of 0.233ns. It might not be a good idea to bring something from the past into the future (if we ever had transfers faster than a ns and time measurement precise enough...). Whatever!

Something else: time.Unix returns a time in the current system's timezone, not in UTC (while the timetag = 0 case gives a UTC time). It might or might not be what we want. We could turn the result of time.Unix into a UTC time using the time.Time.In method (+ the time.UTC constant).


edited to fix mistake in the function implementation

edited to add the inverse operation

@tleb
Copy link
Author

tleb commented Mar 8, 2022

Cool to see this lib being updated. Are you using it in a project? Music-related?

@hypebeast
Copy link
Owner

Hey :) First, thanks a lot for reporting the issue and your work. You were of course completely right. Happy that you discovered this bug.

I used OSC a lot during my work at my former employer. There we used OSC for controlling devices and content in interactive installations, tv shows, etc.

@tleb
Copy link
Author

tleb commented Mar 26, 2022

No problem, I was looking at using OSC for a Go project.

Nice! That's the kind of domain I'm looking at as well, having background in stage lighting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants