Re: [PATCH] winecoreaudio.drv: Improve underrun handling

[Ken Thomases]

On Jan 27, 2012, at 3:46 PM, Andrew Eikum wrote:
> - if(list_count(&This->queued_bufinfos) == 0){
> -
>    sc = AudioQueueGetCurrentTime(This->aqueue, NULL, &req_time, NULL);

This isn't quite the same as the next start time.  Even if there are no remaining queued buffers, the queue may still be playing some audio from the last of them.  In that case, the queue's current time would not be the actual desired start time of this buffer.  Have you verified that the queue schedules it after whatever audio may be playing?

Also, what about the fact that AudioQueueGetCurrentTime() seems to return an unreliable time when the queue is paused?
> - if(req_time.mFlags & kAudioTimeStampSampleTimeValid){
> -
>    sc = AudioQueueEnqueueBufferWithParameters(This->aqueue,
> -
>            This->public_buffer, 0, NULL, 0, 0, 0, NULL, &req_time, &start_time);
> -
>    if(sc != noErr){
> -
>        OSSpinLockUnlock(&This->lock);
> -
>        WARN("Unable to enqueue buffer: %lx\n", sc);
> -
>        return E_FAIL;
> -
>    }
> - }else{
> -
>    sc = AudioQueueEnqueueBufferWithParameters(This->aqueue,
> -
>            This->public_buffer, 0, NULL, 0, 0, 0, NULL, NULL, &start_time);
> -
>    if(sc != noErr){
> -
>        OSSpinLockUnlock(&This->lock);
> -
>        WARN("Unable to enqueue buffer: %lx\n", sc);
> -
>        return E_FAIL;
> -
>    }

I would avoid duplicating those two branches just to change between &req_time or NULL.  Either use a pointer variable to hold the argument you're going to pass, computed in an "if", or use the conditional ?: operator inline.

-Ken