Ableton / link

Ableton Link
Other
1.09k stars 149 forks source link

Changes Clock::Ticks to be signed int64 #132

Closed pwnified closed 1 year ago

pwnified commented 1 year ago

The host time in core audio callbacks is not guaranteed to be higher than an arbitrary song length, for example on the simulator right after a reboot. The mHostTime in the timestamp could be a low value compared to a beat remapping of, say, an hour into a song. Playing a song starting at the one hour mark will then cause a negative songZero, unless the computer running the simulator has been running for more than an hour. And the unsigned datatype is then misinterpreted.

This pull request changes the datatype to signed to allow for these negative anchors.

pwnified commented 1 year ago

It should be noted that this is a bugfix, and the problem is exhibited on iPads/iPhones after a reboot as well. The mHostTime starts from zero at reboot and will then cause negative anchors for any beatTime request larger than the devices uptime.

fgo-ableton commented 1 year ago

Thanks for the PR! I understand the issue. I am wondering what the reason to query this host time is though. I can't come up with an actual scenario where one would require a host time at a song position that lies so far in the past. Addressing the issue by changing the value to unsigned integers will break for machines that have been running for a long time as mach_absolute_time() returns a uint64.

pwnified commented 1 year ago

Well, for our app, everything is stored as a sample offset from the beginning of the song, or 'songZero' a hostTime which is recomputed whenever playback starts. If playback is starting at beat 1000, I use: ABLLinkRequestBeatAtStartPlayingTime(1000, hostTimeAtBufferStart);

So any recording which is tagged with hostTimes can just subtract the song zero to get the offset. This is as easy as calling ABLLinkTimeAtBeat(0) to get the host time at the zero reference. This will lead to the negative host times right after a reboot.

I'm surprised this hasn't been brought up before. Is using beat times instead of host times the obvious solution? That would at least have an easy song zero, which would simply be zero. Unfortunately it's too hard to change this aspect of the engine currently.

As far as losing 1 bit from the uint64, we still have around 12,000 years :)

Anyway, I can just change it here and it works for our purposes, and doesn't affect other hosts that use the unsigned version.

fgo-ableton commented 1 year ago

I see. Thanks for the explanation! I guess the reason for the issue not coming up before is that referencing stuff like this using host time is not the most common approach. Anyways, the purpose of ticksToMicros and microsToTicks is to translate between the native clock representation and chrono::microseconds. So changing Ticks to int64 would defeat that purpose and likely cause warnings/errors for anyone using this as it is now. Of cause you're right regarding microsToTicks not working for negative values. I'll add a comment and an assertion documenting that. Other than that, as you are neither using chromo::microseconds nor the Darwin native format, I think it makes sense to do the conversion to what you are using outside the Link library.

pwnified commented 1 year ago

Yeah, that makes sense to have Ticks match the native type. However I'll still need to cast within the library to get this to work, ie:

  Micros ticksToMicros(const Ticks ticks) const
  {
    return Micros{llround(mTicksToMicros *  static_cast<std::int64_t>(ticks))};
  }

Since this is the point of interpretation. Also it will still throw Undefined Behavior errors in Xcode with the sanitizer turned on, when calling microsToTicks with negative micros, for example: -139920 is outside the range of representable values of type 'unsigned long long' But then casting this misinterpreted value outside the library to an int64_t works.