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

reduce sample buffer memory usage #11988

Merged

Conversation

abique
Copy link
Contributor

@abique abique commented Sep 16, 2023

This set of changes reduce the amount of memory used by the application for storing audio samples.

Here two memory dump (SVG), before and after so you can compare.
pprof-after
pprof-before

I had a look over BalanceGroupState, and this one is safe to shrink.

@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 9216fa7 to 9aefb27 Compare September 16, 2023 00:12
@abique
Copy link
Contributor Author

abique commented Sep 16, 2023

I'll take a look at the compiler errors later today, this is strange my build didn't pick it up.

@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 9aefb27 to 638c747 Compare September 16, 2023 10:34
@abique
Copy link
Contributor Author

abique commented Sep 16, 2023

I've run the tests and some failed:

[  FAILED  ] 7 tests, listed below:
[  FAILED  ] AnalyzerWaveformTest.canary
[  FAILED  ] EngineMixerTest.SingleChannelOutputWorks
[  FAILED  ] EngineMixerTest.SingleChannelPFLOutputWorks
[  FAILED  ] EngineMixerTest.TwoChannelOutputWorks
[  FAILED  ] EngineMixerTest.TwoChannelPFLOutputWorks
[  FAILED  ] EngineMixerTest.ThreeChannelOutputWorks
[  FAILED  ] EngineMixerTest.ThreeChannelPFLOutputWorks

Let's see if it is a regression.

@abique
Copy link
Contributor Author

abique commented Sep 16, 2023

The regression is caused by: constexpr unsigned int MAX_BUFFER_LEN = 32 * 1024;.
Maybe there is a problem within the test itself.

src/util/defs.h Outdated Show resolved Hide resolved
@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 638c747 to 4f798ae Compare September 16, 2023 12:44
@abique
Copy link
Contributor Author

abique commented Sep 16, 2023

Actually the problem found by the test suite is in the test itself.
It doesn't correctly compare the output and the reference:

The test will perform a for loop to consume all the input and only the latest block will remain in memory, while it will then compare it to the full length buffer.

@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 4f798ae to d4c5bea Compare September 16, 2023 13:25
@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from d4c5bea to 72eda03 Compare September 16, 2023 13:31
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

sorry for the pedantic comments in regards to our codestyle, but we really want to maintain some consistency here. If there are good reasons to change something we'd love to discuss possible improvements.

src/util/defs.h Outdated Show resolved Hide resolved
src/util/defs.h Outdated Show resolved Hide resolved
src/util/defs.h Outdated Show resolved Hide resolved
@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 72eda03 to d6d7f40 Compare September 16, 2023 15:41
@abique
Copy link
Contributor Author

abique commented Sep 16, 2023

sorry for the pedantic comments in regards to our codestyle, but we really want to maintain some consistency here. If there are good reasons to change something we'd love to discuss possible improvements.

It is your house, keep it clean. 👍

@abique
Copy link
Contributor Author

abique commented Sep 16, 2023

Regarding the max buffer size, a few thing.

  1. it can't be too big, otherwise the GUI will feel bad (vu meters don't refresh often for example)
  2. latency is bad, especially for live performances; I personally think that 256@44.1 is a safe default, 1024@44.1 already feels like shit.
  3. I don't know how often you get the MIDI info, and if you run them ASAP or if you shift them within the block to avoid jitters, but anyway, at large blocks you'll probably feel more "jittery"

I don't think there is a point in working in higher sample rate than 48K except if your HW can't do it.

Then in soundmanagerconfig.h:

    // Size1xms presents the first buffer size of 2^X
    // that results in a buffer time above 1 ms
    // It is 1.45 ms @ 44.1 kHz
    // The other values are representing the following 2^X sizes.
    enum class AudioBufferSizeIndex {
        Size1xms = 1,
        Size2xms = 2,
        Size5xms = 3,
        Size10xms = 4,
        Size20xms = 5,
        Size40xms = 6,
        Size80xms = 7,
    };

    // Represents the sample rate independent frame/period
    // index values in case of Jack
    enum class JackAudioBufferSizeIndex {
        SizeAuto = 5,
        Size2048fpp = 6,
        Size4096fpp = 7,
    };

static constexpr auto kMaxAudioBufferSizeIndex =
            static_cast<unsigned int>(AudioBufferSizeIndex::Size80xms);
static constexpr auto kDefaultAudioBufferSizeIndex =
            static_cast<unsigned int>(AudioBufferSizeIndex::Size20xms);

So at 44.1, min is 64 samples, max 4096.

Now the sample rate:

    static constexpr value_t kValueMin = 8000;   // lower bound (inclusive, = minimum MP3 sample rate)
    static constexpr value_t kValueMax = 192000; // upper bound (inclusive)

You've decided to support up to 192kHz.
According to dlgprefsound.cpp, then you'll let the user choose up to 16384.

Here I suggest three things:

  1. Make the max buffer size at 44.1kHz to 1024 frames or at most 2048. Really 1024 already feels like shit, so I don't think that there's anything good in letting the user put himself in a bad experience. I think pipewire's default is 512 at 44.1?
  2. Limit the max buffer size to 4K, even if the user is at 192kHz. If you want to run at 192kHz, then you probably can afford a low latency (or just use a lower sampling rate...).
  3. Limit the samplerate to 96kHz

I really think that offering 2K at 44.1kHz is not a service to the user.

Let me know in which direction we're going and I'll adjust the settings dialog and clamp it well.

@Swiftb0y
Copy link
Member

I'm not familiar with many parts of the audio engine, but I think we already do support low latency's via low periods, the thing is just that sometimes we just allocate MAX_BUFFER_SIZE upfront but only use the fraction defined by the user. Thats why there is high memory size, but still reasonable latency.

I don't think there is a point in working in higher sample rate than 48K except if your HW can't do it.

That might be true for the majority of users, but some users still want that higher samplerate. Same applies for latency (better to have high latency and no underruns instead of constant underruns). Unless this forces us to make tradeoffs that worsens the experience for the majority, I don't see why we shouldn't support those buffer sizes & samplerates.

I really think that offering 2K at 44.1kHz is not a service to the user.

I think its reasonable to expect users to know what settings they're choosing. If people want to use those settings because they might be forced to its better to let them instead of patronizing them in that regard.

@abique
Copy link
Contributor Author

abique commented Sep 16, 2023

I think its reasonable to expect users to know what settings they're choosing. If people want to use those settings because they might be forced to its better to let them instead of patronizing them in that regard.

Not necessarily, it could be that someone just wants to try mixing, doesn't know about buffer size and its implications and ends up with a setting leading to a bad user experience.

Having more options doesn't means a better beginner experience.

Let's keep the current settings then, but maybe consider advertising 256@44.1kHz as a good default (or even 128). Currently the drop down menu doesn't give any hint, maybe add an auto entry which will pick a good default?

@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from d6d7f40 to b464684 Compare September 16, 2023 19:41
@Swiftb0y
Copy link
Member

Yes, we should definitely try to choose a good default, but we shouldn't take options away from power users or people that know what they're doing just so beginners don't accidentally choose the wrong setting.

@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from b464684 to dbd21c1 Compare September 16, 2023 23:41
src/engine/cachingreader/cachingreaderchunk.h Show resolved Hide resolved
src/engine/cachingreader/cachingreaderchunk.h Show resolved Hide resolved
src/util/defs.h Show resolved Hide resolved
src/util/defs.h Outdated Show resolved Hide resolved
src/util/defs.h Outdated Show resolved Hide resolved
src/test/reference_buffers/SingleChannelOutputWorks-main Outdated Show resolved Hide resolved
@abique abique marked this pull request as draft September 20, 2023 16:45
@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from dbd21c1 to 7edfd32 Compare September 20, 2023 16:45
@abique
Copy link
Contributor Author

abique commented Sep 20, 2023

Something you could consider, is to always run the engine at 44.1kHz or 48kHz, and then down/up sample before reading/writing to the audio interface.

@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 5810308 to 72ccc99 Compare September 20, 2023 17:00
@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch 2 times, most recently from 693f0e5 to 298dc78 Compare September 20, 2023 17:36
@abique
Copy link
Contributor Author

abique commented Sep 20, 2023

I've made MAX_BUFFER_LEN back to were it initially was and tried to use kMaxEngineSamples or kMaxEngineFrames when applicable.

@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 8c7f587 to 84255ce Compare September 20, 2023 17:55
@abique abique marked this pull request as ready for review September 20, 2023 17:56
@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 84255ce to c8b99a5 Compare September 20, 2023 19:47
@abique abique force-pushed the alex/reduce-sample-buffer-memory-usage branch from 3eb8bdb to ba1ac45 Compare September 21, 2023 05:43
@abique
Copy link
Contributor Author

abique commented Sep 21, 2023

BTW, I've run the tests with ASAN.
I've also run the application with ASAN, played a few tracks, used some effects, pitch up/down, stretch... I didn't test everything but I've done basic testing and it was working.

@abique
Copy link
Contributor Author

abique commented Sep 27, 2023

Hi,
Did anyone had a chance to review the changes?
Are you happy now?

@daschuer
Copy link
Member

Yes, I had a brief review and it looks good now. I just want to do some test. Hope I find time soon.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

All is working good. Thank you very much.

@daschuer daschuer merged commit ab8d11f into mixxxdj:main Oct 1, 2023
13 checks passed
@ronso0 ronso0 changed the title Alex/reduce sample buffer memory usage reduce sample buffer memory usage Oct 1, 2023
@abique abique deleted the alex/reduce-sample-buffer-memory-usage branch October 3, 2023 12:26
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