I'm using allegro audio streams for a while now. Basically they do the job but there are some things that always bugged me. I changed the allegro source to solve these problems for our project and it's not a big problem maintaining these local changes but maybe they are useful for other people too and could be integrated into allegro:

Note:
I'm not using al_load_audio_stream. I create streams using al_create_audio_stream because I have to create some sample data on the fly. I'm working with the latest unstable release 5.1.7 but I checked with the GIT repository to make sure the issues are still there.

1. Newly created streams cannot be supplied with data immediately.
When I create a new audio stream the stream creates its fragments. Instead of starting with all fragments available to be filled with sample data the stream starts playing the newly created fragment which are zero initialized so they contain silence. I think this is a bug. It looks like the newly created fragments are being put into the wrong list. I fixed this and it works fine for me. I can immediately supply a new stream with sound samples and it can start instantly.

2. It's not possible to get the exact position of the stream.
I keep track of what sample data got written to which stream fragment. I know that the stream is somewhere in the data I provided with the last couple of uploads. I can guess the currently played fragment if I look at the amount of free fragments. I think it would also work to cast a ALLEGRO_AUDIO_STREAM into a ALLEGRO_SAMPLE_INSTANCE because a stream contains a sample instance as its first member. This way I can query the ALLEGRO_SAMPLE currently used and al_get_sample_data should return the pointer to the stream fragment currently played. I could also use al_get_sample_instance_position on the casted stream to get the position inside the played fragment.
This approach partly works but it's ugly and can cause a race condition. This is the problem:
-My thread queries the currently played fragment
-background thread of allegro changes the current fragment
-My thread queries the current sample position inside the fragment but it doesn't fit to the previously queried fragment

I think there is a workaround for this but what's really needed is a function that can return the current fragment and the local position inside this fragment in one call. Something like this:
void *al_get_audio_stream_position(const ALLEGRO_AUDIO_STREAM *stream, int * sample). I have implemented this and it seems to work.

3. It's hard to reset a stream without recreating it completely.
When stopping the stream I want it to be reusable as quickly as possible. This is important for seeking in a track or when the stream has to be restarted quickly. Currently al_set_audio_stream_playing(xxxx, false) just resets the sample position inside the played fragment. What I need is that all fragments are available to be refilled immediately.

4. Race condition in _al_kcm_refill_stream
I had a look at _al_kcm_refill_stream. It seems to change the lists that contain the used and free fragment buffers but there is no mutex being used to prevent concurrent access from the background thread calling _al_kcm_refill_stream and the threads calling al_get_audio_stream_fragment/al_set_audio_stream_fragment. I added code for locking/unlocking stream->spl.mutex and it removed some weird playback problems I had before.
https://www.allegro.cc/files/attachment/608047

I'm hoping for some kind of feedback. May be I'm wrong with some part of my analysis but I think at least 4. should be fixed. I would provide patches for all of these issues but it would be nice if some allegro developer could answer to this first.

Thanks!

OK, I added a patch to the initial post.

I see. I wasn't aware of the fact that the mutex of a sample instance is set by calling _al_kcm_stream_set_mutex. So this means that the mutex is propagated to the audio stream from a "root" voice? In that case the mutex should already be locked when _al_kcm_refill_stream is called...? I didn't have any deadlocks because on my platform all mutexes seem to be recursive. So the playback problems must be elsewhere ... I removed the patch but I cannot reproduce the issues anymore. May be something else fixed it. Sorry for that. Thanks for looking at this.

I continued working on the other points I made. I will post patches tomorrow. Could you please tell me how audio streams are supposed to behave when being stopped? If they are stopped by calling
al_set_audio_stream_playing(..., false), what's supposed to happen? They seem to do different things depending on their parent. If it's a voice the voice is stopped and that's it. If it's a mixer the sample position in a fragment is reset. Should the stream reset itself like sample instances do?

Edit:
So here's the patch for point 1: newly created streams put all fragments into the "pending_bufs" list. This causes the stream to play them right away. Because the fragment buffers are zero initialized silence is played. The patch changes that to put all created fragments into the "used_bufs" list.
https://www.allegro.cc/files/attachment/608050

Thanks

Note:
I think playing the zero initialized fragment buffers will only play silence for signed sample data. When the stream is created with an 8 bit unsigned format or something like that there could be a crack at the beginning of the play back and when the real audio data starts. But initialization with silence at several places in the audio addon is a different topic.

Thanks.

With "reset to start" I meant resetting the fake sample to the beginning. IMHO the current behavior is a little bit confusing:

