My first open source contribution was a bug nobody could hear

I am an audio DSP student, and for a long time "contribute to open source" sat on my list next to "learn to swim properly". Something I would clearly do at some point, with no first step attached to it.

This is what the first step turned out to be: a bug in Surge XT, a free synthesizer, where notes started up to 15 milliseconds late and no one had noticed because you cannot hear a delay you have nothing to compare it against.

The pull request is in review as I write this. What follows is the part I would have wanted to read before starting, including the four times I was wrong.

Picking something

I did not go looking for a project. I went looking for an issue I could hold in my head.

The one I picked was titled BLIT oscillators are a bit late. It had three properties I would now look for on purpose:

• A symptom stated as a measurement, not as a feeling. "Late" is checkable. "Sounds weird" is not.
• A small surface. Three files, one function each.
• No product decision inside it. Nobody had to agree on what the feature should be. There was a right answer and the code was not producing it.

Issues that are open for a while are not always hard. Sometimes they are open because they are boring, or because they sit in a corner of the codebase the maintainers rarely touch. That is a good place for a newcomer to be useful.

Reading until the bug fits in one sentence

Surge has an oscillator setting called retrigger. With it on, every note starts at the same point in the waveform. With it off, notes are supposed to start at a random point, so stacked voices do not phase-lock into an artificial-sounding block.

The code did this:

double st = 0.5 * drand * storage->note_to_pitch_inv_tuningctr(detune);
oscstate[i] = st;

I stared at that for a while before the problem clicked, and the click was a vocabulary problem. oscstate is not phase. It is the remaining phase space before the next impulse fires. The oscillator counts it down and emits when it reaches zero.

So with the output buffers freshly cleared and the level tracking at zero, there is nothing to emit until that countdown finishes. The voice is not starting at a random phase. It is silent, and then it starts at phase zero.

That is the whole bug in one sentence: it was a random delay, not a random start phase.

I could not have written the fix before I could write that sentence. Every hour I spent reading instead of typing paid for itself twice.

Measuring before touching anything

I wrote a test that initialised each oscillator 300 times and counted samples until the first non-zero output. Before any fix, in oversampled samples:

Oscillator	MIDI 24	MIDI 60	MIDI 96
Classic	665.6	82.8	10.0
Wavetable	1263.1	167.5	21.7
S&H Noise	655.4	81.5	9.9

It halves per octave, which is what a fixed fraction of a cycle should do. At MIDI 24 that is 7.5 ms off the front of the attack on average, and around 15 ms at worst. On a bass note with a fast attack, that is the difference between a note that lands and a note that arrives.

Those numbers did more for the pull request than the patch did. They turned "this feels off" into something a maintainer could check in thirty seconds.

Three oscillators, three different fixes

The temptation was to write one patch and apply it three times. That was wrong, and working out why was the interesting part.

Classic builds its waveform as a four segment cycle, tracking the current level and a DC slope across each segment. To start mid-cycle you have to replay that bookkeeping up to the segment you are landing in, otherwise the level is wrong and the waveform is wrong from there on.

Wavetable emits pure steps with no DC ramp. So it starts at a random index and lets the first impulse go through the existing sinc convolution. The opening step comes out band limited for free. This one ended up being the cleanest of the three because I did less to it.

S&H Noise holds a random value by construction. There is no level to reconstruct. It draws one and starts partway through the segment holding it.

Same bug, three shapes, because the thing being reconstructed is different in each.

Four times I was wrong

This is the section I would actually read.

1. I ran a subset of the tests

I ran the [dsp] tag. 856,979 assertions, all green. I felt good.

The [osc] tag was red. There was a serious bug in my S&H change: I had copied a pattern from the Classic oscillator that used a first_run flag, but in S&H that flag was set once in the constructor and never cleared, because until then nothing read it. My new code ran on every audio block, forever.

Run the whole suite. A tag is a filter you chose while holding an assumption.

2. I argued with the evidence

When those tests failed, I reasoned that they could not be my fault: my change only affected the first block, and the failing tests measured steady-state frequency. A transient cannot shift a steady-state measurement.

That reasoning was airtight and the conclusion was false. I reverted my S&H change alone, the tests passed, and my elegant argument was worth nothing. The flag was never cleared, so there was no transient. It was every block.

When a measurement disagrees with your reasoning, the measurement is not the thing that needs explaining away.

3. I tested a stale binary

Twice. The build command failed with cmake: command not found, the test runner ran anyway using the previous binary, and I spent time interpreting results from code that no longer existed.

Now I read the first line of build output before I read the last line of test output.

4. My test harness never reached the code

This one is my favourite, because it bit me three times before I recognised the pattern.

In Surge, an oscillator reads its parameters from a scene data block, not from the parameter objects you set. If you set the parameter and forget to push it across, the oscillator runs on zeros, and zeros are usually a valid configuration that produces plausible-looking output.

So I had a measurement showing my sync fix did nothing (the parameter never arrived), and later a correlation test reporting a beautiful 0.9995 on a waveform that was degenerate because two of its four segments had collapsed to zero length.

A test harness is code. It has bugs. When a result surprises you, suspect the harness before you suspect the thing you are measuring.

The review was the best part

I opened the pull request expecting either silence or a nitpick about brace style. Instead the maintainer wrote several paragraphs and found things I had missed:

• The wavetable oscillator recomputes its mipmap level only at the start of a cycle. Starting mid-cycle skipped it, so the first block ran 1137 convolutions where the steady state needs 49. Not audible, but a CPU spike on every note-on.
• My replay loop used > where it needed >=, so a random draw of exactly 1.0 would fall out of the loop with the wrong state. Probability about one in sixteen million, which is roughly once per hour of dense playing.
• The seeding computed the oscillator period without the sync parameter, while the running code includes it. A synced voice started at the wrong rate. At an extreme setting, the first block ran at 48% of the correct rate.

One of his suggestions I did not follow literally, and saying so was fine. He proposed guarding a whole block on an extra condition. Tracing it, that block also increments a sample counter, which would have fired dozens of times in the first block. I split the guard instead and explained why in the reply. He was fine with it. The suggestion was pointing at a real bug, and the specific line was the fastest way to describe where it lived.

The thing I almost shipped without checking

The maintainer asked for a test proving that retrigger-off output is the retrigger-on output shifted in time. I wrote it. It passed. I was pleased with it.

Then I broke the oscillator on purpose to watch the test catch it.

It did not. It passed on broken code.

The reason is that the Classic oscillator resets its level absolutely at the start of each cycle, so a seeding error erases itself within one cycle, and my comparison window started after that. The test was measuring "does it eventually produce the right waveform", which is a real property, and not the one I had claimed.

A test you have never seen fail is a decoration. Break the code and watch it go red before you believe it.

What I would tell myself

• Pick an issue whose symptom is a number.
• Read until you can state the bug in one sentence. If you cannot, you are not ready to fix it.
• Measure first. The before-and-after table is what gets your pull request read.
• Run everything, not the subset that matches your mental model.
• Suspect your harness.
• Break your own test on purpose.
• Write down why you did not follow a suggestion, rather than quietly not following it.

The patch itself is maybe forty lines. Everything else was working out what those forty lines needed to be, and then finding out which parts of my confidence were unearned.

The pull request is surge-synthesizer/surge#8543 if you want to read the full thread. The review is more instructive than the diff.