-
Notifications
You must be signed in to change notification settings - Fork 258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Waveout driver #466
Waveout driver #466
Conversation
Looks pretty good so far. However, all those Sleep() calls in delete_*()
are not acceptable. Please review Microsofts example implementation and try
to implement a more deterministic cleanup:
https://docs.microsoft.com/en-us/windows/desktop/directshow/audio-streaming-sample-code
|
I implemented a new way for closing the driver, I hope you will like more than the previous one.
|
While this looks better, pls. review the documentation of waveOutProc callback function. It says:
I.e. calling |
Because waveOutReset() is also known to cause a deadlock if it is called in the same thread of the callback context. It should be used with CALLBACK_THREAD, or with CALLBACK_FUNCTION when there are no buffer playing at the same time (the sample that you linked is using streaming interface for this purpose). |
I thought about calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that I just now after your change fully understand your previous comment. I didn't expect it to be that hard. Anyway, this looks better. Still, I have two questions:
src/drivers/fluid_waveout.c
Outdated
if (dev->hWaveOut != NULL) | ||
{ | ||
waveOutReset(dev->hWaveOut); | ||
waveOutClose(dev->hWaveOut); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you closing waveOut before unpreparing the headers? Shouldn't it be
waveOutUnprepareHeader()
waveOutClose()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I did a copy/paste mistake ^^;
Fixed.
PostThreadMessage(dev->dwThread, WM_CLOSE, 0, 0); | ||
WaitForSingleObject(dev->hThread, INFINITE); | ||
|
||
CloseHandle(dev->hThread); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you quit the thread after waveOut has been closed? This seems wrong. The thread could be preempted during the call to fluid_synth_write_*()
. During that time, waveOut could be closed. When the thread wakes up again, it will call a disposed waveOut instance, which is likely to crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that such call to waveOutWrite will return MMSYSERR_INVALHANDLE.
If the thread is closed before the waveOut handle, I'm a bit feared about what it could happen inside the waveOut functions, because I have no control on them. Perhaps, we could call waveOutReset, close the thread and finally call waveOutClose, probably it should be less risky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the thread is closed before the waveOut handle, I'm a bit feared about what it could happen inside the waveOut functions
I guess it would just keep filling the message queue. Should be unproblematic. WinMidi does it the same way.
Perhaps, we could call waveOutReset, close the thread and finally call waveOutClose
This should be well defined, but could result in a short sequence of audible sound. Think of:
waveOutReset()
// playback stopped, but thread is still busy inside `fluid_synth_write_*()`
waveOutWrite()
// thread finishes
waveOutUnprepareHeader()
waveOutClose()
I was also thinking that perhaps it would be worth to unify the winmidi and waveout drivers in a single source. They will both use the same thread, the MIDI for adding the buffers for SYSEX, the waveout will play the samples with waveOutWrite(). I will try to do some experiments. |
We have separate cmake options and separate |
They are. I just made it. |
In my opinion, letting the waveOut to flush the buffers for few milliseconds is still the best solution
1d5ebce I'm not quite convinced that it's correct to call What's the problem with the previous solution? |
Because the buffer is in the DONE state.
Actually, the problem of previous solution was that the waveOutReset put all buffers in DONE state, but it does not unprepare them. If the waveOutReset is called when the thread is executing for example the fluid_synth_write_s16, then it will play it again, and the later waveOutUnprepareHeader() will fail with WAVERR_STILLPLAYING return value. |
Ofc., I was missing that.
Ok, then let's take the current approach. Would be nice if you could re-test this on Win98, NT4.0 and XP on occasion. |
I just re-tested on XP and Windows 98, no problems found. I have not NT 4.0 at the moment, but I'm confident that it works fine. |
Good, if it works on Win98 it probably works everywhere else. Thanks! |
This is a new driver that adds sound output by using Windows WaveOut APIs.
It is an alternative to DirectSound driver for Windows based platforms.
It has been tested under some Operating Systems, like Windows CE x86 and Windows Mobile 2003 for ARMv5. Those OSs do not support DirectSound at all, so there is no other choice to use WaveOut APIs if you want to get sound output. These platforms also supports only UNICODE functions, I already implemented support for widechars. Windows NT 4.0 supports just DirectSound 3.0a, but I remember that in the past I was never able to make it working properly, so I would suggest to use WaveOut driver also on that system. I tested it also on Windows 98 SE, Windows XP 32bit and Windows 7 64 bits without problems.