allegro audio stream improvements
muhkuh

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!

Peter Wang

Let's start with 4.

muhkuh

OK, I added a patch to the initial post.

Peter Wang

Sorry, it introduces a deadlock in ex_synth for me. I'll try to look deeper some other day if you don't.

Please just attach patches to replies instead of editing the initial post to save confusion.

edit: ok, I looked further. al_set_audio_stream_fragment should be locking the same mutex as _al_voice_update (which calls ->spl_read ... which calls _al_kcm_refill_stream), which is how we're supposed to be preventing the races. It also explains the deadlock, because your patch causes _al_kcm_refill_stream to re-lock the same mutex as _al_voice_update. Not to say there isn't a problem elsewhere.

muhkuh

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.

Peter Wang

Thanks for the patch. I think that should be ok.

muhkuh said:

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.

Hmm. The stream->spl is a fake sample for holding the audio stream fragments. In the mixer parent case, setting the sample position to fragment length should be to simulate reaching the end of the current fragment. Maybe the voice case is wrong for leaving that out?

Quote:

Should the stream reset itself like sample instances do?

Reset to the start? For a proper audio stream we couldn't do that anyway. It was a mistake mixing up audio streams and the automatically-fed audio streams.

Quote:

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.

Right. We do have _al_kcm_get_silence which is supposed to be used.

muhkuh

Thanks.

Reset to the start? For a proper audio stream we couldn't do that anyway. It was a mistake mixing up audio streams and the automatically-fed audio streams.

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

Peter Wang

I agree that it would be better, at least more orthogonal, to pause instead of stop&reset. Between voices and mixers, I think almost all users would connect to a mixer. If we're going to break something, that suggests we break the voice case in favour of the mixer case.

We might need to introduce new methods for pause/resume instead.

I think resetting the sample instance position on 'stop' is not too bad, as many users probably unconsciously expect the next 'play' to start from the beginning. Just a guess.

muhkuh

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

Peter Wang

Thanks. These patches look correct so I've committed them. Tested a bit in wine.

https://www.allegro.cc/files/attachment/608053
https://www.allegro.cc/files/attachment/608054

I didn't look at the other one yet because it's superficially ugly ;) but I will later.

muhkuh

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.

Thomas Fjellstrom

Dude, I have to say, you rock. Thanks for looking into the audio addon. It hasn't seen the love it deserves, until now :)

Peter Wang

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

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

muhkuh said:

How is that supposed to work?

Oops! Clearly it doesn't, at all.

muhkuh

Hi again,

I added a function to retrieve the currently played fragment buffer and the current sample offset into that buffer. This function makes it possible to know exactly what sample is currently being played. I also use it to check if the stream has reached the last sample to stop playback. Otherwise I would have to wait until the last fragment buffer played all of its data even though it has only been partially filled.
I also added some documentation. Unfortunately I havn't been able build the docs on my platform. I donwloaded pandoc for windows but building the html stuff terminates with an error:
Generating html/refman/getting_started.html
..\..\..\git\allegro\docs\scripts\dawk.c:152: error opening file for writing: \s7j8.

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

As of now this should be the last thing I need for our project. But I will start to fix the silence issue.

Thanks

Peter Wang

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.

muhkuh

There are several things I need the exact position in the audio stream for:

  1. Display the exact position in a progress bar.

  2. The last fragment buffer submitted for a stream is often not completely filled with data. But I want to know exactly when the stream reaches the end of the played data to correctly flag it as stopped. So I'm polling the exact position of the stream.

  3. I'm synching video data to audio. I need the exact sample position inside the stream as it becomes the "time source" for the video.

  4. I'm automatically use streamed audio when the amount of samples to play exceeds a certain amount. So I have to make my implementation behave the same with sample instances as it does with audio streams.

The imprecision of the real playback position increases with the amount of buffered data for a stream.

I totally agree that the interface is ugly. I tried to make a simple solution that doesn't require any modification to the audio stream data structure but uses the information already available. But if you are OK with adding stuff to the audio stream struct for this I can try to find something better.

Edit:
Here is my next proposal:
https://www.allegro.cc/files/attachment/608062

I hope I used the correct data type for a 64 bit integer. It is needed because 32bit might not be enough.

Peter Wang

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.

muhkuh

Hi,

I converted the integers to uint64_t as you asked. But I'm not sure about the rest. I meant al_get_audio_stream_played_samples to return the number of samples played by the audio stream since it was last started. I mean "played" in the sense of how many samples have been consumed by the mixer or the voice.

I think for the most basic definition of an audio stream querying the position makes no sense. It's just made for continuously playing data provided by a source. But requesting the number of played samples since playback started is something meaningful. But unfortunately in allegro_audio this basic audio stream is mixed with the higher level concept of a stream that plays a file and supports things as looping and seeking. Some functions only work on self-updating streams. It would be easier if the self-updating streams would be built on top of the audio streams instead of like it is now. But I guess this cannot be changed anymore. (Or can it?)

Calling al_get_audio_stream_played_samples on a self-updating stream still returns some meaningful information. It's simply for how long the stream has played in samples. If you really want a "al_get_audio_stream_position":
What should al_get_audio_stream_position return for a self-updating stream? The sample position relative to the beginning of the streamed file?

Should al_get_audio_stream_position_secs continue to return the feeder position as it does now. IMHO the feeder position isn't very useful. This value will always be somewhat larger than the real position.

Thanks

Elias
muhkuh said:

It would be easier if the self-updating streams would be built on top of the audio streams instead of like it is now. But I guess this cannot be changed anymore. (Or can it?)

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.

Quote:

Calling al_get_audio_stream_played_samples on a self-updating stream still returns some meaningful information.

Yes, I think that makes sense.

muhkuh

OK. I changed the data type to uint64_t. Is this commitable like that?
https://www.allegro.cc/files/attachment/608067

In addition to that I have another small patch. It removes an unnecessary memory allocation when changing the gain of a sample instance. As far as I can see the channel matrix is freed whenever a sample instance is detached from a mixer. Additionally the number of channels of a mixer or a sample instance cannot change for the lifetime of these objects. So it should be possible to reuse the memory of the old matrix when it has to be recomputed.
https://www.allegro.cc/files/attachment/608068

I also have a patch ready that adds an al_fill_silence function to replace the broken al_get_silence+memset solution but I'd like to get the other things committed/discussed first.

Thanks

Peter Wang
muhkuh said:

It's just made for continuously playing data provided by a source. But requesting the number of played samples since playback started is something meaningful.

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

Quote:

What should al_get_audio_stream_position return for a self-updating stream? The sample position relative to the beginning of the streamed file?

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.

Quote:

But I guess this cannot be changed anymore. (Or can it?)

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.

muhkuh

OK. I'll try to do the changes you asked for.

I already worked on a better way to fill buffers with silence. Here is what I have so far. It's still quite untested. I need more time to see if the changes to the alsa and oss backends work correctly. But please have a look at the patch and tell me if the general direction is OK. I thought it might be beneficial to make al_fill_silence extenally available. You need something like this if you want to put silence into the last uploaded fragment of a stream that is only partially filled with data.

Thanks

aureus
muhkuh said:

I'm synching video data to audio. I need the exact sample position inside the stream as it becomes the "time source" for the video.

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:

#SelectExpand
1#include <math.h> 2 3... 4 5/* Function: al_sync_audio_stream_to_stream 6 */ 7bool al_sync_audio_stream_to_stream(ALLEGRO_AUDIO_STREAM *stream, ALLEGRO_AUDIO_STREAM *refStream, double offsetSecs, double loopStart, double loopEnd) 8{ 9 if (stream->seek_feeder && refStream->get_feeder_position && stream->get_feeder_position && stream->spl.mutex == refStream->spl.mutex) { 10 maybe_lock_mutex(stream->spl.mutex); 11 double timeAdjusted = refStream->get_feeder_position(refStream) + offsetSecs; 12 if (al_get_audio_stream_playmode(stream) == ALLEGRO_PLAYMODE_LOOP && loopEnd > loopStart) 13 { 14 timeAdjusted -= loopStart; 15 double loopDuration = loopEnd - loopStart; 16 timeAdjusted -= floor(timeAdjusted / loopDuration) * loopDuration; 17 timeAdjusted += loopStart; 18 } 19 bool ret = stream->seek_feeder(stream, timeAdjusted); 20 maybe_unlock_mutex(stream->spl.mutex); 21 return ret; 22 } 23 24 return false; 25}

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.

muhkuh

@aureus:

could you please describe more detailed what the function is supposed to do? Basically you want to have two audio streams playing in sync and cross fade between them? Like in DJ Hero? How do you use this function? Several times or just one time to sync the streams?

What I do to make sure several streams play in sync is:
-create a separate mixer that is stopped and attach it to the global mixer
-create the streams and attach them to the separate mixer
-start the separate mixer

This will make sure the streams will start exactly at the same time. Otherwise it might happen that the OS makes a context switch between starting the first and the second stream which can introduce some delay under rare conditions.

I'm not using the self updating streams though but provide the sample data myself. Looping and offsets are handled by my code. If one of the synced streams should start later I insert silence at the beginning for the duration of the delay.

Regards

@Peter Wang

I tried really hard to find a way to use the played sample count of a stream to calculate the position for the self-updating streams. There are fundamental problems with that. It seems easy to do at first but there are some things that are hard to solve.

Self updating streams support loops. Currently the loop border can change at any time. It can happen while the stream is already playing and even worse. Please take a look at this code from _al_kcm_feed_stream:

#SelectExpand
1 /* In case it reaches the end of the stream source, stream feeder will 2 * fill the remaining space with silence. If we should loop, rewind the 3 * stream and override the silence with the beginning. 4 * In extreme cases we need to repeat it multiple times. 5 */ 6 while (bytes_written < bytes && 7 stream->spl.loop == _ALLEGRO_PLAYMODE_STREAM_ONEDIR) { 8 size_t bw; 9 al_rewind_audio_stream(stream); 10 maybe_lock_mutex(stream->spl.mutex); 11 bw = stream->feeder(stream, fragment + bytes_written, 12 bytes - bytes_written); 13 bytes_written += bw; 14 maybe_unlock_mutex(stream->spl.mutex); 15 }

The mutex is locked and unlocked several times. Especially for small loops where the while loop might run through several iterations this will allow for the loop bordes to be changed while a single fragment buffer is being filled.

I see no way to use the played_samples or played_sample_buffers counts to reliably get the correct playback position under these circumstances. It should at least be made sure that the loop borders are constant while a fragment buffer is being filled. But this would still require me to store the loop borders for every fragment buffer to be able to get the correct playback position based on the played_samples count. The implementation would be quite messy.

It would be even better to automatically stop and restart the stream if the loop borders change. But this will lead to a change in behavior. Code that relied on setting the loop borders while the stream is running would get different results. The implementation would be much easier but still quite some work.

You could argue that it's not required to get the real playback position in these extreme cases or that it would be OK to just reset the real playback position when reaching the end of a loop. But in this case it would also be OK to use the old implementation of al_get_audio_stream_position_secs as it does the same thing.

Thinking about the whole issue again I still see no real benefit from calling the function al_get_audio_stream_position instead of al_get_played_samples and making it work like you want it to for self-updating streams. For simple audio stream the function would have the wrong name because they don't have the concept of a position. For self-updating streams the function is hard to implement without changing the way these streams behave. It also further increases the interweaving (I'm not sure if this is the right word, hope you understand what I mean) of basic audio streams and higher level self-updating streams that isn't a good thing as we already agreed on. So I'm kindly asking again if it wouldn't be better to just add the function as al_get_audio_stream_played_samples and leave al_get_audio_stream_position_secs as it is?

If you still think al_get_audio_stream_position is the right way to go: What can I change about the way setting loop borders works right now? Can I stop the stream when loop borders are being changed?

Thanks

Peter Wang
muhkuh said:

I already worked on a better way to fill buffers with silence. Here is what I have so far. It's still quite untested.

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.

Quote:

I thought it might be beneficial to make al_fill_silence extenally available. You need something like this if you want to put silence into the last uploaded fragment of a stream that is only partially filled with data.

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.

muhkuh said:

So I'm kindly asking again if it wouldn't be better to just add the function as al_get_audio_stream_played_samples and leave al_get_audio_stream_position_secs as it is?

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!

muhkuh

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.

I wasn't sure about how endianness is treated by allegro. My code assumed that the data has little endian byte order and keeps that byte order no matter what the system uses. I was under the assumption that allegro expects PCM in that format because it's most widely used. I had a look at the wav loaders and it seems like I was wrong. Data is converted to the system's byte order. I'm not sure about casting the buffer to uint16_t* for UINT16 though because of alignment issues. There might be some case where the provided buffer isn't 16 bit aligned. May be this could be a problem on some platforms allegro runs on? I would try to play it safe and assume the memory is unaligned.

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.

Sorry. I don't understand. Do you mean zeroing out in opposite to filling with silence? What should be best written to the partial buffer? Should I make the function private again? Here is an updated version of the patch:
https://www.allegro.cc/files/attachment/608087

muhkuh said:
So I'm kindly asking again if it wouldn't be better to just add the function as al_get_audio_stream_played_samples and leave al_get_audio_stream_position_secs as it is?
Sure, we can do that. Thanks for looking into it.

I tried to create a patch that will add al_get_audio_stream_played_samples as well as al_get_audio_stream_position. The first works on all audio streams. The latter works only for self-updating streams. I tried to make al_get_audio_stream_position work as exactly as possible. I think I managed to get the common uses cases right for looping self-updating audio streams that have an intro, a loop area and an outro. The downside is that this requires some changes:

1. The stream has to remember the loop borders and keep track of the loop count. This is required to calculate the exact position from the number of played samples since the stream started.

2. Changing the loop area, seeking and rewinding the stream causes the stream to stop and restart. I don't see a big problem with this change but it is a change in behavior nonetheless. Previously loop borders could change at any time while playing. Seeking and rewinding should actually be more responsive now as they invalidate all fragment buffers and immediately allow new data to be uploaded by the feeder.

3. When the feeder has to be rewound at the end of the input data to allow seamless looping the stream's mutex will stay locked to prevent any changes to the loop area.

You can judge yourself if this is worth the effort. Doing it that way will make it easier to separate normal streams and self-updating streams in the future. al_get_audio_stream_played_samples can stay where it is and al_get_audio_stream_position can be moved to a new place. al_get_audio_stream_position could even stay internal for the time being.
https://www.allegro.cc/files/attachment/608088

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

I already tried to adopt to your coding style. I'm sorry that I still missed something. I configured my IDE to an indent of 3 with inserted spaces. I also tried to use the K&R style brackets. Could you please point me at the mistakes I made for the new patches?

Thank you for your comments!

Regards

Peter Wang
muhkuh said:

I wasn't sure about how endianness is treated by allegro. My code assumed that the data has little endian byte order and keeps that byte order no matter what the system uses. I was under the assumption that allegro expects PCM in that format because it's most widely used. I had a look at the wav loaders and it seems like I was wrong. Data is converted to the system's byte order. I'm not sure about casting the buffer to uint16_t* for UINT16 though because of alignment issues. There might be some case where the provided buffer isn't 16 bit aligned. May be this could be a problem on some platforms allegro runs on? I would try to play it safe and assume the memory is unaligned.

Nah, I doubt it.

I will commit your patch with some changes (attached). It turns out we misinterpreted the 24-bit formats. Those actually use the lower 24-bits of 32-bit words. The dsound driver change was wrong and caused a crash.

Quote:

Sorry. I don't understand. Do you mean zeroing out in opposite to filling with silence? What should be best written to the partial buffer?

I always assumed it's slightly better to fade from the last sample value down to zero. Not sure it's true.

Will look at your other patch later. (edit: below)

Quote:

Could you please point me at the mistakes I made for the new patches?

Mostly add spaces around operators, and keep the lines within 80 chars unless it's really too hard.

Quote:

I tried to create a patch that will add al_get_audio_stream_played_samples as well as al_get_audio_stream_position. The first works on all audio streams. The latter works only for self-updating streams. I tried to make al_get_audio_stream_position work as exactly as possible.

I think it would be fine when the details are worked out, and the change in behaviour is probably acceptable. On the other hand, is there any reason to make this change if we want to deprecate self-updating streams, and al_get_audio_stream_played_samples exists for your use case?

aureus
muhkuh said:

@aureus:

could you please describe more detailed what the function is supposed to do? Basically you want to have two audio streams playing in sync and cross fade between them? Like in DJ Hero? How do you use this function? Several times or just one time to sync the streams?

What I do to make sure several streams play in sync is:
-create a separate mixer that is stopped and attach it to the global mixer
-create the streams and attach them to the separate mixer
-start the separate mixer

This will make sure the streams will start exactly at the same time. Otherwise it might happen that the OS makes a context switch between starting the first and the second stream which can introduce some delay under rare conditions.

The effect of the function is to set the playback time of the target stream to the current playback time of the reference stream, plus a specified offset, modulo the specified start and end times. If the two streams are on the same mixer and both playing, this will ensure that they are playing exactly in sync with each other.

What you suggested is something I considered, but found impractical for a few reasons: The game I'm working on could potentially have dozens of different music streams playing at various times in the course of a single play session. Your approach would require that I start each stream I want to play at launch, setting its volume to 0 and spending a lot of CPU time keeping its buffers filled so that it can be turned on and remain in sync. This function, on the other hand, will allow me to start a new stream and sync it to a stream that's always playing, like so:

#SelectExpand
1// Not a full-featured sound manager; just for illustrative purposes 2class SoundManager { 3public: 4 SoundManager() { 5 m_mixer = al_get_default_mixer(); 6 m_refStream = al_load_audio_stream("path/to/data/oneMinuteOfSilence.ogg"); 7 al_attach_audio_stream_to_mixer(m_refStream, m_mixer); 8 al_set_audio_stream_playmode(m_refStream, ALLEGRO_PLAYMODE_LOOP); 9 al_set_audio_stream_playing(m_refStream, true); 10 } 11 12 void playStreamSynced(ALLEGRO_AUDIO_STREAM * newStream, double loopStart, double loopEnd) { 13 al_set_audio_stream_gain(newStream, 0.f); 14 al_attach_audio_stream_to_mixer(newStream, m_mixer); 15 al_set_audio_stream_playmode(newStream, ALLEGRO_PLAYMODE_LOOP); 16 al_set_audio_stream_loop_secs(newStream, loopStart, loopEnd); 17 al_set_audio_stream_playing(newStream, true); // start the stream before syncing or it may go out of sync waiting for you to tell it to start. 18 al_sync_audio_stream_to_stream(newStream, m_refStream, 0.0, loopStart, loopEnd); 19 al_set_audio_stream_gain(newStream, 1.f); // should be a fade in here to prevent clipping, but that's beyond the scope of this example. 20 } 21protected: 22 ALLEGRO_MIXER * m_mixer; 23 ALLEGRO_AUDIO_STREAM * m_refStream; 24};

Does this answer your questions?

Thread #613415. Printed from Allegro.cc