[AD] 4.2.0 beta 4 - sound.c bug? - suggested fix within

[Shawn Walker <binarycrusader@xxxxxxxxxx>]

Yes, it's me again ;}

I'm running a game that has this crash right now when starting during
sound initialization using the following code:
 if(install_sound(DIGI_AUTODETECT,MIDI_AUTODETECT,NULL)==0) {
   set_volume(255,255); //maximum to whatever card is, volume controlled in game
 }

Here's the backtrace:
t@xxxxxxxxxx (l@xxxxxxxxxx) program terminated by signal ABRT (Abort)
0xbfd0e057: _lwp_kill+0x0007:   jae      _lwp_kill+0x15 [ 0xbfd0e065, .+0xe ]
Current function is al_assert
  566            abort();
(dbx) where
  [1] _lwp_kill(0x1, 0x6), at 0xbfd0e057
  [2] raise(0x6), at 0xbfcbae3b
  [3] abort(0xbff09006, 0xbfffdc90, 0xbfffb824, 0x0, 0x65737341,
0x66207472), at 0xbfc9e889
=>[4] al_assert(file = 0xbff8ce65 "./src/sound.c", line = 1589), line
566 in "allegro.c"
  [5] voice_set_volume(voice = 0, volume = -1), line 1589 in "sound.c"
  [6] set_volume(digi_volume = 255, midi_volume = 255), line 752 in "sound.c"
  [7] CAllegro::init_allegro(this = 0x808a640), line 47 in "sh_alleg.cpp"
  [8] CAllegro::startup(this = 0x808a640), line 131 in "sh_alleg.cpp"
  [9] main(argc = 1, argv = 0x8046f34), line 63 in "main.cpp"

Here's my analysis:

1) Game calls set_volume(255,255) - from what I can see in the docs,
this is legitimate

2) Allegro library function set_volume calls voice_get_volume for each
voice, voice_get_volume returns -1 because the current volume cannot
be determined for the current voice (because it has finished or been
preempted by a different sound). So, -1 is a legitimate return value
for voice_get_volume according to the source code comments.

3) Allegro library function set_volume then calls voice_set_volume for
each voice

4) Allegro library function voice_set_volume fails with an ASSERT
error because volume is < 0 (-1 in this case).

My conclusion:

Allegro's voice_set_volume should not ASSERT(volume >= 0 && volume <=
255), since Allegro's *own* voice_get_volume function *can*
legitimately return -1. Instead, it should:

ASSERT(volume >= -1 && volume <= 255)

if (volume == -1)
  return;

This change seems to work without ill effect, but I leave it to the
experts what to do.

Right now if you're *not* using the debugging library it takes the
volume of -1, and continues to work. So it seems that this value
doesn't hurt anything...but at the same time it doesn't make a whole
lot of sense to set voices to -1 volume :}

Thoughts? :)

-- 
Shawn Walker, Software and Systems Analyst
binarycrusader@xxxxxxxxxx - http://binarycrusader.blogspot.com/