Possible enhancements to AudioPlayQueue

Just taking a look at this as it's been pulled in, so wanted to ensure all is still well with dynamic audio objects. Made a few tweaks, and while I was scanning the code found these two nasties:

This one contradicts the API which states "This function may return NULL if no memory is available". Nope, by the look of it, if no memory is available it'll stall the whole application until a block becomes available. Which may be never...
Code:
int16_t * AudioPlayQueue::getBuffer(void)
{
	if (userblock) return userblock->data;
	[COLOR="#FF0000"]while (1) {
		userblock = allocate();
		if (userblock) return userblock->data;
		yield();
	}[/COLOR]
}
Because getBuffer() is called from AudioPlayQueue::play(), that inherits the problem, too, but because play() returns void there's no way for the user to know there's a problem (apart from the stall).

This would appear to stall until the next audio interrupt causes AudioPlayQueue::update to free up a queue space, but in this case doesn't even call yield():
Code:
void AudioPlayQueue::playBuffer(void)
{
	uint32_t h;

	if (!userblock) return;
	h = head + 1;
	if (h >= max_buffers) h = 0;
	[COLOR="#FF0000"]while (tail == h) ;[/COLOR] // wait until space in the queue
	queue[h] = userblock;
	head = h;
	userblock = NULL; 
}

I can see from git Blame that these are both long-standing bugs, not part of your new code.

I'd suggest getBuffer() should behave as documented, and return NULL if no audio block is available.

I'd suggest playBuffer() should return some sort of success/fail indication: it's not 100% clear what, as there are three outcomes:
  1. no userblock allocated, so it wasn't queued / didn't need to be
  2. no queue space available, userblock not queued
  3. userblock queued as requested

Thoughts?

Cheers

Jonathan
 
Yes I quite agree it doesn't seem to be very friendly to the rest of the system, but I added the minimum
extra to allow individual sample queuing, in a way that matched the existing logic. At least you can
specify a smaller max_buffers now as the defaults are quite large.

Changing the function will break existing code out there, so there's that to consider. The "many caveats"
comment in the docs perhaps suggests that its in need of reworking nonetheless.
 
It's pretty hard to see how any functional code could depend on the horrible behaviour seen here!

I may do a branch and PR to see if Paul will contemplate getting a fix in, since he's clearly paying attention to AudioPlayQueue changes at the moment. Unless you want to have a go at it, given that I'm very much a n00b around these parts...
 
Back
Top