AudioControl outputs stale data #2839

jamshark70 · 2017-04-14T01:03:18Z

If an AudioControl is mapped to a bus, and there is no Out unit actively writing to the bus, the AudioControl outputs the last control buffer's worth of audio indefinitely, producing a noisy oscillation at the frequency ControlRate.ir Hz.

(
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, {
	f = Synth.tail(s.defaultGroup, \fx);
	f.map(\a_in, b);
});

p.stop;
f.free;

In and InFeedback do not have the same problem.

In's next functions check the bus's touched flag, and if the bus wasn't touched, it zeroes its output. InFeedback uses touched and it looks like it's supposed to look back no further than the previous control cycle. (https://github.com/supercollider/supercollider/blob/master/server/plugins/IOUGens.cpp#L684). InFeedback works properly (it ignores stale data) if the synth is writing to a different bus from the one from which InFeedback is reading. I thought InFeedback might be broken, but that's because my initial example did not use a private bus for the source signal. The fxbus synth itself was touching the same bus that InFeedback was using. I've updated the example to avoid this problem.

AudioControl makes no reference to touched at all (https://github.com/supercollider/supercollider/blob/master/server/plugins/IOUGens.cpp#L201) -- seems to have been written without any awareness of "touched" or "untouched" buses at all.

One user impact for me: I have a teaching method that uses JITLib proxies as modular synth components, connecting them by audio bus mapping, e.g., ~osc <>> ~filter <>> ~out. A further option is to replace an oscillator with a Pbind -- but, because of this bug, the Pbind must not play staccato notes, because any AudioControls listening to it will get garbage data in the gaps between the notes. That's an unwelcome limitation, more so because the server has a mechanism to prevent it (but AudioControl doesn't use it).

The text was updated successfully, but these errors were encountered:

jamshark70 · 2017-04-14T02:34:25Z

Updated issue description -- some points were incorrect.

jamshark70 · 2017-04-14T03:07:43Z

And, I had to re-update it. The test case I initially posted was incorrectly written. Fixed now.

Poking through the "touched" logic a bit.

unit->mWorld->mBufCounter appears to be incremented every control cycle, in the server's audio driver. SC_Jack.cpp, SC_CoreAudio.cpp and SC_PortAudio.cpp all have a line like this: for (int i = 0; i < numBufs; ++i, mWorld->mBufCounter++, bufFramePos += bufFrames).
Every audio bus has a touched int32. Out, ReplaceOut and XOut set touched to the current value of unit->mWorld->mBufCounter.
If touched[i] != unit->mWorld->mBufCounter, then no Out unit did anything to the bus in this control cycle.
unit->mWorld->mBufCounter - touched[i] should tell you how many control cycles ago the bus was touched.
InFeedback copies bus data if this difference is either 0 (some synth touched the bus previously within this control cycle) or 1 (some synth touched it in the immediately preceding control cycle only). Older data are replaced by zeroes.
Because AudioControl didn't take this logic into account, it copies whatever is in the bus's buffer, no matter how old it is.

I think the fix should be simple: just copy the good stuff about touched into AudioControl.

(Fun fact: Using a signed int32, sample rate 44.1kHz, and the default block size 64, you have over 865 hours before the counter wraps around into negative numbers: 0x7fffffff * (64/44100) / 60 / 60 = 865.70265437138 -- I'm content not to worry about that limitation.)

Sciss · 2017-04-14T07:38:54Z

Great analysis. I have been struggling with beeps and noises when using n_mapa things.

telephon · 2017-04-14T09:30:55Z

yes, me too!

bagong · 2017-04-14T10:08:51Z

This gets my vote for the issue report of the year!

jamshark70 · 2017-04-14T10:36:50Z

I'll gladly donate the trophy to charity if it's fixed in 3.9 ;)

jamshark70 · 2017-04-16T00:53:13Z

FWIW, I took a quick stab at fixing this, but I succeeded only in crashing the server. I'm not good enough in C++ to fix this myself, but maybe somebody else can see what I did wrong. (At first glance, comparing with InFeedback, there's a "guard" test that I don't understand -- I mean, I can guess a vague idea of its purpose, but how it works or how to translate it into AudioControl, I don't know.)

I'm not submitting this as a PR because it doesn't work ;)

From d824253362c6f41111200ec14f5574b556b5289c Mon Sep 17 00:00:00 2001
From: James Harkins <jamshark70@dewdrop-world.net>
Date: Fri, 14 Apr 2017 17:12:13 +0800
Subject: [PATCH] Plugins: Try to fix AudioControl stale data issue

---
 server/plugins/IOUGens.cpp | 88 +++++++++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 36 deletions(-)

diff --git a/server/plugins/IOUGens.cpp b/server/plugins/IOUGens.cpp
index ad6cdd5..5d81313 100644
--- a/server/plugins/IOUGens.cpp
+++ b/server/plugins/IOUGens.cpp
@@ -68,6 +68,7 @@ struct OffsetOut : public IOUnit
 struct AudioControl : public Unit
 {
 	float *prevVal; // this will have to be a pointer later!
+	int32 *m_busTouched;
 };
 
 const int kMaxLags = 16;
@@ -187,6 +188,8 @@ void Control_Ctor(Unit* unit)
 void AudioControl_next_k(AudioControl *unit, int inNumSamples)
 {
 	uint32 numChannels = unit->mNumOutputs;
+	int32 *touched = unit->m_busTouched;
+	const int32 bufCounter = unit->mWorld->mBufCounter;
 	float *prevVal = unit->prevVal;
 	float **mapin = unit->mParent->mMapControls + unit->mSpecialIndex;
 	for(uint32 i = 0; i < numChannels; ++i, mapin++){
@@ -194,27 +197,32 @@ void AudioControl_next_k(AudioControl *unit, int inNumSamples)
 		int *mapRatep;
 		int mapRate;
 		float nextVal, curVal, valSlope;
+		int diff = bufCounter - touched[i];
 		mapRatep = unit->mParent->mControlRates + unit->mSpecialIndex;
 		mapRate = mapRatep[i];
-		switch (mapRate) {
-			case 0 : {
-				for(int j = 0; j < inNumSamples; j++){
-					out[j] = *mapin[0];
-				}
-			} break;
-			case 1 : {
-				nextVal = *mapin[0];
-				curVal = prevVal[i];
-				valSlope = CALCSLOPE(nextVal, curVal);
-				for(int j = 0; j < inNumSamples; j++){
-					out[j] = curVal; // should be prevVal
-					curVal += valSlope;
-				}
-				unit->prevVal[i] = curVal;
-			} break;
-			case 2 : Copy(inNumSamples, out, *mapin);
-			break;
-		}
+		if(diff != 1 && diff != 0) {
+			Fill(inNumSamples, out, 0.f);
+		} else {
+			switch (mapRate) {
+				case 0 : {
+					for(int j = 0; j < inNumSamples; j++){
+						out[j] = *mapin[0];
+					}
+				} break;
+				case 1 : {
+					nextVal = *mapin[0];
+					curVal = prevVal[i];
+					valSlope = CALCSLOPE(nextVal, curVal);
+					for(int j = 0; j < inNumSamples; j++){
+						out[j] = curVal; // should be prevVal
+						curVal += valSlope;
+					}
+					unit->prevVal[i] = curVal;
+				} break;
+				case 2 : Copy(inNumSamples, out, *mapin);
+				break;
+			}
+       		}
 	}
 }
 
@@ -226,29 +234,37 @@ void AudioControl_next_1(AudioControl *unit, int inNumSamples)
 	int mapRate;
 	float nextVal, curVal, valSlope;
 	float* prevVal;
+	int32 *touched = unit->m_busTouched;
+	const int32 bufCounter = unit->mWorld->mBufCounter;
+
 	prevVal = unit->prevVal;
 	curVal = prevVal[0];
 	mapRatep = unit->mParent->mControlRates + unit->mSpecialIndex;
 	mapRate = mapRatep[0];
 
