Skip to content

Conversation

@Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Jun 21, 2024

MemoryHelper contains 1 function alignedMalloc which is used in only AudioEngine. Removed the usage and cleaned up the variables previously using the function to use unique pointers.

@Rossmaxx
Copy link
Contributor Author

I've found out just now that aligned_alloc is missing from windows, that's what is triggering the CI fail.

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

I see no reason for these buffers to be aligned in the first place, so I'd simply replace the aligned allocations with normal ones.

In the future, if we want to make an aligned allocation, it would be better to use the aligned versions of operator new and operator delete, which are more idiomatic in C++ code and supported on all platforms. Ideally we would wrap these in a custom allocator, so we can use standard library containers instead of manually allocating arrays on the heap.

@Rossmaxx
Copy link
Contributor Author

I'd simply replace the aligned allocations with normal ones.

I didn't wanted to introduce any behaviour changes. Also, i have little knowledge on what might break so did the change with minimum cover. If someone wants to refactor, it's completely fine with me. I am just afraid of breaking something unexpected in the process.

If aligning can be removed safely, then it would be better, saving some code and also makes it easier to get into it at a future time. But like i said, something might break somewhere unexpectedly.

@DomClark
Copy link
Member

I had a look and I'm fairly sure nothing will break. Do test it yourself, though.

@sakertooth
Copy link
Contributor

You need to also change the type of the variables m_outputBufferRead and m_outputBufferWrite in AudioEngine.h both to std::unique_ptr<surroundSampleFrame[]>.

@Rossmaxx
Copy link
Contributor Author

I was looking for what caused the error. Thanks for this hint

@sakertooth
Copy link
Contributor

->get should be .get since std::unique_ptr is a class type, not a pointer type.

@Rossmaxx
Copy link
Contributor Author

->get should be .get since std::unique_ptr is a class type, not a pointer type.

learned that the hard way

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

I recommend compiling your changes to see if they work or not before pushing them.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

Be sure to test to see if you can start lmms fine now, just in case.

@Rossmaxx
Copy link
Contributor Author

I can start lmms fine and ran greppi - krem kakkuja and dirtylove projects for 1 - 2 mins each.

@Rossmaxx Rossmaxx merged commit aaca5fb into LMMS:master Jun 24, 2024
@Rossmaxx Rossmaxx deleted the remove-memoryhelper branch June 24, 2024 15:52
Rossmaxx added a commit to Rossmaxx/lmms that referenced this pull request Jun 27, 2024
* remove memory helper and replace alignedMalloc with stl version

* it's aligned_alloc

* forgot about destructor

* switch to unique pointers

Co-authored-by: saker <[email protected]>

* compile fix

---------

Co-authored-by: saker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants