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

Open
wants to merge 5 commits into
base: 2.5
Choose a base branch
from

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Nov 15, 2024

Based on #13890

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.

Comment on lines +132 to +133
if (dispatch_group_wait(group,
dispatch_time(DISPATCH_TIME_NOW, TIMEOUT_MS * 1000000)) ==
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks reasonably correct to me according to the apple docs. A QWaitCondition would also just essentially wait, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think so. Implementing the other wait with a GCD group instead of a QWaitCondition might btw be worth a try. Since the async handler is likely already invoked on some GCD queue, we'll probably run into less unexpected things there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, my bad. I thought this was the place where the earlier comments where referring to, not the QThread::sleep in AudioUnitManifest.

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

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.

3 participants