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

fixes (possibly?) for AudioControl mapping #3063

Merged
merged 13 commits into from
Aug 12, 2017
Merged

fixes (possibly?) for AudioControl mapping #3063

merged 13 commits into from
Aug 12, 2017

Conversation

joshpar
Copy link
Member

@joshpar joshpar commented Jul 29, 2017

fixes #2839

Copy link
Member

@telephon telephon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good - I'll test later. Could you fix the formatting please? You may have to set the preferences in xcode (if you are using that) to use 4 spaces instead of tabs, and then reformat.

@@ -35,6 +35,7 @@ struct Graph
uint32 mNumControls;
float *mControls;
float **mMapControls;
uint32 mAudioBusOffset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting? maybe convert tabs to space?

@joshpar
Copy link
Member Author

joshpar commented Jul 29, 2017 via email

@telephon
Copy link
Member

telephon commented Jul 30, 2017

This definitely fixes the bug in #2839. Thank you very much for this.

The formatting seems to be tabs, not spaces, probably it was this way round. I can fix that.

@telephon
Copy link
Member

OK, done.

@joshpar
Copy link
Member Author

joshpar commented Jul 30, 2017 via email

@telephon
Copy link
Member

@joshpar wrote (#2839):

I think this can be closed. But I'll leave some notes here. Unlike In and other bus UGens, AudioControl does NOT look right at the bus. It looks at a chunk of memory in the Graph that is allocated to store audio mapped data (similar to the way Controls work). As a result, when the Graph has Audio being mapped to it, a reference to the audio bus that is mapped needs to be stored - I added a place in the Graph to store the index of the mapped audio control. This is then checked against to see if data has been written to it (touched) or not. However, I'm not positive this will work well with multiple audio mapped controls. If someone can test for that in their usual usage, and if there is a problem create an new issue, I can dig deeper for other use cases.

@telephon
Copy link
Member

telephon commented Jul 30, 2017

@joshpar

rereading the example above, I've found that the new version doesn't output any glitches, but also doesn't output any audio.

Here is a slightly easier to use version of the first one by @jamshark70:

(
SynthDef(\fxbus, { |out, in|
	var sig = In.ar(in, 2);
	ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;

SynthDef(\fx, { |out, a_in = #[0, 0]|
	ReplaceOut.ar(out, HPF.ar(a_in, 800));
}).add;

SynthDef(\boop, { |out, freq = 440, gate = 1, amp = 0.1|
	var sig = Mix(VarSaw.ar(freq * [1, 1.004])) * amp,
	eg = EnvGen.ar(Env.adsr(0.01, 0.1, 0.5, 0.001, curve: 0), gate, doneAction: 2);
	sig = LPF.ar(sig, 3000);
	Out.ar(out, (sig * eg).dup(2));
}).add;
)

s.boot;
s.scope;

(
b = Bus.audio(s, 2);

p = Pbind(
	\instrument, \boop,
	\degree, Pwhite(-7, 7, inf),
	\dur, 1,
	\legato, 0.25,
	\out, b
).play;
)

// OK:
f = Synth.tail(s.defaultGroup, \fxbus, [in: b, out: 0]);  

// end this one.
f.free;

// try the audio mapped version: currently silent
(
s.makeBundle(nil, {
	f = Synth.tail(s.defaultGroup, \fx, [\out: 0]);
	//s.sendMsg("/n_mapan", f.nodeID, \a_in, b.index, 2);
	f.map(\a_in, b);
});
)

p.stop;
f.free;

for me, the last bit outputs silence now (it used to outputs glitches).

@telephon
Copy link
Member

telephon commented Jul 30, 2017

Also, this one crashes the server now :
(to be sure I've checked with a clean build)



(
SynthDef(\fx, { |out|
	ReplaceOut.ar(out, HPF.ar(\in.ar(0 ! 2), 800));
}).add;
)


(
b = Bus.audio(s, 2);

s.makeBundle(nil, {
	f = Synth.tail(s.defaultGroup, \fx, [\out: 0]);
	f.map(\in, b);
});
)

The OSC messages are identical:

With an "a_in" control name:
[ "/n_mapan", 1002, "a_in", 10, 2 ]

With an "in" control name:
[ "/n_mapan", 1011, "in", 10, 2 ]

@joshpar
Copy link
Member Author

joshpar commented Jul 30, 2017 via email

@telephon
Copy link
Member

Sure, here you are:

(
SynthDef(\fxbus, { |out, in|
	var sig = In.ar(in, 2);
	ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;

SynthDef(\fx, { |out, a_in = #[0, 0]|
	ReplaceOut.ar(out, HPF.ar(a_in, 800));
}).add;

)

s.boot;
s.scope;

(
b = Bus.audio(s, 2);

{ Out.ar(b, SinOsc.ar(330) * 0.1 ! 2) }.play;
)

f = Synth.tail(s.defaultGroup, \fxbus, [in: b, out: 0]);  // OK

f.free;

(
s.makeBundle(nil, {
	f = Synth.tail(s.defaultGroup, \fx, [\out: 0]);
	//s.sendMsg("/n_mapan", f.nodeID, \a_in, b.index, 2);
	f.map(\a_in, b);
});
)

p.stop;
f.free;

@joshpar
Copy link
Member Author

joshpar commented Jul 31, 2017 via email

joshpar added 2 commits July 30, 2017 19:42
…percollider into AudioControlFix

# Conflicts:
#	include/plugin_interface/SC_Graph.h
#	server/plugins/IOUGens.cpp
#	server/scsynth/SC_Graph.cpp
@joshpar
Copy link
Member Author

joshpar commented Jul 31, 2017

new fixes are in for checking. @telephon - was the sample that crashed something that you would expect to hear audio with?

@telephon
Copy link
Member

@joshpar no, I didn't expect audio to come out of that example. It was just there to crash the server.

@joshpar
Copy link
Member Author

joshpar commented Jul 31, 2017 via email

@mossheim
Copy link
Contributor

Does the pull request automatically get updated or do I need to do a new one?

If you're pushing to the remote branch that you used to make the PR, the PR gets updated automatically.

@joshpar
Copy link
Member Author

joshpar commented Jul 31, 2017 via email

@telephon
Copy link
Member

Excellent. This works for the following test, and doesn't crash any of the above.

(
SynthDef(\fxbus, { |out, in|
	var sig = In.ar(in, 2);
	ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;

SynthDef(\fx, { |out|
	ReplaceOut.ar(out, HPF.ar(\in.ar(0 ! 2), 800));
}).add;
SynthDef(\source, { |out|
	Out.ar(out, SinOsc.ar(330) * 0.1 ! 2 * Line.kr(1, 0, 0.8, doneAction:2))
}).add;
)

s.boot;
s.scope;

(
b = Bus.audio(s, 2);

fork { 
	loop { 
		Synth(\source, [\out, b]);
		1.5.wait 
	} 
};
)

f = Synth.tail(s.defaultGroup, \fxbus, [in: b, out: 0]);  // OK

f.free;

(
s.makeBundle(nil, {
	f = Synth.tail(s.defaultGroup, \fx, [\out: 0]);
	f.map(\in, b);
});
)

p.stop;
f.free;

@telephon
Copy link
Member

OK, so one more thing. So far, the audio rate controls worked as analogies to InFeedback, so that it would grab the previous buffer content if necessary. This allowed us to ignore node order if there is only one writer on that bus.

This is the reason why the ProxySpace example still doesn't work:


( 
SynthDef(\boop, { |out, freq = 440, gate = 1, amp = 0.1| 
	var sig = Mix(VarSaw.ar(freq * [1, 1.004])) * amp, 
	eg = EnvGen.ar(Env.adsr(0.01, 0.1, 0.5, 0.001, curve: 0), gate, doneAction: 2); 
	sig = LPF.ar(sig, 3000); 
	Out.ar(out, (sig * eg).dup); 
}).add
) 

(
p = ProxySpace.new.push; 
~out = { \in.ar(0 ! 2) };
~out.play(vol: 0.2);
~player = Pbind( \instrument, \boop, \degree, Pwhite(-7, 7, inf), \dur, 1, \sustain, 0.8); 
~player <>> ~out; 

)

While this one works:

(
p = ProxySpace.new.push; 
~player = Pbind( \instrument, \boop, \degree, Pwhite(-7, 7, inf), \dur, 1, \sustain, 0.8); 
~player <>> ~out; 
~out = { \in.ar(0 ! 2) };
~out.play(vol: 0.2); 
)

I'll post a simpler test case.

@telephon
Copy link
Member

Here is a test case:


(
SynthDef(\fxbus, { |out, in|
	var sig = InFeedback.ar(in, 2);
	ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;

SynthDef(\fx, { |out|
	ReplaceOut.ar(out, HPF.ar(\in.ar(0 ! 2), 800));
}).add;
SynthDef(\source, { |out|
	Out.ar(out, SinOsc.ar(330) * 0.1 ! 2 * Line.kr(1, 0, 0.8, doneAction:2))
}).add;
)

s.boot;
s.scope;

(
b = Bus.audio(s, 2);

fork { 
	loop { 
		Synth.tail(s.defaultGroup, \source, [\out, b]);
		1.5.wait 
	} 
};
)

f = Synth.head(s.defaultGroup, \fxbus, [in: b, out: 0]);  // OK

f.free;

// add to head: no sound.
(
s.makeBundle(nil, {
	f = Synth.head(s.defaultGroup, \fx, [\out: 0]);
	f.map(\in, b);
});
)

// add after: yes sound.
(
s.makeBundle(nil, {
	f = Synth.after(s.defaultGroup, \fx, [\out: 0]);
	f.map(\in, b);
});
)

p.stop;
f.free;

@joshpar
Copy link
Member Author

joshpar commented Jul 31, 2017 via email

@telephon
Copy link
Member

telephon commented Jul 31, 2017

Modulating the bus argument of an InFeedback doesn't cause glitches:

https://github.com/supercollider/supercollider/blob/master/server/plugins/IOUGens.cpp#L666


(
SynthDef(\fxbus, { |out, in|
	var sig = InFeedback.ar(in, 2);
	ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;
SynthDef(\source, { |out|
	Out.ar(out, SinOsc.ar(330) * 0.1 ! 2)
}).add;
)


(
b = Bus.audio(s, 2);
Synth(\source, [\out, b]);

f = Synth.head(s.defaultGroup, \fxbus, [in: b, out: 0]);  // OK

fork { loop { 0.5.wait; f.set(\in, 99); 0.5.wait; f.set(\in, b); } };
)

Why must the AudioControlbe different? (sorry, just asking.)

@mossheim
Copy link
Contributor

mossheim commented Aug 4, 2017

glad @vivid-synth asked - using the original testing code, this now crashes supernova (before, it just had the same issue).

(
SynthDef(\fxbus, { |out, in|
	var sig = InFeedback.ar(in, 2);
	ReplaceOut.ar(out, HPF.ar(sig, 800));
}).add;

SynthDef(\fx, { |out, a_in = #[0, 0]|
	ReplaceOut.ar(out, HPF.ar(a_in, 800));
}).add;

SynthDef(\boop, { |out, freq = 440, gate = 1, amp = 0.1|
	var sig = Mix(VarSaw.ar(freq * [1, 1.004])) * amp,
	eg = EnvGen.ar(Env.adsr(0.01, 0.1, 0.5, 0.001, curve: 0), gate, doneAction: 2);
	sig = LPF.ar(sig, 3000);
	Out.ar(out, (sig * eg).dup);
}).add;
)

s.boot;
s.scope;

b = Bus.audio(s, 2);

(
p = Pbind(
	\instrument, \boop,
	\degree, Pwhite(-7, 7, inf),
	\dur, 1,
	\legato, 0.25,
	\out, b
).play;
)

f = Synth.tail(s.defaultGroup, \fxbus, [in: b, out: 0]);  // OK

f.free;

(
s.makeBundle(nil, { // SUPERNOVA CRASH
	f = Synth.tail(s.defaultGroup, \fx);
	f.map(\a_in, b);
});
)

p.stop;
f.free;

@mossheim
Copy link
Contributor

mossheim commented Aug 4, 2017

i'm getting a bad access here:

int thisChannelOffset = channelOffsets[unit->mSpecialIndex + i];

this mirrors changes introduced to scsynth in fdf1fb0
@mossheim
Copy link
Contributor

mossheim commented Aug 4, 2017

@joshpar , i took your changes from fdf1fb0 and applied them to similar spots in supernova. the original test code now works with supernova like it did with scsynth.

@@ -35,6 +35,8 @@ struct Graph
uint32 mNumControls;
float *mControls;
float **mMapControls;
int32 *mAudioBusOffsets;
uint32 mAudioBusOffset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears to have been accidentally reintroduced - could you please remove it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or I can do it, I don't want this to get too confusing though :)

@joshpar
Copy link
Member Author

joshpar commented Aug 4, 2017 via email

@telephon
Copy link
Member

telephon commented Aug 4, 2017

great!

@mossheim
Copy link
Contributor

mossheim commented Aug 4, 2017

all right. should any tests / other documentation be added for this? i am otherwise good to merge.

@telephon
Copy link
Member

telephon commented Aug 8, 2017

i think it is ok to merge. we do need some systematic tests for mapping, but this is another issue

@vivid-synth
Copy link
Member

Thanks @joshpar and @brianlheim !

@joshpar joshpar changed the base branch from master to 3.9 August 12, 2017 05:03
@joshpar
Copy link
Member Author

joshpar commented Aug 12, 2017

@snappizz - pretty sure this is ready to go

@nhthn nhthn added this to the 3.9 milestone Aug 12, 2017
@nhthn nhthn self-assigned this Aug 12, 2017
@telephon telephon merged commit 891fcbc into 3.9 Aug 12, 2017
@nhthn nhthn removed their assignment Aug 12, 2017
@@ -35,6 +35,7 @@ struct Graph
uint32 mNumControls;
float *mControls;
float **mMapControls;
int32 *mAudioBusOffsets;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timblechmann changing an API from v1 to v2 gives me some pause -- will this potentially break existing code?

Copy link
Contributor

@nhthn nhthn Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't break any code, but it does break binaries. you can't use server 3.9 with plugin binaries from 3.8, or vice versa and you have to recompile all your plugins.

@joshpar joshpar mentioned this pull request Aug 12, 2017
@nhthn nhthn deleted the AudioControlFix branch August 14, 2017 03:19
dyfer added a commit to dyfer/supercollider that referenced this pull request Sep 29, 2017
@dyfer
Copy link
Member

dyfer commented Nov 2, 2017

I know this is closed, but it's causing a rather likely crashes in supernova: #3196 I think the referenced issue needs to be addressed before this gets included in the 3.9 release...

@nhthn
Copy link
Contributor

nhthn commented Nov 4, 2017

two issues that have since been identified as side effects of this PR: #3196 and #3266. i have marked them as required for 3.9.

dyfer pushed a commit to dyfer/supercollider that referenced this pull request Dec 9, 2017
In sc_synthdef::prepare(void), the memory requirement for a sc_synth
structure was underreported because it did not change with supercollider#3063.
As a result, there was memory corruption. This commit both fixes the
issue and adds clarifying comments to the calculations.

Fixes supercollider#3196, supercollider#3266.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants