Re: audio.c converted from WaitForSingleObject to poll
On Wed, 31 Oct 2001, Simon Britnell wrote:
2) Removes the need to calculate a sleep interval in the wodPlayer main loop, as it loops only when dsp is ready to be written or a command is ready to be received.
Well, it seems like a bad idea to make the player thread enter a busy loop when there isn't enough data to fill the device buffers, don't you think? (Well, though I'd like buffer complete notifications to happen more frequently than they do in Wine's current implementation, making the player thread use 100% CPU isn't the way...)
Hi, I'm writing a multi-track recorder, so this is a problem I've encountered. My solution went like this. Mux in own thread, Player in own thread with circular buffer in the middle. The Mux takes all inputs and multiplexes them into the circular buffer. It blocks when the buffer is full. In the case of wine, I would guess the Mux isn't really needed, just some function that writes into the circular buffer, blocking if its full. The player reads from the buffer, blocks on empty, writes to /dev/dsp otherwise. Also the buffer size is a multiple of the fragment size of the output device. For OSS the output buffer is split into fragments, for SBLive 16 fragments of 4K each. So the player also blocks on the buffer, when there are less than fragment_size bytes available. In this model, there are no timed waits or yields, just blocks and notifies, so happily no spinlock type situations. I have all this written, unfortunately its in Java, but the concept should be translatable to C. I hope this is relevant/useful. Paul Lorenz --- Ove Kaaven <ovehk(a)ping.uio.no> wrote:
On Wed, 31 Oct 2001, Simon Britnell wrote:
2) Removes the need to calculate a sleep interval in the wodPlayer main loop, as it loops only when dsp is ready to be written or a command is ready to be received.
Well, it seems like a bad idea to make the player thread enter a busy loop when there isn't enough data to fill the device buffers, don't you think? (Well, though I'd like buffer complete notifications to happen more frequently than they do in Wine's current implementation, making the player thread use 100% CPU isn't the way...)
__________________________________________________ Do You Yahoo!? Make a great connection at Yahoo! Personals. http://personals.yahoo.com
The Mux takes all inputs and multiplexes them into the circular buffer. It blocks when the buffer is full. In the case of wine, I would guess the Mux isn't really needed, just some function that writes into the circular buffer, blocking if its full. no the Win32 API doesn't behave like this. queueing a buffer for playback is supposed to be non blocking so we shouldn't limit the size of the buffer
Also the buffer size is a multiple of the fragment size of the output device. For OSS the output buffer is split into fragments, for SBLive 16 fragments of 4K each. again, the Win32 API doesn't put any constraints on the size of input buffers, so we need to do the split in order to fill the OSS fragments
A+ -- --------------- Eric Pouech (http://perso.wanadoo.fr/eric.pouech/) "The future will be better tomorrow", Vice President Dan Quayle
Ove Kaaven wrote:
On Wed, 31 Oct 2001, Simon Britnell wrote:
2) Removes the need to calculate a sleep interval in the wodPlayer main loop, as it loops only when dsp is ready to be written or a command is ready to be received.
Well, it seems like a bad idea to make the player thread enter a busy loop when there isn't enough data to fill the device buffers, don't you think? (Well, though I'd like buffer complete notifications to happen more frequently than they do in Wine's current implementation, making the player thread use 100% CPU isn't the way...)
where's the busy wait ? the code blocks on poll for two types on events - /dev/dsp becomes writable (some new fragments can be written) - a new message has been sent so the playback thread is blocked otherwise (unless I underlooked something) A+ -- --------------- Eric Pouech (http://perso.wanadoo.fr/eric.pouech/) "The future will be better tomorrow", Vice President Dan Quayle
On Wed, 31 Oct 2001, eric pouech wrote:
where's the busy wait ? the code blocks on poll for two types on events - /dev/dsp becomes writable (some new fragments can be written)
So, what do you think happens when /dev/dsp is *always* writable (due to insufficient data from the app)? Then it'll never block.
where's the busy wait ? the code blocks on poll for two types on events - /dev/dsp becomes writable (some new fragments can be written)
So, what do you think happens when /dev/dsp is *always* writable (due to insufficient data from the app)? Then it'll never block. you just need to change the condition in poll for the number of fd:s to be looked upon and use 2 only when playing and some wavehdr have been queued but not sent yet to OSS
A+ -- --------------- Eric Pouech (http://perso.wanadoo.fr/eric.pouech/) "The future will be better tomorrow", Vice President Dan Quayle
On Wed, 31 Oct 2001, eric pouech wrote:
where's the busy wait ? the code blocks on poll for two types on events - /dev/dsp becomes writable (some new fragments can be written)
So, what do you think happens when /dev/dsp is *always* writable (due to insufficient data from the app)? Then it'll never block. you just need to change the condition in poll for the number of fd:s to be looked upon and use 2 only when playing and some wavehdr have been queued but not sent yet to OSS
Yes... but the patch should not be applied without doing so, which was my point... but I have concerns that notification accuracy will suffer, too.
Please forgive the length of this missive, I have not the time to write a short one. There are a couple of (serious) bugs in my patch to do with notifications (more latere). It has, however, uncovered a couple of race conditions that applications may trigger to hang the wodPlayer with or without this patch. -- BUSY LOOP -- There is no busy loop because: retval=poll( pollfd.all, wwo->state==WINE_WS_PLAYING), -1 ); only polls the dsp when in the playing state and: if( !wodPlayer_WriteFragments(wwo) ) { /* stop playing if there's nothing to play */ wwo->state = WINE_WS_STOPPED; } Causes the state to become stopped if we run out of things to write. -- BUGS -- Some wave completions will not be notified until something else is played (Doh!). I'm still thinking about this, but the (wrong) easy solution would be to notify on completion of the write instead of completion of the playback. wodGetPosition doesn't keep pace with how much has actually been played. This causes sound to fail in HalfLife. I'd like to remedy this by keeping track of the total written rather than the total played and to return that value less the amount ioctl says is still in the buffer to be played. -- RACE CONDITION -- wodPlayer_Notify (which, as you may guess from the bugs above, is my arch nemesis) triggers a callback into the application. If the application makes any wod calls which wait on the wodPlayer thread to set an event (wodReset,wodRestart or wodPause), the wodPlayer thread hangs waiting for wodPlayer_Notify to return, which is waiting for wodPlayer to SetEvent(wwo->hEvent). Any other threads attempting to wodReset, wodRestart or wodPause will then pile up behind the stalled wodPlayer, potentially hanging the whole program. Looking at the code, it appears at first blush that we may simply remove the SetEvent() and the WaitForSingleObject() calls to remove this race condition. It seems unlikely that this will cause any problems. -- WHERE TO FROM HERE -- I shall put my poll patch to one side and provide two patches against the encumbent audio.c The first patch will remove the SetEvent -> WaitForSingleObject on wodPause,wodReset and wodRestart. The second patch will update wodGetPosition per my above suggestion. I will then re-examine the poll patch/notification timeliness issue to see if I can come up with something reasonable. Questions? Objections? Flames ? :) __________________________________________________ Do You Yahoo!? Make a great connection at Yahoo! Personals. http://personals.yahoo.com
wodGetPosition doesn't keep pace with how much has actually been played. This causes sound to fail in HalfLife. who does it cause to fail ?
Looking at the code, it appears at first blush that we may simply remove the SetEvent() and the WaitForSingleObject() calls to remove this race condition. It seems unlikely that this will cause any problems. well, from a pure semantic point of view, I think we have to wait. for example, for a wodReset, the application would expect all queued wavehdr to be returned (in a notification) before the wodReset returns. not synchronizing, would loose this part
for wodGetPosition, it currently returns the position of data which have been notified to the app (and which has been actually played). What's the exact issue on HL ? A+ -- --------------- Eric Pouech (http://perso.wanadoo.fr/eric.pouech/) "The future will be better tomorrow", Vice President Dan Quayle
--- eric pouech <eric.pouech(a)wanadoo.fr> wrote:
well, from a pure semantic point of view, I think we have to wait.
I've shown (to myself) experimentally that just removing the waits breaks. What I've done instead is reversed my poll patch and added a patch to mmsystem.c which causes application (and possibly some non-application) callbacks to be called from a different thread so that wodPlayer_Notify doesn't ever halt waiting for the application to return (which may in turn be waiting on wodReset,wodRestart, wodClose, wodPause etc). I'll post this to wine-patches in just a moment.
for wodGetPosition, it currently returns the position of data which have been notified to the app (and which has been actually played).
What's the exact issue on HL ?
With the audio.c poll patch I posted, wodPlayer_Notify doesn't update the total bytes played until after sufficient time has elapsed for the dsp to have finished playing it. The absence of more data to send to the dsp may stop the polling loop before all sounds have been played, meaning that the total bytes for a the last sample written may not be updated until the next wave is queued to be written to the dsp. It appears that HL loops checking wodGetPosition until the last sound has been played before writing the next sound. This never happens with my poll patch because of the issue in the previous para. Hence: no sound. I verified this by doing another evil hack which just notified and updated total bytes as soon as they were written. The sound returned. __________________________________________________ Do You Yahoo!? Make a great connection at Yahoo! Personals. http://personals.yahoo.com
What I've done instead is reversed my poll patch and added a patch to mmsystem.c which causes application (and possibly some non-application) callbacks to be called from a different thread so that wodPlayer_Notify doesn't ever halt waiting for the application to return (which may in turn be waiting on wodReset,wodRestart, wodClose, wodPause etc). I'll post this to wine-patches in just a moment. well, I think it's a bit overkill. furthermore, this won't let synchronize that the notification has been done before returning from the call. this stops waiting in notify, but doesn't synchronize at the end (except if you wrap every entry point in winmm/mmsys which might generate a notification with a synchro with this extra thread)
I think we should not use a general event for the synchro in audio.c, but rather create a new event for each time we need to wait (wodReset...) and pass it to the WWO_MSG struct, hence removing the race condition. does the attached patch solves your issue ?
With the audio.c poll patch I posted, wodPlayer_Notify doesn't update the total bytes played until after sufficient time has elapsed for the dsp to have finished playing it. The absence of more data to send to the dsp may stop the polling loop before all sounds have been played, meaning that the total bytes for a the last sample written may not be updated until the next wave is queued to be written to the dsp. It appears that HL loops checking wodGetPosition until the last sound has been played before writing the next sound. This never happens with my poll patch because of the issue in the previous para. Hence: no sound. I verified this by doing another evil hack which just notified and updated total bytes as soon as they were written. The sound returned. ok (I didn't get it was with the poll patch applied) but OTOH, the poll patch can have nasty effects: 1/ let's assume an app uses only two wavehdr:s which cumuled size if smaller than the OSS play ring 2/ the app will send the two wavehdr:s and they will be queued and written to /dev/dsp 3/ if we use the modification to not have the busy wait, then no notification will be ever made (IIRC some Flash apps are doing this). They normally wait for notification to occur in order to resubmit the same wavehdr:s will different values
so, I think the poll patch (even with some modifications) introduces some more evil parts than it fixes (and your deadlock issues don't come from the playback loop) A+ -- --------------- Eric Pouech (http://perso.wanadoo.fr/eric.pouech/) "The future will be better tomorrow", Vice President Dan Quayle Index: dlls/winmm/wineoss/audio.c =================================================================== RCS file: /usr/share/cvs/cvsroot/wine/wine/dlls/winmm/wineoss/audio.c,v retrieving revision 1.44 diff -u -r1.44 audio.c --- dlls/winmm/wineoss/audio.c 2001/08/18 16:09:41 1.44 +++ dlls/winmm/wineoss/audio.c 2001/11/01 10:19:52 @@ -87,6 +87,7 @@ typedef struct { int msg; DWORD param; + HANDLE hEvent; } WWO_MSG; typedef struct { @@ -109,9 +110,9 @@ DWORD dwRemain; /* number of bytes to write to end the current fragment */ /* synchronization stuff */ + HANDLE hStartUpEvent; HANDLE hThread; DWORD dwThreadID; - HANDLE hEvent; #define WWO_RING_BUFFER_SIZE 30 WWO_MSG messages[WWO_RING_BUFFER_SIZE]; int msg_tosave; @@ -522,29 +523,40 @@ } -int wodPlayer_Message(WINE_WAVEOUT *wwo, int msg, DWORD param) +static int wodPlayer_Message(WINE_WAVEOUT *wwo, int msg, DWORD param, BOOL wait) { + HANDLE hEvent; + EnterCriticalSection(&wwo->msg_crst); if ((wwo->msg_tosave == wwo->msg_toget) /* buffer overflow ? */ - && (wwo->messages[wwo->msg_toget].msg)) + && (wwo->messages[wwo->msg_toget].msg)) { ERR("buffer overflow !?\n"); LeaveCriticalSection(&wwo->msg_crst); return 0; } + hEvent = wait ? CreateEventA(NULL, FALSE, FALSE, NULL) : INVALID_HANDLE_VALUE; + wwo->messages[wwo->msg_tosave].msg = msg; wwo->messages[wwo->msg_tosave].param = param; + wwo->messages[wwo->msg_tosave].hEvent = hEvent; + wwo->msg_tosave++; if (wwo->msg_tosave > WWO_RING_BUFFER_SIZE-1) wwo->msg_tosave = 0; LeaveCriticalSection(&wwo->msg_crst); /* signal a new message */ SetEvent(wwo->msg_event); + if (wait) + { + WaitForSingleObject(hEvent, INFINITE); + CloseHandle(hEvent); + } return 1; } -int wodPlayer_RetrieveMessage(WINE_WAVEOUT *wwo, int *msg, DWORD *param) +static int wodPlayer_RetrieveMessage(WINE_WAVEOUT *wwo, int *msg, DWORD *param, HANDLE *hEvent) { EnterCriticalSection(&wwo->msg_crst); @@ -557,6 +569,7 @@ *msg = wwo->messages[wwo->msg_toget].msg; wwo->messages[wwo->msg_toget].msg = 0; *param = wwo->messages[wwo->msg_toget].param; + *hEvent = wwo->messages[wwo->msg_toget].hEvent; wwo->msg_toget++; if (wwo->msg_toget > WWO_RING_BUFFER_SIZE-1) wwo->msg_toget = 0; @@ -642,6 +655,7 @@ DWORD param; DWORD tc; BOOL had_msg; + HANDLE ev; wwo->state = WINE_WS_STOPPED; @@ -652,7 +666,7 @@ wwo->dwPlayedTotal = 0; TRACE("imhere[0]\n"); - SetEvent(wwo->hEvent); + SetEvent(wwo->hStartUpEvent); for (;;) { /* wait for dwSleepTime or an event in thread's queue @@ -684,17 +698,17 @@ TRACE("imhere[2] (q=%p p=%p) tc = %08lx\n", wwo->lpQueuePtr, wwo->lpPlayPtr, GetTickCount()); had_msg = FALSE; - while (wodPlayer_RetrieveMessage(wwo, &msg, ¶m)) { + while (wodPlayer_RetrieveMessage(wwo, &msg, ¶m, &ev)) { had_msg = TRUE; switch (msg) { case WINE_WM_PAUSING: wodPlayer_Reset(wwo, uDevID, FALSE); wwo->state = WINE_WS_PAUSED; - SetEvent(wwo->hEvent); + SetEvent(ev); break; case WINE_WM_RESTARTING: wwo->state = WINE_WS_PLAYING; - SetEvent(wwo->hEvent); + SetEvent(ev); break; case WINE_WM_HEADER: lpWaveHdr = (LPWAVEHDR)param; @@ -711,14 +725,14 @@ break; case WINE_WM_RESETTING: wodPlayer_Reset(wwo, uDevID, TRUE); - SetEvent(wwo->hEvent); + SetEvent(ev); break; case WINE_WM_CLOSING: /* sanity check: this should not happen since the device must have been reset before */ if (wwo->lpQueuePtr || wwo->lpPlayPtr) ERR("out of sync\n"); wwo->hThread = 0; wwo->state = WINE_WS_CLOSED; - SetEvent(wwo->hEvent); + SetEvent(ev); ExitThread(0); /* shouldn't go here */ default: @@ -895,14 +909,15 @@ InitializeCriticalSection(&wwo->msg_crst); if (!(dwFlags & WAVE_DIRECTSOUND)) { - wwo->hEvent = CreateEventA(NULL, FALSE, FALSE, NULL); + wwo->hStartUpEvent = CreateEventA(NULL, FALSE, FALSE, NULL); wwo->hThread = CreateThread(NULL, 0, wodPlayer, (LPVOID)(DWORD)wDevID, 0, &(wwo->dwThreadID)); - WaitForSingleObject(wwo->hEvent, INFINITE); + WaitForSingleObject(wwo->hStartUpEvent, INFINITE); + CloseHandle(wwo->hStartUpEvent); } else { - wwo->hEvent = INVALID_HANDLE_VALUE; wwo->hThread = INVALID_HANDLE_VALUE; wwo->dwThreadID = 0; } + wwo->hStartUpEvent = INVALID_HANDLE_VALUE; TRACE("fd=%d fragmentSize=%ld\n", wwo->unixdev, wwo->dwFragmentSize); @@ -942,10 +957,8 @@ ret = WAVERR_STILLPLAYING; } else { TRACE("imhere[3-close]\n"); - if (wwo->hEvent != INVALID_HANDLE_VALUE) { - wodPlayer_Message(wwo, WINE_WM_CLOSING, 0); - WaitForSingleObject(wwo->hEvent, INFINITE); - CloseHandle(wwo->hEvent); + if (wwo->hThread != INVALID_HANDLE_VALUE) { + wodPlayer_Message(wwo, WINE_WM_CLOSING, 0, TRUE); } if (wwo->mapping) { munmap(wwo->mapping, wwo->maplen); @@ -988,7 +1001,7 @@ lpWaveHdr->lpNext = 0; TRACE("imhere[3-HEADER]\n"); - wodPlayer_Message(&WOutDev[wDevID], WINE_WM_HEADER, (DWORD)lpWaveHdr); + wodPlayer_Message(&WOutDev[wDevID], WINE_WM_HEADER, (DWORD)lpWaveHdr, FALSE); return MMSYSERR_NOERROR; } @@ -1047,8 +1060,7 @@ } TRACE("imhere[3-PAUSING]\n"); - wodPlayer_Message(&WOutDev[wDevID], WINE_WM_PAUSING, 0); - WaitForSingleObject(WOutDev[wDevID].hEvent, INFINITE); + wodPlayer_Message(&WOutDev[wDevID], WINE_WM_PAUSING, 0, TRUE); return MMSYSERR_NOERROR; } @@ -1067,8 +1079,7 @@ if (WOutDev[wDevID].state == WINE_WS_PAUSED) { TRACE("imhere[3-RESTARTING]\n"); - wodPlayer_Message(&WOutDev[wDevID], WINE_WM_RESTARTING, 0); - WaitForSingleObject(WOutDev[wDevID].hEvent, INFINITE); + wodPlayer_Message(&WOutDev[wDevID], WINE_WM_RESTARTING, 0, TRUE); } /* FIXME: is NotifyClient with WOM_DONE right ? (Comet Busters 1.3.3 needs this notification) */ @@ -1095,8 +1106,7 @@ } TRACE("imhere[3-RESET]\n"); - wodPlayer_Message(&WOutDev[wDevID], WINE_WM_RESETTING, 0); - WaitForSingleObject(WOutDev[wDevID].hEvent, INFINITE); + wodPlayer_Message(&WOutDev[wDevID], WINE_WM_RESETTING, 0, TRUE); return MMSYSERR_NOERROR; }
participants (4)
-
eric pouech -
Ove Kaaven -
Paul Lorenz -
Simon Britnell