basics: Change sampleFrame to use std::array

[JohannesLorenz]

... in order to make standard containers be able to store it.

[JohannesLorenz]

This PR does 3 things:

• Change sampleFrame and surroundSampleFrame to use std::array
• Remove definition of sampleFrameA (seems to be unused since 2008)
• Change code around libsamplerate use (I haven't checked that code myself yet checked, looks good now)

[JohannesLorenz]

Sorry, I had to do a force-push, it did not compile before.
@Reflexe This branch will have merge conflicts in 2 lines, I marked them with "@recording".

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.2.3-714%2Bga5b449a-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.2.3-714%2Bga5b449a26-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.2.3-714%2Bga5b449a26-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.2.2-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.2.2-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.2.3-714%2Bga5b449a26-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8872-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-714%2Bga5b449a-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8872?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://8873-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-714%2Bga5b449a26-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8873?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8870-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-714%2Bga5b449a26-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8870?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/k3rp3mucni8h1pfg/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35304503"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/eekmkcfrnr7ij79b/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35304503"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://8869-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-714%2Bga5b449a26-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8869?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "e943f188effac66fe95a2d28b168ff7b7802d387"}

[JohannesLorenz]

Ready to review now. @DomClark I added you as reviewer. Of course, feel free to remove yourself if you don't want to review.

IMO there are 2 questions:

1. Should we keep the code like it is in the PR around libsamplerate? @DomClark mentioned he was not sure "what the rules are about passing C++ objects to C code, especially when this is going to access _src[0].data() out-of-bounds". I think the only alternative would be 2 slow memcpys for input and output for each resampling (or not use libsamplerate at all). Maybe it's best to leave it as it is and only change it when anyone complains?
2. In general, there are tons of code possibly affected by this. Most stuff would be array access, but imagine anyone casts sampleFrame* to void*, or uses type traits, or ... It's so much to check, so better don't check it at all?

[PhysSong]

when this is going to access _src[0].data() out-of-bounds

This is a tricky problem, but I think this won't make stuff worse because OOB accesses are problematic when accessing to C objects as well.

imagine anyone casts sampleFrame* to void*, or uses type traits

std::array<T, N> mimics T[N] quite well, but there are some differences including:

• You can cast T[N] to T* implicitly, but you can't do that with std::array
• std::is_array will return false for std::array

However, I don't think that will cause issues, at least for now.

@DomClark Can we merge this as-is?

[DomClark]

Sorry for not getting round to this sooner; I've had other stuff on the past few weeks, but it's all done now.

I agree that the semantic changes you mentioned aren't likely to be a problem:

• You can cast T[N] to T* implicitly, but you can't do that with std::array

  CI passed for this PR, so I guess we don't do this.

• std::is_array will return false for std::array

  GitHub search for is_array in this repo returns no results.

One other difference that might be a problem is what happens when passing these as arguments. T[N] as an argument decays to T*, so you get pointer semantics. std::array will give value semantics instead. See http://cpp.sh/2flqh for an example. I will check the code to see if we rely on this anywhere.

@DomClark Can we merge this as-is?

I saw this report on Discord from @qnebra

Hmm, this is strange. latest master + #5536 freeze during rendering, and later completly crash.
And now clean master to check.
checked - render normally

So I want to take a closer look at this.

[qnebra]

@DomClark To add more context for investigation of this bug, I use a lot of VST in projects. Propably completely irrelevant at all, but maybe will help.

[PhysSong]

@qnebra The freezing might be related to the embedding method(the default is XEmbed on Linux). If you run LMMS from the build directory, it won't use the default LMMS setting file. Can you test this PR with the "none" embedding method? Please make sure you check the setting before you open your project files.

[qnebra]

Well, I use Windows at this moment, also not touched embedding settings in lmms. I will test this further after fixing broken build system on my PC.

[qnebra]

@qnebra The freezing might be related to the embedding method(the default is XEmbed on Linux). If you run LMMS from the build directory, it won't use the default LMMS setting file. Can you test this PR with the "none" embedding method? Please make sure you check the setting before you open your project files.

Export crashes even if only track in project is single 3xOSC, completly default settings.
Here is Win installer of current master with only this PR merged, you will figure out by name as I add "stdarray":

https://drive.google.com/file/d/1caGagpw_IqobJ99E-Nvi9v7_2s4YCBvy/view?usp=sharing

[JohannesLorenz]

@DomClark do you still plan to investigate this?

[DomClark]

I investigate the crashes and freezing, and they appear to be unrelated to this PR. I will get on with checking the code for the potential issue I mentioned above.

[DomClark]

I only found one instance which will break with the new pass-by-value behaviour:

lmms/plugins/Delay/StereoDelay.h

Line 48 in 1dab416

void tick( sampleFrame frame );

It should be sufficient to change it to a reference.

[JohannesLorenz]

I only found one instance which will break with the new pass-by-value behaviour

I just cross-checked your analysis (grepping for '([^(]*sampleFrame[[:space:]]*[^&*].*)'), and found only this occurrence, but no other ones. So I fixed and pushed it. I think that issue is solved now.

I also pushed a commit removing the LocklessRingBuffer specialization for sampleFrame (@he29-net sorry for removing your code! do you want to check my commit 52a8b94?)

@DomClark should we analyze for any other issues? Do you recommend further testing? Or can this be merged?

[DomClark]

LGTM. I can't think of any other potential issues, although obviously that doesn't mean there aren't any. I did my search in a similar way; the only way something might have been missed is if a sampleFrame were used under a different name (using/typedef/template parameter), but I couldn't find any such cases (I only found two template uses - the vector in the LV2 code, and the ringbuffer, which are both fine). I'm not sure what we can do to test this further, other than to cover every bit of code that handles audio, and I think the best way to do that is to have people actually use this. So I think it can be merged.

Code considerations:

• Should the typedefs and C-style float pointer casts be replaced by the more modern using and const_cast? Maybe out of scope, but since those lines are touched anyway they could be updated.
• Are the merge conflict warnings worth keeping? The recording PR already has lots of merge conflicts, including in that same file.

[PhysSong]

The recording PR already has lots of merge conflicts

I think there is a plan to split that into several small parts. I'm not sure if we will do that, BTW.

[JohannesLorenz]

Should the typedefs and C-style float pointer casts be replaced by the more modern using and const_cast?

using sounds good.

The cast to non-const is because libsamplerate used float* data_in (without const) in the past, but they fixed it to const float* data_in in January 2013. If our CI accepts it, can I remove this const cast, or should I keep it for compatibility (and use const_cast)?

Are the merge conflict warnings worth keeping? The recording PR already has lots of merge conflicts, including in that same file.

Somehow I think they can be removed. I'll do that soon.

[JohannesLorenz]

OK, I just tested, our CI fails without the const cast 👎 So I'm now adding const_cast.

[JohannesLorenz]

OK, CI passes, all is reviewed, @he29-net also replied he is ok with 21ee7f1. I'll squash and merge this in the next hours.