Modify memory management of AudioIO and Effects subsystems #430

emabrey · 2021-08-05T21:41:24Z

Modify memory usage of audio playback and effects preview features to eliminate leaks and use heap memory.

Signed-off-by: Emily Mabrey [email protected]
Helped-by: Alex Disibio [email protected]
Helped-by: Rikard Jansson [email protected]

This is a remake of the previous PR because the previous one wasn't broad enough to cover all the changes within the scope of this one.

Checklist

I have signed off my commits using -s or Signed-off-by* (See: Contributing § DCO)
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code*
I made sure the title of the PR reflects the core meaning of the issue you are solving*
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"*

* indicates required

nyanpasu64 · 2021-08-06T04:56:36Z

Fails to build on Windows with MSVC:

C:\Users\nyanpasu64\code\tenacity\src\AboutDialog.cpp:241: error: C2065: 'USE_PORTMIXER': undeclared identifier

C:\Users\nyanpasu64\code\tenacity\src\AboutDialog.cpp:241: error: C2665: 'AboutDialog::AddBuildInfoRow': none of the 2 overloads could convert all the argument types
C:\Users\nyanpasu64\code\tenacity\src\AboutDialog.h(90): note: could be 'void AboutDialog::AddBuildInfoRow(wxTextOutputStream *,const TranslatableString &,const wxChar *)'
C:\Users\nyanpasu64\code\tenacity\src\AboutDialog.cpp(241): note: while trying to match the argument list '(wxTextOutputStream *, const wchar_t [10], TranslatableString)'

master 5c8f61a builds fine.

Replacing this line with AddBuildInfoRow(&informationStr, wxT("PortMixer"), XO("Sound card mixer support"), disabled); (treating portmixer as disabled) fixes both errors, and the build completes successfully.

emabrey · 2021-08-06T19:53:13Z

Fails to build on Windows with MSVC:

C:\Users\nyanpasu64\code\tenacity\src\AboutDialog.cpp:241: error: C2065: 'USE_PORTMIXER': undeclared identifier

C:\Users\nyanpasu64\code\tenacity\src\AboutDialog.cpp:241: error: C2665: 'AboutDialog::AddBuildInfoRow': none of the 2 overloads could convert all the argument types
C:\Users\nyanpasu64\code\tenacity\src\AboutDialog.h(90): note: could be 'void AboutDialog::AddBuildInfoRow(wxTextOutputStream *,const TranslatableString &,const wxChar *)'
C:\Users\nyanpasu64\code\tenacity\src\AboutDialog.cpp(241): note: while trying to match the argument list '(wxTextOutputStream *, const wchar_t [10], TranslatableString)'

master 5c8f61a builds fine.

Replacing this line with AddBuildInfoRow(&informationStr, wxT("PortMixer"), XO("Sound card mixer support"), disabled); (treating portmixer as disabled) fixes both errors, and the build completes successfully.

That is a bug which is already addressed in master, I just needed to pull changes and merge.

Be-ing · 2021-08-07T06:54:00Z

Your commits are difficult to review due to mixing functional changes with style changes.

nbsp · 2021-08-08T06:40:12Z

Your commits are difficult to review due to mixing functional changes with style changes.

Agreed. This needs to be rebased into at least three separate commits, for cache-friendliness, memory leak fixes, and stylistic changes.

akleja · 2021-08-08T14:13:14Z

I've done quite a bit of testing and ran a memory analysis both with and without using realtime effects. No new memory leaks and no other issues that I can find.
There is another audio playback issue, but it is not related to memory management but rather device handling, so I'm opening a separate issue about that.
I leave the thorough code inspection to those more qualified, but from what I can tell everything looks good.

Be-ing · 2021-08-08T16:07:31Z

Maybe beyond the scope of this PR, but why are C arrays being used all over C++ code??

Paul-Licameli · 2021-08-08T16:10:56Z

Maybe beyond the scope of this PR, but why are C arrays being used all over C++ code??

Instead of what?

Be-ing · 2021-08-08T16:11:41Z

C++ classes with ergonomic zero cost abstractions

Paul-Licameli · 2021-08-08T16:13:59Z

C++ classes with ergonomic zero cost abstractions

I don't get your fancy words. Name a standard C++ class that you mean. This, maybe? But it's only for use with an array bound known at compile time. https://en.cppreference.com/w/cpp/container/array

Be-ing · 2021-08-08T16:15:03Z

No, write your own, or use a library with utility classes for working with audio buffers, or maybe copy code from Mixxx.

Paul-Licameli · 2021-08-08T16:16:00Z

Can you point to some code you would rewrite?

Be-ing · 2021-08-08T16:17:37Z

Every use of a C array in C++ code (except where interfacing with a C library)

Paul-Licameli · 2021-08-08T16:28:36Z

There is much in a 20 year old accumulation of code dating from only shortly after the 1998 C++ standard, that could be made nicer and more modern, using types and templates and the newer standard library more smartly.

See TranslatableString for instance, added in many many places, with the advantage that it stops careless neglect of internationalization of new literals with compilation failures.

I know the guy who did that. He also made a comprehensive cleanup of naked news and deletes in favor of smart pointers, so he knows a thing about allocations too. But even that guy has limited time available to fix every primitive C idiom that the early developers hadn't unlearned.

The transformations I named brought a real advantage in maintainability. What advantage would your rewrites bring?

n0toose · 2021-08-08T17:00:31Z

Regarding the commit messages, you need to use put brackets (<[email protected]>) in order for the e-mails to be valid.

Semisol · 2021-08-08T17:02:35Z

Regarding the commit messages, you need to use put brackets (<[email protected]>) in order for the e-mails to be valid.

...? they are already there, GitHub converts emails sometimes to w/o brackets but as a link

Be-ing

It's difficult to find the functional changes here mixed in with the formatting changes. I cannot in good conscious approve merging this without a clear view of what is actually changing. The little bit I did notice is very concerning as it introduces more time unbounded behavior in the audio thread.

I think we should revert #412, step back, and reconsider priorities here. There are tons of problems to fix in this codebase, but before we start on that journey (which I anticipate will take many years), I think we should focus on getting a first release out. For that we need to clean up the build system (#228) and complete rebranding. Then let's dig into the C++ and start refactoring.

Be-ing · 2021-08-08T17:29:19Z

src/AudioIO.cpp

@@ -3979,6 +3979,7 @@ bool AudioIoCallback::FillOutputBuffers(
       // wxASSERT( maxLen == toGet );

       em.RealtimeProcessEnd();
+       delete bufHelper.release();


No time unbounded behavior in the audio thread.

Paul-Licameli · 2021-08-08T17:53:36Z

but before we start on that journey (which I anticipate will take many years),

Welcome to my world. You will need patience, persistence!

Be-ing · 2021-08-08T18:00:15Z

I am very familiar with the world of cleaning up horrible technical decisions made long ago.

Paul-Licameli · 2021-08-08T18:07:03Z

I am very familiar with the world of cleaning up horrible technical decisions made long ago.

We may be attuned to different horriblenesses. I have worked for years on several that may be disjoint from your concerns. Using RAII better, using types better, and, ongoing, fixing problems in the compilation and linkage dependency graph that make the program too monolithic.

src/AudioIO.cpp

Be-ing · 2021-08-09T12:55:15Z

src/effects/RealtimeEffectBufferHelper.h

+#ifndef REALTIME_EFFECT_BUFFER_HELPER_H
+#define REALTIME_EFFECT_BUFFER_HELPER_H


Use #pragma once in new files

We should 100% not use #pragma once standalone. We can add it in addition, if you want, since that may speed up compilation in some situations. But I actually already looked into this exact issue probably 4 days ago and came to the conclusion using a #pragma here isn't the best idea.

src/AudioIOBufferHelper.h

Be-ing · 2021-08-09T12:58:58Z

src/effects/RealtimeEffectBufferHelper.h

+        this->chans = chans;
+        this->numSamples = numSamples;


Move this to an initializer list

src/effects/RealtimeEffectBufferHelper.h

Be-ing · 2021-08-09T13:14:52Z

src/effects/RealtimeEffectBufferHelper.h

+    float** obuf;
+    float* temp;
+
+    RealtimeEffectBufferHelper(float** buffers, const unsigned int chans, const size_t numSamples) {


Is this number of samples or number of frames?

It's not obvious what the difference between the buffers parameter passed into this constructor is and the variables initialized within it. If I had a documentation comment explaining what the purpose of the class was that might be more apparent, but as it is I have to thoroughly read the entire implementation of the class to understand this.

src/effects/RealtimeEffectBufferHelper.h

Be-ing · 2021-08-09T13:17:47Z

src/effects/RealtimeEffectBufferHelper.h

+        this->chans = chans;
+        this->numSamples = numSamples;
+
+        // Allocate the in/out buffer arrays


Buffers are arrays, so is this just a buffer or an array of buffers?

This would be much clearer if there was a C++ class for buffers rather than writing C inside C++

src/effects/RealtimeEffectBufferHelper.h

Be-ing · 2021-08-09T13:23:17Z

src/effects/RealtimeEffectBufferHelper.h

+    public:
+
+    // Allocate the in/out buffer arrays
+    float** ibuf;


float** inputBuffer;
spell out variables please

Or is this inputBuffers, plural?

I'm copying the original naming.

src/effects/RealtimeEffectBufferHelper.h

src/AudioIO.cpp

Be-ing

I can't adequately review this without a ton of effort with whitespace changes intermixed with functional changes. Is your editor doing this automatically? If so, please reconfigure it to stop doing so.

Improves performance of project loading substantively. Signed-off-by: Emily Mabrey <[email protected]> Helped-by: Alex Disibio <[email protected]>

Signed-off-by: Emily Mabrey <[email protected]>

Psychosynthesis · 2022-05-24T23:19:11Z

I have nothing to do with this PR, but I want to support doubts about the advisability of replacing C-type arrays with something more unwieldy unnecessarily.

emabrey assigned emabrey and akleja Aug 5, 2021

emabrey linked an issue Aug 6, 2021 that may be closed by this pull request

Pop when starting playback on WASAPI backend #288

Open

1 task

emabrey mentioned this pull request Aug 6, 2021

Pop when starting playback on WASAPI backend #288

Open

1 task

emabrey added this to the 0.1.0 milestone Aug 7, 2021

emabrey marked this pull request as ready for review August 8, 2021 03:44

emabrey requested review from akleja, Be-ing and a team August 8, 2021 04:57

Be-ing suggested changes Aug 8, 2021

View reviewed changes

emabrey force-pushed the modify_memory_audio_subsystems branch 2 times, most recently from cbf8a24 to 0572d94 Compare August 9, 2021 09:16