Skip to content

Keep Rubberband Faster selected during testing a Rubberband V2 build #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
daschuer wants to merge 3 commits into from
Closed

Keep Rubberband Faster selected during testing a Rubberband V2 build #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
695cee7
Keep Rubberband Finer selected during testing a Rubberband V2 build
daschuer Jul 24, 2022
922ff6b
Introduce ngineBuffer::defaultKeylockEngine() for a single source of …
daschuer Jul 27, 2022
8debe8e
Add a comment about enum KeylockEngine and backward compatibility
daschuer Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/engine/bufferscalers/enginebufferscalerubberband.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ double EngineBufferScaleRubberBand::scaleBuffer(
return framesRead;
}

// static
bool EngineBufferScaleRubberBand::isEngineFinerAvailable() {
return RUBBERBANDV3;
}
Expand Down
3 changes: 2 additions & 1 deletion src/engine/bufferscalers/enginebufferscalerubberband.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ class EngineBufferScaleRubberBand : public EngineBufferScale {
~EngineBufferScaleRubberBand() override;

// Let EngineBuffer know if engine v3 is available
bool isEngineFinerAvailable();
static bool isEngineFinerAvailable();

// Enable engine v3 if available
void useEngineFiner(bool enable);

Expand Down
17 changes: 6 additions & 11 deletions src/engine/enginebuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,22 +807,17 @@ void EngineBuffer::slotKeylockEngineChanged(double dIndex) {
m_pScaleKeylock = m_pScaleST;
break;
case RUBBERBAND_FASTER:
default:
if (m_pScaleRB->isEngineFinerAvailable()) {
// trigger reconstruction of RubberBandStretcher with v2 options
m_pScaleRB->useEngineFiner(false);
}
m_pScaleRB->useEngineFiner(false);
m_pScaleKeylock = m_pScaleRB;
break;
case RUBBERBAND_FINER:
if (m_pScaleRB->isEngineFinerAvailable()) {
// trigger reconstruction of RubberBandStretcher with v3 options
m_pScaleRB->useEngineFiner(true);
} else {
m_pKeylockEngine->set(static_cast<double>(RUBBERBAND_FASTER));
}
m_pScaleRB->useEngineFiner(
true); // in case of Rubberband V2 it falls back to RUBBERBAND_FASTER
m_pScaleKeylock = m_pScaleRB;
break;
default:
slotKeylockEngineChanged(defaultKeylockEngine());
break;
}
}

Expand Down
27 changes: 25 additions & 2 deletions src/engine/enginebuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

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

@ronso0 ronso0 Jul 26, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@daschuer daschuer Jul 26, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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?

#include "engine/cachingreader/cachingreader.h"
#include "engine/engineobject.h"
#include "engine/sync/syncable.h"
Expand Down Expand Up @@ -74,6 +75,8 @@ class EngineBuffer : public EngineObject {
};
Q_DECLARE_FLAGS(SeekRequests, SeekRequest);

// This enum is also used in mixxx.cfg
// Don't remove or swap values to keep backward compatibility
enum KeylockEngine {
SOUNDTOUCH,
RUBBERBAND_FASTER,
Expand Down Expand Up @@ -150,12 +153,32 @@ class EngineBuffer : public EngineObject {
case RUBBERBAND_FASTER:
return tr("Rubberband (better)");
case RUBBERBAND_FINER:
return tr("Rubberband R3 (near-hi-fi quality)");
if (EngineBufferScaleRubberBand::isEngineFinerAvailable()) {
return tr("Rubberband R3 (near-hi-fi quality)");
}
[[fallthrough]];
default:
return tr("Unknown (bad value)");
return tr("Unknown, using Rubberband (better)");
}
}

static bool isKeylockEngineAvailable(KeylockEngine engine) {
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;
}
Comment on lines +166 to +180

@Swiftb0y Swiftb0y Jul 29, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ronso0 ronso0 Aug 1, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@daschuer daschuer Aug 1, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Swiftb0y Swiftb0y Aug 1, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Request that the EngineBuffer load a track. Since the process is
// asynchronous, EngineBuffer will emit a trackLoaded signal when the load
// has completed.
Expand Down
7 changes: 3 additions & 4 deletions src/engine/enginemaster.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,9 @@ EngineMaster::EngineMaster(
ConfigKey(EngineXfader::kXfaderConfigKey, "xFaderReverse"));
m_pXFaderReverse->setButtonMode(ControlPushButton::TOGGLE);

