Potential infinite loop in sound mixer

[ivan-mogilko]

I rewrote this ticket slightly after more investigation. It appears the origin of it is similar to one mentioned in #18: in our program we do not ensure fixed and "even" size of queued buffers, and simply pass as much as decoder gave to us.

While above is our program's flaw, there's a place in mojoAL where it lacks a safety check, and is capable of going into the infinite loop.

For the reference, we experienced this with the following wav file, because SDL_Sound's decoder was retrieving unevenly sized buffers from it:
random6.zip

The location in the mojoAl's code where the infinite loop occurs:

mojoAL/mojoal.c

Lines 1423 to 1430 in bfaf2f2

	while ( (((mixlen = SDL_AudioStreamAvailable(src->stream)) / bufferframesize) < framesneeded) && (src->offset < buffer->len) ) {
	const int framesput = (buffer->len - src->offset) / bufferframesize;
	const int bytesput = SDL_min(framesput, 1024) * bufferframesize;
	FIXME("dynamically adjust frames here?"); /* we hardcode 1024 samples when opening the audio device, too. */
	SDL_AudioStreamPut(src->stream, data, bytesput);
	src->offset += bytesput;
	data += bytesput / sizeof (float);
	}

What causes infinite loop is this:

1. const int framesput = (buffer->len - src->offset) / bufferframesize - in this expression (buffer->len - src->offset) appears to be less than bufferframesize, and this framesput equals zero.
2. because of that src->offset never advances, and while exit condition will be never met.

I suppose that mojoAl could at least have an assertion and/or safety break there, to not hang in that loop.

[icculus]

While above is our program's flaw

I really don't think it is, and where MojoAL has a problem with this, I consider it a MojoAL bug.

[ivan-mogilko]

@icculus I think following line may also fail in case of irregular sized buffer remains:
data += bytesput / sizeof (float);

I ended up with a following temp hotfix for now (hopefully, the math is right...):

const int bytesleft = (buffer->len - src->offset);
const int framesput = (bytesleft + (bufferframesize - 1)) / bufferframesize;
// [HOTFIX] ensure bytesput is positive even if remains are less than bufferframesize
const int bytesput = SDL_min(SDL_min(framesput, 1024) * bufferframesize, bytesleft);
FIXME("dynamically adjust frames here?");  /* we hardcode 1024 samples when opening the audio device, too. */
SDL_AudioStreamPut(src->stream, data, bytesput);
src->offset += bytesput;
data += (bytesput + (sizeof (float) - 1)) / sizeof (float);

But I cannot tell whether this is technically a correct thing to do; as I never worked with SDL audio functions before, so idk if it's okay to pass irregular lengths into SDL_AudioStreamPut, and also what will happen if data gets filled with "incomplete" float element.

[ericoporto]

@icculus this is the only thing we currently have different from mojoAL upstream (this commit), it would be nice if at least the hotfix could be in at least until some better fix is done.