Clicks and Pops

[hpmax]

See comments on changes in openvario/variod Issue #33.

[kedder]

@hpmax thanks. Please note that comments in #33 will be pretty much inaccessible to anyone. IOW anyone who will want to deal with the codebase will not be able to find them. So it is better to explain the code in the code itself.

I don't see anything wrong with the code, but frankly I don't really understand any of it. The explanation in #33 doesn't help much, as it assumes full understanding of the code and variables, which I don't possess.

[hpmax]

Basically, the audiovario code calls a function called synthesize_vario which fills a buffer to top off the audio stream. The function is passed a pointer to the buffer, the number of words in the buffer, and a value.

The function does the following:

1) if its muted or within the deadband it fills the buffer with 0's.
2) if it's going down it calculates a frequency based on variod.conf data and the value and produces a triangular wave. 3) If it's going up, it does the same thing but it ALSO modulates it with rectangular pulse with a rise and fall time.

My version improves the efficiency a bit, but the main thing it does is it doesn't fill the buffer to the length requested. If fills as much (less than or equal to the requested length) so that the last value filled will be as close to zero as possible.

There is some chance that if you switch into or out of deadband (or mute) or if the buffer underflows that the audio could go directly from full positive or full negative directly to 0. The new code ensures that it always puts an integer (or near integer) number of cycles into the buffer that's copied to the audio stream.

[kedder]

I see, that sounds reasonable. What is the length of the buffer (in seconds)?

[hpmax]

The sample rate and buffer size are both defined in audiovario.h. Sample rate is 44,100 Samples/second, and the buffer is 4096 samples which is 92.8ms long. Both could easily be adjusted. I think I should clarify something though...

The way this works is the "stream_write_cb" function is called and a certain number of bytes (samples*2) are requested. That number can be more or less than the buffer size. If it's less, (and as far as I can tell, it always is) only that number will be filled. I was seeing typical numbers in the 500-1000 sample range. However, you did make me think when you asked this question and there is a bug in my code.

The way the "as-published" code works if more samples are requested than the buffer_size (which I've never actually seen happen), it will call synthesize_vario, and request a 'buffer_size' number of samples. It then writes them to the stream, and keeps repeating the two steps until it has accumulated the total number of samples requested. This caused problems for my version, because it could (and likely would) run into a situation where it ALWAYS has samples remaining and my code won't fill it. So I disabled the capability to do more than a buffer_size worth of samples (without realizing why it was there).

I have created a new version which puts the call to synthesize_vario and the pa_stream_write in a do_while loop which it continues until the number of bytes_remaining is less than the buffer_size.

I also changed it to use memset to write 0's into the buffer. I assume that's better than a for loop... maybe not.

The code compiles but I want to test it before committing it and the PR should definitely not be approved until I have the new code pushed. I'll have it by the weekend at the latest but hopefully I'll get the opportunity to try it within 12 hours.

For reference: 0 m/s is 350Hz, + 120Hz / m/s when climbing. It's 350Hz / (1 + .85 * m/s) when descending. So, 1m/s is 189Hz, 2 m/s is 129Hz, 3m/s is 98.6Hz, 4 m/s is 79.5Hz, 5m/s is 66.66Hz, 10 m/s is 36.84 Hz. In order for my methodology to work, the samples requested must be at least .5 / Frequency, so for 10 m/s it needs to be at least 13.5ms.

I think this should still be fine, but this is where my knowledge of pulseaudio falls down. I'm not sure how it knows how many samples to request. If it's periodically monitoring how many samples pa needs, that should work great. One thing I could do, is have it fill the requested number of samples if the requested number of samples is less than you need to get back to the end of a half-cycle... Or I could have it only do that if the frequency is low.

[hpmax]

Okay, update is tested (or at least it doesn't appear to break anything) and pushed.

[hpmax]

I'd like to make a few more efficiency improvements. I can hold off an wait for this to be accepted though.

[hpmax]

So apparently I was attempting to use tabs to do indents. It appears the original code used spaces. Line 129 was wrong because I accidentally used spaces. Lines 226/229 were wrong because the original code used spaces and I used tabs. Synthesize_vario and stream_write_cb are now entirely tabs. If this is an issue, I can switch everything back to spaces.