Loudness Contour effect by daschuer · Pull Request #1141 · mixxxdj/mixxx

daschuer · 2017-01-21T22:50:05Z

This PR adds a Loudness Contour effect intended to be placed at the master output.

It has a master gain mapping Knob, which allows these use cases.

If your soundsystem volume is leveled for full volume. You can map the the Master gain and use it to reduce to dinner volume. You loose some bits of simple precision, but that should be OK.
Once you rise the volume up, the Loudness effect goes away as it happens on your living room amplifier.
If you like to put the loudness contour to the signal and level the volume at the amplifier or use the master gain dependently, you can disables the master knob mapping.

daschuer · 2017-01-21T22:52:16Z

https://bugs.launchpad.net/mixxx/+bug/1657685

rryan · 2017-01-22T05:46:42Z

+
+    if (enableState != EffectProcessor::DISABLING) {
+
+        bool useMatser = m_pUseMasterGain->toBool();


typo: useMaster

rryan · 2017-01-22T05:48:11Z

+            pState->m_oldSampleRate = sampleRate;
+
+            if (useMatser) {
+                masterGain = std::min(std::max(masterGain, 0.03), 1.0); // Limit at 0 .. -30 dB


math_clamp(masterGain, 0.03, 1.0) from util/math.h is more readable IMO

rryan · 2017-01-22T07:02:59Z

@@ -0,0 +1,159 @@
+#include <effects/native/loudnesscontoureffect.h>


please use " instead of <>

rryan · 2017-01-22T07:15:44Z

+    manifest.setEffectRampsFromDry(true);
+    manifest.setIsMixingEQ(true);
+
+    EffectManifestParameter* low = manifest.addParameter();


Since you added gain tracking, do we need this parameter? What's the use case if the master-gain-tracking perfectly adjusts the curve to compensate? Usually stereos just have a "loudness" button and not a manual-adjust knob, right?

My old Yamaha Amp has such a loudness knob and I miss it on my new 5.1 one where I cannot even disable loudness.

The use case here is that the loudness level depends on the systems volume, which we cannot control, because Mixxx has no access to the Amp volume. So the user has the option to pick a suitable value for the current loudness.

rryan · 2017-01-22T07:19:39Z

+    manifest.setAuthor("The Mixxx Team");
+    manifest.setVersion("1.0");
+    manifest.setDescription(QObject::tr(
+        "A filter to compensate the loudness contour at lower volume"));


I think we need a bit of explanation for users who don't know why you would want that.

What about?
"Amplifies low and high frequencies at low volumes to compensate for reduced sensitivity of the human ear."

I wish we could add links to the description, like:
https://en.wikipedia.org/wiki/Loudness_compensation

rryan · 2017-01-22T07:22:58Z

+        : m_pLoudness(pEffect->getParameterById("loudness")),
+          m_pUseMasterGain(pEffect->getParameterById("useMasterGain")) {
+    Q_UNUSED(manifest);
+    m_pMasterGain = std::make_unique<ControlProxy>("[Master]", "gain");


I think it'd be more general if you plumbed the gain for the group through the GroupFeatureState. This way, if the user enabled LoudnessContour for Channel1, it would adjust using Channel1's gain instead of master.

I have also considered this, but the use case for the channel gain is to level the track, not to set the volume. The alternative would be the volume slider. But this effect does basically the opposite what we need for mixing. Where we normally kill the base.
Using the loudness knob independently seams to be a good solution.

I will add super-knob mapping than this can go to a quick effect.

I have also considered this, but the use case for the channel gain is to level the track, not to set the volume.

Is that a safe assumption? What if the user is not using internal mixing and they have the channel routed to a mixer instead of mixing internally?

The user can always use the "Loudness" parameter. The master gain knob mapping works only in the use-case the sound system is leveled for full volume and the user used the master gain to reduce the loudness. For all other use-cases this mapping is IMHO pointless.
But If we have other use cases, it would be no issue to to add them.

I have also considered to add it to the EQ Rack, but currently we allow only one master EQ.
Or we can introduce a master Quick effect rack for it.

works only in the use-case the sound system is leveled for full volume

The user could have done that for channel1 too -- my point is just that you are limiting the possible options for the user by hard-coding it to the master gain and I see no reason to do that.

OK, do you which to use the channel gain? Since the mapping is switchable it should be no issue to add that.

rryan · 2017-01-22T07:23:16Z

+#ifndef LOUDNESSCONTOUREFFECT_H
+#define LOUDNESSCONTOUREFFECT_H
+
+#include <QMap>


Conflicts: build/depends.py

daschuer · 2017-01-26T21:48:17Z

I have thought a bit about the GroupFeatureState and future requirements. This GroupFeatureState is a candidate for growing. It is no deal for me to add now additional gain member, but it requires additional engine code. This is quite easy in this case, but can be complicated for future cases.

This effect is most likely the only effect which will make ever use of gain. But adding it to GroupFeatureState put persistent CPU load on any effect chain call for any channel even if the loudness effect is not used.

The GroupFeatureState has already 20 values an 20 exist flags. Every value is set for every effect channel every callback cycle, but in almost all cases the values are not used.
If we also allow to access Control objects from the effect depending on the channel, we would have no performance penalty and we have an easy way to access any control object which is needed.

It would be also no problem to implement a fancy transport effect which accesses many controls, without bothering the user if other effects are loaded.

GroupFeatureState will remain, because it is handy to transfer a set of values that belongs together without races. The beat values are a perfect example for it.

I could use the gain CO here for a prove of concept how to add a CO to the EffectGroupState.

What do you think?

rryan · 2017-01-27T04:49:12Z

But adding it to GroupFeatureState put persistent CPU load on any effect chain call for any channel even if the loudness effect is not used.

The additional CPU load is negligible -- a single move instruction per deck to copy the gain into a new double field in the GroupFeatureState. The GroupFeatureState is passed to the effects via const reference so there is no cost for adding additional state.

Every value is set for every effect channel every callback cycle, but in almost all cases the values are not used.

Yes, they are unused sofar but I added them to make it easy on effect writers so they wouldn't have to do the plumbing themselves and could focus on writing their effect. .

I could use the gain CO here for a prove of concept how to add a CO to the EffectGroupState.

What do you think?

I think we should remove all CO use from the engine, so I don't want to add more places that use them. GroupFeatureState is a low-cost mechanism for exposing per-group state to the effects -- let's use that.

daschuer · 2017-01-27T19:40:58Z

Finished!

rryan · 2017-01-27T19:49:07Z

Thanks! LGTM

daschuer · 2017-01-27T20:40:49Z

Thank you for testing.

rryan · 2017-02-06T03:07:26Z

I just noticed this is marked as a mixing EQ so it shows up as a choice for the deck EQs -- @daschuer, did you intend to do that? What's the use case there?

daschuer · 2017-02-06T07:02:04Z

That is just a typo. Originally it should be isForFilterKnob(true). But since this is gone now the line can be removed.

rryan · 2017-02-06T16:52:22Z

Got it -- done: 95f74bd

Added a Loudness Contour effect

f9b23f5

rryan reviewed Jan 22, 2017

View reviewed changes

daschuer added 2 commits January 22, 2017 23:50

Tweaked texts and added a Superknob linking

dd6799a

Merge remote-tracking branch 'upstream/master' into loudnessEffect

edd66ed

Conflicts: build/depends.py

Use GroupFeatureState to act on the gain or channel gain knob.

89eaae3

break long lines

eecfe8d

daschuer merged commit ad789bd into mixxxdj:master Jan 27, 2017

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

daschuer deleted the loudnessEffect branch September 7, 2021 21:06

mixxxbot mentioned this pull request Aug 23, 2022

Add a Loudness Contour effect #8765

Closed


		if (enableState != EffectProcessor::DISABLING) {

		bool useMatser = m_pUseMasterGain->toBool();

		@@ -0,0 +1,159 @@
		#include <effects/native/loudnesscontoureffect.h>

Uh oh!

Conversation

daschuer commented Jan 21, 2017

Uh oh!

daschuer commented Jan 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer commented Jan 26, 2017

Uh oh!

rryan commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Jan 27, 2017

Uh oh!

rryan commented Jan 27, 2017

Uh oh!

daschuer commented Jan 27, 2017

Uh oh!

rryan commented Feb 6, 2017

Uh oh!

daschuer commented Feb 6, 2017

Uh oh!

rryan commented Feb 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rryan commented Jan 27, 2017 •

edited

Loading