Rubberband V3 stretcher

[Swiftb0y]

if we're compiling with rubberband v3 available, automatically use the finer engine. The latest commit builds and works with Rubberband v2 and v3. This is not really intended to be merged because we want the engine to be configurable in the preferences. Consider this PR a starting point for anyone that wants to work on prefence integration.

[Swiftb0y]

I also forgot to mention that with the R3 engine, changing the pitch results in temporary XRuns. We need to investigate that.

[Be-ing]

It would be nice to add a CMake warning for old Rubberband versions noting that a new feature will not be available.

[Swiftb0y]

Sure, I can look into that.

[ronso0]

I also forgot to mention that with the R3 engine, changing the pitch results in temporary XRuns. We need to investigate that.

True, also more xruns in general, CPU increased by ~25% (no waveforms, no spinnies). Still didn't do a sound check.
(tested v2 / v3 with #4855)

[Swiftb0y]

True, also more xruns in general, CPU increased by ~25% (no waveforms, no spinnies).

True, but that is too be expected. As I already mentioned on Zulip, I had to double my buffer size to 10ms, but it works after that (fedora with full preempt kernel).

[Swiftb0y]

It would be nice to add a CMake warning for old Rubberband versions noting that a new feature will not be available.

I tried a bunch of things but can't get anything reliable. I can use the version acquired via PkgConfig, but that will only use the system installation and not the one actually used when specifing alternate library and include dirs.

I don't consider it very important as well...

[Swiftb0y]

I also forgot to mention that with the R3 engine, changing the pitch results in temporary XRuns. We need to investigate that.

I didn't find much time to work on this. I'm pretty sure its an issue within rubberband, but I need to spend more time confirming that.
For now, I consider this PR good enough to be merged. It won't be used much anyways because of the lack of Rubberband 3 availability.

[ywwg]

can we ifdef it based on the rubberband version?

[Swiftb0y]

I was only able to verify that this is not present the newer rubberband version. So I didn't want to introduce regressions based on speculation.

[ywwg]

interestingly I only get underruns when the musical pitch changes. If I turn on keylock I can move around the pitch slider and it doesn't drop at all (5ms buffer). But if I click the up-down arrows for key matching, or if I change key with keylock off and adjust the pitch slider (so that the music pitch changes) I get dropouts on every adjustment

[ywwg]

Can we print out a warning during compilation (or cmake configure time) if the user has rubberband 3 installed to say that the library is only experimentally supported right now? otherwise if we merge this as-is we will get a lot of redundant reports about the dropout issues.

[ronso0]

@ywwg

This is not really intended to be merged because we want the engine to be configurable in the preferences. Consider this PR a starting point for anyone that wants to work on prefence integration.

I built on top of this in #4855 and added the pref option. Please try that (select RBv2) and verify it performs as it does in main.
Given the xruns, I'll re-add the "experimental" disclaimer to the RBv3 option in the preferences until someone tackles the cleanup in src/engine/bufferscalers/enginebufferscalerubberband.cpp

[Swiftb0y]

Can we print out a warning during compilation (or cmake configure time) if the user has rubberband 3 installed to say that the library is only experimentally supported right now? otherwise if we merge this as-is we will get a lot of redundant reports about the dropout issues.

I looked into it, its not trivial. #4853 (comment)

[daschuer]

LGTM, thank you.

[Swiftb0y]

ping me before merging please so I can autosquash the fixup commits.

[Swiftb0y]

Since this already got two approvals, I went a ahead and squashed anyways. Merge?

[daschuer]

Yes, thank you.

[ronso0]

I don't really understand why this was merged without adding a v2/v3 switch first? This is a performance regression for all who have v3 installed. Did I miss something? Or maybe I'm overestimating that because v3 didn't land in any distro yet anyway, and users who install it manually should be aware of the sideeffects?

[Swiftb0y]

This is a performance regression for all who have v3 installed.

Its unlikely someone will have v3 installed.

The plan was to merge #4855 asap if I'm not mistaken.

[daschuer]

I don't really understand why this was merged without adding a v2/v3 switch first?

@ronso0 Oh I must have miss-read your initial comment in #4855 that you wait for this to be merged.

But anyway, I am just working on the final bit and than we can hopefully merge that as well to get around possible regressions.

[ywwg]

yeah normally we like to preserve behavior but having a quick followup seems fine here

[ronso0]

@ronso0 Oh I must have miss-read your initial comment in #4855 that you wait for this to be merged.

Sorry that was unclear. I expected this to be polished, than I'd rebase my followup and we merge both quickly one after another.
Anyway, probably only few are affected so I'll finish mine (ours) asap.