1. When a sample instance is stopped by al_set_sample_instance_playing(xxxx, false) the position is reset but only if the sample is connected to a mixer. Otherwise the connected voice is stopped. So in the voice case it's like a pause and with a mixer it's like a stop.

2. When an audio stream is stopped by al_set_audio_stream_playing(xxxx, false) something similar happens. In the voice case the voice is stopped. But in the mixer case the fake sample position is reset.

From my point of view sample instances and audio streams should have the same behavior no matter what they are connected to. I think it would be better to just pause the sample instance or stream like it happens in the voice case. This would make it easier to implement a pause feature. Currently to pause a sample instance connected to a mixer you have to stop it and reset the sample position by calling:
unsigned int oldposition=al_get_sample_instance_position(xxxx);
al_set_sample_instance_playing(xxxx, false);
al_set_sample_instance_position(xxxx, oldposition);

This is no big deal but it will not work perfectly. There is a gap between al_get_sample_instance_position and al_set_sample_instance_playing so oldposition might be slightly off.

For audio streams resetting the fake sample position means that when restarting the stream again some already played samples will be played again.

So in an ideal world al_set_sample_instance_playing(xxxx, false) and al_set_audio_stream_playing(xxxx, false) would not reset the (fake) sample instance position at all. Instead there should be some other way to reset the position.
For sample instances this could be calling al_set_sample_instance_position. Fortunately there is also another function called al_stop_sample_instance. Currently it just calls al_set_audio_stream_playing(xxxx, false) but it could reset the sample position to zero.
For audio streams setting the position isn't possible. But it would be possible to add a function called al_stop_audio_stream(...). If it is called the fake sample instance position is reset, the play flag is set to false and all fragment buffer are being made available again. This is actually the same as recreating the stream without having to reallocate the memory for the fragment buffers.

Would it be possible to make such a change in the unstable version? To some extend it would change the way stuff works. I think for audio streams it should not break anything. But for sample instances code that relies on al_set_sample_instance_playing(xxx, false) resetting the sample position would break. An the other hand this code would also break if the user connects the sample instance to a voice instead of a mixer ... and vice versa.

What do you think? If you would rather like to see code I can make a patch that illustrates what I'm asking for.

Thanks

Hello again!

After looking at the source for a couple of hours I'm quite sure the intention of the implementation is to stop sample instances when setting the play flag to false. Even when being attached to a voice the same thing should happen because the stop_voice functions of the various backends are supposed to reset the sample position in the backend specific buffers.

So I created a patch that does the same thing for streams:
https://www.allegro.cc/files/attachment/608052

I also changed the case of an audio stream being attached to a voice. When trying to test this I ran into a couple of issues with the dsound backend though:
1. A direct sound voice cannot be restarted once it has been stopped. The background thread simply won't start again. After fixing this I got crashes in the _dsound_update function. The first time the direct sound buffer is filled the code assumes that enough samples are available in the audio stream to completely fill the buffer. But after my last change (the one you already committed) newly created streams don't start with all their fragment buffers filled with silence anymore making it more likely to trigger the bug. Additionally stream_read in kcm_voice.c doesn't correctly return the number of filled samples when no data is available. So I also fixed these issues:
https://www.allegro.cc/files/attachment/608053

When I tested the changes in the dsound backend I stumbled upon two non critical bugs. The sample position reported by a dsound voice is calculated wrong and the sample position in the direct sound buffer is reset when a voice starts instead of when it stops. Even the comments above the functions tell otherwise. I fixed these issues in a separate patch:
https://www.allegro.cc/files/attachment/608054

I hope the other backends don't have similar issues that pop up now. But you already said that attaching stuff directly to a voice isn't what people usually do.

Thanks

Thanks. I think I also found a bug in the way buffers are filled with silence at various places. Usually something like this is done:

   int silence_value = _al_kcm_get_silence(voice->depth);
   memset(silence, silence_value, buffer_size);

How is that supposed to work? memset converts silence_value to an unsigned char. But for 16 or 24 bit unsigned sample data there is not a single byte value that can be written. This is how al_kcm_get_silence looks like:

/* Returns a silent sample frame. */
int _al_kcm_get_silence(ALLEGRO_AUDIO_DEPTH depth)
{
   switch (depth) {
      case ALLEGRO_AUDIO_DEPTH_UINT8:
         return 0x80;
      case ALLEGRO_AUDIO_DEPTH_INT16:
         return 0x8000;
      case ALLEGRO_AUDIO_DEPTH_INT24:
         return 0x800000;
      default:
         return 0;
   }
}

I think the reason why it currently works is that two bugs cancel each other. The values for ALLEGRO_AUDIO_DEPTH_INT16 and ALLEGRO_AUDIO_DEPTH_INT24 are wrong. Silence for signed data should be zero. But because memset converts these values to zero anyway that's what is written. I don't think memset can be used for unsigned sample data other than ALLEGRO_AUDIO_DEPTH_UINT8. What's required is a function like this:

void _al_kcm_fill_silence(ALLEGRO_AUDIO_DEPTH depth, 
   ALLEGRO_CHANNEL_CONF conf, 
   void * buffer, 
   unsigned int sample_count);

I don't know if ALLEGRO_AUDIO_DEPTH_UINT16 and ALLEGRO_AUDIO_DEPTH_UINT24 are that common though. But I would fix this after you had a look at the remaining patch.

https://www.allegro.cc/files/attachment/608052

Ok, committed with a follow up for a problem I saw while testing it.

Oops! Clearly it doesn't, at all.

I don't like that interface. It implicitly restricts our implementation in the future, and having the user remember and compare pointers isn't very nice. Perhaps we can keep a count of the total number of fragment buffers which have been submitted upstream, or something like that? I'm not completely clear about your use case.

About the documentation issue, maybe tmpnam called by generate_temp_file is returning a filename in the root directory (!), and on modern Windows is not writeable by normal users.

You should use uint64_t. played_fragments might as well be uint64_t. How about "al_get_audio_stream_position" as 'played' is not quite true? It's also analogous to "al_get_sample_instance_position".

Unfortunately(?) there exists "al_get_audio_stream_position_secs" which may return something else entirely. I guess your function doesn't yet operate correctly on the self-updating audio streams created with al_load_audio_stream? Maybe we can align al_get_audio_stream_position and al_get_audio_stream_position_secs then.

Yes, I agree. An ALLEGRO_AUDIO_STREAM should just be what al_create_audio_stream returns and never something else. And it would not have things like seek methods.

al_load_audio_stream should return a class ALLEGRO_AUDIOFILE_STREAM or similar which would represent the audio file streamed from the filesystem, on which things like seeking make sense.

And you could then stream audio data from an ALLEGRO_AUDIOFILE_STREAM to an ALLEGRO_AUDIO_STREAM. There would be a high level function which works like the current al_load_audio_stream just doing that in a background thread - and there could also be an API letting you do it yourself so you get full control over buffering and lag.

But yes, I guess there is no way of doing that in a backwards compatible way now.

Yes, I think that makes sense.

I agree. My quibble was only in the name of the function.

Yes, I think so. And change al_get_audio_stream_position_secs to match al_get_audio_stream_position, just returning different units. I think the change will not be noticeable to most users.

Attached a slightly modified version of your patch to continue from if you want.

We can consider deprecating the current functions in 5.2.x, but I think we need to have a replacement in place.

I committed your other patch, thanks.

I'd find this functionality very useful as well. I have a very kludgey client-side solution to the problem, but I fear that it may be platform-specific and I'm not even 100% sure it's correct. Please let me know if you come up with a solution to this, or if you want to know what I did.

On a related note:
In my local copy of 5.1.7, I added a function to synchronize two audio streams. I've been working on a game in which I want to crossfade different music files while keeping the timing accurate, so I needed something like this.

This is the source for the added function:

In allegro_audio.h:
ALLEGRO_KCM_AUDIO_FUNC(bool, al_sync_audio_stream_to_stream, (ALLEGRO_AUDIO_STREAM *stream, ALLEGRO_AUDIO_STREAM *refStream, double offsetSecs, double loopStart, double loopEnd));

And in kcm_stream.c:

Is this something that might make sense to add to 5.1.x?

Some notes:
- The function only works if the two streams have the same mutex (read: they're on the same mixer).
- Since the loop start and end times are specific to the acodec implementations, I wasn't able to access them directly from the ALLEGRO_AUDIO_STREAM pointer; hence the additional loopStart and loopEnd times. This makes the function call rather awkward as the developer will need to keep track of each stream's loop start and end times. Perhaps those variables should be pulled from the acodec-specific implementations and added ALLEGRO_AUDIO_STREAM.

I would order the parameters as: buf, samples, depth, chan_conf. This matches al_create_sample.

The UINT16 case is written strangely and would be wrong for big-endian. Why not just write 16-bit units? The UINT24 case will probably require an #ifdef for endianness.

Maybe. Users who are able to fill a fragment with non-silence probably don't need much help to fill it with silence Actually, simply zeroing out the end of a partial buffer may result in an audible discontinuity which suggests not to encourage it.

Sure, we can do that. Thanks for looking into it.

PS. Since you will be submitting more patches, I would appreciate if you try to follow our coding style more closely in future. Thanks!