Audio Output Strategies - richardjwild/arctracker GitHub Wiki
The result of the good work so far naturally drew my attention to the write_audio_data function, which was quite messy, mainly as a result of the ugly #ifdef directive that switched in and out the ALSA api call, if it was available:
__int16_t *audio_buffer = mix(channel_buffer, p_module->num_channels);
/* send the data to the audio device */
if (p_api == OSS) {
ssize_t len = write(*(int *)p_ah_ptr, audio_buffer, BUF_SIZE);
if (len == -1) {
perror("audio write");
return (AUDIO_WRITE_ERROR); /* bomb out */
}
} else if (p_api == ALSA) {
#ifdef HAVE_LIBASOUND
snd_pcm_sframes_t err = snd_pcm_writei (*(snd_pcm_t **)p_ah_ptr, audio_buffer, nframes);
if (err != nframes) {
fprintf (stderr, "write to audio interface failed (%s)\n", snd_strerror (err));
return (ALSA_ERROR); /* bomb out */
}
#else
1; /* this should not happen */
#endif
}
I especially liked the /* this should not happen */. Yeah, right.
So I wanted to replace this with some kind of strategy, where the OSS and ALSA output can be switched without this code even knowing about it. But to give this refactoring the best chance of success, first I had to clean up the code around it. What I felt was the first problem was all this calculation concerning BUF_SIZE. Indeed the constant was seemingly being treated in places as the buffer size in bytes, and in other places as the buffer size in frames:
$ grep BUF_SIZE *.c
play_mod.c:long channel_buffer[BUF_SIZE * MAX_CHANNELS];
play_mod.c: allocate_audio_buffer(BUF_SIZE);
play_mod.c: nframes>((BUF_SIZE - bufptr)>>2)?((BUF_SIZE - bufptr)>>2):nframes,
play_mod.c: frames_written = nframes>((BUF_SIZE - bufptr)>>2)?((BUF_SIZE - bufptr)>>2):nframes;
play_mod.c: if (bufptr == BUF_SIZE) {
play_mod.c: ssize_t len = write(*(int *)p_ah_ptr, audio_buffer, BUF_SIZE);
resample.c: resample_buffer = (unsigned char *) allocate_array(BUF_SIZE, sizeof(unsigned char));
resample.c: memset(resample_buffer, 0, BUF_SIZE);
This desperately needed to be made more consistent, so I replaced BUF_SIZE with two new constants AUDIO_BUFFER_SIZE_FRAMES and AUDIO_BUFFER_SIZE_BYTES and modified the code to use these. That done, I then refactored write_audio_data to make it more clear, which mainly involved renaming things and extracting variables to hold calculation results and give them names. This made the function longer, but more clear.
The next thing I wanted to change was this code in write_audio_data:
frames_requested = p_nframes >> 8;
nframes_fraction += p_nframes - (frames_requested << 8);
if (nframes_fraction > 256) {
frames_requested++;
nframes_fraction -= 256;
}
The reason for this code is that the function is being called once per tick, but the number of sample frames per tick is a fraction, not a whole number, so this code was doing some fixed-point arithmetic to keep of the frame fraction. This made the function quite awkward, so I pushed that responsibility up to the calling function so that write_audio_data could simply accept the number of frames requested and handle that. While doing that, I also noticed that the mod_details structure was being passed merely to get the number of channels, so I changed the parameter to accept the number of channels instead. I also renamed the parameters to write_audio_data, because one of the many things I wanted to change in this codebase was to remove all the p_ prefixes to the function parameter names. Finally, I moved the channel buffer offset calculation in the hope this would make it more clear what was going on.
With all this done, I felt like the campsite was tidy enough for me to tackle the big one. I created a new file audio_api.h which effectively defined an interface:
#ifndef INCLUDE_AUDIO_API_H
typedef struct {
void (*write_audio)(__int16_t *audio_buffer);
} audio_api_t;
#define INCLUDE_AUDIO_API_H
#endif
I created a new module a new module oss.c/oss.h which presented a single public function initialise_oss:
#include "audio_api.h"
static int audio_handle;
static int audio_buffer_size_bytes;
static audio_api_t oss_audio_api;
audio_api_t initialise_oss(long p_sample_rate, int audio_buffer_frames);
This function would open the audio device, set it up using code moved from initialise.c, and return an instance of the audio_api_t struct with the write_audio function pointer set pointing at a private function inside oss.c. This was how I intended to implement my strategy pattern approach. I then changed play_module to accept an audio_api_t instance instead of the audio handle, and made it call write_audio to output the contents of the audio buffer. In this way play_mod.c became completely ignorant of the audio api that was being used; I hid the audio api behind an abstraction of my own. It took surprisingly little debugging before I got this change to work (I expected much worse).
With this done, some opportunities for improvement immediately showed themselves. First, I added a member to the audio_api_t structure to hold the audio buffer size, so that I no longer had to refer to the constant everywhere. I also extracted a function to calculate the channel buffer offset and declared the loop counter inline (we have been able to do that since C99 but I did not know that at the time).
Next thing, I created yet another module write_audio.c and moved the write_audio function plus all the other functions related to audio writing into there. Now play_mod.c no longer had any code to do with writing audio.
The sample rate was another thing that would be convenient stored in the audio_api struct, so I moved it there.
Now that the audio writing code was in a module of its own, I could start to start to take advantage of the natural encapsulation that affords you by creating private static variables and treating them a bit like instance variables, avoiding the need to pass them around in function arguments everywhere. I also started extracting functions, inlining them so that there would be no function-call overhead in the compiled code. Continuing in the same vein, the master gain could be stored in a static variable too and therefore could be removed from the write_audio_data parameter list. I then had a fit of renaming things, and did a pretty aggressive refactor of the code in write_audio.c.
When I was done, it looked like this which I was pretty happy with. I wasn't finished, because I also wanted to move gain and stereo adjustment out, but for the moment it would do, because the time had come to prove the strategy concept: I needed to reintroduce support for the ALSA api in a module of its own. Like the OSS refactor effort, this also was surprisingly easy to get working. Having done so, I then split up the code into separate functions that performed the audio device configuration, following as far as possible the same pattern as in oss.c. With a couple of minor changes, I was finished. Now the code for either audio API was entirely encapsulated in its own compilation unit, and I had established a pattern for adding support for more audio APIs later if I so wished.
I was very happy with this piece of work. The only place where code was being switched in and out by #ifdef HAVE_LIBASOUND directives now was inside alsa.c, and was done in such a way that initialise_alsa returned an error "ALSA playback is not available" if it was unavailable and the user requested it. This, I felt, was the right way to handle feature toggles: to fully capture the toggled code in a single block, rather than sprinkling optional code blocks throughout the codebase.