[PATCH v4 1/2] winmm: Test that a callback passed to `waveOutOpen` can call `waveOutWrite` without getting called reentrantly.

[Liam Murphy]

This behaviour is relied upon by Teardown, see https://github.com/ValveSoftware/Proton/issues/4332.

Signed-off-by: Liam Murphy 
---
v2: Add `todo_wine` to avoid test failure
---
v3: Use block comments instead of line comments
---
v4: Simplify test
---
 dlls/winmm/tests/wave.c | 160 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 160 insertions(+)

diff --git a/dlls/winmm/tests/wave.c b/dlls/winmm/tests/wave.c
index 2f0c8443363..83e83a0150e 100644
--- a/dlls/winmm/tests/wave.c
+++ b/dlls/winmm/tests/wave.c
@@ -865,6 +865,164 @@ EXIT:
     HeapFree(GetProcessHeap(), 0, frags);
 }

+struct reentrancy_callback_data {
+    HANDLE hevent;
+    /* Which thread is currently running the callback, or 0 if it isn't running. */
+    DWORD running_thread;
+    /* How many times the callback's been called. */
+    BOOL call_num;
+};
+
+static void CALLBACK reentrancy_callback_func(HWAVEOUT hwo, UINT uMsg,
+                                   DWORD_PTR dwInstance,
+                                   DWORD_PTR dwParam1, DWORD_PTR dwParam2)
+{
+    struct reentrancy_callback_data *data = (struct reentrancy_callback_data *)dwInstance;
+
+    data->call_num += 1;
+
+    todo_wine_if(data->call_num == 3)
+    ok(data->running_thread != GetCurrentThreadId(), "winmm callback called reentrantly, with message %u\n", uMsg);
+    if (data->running_thread) {
+        if (data->running_thread != GetCurrentThreadId()) trace("Callback running on two threads simultaneously\n");
+
+        /* Don't mess with what the original call was doing. */
+        return;
+    }
+    data->running_thread = GetCurrentThreadId();
+
+    /* We have to wait for a `WOM_DONE` event rather than just using the `WOM_OPEN` event
+     * so that we're running on the winmm audio thread, so that we can block it
+     * to prevent the next `WOM_DONE` event just getting fired in the background
+     * (and then presumably calling the callback on another thread?) */
+    if (uMsg == WOM_DONE) {
+        if (data->call_num == 2) {
+            MMRESULT rc;
+            /* Reuse the header which just finished. */
+            WAVEHDR *hdr = (WAVEHDR *)dwParam1;
+
+            /* Each frame is supposed to last 100ms, so 150ms should be enough to guarantee that the last one has finished. */
+            Sleep(150);
+
+            /* This is needed to trigger the bug in wine,
+             * since winmm checks for `WOM_DONE` within `WINMM_PushData`, which this calls. */
+            rc = waveOutWrite(hwo, hdr, sizeof(*hdr));
+            ok(rc==MMSYSERR_NOERROR,"waveOutWrite(): rc=%s\n", wave_out_error(rc));
+        }
+    }
+
+    data->running_thread = 0;
+    /* Only do this at the end of the function so that it isn't possible for the main thread
+     * to mistakenly call `waveOutClose` while the test is running. */
+    SetEvent(data->hevent);
+}
+
+/* Test whether the callback gets called reentrantly when `waveOutWrite` is called
+ * from within the callback passed to `waveOutOpen`. */
+static void wave_out_test_no_reentrancy(int device)
+{
+    HWAVEOUT wout;
+    HANDLE hevent = CreateEventW(NULL, FALSE, FALSE, NULL);
+    WAVEHDR *frags = 0;
+    MMRESULT rc;
+    int headers = 2;
+    int loops = 0;
+    WAVEFORMATEX format;
+    WAVEFORMATEX* pwfx = &format;
+    DWORD flags = CALLBACK_FUNCTION;
+    double duration = 0.2;
+    DWORD_PTR callback = 0;
+    struct reentrancy_callback_data callback_data;
+    DWORD_PTR callback_instance = 0;
+    char * buffer;
+    DWORD length;
+    DWORD frag_length;
+    int i;
+    BOOL done;
+
+    format.wFormatTag=WAVE_FORMAT_PCM;
+    format.nChannels=1;
+    format.wBitsPerSample=8;
+    format.nSamplesPerSec=22050;
+    format.nBlockAlign=format.nChannels*format.wBitsPerSample/8;
+    format.nAvgBytesPerSec=format.nSamplesPerSec*format.nBlockAlign;
+    format.cbSize=0;
+
+    callback = (DWORD_PTR)reentrancy_callback_func;
+    callback_data.hevent = hevent;
+    callback_data.running_thread = 0;
+    callback_data.call_num = 0;
+    callback_instance = (DWORD_PTR)(&callback_data);
+
+    wout=NULL;
+    rc=waveOutOpen(&wout,device,pwfx,callback,callback_instance,flags);
+    if (rc!=MMSYSERR_NOERROR) {
+        trace("`waveOutOpen` failed with rc %s, skipping", mmsys_error(rc));
+        goto EXIT;
+    }
+
+    rc=WaitForSingleObject(hevent,9000);
+    ok(rc==WAIT_OBJECT_0, "missing WOM_OPEN notification\n");
+
+    frags = HeapAlloc(GetProcessHeap(), 0, headers * sizeof(WAVEHDR));
+
+    buffer=wave_generate_silence(pwfx,duration / (loops + 1),&length);
+
+    /* make sure fragment length is a multiple of block size */
+    frag_length = ((length / headers) / pwfx->nBlockAlign) * pwfx->nBlockAlign;
+
+    for (i = 0; i < headers; i++) {
+        frags[i].lpData=buffer + (i * frag_length);
+        if (i != (headers-1))
+            frags[i].dwBufferLength=frag_length;
+        else {
+            /* use remainder of buffer for last fragment */
+            frags[i].dwBufferLength=length - (i * frag_length);
+        }
+        frags[i].dwFlags=0;
+        frags[i].dwLoops=0;
+        rc=waveOutPrepareHeader(wout, &frags[i], sizeof(frags[0]));
+        ok(rc==MMSYSERR_NOERROR,
+           "waveOutPrepareHeader(%s): rc=%s\n",dev_name(device),wave_out_error(rc));
+    }
+
+    if (rc==MMSYSERR_NOERROR) {
+        /* Queue up the initial fragments. */
+        rc=waveOutWrite(wout, &frags[0], sizeof(frags[0]));
+        ok(rc==MMSYSERR_NOERROR,"waveOutWrite(%s): rc=%s\n",
+           dev_name(device),wave_out_error(rc));
+
+        rc=waveOutWrite(wout, &frags[1], sizeof(frags[0]));
+        ok(rc==MMSYSERR_NOERROR,"waveOutWrite(%s): rc=%s\n",
+           dev_name(device),wave_out_error(rc));
+    }
+
+    done = FALSE;
+    while (!done) {
+        rc=WaitForSingleObject(hevent,8000);
+        ok(rc==WAIT_OBJECT_0, "missing WOM_DONE notification\n");
+
+        done = TRUE;
+        for (i = 0; i < headers; i++) {
+            if (!(frags[i].dwFlags & WHDR_DONE)) {
+                done = FALSE;
+            }
+        }
+    }
+
+    /* Calling `waveOutClose` from within the callback hangs on Windows, so we have to make sure to do it here instead. */
+    rc = waveOutClose(wout);
+    ok(rc==MMSYSERR_NOERROR,"waveOutClose(): rc=%s\n", wave_out_error(rc));
+
+    rc=WaitForSingleObject(hevent,1500);
+    ok(rc==WAIT_OBJECT_0, "missing WOM_CLOSE notification\n");
+
+    HeapFree(GetProcessHeap(), 0, buffer);
+EXIT:
+    CloseHandle(hevent);
+    HeapFree(GetProcessHeap(), 0, frags);
+}
+
 static void wave_out_test_device(UINT_PTR device)
 {
     WAVEOUTCAPSA capsA;
@@ -984,6 +1142,8 @@ static void wave_out_test_device(UINT_PTR device)
     trace("     %s\n",wave_out_caps(capsA.dwSupport));
     HeapFree(GetProcessHeap(), 0, nameA);

+    wave_out_test_no_reentrancy(device);
+
     if (winetest_interactive && (device != WAVE_MAPPER))
     {
         trace("Playing a 5 seconds reference tone.\n");

[PATCH v4 2/2] winmm: Don't do anything in `WINMM_BeginPlaying` if the device is already playing

[Liam Murphy]

This stops wave output devices' callbacks from being called reentrantly when they call `waveOutWrite`.

It was occuring because `waveOutWrite` calls `WINMM_BeginPlaying`, which calls `WOD_PushData`, which checks for completed audio frames and calls the callback to notify it if there are any.

By only calling `WOD_PushData` if the device isn't already playing, this effectively eliminates the problem. The callback will only be called when a frame is completed, so for it to be called when the device is stopped, it must have been the completion of the last frame in the queue. However, that means that no more `WOM_DONE` events are occuring which could trigger the callback.

If it's called on the completion of the second-last frame, `device->stopped` can't be set by calling `waveOutWrite`, since it's updated within `WOD_PushData`, which won't get called if the device isn't stopped.

It is still possible to trigger the callback reentrantly by other means (namely, by using `waveOutRestart`), but the desired behaviour is only that it can't be called reentrantly through just `waveOutWrite`.

Signed-off-by: Liam Murphy 
---
 dlls/winmm/tests/wave.c | 1 -
 dlls/winmm/waveform.c   | 8 ++++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/dlls/winmm/tests/wave.c b/dlls/winmm/tests/wave.c
index 83e83a0150e..261eee76109 100644
--- a/dlls/winmm/tests/wave.c
+++ b/dlls/winmm/tests/wave.c
@@ -881,7 +881,6 @@ static void CALLBACK reentrancy_callback_func(HWAVEOUT hwo, UINT uMsg,

data->call_num += 1;

-    todo_wine_if(data->call_num == 3)
     ok(data->running_thread != GetCurrentThreadId(), "winmm callback called reentrantly, with message %u\n", uMsg);
     if (data->running_thread) {
         if (data->running_thread != GetCurrentThreadId()) trace("Callback running on two threads simultaneously\n");
diff --git a/dlls/winmm/waveform.c b/dlls/winmm/waveform.c
index 1159b48b483..3a6fc75022b 100644
--- a/dlls/winmm/waveform.c
+++ b/dlls/winmm/waveform.c
@@ -1948,11 +1948,11 @@ static MMRESULT WINMM_BeginPlaying(WINMM_Device *device)

TRACE("(%p)\n", device->handle);

-    if(device->render)
-        /* prebuffer data before starting */
-        WOD_PushData(device);
-
     if(device->stopped){
+        if(device->render)
+            /* prebuffer data before starting */
+            WOD_PushData(device);
+
         device->stopped = FALSE;

hr = IAudioClient_Start(device->client);