Simplify sample frame operations (make it a class)

[michaelgregorius]

Turn sampleFrame into a class

This pull request turns sampleFrame into a class with useful constructors, operators and methods which greatly simplify working with it.

Please refer to the first adjustments in some core classes and plugins to see how much clearer and concise the code becomes. With these changes it becomes possible to remove rather complex code that is repeated over and over again in the code base.

Removal of surroundSampleFrame

The type surroundSampleFrame is removed to enable turning sampleFrame into a class. This feature is not supported anyway (was it ever?).

In the old implementation and with the default definition surroundSampleFrame was interchangeable with sampleFrame because both resolved to an array with two entries. Some code relied on this and thus if the line #define LMMS_DISABLE_SURROUND was commented out in lmms_basics.h then the code did not compile anymore. The reason was that doing so turned surroundSampleFrame into an array with four values instead of two.

I am also not aware of any support for surround sound in the application. The faders and mixers do not seem to support more that two channels and the instruments and effects all expect a sampleFrame, i.e. stereo channels. It therefore makes sense to remove the "feature" so that it does not hinder further improvement and development of the application.

Proposed next steps

I propose the following next steps:

• Merge this pull request as a first "commitment" to the removal of the surround sample frame and the new functionality of sampleFrame.
• Create further pull requests to continue with the adjustment of other code and plugins over time.
• Introduce a buffer class that knows its length and that provides certain functionalities: amplification, finding the max, etc. This will also make the interfaces of the instruments and effects nicer.
• If wanted implement support for more than two channels. This would be a rather big undertaking obviously. The master bus has to expose all audio outputs and the mixer must support appropriate routing of channels.

The "commitment" in the first step is a "safeguard" for me so that I do not spend weeks to adjust the code only for the pull request to be denied.

Note

It's best to review to pull request commit-wise. Commits 1c86e7e90a1 and 974fbacc038 should be most interesting ones as they show what's possible now.

[messmerd]

As a class, sampleFrame should be renamed SampleFrame to follow the UpperCamelCase convention.

[sakertooth]

I cant say I'm a fan of the inheritance to std::array. I would prefer composition over inheritance and move sampleFrame into its own file. Also use the UpperCamelCase style as previously mentioned by @messmerd.

[Veratil]

I cant say I'm a fan of the inheritance to std::array. I would prefer composition over inheritance and move sampleFrame into its own file.

I agree with this. Don't inherit from STL containers, they don't have virtual destructors.

[michaelgregorius]

@sakertooth, @Veratil, I agree. It was the quickest way to add methods to sampleFrame. However, this is now fixed with commit bd408529e24.

[michaelgregorius]

@Veratil, thanks for the (as always) thorough review! I have applied all you suggestions with commit 925a58c35db and came to love the "Add suggestion to batch" feature while doing so. :sweat_smile:

[michaelgregorius]

As a class, sampleFrame should be renamed SampleFrame to follow the UpperCamelCase convention.

@messmerd, I'd like to do this as a last step when it is clear that this PR is accepted and will go through before I start this undertaking, especially since it will potentially touch lots of files.

On the other hand it will hopefully just be a mechanical commit that does not need much code review.

[michaelgregorius]

I have a local branch ready where sampleFrame is renamed to SampleFrame and where it's moved into its own file.

[michaelgregorius]

My plan is to await @DomClark's review, merge this PR if everything is OK and then to do the renaming and the moving in a separate PR that I will create close to this one.

This way it will be clearer to review. Another reason is that there might be build problems because I had to adjust some files, e.g. drivers, "blindly" because I cannot compile them. Doing so will keep things a bit cleaner.

Once the second PR is through the adjustment of all the client code can be continued.

[messmerd]

A while back I looked at some of the code using sample frames and saw that we were using some reinterpret_casts. It may be a good idea to go through and check that this PR will not introduce undefined behavior in those places

[michaelgregorius]

@messmerd, do you mean the casts in LadspaEffect::processAudioBuffer and PlayHandle::buffer? Or are there any other ones?

[messmerd]

Yes, things like that. We should probably see if the reinterpret_casts can be avoided without negatively impacting performance.

[michaelgregorius]

Edit: Problem is fixed and has been identified as being caused by a merge commit with changes unrelated to this PR (b2f2fc4ad13).

This PR should not be merged yet. While removing the reinterpret_casts I noticed that there is a problem with single stream instruments like LB302. Unfortunately, I have no idea what's causing this. I'd be grateful if someone can point me in the right direction.

Steps to reproduce:

1. Build the branch of this PR.
2. Start LMMS.
3. Add an LB302.
4. Play a note on the LB302, e.g. press a key on the keyboard of the instrument.

A debug build then hangs, i.e. no sound is played. If I break execution then the main thread seems to be in the area of some mutexes/locks.

If I load and play the song "AngryLlama-NewFangled" from the demos which also sports some LB302 instances then the main thread hangs in some drawing code.

Some of the affected instruments:

• Gig Player
• LB302
• Opulenz
• Sf2 Player

[michaelgregorius]

Fixed some warnings but they have not been the cause of the problem described above.

[michaelgregorius]

Git bisect shows commit b2f2fc4ad13 as the first bad commit. Perhaps I just need to merge master as the commit does not seem to have caused problems in master or they have been fixed there in the meantime.

[michaelgregorius]

The problem is indeed fixed by a merge with master which was done with commit e9969e769d7.

[michaelgregorius]

@messmerd, I have removed two unnecessary reinterpret_casts with commit d57391341a1.

@DomClark, I consider this pull request ready for review again.

[michaelgregorius]

@DomClark, do you still intend to review this pull request? It is already getting stale and is getting the first merge conflicts that must be resolved.

[DomClark]

Sorry for holding this up - the project is moving quite fast at the moment, and while that's a good thing, there's a bit more going on than I have time to keep up with. I will try to get to this ASAP (hopefully tomorrow).

[michaelgregorius]

Thanks @DomClark! You will find that there is a certain trade-off between ease of operations on stereo samples and performance. It is not possible anymore to use memcpy (see for example the changes that had to be made in commit ba30c425ef7). Zeroing buffers or copying them becomes a bit more "involved" (see the changes in include/lmms_basics.h). However, I did not experience any real impact due to the changes.

The plus-side is that operations with stereo sample become much easier and there is less low-level code repetition which should also lead to lesser bugs and better to understand as well as more compact code. Please refer to the first changes that have been made in some plugins.

[michaelgregorius]

Pushed the renaming to upper-case SampleFrame and moving into it's own file as requested.

[michaelgregorius]

Thanks for the review @DomClark! I have addressed all remarks of the review.

[Rossmaxx]

What's left?

[michaelgregorius]

What's left?

I'm waiting for the final approval of @DomClark.

[michaelgregorius]

Any updates, @DomClark? :wink:

[Rossmaxx]

Also, #7335 simplifies some memory allocation and as a result, brings in some conflicts. Might as well look into it.