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

Ableton Link synchronization #3351

Closed
wants to merge 34 commits into from
Closed

Conversation

@smiarx
Copy link
Contributor

@smiarx smiarx commented Dec 23, 2017

This is a follow up of #3347. This branch adds Ableton Link synchronization to TempoClock beats and tempo.

Testing

I only have tested it in Linux as I write this.
You can test it with any SuperCollider object that uses TempoClock for scheduling, such as Pbind or Routine. A quant value must set to ensure proper phase synchronization of beats, and server latency must be set to zero when working with audio. For example

t = TempoClock(90/60).link;
Pbind(\degree, Pseq([0,1,2,3],inf), \latency, 0).play(t, quant:4);

Any other program that implements Link can be used to test synchronization, although LinkHut which is available in link's sources examples is simple and good enough for that purpose.

Interface & Design

The methods added to TempoClock are:

  • link(state=true) - (des)activates link synchronization
  • numPeers - gives the number of other client connected in the session
  • linkTempoChanged - can be set to a callback function called when the session's tempo change.

On the C++ side, a mLink variable is added to the TempoClock class, and is instantiated to a Link object with corresponding time, beat and tempo value when link state is set to true. Every time, beat or tempo related method then checks whether link is up and delegates to mLink if so.
When link is turned off, mLink calls its destructor and set TempoClock's mBaseBeats, mBaseSeconds and mTempo to ensure continuity between link and SuperCollider native clock.

Further changes

A few improvements can be made:

  • Time signature (beatsPerBar in SC or quantum in Link) changes during a link session isn't handled well right now and will result in phase shifts.
  • Add class methods for link in TempoClock that forward to the default instance
  • Add documentation
  • Add a CMake option to enable/disabled Link functionality at build time
@brianlheim
Copy link
Member

@brianlheim brianlheim commented Dec 23, 2017

OK. I think this is a great addition, but I'm not a huge fan of how it's been added. Extensions like this should be done with inheritance and composition, not by directly adding fields and methods to existing classes. Things would become a mess if we extended for every integration such as this.

Bits of code like

void LinkEnable(double seconds, double beatsPerBar);

which is essentially a constructor for a contained object, and

 if(!mLink)
            return (secs - mBaseSeconds) * mTempo + mBaseBeats;
        else{
            auto timeline = mLink->captureAppTimeline();
            double beats = timeline.beatAtTime(hrToLinkTime(secs), mQuantum);
            return beats;
        }

are code smells that say to me that this design could use some work. Are there any hurdles to rearchitecting it as a LinkClock that inherits from TempoClock in SC and then using inheritance or composition for a separate clock type in the C++ code?

I'd also recommend:

  • taking the time to format your code (i.e. match the whitespace convention in the file you're modifying)
  • putting each statement on its own line (returns and ++g->sp;). this is a convention that is unfortunately not consistent in the code, but it's an easy one to follow for new code.
  • not adding variables at file scope unless absolutely necessary (as you did with linkTimeOfInitialization)
  • adding CMake options so that users can build without this integration if they choose
@smiarx
Copy link
Contributor Author

@smiarx smiarx commented Dec 23, 2017

The idea behind adding this directly to TempoClock was that I could turn link on or off during a session without having to restart my patterns and routines. With a separate LinkClock you'll have to run your code again with the right clock when you want to get in or out of the link session.
It also makes every subclasses of TempoClock inherit the link functionalities.

I guess another solution would to let SC class as it is, but create a subclass of the C++ TempoClock class. Switching link on would mean replacing the C++ object pointed by the sclang object by an instance of LinkClock with the same properties. Primitives and SClang code can then be left unchanged, and the right C++ methods are selected using polymorphism, but it also means making them virtual.

@brianlheim
Copy link
Member

@brianlheim brianlheim commented Dec 23, 2017

Would an approach using composition be better? If you can show me some example code that would make it clearer how you intend to use it.

@smiarx
Copy link
Contributor Author

@smiarx smiarx commented Dec 23, 2017

What do you mean by an approach using composition ?
The idea I had in mind was that if run some code in SC, let's say

Pbind(\degree, Prand((0..12), inf)).play(quant:4);

I can join or quit a Link session juste by typing TempoClock.default.link(true) or TempoClock.default.link(false) and it would change the tempo and beat phase accordingly without interruption.
Whereas with a different clock class, I would have to start a new clock object an reevaluate the previous code every time I want to start or stop link

t = LinkClock(2);
Pbind(\degree, Prand((0..12), inf)).play(quant:4);

In this particular example it's not that big of a deal, but it could be annoying when you have a lot of different pieces of code running simultaneously.

In my case I also have a lot of custom objects that run different Routines, the second option would mean completely replace all of these objects with the correct new clock.

It seemed more intuitive to me this way, but I can definitely understand the argument that it doesn't fit well with the design of SuperCollider.

@nhthn nhthn added this to the 3.10 milestone Dec 23, 2017
@telephon
Copy link
Member

@telephon telephon commented Dec 25, 2017

What do you mean by an approach using composition ?

I think brian means composition instead of inheriance. https://en.wikipedia.org/wiki/Composition_over_inheritance

Optimal would be an object that would adjust any clock externally, through its public interface.

So something like, e.g.:

a = Link(TempoClock.default);
a.start;
@nhthn
Copy link
Contributor

@nhthn nhthn commented Dec 25, 2017

again, thanks for your work on this! i think it's pretty important that we leave open the possibility to compile sclang without Link when adding this functionality, since we should try to minimize "hard" dependencies whenever possible. (i'm undecided on whether it should be on by default... leaning yes since Link is not a huge dependency and itself has almost no dependencies.)

@colinsullivan
Copy link
Contributor

@colinsullivan colinsullivan commented Dec 26, 2017

Greetings,

This functionality is important to me and I'm happy to see it being prioritized.

I have been using a hack all in sclang, to update the tempo clock as it receives Ableton Link state over OSC.

I'm happy to take some time testing on OSX with other Ableton Link tools I've been building.

@colinsullivan
Copy link
Contributor

@colinsullivan colinsullivan commented Dec 26, 2017

@telephon

In terms of the design, it could go the other way too, i.e.

link = Link();
clock = TempoClock.default;
clock.follow(link);
@smiarx
Copy link
Contributor Author

@smiarx smiarx commented Dec 26, 2017

@telephon Ok I get the idea, so the TempoClock object can be used as it is and the Link object will only be used to modify the clock properties according to the Link session ? Seems fairly doable, and easier to to add a CMake option to deactivate the functionality this way. One thing though, if I want to modify the clock tempo or beats, I also want to forward the change to my Link object, so there should also be a way to register it.

smiarx added 4 commits Dec 29, 2017
Create a new LinkClock object which starts with the same tempo, beats
and seconds properties as a TempoClockObject.
It also stops the TempoClock and reschedules its tasks, creating a
seamless transition between the two clocks.
@smiarx
Copy link
Contributor Author

@smiarx smiarx commented Dec 29, 2017

I think I have found a nice compromise between all the solutions mentioned above.
I have made a LinkClock class which is a subclass of TempoClock with previous methods numPeers and tempoChanged. I've also added a newFromTempoClock class method, which takes a TempoClock, stops it and reschedules all its task to a new LinkClockobject.

So if I start a few tasks

t = TempoClock(2);
Pbind(\degree, Pseq([0,1,2,3],inf), \latency, 0).play(t, quant:4);
t.play({"X".postln; 1}, 4);
Routine { loop { "O".postln; 1.wait } }.play(t, quant:4);

I can switch Link seamlessly with

l = LinkClock.newFromTempoClock(t);
@telephon
Copy link
Member

@telephon telephon commented Dec 29, 2017

looks very good.

Mainly out of curiosity: why did you need to compromise? Was there something you couldn't do with an object that could control any clock through its public interface?

@smiarx
Copy link
Contributor Author

@smiarx smiarx commented Dec 29, 2017

The Link API only has a callback for tempo change, so when there is phase correction of the beat (when connecting to the session) the link clock and the tempo clock shift. I thought it would be safer to make the link object its own clock and let it handle seconds/beats conversions and scheduling.

@smiarx
Copy link
Contributor Author

@smiarx smiarx commented Jun 18, 2018

There is a problem with that, which we never resolved. Some patterns use thisThread.clock.beats to decide some of its behavior. If there's a timing offset, then the beats reflect the scheduled time, not the sounding time. There's a potential for confusion, if this scheduling time is compared with expected sounding time

Couldn't we resolve this issue like this ?

