-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
frontend: Refactor and update Audio Mixer #12735
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
base: master
Are you sure you want to change the base?
Conversation
7e94625 to
e9f1da9
Compare
|
Small request for this redesign, if it's feasible and easy enough: can you add one more checkbox to the menu to show sources that are inactive, but present on the previewed screen? Of course it only makes sense in the studio mode, but it would be a great help when you need to adjust levels of a source that will become active with next transition, but without having to find it in the list of all available sources. |
895cced to
95167b7
Compare
a64d278 to
f506e3e
Compare
|
This PR causes ugly pixelated rendering of VolumeMeter text on Linux at 125% DPI scale (and probably Windows as well).
I think you're rendering the text off-screen ( Looking at the QPixmap docs, I found https://doc.qt.io/qt-6/qpainter.html#drawing-high-resolution-versions-of-pixmaps-and-images:
One interesting possible strategy is "multiply size by DPI, round to nearest pixel, then set the QPixmap DPR to the screen, then blit to the widget". If you can convince Qt to not scale it at all (haven't tested), you could get exact rendering, possibly with one pixel cut off or missing at the edges :( Still waiting for Qt to add https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserverEntry/devicePixelContentBoxSize 🦗 VS Code faced a similar problem with their terminal at xtermjs/xterm.js#2662 (comment) and xtermjs/xterm.js#3926 |
Thanks for catching this. I'm travelling this week but hopefully the fix will be as simple as adjusting the Pixmap size to be
|
81ec88b to
68e30cd
Compare
|
I’m using HTML in the source names, but it’s no longer being reflected in the audio mixer. Example: I learned how to add color to source names from this video: |
gxalpha
left a comment
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.
things I noticed when scrolling through. Definitely make sure to revert the obs-browser changes.
God I hate that rebasing does that |
68e30cd to
b29ffee
Compare
PatTheMav
left a comment
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.
First round of review, will continue over the next few days.
c2a7677 to
5f23356
Compare
8cf2d01 to
76fd9ae
Compare
f8e40bd to
de49c0c
Compare
de49c0c to
99e9a12
Compare
PatTheMav
left a comment
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.
During testing I found that there are two ways to toggle the display of hidden sources: Either via the Options menu or via the hidden source counter.
That itself is a bit of a UX smell, because there should be a single, canonical, way to do this, not multiple ways. But because there are multiple ways, this creates a bidirectional state binding between both of these controls and at least in my tests, toggling the display of hidden sources via the counter label does not also uncheck the checkbox in the "Options" submenu, even though it should.
So either this binding needs to be made truly bidirectional, or the duplication in functionality is resolved.
Also the on_actionMixerToolbarMenu_triggered function never seems to run in my tests, even though I get the context menu both by clicking on the "Options" button or right-clicking into the mixer area. How is this menu triggered?
Similarly debugHideControls exists, but doesn't seem to be hooked up to anything?
| } | ||
| }; | ||
|
|
||
| QMetaObject::invokeMethod(App(), "Exec", Qt::QueuedConnection, Q_ARG(VoidFunc, msgBox)); |
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.
Does this execute exec recursively, effectively creating 2 nested event loops just for a message box, or does Exec do something else (hence the capitalisation)?
In general, use of exec is a big code smell in Qt (they discourage its use explicitly) and we should move away from it rather than double down on its use.
|
|
||
| obs_fader_attach_source(obs_fader, source); | ||
|
|
||
| /* Call volume changed once to init the slider position and label */ |
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.
| /* Call volume changed once to init the slider position and label */ | |
| // Call volume changed once to init the slider position and label |
| buttonLayout->setContentsMargins(0, 0, 0, 0); | ||
| buttonLayout->setSpacing(0); | ||
|
|
||
| //buttonLayout->addItem(new QSpacerItem(0, 0, QSizePolicy::Minimum, QSizePolicy::MinimumExpanding)); |
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.
Refactor or remove.
| obs_source_set_muted(source, mute); | ||
|
|
||
| if (!mute && unassigned) { | ||
| // muteButton->setChecked(true); |
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.
Refactor or remove.
|
|
||
| if (!mute && unassigned) { | ||
| // muteButton->setChecked(true); | ||
| /* Show notice about the source no being assigned to any tracks */ |
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.
| /* Show notice about the source no being assigned to any tracks */ | |
| // Show notice about the source no being assigned to any tracks |
| float levelTotal; | ||
| float levelCount; |
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.
According to clang these two are unused and should be removed (dead code).
| return; | ||
| } | ||
|
|
||
| auto getPreviewSources = [this](obs_scene_t *, obs_sceneitem_t *item) /* -- */ |
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.
What's the purpose of the comment at the end of the line?
|
|
||
| using getPreviewSources_t = decltype(getPreviewSources); | ||
|
|
||
| auto previewEnum = [](obs_scene_t *scene, obs_sceneitem_t *item, void *data) -> bool /* -- */ |
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.
What's the purpose of the comment at the end of the line?
|
|
||
| void AudioMixer::obsSourceActivated(void *data, calldata_t *params) | ||
| { | ||
| obs_source_t *source = (obs_source_t *)calldata_ptr(params, "source"); |
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.
| obs_source_t *source = (obs_source_t *)calldata_ptr(params, "source"); | |
| obs_source_t *source = static_cast<obs_source_t *>(calldata_ptr(params, "source")); |
Don't use unsafe C-style casts in C++ code, always use C++ cast operations.
Applies to all C-style casts in this file.
|
|
||
| #include "moc_AudioMixer.cpp" | ||
|
|
||
| static constexpr int GLOBAL_SOURCE_TOTAL = 6; |
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.
The static qualifier doesn't make much sense here: The variable is defined in the global scope, which gives it static storage duration by default, but it's also qualified as constexpr which makes it implicitly const and thus has internal linkage by default.
See also https://www.learncpp.com/cpp-tutorial/scope-duration-and-linkage-summary/.
|
|
||
| void VolumeControl::renameSource() | ||
| { | ||
| QAction *action = reinterpret_cast<QAction *>(sender()); |
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'm afraid that I kinda know the answer already, but: Is it really necessary for the VolumeControl class to take care of all these arcana related to renaming a source? Isn't this code implemented somewhere else already (e.g. the source dock), meaning that if something changes about the process to rename a source that we need to remember that the VolumeControl also renames sources and thus needs to be updated as well?




Description
Moves a bunch of logic out of OBSBasic into a central AudioMixer class, refactors a bunch of code to clean up, and adds a pile of new functionality to the mixer.
Adds a new MenuCheckbox widget for having a checkbox toggle inside a QMenu that does not close the menu after clicking.
The primary addition in this PR is a new audio monitoring toggle button in the mixer itself. Other features have also been added such as the ability to show Hidden sources, Inactive sources, adjust how they get sorted, and an easier way to toggle the layout.
PR also depends on #12734 for icon coloring.
Motivation and Context
With the fixes made as part of #12175, it is now far more reasonable for us to expose audio monitoring more directly to users. It is often very desirable to be able to toggle on monitoring for things like Media Sources or some Video Devices such as capture cards.
I elected to also clean up the Audio Mixer code a bunch while working on this feature addition, and add some other additional features and UI adjustments.
There is more that I want to do and clean up still but I'm trying to keep this PR at least moderately reasonable.
How Has This Been Tested?
Tested muting and monitoring on multiple audio sources both via the Mixer dock, the Advanced Audio Properties window, as well as a separate Audio Mixer window utilizing the same widget, to test that things are properly encapsulated and not kept in any local widget state.
Types of changes
Checklist: