[PATCH 0/6] MR536: xactengine3_7: Add helper function to add entries

[Alistair Leslie-Hughes (＠alesliehughes)]

Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536

[PATCH 1/6] xactengine3_7: Add helper function to add entries

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>

Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
---
 dlls/xactengine3_7/xact_dll.c | 67 +++++++++++++++++------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/dlls/xactengine3_7/xact_dll.c b/dlls/xactengine3_7/xact_dll.c
index ee559dd4b07..a06ca67ad8d 100644
--- a/dlls/xactengine3_7/xact_dll.c
+++ b/dlls/xactengine3_7/xact_dll.c
@@ -114,6 +114,33 @@ static void wrapper_remove_entry(XACT3EngineImpl *engine, void *key)
     }
 }
 
+static HRESULT wrapper_add_entry(XACT3EngineImpl *engine, void *fact, void *xact)
+{
+    struct wrapper_lookup *lookup;
+    UINT ret;
+
+    lookup = HeapAlloc(GetProcessHeap(), 0, sizeof(*lookup));
+    if (!lookup)
+    {
+        ERR("Failed to allocate wrapper_lookup!\n");
+        return E_OUTOFMEMORY;
+    }
+    lookup->fact = fact;
+    lookup->xact = xact;
+
+    EnterCriticalSection(&engine->wb_wrapper_lookup_cs);
+    ret = wine_rb_put(&engine->wb_wrapper_lookup, lookup->fact, &lookup->entry);
+    LeaveCriticalSection(&engine->wb_wrapper_lookup_cs);
+
+    if (ret)
+    {
+        WARN("wrapper_lookup already present in the tree??\n");
+        HeapFree(GetProcessHeap(), 0, lookup);
+    }
+
+    return S_OK;
+}
+
 typedef struct _XACT3CueImpl {
     IXACT3Cue IXACT3Cue_iface;
     FACTCue *fact_cue;
@@ -1095,9 +1122,9 @@ static HRESULT WINAPI IXACT3EngineImpl_CreateInMemoryWaveBank(IXACT3Engine *ifac
         DWORD dwAllocAttributes, IXACT3WaveBank **ppWaveBank)
 {
     XACT3EngineImpl *This = impl_from_IXACT3Engine(iface);
-    struct wrapper_lookup *lookup;
     XACT3WaveBankImpl *wb;
     FACTWaveBank *fwb;
+    HRESULT hr;
     UINT ret;
 
     TRACE("(%p)->(%p, %lu, 0x%lx, 0x%lx, %p)\n", This, pvBuffer, dwSize, dwFlags,
@@ -1119,28 +1146,14 @@ static HRESULT WINAPI IXACT3EngineImpl_CreateInMemoryWaveBank(IXACT3Engine *ifac
         return E_OUTOFMEMORY;
     }
 
-    lookup = HeapAlloc(GetProcessHeap(), 0, sizeof(*lookup));
-    if (!lookup)
+    hr = wrapper_add_entry(This, fwb, &wb->IXACT3WaveBank_iface);
+    if (FAILED(hr))
     {
         FACTWaveBank_Destroy(fwb);
         HeapFree(GetProcessHeap(), 0, wb);
         ERR("Failed to allocate wrapper_lookup!\n");
         return E_OUTOFMEMORY;
     }
-    lookup->fact = fwb;
-    lookup->xact = &wb->IXACT3WaveBank_iface;
-
-    EnterCriticalSection(&This->wb_wrapper_lookup_cs);
-
-    ret = wine_rb_put(&This->wb_wrapper_lookup, lookup->fact, &lookup->entry);
-
-    LeaveCriticalSection(&This->wb_wrapper_lookup_cs);
-
-    if (ret)
-    {
-        WARN("wrapper_lookup already present in the tree??\n");
-        HeapFree(GetProcessHeap(), 0, lookup);
-    }
 
     wb->IXACT3WaveBank_iface.lpVtbl = &XACT3WaveBank_Vtbl;
     wb->fact_wavebank = fwb;
@@ -1158,11 +1171,11 @@ static HRESULT WINAPI IXACT3EngineImpl_CreateStreamingWaveBank(IXACT3Engine *ifa
 {
     XACT3EngineImpl *This = impl_from_IXACT3Engine(iface);
     FACTStreamingParameters fakeParms;
-    struct wrapper_lookup *lookup;
     wrap_readfile_struct *fake;
     XACT3WaveBankImpl *wb;
     FACTWaveBank *fwb;
     UINT ret;
+    HRESULT hr;
 
     TRACE("(%p)->(%p, %p)\n", This, pParms, ppWaveBank);
 
@@ -1192,28 +1205,14 @@ static HRESULT WINAPI IXACT3EngineImpl_CreateStreamingWaveBank(IXACT3Engine *ifa
         return E_OUTOFMEMORY;
     }
 
-    lookup = HeapAlloc(GetProcessHeap(), 0, sizeof(*lookup));
-    if (!lookup)
+    hr = wrapper_add_entry(This, fwb, &wb->IXACT3WaveBank_iface);
+    if (FAILED(hr))
     {
         FACTWaveBank_Destroy(fwb);
         HeapFree(GetProcessHeap(), 0, wb);
         ERR("Failed to allocate wrapper_lookup!\n");
         return E_OUTOFMEMORY;
     }
-    lookup->fact = fwb;
-    lookup->xact = &wb->IXACT3WaveBank_iface;
-
-    EnterCriticalSection(&This->wb_wrapper_lookup_cs);
-
-    ret = wine_rb_put(&This->wb_wrapper_lookup, lookup->fact, &lookup->entry);
-
-    LeaveCriticalSection(&This->wb_wrapper_lookup_cs);
-
-    if (ret)
-    {
-        WARN("wrapper_lookup already present in the tree??\n");
-        HeapFree(GetProcessHeap(), 0, lookup);
-    }
 
     wb->IXACT3WaveBank_iface.lpVtbl = &XACT3WaveBank_Vtbl;
     wb->fact_wavebank = fwb;
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/536

[PATCH 2/6] xactengine3_7: Map SoundBank interfaces

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>

Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
---
 dlls/xactengine3_7/xact_dll.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/dlls/xactengine3_7/xact_dll.c b/dlls/xactengine3_7/xact_dll.c
index a06ca67ad8d..b56cd5b46e6 100644
--- a/dlls/xactengine3_7/xact_dll.c
+++ b/dlls/xactengine3_7/xact_dll.c
@@ -346,6 +346,7 @@ typedef struct _XACT3SoundBankImpl {
     IXACT3SoundBank IXACT3SoundBank_iface;
 
     FACTSoundBank *fact_soundbank;
+    XACT3EngineImpl *engine;
 } XACT3SoundBankImpl;
 
 static inline XACT3SoundBankImpl *impl_from_IXACT3SoundBank(IXACT3SoundBank *iface)
@@ -482,6 +483,8 @@ static HRESULT WINAPI IXACT3SoundBankImpl_Destroy(IXACT3SoundBank *iface)
     TRACE("(%p)\n", This);
 
     hr = FACTSoundBank_Destroy(This->fact_soundbank);
+
+    wrapper_remove_entry(This->engine, This->fact_soundbank);
     HeapFree(GetProcessHeap(), 0, This);
     return hr;
 }
@@ -1088,6 +1091,7 @@ static HRESULT WINAPI IXACT3EngineImpl_CreateSoundBank(IXACT3Engine *iface,
     XACT3SoundBankImpl *sb;
     FACTSoundBank *fsb;
     UINT ret;
+    HRESULT hr;
 
     TRACE("(%p)->(%p, %lu, 0x%lx, 0x%lx, %p): stub!\n", This, pvBuffer, dwSize, dwFlags,
             dwAllocAttributes, ppSoundBank);
@@ -1108,8 +1112,18 @@ static HRESULT WINAPI IXACT3EngineImpl_CreateSoundBank(IXACT3Engine *iface,
         return E_OUTOFMEMORY;
     }
 
+    hr = wrapper_add_entry(This, fsb, &sb->IXACT3SoundBank_iface);
+    if (FAILED(hr))
+    {
+        FACTSoundBank_Destroy(fsb);
+        HeapFree(GetProcessHeap(), 0, sb);
+        ERR("Failed to allocate wrapper_lookup!\n");
+        return E_OUTOFMEMORY;
+    }
+
     sb->IXACT3SoundBank_iface.lpVtbl = &XACT3SoundBank_Vtbl;
     sb->fact_soundbank = fsb;
+    sb->engine = This;
     *ppSoundBank = &sb->IXACT3SoundBank_iface;
 
     TRACE("Created SoundBank: %p\n", sb);
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/536

[PATCH 3/6] xactengine3_7: Map IXACT3Cue interfaces

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>

Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
---
 dlls/xactengine3_7/xact_dll.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/dlls/xactengine3_7/xact_dll.c b/dlls/xactengine3_7/xact_dll.c
index b56cd5b46e6..d34ee0daf0c 100644
--- a/dlls/xactengine3_7/xact_dll.c
+++ b/dlls/xactengine3_7/xact_dll.c
@@ -144,6 +144,7 @@ static HRESULT wrapper_add_entry(XACT3EngineImpl *engine, void *fact, void *xact
 typedef struct _XACT3CueImpl {
     IXACT3Cue IXACT3Cue_iface;
     FACTCue *fact_cue;
+    XACT3EngineImpl *engine;
 } XACT3CueImpl;
 
 static inline XACT3CueImpl *impl_from_IXACT3Cue(IXACT3Cue *iface)
@@ -188,6 +189,7 @@ static HRESULT WINAPI IXACT3CueImpl_Destroy(IXACT3Cue *iface)
     ret = FACTCue_Destroy(This->fact_cue);
     if (ret != 0)
         WARN("FACTCue_Destroy returned %d\n", ret);
+    wrapper_remove_entry(This->engine, This->fact_cue);
     HeapFree(GetProcessHeap(), 0, This);
     return S_OK;
 }
@@ -395,6 +397,7 @@ static HRESULT WINAPI IXACT3SoundBankImpl_Prepare(IXACT3SoundBank *iface,
     XACT3CueImpl *cue;
     FACTCue *fcue;
     UINT ret;
+    HRESULT hr;
 
     TRACE("(%p)->(%u, 0x%lx, %lu, %p)\n", This, nCueIndex, dwFlags, timeOffset,
             ppCue);
@@ -415,8 +418,18 @@ static HRESULT WINAPI IXACT3SoundBankImpl_Prepare(IXACT3SoundBank *iface,
         return E_OUTOFMEMORY;
     }
 
+    hr = wrapper_add_entry(This->engine, fcue, &cue->IXACT3Cue_iface);
+    if (FAILED(hr))
+    {
+        FACTCue_Destroy(fcue);
+        HeapFree(GetProcessHeap(), 0, cue);
+        ERR("Failed to allocate wrapper_lookup!\n");
+        return E_OUTOFMEMORY;
+    }
+
     cue->IXACT3Cue_iface.lpVtbl = &XACT3Cue_Vtbl;
     cue->fact_cue = fcue;
+    cue->engine = This->engine;
     *ppCue = &cue->IXACT3Cue_iface;
 
     TRACE("Created Cue: %p\n", cue);
@@ -457,8 +470,18 @@ static HRESULT WINAPI IXACT3SoundBankImpl_Play(IXACT3SoundBank *iface,
             return E_OUTOFMEMORY;
         }
 
+        hr = wrapper_add_entry(This->engine, fcue, &cue->IXACT3Cue_iface);
+        if (FAILED(hr))
+        {
+            FACTCue_Destroy(fcue);
+            HeapFree(GetProcessHeap(), 0, cue);
+            ERR("Failed to allocate wrapper_lookup!\n");
+            return E_OUTOFMEMORY;
+        }
+
         cue->IXACT3Cue_iface.lpVtbl = &XACT3Cue_Vtbl;
         cue->fact_cue = fcue;
+        cue->engine = This->engine;
         *ppCue = &cue->IXACT3Cue_iface;
     }
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/536

[PATCH 4/6] xactengine3_7: Map IXACT3Wave interfaces

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>

Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
---
 dlls/xactengine3_7/xact_dll.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/dlls/xactengine3_7/xact_dll.c b/dlls/xactengine3_7/xact_dll.c
index d34ee0daf0c..da2403ea9d1 100644
--- a/dlls/xactengine3_7/xact_dll.c
+++ b/dlls/xactengine3_7/xact_dll.c
@@ -542,6 +542,7 @@ typedef struct _XACT3WaveImpl {
     IXACT3Wave IXACT3Wave_iface;
 
     FACTWave *fact_wave;
+    XACT3EngineImpl *engine;
 } XACT3WaveImpl;
 
 static inline XACT3WaveImpl *impl_from_IXACT3Wave(IXACT3Wave *iface)
@@ -557,6 +558,7 @@ static HRESULT WINAPI IXACT3WaveImpl_Destroy(IXACT3Wave *iface)
     TRACE("(%p)\n", This);
 
     hr = FACTWave_Destroy(This->fact_wave);
+    wrapper_remove_entry(This->engine, This->fact_wave);
     HeapFree(GetProcessHeap(), 0, This);
     return hr;
 }
@@ -722,6 +724,7 @@ static HRESULT WINAPI IXACT3WaveBankImpl_Prepare(IXACT3WaveBank *iface,
     XACT3WaveImpl *wave;
     FACTWave *fwave;
     UINT ret;
+    HRESULT hr;
 
     TRACE("(%p)->(0x%x, %lu, 0x%lx, %u, %p)\n", This, nWaveIndex, dwFlags,
             dwPlayOffset, nLoopCount, ppWave);
@@ -742,8 +745,18 @@ static HRESULT WINAPI IXACT3WaveBankImpl_Prepare(IXACT3WaveBank *iface,
         return E_OUTOFMEMORY;
     }
 
+    hr = wrapper_add_entry(This->engine, fwave, &wave->IXACT3Wave_iface);
+    if (FAILED(hr))
+    {
+        FACTWave_Destroy(fwave);
+        HeapFree(GetProcessHeap(), 0, wave);
+        ERR("Failed to allocate wrapper_lookup!\n");
+        return E_OUTOFMEMORY;
+    }
+
     wave->IXACT3Wave_iface.lpVtbl = &XACT3Wave_Vtbl;
     wave->fact_wave = fwave;
+    wave->engine = This->engine;
     *ppWave = &wave->IXACT3Wave_iface;
 
     TRACE("Created Wave: %p\n", wave);
@@ -784,8 +797,18 @@ static HRESULT WINAPI IXACT3WaveBankImpl_Play(IXACT3WaveBank *iface,
             return E_OUTOFMEMORY;
         }
 
+        hr = wrapper_add_entry(This->engine, fwave, &wave->IXACT3Wave_iface);
+        if (FAILED(hr))
+        {
+            FACTWave_Destroy(fwave);
+            HeapFree(GetProcessHeap(), 0, wave);
+            ERR("Failed to allocate wrapper_lookup!\n");
+            return E_OUTOFMEMORY;
+        }
+
         wave->IXACT3Wave_iface.lpVtbl = &XACT3Wave_Vtbl;
         wave->fact_wave = fwave;
+        wave->engine = This->engine;
         *ppWave = &wave->IXACT3Wave_iface;
     }
 
@@ -1293,6 +1316,7 @@ static HRESULT WINAPI IXACT3EngineImpl_PrepareWave(IXACT3Engine *iface,
     XACT3WaveImpl *wave;
     FACTWave *fwave = NULL;
     UINT ret;
+    HRESULT hr;
 
     TRACE("(%p)->(0x%08lx, %s, %d, %ld, %ld, %d, %p)\n", This, dwFlags, debugstr_a(szWavePath),
           wStreamingPacketSize, dwAlignment, dwPlayOffset, nLoopCount, ppWave);
@@ -1312,8 +1336,18 @@ static HRESULT WINAPI IXACT3EngineImpl_PrepareWave(IXACT3Engine *iface,
         return E_OUTOFMEMORY;
     }
 
+    hr = wrapper_add_entry(This, fwave, &wave->IXACT3Wave_iface);
+    if (FAILED(hr))
+    {
+        FACTWave_Destroy(fwave);
+        HeapFree(GetProcessHeap(), 0, wave);
+        ERR("Failed to allocate wrapper_lookup!\n");
+        return E_OUTOFMEMORY;
+    }
+
     wave->IXACT3Wave_iface.lpVtbl = &XACT3Wave_Vtbl;
     wave->fact_wave = fwave;
+    wave->engine = This;
     *ppWave = &wave->IXACT3Wave_iface;
 
     TRACE("Created Wave: %p\n", wave);
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/536

[PATCH 5/6] xactengine3_7: Record context for each notications

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>

This helps the first lockup but starting a game causes another freeze.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49678

Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
---
 dlls/xactengine3_7/xact_dll.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/dlls/xactengine3_7/xact_dll.c b/dlls/xactengine3_7/xact_dll.c
index da2403ea9d1..9981d25a9f5 100644
--- a/dlls/xactengine3_7/xact_dll.c
+++ b/dlls/xactengine3_7/xact_dll.c
@@ -69,8 +69,7 @@ typedef struct _XACT3EngineImpl {
     XACT_GETOVERLAPPEDRESULT_CALLBACK pGetOverlappedResult;
     XACT_NOTIFICATION_CALLBACK notification_callback;
 
-    void *wb_prepared_context;
-    void *wb_destroyed_context;
+    void *contexts[17];
     struct wine_rb_tree wb_wrapper_lookup;
     CRITICAL_SECTION wb_wrapper_lookup_cs;
 } XACT3EngineImpl;
@@ -1017,6 +1016,7 @@ static void FACTCALL fact_notification_cb(const FACTNotification *notification)
 
     xnotification.type = xact_notification_type_from_fact(notification->type);
     xnotification.timeStamp = notification->timeStamp;
+    xnotification.pvContext = engine->contexts[notification->type - 1];
 
     if (notification->type == XACTNOTIFICATIONTYPE_WAVEBANKPREPARED
             || notification->type == XACTNOTIFICATIONTYPE_WAVEBANKDESTROYED)
@@ -1034,10 +1034,6 @@ static void FACTCALL fact_notification_cb(const FACTNotification *notification)
             xnotification.waveBank.pWaveBank = lookup->xact;
         }
         LeaveCriticalSection(&engine->wb_wrapper_lookup_cs);
-        if (notification->type == XACTNOTIFICATIONTYPE_WAVEBANKPREPARED)
-            xnotification.pvContext = engine->wb_prepared_context;
-        else
-            xnotification.pvContext = engine->wb_destroyed_context;
     }
     else
     {
@@ -1490,12 +1486,8 @@ static HRESULT WINAPI IXACT3EngineImpl_RegisterNotification(IXACT3Engine *iface,
 
     TRACE("(%p)->(%p)\n", This, pNotificationDesc);
 
-    if (pNotificationDesc->type == XACTNOTIFICATIONTYPE_WAVEBANKPREPARED)
-        This->wb_prepared_context = pNotificationDesc->pvContext;
-    else if (pNotificationDesc->type == XACTNOTIFICATIONTYPE_WAVEBANKDESTROYED)
-        This->wb_destroyed_context = pNotificationDesc->pvContext;
-
     unwrap_notificationdesc(&fdesc, pNotificationDesc);
+    This->contexts[pNotificationDesc->type - 1] = pNotificationDesc->pvContext;
     fdesc.pvContext = This;
     return FACTAudioEngine_RegisterNotification(This->fact_engine, &fdesc);
 }
@@ -1509,6 +1501,7 @@ static HRESULT WINAPI IXACT3EngineImpl_UnRegisterNotification(IXACT3Engine *ifac
     TRACE("(%p)->(%p)\n", This, pNotificationDesc);
 
     unwrap_notificationdesc(&fdesc, pNotificationDesc);
+    This->contexts[pNotificationDesc->type - 1] = pNotificationDesc->pvContext;
     fdesc.pvContext = This;
     return FACTAudioEngine_UnRegisterNotification(This->fact_engine, &fdesc);
 }
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/536

[PATCH 6/6] xactengine3_7: Implement callback for supported messages

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>

---
 dlls/xactengine3_7/xact_dll.c | 69 +++++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 16 deletions(-)

diff --git a/dlls/xactengine3_7/xact_dll.c b/dlls/xactengine3_7/xact_dll.c
index 9981d25a9f5..f00c0c4ac1f 100644
--- a/dlls/xactengine3_7/xact_dll.c
+++ b/dlls/xactengine3_7/xact_dll.c
@@ -140,6 +140,23 @@ static HRESULT wrapper_add_entry(XACT3EngineImpl *engine, void *fact, void *xact
     return S_OK;
 }
 
+/* Must be protected by engine->wb_wrapper_lookup_cs */
+static void* wrapper_find_entry(XACT3EngineImpl *engine, void *faudio)
+{
+    struct wrapper_lookup *lookup;
+    struct wine_rb_entry *entry;
+
+    entry = wine_rb_get(&engine->wb_wrapper_lookup, faudio);
+    if (entry)
+    {
+        lookup = WINE_RB_ENTRY_VALUE(entry, struct wrapper_lookup, entry);
+        return lookup->xact;
+    }
+
+    WARN("cannot find interface in wrapper lookup\n");
+    return NULL;
+}
+
 typedef struct _XACT3CueImpl {
     IXACT3Cue IXACT3Cue_iface;
     FACTCue *fact_cue;
@@ -1002,10 +1019,8 @@ static void FACTCALL fact_notification_cb(const FACTNotification *notification)
 {
     XACT3EngineImpl *engine = (XACT3EngineImpl *)notification->pvContext;
     XACT_NOTIFICATION xnotification;
-    struct wrapper_lookup *lookup;
-    struct wine_rb_entry *entry;
 
-    TRACE("notification %p\n", notification->pvContext);
+    TRACE("notification %d, context %p\n", notification->type, notification->pvContext);
 
     /* Older versions of FAudio don't pass through the context */
     if (!engine)
@@ -1018,28 +1033,50 @@ static void FACTCALL fact_notification_cb(const FACTNotification *notification)
     xnotification.timeStamp = notification->timeStamp;
     xnotification.pvContext = engine->contexts[notification->type - 1];
 
+    EnterCriticalSection(&engine->wb_wrapper_lookup_cs);
     if (notification->type == XACTNOTIFICATIONTYPE_WAVEBANKPREPARED
             || notification->type == XACTNOTIFICATIONTYPE_WAVEBANKDESTROYED)
     {
-        EnterCriticalSection(&engine->wb_wrapper_lookup_cs);
-        entry = wine_rb_get(&engine->wb_wrapper_lookup, notification->waveBank.pWaveBank);
-        if (!entry)
-        {
-            WARN("cannot find wave bank in wrapper lookup\n");
-            xnotification.waveBank.pWaveBank = NULL;
-        }
-        else
-        {
-            lookup = WINE_RB_ENTRY_VALUE(entry, struct wrapper_lookup, entry);
-            xnotification.waveBank.pWaveBank = lookup->xact;
-        }
-        LeaveCriticalSection(&engine->wb_wrapper_lookup_cs);
+        xnotification.waveBank.pWaveBank = wrapper_find_entry(engine, notification->waveBank.pWaveBank);
+    }
+    else if(notification->type == XACTNOTIFICATIONTYPE_SOUNDBANKDESTROYED)
+    {
+        xnotification.soundBank.pSoundBank = wrapper_find_entry(engine, notification->soundBank.pSoundBank);
+    }
+    else if (notification->type == XACTNOTIFICATIONTYPE_WAVESTOP
+#if XACT3_VER >= 0x0205
+             || notification->type == XACTNOTIFICATIONTYPE_WAVEDESTROYED
+             || notification->type == XACTNOTIFICATIONTYPE_WAVELOOPED
+             || notification->type == XACTNOTIFICATIONTYPE_WAVEPLAY
+             || notification->type == XACTNOTIFICATIONTYPE_WAVEPREPARED)
+#else
+             )
+#endif
+    {
+        xnotification.wave.cueIndex = notification->wave.cueIndex;
+        xnotification.wave.pCue = wrapper_find_entry(engine, notification->wave.pCue);
+        xnotification.wave.pSoundBank = wrapper_find_entry(engine, notification->wave.pSoundBank);
+#if XACT3_VER >= 0x0205
+        xnotification.wave.pWave = wrapper_find_entry(engine, notification->wave.pWave);
+#endif
+        xnotification.wave.pWaveBank = wrapper_find_entry(engine, notification->wave.pWaveBank);
+    }
+    else if (notification->type == XACTNOTIFICATIONTYPE_CUEPLAY ||
+             notification->type == XACTNOTIFICATIONTYPE_CUEPREPARED ||
+             notification->type == XACTNOTIFICATIONTYPE_CUESTOP ||
+             notification->type == XACTNOTIFICATIONTYPE_CUEDESTROYED)
+    {
+        xnotification.cue.pCue = wrapper_find_entry(engine, notification->cue.pCue);
+        xnotification.cue.cueIndex = notification->cue.cueIndex;
+        xnotification.cue.pSoundBank = wrapper_find_entry(engine, notification->cue.pSoundBank);
     }
     else
     {
+        LeaveCriticalSection(&engine->wb_wrapper_lookup_cs);
         FIXME("Unsupported callback type %d\n", notification->type);
         return;
     }
+    LeaveCriticalSection(&engine->wb_wrapper_lookup_cs);
 
     engine->notification_callback(&xnotification);
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/536

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> return E_OUTOFMEMORY;
>      }
>
> -    lookup = HeapAlloc(GetProcessHeap(), 0, sizeof(*lookup));
> -    if (!lookup)
> +    hr = wrapper_add_entry(This, fwb, &wb->IXACT3WaveBank_iface);
> +    if (FAILED(hr))
>      {
>          FACTWaveBank_Destroy(fwb);
>          HeapFree(GetProcessHeap(), 0, wb);
>          ERR("Failed to allocate wrapper_lookup!\n");

There is probably little need to have this error message, given that's in the callback already.

Same below and in later patches.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5055

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> +{
> +    struct wrapper_lookup *lookup;
> +    UINT ret;
> +
> +    lookup = HeapAlloc(GetProcessHeap(), 0, sizeof(*lookup));
> +    if (!lookup)
> +    {
> +        ERR("Failed to allocate wrapper_lookup!\n");
> +        return E_OUTOFMEMORY;
> +    }
> +    lookup->fact = fact;
> +    lookup->xact = xact;
> +
> +    EnterCriticalSection(&engine->wb_wrapper_lookup_cs);
> +    ret = wine_rb_put(&engine->wb_wrapper_lookup, lookup->fact, &lookup->entry);
> +    LeaveCriticalSection(&engine->wb_wrapper_lookup_cs);
> I used the `wb_` prefix for "WaveBank" because I expected to use different locks and RB trees for different object types. Since you are using the same lock and tree for all object types (which is probably more sensible), maybe the prefix could be removed.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5057

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> +{
> +    struct wrapper_lookup *lookup;
> +    UINT ret;
> +
> +    lookup = HeapAlloc(GetProcessHeap(), 0, sizeof(*lookup));
> +    if (!lookup)
> +    {
> +        ERR("Failed to allocate wrapper_lookup!\n");
> +        return E_OUTOFMEMORY;
> +    }
> +    lookup->fact = fact;
> +    lookup->xact = xact;
> +
> +    EnterCriticalSection(&engine->wb_wrapper_lookup_cs);
> +    ret = wine_rb_put(&engine->wb_wrapper_lookup, lookup->fact, &lookup->entry);
> +    LeaveCriticalSection(&engine->wb_wrapper_lookup_cs);
> The `wb_` prefix can be removed. It standed for "WaveBank", but it's more sensible to use the same lock and RB tree for all object types as you are doing.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5058

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> +{
> +    struct wrapper_lookup *lookup;
> +    UINT ret;
> +
> +    lookup = HeapAlloc(GetProcessHeap(), 0, sizeof(*lookup));
> +    if (!lookup)
> +    {
> +        ERR("Failed to allocate wrapper_lookup!\n");
> +        return E_OUTOFMEMORY;
> +    }
> +    lookup->fact = fact;
> +    lookup->xact = xact;
> +
> +    EnterCriticalSection(&engine->wb_wrapper_lookup_cs);
> +    ret = wine_rb_put(&engine->wb_wrapper_lookup, lookup->fact, &lookup->entry);
> +    LeaveCriticalSection(&engine->wb_wrapper_lookup_cs);
> The `wb_` prefix can be removed. It standed for "WaveBank", but it's more sensible to use the same lock and RB tree for all object types as you are doing.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5059

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> XACT_GETOVERLAPPEDRESULT_CALLBACK pGetOverlappedResult;
>      XACT_NOTIFICATION_CALLBACK notification_callback;
>
> -    void *wb_prepared_context;
> -    void *wb_destroyed_context;
> +    void *contexts[17];

Is this magic constant a good idea? Unfortunately we don't have a maximum constant for the notification types, but I'd rather go with `XACTNOTIFICATIONTYPE_MAX` and tolerate the wasted memory than leave a magic constant.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5060

[Zebediah Figura]

On 7/27/22 08:28, Giovanni Mascellani (@giomasce) wrote:
> Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> > XACT_GETOVERLAPPEDRESULT_CALLBACK pGetOverlappedResult;
> >       XACT_NOTIFICATION_CALLBACK notification_callback;
> >
> > -    void *wb_prepared_context;
> > -    void *wb_destroyed_context;
> > +    void *contexts[17];
>
> Is this magic constant a good idea? Unfortunately we don't have a maximum constant for the notification types, but I'd rather go with `XACTNOTIFICATIONTYPE_MAX` and tolerate the wasted memory than leave a magic constant.

Or we could just define our own constant.

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> TRACE("(%p)->(%p)\n", This, pNotificationDesc);
>
> -    if (pNotificationDesc->type == XACTNOTIFICATIONTYPE_WAVEBANKPREPARED)
> -        This->wb_prepared_context = pNotificationDesc->pvContext;
> -    else if (pNotificationDesc->type == XACTNOTIFICATIONTYPE_WAVEBANKDESTROYED)
> -        This->wb_destroyed_context = pNotificationDesc->pvContext;
> -
>      unwrap_notificationdesc(&fdesc, pNotificationDesc);
> +    This->contexts[pNotificationDesc->type - 1] = pNotificationDesc->pvContext;

This and similar accesses are undefined behavior when the caller requests an invalid notification type, and it doesn't seem we have any protection against that. What does happen on Windows when an invalid notification type is requested?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5061

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> TRACE("(%p)->(%p)\n", This, pNotificationDesc);
>
> unwrap_notificationdesc(&fdesc, pNotificationDesc);
> +    This->contexts[pNotificationDesc->type - 1] = pNotificationDesc->pvContext;

Does it make sense to store this pointer? We're unregistering notifications, so we don't expect this context to be used anymore (until the next `RegisterNotification()` call, but then a new pointer will be given). Does the caller have any visibility into the pointer stored here?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5062

[Giovanni Mascellani (＠giomasce)]

On Wed Jul 27 13:32:40 2022 +0000, Giovanni Mascellani wrote:
> Does it make sense to store this pointer? We're unregistering
> notifications, so we don't expect this context to be used anymore (until
> the next `RegisterNotification()` call, but then a new pointer will be
> given). Does the caller have any visibility into the pointer stored here?
> Thinking better, it might be the case that the pointer passed here is somewhat visible to the caller, given that the XACT notification system is quite convoluted and notifications can be registered and unregistered for individual objects. It wouldn't be bad to have some tests showing this behaviour on Windows, though.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5081

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about dlls/xactengine3_7/xact_dll.c:
> +        xnotification.wave.cueIndex = notification->wave.cueIndex;
> +        xnotification.wave.pCue = wrapper_find_entry(engine, notification->wave.pCue);
> +        xnotification.wave.pSoundBank = wrapper_find_entry(engine, notification->wave.pSoundBank);
> +#if XACT3_VER >= 0x0205
> +        xnotification.wave.pWave = wrapper_find_entry(engine, notification->wave.pWave);
> +#endif
> +        xnotification.wave.pWaveBank = wrapper_find_entry(engine, notification->wave.pWaveBank);
> +    }
> +    else if (notification->type == XACTNOTIFICATIONTYPE_CUEPLAY ||
> +             notification->type == XACTNOTIFICATIONTYPE_CUEPREPARED ||
> +             notification->type == XACTNOTIFICATIONTYPE_CUESTOP ||
> +             notification->type == XACTNOTIFICATIONTYPE_CUEDESTROYED)
> +    {
> +        xnotification.cue.pCue = wrapper_find_entry(engine, notification->cue.pCue);
> +        xnotification.cue.cueIndex = notification->cue.cueIndex;
> +        xnotification.cue.pSoundBank = wrapper_find_entry(engine, notification->cue.pSoundBank);
> It doesn't look like all these notifications are implemented in FAudio, though. For example, I cannot find in the code where `CUEPREPARED` would be notified. Here too it wouldn't be bad to have some tests.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/536#note_5082