Keep Rubberband Faster selected during testing a Rubberband V2 build by daschuer · Pull Request #26 · ronso0/mixxx

daschuer · 2022-07-24T21:44:59Z

No description provided.

coveralls · 2022-07-24T23:03:29Z

Pull Request Test Coverage Report for Build 2744453422

4 of 8 (50.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 34.304%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/engine/enginebuffer.cpp	1	5	20.0%

Totals
Change from base Build 2725155026:	0.02%
Covered Lines:	25723
Relevant Lines:	74986

💛 - Coveralls

ronso0 · 2022-07-25T20:50:56Z

+    if (index >= 0) {
+        keylockComboBox->setCurrentIndex(index);
+    }
+


RUBBERBAND_FASTER is always available and therefore in the combobox.
Why are these safety measures necessary?

The line below can be removed now?

Yes the line below can be removed.

In the current version index and engine have the same integer number.
This may change when a user has engine 5 of a yet unknown future version or such. It will become the last combo box position. Which is 2 or 3.

That is why I have made the selection index independent from the selected engine, which is now stored as user data.
Even though I know the index already Ihere, I look it up for correctness. A kind of future Prove measure.

I'd this OK?

Okay that makes sense.
It' just I don't see how RUBBERBAND_FASTER could not be available. For additional scaler engines one would need to modify the enginebuffer enum in order to make them available, and if RUBBERBAND_FASTER was removed from the enum class (or renamed) static_cast<int>(EngineBuffer::RUBBERBAND_FASTER) wouldn't compile anyway, right?
Or is this supposed to cover the case where RUBBERBAND_FASTER is in the enum but deactivated for some reason?

Oh sorry I think I confused you due to my wrong commit message. RUBBERBAND_FASTER will be always available (for now)

In a future version, let's say we remove Soundtouch, than we should keep the enum value, but remove the selection from the combo box. I will add a comment that the enum values must not be changed only appended.

ronso0 · 2022-07-25T20:52:29Z

Thanks for this PR. Actually I tried that 'static' approach but it didn't work out for some reason.

I left a note and will take a closer look soonish.

ronso0 · 2022-07-25T20:56:03Z

the commit message
"Keep Rubberband Faster selected during testing a Rubberband V2 build"
should be
"Keep 'Rubberband Finer' selected during testing a Rubberband V2 build"

ronso0 · 2022-07-26T08:46:07Z


 #include "audio/frame.h"
 #include "control/controlvalue.h"
+#include "engine/bufferscalers/enginebufferscalerubberband.h"


do I get that right:
besides the other improvements in this PR, including only the scaler header (instead of the rubberband header) is more efficient because of

pragma once (which we can't put in the rubberband header)?

the scaler header is a little smaller than the rubberband header

#pragma once is these days equal efficient than #ifndef RUBBERBAND_STRETCHER_H
The pure size might be an issue, but all this effect only the duration of the build run anyway.

My driver for this change was hiding of information. In the current solution the scaler combobox work without any hard coded knowledge about the available scaler.

There is however the issue with the "EngineBuffer::RUBBERBAND_FASTER" as default value. We may refactor this also into the engine buffer.

Than we are in the situation that we do not have to touch the preferences for any new scaler mode.

What do you think? Shall I add this change as well?

…the default

daschuer · 2022-07-27T06:22:11Z

I have updated the PR including the single source of default idea.

ronso0 · 2022-07-27T08:13:20Z

Nice, thank you!
I'll test this asap (hopefully this week)

Swiftb0y · 2022-07-29T13:33:34Z

+        switch (engine) {
+        case SOUNDTOUCH:
+            return true;
+        case RUBBERBAND_FASTER:
+            return true;
+        case RUBBERBAND_FINER:
+            return EngineBufferScaleRubberBand::isEngineFinerAvailable();
+        default:
+            return false;
+        }
+    }
+
+    static KeylockEngine defaultKeylockEngine() {
+        return RUBBERBAND_FASTER;
+    }


IIRC, we allow compiling without rubberband or soundtouch, right? If so, we need the appropriate ifdefs here.

IIRC, we allow compiling without rubberband or soundtouch, right?

Do we? How?
Until now the enum was fixed.

Currently Rubberband and Soundtouch are "required" in our CMakeList:
https://github.com/daschuer/mixxx/blob/main/CMakeLists.txt#L2461
https://github.com/daschuer/mixxx/blob/main/CMakeLists.txt#L2449

Ok nevermind. I thought I've read some code that handled the absence of a pitch-/timestretching algorithm that lead me to believe they were optional.

Swiftb0y

lgtm otherwise

Swiftb0y · 2022-08-01T14:08:43Z

-        keylockComboBox->addItem(
-                EngineBuffer::getKeylockEngineName(
-                        static_cast<EngineBuffer::KeylockEngine>(i)));
+        auto engine = static_cast<EngineBuffer::KeylockEngine>(i);


Suggested change

auto engine = static_cast<EngineBuffer::KeylockEngine>(i);

const auto engine = static_cast<EngineBuffer::KeylockEngine>(i);

thanks, cherry-picked this (and same change below in loadSettings()).

ronso0 · 2022-08-02T09:29:57Z

Thanks @daschuer and @Swiftb0y
I added (and tweaked) the commits to mixxxdj#4855

github-actions Bot added the ui label Jul 24, 2022

daschuer mentioned this pull request Jul 24, 2022

Rubberband: switch between v2 and v3 at runtime mixxxdj/mixxx#4855

Merged

ronso0 reviewed Jul 25, 2022

View reviewed changes

ronso0 reviewed Jul 26, 2022

View reviewed changes

daschuer added 3 commits July 27, 2022 07:15

Keep Rubberband Finer selected during testing a Rubberband V2 build

695cee7

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

922ff6b

…the default

Add a comment about enum KeylockEngine and backward compatibility

8debe8e

daschuer force-pushed the rubberband3-switch branch from 5391549 to 8debe8e Compare July 27, 2022 05:51

Swiftb0y reviewed Jul 29, 2022

View reviewed changes

Swiftb0y reviewed Aug 1, 2022

View reviewed changes

ronso0 closed this Aug 26, 2022

	auto engine = static_cast<EngineBuffer::KeylockEngine>(i);
	const auto engine = static_cast<EngineBuffer::KeylockEngine>(i);

Conversation

daschuer commented Jul 24, 2022

Uh oh!

coveralls commented Jul 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2744453422

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Jul 25, 2022

Uh oh!

ronso0 commented Jul 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer commented Jul 27, 2022

Uh oh!

ronso0 commented Jul 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swiftb0y left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jul 24, 2022 •

edited

Loading