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

Fix manual ducking #2759

Merged
merged 3 commits into from
May 7, 2020
Merged

Fix manual ducking #2759

merged 3 commits into from
May 7, 2020

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented May 6, 2020

Now the ducking gain is only applied when talkover is enabled.

Before the manual mode has always applied the Ducking gain. This does not make much sense, because the user can also lower the deck gain to have the same effect.

This Fixes a a regression since the introduction of the postfader effects in 2.1

now the ducking gain is only applied when talkover is enabled
@daschuer daschuer force-pushed the manual_ducking_fix branch from cca9725 to 27a585e Compare May 6, 2020 23:12
@daschuer daschuer changed the base branch from master to 2.2 May 6, 2020 23:13
@daschuer daschuer added this to the 2.2.4 milestone May 6, 2020
// clear state from the last round of compressor gain calculation.
void clearKeys();
/// Forces the above threshold flag to the given value without calculations
void setAboveThreshold(bool value);
Copy link
Member

Choose a reason for hiding this comment

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

This could be inline like the other setXyz() functions above.

@ronso0
Copy link
Member

ronso0 commented May 7, 2020

Nice, thank you for the fix. I was about to propose that fix.
Works nicely and prevents lowered / muted master volume after start with hidden Mics.

Fixes https://bugs.launchpad.net/mixxx/+bug/1394968 and I can close #2006. Thx!

m_pTalkoverDucking->processKey(m_pTalkover, m_iBufferSize);
break;
case EngineTalkoverDucking::MANUAL:
qDebug() << "m_activeTalkoverChannels.size()" << m_activeTalkoverChannels.size();
Copy link
Contributor

Choose a reason for hiding this comment

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

In debug log in real-time code??

Copy link
Member Author

Choose a reason for hiding this comment

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

Ups ...

daschuer and others added 2 commits May 7, 2020 09:45
Previously, engine/enginetalkoverducking returned opposite values
for 'strength' in Auto and Manual mode. This commit removes the
inversion from Auto mode, thus the Strength knob always affects the
music volume in the same way with both modes, much like a Muisc Volume
knob: * fully left: music muted completely
      * fully right: music volume unchanged
@ronso0
Copy link
Member

ronso0 commented May 7, 2020

Just that I get our Merge policy straight, finally:

  • if we squash and merge this (after a LGTM from a c++ pro) the commit message doesn't fully reflect the two separate fixes
  • if I create a Merge Commit we have "silnce debug" commit, as well, which just removes something that was introduced in a previous commit and shouldn't end up in the final commit history, right?

So if I get that right the first two commits should be squashed, then we Merge Commit, right?

@Holzhaus
Copy link
Member

Holzhaus commented May 7, 2020

I think we should either
a) just merge and keep the history, or
b) do an interactive rebase, cut-and-paste the pick 8af17c2 Silence Debug right after the line that introduced the debug statement and replace pick with fixup in the pasted line.

@daschuer
Copy link
Member Author

daschuer commented May 7, 2020

The general rule applies here, no rebase during a review. That's easy and saved all parties from extra work.

I must admit that since the found issue effects the top most commit, I should have sqashed that.
But since no one cares after a while lets save us from the work and merge this.

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.

5 participants