-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AudioUnitBackend: Initialize parameters concurrently and load music effects too #13887
Conversation
I'm unsure who can properly review this. @m0dB has some apple-specific experience, right? |
Yes, I am very familiar with AU and I will review this when I find the time. |
c65f869
to
9cd422f
Compare
I've rebased this to 2.5 since I believe the improvements in stability would be worth having there too. The GUI stuff (#13888) can then go to 2.6. |
9cd422f
to
fa3473a
Compare
Not sure if its worth the regression risk. I'll leave that decision to @m0dB |
I ran into occasional crashes with ShaperBox at startup, the only reason why it didn't affect the release build was that "luckily" that plugin was marked as a music effect (and therefore not loaded). Since it could affect other (regular effect) plugins though, I think instantiating everything out-of-process would be worth it 😅 |
Apart from my objection against the use of a sleep in a while loop instead of a wait condition, this LGTM. |
fa3473a
to
6d9d281
Compare
The test failures confuse me. Did I miss some race condition somewhere? I'm not familiar enough with |
c24f979
to
46e8052
Compare
I don't quite understand why we get sporadic segfaults with the Any ideas? The sleep loop seemed to work more reliably, maybe it would be less risky to stick to that for now? |
Hm, let me quickly run this in the debugger if I can see what goes wrong. But yeah, let's stick with the while sleep loop otherwise. |
Please feel free to revert the QWaitCondition to the while sleep loop; I have a local branch with the version that crashes so I can debug this. |
Hm, mixxx-test freezes on me at:
Have to force quit. |
46e8052
to
6d79c2a
Compare
Same happens with your branch... If I skip the hanging test, it hangs on
I am going to try with 2.5, maybe something is messed up with my build environment. Edit: ok, 2.5 also hangs. Going to do a clean build and see if that helps. Sorry for the noise. |
Slightly related. I once hit a very nasty bug when calling |
I suppose there must be a logic error somewhere. The dispatch group-based implementation seems to have the same issue as the I have reset the branch back to the sleep loop implementation (for some reason GitHub is still processing the push, not sure why the updated commits don't show up). |
6d79c2a
to
b0fd89b
Compare
This fixes the issue described in mixxxdj#13887 (comment)
a421ea1
to
cb1d61f
Compare
This fixes the issue described in mixxxdj#13887 (comment)
This fixes the issue described in mixxxdj#13887 (comment)
aa2c35d
to
711da8c
Compare
Thanks for splitting out the shared pointer refactoring, I agree it makes sense to include in 2.5. As for this PR, it looks safe to me, and I don't see a reason not to include it in 2.5, except for the limited testing exposure this has had... |
This fixes the issue described in mixxxdj#13887 (comment)
711da8c
to
df9caf2
Compare
This fixes the issue described in mixxxdj#13887 (comment)
df9caf2
to
1ab3f05
Compare
@fwcd Please rebase to main and fix the merge conflicts |
This should make it easier to swap out the implementation in the future.
1ab3f05
to
9176536
Compare
LGTM! Thank you! |
Based on #13945
This addresses a todo note in the
AudioUnitBackend
implementation:mixxx/src/effects/backends/audiounit/audiounitbackend.mm
Lines 81 to 82 in 028e3ff
With this PR, Audio Units plugins are now instantiated concurrently and out-of-process during the initial load. This has a few advantages over the current approach of instantiating everything synchronously:
Additionally, we now load plugins of the category
kAudioUnitType_MusicEffect
(in addition tokAudioUnitType_Effect
), which are MIDI controllable, but otherwise function just like regular effects. This is useful, since it lets Mixxx discover plugins like ShaperBox, which are marked as music effects.