@@ -476,7 +476,9 @@ EventStreamPlayer : PauseStream {
 
        prNext { arg inTime;
                var nextTime;
-               var outEvent = stream.next(event.copy);
+               var outEvent;
+               thisThread.beats = thisThread.beats + (event[\timingOffset] ? 0);
+               outEvent = stream.next(event.copy);
                if (outEvent.isNil) {
                        streamHasEnded = stream.notNil;
                        cleanup.clear;

or like this

@@ -476,7 +476,10 @@ EventStreamPlayer : PauseStream {
 
        prNext { arg inTime;
                var nextTime;
-               var outEvent = stream.next(event.copy);
+               var inEvent, outEvent;
+               inEvent = event.copy;
+               inEvent[\beats] = thisThread.beats + (event[\timingOffset] ? 0);
+               outEvent = stream.next(inEvent);
                if (outEvent.isNil) {
                        streamHasEnded = stream.notNil;
                        cleanup.clear;
@telephon
Copy link
Member

@telephon telephon commented Jun 18, 2018

You use timingOffset to delay an event relative to the clock's beats. It shouldn't affect the clock's beats, and the clock or thread shouldn't need to know about it.

If the clock has a syncOffset parameter, then this is valid for all things scheduled on it. I think this would be completely ok (or at least I haven't seen a reason why you shouldn't have two separate if necessary LinkClocks, which have different syncOffset, if you are scheduling different things).

James Harkin's had a good idea to make it clear what the offset actually means.

@jamshark70
Copy link
Contributor

@jamshark70 jamshark70 commented Jun 18, 2018

Julian is correct. Absolutely do not under any circumstances mess with the clock's internal state. You can have methods to report different varieties of "now" -- we already have seconds vs beats, and the suggestion is to add a method to distinguish between non-offset logical time and offset, "sounding" logical time. But that's a matter of reporting the clock's internal state in different ways, not of modifying the clock.

@smiarx
Copy link
Contributor Author

@smiarx smiarx commented Jun 18, 2018

I see. What about the second the second solution ? Since "sounding time" is only relevant in a note event context, we can directly attach the value to processed events. That way it can be retrieved using Pfunc{ |ev| ev[\beats] } or Pkey(\beats).

@telephon
Copy link
Member

@telephon telephon commented Jun 18, 2018

Since "sounding time" is only relevant in a note event context […]

We shouldn't really assume that any sound is played. When you have a LinkClock, you just want something to happen in sync with what the other devices are doing. For this, it is good if the clock knows how to handle any kind of latency between the scheduled time and the "happening time". We shouldn't have to change all kinds of places where clocks could play a role. The very purpose of the clock itself is to maintain synchronicity, so no other part of the system should have to care about it.

Copy link
Member

@telephon telephon left a comment

I've made a suggestion of how to implement the latency question. Please everyone involved in the discussion, feel free to scrutinize or suggest alternatives. I just wanted to have a concrete thing to talk about.

this.schedAbs(oldQueue[i], task);
};

^this

This comment has been minimized.

@telephon

telephon Jun 18, 2018
Member

no need to return ^this, because this is what happens when you don't return.


LinkClock : TempoClock {

var <>tempoChanged;

This comment has been minimized.

@telephon

telephon Jun 18, 2018
Member

you could add an instance variable:
var <>aheadOfTime = 0

Then, the primitive code below should take this value into account. It would tell the other clocks the time plus the aheadOfTime value. The other clocks would do the same.

As a clock user, you'd write:

// for example:
t = LinkClock.new;
t.aheadOfTime = server.latency

This comment has been minimized.

@smiarx

smiarx Jun 19, 2018
Author Contributor

If I understand correctly we want an event scheduled at time t0 to be executed at time t0 - aheadOfTime. We should have thisThread.seconds == t0 at execution and then maybe another value thisThread.realSeconds == t0 - aheadOfTime.

Also sendBundle use thisThread.seconds to compute the OSC time tag, so there are two solutions if we want the sound to be really effective at t0:

  • Make sendBundle use thisThread.realSeconds instead, and the usage would be as you described.
  • Let sendBundle use thisThread.seconds, and you should have
s.latency = 0
t.aheadOfTime = 0.2// whatever pre-computing time is needed to prevent late messages

I would personally opt for the second option since latency should really mean "how late do we want to hear the sound".

This comment has been minimized.

@telephon

telephon Jun 19, 2018
Member

We can't set the server latency to zero, because other threads with other clocks may also schedule bundles, and they will be late.

Isn't there a third option? I imagine this:

Let sendBundle use thisThread.seconds as usual, but when synchronized with the other link clocks, this value corresponds to a time t + aheadOfTime. In other words, when the clock receives a signal from another clock, saying "for me it is four o'clock", it will set its internal, actual time to "four o'clock + aheadOfTime". This time is then used by thisThread.seconds, which schedules the event a little earlier, and therefore compensates whatever value you give it (like s.latency).

This comment has been minimized.

@telephon

telephon Jun 19, 2018
Member

I don't know much about the clock internals, so it might be all wrong …

This comment has been minimized.

@smiarx

smiarx Jun 20, 2018
Author Contributor

We can't set the server latency to zero, because other threads with other clocks may also schedule bundles, and they will be late.

I don't understand how it is an issue. We wan't LinkClock events to hit the speaker right on time so I wouldn't make a lot of sense to have other thread producing sound on the same server but with a non-zero latency ? Plus we can always set the latency thread-wise with the \latency entry in Pbind.

Let sendBundle use thisThread.seconds as usual, but when synchronized with the other link clocks, this value corresponds to a time t + aheadOfTime

This is also what I had in mind, and scheduling would happen like this:

  1. an event is scheduled at time t0, with a LinkClock setting its internal clock to the other clocks time plus aheadOfTime
  2. the event is executed at time t0 - aheadOfTime for the other clocks, so at time t0 for LinkClock and thus thisThread.seconds == t0.
  3. a bundle is created with a latency latency. Internally sendBundle compute the OSC time tag with the value thisThread.seconds + latency == t0 + latency and send it at t0 - aheadOfTime.
  4. the audio hits the speaker precisely at t0 + latency

This way aheadOfTime means "how far ahead do we compute events, and latency means "how far behind do we hear events", and they do not depend on each other (meaning changing aheadOfTime won't change the effective audio latency).

This comment has been minimized.

@smiarx

smiarx Jun 21, 2018
Author Contributor

Here is how i tried to implement it if it can help:
smiarx@7357216

This comment has been minimized.

@telephon

telephon Jun 21, 2018
Member

thanks! why is the IFDEF necessary? smiarx@7357216#diff-862389468167bb46f32e36801276b4cbR804

This comment has been minimized.

@smiarx

smiarx Jun 21, 2018
Author Contributor

Not necessary, it's just to avoid unnecessary calculations when Link is deactivated at compile time.

This comment has been minimized.

@telephon

telephon Jun 21, 2018
Member

ok, makes sense.

Have you tested it already?

This comment has been minimized.

@smiarx

smiarx Jun 24, 2018
Author Contributor

Yes with the example in the helpfile and

s.latency = 0;
l.aheadOfTime = 0.2;

I get an average latency of <1ms between the two programs.

@brianlheim
Copy link
Member

@brianlheim brianlheim commented Jun 21, 2018

(At the risk of causing further noise)

Can someone please summarize the status of this PR and #3790? Would it perhaps be a good idea to get on a call or move this discussion to some faster and more present medium of communication?

@smiarx
Copy link
Contributor Author

@smiarx smiarx commented Jun 24, 2018

@brianlheim LinkClock is functional but there is a debate on how to manage synchronization: most programs use ahead of time computation to obtain effective zero-latency, while SC has a fixed latency defined by s.latency. Since adding a delay to every program other than SC is unproductive and since LinkClock should work out-of-the box without too many adjustments, there is a consensus that LinkClock should also implement a mechanism to compute events ahead of time. We have yet to agree on the design of this mechanism.

#3790 fixes a few incoherences regarding buffer latency compensation of time-tagged events in scsynth & supernova that I noticed while testing:

  • scsynth compensate latency.
  • scsynth/JACK has a sign error resulting in delayed events.
  • supernova doesn't compensate latency.

I agree on moving this discussion to somewhere more practical.

@gonzaloflirt
Copy link

@gonzaloflirt gonzaloflirt commented Jun 27, 2018

What do people thing adding something along the lines of https://github.com/byulparan/LinkUGen?
Maybe in a follow-up PR.

@brianlheim
Copy link
Member

@brianlheim brianlheim commented Jun 30, 2018

@gonzaloflirt - Thanks for bringing that to people's attention, but it's off-topic for this PR, so please post it somewhere else (perhaps the mailing list).

@smiarx Thanks for the summary! OK, how should we hash this out? An email thread could be good, or we could try to organize a Skype call. I think if we have an hour or two to talk it out in person with everyone involved, we can reach a decision quickly and clear up this traffic jam. :) I don't have your contact info other than this thread, you can email me privately at the address on my profile.

@nhthn
Copy link
Contributor

@nhthn nhthn commented Jul 8, 2018

any updates? if no progress happens soon, this will have to be delayed to 3.11.

@telephon
Copy link
Member

@telephon telephon commented Jul 8, 2018

@snappizz there is an email discussion which hasn't quite started properly. I'll try and rekindle it.

@telephon
Copy link
Member

@telephon telephon commented Jul 8, 2018

Just not to lose it, here is the suggested solution the the remaining issue (adjustment of latency):

smiarx@7357216

@nhthn
Copy link
Contributor

@nhthn nhthn commented Jul 14, 2018

hey all -- thanks for your work on this, but we haven't seen progress in while and it isn't clear to me what's left to do here.

i talked this over with some other devs and, although this would be cool to get in 3.10, we would like to get it right the first time we merge it. there is only so long we can wait for this PR since there are many important bugfixes waiting to be released.

i'm giving this an arbitrary week or so, and if there isn't some kind of progress, we'll have to postpone it to 3.11.

@jamshark70
Copy link
Contributor

@jamshark70 jamshark70 commented Jul 15, 2018

I feel strongly that the primary use case is: a user wrote some patterns and is happily playing them on TempoClock. Now the user wants to play them in time with Live. I think we should make every effort so that the user should need only to do TempoClock.default = LinkClock.new(...) and it just works.

I believe, long-term, that this will lead to deprecating server latency. That is a radical suggestion, but think about it. Scheduling musical events in beats, while server latency is in seconds, is a design flaw that makes it next to impossible to handle sync with other apps while the (externally controlled) tempo is changing. Latency or aheadOfTime needs to be in beats, consistently. That's a quite massive change but, in the long term, we can't avoid it. In the short term, we'll have to maintain both and deprecate the older usage.

Also aheadOfTime needs to be implemented for TempoClock as well.

It pains me to say this (and I really really could use Link in a class next semester), but I don't think it's possible for us to "get it right the first time" in the next couple of weeks... especially considering that, six days ago, I wrote extensively about this in the e-mail thread that Brian started (off list) and subsequently saw exactly zero replies. We could have hammered a lot of that out by now, but... oh well.

@jamshark70
Copy link
Contributor

@jamshark70 jamshark70 commented Jul 15, 2018

Or, redefine latency to be beats rather seconds: OSC primitives that calculate timestamps should get the tempo from the current clock, and divide it into the latency.

It would affect very slow and very fast tempi (slow tempi may be less responsive, fast tempi may leave insufficient latency without raising the number).

Accelerandi and ritardandi would have slightly different timing. I started to investigate the concrete impact but wasn't able to finish it in the 15 minutes that I had.

Ah... better, maybe: consider latency in beats if the current clock is a LinkClock, in seconds otherwise. Then existing use cases would not be affected.

@nhthn
Copy link
Contributor

@nhthn nhthn commented Jul 15, 2018

well, that's a bummer. thanks for the info james.

@nhthn nhthn removed this from the 3.10 milestone Jul 15, 2018
@nhthn nhthn removed this from Doing in 3.10 Jul 15, 2018
@nhthn nhthn added this to To Do in 3.11 Release Jul 15, 2018
@jamshark70
Copy link
Contributor

@jamshark70 jamshark70 commented Jul 16, 2018

It might be very close based on the aheadOfTime commit that Julian mentioned. Dunno, maybe I'll take a crack on my trip back home (leaving tomorrow so I won't do much today).

It wouldn't hurt if there could be some feedback on the desired interface. Did I get anything wrong or miss anything?

@telephon
Copy link
Member

@telephon telephon commented Jul 16, 2018

It would be good if you could try.

Btw. I would also be OK with a provisional solution for 3.10 with a warning that it will change.

@nhthn
Copy link
Contributor

@nhthn nhthn commented Sep 14, 2018

would anyone be open to closing this PR and opening a new one with the conflicts fixed? this discussion has gotten really long and i think it would be good to start with a clean slate.

@jamshark70
Copy link
Contributor

@jamshark70 jamshark70 commented Sep 15, 2018

would anyone be open to closing this PR and opening a new one with the conflicts fixed?

Fine with me 👍

@nhthn
Copy link
Contributor

@nhthn nhthn commented Sep 21, 2018

rebased in #4081.

@nhthn nhthn closed this Sep 21, 2018
@nhthn nhthn removed this from To Do in 3.11 Release Jan 14, 2019
@jamshark70 jamshark70 mentioned this pull request Feb 24, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet