-
-
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
Effects: Add support for GUIs and reference implementation for Audio Unit #13888
base: main
Are you sure you want to change the base?
Conversation
I wonder why the frequencies from 10 to 12 kHz seem to be missing. For comparison, here's some white noise in both Logic and Mixxx, both viewed through the TDR Prism spectrum analyzer:
Notably, this negative spike does not show up when opening the recorded mix in Logic: ![]() I wonder if there's something wrong in the effect implementation (passing the audio buffer in some strange way) or whether there's something elsewhere in the Mixxx engine that could be causing this? Edit: Investing this, I've found that soloing the frequency band above the spike seems to be play sounds in a much lower frequency range... it feels like some kind of overflow, but I haven't dug too deeply into this to figure out the precise issue. This happens with different plugins too, so it's likely an issue somewhere between Mixxx and the Audio Unit integration. Edit 2: It has occured to me that this might simply be a sampling rate mismatch between Mixxx and the plugin (48 vs 41 kHz). That should roughly line up with the frequency spike, I remember seeing warnings about that in the console and since the Audio Unit only applies processing, it's usually not even audibly noticeable aside from frequencies being off in the analysis. We might thus have to look into resampling if Core Audio doesn't already provide facilities for this. Edit 3: Moved to #13901. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple surface level comments. We should consider creating more C++like friendly wrappers for the underlying native C calls...
std::unique_ptr<QDialog> createUI() override { | ||
QLabel* label = new QLabel(); | ||
label->setText("This effect has no UI"); | ||
|
||
QVBoxLayout* layout = new QVBoxLayout(); | ||
layout->addWidget(label); | ||
|
||
std::unique_ptr<QDialog> dialog = std::make_unique<QDialog>(); | ||
dialog->setWindowTitle("No UI available"); | ||
dialog->setLayout(layout); | ||
|
||
return dialog; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really needed? This should only be shown when a plugin has hasUI
is true even though it shouldn't be right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, I haven't gotten around to do that check yet and decided that this was nicer than an empty dialog 😄
I've also considered dropping the hasUI
and just returning an optional dialog here, but that wouldn't give the UI a chance to hide/style the UI button beforehand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... I think hiding the button if not available would be definitely good to have. I'd prefer to keep that if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One consequence of always providing a dialog is that we keep a dialog open when the user switches to an effect that doesn't provide a UI. On the one hand I think this could be nice (e.g. the user might arrange their effect UIs on the screen in a particular way and it might be nice to keep them open until explicitly closed). On the other hand, the blanket implementation doesn't really do anything useful, so we might as well also just close the UI, in which case the next effect with a UI will automatically open the dialog since the control object enabling the UI is still toggled on.
Don't really have a preference what the best/most intuitive behavior is here. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should follow the approach DAWs usually take. You can open as many effect UIs as there are effects and if you replace one effect with one that doesn't have a window or you remove the effect, the window just closes. Now you usually don't replace effects in DAWs directly (you insert a new one and remove the old one) but in mixxx we can replace an effect directly since we have slots. In that case we can consider replacing the old UI with the new UI directly (if the old effect had its UI open). Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you usually don't replace effects in DAWs directly
Depends on the DAW, at least in Logic the workflow is similar to Mixxx, where effects are essentially also slots with a dropdown.
in mixxx we can replace an effect directly since we have slots. In that case we can consider replacing the old UI with the new UI directly (if the old effect had its UI open). Wdyt?
Sounds reasonable. The way the API is designed currently, we'd still instantiate a new dialog (the new effect UI might want to set its own title/resizability/etc.), but I was thinking of maybe just carrying over the position manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on the DAW, at least in Logic the workflow is similar to Mixxx, where effects are essentially also slots with a dropdown.
ah yeah. My primary experience lies with bitwig and ableton...
but I was thinking of maybe just carrying over the position manually.
That may not be feasible on all platforms (not particularly relevant for MacOS I guess, but I don't think we could implement the same on wayland for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least on macOS, carrying over the position with QWidget::pos
and QWidget::move
seems to work well. The effect UI button is now hidden if the effect doesn't support a UI, but I've decided to keep the blanket dialog implementation and also to keep it open when switching to an effect that doesn't have a UI, since IMO it provides less suprising semantics:
- When switching between a number of different effects, it's convenient to keep the window open (and positioned) instead of having it close.
- The user may decide to map the corresponding control object to a MIDI controller, in which case we could still show the blanket dialog for effects that don't have a UI, to provide visual feedback
ad0f259
to
c3837ae
Compare
Marking this one as a draft until we figure out |
fbdc633
to
8dca13f
Compare
Instead of accessing the underlying NSView directly, which is unsupported and fragile (e.g. changing it from Qt may just wipe it out), we use the official `QWindow::fromWinId` and `QWidget::createWindowContainer` to wrap native views. This also lets us share/abstract out generic effect dialog logic, such as thinner titlebars or floating-ness across other platforms or effect backends.
This uses a smaller title bar on macOS and automatically causes the dialog to float above other windows.
Unfortunately, this seems to work too unreliably for plugins that implement their own draggable resize handle (apparently we don't get a frame changed notification if the outer window is already fixed and the plugin attempts to change the view frame?)
This handles the case where the user switches to the "empty" effect correctly, which previously would keep the button visible.
This fixes a race condition
If you consider "Update the parameter knobs on the Mixxx side after changing them from the GUI" out of scope of this PR, than please create an issue with target |
Based on #13887, fixes #13147
This PR adds a generic interface for effect backends for exposing GUIs in the form of
QDialog
s, along with a reference implementation for Audio Unit effects on macOS1:Effect UIs are toggled via a new button, implemented in LateNight/Palemoon (the only skin for now) and shown for effects that provide a UI:
These can be hidden via the skin settings to reduce UI clutter, if not needed:
To do
DlgEffect
abstraction that sets up a generic effect UI (a floating dialog)ui_shown
control object when the window is closed manuallyEffectManifest
now exposes ahasUI
property, which may be helpful for that.Potentially out-of-scope
Feedback welcome!
Footnotes
Support for UIs of other effect backends like LV2 (or VST plugins once we implement VST effect plugins #9019) could be added in future PRs as needed. ↩