Re: [PATCH 2/2] winmm: Use a thread to send driver messages on Win9x (try 2)

[Andrew Eikum]

This seems like something you should be able to write a test for. I'm
also curious how this manages to work on Windows >= XP

You need to be more careful about thread shutdown. See
WINMM_StartDevicesThread and WINMM_DevicesThreadProc. I think you
could use the same logic, including WINMM_DevicesThreadDone, to
determine when to quit your new thread.

Andrew

On Thu, Dec 01, 2016 at 11:32:35AM -0200, Bruno Jesus wrote:
> try 2:
> Use a better way to detect Win9x (thanks Dmitry)
> Use a single point of config/teardown and a pointer to callback (thanks Nikolay)
>
> More detailed explanation in the patch comments. Basically the games use a DLL that calls SuspendThread from inside the WinMM callback, this makes the program hang as we use the "main thread" to dispatch callbacks.
>
> Games tested in Win95 config:
> Heroes of Might & Magic (well known affected)
> Deadlock (well known affected)
> Worms 2 (just to ensure things still work)
> Age of Empires (just to ensure things still work)
> Shivers (just to ensure things still work)
>
> Fixes bug https://bugs.winehq.org/show_bug.cgi?id=3930
> Signed-off-by: Bruno Jesus
>
> dlls/winmm/waveform.c | 117 ++++++++++++++++++++++++++++++++++++++++++--------
>  dlls/winmm/winemm.h   |   1 +
>  dlls/winmm/winmm.c    |   2 +-
>  3 files changed, 101 insertions(+), 19 deletions(-)
>
> diff --git a/dlls/winmm/waveform.c b/dlls/winmm/waveform.c
> index 7308519..4387c44 100644
> --- a/dlls/winmm/waveform.c
> +++ b/dlls/winmm/waveform.c
> @@ -134,6 +134,13 @@ struct _WINMM_MMDevice {
>      WINMM_Device *devices[MAX_DEVICES];
>  };
>
> +static struct {
> - HANDLE thread, event_main, event_thread;
> - WINMM_CBInfo *info;
> - WORD msg;
> - DWORD_PTR param1, param2;
>
> +} win9x;
> -
>
> static WINMM_MMDevice *g_out_mmdevices;
>  static WINMM_MMDevice **g_out_map;
>  static UINT g_outmmdevices_count;
> @@ -192,6 +199,77 @@ static LRESULT WID_Open(WINMM_OpenInfo *info);
>  static LRESULT WID_Close(HWAVEIN hwave);
>  static MMRESULT WINMM_BeginPlaying(WINMM_Device *device);
>
> +static void (*pWINMM_NotifyClient)(WINMM_CBInfo *info, WORD msg, DWORD_PTR param1,
> -
>    DWORD_PTR param2);
> -
> -
>
> +static DWORD WINAPI win9x_callback_thread(LPVOID param)
> +{
> - while(WaitForSingleObject(win9x.event_thread, INFINITE) == WAIT_OBJECT_0)
> - {
> -
>    if (!win9x.info->hwave) break;
> -
>    DriverCallback(win9x.info->callback, win9x.info->flags, (HDRVR)win9x.info->hwave,
> -
>                   win9x.msg, win9x.info->user, win9x.param1, win9x.param2);
> -
>    SetEvent(win9x.event_main);
> - }
> - return 0;
>
> +}
> -
>
> +/* Note: NotifyClient should never be called while holding the device lock
> -
> - since the client may call wave* functions from within the callback. */
>
> +static inline void WINMM_NotifyClient(WINMM_CBInfo *info, WORD msg, DWORD_PTR param1,
> -
>    DWORD_PTR param2)
>
> +{
> - DriverCallback(info->callback, info->flags, (HDRVR)info->hwave,
> -
>    msg, info->user, param1, param2);
>
> +}
> -
>
> +static inline void WINMM_NotifyClient9x(WINMM_CBInfo *info, WORD msg, DWORD_PTR param1,
> -
>    DWORD_PTR param2)
>
> +{
> - /* Prepare callback data for the thread, dispatch and wait response */
> - win9x.info = info;
> - win9x.msg = msg;
> - win9x.param1 = param1;
> - win9x.param2 = param2;
> - SetEvent(win9x.event_thread);
> - WaitForSingleObject(win9x.event_main, INFINITE);
>
> +}
> -
>
> +void WINMM_SetupCallback(void)
> +{
> - pWINMM_NotifyClient = WINMM_NotifyClient;
> - if (!(GetVersion() & 0x80000000))
> -
>    return;
> -
> - /* WAIL32.DLL from Rad Game Tools is an audio helper DLL that simplifies the
> -
> * use of sound effects in an application. It seems to be released in 1996 and
> -
> * is used at least by Heroes of Might and Magic and Deadlock. Unfortunately
> -
> * during a callback to the DLL it calls SuspendThread on the main thread and
> -
> * keeps doing some stuff from inside the callback until it does a ResumeThread
> -
> * and returns from the callback. This works in Windows 95/98 because they use
> -
> * a separate thread to send callback messages. XP changed this by going back
> -
> * to a single thread but the games still works due to shim.
> -
> */
> - TRACE("Using old behavior of WinMM callbacks\n");
> - win9x.event_thread = CreateEventA(NULL, FALSE, FALSE, NULL);
> - win9x.event_main = CreateEventA(NULL, FALSE, FALSE, NULL);
> - win9x.thread = CreateThread(NULL, 0, win9x_callback_thread, NULL, 0, NULL);
> - if (win9x.event_thread && win9x.event_main && win9x.thread)
> -
>    pWINMM_NotifyClient = WINMM_NotifyClient9x;
> - else
> - {
> -
>    ERR("Failed to create resources for callback, expect problems!\n");
> -
>    TerminateThread(win9x.thread, 0);
> -
>    CloseHandle(win9x.event_main);
> -
>    CloseHandle(win9x.event_thread);
> -
>    CloseHandle(win9x.thread);
> -
>    win9x.event_main = win9x.event_thread = win9x.thread = NULL;
> -
>    return;
> - }
>
> +}
> -
>
> +/* tear down function for DLL process detach */
>  void WINMM_DeleteWaveform(void)
>  {
>      UINT i, j;
> @@ -241,6 +319,18 @@ void WINMM_DeleteWaveform(void)
>      HeapFree(GetProcessHeap(), 0, g_device_handles);
>      HeapFree(GetProcessHeap(), 0, g_handle_devices);
> - if (win9x.thread)
> - {
> -
>    /* warn the thread so it can die gracefully */
> -
>    win9x.info->hwave = 0;
> -
>    SetEvent(win9x.event_thread);
> -
> -
>    CloseHandle(win9x.event_main);
> -
>    CloseHandle(win9x.event_thread);
> -
>    CloseHandle(win9x.thread);
> -
>    win9x.event_main = win9x.event_thread = win9x.thread = NULL;
> - }
> - DeleteCriticalSection(&g_devthread_lock);
>
> }
>
> @@ -370,15 +460,6 @@ static WINMM_Device *WINMM_GetDeviceFromHWAVE(HWAVE hwave)
>      return device;
>  }
>
> -/* Note: NotifyClient should never be called while holding the device lock
> -
> - since the client may call wave* functions from within the callback. */
>
> -static inline void WINMM_NotifyClient(WINMM_CBInfo *info, WORD msg, DWORD_PTR param1,
> -
>    DWORD_PTR param2)
>
> -{
> - DriverCallback(info->callback, info->flags, (HDRVR)info->hwave,
> -
>    msg, info->user, param1, param2);
>
> -}
> -
>
> static MMRESULT hr2mmr(HRESULT hr)
>  {
>      switch(hr){
> @@ -1753,7 +1834,7 @@ exit:
>          WAVEHDR *next = first->lpNext;
>          first->dwFlags &= ~WHDR_INQUEUE;
>          first->dwFlags |= WHDR_DONE;
> -
>    WINMM_NotifyClient(&cb_info, WOM_DONE, (DWORD_PTR)first, 0);
> -
>    pWINMM_NotifyClient(&cb_info, WOM_DONE, (DWORD_PTR)first, 0);
>    first = next;
> }
>
> }
> @@ -1929,7 +2010,7 @@ exit:
>              WAVEHDR *next = first->lpNext;
>              first->dwFlags &= ~WHDR_INQUEUE;
>              first->dwFlags |= WHDR_DONE;
> -
>        WINMM_NotifyClient(&cb_info, WIM_DATA, (DWORD_PTR)first, 0);
> -
>        pWINMM_NotifyClient(&cb_info, WIM_DATA, (DWORD_PTR)first, 0);
>        first = next;
>    }
> }
>
> @@ -2022,9 +2103,9 @@ static LRESULT WINMM_Reset(HWAVE hwave)
>          first->dwFlags &= ~WHDR_INQUEUE;
>          first->dwFlags |= WHDR_DONE;
>          if(is_out)
> -
>        WINMM_NotifyClient(&cb_info, WOM_DONE, (DWORD_PTR)first, 0);
> -
>        pWINMM_NotifyClient(&cb_info, WOM_DONE, (DWORD_PTR)first, 0);
>    else
> -
>        WINMM_NotifyClient(&cb_info, WIM_DATA, (DWORD_PTR)first, 0);
> -
>        pWINMM_NotifyClient(&cb_info, WIM_DATA, (DWORD_PTR)first, 0);
>    first = next;
> }
>
> @@ -2774,7 +2855,7 @@ MMRESULT WINAPI waveOutOpen(LPHWAVEOUT lphWaveOut, UINT uDeviceID,
>      cb_info.user = dwInstance;
>      cb_info.hwave = info.handle;
> - WINMM_NotifyClient(&cb_info, WOM_OPEN, 0, 0);
> -
>
> pWINMM_NotifyClient(&cb_info, WOM_OPEN, 0, 0);
>
> return res;
>
> }
> @@ -2802,7 +2883,7 @@ UINT WINAPI waveOutClose(HWAVEOUT hWaveOut)
>      res = SendMessageW(g_devices_hwnd, WODM_CLOSE, (WPARAM)hWaveOut, 0);
>  if(res == MMSYSERR_NOERROR)
> -
>    WINMM_NotifyClient(&cb_info, WOM_CLOSE, 0, 0);
> -
>    pWINMM_NotifyClient(&cb_info, WOM_CLOSE, 0, 0);
>
> return res;
>
> }
> @@ -3410,7 +3491,7 @@ MMRESULT WINAPI waveInOpen(HWAVEIN* lphWaveIn, UINT uDeviceID,
>      cb_info.user = dwInstance;
>      cb_info.hwave = info.handle;
> - WINMM_NotifyClient(&cb_info, WIM_OPEN, 0, 0);
> -
>
> pWINMM_NotifyClient(&cb_info, WIM_OPEN, 0, 0);
>
> return res;
>
> }
> @@ -3438,7 +3519,7 @@ UINT WINAPI waveInClose(HWAVEIN hWaveIn)
>      res = SendMessageW(g_devices_hwnd, WIDM_CLOSE, (WPARAM)hWaveIn, 0);
>  if(res == MMSYSERR_NOERROR)
> -
>    WINMM_NotifyClient(&cb_info, WIM_CLOSE, 0, 0);
> -
>    pWINMM_NotifyClient(&cb_info, WIM_CLOSE, 0, 0);
>
> return res;
>
> }
> @@ -3586,7 +3667,7 @@ UINT WINAPI waveInStop(HWAVEIN hWaveIn)
>      if(buf){
>          buf->dwFlags &= ~WHDR_INQUEUE;
>          buf->dwFlags |= WHDR_DONE;
> -
>    WINMM_NotifyClient(&cb_info, WIM_DATA, (DWORD_PTR)buf, 0);
> -
>    pWINMM_NotifyClient(&cb_info, WIM_DATA, (DWORD_PTR)buf, 0);
>
> }
>
> return MMSYSERR_NOERROR;
>
> diff --git a/dlls/winmm/winemm.h b/dlls/winmm/winemm.h
> index d7bab0f..10cd0f4 100644
> --- a/dlls/winmm/winemm.h
> +++ b/dlls/winmm/winemm.h
> @@ -151,6 +151,7 @@ void		TIME_MMTimeStop(void) DECLSPEC_HIDDEN;
>
> MMRESULT WINMM_CheckCallback(DWORD_PTR dwCallback, DWORD fdwOpen, BOOL mixer) DECLSPEC_HIDDEN;
>
> +void WINMM_SetupCallback(void) DECLSPEC_HIDDEN;
>  void WINMM_DeleteWaveform(void) DECLSPEC_HIDDEN;
>
> /* Global variables */
> diff --git a/dlls/winmm/winmm.c b/dlls/winmm/winmm.c
> index 0f3cd5d..358f139 100644
> --- a/dlls/winmm/winmm.c
> +++ b/dlls/winmm/winmm.c
> @@ -139,7 +139,7 @@ BOOL WINAPI DllMain(HINSTANCE hInstDLL, DWORD fdwReason, LPVOID fImpLoad)
>      if (!WINMM_CreateIData(hInstDLL))
>          return FALSE;
> -
> -
>    WINMM_SetupCallback();
>    break;
> case DLL_PROCESS_DETACH:
>        if(fImpLoad)
>
> -- 
> 2.9.3