tenacity-legacy icon indicating copy to clipboard operation
tenacity-legacy copied to clipboard

Modify memory management of AudioIO and Effects subsystems

Open emabrey opened this issue 2 years ago • 19 comments

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 or Signed-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

emabrey avatar Aug 05 '21 21:08 emabrey

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.

nyanpasu64 avatar Aug 06 '21 04:08 nyanpasu64

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.

emabrey avatar Aug 06 '21 19:08 emabrey

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

Be-ing avatar Aug 07 '21 06:08 Be-ing

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.

nbsp avatar Aug 08 '21 06:08 nbsp

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.

akleja avatar Aug 08 '21 14:08 akleja

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

Be-ing avatar Aug 08 '21 16:08 Be-ing

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

Instead of what?

Paul-Licameli avatar Aug 08 '21 16:08 Paul-Licameli

C++ classes with ergonomic zero cost abstractions

Be-ing avatar Aug 08 '21 16:08 Be-ing

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

Paul-Licameli avatar Aug 08 '21 16:08 Paul-Licameli

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

Be-ing avatar Aug 08 '21 16:08 Be-ing

Can you point to some code you would rewrite?

Paul-Licameli avatar Aug 08 '21 16:08 Paul-Licameli

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

Be-ing avatar Aug 08 '21 16:08 Be-ing

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?

Paul-Licameli avatar Aug 08 '21 16:08 Paul-Licameli

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

n0toose avatar Aug 08 '21 17:08 n0toose

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

Semisol avatar Aug 08 '21 17:08 Semisol

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

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

Paul-Licameli avatar Aug 08 '21 17:08 Paul-Licameli

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

Be-ing avatar Aug 08 '21 18:08 Be-ing

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.

Paul-Licameli avatar Aug 08 '21 18:08 Paul-Licameli

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.

Psychosynthesis avatar May 24 '22 23:05 Psychosynthesis