Re: [PATCH 1/2] xaudio2: Fix support for FAudio 19.06 and newer
I’m really glad you butted in because this is actually a very good point (sorry I didn’t see it earlier, I only get wine-devel messages as digests). So maybe what I can do is add something totally different like `FAudio_CommitOperationSet` and in the documentation deliberately put the phrase “CommitChanges” in so users wondering where that function went can go find it quickly. I still think my bogus CommitChanges needs to be scrapped somehow but if we can agree that any use of CommitChanges is a bug (yes, even retroactively), maybe I can just make it do something extremely unpleasant that still allows the program to run, but make it so anyone using it can see something is wrong (stderr messages maybe?). The main thing that is a concern is that CommitChanges affects a HUGE amount of how the library works, which is why I want to destroy it as soon as I possibly can. The next release of FAudio is going to support operation sets, meaning any call that uses an operation set value other than `NOW` is going to be put in a queue somewhere, and will not be fired until Commit is called. We can’t ignore the old CommitChanges because doing so means leaving a stub, meaning commands will never be fired and will halt lots of audio activity and basically introduce a massive memory leak into the program. We also can’t make any assumptions about a default operation set because that breaks timing accuracy, meaning data can go missing and crash, or deadlocks can occur if callbacks contain threading activity in them. So the question is: Can we agree that use of CommitChanges is an _application_ bug and tell users to migrate to CommitOperationSet, or do people expect me to somehow make CommitChanges not break as described above? If the former, cool, I can maybe make that work and deprecate the old function (I still really want to delete it at some point, though). If the latter, I’d rather people get mad at me over something that, based on the feedback presented, was a good decision compared to letting users deliberately do something that breaks programs for “compatibility”, if that phrase makes _any_ sense. -Ethan
Apologies for butting in, but I'm not sure this is any better. Code that's written to call FAudio_CommitChanges would expect this function to have the same specified behavior after an update. If code needs updating to use the new version, having a separate FAudio_CommitChangesBADAPI function seems a bit pointless; for the same effort of changing the call to restore old behavior, you could just ensure FAudio_CommitChanges is used correctly (unless the behavior change is that significant, in which case it really should be a new function).
When it comes to ABI changes, it should represent a massive change for the interface and be a complete refresh. If you have libfaudio.so.0 and libfaudio.so.1, for example, what happens if a process tries to pull in both (e.g. the main app links against libfaudio.so.1, but the app also links against libfoo.so which links to libfaudio.so.0)? Now you have two separate FAudio_CommitChanges symbols with two different behaviors, and something will break. I've actually run into this problem with SDL, where I link against SDL2 and SDL_sound, but on some distros SDL_sound links against SDL1; it all compiles and links fine, but misbehaves at runtime for no immediately obvious reason (if I didn't know SDL_sound might be linked against SDL1, while I link against SDL2, I'd be baffled as to why seemingly correct calls are just failing for some people and not others).
On 5/2/19 6:45 PM, Ethan Lee wrote:
I’m really glad you butted in because this is actually a very good point (sorry I didn’t see it earlier, I only get wine-devel messages as digests). So maybe what I can do is add something totally different like `FAudio_CommitOperationSet` and in the documentation deliberately put the phrase “CommitChanges” in so users wondering where that function went can go find it quickly. I still think my bogus CommitChanges needs to be scrapped somehow but if we can agree that any use of CommitChanges is a bug (yes, even retroactively), maybe I can just make it do something extremely unpleasant that still allows the program to run, but make it so anyone using it can see something is wrong (stderr messages maybe?).
With GCC/Clang, you can mark a function with __attribute__((deprecated)) in the header, which causes a compile-time warning if code tries to use it. I think it's also possible to supply a message, to give a specific reason and/or alternative. The code will still compile and thus maintain API compatibility, but the developer will know it's an outdated function it should now avoid using. The library can also still export some version of the function, maintaining ABI compatibility too. As for what to do regarding the old function's behavior, it ultimately depends. I'm not very familiar with XAudio, so I don't know exactly what the OperationSet parameter is for. Why wasn't it needed before, and why is it needed now? What did the function do before without it (I assume it worked in some capacity)?
It looks like both __attribute__((deprecated)) and __declspec(deprecated) both support messages, so I can do that as well. I’d still throw in a big fat error message in at runtime just in case somebody attempts to update FAudio by itself. The original CommitChanges was a stub since we didn’t support OperationSet at all; for all functions we treated all calls as if they submitted `COMMIT_NOW`, which runs the command immediately instead of queueing it for later. This mostly works, but has the aforementioned timing issues and doesn’t allow for proper batching (for example, playing two sources at once). Microsoft’s overview explains the feature pretty well: https://docs.microsoft.com/en-us/windows/desktop/xaudio2/xaudio2-operation-s... <https://docs.microsoft.com/en-us/windows/desktop/xaudio2/xaudio2-operation-sets> So by adding support for operation sets we’re not just changing behavior for one function, we’re changing behavior for a dozen functions, where any one function not falling in line can topple the whole thing over. This is also how we found the issue in the first place; because it was a stub and nobody was using operation sets, nobody noticed the bad signature. -Ethan
On May 2, 2019, at 11:13 PM, Chris Robinson <chris.kcat(a)gmail.com> wrote:
On 5/2/19 6:45 PM, Ethan Lee wrote:
I’m really glad you butted in because this is actually a very good point (sorry I didn’t see it earlier, I only get wine-devel messages as digests). So maybe what I can do is add something totally different like `FAudio_CommitOperationSet` and in the documentation deliberately put the phrase “CommitChanges” in so users wondering where that function went can go find it quickly. I still think my bogus CommitChanges needs to be scrapped somehow but if we can agree that any use of CommitChanges is a bug (yes, even retroactively), maybe I can just make it do something extremely unpleasant that still allows the program to run, but make it so anyone using it can see something is wrong (stderr messages maybe?).
With GCC/Clang, you can mark a function with __attribute__((deprecated)) in the header, which causes a compile-time warning if code tries to use it. I think it's also possible to supply a message, to give a specific reason and/or alternative. The code will still compile and thus maintain API compatibility, but the developer will know it's an outdated function it should now avoid using. The library can also still export some version of the function, maintaining ABI compatibility too.
As for what to do regarding the old function's behavior, it ultimately depends. I'm not very familiar with XAudio, so I don't know exactly what the OperationSet parameter is for. Why wasn't it needed before, and why is it needed now? What did the function do before without it (I assume it worked in some capacity)?
On 5/2/19 8:34 PM, Ethan Lee wrote:
It looks like both __attribute__((deprecated)) and __declspec(deprecated) both support messages, so I can do that as well. I’d still throw in a big fat error message in at runtime just in case somebody attempts to update FAudio by itself.
The original CommitChanges was a stub since we didn’t support OperationSet at all; for all functions we treated all calls as if they submitted `COMMIT_NOW`, which runs the command immediately instead of queueing it for later.
If it originally did nothing, then IMO it can continue to do nothing. Old code will behave as it has (calls get submitted with COMMIT_NOW, CommitChanges is a no-op), while new code will get a deprecation warning if it tried to use CommitChanges.
So by adding support for operation sets we’re not just changing behavior for one function, we’re changing behavior for a dozen functions, where any one function not falling in line can topple the whole thing over. This is also how we found the issue in the first place; because it was a stub and nobody was using operation sets, nobody noticed the bad signature.
If operation sets weren't supported before, and an app tried to use them anyway, it would already have issues. So I think it should be fine as long as CommitChanges does nothing when all previous OperationSets were COMMIT_NOW. In this case, it may actually be more compatible to have CommitChanges() act as CommitChangesSet(COMMIT_ALL). For old apps that only used COMMIT_NOW, as they should have without operation set support, CommitChanges will do nothing since there's no operation sets to commit. Old apps that tried to use operation sets will at least get them committed with CommitChanges, even if they're committed at the wrong time (which was already happening anyway). If an old app tried to use operation sets and never called CommitChanges, it's out of luck. For new apps, it can use operation sets and will be told to use CommitChangesSet or whatever, instead of CommitChanges.
participants (2)
-
Chris Robinson -
Ethan Lee