reduce memory usage for Echo effect by Be-ing · Pull Request #1302 · mixxxdj/mixxx

Be-ing · 2017-07-04T03:13:34Z

The Echo effect refers to this constant to determine the size of the buffers it allocates, resulting in a huge waste of memory when this constant is 192 kHz. When effects maintain state for both the master output and headphones output in PR #1254, this resulted in the Echo effect allocating about 400 MB!

There is no reason Mixxx should support 192 kHz, even if a user's sound card does. It provides no benefit and is likely to introduce introduce intermodulation distortion when playing ultrasonic
frequencies. Refer to http://xiph.org/~xiphmont/demo/neil-young.html for details.

Be-ing · 2017-07-04T03:45:53Z

The reference to this constant in the Echo effect was introduced by @rryan in de43d2d to fix Bug #1658508.

daschuer · 2017-07-06T19:48:24Z

Unfortunately, we cannot do this without rejecting valid 192 kHz tracks.
Mixxx has the concept to pass them bit perfect into the tempo and pitch shift engine (enginebuffer) to have only one re-sampling stage.
The following audio processing, including effects is done with the output sample rate which is currently limited to 92 kHz.

Be-ing · 2017-07-06T19:53:45Z

How about using two constants? We could call them something like kSamplingRateFileMax and kSamplingRatePlaybackMax.

daschuer · 2017-07-06T20:04:54Z

That works for me. These constants value should be used here as well:

mixxx/src/soundio/soundmanager.cpp

Line 78 in 2bcf344

m_samplerates.push_back(96000);

To be honest I have not understood the source of 400 MB
This is good for 136 Echo effect states @192 kHz.
192kHz * size_off(float) * 2 s * 2 ch = ~3 MB

Instead of allocation the maximum possible buffer in echo, we should allocate a reasonable buffer and if one day someone enables an insane sample rate, the echo should limit its delay without the risk of a memory overflow.

Be-ing · 2017-07-06T20:08:27Z

To be honest I have not understood the source of 400 MB

(4 decks + 4 mics + 4 aux + 64 samplers) x 2 [master and headphone outputs] group states

daschuer · 2017-07-06T20:16:54Z

We are doing something else wrong ..

uklotzde · 2017-07-08T09:51:46Z

The maximum sample rate is also used for validating metadata read from audio files. We should not adjust this, just because some internal filter is badly designed.

I agree with Daniel:

Allocate a buffer for 48 kHz
Adjust the size of the buffer later if required (copying the currently buffered data) and log an informational message

Allocating additional memory in a real-time scenario is not ideal, but otherwise the effects framework would need to be redesigned to handle varying sample rates. Changing the internal sample rate of the engine should only happen in a non-real-time situation.

uklotzde · 2017-07-08T09:53:42Z

~~And, of course, the buffer currently only has a size of 3 MB.~~ Ok, for each buffer.

uklotzde · 2017-07-08T10:01:46Z

...and constants for specific use cases should be defined in the corresponding context instead of in a common place. 192 kHz is the maximum that Mixxx supports. If the engine has a lower limit it should define it's own constant.

Be-ing · 2017-07-08T23:16:53Z

Allocating additional memory in a real-time scenario is not ideal, but otherwise the effects framework would need to be redesigned to handle varying sample rates.

I think we should redesign the engine and engine side of the effects system to reallocate buffers when the playback sample rate changes. But I think that could be a large task. For now we need a quick fix. IMO allocating memory in the audio callback thread is a non-starter.

Changing the internal sample rate of the engine should only happen in a non-real-time situation.

Correct, which would make it possible to tell the engine and effects system to reallocate their buffers from DlgPrefSound.

Be-ing · 2017-07-08T23:29:40Z

that is a large task

Hmm, it might not be so difficult actually. I'll try implementing it in #1254 after #1279 is merged.

Be-ing · 2017-11-19T20:15:16Z

So how shall we address this for now? Two options occur to me:

Add a new constant for max playback sample rate
Hardcode 96 kHz in the Echo code

uklotzde · 2017-11-19T20:58:34Z

I vote for a separate constant kMaxPlaybackSampleRate that explicitly states the limits of the engine.

Constants, types, and version numbers are not limited resources and should be used whenever needed. The constant doesn't magically disappear if we hide it somewhere inside the echo code.

Be-ing · 2017-11-20T07:35:11Z

Okay, I have introduced a new constant and used it for Echo.

uklotzde · 2017-11-20T19:39:30Z

    static constexpr SINT kSamplingRate96kHz   = 96000;
    static constexpr SINT kSamplingRate192kHz  = 192000;
    static constexpr SINT kSamplingRateMax     = kSamplingRate192kHz; // upper bound
+    static constexpr SINT kSamplingRateMaxPlayback = kSamplingRate96kHz;


I would like to see this constant at some central location within the engine code base instead of in this unbiased header file. If the engine decides to restrict the sample rate for playback it should define its own constant for this purpose.

uklotzde · 2017-11-20T19:44:12Z

You may copy the new engine.h header file that I added in my audiosourcev2 branch. I used it for explicitly defining the (in)famous kEngineChannelCount = 2 constant ;) The data type is not yet available, SINT should work.

daschuer · 2017-11-21T07:12:19Z

I am currently trying to get my head around this, with #1254 in mind.

I have read a bit how others solves this, an I cannot get rid of the feeling we are doing something wrong. I can't express this very well, but this issue here can be easily solved using the classic hardware solution of a send bus. In this case we need only one or two instances of an classic send effect like echo.

The drawback is, that it requires some more brain how to make his easy accessable.
We have no big problems here since we can tweak our internal effects, but if we think to high quality LV2 reverbs, with external GUI, a mandatory 136 instances solution sounds bad.

Be-ing · 2017-11-21T07:48:55Z

There may be a better solution, but nothing comes to mind immediately. At least for 2.1, I think only allocating buffers for 96 kHz maximum and not letting samplers be routed to effects (which was a capability of the engine not exposed to users in Mixxx 2.0, so I don't think anyone is going to miss it) is a good enough solution.

daschuer · 2017-11-21T16:20:38Z

Reducing the buffer here is a good thing in any way. The root cause for the huge memory consumption is the amount if instances though.

Be-ing · 2017-11-26T01:21:17Z

Ack, this is trickier than I thought. Although we only list 96k as the max sample rate in the preferences, it is possible to use Mixxx with JACK using higher sample rates...

Be-ing · 2017-11-26T01:44:47Z

Ehhhh this would require some changes to the effects system that would conflict with #1254, so let's wait until that is merged... :/

The Echo effect referred to the mixxx::AudioSignal::SampleRate::max() constant, equal to 192 kHz, to determine the size of the buffers it allocates. This resulted in a huge waste of memory. There is no reason Mixxx should support 192 kHz, even if a user's sound card does. It provides no benefit and is likely to introduce introduce intermodulation distortion when playing ultrasonic frequencies. Refer to http://xiph.org/~xiphmont/demo/neil-young.html for details.

Be-ing · 2017-12-08T00:21:25Z

Okay, I've done a bit of refactoring so the Echo effect and preferences refer to the same max sampling rate constant for playback.

There is a tradeoff between the maximum delay time and the memory required by the effect. The higher the maximum delay time, the lower the minimum BPM that can use the full range of 2 beats. The cutoff point is arbitrary. The default minimum BPM for analysis is set at 70, so I think it is fairly unlikely that a user would use the Echo effect with the maximum delay time of 2 beats with a track less than 60 BPM.

Be-ing · 2017-12-08T04:54:59Z

We are doing something else wrong ..

As a quick proof-of-concept hack, I lowered the MAX_BUFFER_LEN constant in src/util/defs.h to from 160000 (where did that number come from? There is no information in the source code) to 4096 (which I think is the default) on this branch. That saved 97 MB of memory. I tested again on my personal branch with this and #1254 and still 91 MB saved. We should get rid of this lazy MAX_BUFFER_LEN constant and allocate buffer sizes actually needed with the user's configuration.

uklotzde · 2017-12-08T10:14:55Z

    EchoGroupState()
-            : delay_buf(mixxx::AudioSignal::SampleRate::max() * kMaxDelaySeconds *
-                        kChannelCount) {
+            : delay_buf(kMaxDelaySeconds * kChannelCount *


Please add some static functions to SoundManager to avoid leaking internal storage and ordering and to make the code more readable:
static constexpr SINT minSupportedSampleRate()
static constexpr SINT maxSupportedSampleRate()
static bool isSupportedSampleRate(SINT sampleRate)

uklotzde · 2017-12-08T10:16:25Z

+    // introduce introduce intermodulation distortion when playing ultrasonic
+    // frequencies. Refer to http://xiph.org/~xiphmont/demo/neil-young.html
+    // for details.
+    constexpr static const unsigned int s_iSupportedSampleRates[3] = {


Should be private. Do you explicitly need to state the size here?

uklotzde · 2017-12-08T10:18:20Z

-bool SoundManagerConfig::checkSampleRate(const SoundManager &soundManager) {
-    if (!soundManager.getSampleRates(m_api).contains(m_sampleRate)) {
-        return false;
+bool SoundManagerConfig::checkSampleRate() {


Replaced by:
return SoundManager::isSupportedSampleRate(m_sampleRate);

uklotzde · 2017-12-08T10:21:38Z

-            m_sampleRate = sampleRates.first();
-        } else {
-            qWarning() << "got empty sample rate list from SoundManager, this is a bug";
+        m_sampleRate = defaultSampleRate;


if (SoundManager::isSupportedSampleRate(defaultSampleRate)) { m_sampleRate = defaultSampleRate; } else { DEBUG_ASSERT(SoundManager::isSupportedSampleRate(kFallbackSampleRate)); m_sampleRate = kFallbackSampleRate; }

uklotzde · 2017-12-08T10:36:32Z

A good step into the right direction to get rid of all those implicit constants and assumptions that are scattered everywhere in the code! Please just make the code more safe and readable and we will end up with even less code. Less code = less bugs.

We should publish more of these small, easy to review PRs that improve the maintainability and readability of existing code before introducing new features! I must confess that my own PRs always get much too big. If we would have more committers that are able to review and integrate small PRs fast it would be a big improvement ;)

Be-ing · 2017-12-11T01:40:11Z

This conflicts with ongoing work on #1254. I'll merge the changes to SoundManager and SoundManagerConfig from this branch onto that.

daschuer · 2017-12-11T06:54:08Z

I am a bit concerned, that #1254 will get to big.
There is no strong need for the changes here if we create the memory on demand. Can't we do it later in a separate PR?

Be-ing · 2017-12-11T06:56:29Z

Yeah, there's no need to rush this anymore.

Be-ing force-pushed the fix_max_sampling_rate_constant branch from 9684140 to 7d9b9f1 Compare July 4, 2017 03:52

This was referenced Jul 4, 2017

move effects post-fader #1254

Merged

microphone I/O revamp #1279

Merged

uklotzde added the effects label Oct 29, 2017

Be-ing changed the title ~~reduce AudioSignal::kSamplingRateMax to the max in preferences~~ reduce memory usage for Echo effect Nov 19, 2017

Be-ing force-pushed the fix_max_sampling_rate_constant branch from 7d9b9f1 to 48ab2e9 Compare November 19, 2017 22:40

uklotzde reviewed Nov 20, 2017

View reviewed changes

uklotzde added this to the 2.1.0 milestone Nov 25, 2017

Be-ing force-pushed the fix_max_sampling_rate_constant branch from 48ab2e9 to 26cec56 Compare December 8, 2017 00:18

uklotzde reviewed Dec 8, 2017

View reviewed changes

Be-ing closed this Dec 11, 2017

Be-ing deleted the fix_max_sampling_rate_constant branch April 24, 2019 23:43

Uh oh!

Conversation

Be-ing commented Jul 4, 2017

Uh oh!

Be-ing commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Jul 6, 2017

Uh oh!

Be-ing commented Jul 6, 2017

Uh oh!

daschuer commented Jul 6, 2017

Uh oh!

Be-ing commented Jul 6, 2017

Uh oh!

daschuer commented Jul 6, 2017

Uh oh!

uklotzde commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uklotzde commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uklotzde commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Jul 8, 2017

Uh oh!

Be-ing commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uklotzde commented Nov 19, 2017

Uh oh!

Be-ing commented Nov 20, 2017

Uh oh!

uklotzde Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

uklotzde commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Nov 21, 2017

Uh oh!

Be-ing commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Nov 21, 2017

Uh oh!

Be-ing commented Nov 26, 2017

Uh oh!

Be-ing commented Nov 26, 2017

Uh oh!

Be-ing commented Dec 8, 2017

Uh oh!

Be-ing commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uklotzde Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uklotzde Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uklotzde Dec 8, 2017

Choose a reason for hiding this comment

Uh oh!

uklotzde Dec 8, 2017

Choose a reason for hiding this comment

Uh oh!

uklotzde commented Dec 8, 2017

Uh oh!

Be-ing commented Dec 11, 2017

Uh oh!

daschuer commented Dec 11, 2017

Uh oh!

Be-ing commented Jul 4, 2017 •

edited

Loading

uklotzde commented Jul 8, 2017 •

edited

Loading

uklotzde commented Jul 8, 2017 •

edited

Loading

uklotzde commented Jul 8, 2017 •

edited

Loading

Be-ing commented Jul 8, 2017 •

edited

Loading

Be-ing commented Nov 19, 2017 •

edited

Loading

uklotzde commented Nov 20, 2017 •

edited

Loading

Be-ing commented Nov 21, 2017 •

edited

Loading

Be-ing commented Dec 8, 2017 •

edited

Loading

uklotzde Dec 8, 2017 •

edited

Loading

uklotzde Dec 8, 2017 •

edited

Loading