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

Test: fix mic Auto ducking #2634

Closed
wants to merge 2 commits into from
Closed

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 7, 2020

https://bugs.launchpad.net/mixxx/+bug/1737113
"Manual duck not works"
maybe also https://bugs.launchpad.net/mixxx/+bug/1662536
"Manual Ducking inconsistent behavior"

I'm sure this fix is not the most efficient solution as I'm not very much into the engine code.
First I'd like to check if I got the concept right, after that please comment how the code can be improved.

The Ducking Mode toolip says
Off: Do not reduce music volume
Auto: Automatically reduce music volume when microphones are in use. Adjust the amount the music volume is reduced with the Strength knob.
Manual: Reduce music volume by a fixed amount set by the Strength knob

So Auto mode should reduce the music volume if any Mic Talkover button is pressed?

First I looked at EngineTalkoverDucking::getGain because this is what is called from EngineMaster::process to adjust the master gain. With AUTO mode, this does some compression calculation that seems to depend on the Mic level but hardly returns values below 0.9, even if I crank up the Mic pregain to make the Mic input clip. So, hardly any effect on master gain.

double talkOvergain = 1.0;
if (!m_activeTalkoverChannels.isEmpty()) {
talkOvergain = m_pTalkoverDucking->getGain();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

m_activeTalkoverChannels is cleared and then populated in processChannels() which is called a few lines above, so I guess it's correct to use it for checking if any talkover is enabled?

case EngineTalkoverDucking::AUTO:
return calculateCompressedGain(numFrames);
case EngineTalkoverDucking::MANUAL:
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

... or should I use the switch like before, simply return m_pDuckStrength->get() for Auto and Manual and keep the warning in place?

Copy link
Member

Choose a reason for hiding this comment

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

This change means that auto is no longer auto, it's just using the manual setting. The point of auto is that it's supposed to pick a good level to compress the audio to. If it's picking bad numbers, then we need to fix calculateCompressedGain

@ronso0 ronso0 force-pushed the mic-auto-ducking branch from 78c2aa8 to ae64abe Compare April 7, 2020 23:10
@ywwg
Copy link
Member

ywwg commented Apr 8, 2020

can you explain what was broken about the code before? I'm not sure I understand the fix

@ronso0
Copy link
Member Author

ronso0 commented Apr 8, 2020

see https://bugs.launchpad.net/mixxx/+bug/1737113
in master, try Auto duck mode yourself: it has no effect on the music volume, no when talkover is active on any mic.

This fixes it.

@ronso0
Copy link
Member Author

ronso0 commented Apr 8, 2020

@ywwg Sorry for the confusion. Looks like I cleaned up too much before pushing last night, I was focused just on the Auto mode last night.
I added the check for Auto mode. Works as it should for all ducking modes now.

Still, this is ust a test. In the beginning I thought enginetalkoverducking.cpp could take care of checking for active talkover, but the additional includes and passing channels/groups around seemed too complicated for that simple purpose.
Earlier in EngineMaster::process() there's a place where the ducking mode is read already, maybe using a switch OFF/AUTO/MANUAL there is more efficient than requesting the ducking mode once more for talkover gain?

@ronso0
Copy link
Member Author

ronso0 commented Apr 8, 2020

Think I found the root issue: the Strength knob effect depends on the mode.
This is very confusing, and it doesn't match the tooltip description.
MAN Strength knob is like a music vol knob:
strength=0 > max music ducking, strength=1 > no ducking
AUTO strength knob is more like a Mic Vol knob:
strength=0 > no ducking, strength=1 > max ducking

So, disregard my changes here. We need to fix the enginesidechainprocessor as you said.
Unfortunately a PR can be changed from Draft to Review, but not the other way around...
found it

@ronso0 ronso0 marked this pull request as draft April 9, 2020 16:12
@ronso0
Copy link
Member Author

ronso0 commented Apr 16, 2020

@ywwg
I added my findings to https://bugs.launchpad.net/mixxx/+bug/1737113
IMO we should revert the knob direction in Auto mode for a consistent experience.
Simply using 1-strength instead of strength in calculateCompressedGain() seems to do the job. Though I only tested by looking at the VU meters. To check if calculateCompressedGain() returns a adequate value we'll have to test with a real Mic attached, but I don't know when I'll have the chance to do so.
Maybe this is also a good moment to think about making attackTime and decayTime configurable via knobs?

Besides, I think it would also be appreciated to have an Auto mode that engages ducking when Talk is pressed: you could have a small pause with low music before the voice comes in, compared to the existing Auto mode which -by it's nature- always overlaps music and voice a bit.

@daschuer
Copy link
Member

daschuer commented May 7, 2020

Ups, I was not aware that his branch exists. Sorry.
A alternative solution was merged here #2759 so this one can be closed.

@daschuer daschuer closed this May 7, 2020
@ronso0
Copy link
Member Author

ronso0 commented May 7, 2020

Jup, closing this is correct. Actually it was just me starting to poke around in the Mic engine code and was superseeded by our latest mic fixes.

@ronso0 ronso0 deleted the mic-auto-ducking branch May 7, 2020 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants