Convolution2L: fixes #723

[sonoro1234]

Fixes this old issue that made hear clicks in Convolution2L
Correction of unit->m_tempbuf DC and Nyquist uninitialized.

[telephon]

There is another multiplication that looks similar:
https://github.com/sonoro1234/supercollider/blob/5da2096a2098b239db82c33077941ae9ce17901e/server/plugins/Convolution.cpp#L626

Is there a reason it shouldn't be changed, too?

[sonoro1234]

In the first p3 is the intended destination of multiplication in the second is p1
compare
https://github.com/sonoro1234/supercollider/blob/5da2096a2098b239db82c33077941ae9ce17901e/server/plugins/Convolution.cpp#L608
versus
https://github.com/sonoro1234/supercollider/blob/5da2096a2098b239db82c33077941ae9ce17901e/server/plugins/Convolution.cpp#L638

[telephon]

OK, just wanted to make sure.

[sonoro1234]

Shall we commit?

[mossheim]

Unit tests, if possible, would be nice.

[sonoro1234]

Sorry, what does Unit tests means. Can I help with that?

[mossheim]

Sorry, what does Unit tests means.

https://en.wikipedia.org/wiki/Unit_testing

Can I help with that?

Yes, see the tests in testsuite/classlibrary. I think we have some decent examples of UGen unit tests that you could base yours off of, probably https://github.com/supercollider/supercollider/blob/develop/testsuite/classlibrary/TestUGen_Duty.sc is the cleanest IMO.

[sonoro1234]

Perhaps this commit dont deserve a unit test:
You can clearly hear clicks in Convolution2L before the fix. ( Should we use a "click detector" for that?)
No more clicks after the fix ( which is obviously correct taking care of DC and Nyquist frequencies that were previously uninitialized)

[jamshark70]

The point of a unit test is not to verify the current fix. The point is to have an automatic way to tell if any future code changes reintroduce a bug that was already fixed.

[sonoro1234]

I am afraid I wont be able to help with that.

[mossheim]

I am afraid I wont be able to help with that.

Why not? This is something we request of all contributed patches. I would be happy to help answer any questions for unit tests, but simply saying "no" is not acceptable.

[mossheim]

I suppose someone else will have to write the tests then. Or maybe it isn't necessary in this case? It's still unclear to me how well a unit test can be written to target this particular issue. I'm leaving that to those more familiar with this particular issue to decide.

[jamshark70]

I'd estimate, if I wrote this unit test, probably at least an hour, maybe 2. For Victor to write it, it would take more time to get familiar with the unit testing framework.

Apologies in advance, this is touching on some nerves. I should preface by saying some pain is inevitable and I'm deeply impressed by Brian's energy, commitment and willingness to take heat that most of us wouldn't. However I'm concerned about things becoming so strict as to discourage activity.

I get it that we're in the middle of some growing pains as we try to tighten standards for future maintenance, and those are good efforts. It's uncomfortable, though, because (at least for the moment) basically no contribution is good enough. You spend hours tracking down an issue, format your PR, get nitpicky feedback, a bunch of commits later and you think you're done and... "we're gonna need you to spend another 3 hours learning to write, and then writing, a unit test." Um... what?

Now. The nitpicky feedback is necessary or nobody ever realizes that code formatting matters for maintenance. I didn't like getting that kind of feedback but I can accept it, and it's relatively easy to handle.

Unit testing is not so easy. When I wrote that unit test the other week, I looked for documentation but didn't find it. That cost time. But beyond that, being sure you've isolated the right symptom, avoiding false positives, and even just the mechanics of server sync... well, it's easy to say "write a unit test" but I'm an experienced developer here and it took me over an hour and a half for that EnvGen test. And frankly I think that time was wasted because that part of the code will probably never be touched again. It would be better to devise tests around functional specs and find bugs we haven't even thought of yet... but, oh right, we don't have functional specs for anything 😆 so we've had "comprehensive tests for UGen graph building" pending for months, but no plan.

Wow, that turned into a rant... Anyway. Encouraging contributors to write unit tests is good insofar as "the perfect [forward-looking testing plans] shouldn't be the enemy of the good [having tests where there were none]." I think it's great that talking about unit tests is now routine rather than occasional. But I think we are losing some opportunities by diffusing developer energy into possibly useless tests. I would like to see us divert some of the energy of "a unit test for every patch" into more strategic testing.

[mossheim]

@jamshark70

I'm sorry, I don't have time to respond to the rest of your comment at the moment. I will probably reply to it on the mailing list. My time has been short lately because I am in the last few weeks of grad school, preparing for several trips over the next few months, and preparing to move to begin a new job. It is worth longer discussion though; I agree that this has become an issue lately and I apologize for my part in it.

I have a WiP testing guide that I was planning to release soon here: https://gist.github.com/brianlheim/91222d487afa18582c287b0a722ae272. It is hopefully exactly the kind of documentation you were looking for.

Getting into the habit of writing tests will necessarily be difficult at first. If you can take some time to write down why you feel it was wasted time, in a comment on that Gist or in a private/ML message, we can incorporate it into these guidelines so that they encourage effective testing.

[mossheim]

My time has been short lately because I am in the last few weeks of grad school, preparing for several trips over the next few months, and preparing to move to begin a new job.

Which is not to say this is not worth my time. I just don't want you to think I am ignoring you, and also communicate that I'm under a lot of stress at the moment.

[jamshark70]

FWIW, I just tested a reasonably current development branch (without the current PR) against the original reproducer. Issue #723 says you should hear glitching with Convolution2L. I don't hear any glitching, nor do I see anything suspicious in freqscope. As far as I can tell, on my system, the output of Convolution2L is, in this case, indistinguishable from Convolution2.

http://new-supercollider-mailing-lists-forums-use-these.2681727.n2.nabble.com/attachment/7593409/0/lp800HzKernel.aiff

b = Buffer.read(s, "/home/dlm/Downloads/lp800HzKernel.aiff");

(
x = {|kernel, xfade = 1, switch = 0|
    var trig, sig, conv;
    trig = Impulse.kr(1);
    sig = WhiteNoise.ar;
    conv = Select.ar(switch,
        [Convolution2.ar(sig, kernel, trig, 1024),
            Convolution2L.ar(sig, kernel, trig, 1024, xfade)]
    );
	(conv * 0.2).dup
}.play(s, [\kernel, b])
)

x.set(\switch, 1); // glitches (Convolution2L)
x.set(\switch, 0); // glitch-free (Convolution2)

x.release

I will try one other thing, but then I'm out of time for this morning.

[jamshark70]

I'm also not getting any obvious glitching with this one (switching between two kernel buffers).

b = Buffer.alloc(s, 1024, 1);
c = Buffer.alloc(s, 1024, 1);

(
a = {
	var pulse = Impulse.ar(0),
	// you can also try [600, 600] for no timbre change when switching bufs
	// it's totally smooth for me with both UGens
	filters = LPF.ar(pulse, [600, 2400]);
	[b, c].do { |buf, i|
		RecordBuf.ar(filters[i], buf, loop: 0, doneAction: 2)
	};
	Silent.ar(1);
}.play;
)

(
a = { |bufs = #[0, 1], switch = 0|
	var sig = WhiteNoise.ar,
	trig = Impulse.kr(1),
	buf = Demand.kr(trig, 0, Dseq(bufs, inf)),
	frames = BufFrames.ir(bufs[0]),
	c2 = Convolution2.ar(sig, buf, trig, frames),
	// also try xfade = 5 instead of 1 -- again, sounds fine to me
	c2l = Convolution2L.ar(sig, buf, trig, frames, 1);
	sig = Select.ar(switch, [c2, c2l]);
	(sig * 0.2).dup
}.play(args: [bufs: [b, c]]);
)

a.set(\switch, 1);
a.set(\switch, 0);

a.release;

Despite my rant, I don't mind working on a unit test for this one. But, unfortunately, I can't reproduce the original problem 🤦‍♂️ -- so my hands are tied.

Brian, thanks for the comment -- there is certainly no rush, and I totally agree to take that discussion outside of the context of a specific PR.

[sonoro1234]

The only reason I find for not hearing the glitch is that being a problem of uninitialized DC and Nyquist frequencys, depending on the uninitialized value it could be dificult to hear.

https://github.com/supercollider/supercollider/blob/develop/server/plugins/Convolution.cpp#L597

is obviously wrong: The complex multipication of the spectrums is leaving unit->m_tempbuf DC and Nyquist uninitialized

[sonoro1234]

As for the Unit Tests it is not that i am not willing to collaborate, it is more that I dont know what to test specifically. (telling apart the fact that I dont work with sclang)

[telephon]

There is often an alternative to a Unit test: make it clear in the description of the changes (and possibly in the code itself) why there was a problem before and why the change fixes it.

In my understanding, unit tests are particularly useful when they test a broad range of possible expected behaviors so that one gains some confidence during larger development processes that one hasn't overlooked anything. Writing such tests is often motivated by a single fix, but can also be considered an extra and separate step.

[telephon]

@sonoro1234 especially because this commit changes only two lines which are only understandable in context, it would be good if the commit message were a little more detailed.

[jamshark70]

As for the Unit Tests it is not that i am not willing to collaborate, it is more that I dont know what to test specifically

One methodology for deterministic operations is to identify a known correct result for given inputs. Then the test is to do the calculation and check the result against the known value / signal. Should be reasonably straightforward with convolution.

But if you're not using sclang regularly, that will slow you down.

I'm ok to do it, just need a bit of free time (not for a few days).

[chohner]

Any chance of this being merged? I think even without fully understanding the operation it should be obvious that this is essentially a typo. The goal of the operation starting from here:

supercollider/server/plugins/Convolution.cpp

Line 596 in b06fb48

// multiply the dc and nyquist components

is to multiply the complex spectra of p1 and p2, saving the output into p3:

// multiply the dc and nyquist components
p1[0] *= p2[0];
p1[1] *= p2[1];
//complex multiply
for (int i=1; i<numbins; ++i)
{
    float real,imag;
    int realind,imagind;
    realind= 2*i;
    imagind= realind+1;
    real= p1[realind]*p2[realind]- p1[imagind]*p2[imagind];
    imag= p1[realind]*p2[imagind]+ p1[imagind]*p2[realind];
    p3[realind] = real;
    p3[imagind]= imag;
}

This whole section probably copy pasted from an earlier complex multiplication, where p1 and p2 are multiplied and saved into p1, such as here:

supercollider/server/plugins/Convolution.cpp

Line 209 in b06fb48

p1[0] *= p2[0];

//complex multiply time
float * p1= unit->m_fftbuf1;
float * p2= unit->m_fftbuf2;

p1[0] *= p2[0];
p1[1] *= p2[1];

//complex multiply
for (int i=1; i<framesize; ++i) {
    float real,imag;
    int realind,imagind;
    realind= 2*i; imagind= realind+1;
    real= p1[realind]*p2[realind]- p1[imagind]*p2[imagind];
    imag= p1[realind]*p2[imagind]+ p1[imagind]*p2[realind];

    p1[realind] = real;
    p1[imagind]= imag;
}

You can see in the upper code snippet that p3 is written to correctly inside the loop, but the multiplication of p1[0] * p2[0] and p1[1] * p2[1] need to go into p3[0] / p3[1], rather than p1[0] / p1[1]. This PR fixes that.

[sonoro1234]

 I think even without fully understanding the operation...

In your explanation everything is already explained (as it was before in my second comment)
Perhaps only left to say that DC and nyquist are real values, so they are packed in a single complex value by fftw and thats all!!

[chohner]

I know - I just want this in and hope it pays to be a bit more verbose :)

[chohner]

Please find a script attached that clearly reproduces the bug. After convolving with an impulse train, kernel energy should be equal to that of the output.

@jamshark70 (or someone else more familiar with the framework) could maybe use this as a jumping off point for a unit test.

// DC Bug in Convolution2L
// 1. Boot server and allocate kernel & output buffers
(
~frame_size = 2048;
s.waitForBoot({
	b = Buffer.alloc(s, ~frame_size);
	c = Buffer.alloc(s, ~frame_size);
}) ;
)

// 2. Set kernel to zero padded rectified square wave
b.setn(511, Array.fill(1024, 1));

// 3. Convolve impulse train with kernel and write into output buffer
(
{ arg bufnum, kernel, t_trig=0, framesize=2048, fs=48000;
	var impulse_train = Impulse.ar(fs / framesize);
	var conv_result = Convolution2L.ar(impulse_train, kernel, t_trig, framesize);
	BufWr.ar(conv_result, bufnum, Phasor.ar(0, BufRateScale.kr(bufnum), 0, BufFrames.kr(bufnum)));
	0.0 //quiet
}.play(s, args: [\bufnum, c.bufnum, \kernel, b, \fs, s.sampleRate, \framesize, ~frame_size]);
)

// 4. Fetch buffer to compare kernel and output
(
b.loadToFloatArray(action: { arg array; d = array;});
c.loadToFloatArray(action: { arg array; e = array;});
)

// 5. Plot and calculate power
// You can see that DC is removed from the output, dropping the energy by -3 dB
(
var input_power = d.pow(2).mean.sqrt.ampdb; // rect: -3 dB
var output_power = e.pow(2).mean.sqrt.ampdb; // rect:  -6 dB
("Input power:" + input_power.round(0.01) + "dB - Output power:" + output_power.round(0.01) + "dB").postln;
([d, e]).plot(minval:-1, maxval: 1);
)

It should produce the following, broken plot (notice the output shifted down by 0.5):

After the bugfix, the kernel is correctly reproduced without removing DC:

[smrg-lm]

sneaky comment: I believe patcher shouldn't be charged with a whole unit test for typo corrections if the test not trivial, this case looks more like an assertion.

[smrg-lm]

sneak complement: in this case the unit to test wouldn't be the initialization of one parameter.

[m-rest]

Come on guys, get this in. This is a major bug in the convolution plugin and causes terrible audio artifacts.

[mossheim]

The reason this PR took so long to merge was twofold:

1. We have a lot of open PRs and it is not easy to prioritize and get to everything at once
2. Our testing guides/habits on testing were not well formed when this PR was submitted. This PR was collateral damage in the process of working out how stringent our test policy should be

However, because this has been confirmed as working by a couple people already and because of the confusing situation around testing, I have decided to merge this. Ideally though, this is something that would be tested, and we are working on ways to ensure that such tests are easier to put in place in the future, most likely with a very succinct test harness in C++ specifically for testing UGens. That would render any SC test obsolete anyway, so for now it's not a huge deal if we don't have a test for this particular fix.

Sorry for the delay, all, and thanks for your contributions to this discussion. :)

[sonoro1234]

Which is the flow from develop to master?

[mossheim]

We have a document outlaying our git practices on the Wiki - https://github.com/supercollider/supercollider/wiki/git-workflow-and-guidelines

@snappizz - i think we can cherry-pick this to 3.10?