winearts patch

[Eric Pouech]

Jeremy Shaw a écrit :
> Hello,
> 
> This message is largely for Chris Morgan's review, but I thought I
> should open it to any interested parties.
> 
> I have added wave-in support for arts. I also fixed some bugs in the
> waveout code, and made some "improvements". This code seems to work
> really well for me. If no one sees any problems I will submit the
> patch to the patches mailing list.
> 
> Here is a brief description of the changes I made:
> 
> (1) In ARTS_CloseDevice() (now named ARTS_CloseWaveOutDevice()) and
>     wodPlayer_NotifyWait()
> 
> I added 'wwo->sound_buffer = 0' after the HeapFree(). Without it,
> HeapFree() was being called twice on the same buffer, and would crash
> the second time.
Well, I think the bug isn't here but rather in wodPlayer_WriteMaxFrags.
If the buffer is too small:
- we shouldn't destroy the current buffer (and hopefully create a new 
one) but realloc the buffer...
- moreover, destroying the buffer without setting the sound_buffer field 
to NULL is calling for dangling pointers...

> (2) ARTS_AddRingMessage()
> 
> Added the same fix I submitted earlier for wineoss in regards to ring
> buffer resizing.
> 
> (3) wodPlayer_WriteMaxFrags()
> 
> DWORD *bytes, points to the number of bytes the winearts driver thinks
> it should be able to write. However, arts sometimes lies, and says it
> has (for example) 512 bytes available, but when you call arts_write()
> it returns zero bytes written. 
> 
> In that case, I update *bytes to point to 0, otherwise
> wodPlayer_WriteMaxFrags() will keep being called in vain.
correct. It should even be done as soon as we write less bytes than 
expected (content of *byte at upon entry of wodPlayer_WriteMaxFrags)

> (4a) wodPlayer_FeedDSP()
> 
> The old code had the following checks:
> 
>     /* input queue empty and output buffer with no space */
>    if (!wwo->lpPlayPtr && availInQ) {
>          TRACE("Run out of wavehdr:s... flushing\n");
>          wwo->dwPlayedTotal = wwo->dwWrittenTotal;
>          return INFINITE;
>    }
> 
>    if(!availInQ)
>      {
>  	TRACE("no more room, no need to try to feed\n");
> 	return wodPlayer_DSPWait(wwo);
>      }
> 
> I could not understand the purpose of the '&& availInQ' in the first
> check. It seems that if we are out of wavehdrs, then it does not
> matter if there is space in the availInQ or not, because we don't have
> anything to write. So I removed the && availInQ part.
The purpose of the test is to try to always keep the input queue full:
- therefore if we no longer have wave headers to feed the queue with, and
> 
> Looking at it now, I realize that maybe I need to add 'if (!availInQ)'
> before 'wwo->dwPlayedTotal = wwo->dwWrittenTotal'?

IMO, the wwo->dwPlayedTotal = wwo->dwWrittenTotal is wrong in all cases.
If we have no data to write, just way for some new headers to arrive.

> (4b) wodPlayer_FeedDSP()
> 
> In the while loop, I could not figure out the purpose of 'availInQ >
> SPACE_THRESHOLD', so I changed it to 'availInQ'. In reality, the
> SPACE_THRESHOLD check could probably be left in, I am not sure what
> the purpose was in the first place.
Dunno either. Reading Chris response, we need this on OSS (because 
sounds is processed by fragments of fixed size). I don't know how arts 
behaves here... Thrashing it altogether could also work.

A+