Use `SampleRate` type consistently #11904

Holzhaus · 2023-08-31T22:07:08Z

This replaces all remaining occurrences of integer sample rates that I found via git grep and uses the semantic mixxx::audio::SampleRate type instead.

daschuer

Thank you. I left some comments.

daschuer · 2023-09-01T05:32:05Z

src/effects/backends/builtin/filtereffect.cpp

@@ -128,8 +128,8 @@ void FilterEffect::processChannel(
            double qmax = 4 - 2 / 0.6 * ratio;
            clampedQ = math_min(clampedQ, qmax);
        }
-        pState->m_pLowFilter->setFrequencyCorners(1, lpf, clampedQ);
-        pState->m_pHighFilter->setFrequencyCorners(1, hpf, clampedQ);
+        pState->m_pLowFilter->setFrequencyCorners(mixxx::audio::SampleRate(1), lpf, clampedQ);


This looks confusing. I think using mixxx::audio::SampleRate is wrong here, because it not seems to have the same semantic at "1/x s" like in other cases. It is here just the frequency denominator. It looks like it becomes a double at one point in fidlib.
See also: m_hiFreq(kMinCorner / engineParameters.sampleRate()) {
I so there is more refactoring/testing required. Postpone?
.

but the parameter is named samplerate. What should it be named instead if its not used as a samplerate?

It can be refactored into using our mixxx::audio::SampleRate that is always in samples/s.
Currently it looks like it sets the filter to a 1/s sample rate, which is not the case.

I'm unsure what you want me to do TBH.

I am not sure if we have a common understanding about the issue here. Do we?

I propose to revert the Fidlib related changes here and merge the rest of this PR quickly. This way we can consider the best solution for this issue in a smaller PR < 66 files .

I checked in the fidlib code and the sampleRate parameter is actually passed as double rate parameter of the fid_design() method. In the error handling code, rate is used as the second argument of the error message Frequency of %gHz out of range with sampling rate of %gHz. So it seems indeed the "sampling rate". No idea why we pass just 1 though.

Should I revert the changes nonetheless?

So I think we have two options.

Postpone the Fidlib related actions

Use here the real 1/s base sample rate values via our semantic type and adjust the other parameters accordingly.

Undoing all the fidlib-related changes turned out tedious, so I opted for 2. Please have a look at 99e1088.

src/engine/enginevumeter.cpp

src/preferences/dialog/dlgprefsound.cpp

src/soundio/soundmanagerconfig.cpp

Swiftb0y · 2023-09-01T11:21:26Z

src/soundio/soundmanager.cpp

+    m_pConfig->set(ConfigKey("[Soundcard]", "Samplerate"),
+            ConfigValue(static_cast<int>(m_config.getSampleRate().value())));


Suggested change

m_pConfig->set(ConfigKey("[Soundcard]", "Samplerate"),

ConfigValue(static_cast<int>(m_config.getSampleRate().value())));

m_pConfig->setValue<int>(ConfigKey("[Soundcard]", "Samplerate"),

m_config.getSampleRate().value());

/home/jan/Projects/mixxx/src/soundio/soundmanager.cpp: In member function ‘SoundDeviceStatus SoundManager::setConfig(const SoundManagerConfig&)’: /home/jan/Projects/mixxx/src/soundio/soundmanager.cpp:563:57: error: call of overloaded ‘ConfigValue(mixxx::audio::SampleRate::value_t)’ is ambiguous 563 | ConfigValue(m_config.getSampleRate().value())); | ^ In file included from /home/jan/Projects/mixxx/src/preferences/usersettings.h:6, from /home/jan/Projects/mixxx/src/controllers/midi/midimessage.h:10, from /home/jan/Projects/mixxx/src/control/controlbehavior.h:6, from /home/jan/Projects/mixxx/src/control/control.h:9, from /home/jan/Projects/mixxx/src/control/pollingcontrolproxy.h:6, from /home/jan/Projects/mixxx/src/soundio/soundmanager.h:11, from /home/jan/Projects/mixxx/src/soundio/soundmanager.cpp:1: /home/jan/Projects/mixxx/src/preferences/configobject.h:83:14: note: candidate: ‘ConfigValue::ConfigValue(double)’ 83 | explicit ConfigValue(double value); | ^~~~~~~~~~~ /home/jan/Projects/mixxx/src/preferences/configobject.h:82:14: note: candidate: ‘ConfigValue::ConfigValue(int)’ 82 | explicit ConfigValue(int value); | ^~~~~~~~~~~ /home/jan/Projects/mixxx/src/preferences/configobject.h:73:7: note: candidate: ‘ConfigValue::ConfigValue(const ConfigValue&)’ 73 | class ConfigValue { | ^~~~~~~~~~~

okay, Ignore for now. I'll look into it some other time.

I've looked into it. The problem is trivial on the surface (uint32_t can't be promoted to int nor double, thus it would be converted and neither conversion ranks better than the other). The problem gets harder when I looked into fixing it. configobject.h is unnecessarily complex with lots of duplicated code due to a lack of separation of concerns (ConfigValue vs ConfigObject::setValue, which type/function is supposed to do the conversion?). I may do some refactoring in the future.

src/soundio/soundmanagerconfig.cpp

Swiftb0y · 2023-09-01T12:03:22Z

src/effects/backends/builtin/filtereffect.cpp

@@ -128,8 +128,8 @@ void FilterEffect::processChannel(
            double qmax = 4 - 2 / 0.6 * ratio;
            clampedQ = math_min(clampedQ, qmax);
        }
-        pState->m_pLowFilter->setFrequencyCorners(1, lpf, clampedQ);
-        pState->m_pHighFilter->setFrequencyCorners(1, hpf, clampedQ);
+        pState->m_pLowFilter->setFrequencyCorners(mixxx::audio::SampleRate(1), lpf, clampedQ);


but the parameter is named samplerate. What should it be named instead if its not used as a samplerate?

…int`

Swiftb0y

My comments have been resolved, waiting for LGTM from @daschuer

daschuer

Removing the normalization form the filter seems to work. Thank you.
I have left two additional comments.

src/engine/sidechain/networkoutputstreamworker.cpp

daschuer · 2023-09-03T21:07:06Z

src/soundio/sounddevicenotfound.h

+    mixxx::audio::SampleRate getDefaultSampleRate() const override {
+        return SoundDevice::kFallbackSampleRate;


We have a naming issue with kFallbackSampleRate:

SoundManagerConfig::kFallbackSampleRate = mixxx::audio::SampleRate(48000);
constexpr mixxx::audio::SampleRate SoundDevice::kFallbackSampleRate = mixxx::audio::SampleRate(44100);

Than we have:
constexpr auto kDefaultSampleRate = mixxx::audio::SampleRate(44100);
And various definitions in global namespaces.

I think the term kFallbackSampleRate has been invented for the 48000 Hz that all soundcards likely support.
The 44100 is Mixxx's default sample rate that matches CD rips and is the most common sample rate when the decision has been made.

I suggest to use the term kFallbackSampleRate for 48000 Hz and kDefaultSampleRate for 44100 or maybe even better:
kMixxxDefaultSampleRate ..

Than you may check which additional constants can be removed.

I renamed it.

Than you may check which additional constants can be removed.

I don't see what this has to do with this PR to be honest. As you already mentioned yourself, this PR is quite big and I don't get why we keep expanding the scope of PRs even further. I think its totally fine to fix one thing without also fixing some only tangentially related other thing.

…alizer

We know the actual sample rate from the `EngineParameters` object, so there is no need to hardcode some constant.

daschuer

One remaining issue. The rest looks and works good. Thank you.

daschuer · 2023-09-07T16:56:58Z

src/soundio/soundmanagerconfig.cpp

+constexpr mixxx::audio::SampleRate SoundManagerConfig::kMixxxDefaultSampleRate =
+        mixxx::audio::SampleRate(44100);
+constexpr mixxx::audio::SampleRate SoundManagerConfig::kFallbackSampleRate =
+        mixxx::audio::SampleRate(48000);


Suggested change

constexpr mixxx::audio::SampleRate SoundManagerConfig::kMixxxDefaultSampleRate =

mixxx::audio::SampleRate(44100);

constexpr mixxx::audio::SampleRate SoundManagerConfig::kFallbackSampleRate =

mixxx::audio::SampleRate(48000);

const mixxx::audio::SampleRate SoundManagerConfig::kMixxxDefaultSampleRate =

mixxx::audio::SampleRate(44100);

const mixxx::audio::SampleRate SoundManagerConfig::kFallbackSampleRate =

mixxx::audio::SampleRate(48000);

This produces a linker error with ldd.
It should be anyway better to match the definition in the header.

make both constexpr, move to header instead? Or would you consider that to leak implementation details?

987fd3a ok?

…ader This is supposed to fix `ldd` linker errors that may occur with the previous code. See mixxxdj#11904 (comment) for details.

daschuer

LGTM, Thank you

Swiftb0y · 2023-09-08T05:32:02Z

Thank you

Holzhaus added 12 commits September 1, 2023 00:04

refactor(graphiceqeffect): Use constexpr instead of #define

cfcd235

refactor(engine): Use SampleRate type instead of int/unsigned int

a5274ef

refactor(beatmaptest): Use SampleRate type instead of int

dddb991

refactor(track/key*): Use SampleRate type instead of int

8c026b3

refactor(vinulcontrolxwax): Use SampleRate type instead of int

9cbbc3d

refactor(soundio): Use SampleRate type instead of unsigned int

7198952

refactor(cachingreader): Use SampleRate type instead of int

fb3973d

refactor(beatgridtest): Use SampleRate type instead of int

6ce5acf

refactor(bpmcontrol_test): Use SampleRate type instead of int

1ea4ed8

refactor(cuecontrol_test): Use SampleRate type instead of int

ae0dcf9

refactor(searchqueryparsertest): Remove unnecessary sampleRate arg

4401230

refactor(enginevumeter): Use SampleRate type instead of int

1186bf7

github-actions bot added ui code quality labels Aug 31, 2023

Swiftb0y self-assigned this Aug 31, 2023

daschuer requested changes Sep 1, 2023

View reviewed changes

Swiftb0y requested changes Sep 1, 2023

View reviewed changes

Holzhaus added 6 commits September 1, 2023 19:04

chore(enginevumeter): Remove i prefix from m_iSamplesCalculated

ea1caae

chore(soundmanagerconfig): Simplify calculation of framesPerBuffer

ea9299f

chore(dlgprefsound): Use const auto& instead of auto& for loop

6b8f0ff

refactor(soundio): Use SampleRate type instead of double

884fd0b

refactor(soundsourcemad): Use SampleRate type instead of `unsigned …

3d77bba

…int`

fix(dlgprefsound): Directly read SampleRate type from QVariant

f1bdd02

Swiftb0y approved these changes Sep 3, 2023

View reviewed changes

refactor(filtereffect): Pass actual sample rate and remove normalization

99e1088

daschuer requested changes Sep 3, 2023

View reviewed changes

Holzhaus added 3 commits September 6, 2023 10:30

chore(networkoutputstreamworker): Remove redundant SampleRate initi…

a331979

…alizer

refactor(modplug): Use SampleRate/ChannelCount instead of int

e04eb77

chore(soundio): Rename device fallback rate to kMixxxDefaultSampleRate

53ef200

chore(effects): Use actual engine sample rate instead of constant

7991e3d

We know the actual sample rate from the `EngineParameters` object, so there is no need to hardcode some constant.

JoergAtGithub requested a review from daschuer September 7, 2023 06:01

daschuer requested changes Sep 7, 2023

View reviewed changes

chore(soundio): Move sample rate constants to SoundManagerConfig he…

987fd3a

…ader This is supposed to fix `ldd` linker errors that may occur with the previous code. See mixxxdj#11904 (comment) for details.

daschuer approved these changes Sep 8, 2023

View reviewed changes

daschuer merged commit e4f2e05 into mixxxdj:main Sep 8, 2023

This was referenced Sep 9, 2023

Regression: Audio buffer size UI shows inf ms on macOS #11949

Closed

DlgPrefSound: Fix fetching of sample rate #11951

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `SampleRate` type consistently #11904

Use `SampleRate` type consistently #11904

Holzhaus commented Aug 31, 2023

daschuer left a comment

daschuer Sep 1, 2023

Swiftb0y Sep 1, 2023

daschuer Sep 1, 2023 •

edited

Loading

Holzhaus Sep 1, 2023

daschuer Sep 2, 2023

Holzhaus Sep 2, 2023 •

edited

Loading

daschuer Sep 3, 2023

Holzhaus Sep 3, 2023

Swiftb0y Sep 1, 2023

Holzhaus Sep 1, 2023

Swiftb0y Sep 1, 2023

Swiftb0y Sep 3, 2023

Swiftb0y Sep 1, 2023

Swiftb0y left a comment

daschuer left a comment

daschuer Sep 3, 2023

Holzhaus Sep 6, 2023

daschuer left a comment

daschuer Sep 7, 2023

Swiftb0y Sep 7, 2023

Holzhaus Sep 7, 2023

daschuer left a comment

Swiftb0y commented Sep 8, 2023

		m_pConfig->set(ConfigKey("[Soundcard]", "Samplerate"),
		ConfigValue(static_cast<int>(m_config.getSampleRate().value())));

		mixxx::audio::SampleRate getDefaultSampleRate() const override {
		return SoundDevice::kFallbackSampleRate;

Use SampleRate type consistently #11904

Use SampleRate type consistently #11904

Conversation

Holzhaus commented Aug 31, 2023

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus Sep 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Swiftb0y commented Sep 8, 2023

Use `SampleRate` type consistently #11904

Use `SampleRate` type consistently #11904

daschuer Sep 1, 2023 •

edited

Loading

Holzhaus Sep 2, 2023 •

edited

Loading