Rubberband: switch between v2 and v3 at runtime #4855

ronso0 · 2022-07-13T21:29:46Z

Ready for review, though draft since I'll rebase if necessary / when #4853 is merged.

Swiftb0y · 2022-07-14T00:12:56Z

Thanks for the commit. It doesn't build unfortunately (fix is trivial). I'll finish #4853 and and then review this. It'll be a couple of days though.

daschuer

Thank you. I have left some comments for improvement.

src/engine/bufferscalers/enginebufferscalerubberband.cpp

src/engine/bufferscalers/enginebufferscalerubberband.h

daschuer · 2022-07-14T05:32:04Z

src/engine/enginebuffer.cpp

@@ -62,6 +62,8 @@ constexpr SINT kSamplesPerFrame = 2; // Engine buffer uses Stereo frames only
 // Rate at which the playpos slider is updated
 constexpr int kPlaypositionUpdateRate = 15; // updates per second

+#define RUBBERBANDV3 (RUBBERBAND_API_MINOR_VERSION >= 2 && RUBBERBAND_API_MINOR_VERSION >= 7)


Suggested change

#define RUBBERBANDV3 (RUBBERBAND_API_MINOR_VERSION >= 2 && RUBBERBAND_API_MINOR_VERSION >= 7)

#define RUBBERBANDV3 (RUBBERBAND_API_MINOR_VERSION >= 2 && RUBBERBAND_API_MINOR_VERSION >= 7)

But it would be nice to hide the dependency here.

I don't spot the difference in your suggestion?

Agree, including the entirety of RubberbandStretcher.h just for RUBBERBAND_API_MAJOR-/MINOR_VERSION is quite a big dependency, but what other choice do we have?

would a global const defined in enginebufferscalerubberband.h work?

My original point was that the EngineScaler should handle the dependency not EngineBuffer.
Than EngieBuffer can use a member of EngineScaler to query about that.

understood.

We need to differentiate at build time

That as well, thus my naive question about global const (set at compile time in the EngineScaler).

Than EngieBuffer can use a member of EngineScaler to query about that.

and that, too. Though how is that supossed to work for the enum in the EngineBuffer header?

That as well, thus my naive question about global const (set at compile time in the EngineScaler).

The issue with globals consts is that they get to be evaluated after the preprocessor finished. Since we want to do conditional compilation using #if, we need this value sooner, so we need a #define for that then. RubberBandStretcher.h is a pretty lean header (90% is just comments) so I guess its not a big problem to include that a bunch of times.

Though how is that supossed to work for the enum in the EngineBuffer header?

My idea was to remove all the guards from the static code as if we are at a V3 build. If the engine Finer is not available the option need to be hidden from the GUI, that's all.

Sure, but hiding it in the GUI seems more complicated than working with the guards in both EngineBuffer and EngineBufferScaleRubberBand.

Are you suggesting to first populate the keylock combobox with all values, then (probably in
DlgPrefSound::loadSettings) somehow query EngineBuffer / EngineBufferScaleRubberBand to decide if we remove RubberBand finer from the combobox (and also reset the config to RubberBand (faster)) ?

src/engine/enginebuffer.cpp

src/engine/enginebuffer.h

src/engine/bufferscalers/enginebufferscalerubberband.h

ronso0 · 2022-07-16T11:11:22Z

Well, this now builds and I fixed the test (adjusted to changed enum).
I got around the guards in EngineBuffer, but as I said, I don't see a way to hide RubberBand finer) from the pref combobox.¹
I guess with a little work it can at least switch back to RubberBand (faster) if (finer) is not available, but that's bad UX iMO

¹of course there are ways, like using a CO/ControlProxy "isRubberBandV3Available" but that's ugly and hacky (even for my taste) and error-prone.

src/preferences/dialog/dlgprefsound.cpp

daschuer · 2022-07-23T04:41:41Z

Idea:
How about using:
EngineBufferScaleRubberBand::isEngineFinerAvailable()
And populate the "Finer" field only if it is available or if it was selected before.
If we have a Rubberband 2 build we may change pevefix the text with "not installed" or such.

Swiftb0y · 2022-07-23T11:58:04Z

Not a fan of hiding options for no apparent reason. How much more effort would it be to grey it out instead and provide a tooltip explaining why its not available?

daschuer · 2022-07-23T17:20:54Z

Since we do not dynamically link to V2 and V3 users with V2 have no chance to "ungray" the grayed out option. That's why I have proposed to hide it. If we want to use it as a teaser, we need at least a hint how to ungray it.

The only case where wrongly selected is, if the user has selected it in an experimental build and starts Mixxx without V3. Currently the combo-box is empty, but it uses Rubberband Fast. Here we can actually switch back to Rubberband Fast, or inform the user that the selection is not available. This is really an edge case, not worth to code to much around it. IMHO

What do you think?

Swiftb0y · 2022-07-23T17:22:45Z

That's why I have proposed to hide it. If we want to use it as a teaser, we need at least a hint how to ungray it.

Sure, thats what I would've put in tooltip for the greyed out option.

ronso0 · 2022-07-23T20:29:59Z

Idea: How about using: EngineBufferScaleRubberBand::isEngineFinerAvailable() And populate the "Finer" field only if it is available or if it was selected before. If we have a Rubberband 2 build we may change pevefix the text with "not installed" or such.

Again: if you have any solution how to do this without hazzle nad hacks, please present it.
IMO no solution is easier and less error-prone than simply using the guards for the enum in enginebuffer. As @Swiftb0y pointed out the rubberband header is actually rather small.

now the KeylockEngine enum always contains RUBBERBAND_FINER, thus the selector in Preferences > Sound Hardware will always show it, too.

daschuer · 2022-07-24T21:34:53Z

I guess with a little work it can at least switch back to RubberBand (faster) if (finer) is not available, but that's bad UX iMO

In a test branch, I have tried to keep the bad Rubberband value, but since it switches automatically back to default, the value does not persist after another restart. I think the use case is, using Finer in the recent R3 build, test an older Mixxx and go back to the R3 build without loosing the selection of the Finer engine in preference.

With 2.3, the combobox is empty in this case. I think we should display "Unknown (bad value)" for main R2 builds instead and hide the value if a valid value was selected.

daschuer · 2022-07-24T21:46:51Z

Here it is: ronso0#26

It displays now "Unknown, using Rubberband (better)" in case of R3 was selected but is not available in the running build.

ywwg · 2022-07-29T18:10:34Z

Is the rubberband library version a compile-time selection? If so, I would vote to hide the option in the UI. Having a grayed-out option in the prefs for a value which literally cannot be enabled by the user does not communicate anything useful.

Other examples of the same approach:

on linux, I am not presented with ASIO as a sound card access option because there's no way I could possibly use it.
If vinyl control is disabled at compile time, relevant preferences are removed.
We do not show effects that are not installed on the system.

ywwg · 2022-07-29T18:10:41Z

(is this PR still a draft?)

ronso0 · 2022-07-29T20:32:36Z

@ywwg unavailable engines are hidden in ronso0#26 which brings other improvements as well, please take a look.
Still draft until I merge that and clean up this PR, which I'll probably find time for in the next 2-5 days.

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Co-authored-by: ronso0 <ronso0@mixxx.org>

… the default

ronso0 · 2022-08-02T09:30:36Z

With commits from ronso0#26 this is now ready to roll!

Swiftb0y

I didn't want to put more work on you so I went ahead and implemented all my review suggestions for you: ronso0/pull/27

#27) refactor(prefs): improve keylockengine-related typesafety

Swiftb0y

lgtm otherwise

src/preferences/dialog/dlgprefsound.cpp

ronso0 · 2022-08-03T19:02:12Z

would be nice to have two ✔️ before I hit merge : )

daschuer · 2022-08-03T20:39:50Z

I can do a final test after the build issues are solved.

Swiftb0y · 2022-08-03T21:35:15Z

Sorry. The suggestion I posted contained capitalization typos. Sent you a PR.

…Defaults Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

ronso0 · 2022-08-04T20:18:38Z

I don't understand what's wrong with pre-commit
https://github.com/mixxxdj/mixxx/runs/7679981732?check_suite_focus=true
WARNING: The directory '/github/home/.cache/pip' or its parent directory is not owned or is not writable by the current user. The cache has been disabled. Check the permissions and owner of that directory. If executing pip with sudo, you may want sudo's -H flag.

Swiftb0y · 2022-08-04T20:38:24Z

I think versioning issues. seems to me like distlib is installed as part of the systems python installation but something wants a newer distlib version. So in order to install that, it has to uninstall the older version which is part of system install so it can't remove that. I'm not sure. I'm investigating.

Swiftb0y · 2022-08-05T15:38:32Z

@daschuer pre-commit issues are unrelated. Can you do your final test before merge?

daschuer · 2022-08-06T06:19:07Z

Just did a brief test and all works as expected. Thank you all for the good collaboration.

daschuer · 2022-08-06T06:20:11Z

@Swiftb0y merge? Your review is still red.

Swiftb0y

whoops, lgtm.

daschuer · 2022-08-06T21:47:51Z

Thank you.

Rubberband: switch between v2 and v3 at runtime

ronso0 mentioned this pull request Jul 13, 2022

Rubberband V3 stretcher #4853

Merged

daschuer requested changes Jul 14, 2022

View reviewed changes

github-actions bot added ui code quality labels Jul 14, 2022

ronso0 force-pushed the rubberband3-switch branch from d1da15e to c0ea35a Compare July 16, 2022 11:06

daschuer reviewed Jul 23, 2022

View reviewed changes

src/preferences/dialog/dlgprefsound.cpp Outdated Show resolved Hide resolved

ronso0 added 3 commits July 23, 2022 23:30

RubberBand: allow switching v2/v3 at runtime

290ffd7

EngineBuffer: remove RubberBand include and version guards, Note:

107d492

now the KeylockEngine enum always contains RUBBERBAND_FINER, thus the selector in Preferences > Sound Hardware will always show it, too.

Pref Sound: reset to RubberBand v2 if v3 isn't available

447543f

ronso0 force-pushed the rubberband3-switch branch from c75e72c to 447543f Compare July 23, 2022 22:10

daschuer mentioned this pull request Jul 31, 2022

[rubberband] add overlayed rubberband v3 mixxxdj/vcpkg#48

Merged

daschuer and others added 3 commits August 2, 2022 10:44

Keep Rubberband Finer selected during testing a Rubberband V2 build

d862f42

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Co-authored-by: ronso0 <ronso0@mixxx.org>

Introduce EngineBuffer::defaultKeylockEngine() for a single source of…

b4faace

… the default

Add a comment about enum KeylockEngine and backward compatibility

0cfc807

ronso0 mentioned this pull request Aug 2, 2022

Keep Rubberband Faster selected during testing a Rubberband V2 build ronso0/mixxx#26

Closed

ronso0 marked this pull request as ready for review August 2, 2022 09:30

Swiftb0y requested changes Aug 2, 2022

View reviewed changes

refactor(enginebuffer): Make EngineBuffer::KeylockEngine an enum class (

d92a6bf

#27) refactor(prefs): improve keylockengine-related typesafety

Swiftb0y reviewed Aug 3, 2022

View reviewed changes

src/preferences/dialog/dlgprefsound.cpp Outdated Show resolved Hide resolved

keylock engine: use typesafe values also in DlgPrefSound::slotResetTo…

bdebb6e

…Defaults Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

ronso0 force-pushed the rubberband3-switch branch from e4c2530 to bdebb6e Compare August 4, 2022 17:28

ronso0 added this to the 2.4.0 milestone Aug 5, 2022

ronso0 added the engine label Aug 5, 2022

daschuer approved these changes Aug 6, 2022

View reviewed changes

Swiftb0y approved these changes Aug 6, 2022

View reviewed changes

daschuer merged commit 828a189 into mixxxdj:main Aug 6, 2022

ronso0 deleted the rubberband3-switch branch August 26, 2022 12:48

napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 10, 2023

Merge pull request mixxxdj#4855 from ronso0/rubberband3-switch

dde9efd

Rubberband: switch between v2 and v3 at runtime

napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 10, 2023

Merge pull request mixxxdj#4855 from ronso0/rubberband3-switch

fea54af

Rubberband: switch between v2 and v3 at runtime

napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 10, 2023

Merge pull request mixxxdj#4855 from ronso0/rubberband3-switch

21a8cc1

Rubberband: switch between v2 and v3 at runtime

napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 10, 2023

Merge pull request mixxxdj#4855 from ronso0/rubberband3-switch

8dc52db

Rubberband: switch between v2 and v3 at runtime

napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 11, 2023

Merge pull request mixxxdj#4855 from ronso0/rubberband3-switch

7776d64

Rubberband: switch between v2 and v3 at runtime

	#define RUBBERBANDV3 (RUBBERBAND_API_MINOR_VERSION >= 2 && RUBBERBAND_API_MINOR_VERSION >= 7)
	#define RUBBERBANDV3 (RUBBERBAND_API_MINOR_VERSION >= 2 && RUBBERBAND_API_MINOR_VERSION >= 7)

Rubberband: switch between v2 and v3 at runtime #4855

Rubberband: switch between v2 and v3 at runtime #4855

Conversation

ronso0 commented Jul 13, 2022

Swiftb0y commented Jul 14, 2022

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Jul 16, 2022

daschuer commented Jul 23, 2022

Swiftb0y commented Jul 23, 2022

daschuer commented Jul 23, 2022

Swiftb0y commented Jul 23, 2022

ronso0 commented Jul 23, 2022

daschuer commented Jul 24, 2022

daschuer commented Jul 24, 2022

ywwg commented Jul 29, 2022

ywwg commented Jul 29, 2022

ronso0 commented Jul 29, 2022 • edited Loading

ronso0 commented Aug 2, 2022

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

ronso0 commented Aug 3, 2022

daschuer commented Aug 3, 2022

Swiftb0y commented Aug 3, 2022

ronso0 commented Aug 4, 2022

Swiftb0y commented Aug 4, 2022

Swiftb0y commented Aug 5, 2022

daschuer commented Aug 6, 2022

daschuer commented Aug 6, 2022

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer commented Aug 6, 2022

ronso0 commented Jul 29, 2022 •

edited

Loading