Potential Bug with Local In and In.kr?

Questions
1

I was curious if anyone knew about the following error message if it is a bug or what is going on there.

~filterBus = Bus.control(s);
~filterBus2 = Bus.control(s);

{var in1, in2, sig, feed,my,mouseX;
	in1 = (In.kr(~filterBus)*0.4)+0.9;
	in2 = (In.kr(~filterBus2)*0.4)+0.9;

	feed = LocalIn.ar(2);
	// feed = SinOsc.ar(200*[1,1.5],0,0) + feed;
	feed[0] = LeakDC.ar(DelayN.ar(feed[0]+(0.05*feed[1]),0.04,0.02));
	feed[1] = LeakDC.ar(PitchShift.ar(feed[1]+(0.5*feed[0]),0.5,[0.8,1,1.2,1.5])).sum;
	feed = BPF.ar(feed,LFNoise1.kr([0.05,0.05]).exprange(125,2000).poll);
	sig = Saw.ar(50,0.001) + (feed*[MouseX.kr(0.5,2,2,0).poll,MouseY.kr(0.5,2,2,0).poll]);
	sig = Limiter.ar(sig,0.5);
	LocalOut.ar(sig);
	Pan2.ar(sig[0],-0.5) + Pan2.ar(sig[1],0.5);
}.play;

Here it gives me the error:

buffer coloring error: tried to release output with zero count
output: 7 DelayN 0
input: MulAdd 2
exception in GraphDef_Recv: buffer coloring error.
*** ERROR: SynthDef temp__32 not found
FAILURE IN SERVER /s_new SynthDef not found

It’s interesting, since (for this example) the In.kr which seems to be causing the trouble isn’t even doing anything in the patch.

However, if you uncomment the line

feed = SinOsc.ar(200*[1,1.5],0,0) + feed;

in the code everything seems to work.

Thanks!

2

Interesting problem. I don’t have an answer right now; I can probably take a look tomorrow. (Locked up in first round admissions auditions for a few days this week, 12 hour days, not a lot of bandwidth for deep investigation of weird SynthDef issues.)

Will do when I can…

hjh

3

OK, so, this is a hard issue to troubleshoot.

TL;DR: Mutating the arrays returned by multichannel UGens is not safe.

That is, the mistake is here:

	feed[0] = feed[0]+(0.05*feed[1]);
	feed[1] = feed[1]+(0.5*feed[0]);

feed[0] = and feed[1] = overwrite objects in the LocalIn’s output channels array. From that point forward, the LocalIn object has been corrupted, and it can no longer write a proper binary representation of itself into the SynthDef stream. The error message then comes from the server when it is trying to interpret the incorrect binary stream.

The error doesn’t occur when adding the zero amplitude sinewave. Now the array that you are mutating is no longer identical to the LocalIn channels array. Mutating the new array does not corrupt the LocalIn, hence no problem.

Fix:

	feed = feed.copy;
	feed[0] = feed[0]+(0.05*feed[1]);
	feed[1] = feed[1]+(0.5*feed[0]);

A little bit of the troubleshooting process:

The first step was to simplify – take something out of the SynthDef. If it doesn’t change the result (error or no error), then leave it out. That takes the reproducer down to this:

(
d = SynthDef(\test, { |out|
	var in1, in2, sig, feed,my,mouseX;
	in1 = (In.kr(~filterBus)*0.4)+0.9;
	in2 = (In.kr(~filterBus2)*0.4)+0.9;

	feed = LocalIn.ar(2);
	feed[0] = feed[0]+(0.05*feed[1]);

	sig = feed;

	LocalOut.ar(sig);
	Out.ar(out, sig);
}).add.dumpUGens;
)

At this point, the error vs ok SynthDefs dump the same list of UGens (this is the hard part – the dumpUGens output shows each UGen’s link to its inputs but doesn’t show the state of its outputs, so the broken connection was invisible)… so I had to go on a deep dive into the binary stream, where I found one byte different. Inserting some debug output into writeDef stuff revealed that the offending byte comes from one of the LocalIn channels. At this point, I was about to give up but then decided to check whether the array being mutated was identical to the array that’s owned by LocalIn… and it was.

Hard issue, simple fix. Hope this helps.

Edit: @jordan, do you think we should set UGens’ channels arrays to be immutable? I kinda think that would be reasonable, but I don’t remember if for instance math operator optimizations depend on mutation here.

hjh

2 Likes
4

Nice work, that looks like it was hard work to track down!

To be honest I’m not too sure about making things immutable, it isn’t a part of the language I’ve really dung into so I don’t know how well it is respected.

I don’t quite understand your analysis at the moment though…

Local in is a multioutugen, which returns from its constructor an array of output proxies (having constructors not return instances of said class is mighty confusing). So when you make the copy, all you are actually doing is copying the channels array of output proxy - ugen and it’s subclasses explicitly disabled copy copy { ^this }.

So I think if we just added ^channels.copy here…

… It would work? This way, the local in keeps it’s original array, but returns something that can be mutated. I’m not 100% on this because it is quite complex code.

We could then mark the channels array as immutable, but the user shouldn’t be able to access it anyway.

I’m away from pc for tla day or two so won’t be able to check for a bit, but thought I should just get this down.

5

Once an object is frozen, immutability is enforced:

a = [0, 1, 2].freeze;
a[1] = 100;
ERROR: Primitive '_BasicPut' failed.
Attempted write to immutable object.

b = Point(1, 2).freeze;
b.x = -3;
ERROR: Object is immutable

There’s no way to unfreeze, so once an object is immutable, there’s no way for any client to override that.

That idea seems to be a non-starter, however, as I’m finding something very strange with Control and initOutputs. If I add channels = channels.freeze; just before the return from initOutputs, I find that initOutputs is getting called twice (?) and somehow it’s not only the array that has been frozen but also the Control unit itself – so the second attempt to assign anything into one of the Control’s variables throws an immutable error.

I dunno… why are we initing twice? That seems wrong, but I’m out of energy for today to look at SynthDef building logic.

That’s exactly the point. The OP wanted to replace the output channels from LocalIn with further calculations based on those channels. This would be perfectly legitimate by populating a new array – instead of feed[0] = ...; feed[1] = ...;, to write feed = LocalIn.ar(2); feed = [feed[0] something, feed[1] something];. (I guess that) OP assumed that these two approaches were equivalent.

There was never any need to copy the UGen itself – the requirement here is to protect the array that the UGen is holding, because the OutputProxies in this array are used to write input specs for downstream units. (The error occurs because one or both LocalIn outputs refer to the results of later calculations, which haven’t happened yet: it should be LocalIn outputs A, which then feeds A → B → C, but now LocalIn’s OutputProxy refers to C.)

^channels.copy should be enough! The OP’s original error disappears by changing initOutputs like this.

hjh

6

Cool!

I don’t know if this will affect any of the other multioutugens though. I’m not too sure how to test this.

I suppose if we could show that this bug held for other classes too, then it would clearly be a bug fix, but if the other classes don’t show this bug, then it it harder to prove that this change does affect them negatively.

Yeah, the constructor logic here is difficult to dig into!

7

The channels array is used in writeOutputSpecs, so it must not be modified! This applies to all MultiOutUGens.

then it it harder to prove that this change does affect them negatively.

I don’t see how returning a copy of the OutputProxy array could cause any issues.

8

I don’t see how returning a copy of the OutputProxy array could cause any issues.

Yes, this would be the easiest and safest.