-	switch (mapRate) {
-		case 0 : {
-			for(int i = 0; i < inNumSamples; i++){
-				out[i] = *mapin[0];
-			}
-		} break;
-		case 1 : {
-			nextVal = *mapin[0];
-			valSlope = CALCSLOPE(nextVal, curVal);
-			for(int i = 0; i < inNumSamples; i++){
-				out[i] = curVal;
-				curVal += valSlope;
+	int diff = bufCounter - *touched;
+	if(diff != 1 && diff != 0) {
+		Fill(inNumSamples, out, 0.f);
+	} else {
+		switch (mapRate) {
+			case 0 : {
+				for(int i = 0; i < inNumSamples; i++){
+					out[i] = *mapin[0];
 				}
-			unit->prevVal[0] = curVal;
-		} break;
-		case 2 :
-		    Copy(inNumSamples, out, *mapin);
-		break;
+			} break;
+			case 1 : {
+				nextVal = *mapin[0];
+				valSlope = CALCSLOPE(nextVal, curVal);
+				for(int i = 0; i < inNumSamples; i++){
+					out[i] = curVal;
+					curVal += valSlope;
+					}
+				unit->prevVal[0] = curVal;
+			} break;
+			case 2 :
+			    Copy(inNumSamples, out, *mapin);
+			break;
+		}
 	}
 
 }
-- 
1.9.1

joshpar · 2017-04-16T00:56:26Z

Hi James, If you can send me a small SC snippet that shows the bug, I'll try to fix it tonight and verify against your test. Thanks, Josh /* Josh Parmenter www.realizedsound.net/josh */

…

On Apr 15, 2017, at 17:53, jamshark70 ***@***.***> wrote: FWIW, I took a quick stab at fixing this, but I succeeded only in crashing the server. I'm not good enough in C++ to fix this myself, but maybe somebody else can see what I did wrong. (At first glance, comparing with InFeedback, there's a "guard" test that I don't understand -- I mean, I can guess a vague idea of its purpose, but how it works or how to translate it into AudioControl, I don't know.) I'm not submitting this as a PR because it doesn't work ;) From d824253362c6f41111200ec14f5574b556b5289c Mon Sep 17 00:00:00 2001 From: James Harkins ***@***.***> Date: Fri, 14 Apr 2017 17:12:13 +0800 Subject: [PATCH] Plugins: Try to fix AudioControl stale data issue --- server/plugins/IOUGens.cpp | 88 +++++++++++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 36 deletions(-) diff --git a/server/plugins/IOUGens.cpp b/server/plugins/IOUGens.cpp index ad6cdd5..5d81313 100644 --- a/server/plugins/IOUGens.cpp +++ b/server/plugins/IOUGens.cpp @@ -68,6 +68,7 @@ struct OffsetOut : public IOUnit struct AudioControl : public Unit { float *prevVal; // this will have to be a pointer later! + int32 *m_busTouched; }; const int kMaxLags = 16; @@ -187,6 +188,8 @@ void Control_Ctor(Unit* unit) void AudioControl_next_k(AudioControl *unit, int inNumSamples) { uint32 numChannels = unit->mNumOutputs; + int32 *touched = unit->m_busTouched; + const int32 bufCounter = unit->mWorld->mBufCounter; float *prevVal = unit->prevVal; float **mapin = unit->mParent->mMapControls + unit->mSpecialIndex; for(uint32 i = 0; i < numChannels; ++i, mapin++){ @@ -194,27 +197,32 @@ void AudioControl_next_k(AudioControl *unit, int inNumSamples) int *mapRatep; int mapRate; float nextVal, curVal, valSlope; + int diff = bufCounter - touched[i]; mapRatep = unit->mParent->mControlRates + unit->mSpecialIndex; mapRate = mapRatep[i]; - switch (mapRate) { - case 0 : { - for(int j = 0; j < inNumSamples; j++){ - out[j] = *mapin[0]; - } - } break; - case 1 : { - nextVal = *mapin[0]; - curVal = prevVal[i]; - valSlope = CALCSLOPE(nextVal, curVal); - for(int j = 0; j < inNumSamples; j++){ - out[j] = curVal; // should be prevVal - curVal += valSlope; - } - unit->prevVal[i] = curVal; - } break; - case 2 : Copy(inNumSamples, out, *mapin); - break; - } + if(diff != 1 && diff != 0) { + Fill(inNumSamples, out, 0.f); + } else { + switch (mapRate) { + case 0 : { + for(int j = 0; j < inNumSamples; j++){ + out[j] = *mapin[0]; + } + } break; + case 1 : { + nextVal = *mapin[0]; + curVal = prevVal[i]; + valSlope = CALCSLOPE(nextVal, curVal); + for(int j = 0; j < inNumSamples; j++){ + out[j] = curVal; // should be prevVal + curVal += valSlope; + } + unit->prevVal[i] = curVal; + } break; + case 2 : Copy(inNumSamples, out, *mapin); + break; + } + } } } @@ -226,29 +234,37 @@ void AudioControl_next_1(AudioControl *unit, int inNumSamples) int mapRate; float nextVal, curVal, valSlope; float* prevVal; + int32 *touched = unit->m_busTouched; + const int32 bufCounter = unit->mWorld->mBufCounter; + prevVal = unit->prevVal; curVal = prevVal[0]; mapRatep = unit->mParent->mControlRates + unit->mSpecialIndex; mapRate = mapRatep[0]; - switch (mapRate) { - case 0 : { - for(int i = 0; i < inNumSamples; i++){ - out[i] = *mapin[0]; - } - } break; - case 1 : { - nextVal = *mapin[0]; - valSlope = CALCSLOPE(nextVal, curVal); - for(int i = 0; i < inNumSamples; i++){ - out[i] = curVal; - curVal += valSlope; + int diff = bufCounter - *touched; + if(diff != 1 && diff != 0) { + Fill(inNumSamples, out, 0.f); + } else { + switch (mapRate) { + case 0 : { + for(int i = 0; i < inNumSamples; i++){ + out[i] = *mapin[0]; } - unit->prevVal[0] = curVal; - } break; - case 2 : - Copy(inNumSamples, out, *mapin); - break; + } break; + case 1 : { + nextVal = *mapin[0]; + valSlope = CALCSLOPE(nextVal, curVal); + for(int i = 0; i < inNumSamples; i++){ + out[i] = curVal; + curVal += valSlope; + } + unit->prevVal[0] = curVal; + } break; + case 2 : + Copy(inNumSamples, out, *mapin); + break; + } } } -- 1.9.1 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

jamshark70 · 2017-04-16T15:42:27Z

This is as briefly as I can reproduce the problem, using the AudioControl shortcut notation \in.ar and using NodeProxy <>> to handle the mapping automagically.

The issue is present with the default SynthDef, but the gentle 300ms linear release makes it very hard to hear, because the last 1.45 ms of the release segment will be, at maximum, (64/44100 / 0.3).ampdb = -46.3 dB -- and, if amp is the default 0.1, it's really -66.3 dB. So I'm providing an alternate SynthDef with a 1 ms release, to make the issue unmistakable to the ear.

s.boot;

(
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.2
);

~player <>> ~out;
)

p.clear; p.pop;

joshpar · 2017-04-17T04:03:06Z

Thanks @jamshark70 - unfortunately, this is a little more complex, but after going down the wrong path for a couple hours I think I know what I need to do now. More tomorrow. Sorry about the delay.

telephon · 2017-04-20T16:12:51Z

no Problem ~ thanks for taking care of it.

brianlheim · 2017-05-07T00:01:06Z

Hey @joshpar, any news on this?

joshpar · 2017-05-07T03:56:33Z

@brianlheim - a bit. The issue is that the buses we are dealing with are NOT zeroed out on each control period. The touched mechanism that is in place for watching for this is also not looked at for audio buses beyond the ones that go to outputs. So either that mechanism needs to be refactored, or the whole audio bus array of floats needs to be zeroed out on each control period. This will definitely inflict a performance hit that I wasn't happy to see. I started to look at updating the touched approach, and altering the AudioControl UGen to see check to see if a bus was touched before reading from it - but this also means altering Out (and it's buddies) to make sure this flag is set when a bus is written to. I wasn't happy with my first approach at this. I'll try to work more on it this week after some deadlines at work are met.

jamshark70 · 2017-05-07T06:55:50Z

Out already sets the flag. Otherwise In wouldn't work.

AFAICS the approach in In is: if the bus has been touched, read from its buffer; otherwise "fill" the UGen's output with 0s.

I'm not clear why AudioControl can't simply imitate the approach that is already working for In. (I'd have done it myself, but I don't understand the details of the server's internal APIs well enough to write the code.)

joshpar · 2017-05-07T16:41:38Z

I may have missed that then - I’ll take another look.

…

On May 6, 2017, at 11:55 PM, jamshark70 ***@***.***> wrote: Out already sets the flag. Otherwise In wouldn't work. AFAICS the approach in In is: if the bus has been touched, read from its buffer; otherwise "fill" the UGen's output with 0s. I'm not clear why AudioControl can't simply imitate the approach that is already working for In. (I'd have done it myself, but I don't understand the details of the server's internal APIs well enough to write the code.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2839 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTrHvJBm8QArfRDvY950tOaHOA2VhSks5r3Wr2gaJpZM4M9XE->.

jamshark70 · 2017-05-07T22:19:01Z

In_next_a:

if (guard.isValid && (touched[i] == bufCounter)) Copy(inNumSamples, out, in);
else Fill(inNumSamples, out, 0.f);

I think the guard is for access to in, so it isn't necessary in the else branch.

Probably the reason why my attempt crashed is because I don't understand how to use the memory guard properly... maybe? So my try ended up as a Frankenstein mashup of AudioControl and In code.

telephon · 2017-06-03T20:37:46Z

@joshpar, did you have a chance to take another look at this? It is kind of a nasty bug (acoustically) and makes the beautiful audio mapping feature feel unnecessarily risky.

telephon · 2017-07-03T22:16:04Z

Maybe related: #2347.

joshpar · 2017-07-29T00:08:04Z

possible fix is here: #3063

joshpar · 2017-07-30T17:27:27Z

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.

brianlheim · 2017-07-30T17:54:53Z

@joshpar I'm confused. Does #3063 need to be merged before closing this?

joshpar · 2017-07-30T17:55:54Z

I thought @telephon merged it? If not, please reopen

joshpar · 2017-07-30T17:56:48Z

ah - my mistake. Formatting changes were merged into the pull - reopening until merge happens

telephon · 2017-07-30T18:58:36Z

@joshpar it seems, the solution isn't fully there yet :).

nhthn · 2017-08-15T19:27:38Z

this is done, right?

joshpar · 2017-08-15T19:32:13Z

@snappizz - yep - sorry about the last comment, I was signed in with my work ID.

jamshark70 added bug comp: server plugins labels Apr 14, 2017

jamshark70 changed the title ~~AudioControl outputs stale data~~ AudioControl and InFeedback output stale data Apr 14, 2017

jamshark70 changed the title ~~AudioControl and InFeedback output stale data~~ AudioControl outputs stale data Apr 14, 2017

brianlheim added this to Important for 3.9 in 3.9 Top Priority Issues Jul 13, 2017

telephon added the severe label Jul 27, 2017

telephon assigned joshpar Jul 27, 2017

telephon mentioned this issue Jul 30, 2017

fixes (possibly?) for AudioControl mapping #3063

Merged

telephon added the fix proposed label Jul 30, 2017

joshpar closed this Jul 30, 2017

joshpar reopened this Jul 30, 2017

jamshark70 mentioned this issue Aug 14, 2017

Plugin api change #3129

Merged

telephon closed this Aug 15, 2017

nhthn moved this from Important for 3.9 to PR merged in 3.9 Top Priority Issues Sep 9, 2017

supercollider / supercollider

AudioControl outputs stale data #2839

AudioControl outputs stale data #2839

jamshark70 commented Apr 14, 2017 •

edited

jamshark70 commented Apr 14, 2017

jamshark70 commented Apr 14, 2017 •

edited

Sciss commented Apr 14, 2017

telephon commented Apr 14, 2017

bagong commented Apr 14, 2017

jamshark70 commented Apr 14, 2017

jamshark70 commented Apr 16, 2017

joshpar commented Apr 16, 2017

jamshark70 commented Apr 16, 2017

joshpar commented Apr 17, 2017

telephon commented Apr 20, 2017

brianlheim commented May 7, 2017

joshpar commented May 7, 2017

jamshark70 commented May 7, 2017

joshpar commented May 7, 2017

jamshark70 commented May 7, 2017

telephon commented Jun 3, 2017

telephon commented Jul 3, 2017

joshpar commented Jul 29, 2017

joshpar commented Jul 30, 2017

brianlheim commented Jul 30, 2017

joshpar commented Jul 30, 2017

joshpar commented Jul 30, 2017

telephon commented Jul 30, 2017

nhthn commented Aug 15, 2017

joshpar commented Aug 15, 2017

supercollider / supercollider

AudioControl outputs stale data #2839

AudioControl outputs stale data #2839

Comments

jamshark70 commented Apr 14, 2017 • edited

jamshark70 commented Apr 14, 2017

jamshark70 commented Apr 14, 2017 • edited

Sciss commented Apr 14, 2017

telephon commented Apr 14, 2017

bagong commented Apr 14, 2017

jamshark70 commented Apr 14, 2017

jamshark70 commented Apr 16, 2017

joshpar commented Apr 16, 2017

jamshark70 commented Apr 16, 2017

joshpar commented Apr 17, 2017

telephon commented Apr 20, 2017

brianlheim commented May 7, 2017

joshpar commented May 7, 2017

jamshark70 commented May 7, 2017

joshpar commented May 7, 2017

jamshark70 commented May 7, 2017

telephon commented Jun 3, 2017

telephon commented Jul 3, 2017

joshpar commented Jul 29, 2017

joshpar commented Jul 30, 2017

brianlheim commented Jul 30, 2017

joshpar commented Jul 30, 2017

joshpar commented Jul 30, 2017

telephon commented Jul 30, 2017

nhthn commented Aug 15, 2017

joshpar commented Aug 15, 2017

jamshark70 commented Apr 14, 2017 •

edited

jamshark70 commented Apr 14, 2017 •

edited