m_pKeylockEngine = new ControlObject(ConfigKey(group, "keylock_engine"),
true, false, true);
m_pKeylockEngine->set(pConfig->getValueString(
ConfigKey(group, "keylock_engine")).toDouble());
m_pKeylockEngine = new ControlObject(ConfigKey(group, "keylock_engine"), true, false, true);
m_pKeylockEngine->set(pConfig->getValue(ConfigKey(group, "keylock_engine"),
static_cast<double>(EngineBuffer::defaultKeylockEngine())));

// TODO: Make this read only and make EngineMaster decide whether
// processing the master mix is necessary.
Expand Down
48 changes: 28 additions & 20 deletions src/preferences/dialog/dlgprefsound.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,11 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,

keylockComboBox->clear();
for (int i = 0; i < EngineBuffer::KEYLOCK_ENGINE_COUNT; ++i) {
keylockComboBox->addItem(
EngineBuffer::getKeylockEngineName(
static_cast<EngineBuffer::KeylockEngine>(i)));
auto engine = static_cast<EngineBuffer::KeylockEngine>(i);

@Swiftb0y Swiftb0y Aug 1, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ronso0 ronso0 Aug 2, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (EngineBuffer::isKeylockEngineAvailable(engine)) {
keylockComboBox->addItem(
EngineBuffer::getKeylockEngineName(engine), i);
}
}

m_pLatencyCompensation = new ControlProxy("[Master]", "microphoneLatencyCompensation", this);
Expand Down Expand Up @@ -216,15 +218,6 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,

m_pKeylockEngine =
new ControlProxy("[Master]", "keylock_engine", this);
// if EngineBuffer doesn't accept the new engine it resets to RubberBand v2
m_pKeylockEngine->connectValueChanged(
this,
[this](double value) {
m_pSettings->set(ConfigKey("[Master]", "keylock_engine"),
ConfigValue(value));
keylockComboBox->setCurrentIndex(static_cast<int>(value));
},
Qt::DirectConnection);

#ifdef __LINUX__
qDebug() << "RLimit Cur " << RLimit::getCurRtPrio();
Expand Down Expand Up @@ -320,9 +313,9 @@ void DlgPrefSound::slotApply() {
SoundDeviceError err = SOUNDDEVICE_ERROR_OK;
{
ScopedWaitCursor cursor;
m_pKeylockEngine->set(keylockComboBox->currentIndex());
m_pKeylockEngine->set(keylockComboBox->currentData().toDouble());
m_pSettings->set(ConfigKey("[Master]", "keylock_engine"),
ConfigValue(keylockComboBox->currentIndex()));
ConfigValue(keylockComboBox->currentData().toInt()));

err = m_pSoundManager->setConfig(m_config);
}
Expand Down Expand Up @@ -505,10 +498,19 @@ void DlgPrefSound::loadSettings(const SoundManagerConfig &config) {
engineClockComboBox->setCurrentIndex(0);
}

// Default keylock is Rubberband v2
int keylock_engine = m_pSettings->getValue(
ConfigKey("[Master]", "keylock_engine"), 1);
keylockComboBox->setCurrentIndex(keylock_engine);
// Default keylock engine is Rubberband Faster (v2)
int keylock_engine =
m_pSettings->getValue(ConfigKey("[Master]", "keylock_engine"),
static_cast<int>(EngineBuffer::defaultKeylockEngine()));
int index = keylockComboBox->findData(keylock_engine);
if (index >= 0) {
keylockComboBox->setCurrentIndex(index);
} else {
auto engine = static_cast<EngineBuffer::KeylockEngine>(keylock_engine);
keylockComboBox->addItem(
EngineBuffer::getKeylockEngineName(engine), keylock_engine);
keylockComboBox->setCurrentIndex(keylockComboBox->count() - 1);
}

m_loading = false;
// DlgPrefSoundItem has it's own inhibit flag
Expand Down Expand Up @@ -690,8 +692,14 @@ void DlgPrefSound::slotResetToDefaults() {
SoundManagerConfig newConfig(m_pSoundManager.get());
newConfig.loadDefaults(m_pSoundManager.get(), SoundManagerConfig::ALL);
loadSettings(newConfig);
keylockComboBox->setCurrentIndex(EngineBuffer::RUBBERBAND_FASTER);
m_pKeylockEngine->set(EngineBuffer::RUBBERBAND_FASTER);

int keylock_engine = static_cast<int>(EngineBuffer::defaultKeylockEngine());
int index = keylockComboBox->findData(keylock_engine);
DEBUG_ASSERT(index >= 0);
if (index >= 0) {
keylockComboBox->setCurrentIndex(index);
}
m_pKeylockEngine->set(keylock_engine);

masterMixComboBox->setCurrentIndex(1);
m_pMasterEnabled->set(1.0);
Expand Down