Skip to content
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

Merged
merged 8 commits into from
Jan 1, 2025

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Nov 15, 2024

Based on #13945

This addresses a todo note in the AudioUnitBackend implementation:

// TODO: Should we perform this asynchronously (e.g. using Qt's
// threading or GCD)?

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:

  • It's faster. Since all effect manifests are to be loaded synchronously while Mixxx is starting up, doing this concurrently for all plugins potentially improves startup time.
  • It's more robust. Currently a bad plugin implementation could crash Mixxx during startup. By doing this out-of-process and adding a timeout, an unstable plugin shouldn't prevent Mixxx from starting up properly.

Additionally, we now load plugins of the category kAudioUnitType_MusicEffect (in addition to kAudioUnitType_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.

@Swiftb0y
Copy link
Member

I'm unsure who can properly review this. @m0dB has some apple-specific experience, right?

@m0dB
Copy link
Contributor

m0dB commented Nov 15, 2024

Yes, I am very familiar with AU and I will review this when I find the time.

@fwcd fwcd changed the base branch from main to 2.5 November 16, 2024 15:13
@fwcd
Copy link
Member Author

fwcd commented Nov 16, 2024

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.

@Swiftb0y
Copy link
Member

Not sure if its worth the regression risk. I'll leave that decision to @m0dB

@fwcd
Copy link
Member Author

fwcd commented Nov 16, 2024

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 😅

@m0dB
Copy link
Contributor

m0dB commented Nov 17, 2024

Apart from my objection against the use of a sleep in a while loop instead of a wait condition, this LGTM.

@fwcd
Copy link
Member Author

fwcd commented Nov 17, 2024

The test failures confuse me. Did I miss some race condition somewhere? I'm not familiar enough with QWaitCondition to say whether this usage is sound, would appreciate if someone could review that.

Screenshot

image

@fwcd
Copy link
Member Author

fwcd commented Nov 17, 2024

I don't quite understand why we get sporadic segfaults with the QWaitCondition-based implementation. Unless I misunderstand how the wait condition is supposed to be used, the only explanation I see is that the wait condition doesn't like being invoked from some possibly GCD-managed background thread where our callback may be invoked, but that would kind of defeat the purpose of the wait condition and its associated mutex in the first place.

Any ideas? The sleep loop seemed to work more reliably, maybe it would be less risky to stick to that for now?

@m0dB
Copy link
Contributor

m0dB commented Nov 17, 2024

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.

@m0dB
Copy link
Contributor

m0dB commented Nov 17, 2024

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.

@m0dB
Copy link
Contributor

m0dB commented Nov 17, 2024

Hm, mixxx-test freezes on me at:

[ RUN      ] AutoDJProcessorTest.FullIntroOutro_LongerIntro

GMOCK WARNING:
Uninteresting mock function call - returning directly.
    Function call: emitAutoDJStateChanged(0)
NOTE: You can safely ignore the above warning unless this call should not happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean to enforce the call.  See https://github.com/google/googletest/blob/main/docs/gmock_cook_book.md#knowing-when-to-expect-useoncall for details.

GMOCK WARNING:
Uninteresting mock function call - returning directly.
    Function call: emitAutoDJStateChanged(1)
NOTE: You can safely ignore the above warning unless this call should not happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean to enforce the call.  See https://github.com/google/googletest/blob/main/docs/gmock_cook_book.md#knowing-when-to-expect-useoncall for details.

Have to force quit.

@m0dB
Copy link
Contributor

m0dB commented Nov 17, 2024

Same happens with your branch... If I skip the hanging test, it hangs on

[ RUN      ] ControllerScriptEngineLegacyTest.trigger
 test:warning [Main] "Script tried to connect to ([Test], co) using `connectControl` which is deprecated. Use `makeConnection` instead!"

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.

@Swiftb0y
Copy link
Member

the only explanation I see is that the wait condition doesn't like being invoked from some possibly GCD-managed background thread where our callback may be invoked,

Slightly related. I once hit a very nasty bug when calling pthread_cond_signal from a unix signal handler. Someone else fortunately figured out that that is apparently undefined behavior (though only resulted in a deadlock in 1/1000 cases). We fixed it by spinning on an atomic shared variable instead (which I'm not recommending here). I don't kow how the GCD stuff is implemented, but something similar may be the case here.

@fwcd
Copy link
Member Author

fwcd commented Nov 19, 2024

I suppose there must be a logic error somewhere. The dispatch group-based implementation seems to have the same issue as the QWaitCondition-based implementation.

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).

fwcd added a commit to fwcd/mixxx that referenced this pull request Nov 29, 2024
@fwcd
Copy link
Member Author

fwcd commented Nov 29, 2024

I have split out the shared pointer-refactoring into #13945, which IMO is an important bug fix for 2.5, and rebased the other PRs, so we can focus on the async initialization here and on GUIs in #13888, respectively.

fwcd added a commit to fwcd/mixxx that referenced this pull request Nov 29, 2024
fwcd added a commit to fwcd/mixxx that referenced this pull request Nov 29, 2024
@fwcd fwcd force-pushed the au-music-effects branch 2 times, most recently from aa2c35d to 711da8c Compare November 29, 2024 14:08
@m0dB
Copy link
Contributor

m0dB commented Nov 30, 2024

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...

fwcd added a commit to fwcd/mixxx that referenced this pull request Nov 30, 2024
fwcd added a commit to fwcd/mixxx that referenced this pull request Nov 30, 2024
@JoergAtGithub
Copy link
Member

@fwcd Please rebase to main and fix the merge conflicts

@JoergAtGithub
Copy link
Member

LGTM! Thank you!

@JoergAtGithub JoergAtGithub merged commit 6e41513 into mixxxdj:main Jan 1, 2025
13 checks passed
@fwcd fwcd deleted the au-music-effects branch January 2, 2025 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants