tenacity-legacy
tenacity-legacy copied to clipboard
Modify memory management of AudioIO and Effects subsystems
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
- [x] I have signed off my commits using
-s
orSigned-off-by
* (See: Contributing § DCO) - [x] I made sure the code compiles on my machine
- [x] I made sure there are no unnecessary changes in the code*
- [x] I made sure the title of the PR reflects the core meaning of the issue you are solving*
- [x] 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
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 5c8f61af8652d30219f2eb5857f7fc4f595e070e 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.
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.
Your commits are difficult to review due to mixing functional changes with style changes.
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.
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.
Maybe beyond the scope of this PR, but why are C arrays being used all over C++ code??
Maybe beyond the scope of this PR, but why are C arrays being used all over C++ code??
Instead of what?
C++ classes with ergonomic zero cost abstractions
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
No, write your own, or use a library with utility classes for working with audio buffers, or maybe copy code from Mixxx.
Can you point to some code you would rewrite?
Every use of a C array in C++ code (except where interfacing with a C library)
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?
Regarding the commit messages, you need to use put brackets (<[email protected]>
) in order for the e-mails to be valid.
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
but before we start on that journey (which I anticipate will take many years),
Welcome to my world. You will need patience, persistence!
I am very familiar with the world of cleaning up horrible technical decisions made long ago.
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.
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.