Simplify sample frame operations (make it a class) #7156
Conversation
Remove the struct `StereoSample`. Let `AudioEngine::getPeakValues` return a `sampleFrame` instead. Adjust the calls in `Mixer` and `Oscilloscope`.
Some code assumes that `surroundSampleFrame` is interchangeable with `sampleFrame`. Thus, if the line `#define LMMS_DISABLE_SURROUND` is commented out in `lmms_basics.h` then the code does not compile anymore because `surroundSampleFrame` now is defined to be an array with four values instead of two. There also does not seem to be any support for surround sound (four channels instead of two) 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" because it also hinders the improvement of `sampleFrame`, e.g. by making it a class with some convenience methods that act on `sampleFrame` instances. All occurrences of `surroundSampleFrame` are replaced with `sampleFrame`. The version of `BufferManager::clear` that takes a `surroundSampleFrame` is removed completely. The define `SURROUND_CHANNELS` is removed. All its occurrences are replaced with `DEFAULT_CHANNELS`. Most of the audio devices classes, i.e. classes that inherit from `AudioDevice`, now clamp the configuration parameter between two values of `DEFAULT_CHANNELS`. This can be improved/streamlined later. `BYTES_PER_SURROUND_FRAME` has been removed as it was not used anywhere anyway.
Make `sampleFrame` a class with several convenience methods. As a first step and demonstration adjust the follow methods to make use of the new functionality: * `AudioEngine::getPeakValues`: Much more concise now. * `lmms::MixHelpers::sanitize`: Better structure, better readable, less dereferencing and juggling with indices. * `AddOp`, `AddMultipliedOp`, `multiply`: Make use of operators. Might become superfluous in the future.
Add some more operators and methods to `sampleFrame`: * Constructor which initializes both channels from a single sample value * Assignment operator from a single sample value * Addition/multiplication operators * Scalar product Adjust some more plugins to the new functionality of `sampleFrame`.
|
As a class, |
|
I cant say I'm a fan of the inheritance to |
I agree with this. Don't inherit from STL containers, they don't have virtual destructors. |
Using inheritance was the quickest way to enable adding methods to `sampleFrame` without having to reimpement much of `std::array`s interface. This is changed with this commit. The array is now a member of `sampleFrame` and the interface is extended with the necessary methods `data` and the index operator. An `average` method was added so that no iterators need to be implemented (see changes in `SampleWaveform.cpp`).
|
@sakertooth, @Veratil, I agree. It was the quickest way to add methods to |
Apply Veratil's suggestions from the code review Co-authored-by: Kevin Zander <veratil@gmail.com>
…sRemoveSurroundSampleFrame Merge upstream's master into this branch to resolve conflicts in `src/gui/widgets/Oscilloscope.cpp`.
@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. |
|
I have a local branch ready where |
…sRemoveSurroundSampleFrame Conflicts: * plugins/Delay/DelayEffect.cpp
|
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. |
|
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 |
|
@messmerd, do you mean the casts in |
|
Yes, things like that. We should probably see if the reinterpret_casts can be avoided without negatively impacting performance. |
|
Edit: Problem is fixed and has been identified as being caused by a merge commit with changes unrelated to this PR (b2f2fc4). 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:
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:
|
Fix several warnings of the following form: Warnung: »void* memset(void*, int, size_t)« Säubern eines Objekts von nichttrivialem Typ »class lmms::sampleFrame«; use assignment or value-initialization instead [-Wclass-memaccess]
|
Fixed some warnings but they have not been the cause of the problem described above. |
|
Git bisect shows commit b2f2fc4 as the first bad commit. Perhaps I just need to merge |
Uppercase the name of `sampleFrame` so that it uses UpperCamelCase convention.
Move the class `SampleFrame` into its own class and remove it from `lmms_basics.h`. Add forward includes to all headers where possible or include the `SampleFrame` header if it's not just referenced but used. Add include to all cpp files where necessary. It's a bit surprising that the `SampleFrame` header does not need to be included much more often in the implementation/cpp files. This is an indicator that it seems to be included via an include chain that at one point includes one of the headers where an include instead of a forward declaration had to be added in this commit.
|
Pushed the renaming to upper-case |
Return a reference for the compound assignment operators `+=` and `-=`.
Make the constructor that takes a `float` explicit. Remove the assignment operator that takes a `float`. Clients must use the explicit `float` constructor and assign the result. Adjust the code in "BitInvader" accordingly.
Replace `SampleFrame::max` with `SampleFrame::absMax`. Use `absMax` in `DelayEffect::processAudioBuffer`. This should also fix a buggy implementation of the peak computation. Add the function `getAbsPeakValues`. It computes the absolute peak values for a buffer. Remove `AudioEngine::getPeakValues`. It's not really the business of the audio engine. Let `Mixer` and `Oscilloscope` use `getAbsPeakValues`.
Replace the rather mathematical method `scalarProduct` with `sumOfSquaredAmplitudes`. It was always called on itself anyway.
Simplify the `sanitize` function by getting rid of the `bool found` and by zeroing the buffer as soon as a problem is found.
|
Thanks for the review @DomClark! I have addressed all remarks of the review. |
|
What's left? |
I'm waiting for the final approval of @DomClark. |
|
Any updates, @DomClark? 😉 |
|
Also, #7335 simplifies some memory allocation and as a result, brings in some conflicts. Might as well look into it. |
…sRemoveSurroundSampleFrame Conflicts: * include/AudioEngine.h * include/lmms_basics.h * src/core/AudioEngine.cpp
* Remove "#pragme once" * Adjust name of include guard * Remove superfluous includes (leftovers from previous code changes)
Turn
sampleFrameinto a classThis pull request turns
sampleFrameinto 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
surroundSampleFrameThe type
surroundSampleFrameis removed to enable turningsampleFrameinto a class. This feature is not supported anyway (was it ever?).In the old implementation and with the default definition
surroundSampleFramewas interchangeable withsampleFramebecause both resolved to an array with two entries. Some code relied on this and thus if the line#define LMMS_DISABLE_SURROUNDwas commented out inlmms_basics.hthen the code did not compile anymore. The reason was that doing so turnedsurroundSampleFrameinto 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:
sampleFrame.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 1c86e7e and 974fbac should be most interesting ones as they show what's possible now.