Possible inconsistency in the behavior of al_set_sample_instance_playing() ?
Bluebird

[Edit1] See Edit1 (below the original text) for my attempt at solving the problem.

So, I was experimenting with the latest 5.1.8 Allegro libraries, obtained from the repository and compiled in debug mode (I've decided that since that is where all bugfixes are going, I might as well start using it right away). I have discovered that in certain situations, al_set_sample_instance_playing() will return 'false' where I think it should be returning 'true'.

What that means is that code like the following ...

#SelectExpand
1// This is code (almost) directly from my project. 2 3// All necessary initialization here. 4 5/* 6 * Anything here. 7 */ 8 9// 'mSampleInstance' is a smart pointer. 10bool jResult = al_stop_sample_instance(*mSampleInstance); 11assert(jResult); 12assert(!al_get_sample_instance_playing(*mSampleInstance));

... can fail, depending on what is in that "anything here".

I think the example program below will be more descriptive of what I mean. There is also more explanation in the comments ....

#SelectExpand
1 2// This program is intended to demonstrate an inconsistency in the meaning of 3// the return value of al_set_sample_instance_playing(). 4 5#include <allegro5/allegro.h> 6#include <allegro5/allegro_audio.h> 7#include <allegro5/allegro_acodec.h> 8 9#include <iostream> 10#include <cassert> 11 12 13 14int main(int /* argc */, char** /* argv */) 15{ 16 std::cout << "Starting test ..." << std::endl; 17 18 // Initialize Allegro Base. 19 if(!al_init()) 20 { 21 std::cout << "al_init() failed." << std::endl; 22 return -1; 23 } 24 25 // Initialize Allegro Audio add-on. 26 if(!al_install_audio()) 27 { 28 std::cout << "al_install_audio() failed." << std::endl; 29 return -1; 30 } 31 32 // Initialize the Allegro Audio Codec add-on. 33 if(!al_init_acodec_addon()) 34 { 35 std::cout << "al_init_acodec_addon() failed." << std::endl; 36 return -1; 37 } 38 39 assert(al_is_system_installed()); 40 assert(al_is_audio_installed()); 41 42 ALLEGRO_VOICE* pVoice = 0; 43 ALLEGRO_MIXER* pMixer = 0; 44 ALLEGRO_SAMPLE* pSample = 0; 45 ALLEGRO_SAMPLE_INSTANCE* pSampleInstance = 0; 46 47 // Configuration variables. 48 const unsigned int frequency = 44100; 49 const ALLEGRO_CHANNEL_CONF channel_conf = ALLEGRO_CHANNEL_CONF_2; 50 const ALLEGRO_AUDIO_DEPTH audio_depth = ALLEGRO_AUDIO_DEPTH_FLOAT32; 51 52 // Create the voice, mixer, sample and sample instance. 53 pVoice = al_create_voice(frequency, audio_depth, channel_conf); 54 pMixer = al_create_mixer(frequency, audio_depth, channel_conf); 55 56 // Note: I think the sound needs to be a long one (> 20 seconds). I don't 57 // think the problem will appear if the sound is short enough to enter the 58 // 'stopped' state by the time the execution of the main thread reaches the 59 // problem tests. 60 pSample = al_load_sample("./test.ogg"); 61 62 pSampleInstance = al_create_sample_instance(0); 63 64 if(!pVoice) 65 { 66 std::cout << "al_create_voice() failed." << std::endl; 67 return -1; 68 } 69 else if(!pMixer) 70 { 71 std::cout << "al_create_mixer() failed." << std::endl; 72 return -1; 73 } 74 else if(!pSample) 75 { 76 std::cout << "al_load_sample() failed." << std::endl; 77 return -1; 78 } 79 else if(!pSampleInstance) 80 { 81 std::cout << "al_create_sample_instance() failed." << std::endl; 82 return -1; 83 } 84 85 assert(pMixer); 86 assert(pVoice); 87 assert(pSample); 88 assert(pSampleInstance); 89 90 if(!al_set_sample(pSampleInstance, pSample)) 91 { 92 std::cout << "al_set_sample() failed." << std::endl; 93 return -1; 94 } 95 else if(!al_attach_sample_instance_to_mixer(pSampleInstance, pMixer)) 96 { 97 std::cout << "al_attach_sample_instance_to_mixer() failed." << std::endl; 98 return -1; 99 } 100 else if(!al_attach_mixer_to_voice(pMixer, pVoice)) 101 { 102 std::cout << "al_attach_mixer_to_voice() failed." << std::endl; 103 return -1; 104 } 105 106 107 108 // Begin the actual test. 109 110 111 112 bool result = false; 113 114 // Test al_set_sample_instance_playing() with a mixer and data. 115 result = al_set_sample_instance_playing(pSampleInstance, true); 116 assert(result); 117 al_rest(2); 118 119 assert(al_get_sample_instance_playing(pSampleInstance)); 120 result = al_set_sample_instance_playing(pSampleInstance, true); 121 assert(result); 122 al_rest(2); 123 124 assert(al_get_sample_instance_playing(pSampleInstance)); 125 result = al_set_sample_instance_playing(pSampleInstance, false); 126 assert(result); 127 al_rest(2); 128 129 assert(!al_get_sample_instance_playing(pSampleInstance)); 130 result = al_set_sample_instance_playing(pSampleInstance, false); 131 assert(result); 132 al_rest(2); 133 134 // Note: before trying to test al_set_sample_instance_playing() while no mixer 135 // is attached, I first start the sample instance playing again. This test 136 // program will succeed if this code is commented out. I do not know why. 137 result = al_set_sample_instance_playing(pSampleInstance, true); 138 assert(result); 139 al_rest(2); 140 141 // Disconnect the sample instance from the mixer -- note: I do not detach it 142 // from the sample data (notice the commented-out code). 143 { 144 assert(al_get_sample_instance_attached(pSampleInstance)); 145 result = al_detach_sample_instance(pSampleInstance); 146 assert(result); 147 assert(!al_get_sample_instance_attached(pSampleInstance)); 148 149 // Uncommenting this code results in an ASSERT() in 150 // addons/audio/kcm_instance.c:596. Line 594 calls 151 // al_set_sample_instance_playing(spl, false). 152// result = al_set_sample(pSampleInstance, 0); 153// assert(result); 154 } 155 156 assert(!al_get_sample_instance_attached(pSampleInstance)); 157 // The problem begins with this line. It doesn't do what (I think) it should. 158 result = al_set_sample_instance_playing(pSampleInstance, false); 159 160 // This assert(), if uncommented, will fail. Shouldn't calling 161 // al_set_sample_instance_playing(false) always succeed, even when no parent 162 // exists? 163 //assert(result); 164 165 // This assert() will fail because the last call to 166 // al_set_sample_instance_playing(false) does not actually set 'is_playing' to 167 // 'false' for the given sample instance. This is because 168 // al_set_sample_instance_playing() returns early (with a return value of 169 // false) if no parent exists. 170 assert(!al_get_sample_instance_playing(pSampleInstance)); 171 al_rest(2); 172 173 result = al_set_sample_instance_playing(pSampleInstance, true); 174 assert(!result); 175 assert(!al_get_sample_instance_playing(pSampleInstance)); 176 al_rest(2); 177 178 179 // End the actual test. 180 181 182 183 // Clean up. 184 assert(pMixer); 185 assert(pVoice); 186 assert(pSample); 187 assert(pSampleInstance); 188 189 al_destroy_mixer(pMixer); 190 al_destroy_voice(pVoice); 191 al_destroy_sample(pSample); 192 al_destroy_sample_instance(pSampleInstance); 193 194 pMixer = 0; 195 pVoice = 0; 196 pSample = 0; 197 pSampleInstance = 0; 198 199 // Shutdown Allegro Audio add-on; Allegro Base. 200 al_uninstall_audio(); 201 al_uninstall_system(); 202 203 assert(!al_is_system_installed()); 204 assert(!al_is_audio_installed()); 205 206 std::cout << "Test finished." << std::endl; 207 return 0; 208}

Looking at the source code of al_set_sample_instance_playing(), I think at least part of the problem is that, if no parent mixer (or voice) is attached, and this function is called with 'false', then this function will return early (with a return value of 'false'), even if the sample instance is actually playing at the time. That is, 'spl->is_playing' doesn't get set to 'false'. Which causes al_get_sample_instance_playing() to return 'true' :P

#SelectExpand
1/* Function: al_set_sample_instance_playing 2 */ 3bool al_set_sample_instance_playing(ALLEGRO_SAMPLE_INSTANCE *spl, bool val) 4{ 5 ASSERT(spl); 6 7 if (!spl->parent.u.ptr) { 8 _al_set_error(ALLEGRO_INVALID_OBJECT, "Sample has no parent"); 9 return false; 10 } 11 if (!spl->spl_data.buffer.ptr) { 12 _al_set_error(ALLEGRO_INVALID_OBJECT, "Sample has no data"); 13 return false; 14 } 15 16 if (spl->parent.is_voice) { 17 /* parent is voice */ 18 ALLEGRO_VOICE *voice = spl->parent.u.voice; 19 20 return al_set_voice_playing(voice, val); 21 } 22 23 /* parent is mixer */ 24 maybe_lock_mutex(spl->mutex); 25 spl->is_playing = val; 26 if (!val) 27 spl->pos = 0; 28 maybe_unlock_mutex(spl->mutex); 29 return true; 30}

[Edit1]

I have discovered that always calling al_set_sample_instance_playing(splinst, false) before detaching the sample instance from anything seems to reliably fix the apparent problem. This led me to suspect that al_detach_sample_instance() was not doing this itself (although, I think that it should).

So I added the following code to the very end of _al_kcm_detach_from_parent() (taken from al_set_sample_instance_playing(), with a slight modification):

#SelectExpand
1maybe_lock_mutex(spl->mutex); 2spl->is_playing = false; 3spl->pos = 0; 4maybe_unlock_mutex(spl->mutex);

And then I changed the error detection in al_set_sample_instance_playing() to look like this:

#SelectExpand
1 if (!spl->parent.u.ptr) { 2 _al_set_error(ALLEGRO_INVALID_OBJECT, "Sample has no parent"); 3 ASSERT(spl->is_playing == false); 4 5 if(val) return false; 6 else return true; 7 } 8 if (!spl->spl_data.buffer.ptr) { 9 _al_set_error(ALLEGRO_INVALID_OBJECT, "Sample has no data"); 10 ASSERT(spl->is_playing == false); 11 12 if(val) return false; 13 else return true; 14 }

This makes my test program succeed, and doesn't seem to cause my project any problems. I haven't tested any of the examples that come with Allegro yet, though.

I hope this doesn't break something else.

Peter Wang

What if al_set_sample_instance_playing is allowed to change the playing state of the instance, regardless of whether it is attached to a parent or whether it has data This would be consistent with the other instance states (position, speed, gain, pan, playmode) as far as I can tell.

#SelectExpand
1diff --git a/addons/audio/kcm_instance.c b/addons/audio/kcm_instance.c 2index 8094707..737665c 100644 3--- a/addons/audio/kcm_instance.c 4+++ b/addons/audio/kcm_instance.c 5@@ -542,13 +542,9 @@ bool al_set_sample_instance_playing(ALLEGRO_SAMPLE_INSTANCE *spl, bool val) 6 { 7 ASSERT(spl); 8 9- if (!spl->parent.u.ptr) { 10- _al_set_error(ALLEGRO_INVALID_OBJECT, "Sample has no parent"); 11- return false; 12- } 13- if (!spl->spl_data.buffer.ptr) { 14- _al_set_error(ALLEGRO_INVALID_OBJECT, "Sample has no data"); 15- return false; 16+ if (!spl->parent.u.ptr || !spl->spl_data.buffer.ptr) { 17+ spl->is_playing = val; 18+ return true; 19 } 20 21 if (spl->parent.is_voice) {

Bluebird

Yes, that makes sense. It causes the following code to fail at line 3 (and if line 3 is commented out, then it fails at line 4):

#SelectExpand
1assert(!al_get_sample_instance_attached(pSampleInstance)); 2result = al_set_sample_instance_playing(pSampleInstance, true); 3assert(!result); 4assert(!al_get_sample_instance_playing(pSampleInstance));

After experimenting with it for several hours now, I think that actually, the functions in the audio add-on are intended to change as little state per individual function as possible -- am I right? That would mean that user code should actually be written as:

#SelectExpand
1bool play = (/* Some value either true or false. */); 2result = al_set_sample_instance_playing(pSampleInstance, play); 3assert(result); 4assert(al_get_sample_instance_playing(pSampleInstance) == play);

... In order to be correct. And it should be expected to succeed in every case except where the sample instance is attached to a voice (and attaching a sample directly to a voice is not recommended anyway, according to the documentation). That would also mean that setting the 'playing' state to false in _al_kcm_detach_from_parent() (as I originally proposed) is incorrect.

Peter Wang
Bluebird said:

I think that actually, the functions in the audio add-on are intended to change as little state per individual function as possible -- am I right?

That's right.

Thread #613614. Printed from Allegro.cc