[PATCH 1/2] xaudio2: Fix support for FAudio 19.06 and newer
From: Ethan Lee <elee(a)codeweavers.com> Signed-off-by: Ethan Lee <elee(a)codeweavers.com> --- dlls/xaudio2_7/x3daudio.c | 4 ++++ dlls/xaudio2_7/xapo.c | 2 ++ dlls/xaudio2_7/xaudio_dll.c | 6 +++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/dlls/xaudio2_7/x3daudio.c b/dlls/xaudio2_7/x3daudio.c index ee3367e092..f08a425ca1 100644 --- a/dlls/xaudio2_7/x3daudio.c +++ b/dlls/xaudio2_7/x3daudio.c @@ -51,8 +51,12 @@ HRESULT CDECL X3DAudioInitialize(UINT32 chanmask, float speedofsound, X3DAUDIO_HANDLE handle) { TRACE("0x%x, %f, %p\n", chanmask, speedofsound, handle); +#if FAUDIO_ABI_VERSION >= 1 + return F3DAudioInitialize8(chanmask, speedofsound, handle); +#else F3DAudioInitialize(chanmask, speedofsound, handle); return S_OK; +#endif } #endif /* XAUDIO2_VER >= 8 */ diff --git a/dlls/xaudio2_7/xapo.c b/dlls/xaudio2_7/xapo.c index 9788cb38e8..0b2a5932a5 100644 --- a/dlls/xaudio2_7/xapo.c +++ b/dlls/xaudio2_7/xapo.c @@ -330,8 +330,10 @@ static inline HRESULT get_fapo_from_clsid(REFCLSID clsid, FAPO **fapo) return FAPOFX_CreateFXWithCustomAllocatorEXT( (const FAudioGUID*) clsid, fapo, +#if FAUDIO_ABI_VERSION < 1 NULL, 0, +#endif XAudio_Internal_Malloc, XAudio_Internal_Free, XAudio_Internal_Realloc diff --git a/dlls/xaudio2_7/xaudio_dll.c b/dlls/xaudio2_7/xaudio_dll.c index cbbedc4f7f..0f7ae36e66 100644 --- a/dlls/xaudio2_7/xaudio_dll.c +++ b/dlls/xaudio2_7/xaudio_dll.c @@ -1796,9 +1796,13 @@ static HRESULT WINAPI IXAudio2Impl_CommitChanges(IXAudio2 *iface, { IXAudio2Impl *This = impl_from_IXAudio2(iface); - TRACE("(%p)->(0x%x): stub!\n", This, operationSet); + TRACE("(%p)->(0x%x)\n", This, operationSet); +#if FAUDIO_ABI_VERSION >= 1 + return FAudio_CommitChanges(This->faudio, operationSet); +#else return FAudio_CommitChanges(This->faudio); +#endif } static void WINAPI IXAudio2Impl_GetPerformanceData(IXAudio2 *iface, -- 2.21.0
From: Ethan Lee <elee(a)codeweavers.com> Signed-off-by: Ethan Lee <elee(a)codeweavers.com> --- dlls/xaudio2_7/xaudio_dll.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/xaudio2_7/xaudio_dll.c b/dlls/xaudio2_7/xaudio_dll.c index 0f7ae36e66..cef15eacef 100644 --- a/dlls/xaudio2_7/xaudio_dll.c +++ b/dlls/xaudio2_7/xaudio_dll.c @@ -1810,7 +1810,7 @@ static void WINAPI IXAudio2Impl_GetPerformanceData(IXAudio2 *iface, { IXAudio2Impl *This = impl_from_IXAudio2(iface); - TRACE("(%p)->(%p): stub!\n", This, pPerfData); + TRACE("(%p)->(%p)\n", This, pPerfData); FAudio_GetPerformanceData(This->faudio, (FAudioPerformanceData *)pPerfData); } @@ -1821,7 +1821,7 @@ static void WINAPI IXAudio2Impl_SetDebugConfiguration(IXAudio2 *iface, { IXAudio2Impl *This = impl_from_IXAudio2(iface); - TRACE("(%p)->(%p, %p): stub!\n", This, pDebugConfiguration, pReserved); + TRACE("(%p)->(%p, %p)\n", This, pDebugConfiguration, pReserved); FAudio_SetDebugConfiguration(This->faudio, (FAudioDebugConfiguration *)pDebugConfiguration, pReserved); } -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=51691 Your paranoid android. === debian9 (32 bit report) === Report errors: The report seems to have been truncated === debian9 (32 bit WoW report) === Report errors: The report seems to have been truncated
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=51690 Your paranoid android. === debian9 (32 bit report) === Report errors: The report seems to have been truncated === debian9 (32 bit WoW report) === Report errors: The report seems to have been truncated
elee(a)codeweavers.com writes:
diff --git a/dlls/xaudio2_7/x3daudio.c b/dlls/xaudio2_7/x3daudio.c index ee3367e092..f08a425ca1 100644 --- a/dlls/xaudio2_7/x3daudio.c +++ b/dlls/xaudio2_7/x3daudio.c @@ -51,8 +51,12 @@ HRESULT CDECL X3DAudioInitialize(UINT32 chanmask, float speedofsound, X3DAUDIO_HANDLE handle) { TRACE("0x%x, %f, %p\n", chanmask, speedofsound, handle); +#if FAUDIO_ABI_VERSION >= 1 + return F3DAudioInitialize8(chanmask, speedofsound, handle); +#else F3DAudioInitialize(chanmask, speedofsound, handle); return S_OK; +#endif
Was it really necessary to break binary compatibility so soon, and force us to maintain ugly ifdefs and multiple builds of the library? It looks like it wouldn't be hard to avoid it. -- Alexandre Julliard julliard(a)winehq.org
All of the changes are relatively easy to avoid until you get to CommitChanges, which was completely wrong to begin with. It was either wait and let more software become dependent on the incorrect function signature or break right away and get the unavoidable pains out of the way sooner than later. Either way, these ifdefs were going to happen. Breaking compatibility for one single function seemed really awful, so I tried my best to update the rest of the API to be more consistent across the board to justify the breakage a bit more (for example, properly making 2.8+ functions with the ‘8’ suffix). My hope is that this will keep us from having to break it again in the future. Other than CommitChanges, the rest of the changes aren’t terribly interesting, except for F3DAudioInitialize8 which now returns an error code if the input parameters are invalid, which is accurate for 2.8 and higher. -Ethan
On May 1, 2019, at 1:57 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
elee(a)codeweavers.com writes:
diff --git a/dlls/xaudio2_7/x3daudio.c b/dlls/xaudio2_7/x3daudio.c index ee3367e092..f08a425ca1 100644 --- a/dlls/xaudio2_7/x3daudio.c +++ b/dlls/xaudio2_7/x3daudio.c @@ -51,8 +51,12 @@ HRESULT CDECL X3DAudioInitialize(UINT32 chanmask, float speedofsound, X3DAUDIO_HANDLE handle) { TRACE("0x%x, %f, %p\n", chanmask, speedofsound, handle); +#if FAUDIO_ABI_VERSION >= 1 + return F3DAudioInitialize8(chanmask, speedofsound, handle); +#else F3DAudioInitialize(chanmask, speedofsound, handle); return S_OK; +#endif
Was it really necessary to break binary compatibility so soon, and force us to maintain ugly ifdefs and multiple builds of the library? It looks like it wouldn't be hard to avoid it.
-- Alexandre Julliard julliard(a)winehq.org
On 5/1/19 9:06 PM, Ethan Lee wrote:
All of the changes are relatively easy to avoid until you get to CommitChanges, which was completely wrong to begin with. It was either wait and let more software become dependent on the incorrect function signature or break right away and get the unavoidable pains out of the way sooner than later. Either way, these ifdefs were going to happen. Breaking compatibility for one single function seemed really awful, so I tried my best to update the rest of the API to be more consistent across the board to justify the breakage a bit more (for example, properly making 2.8+ functions with the ‘8’ suffix). My hope is that this will keep us from having to break it again in the future. The problem is that distros will never catch up with changes like that in reasonable timeframe, and this change gives another reason to avoid packaging this library.
Other than CommitChanges, the rest of the changes aren’t terribly interesting, except for F3DAudioInitialize8 which now returns an error code if the input parameters are invalid, which is accurate for 2.8 and higher. Can you verify arguments in wine code instead? I think it makes sense to keep faudio independent of various xaudio versions specifics, if doesn't depend already.
-Ethan
On May 1, 2019, at 1:57 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
elee(a)codeweavers.com writes:
diff --git a/dlls/xaudio2_7/x3daudio.c b/dlls/xaudio2_7/x3daudio.c index ee3367e092..f08a425ca1 100644 --- a/dlls/xaudio2_7/x3daudio.c +++ b/dlls/xaudio2_7/x3daudio.c @@ -51,8 +51,12 @@ HRESULT CDECL X3DAudioInitialize(UINT32 chanmask, float speedofsound, X3DAUDIO_HANDLE handle) { TRACE("0x%x, %f, %p\n", chanmask, speedofsound, handle); +#if FAUDIO_ABI_VERSION >= 1 + return F3DAudioInitialize8(chanmask, speedofsound, handle); +#else F3DAudioInitialize(chanmask, speedofsound, handle); return S_OK; +#endif Was it really necessary to break binary compatibility so soon, and force us to maintain ugly ifdefs and multiple builds of the library? It looks like it wouldn't be hard to avoid it.
-- Alexandre Julliard julliard(a)winehq.org
Ethan Lee <elee(a)codeweavers.com> writes:
All of the changes are relatively easy to avoid until you get to CommitChanges, which was completely wrong to begin with. It was either wait and let more software become dependent on the incorrect function signature or break right away and get the unavoidable pains out of the way sooner than later. Either way, these ifdefs were going to happen. Breaking compatibility for one single function seemed really awful, so I tried my best to update the rest of the API to be more consistent across the board to justify the breakage a bit more (for example, properly making 2.8+ functions with the ‘8’ suffix). My hope is that this will keep us from having to break it again in the future.
You can add the fixed function with a new name, and later on deprecate the broken one. Breaking compatibility this way is bad, particularly given the current packaging situation. What's even worse is that old code no longer builds with the new headers, so once you upgrade the library it becomes impossible to do bisects and the like. Please rethink this change. -- Alexandre Julliard julliard(a)winehq.org
I could add F3DAudioInitialize8 separately, but it would mean everybody has to update to 19.05.0x for that def to go away, so the packaging situation doesn’t improve much there. I could possibly do something like FAPOFX_Create7 (well, it could be 5 since it’s xapofx1_5, but it could also be 7 since it’s the XAudio 2.7 spec…?) but that just makes FAudio’s API really confusing, since we would have some of the spec targeting 2.7 with 2.8+ as extension methods and other parts targeting 2.8 with 2.7- as extension methods. The real killer is CommitChanges, which is for sure stuck since it was an entirely bogus function and the old version should not exist at all. I could _maybe_ write in something like... /* For build compatibility. This function was a stub to begin with, so do nothing. */ #define FAudio_CommitChanges(x) … but that only fixes compile compatibility. The ABI increment would thankfully catch it since, presumably, this would only be an issue if someone updated FAudio but not Wine, so the old Wine would still look for libFAudio.so.0, but that’s just moving the issue to a different spot. Same applies to FAPOFX_CreateFX. -Ethan
On May 1, 2019, at 2:31 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Ethan Lee <elee(a)codeweavers.com> writes:
All of the changes are relatively easy to avoid until you get to CommitChanges, which was completely wrong to begin with. It was either wait and let more software become dependent on the incorrect function signature or break right away and get the unavoidable pains out of the way sooner than later. Either way, these ifdefs were going to happen. Breaking compatibility for one single function seemed really awful, so I tried my best to update the rest of the API to be more consistent across the board to justify the breakage a bit more (for example, properly making 2.8+ functions with the ‘8’ suffix). My hope is that this will keep us from having to break it again in the future.
You can add the fixed function with a new name, and later on deprecate the broken one. Breaking compatibility this way is bad, particularly given the current packaging situation.
What's even worse is that old code no longer builds with the new headers, so once you upgrade the library it becomes impossible to do bisects and the like. Please rethink this change.
-- Alexandre Julliard julliard(a)winehq.org
Ethan Lee <elee(a)codeweavers.com> writes:
I could add F3DAudioInitialize8 separately, but it would mean everybody has to update to 19.05.0x for that def to go away, so the packaging situation doesn’t improve much there. I could possibly do something like FAPOFX_Create7 (well, it could be 5 since it’s xapofx1_5, but it could also be 7 since it’s the XAudio 2.7 spec…?) but that just makes FAudio’s API really confusing, since we would have some of the spec targeting 2.7 with 2.8+ as extension methods and other parts targeting 2.8 with 2.7- as extension methods. The real killer is CommitChanges, which is for sure stuck since it was an entirely bogus function and the old version should not exist at all. I could _maybe_ write in something like...
/* For build compatibility. This function was a stub to begin with, so do nothing. */ #define FAudio_CommitChanges(x)
… but that only fixes compile compatibility. The ABI increment would thankfully catch it since, presumably, this would only be an issue if someone updated FAudio but not Wine, so the old Wine would still look for libFAudio.so.0, but that’s just moving the issue to a different spot. Same applies to FAPOFX_CreateFX.
It does make life a bit more complicated, but that's the price to pay for backwards compatibility. You can't increment the ABI every time you find a bug. -- Alexandre Julliard julliard(a)winehq.org
On Wed, May 1, 2019 at 8:51 PM Ethan Lee <elee(a)codeweavers.com> wrote:
I could add F3DAudioInitialize8 separately, but it would mean everybody has to update to 19.05.0x for that def to go away, so the packaging situation doesn’t improve much there. I could possibly do something like FAPOFX_Create7 (well, it could be 5 since it’s xapofx1_5, but it could also be 7 since it’s the XAudio 2.7 spec…?) but that just makes FAudio’s API really confusing, since we would have some of the spec targeting 2.7 with 2.8+ as extension methods and other parts targeting 2.8 with 2.7- as extension methods. The real killer is CommitChanges, which is for sure stuck since it was an entirely bogus function and the old version should not exist at all. I could _maybe_ write in something like...
It's unfortunate that FAudio was released with bogus CommitChanges(). However, ABI compatibility shouldn't be broken that lightly. The right thing to do is to introduce a new version of the function with a different name. The old version of function could probably assume that OperationSet is set to FAUDIO_COMMIT_ALL.
Assuming an OperationSet would break timings for games that do separate sets, so that wouldn’t work - we’d have to keep it as a stub, probably. Any suggestions on a new name? CommitChangesReal? For the suffix idea my plan would end up being to add the new function, deprecate the old function, then upon removing the old function, add it back with the correct signature and then deprecate the badly-named functions. This would apply to FAPOFX_CreateFX as well. I’m also up for suggestions on when to remove the deprecated functions. -Ethan
On May 1, 2019, at 3:28 PM, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
On Wed, May 1, 2019 at 8:51 PM Ethan Lee <elee(a)codeweavers.com> wrote:
I could add F3DAudioInitialize8 separately, but it would mean everybody has to update to 19.05.0x for that def to go away, so the packaging situation doesn’t improve much there. I could possibly do something like FAPOFX_Create7 (well, it could be 5 since it’s xapofx1_5, but it could also be 7 since it’s the XAudio 2.7 spec…?) but that just makes FAudio’s API really confusing, since we would have some of the spec targeting 2.7 with 2.8+ as extension methods and other parts targeting 2.8 with 2.7- as extension methods. The real killer is CommitChanges, which is for sure stuck since it was an entirely bogus function and the old version should not exist at all. I could _maybe_ write in something like...
It's unfortunate that FAudio was released with bogus CommitChanges(). However, ABI compatibility shouldn't be broken that lightly. The right thing to do is to introduce a new version of the function with a different name. The old version of function could probably assume that OperationSet is set to FAUDIO_COMMIT_ALL.
On Wed, May 1, 2019 at 9:51 PM Ethan Lee <elee(a)codeweavers.com> wrote:
Any suggestions on a new name? CommitChangesReal? For the suffix idea my plan would end up being to add the new function, deprecate the old function, then upon removing the old function, add it back with the correct signature and then deprecate the badly-named functions. This would apply to FAPOFX_CreateFX as well. I’m also up for suggestions on when to remove the deprecated functions.
If you cannot come up with a new name. I would personally go with CommitChanges2(). Other APIs use similar convention, e.g. vkCreateRenderPass2() and ID3D11DeviceContext1::CopySubresourceRegion1().
The main conflict there is that we already use numbers to denote spec versions, for example F3DAudioInitialize8 for the XAudio 2.8 revision of X3DAudioInitialize. If it helps, the main thing I’m trying to figure out is what name best explains that it’s the correct function to use right now, but definitely not later. Something that sticks out horribly and will make everyone want to get rid of it. A few ideas I had: - FAudio_CommitChangesBADABI - FAudio_CommitChangesFIXME - FAudio_CommitChangesUSETHIS The idea is that we add the above, deprecate the old one, then for ABI version 1, fix the function signature of the original and deprecate the above function for ABI version 2. As for when those would release, I’m thinking the major version number would be enough time for everyone to adjust… I’m thinking FAudio 20.01 for version 1 and 21.01 for version 2. -Ethan
On May 2, 2019, at 3:26 AM, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
On Wed, May 1, 2019 at 9:51 PM Ethan Lee <elee(a)codeweavers.com> wrote:
Any suggestions on a new name? CommitChangesReal? For the suffix idea my plan would end up being to add the new function, deprecate the old function, then upon removing the old function, add it back with the correct signature and then deprecate the badly-named functions. This would apply to FAPOFX_CreateFX as well. I’m also up for suggestions on when to remove the deprecated functions.
If you cannot come up with a new name. I would personally go with CommitChanges2(). Other APIs use similar convention, e.g. vkCreateRenderPass2() and ID3D11DeviceContext1::CopySubresourceRegion1().
On 05/02/2019 09:15 AM, Ethan Lee wrote:
The main conflict there is that we already use numbers to denote spec versions, for example F3DAudioInitialize8 for the XAudio 2.8 revision of X3DAudioInitialize.
If it helps, the main thing I’m trying to figure out is what name best explains that it’s the correct function to use right now, but definitely not later. Something that sticks out horribly and will make everyone want to get rid of it. A few ideas I had:
- FAudio_CommitChangesBADABI - FAudio_CommitChangesFIXME - FAudio_CommitChangesUSETHIS
The idea is that we add the above, deprecate the old one, then for ABI version 1, fix the function signature of the original and deprecate the above function for ABI version 2.
The idea of backwards compatibility is that a program should still be able to compile and run with any respective combination of old and new versions of the library. Renaming the old function breaks this. It also means that you want to load the new function at runtime rather than assuming it'll be present if compile-time support is there. As for naming convention, a few suggestions are: * FAudio_CommitChanges_a (e.g. libdwarf), or FAudio_CommitChangesA if you insist on title case * FAudio_CommitChangesBySet * FAudio_CommitChanges_v2 * FAudio_CommitChanges_ex (or FAudio_CommitChangesEx, but this has the risk of colliding with a future function Microsoft adds) Personally I'm a fan of a descriptive name like CommitChangesBySet(), but ultimately it's your choice.
participants (7)
-
Alexandre Julliard -
elee＠codeweavers.com -
Ethan Lee
-
Józef Kucia -
Marvin -
Nikolay Sivov -
Zebediah Figura