Skip to content
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.

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

feat: add advanced stem loading COs #13268

Closed
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
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 CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3466,6 +3466,7 @@ if (STEM)
src/sources/soundsourcestem.cpp
src/track/steminfoimporter.cpp
src/track/steminfo.cpp
src/widget/wtrackstemmenu.cpp
)
if(QOPENGL)
target_sources(mixxx-lib PRIVATE
Expand Down
1 change: 0 additions & 1 deletion src/analyzer/constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ namespace mixxx {
// fixed number of channels like the engine does, usually 2 = stereo.
constexpr audio::ChannelCount kAnalysisChannels = mixxx::kEngineChannelOutputCount;
constexpr audio::ChannelCount kAnalysisMaxChannels = mixxx::kMaxEngineChannelInputCount;
constexpr int kMaxSupportedStems = 4;
constexpr SINT kAnalysisFramesPerChunk = 4096;
constexpr SINT kAnalysisSamplesPerChunk =
kAnalysisFramesPerChunk * kAnalysisMaxChannels;
Expand Down
8 changes: 8 additions & 0 deletions src/engine/cachingreader/cachingreader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,11 @@ CachingReaderChunkForOwner* CachingReader::lookupChunkAndFreshen(SINT chunkIndex
}

// Invoked from the UI thread!!
#ifdef __STEM__
void CachingReader::newTrack(TrackPointer pTrack, mixxx::StemChannelSelection stemMask) {
#else
void CachingReader::newTrack(TrackPointer pTrack) {
#endif
auto newState = pTrack ? STATE_TRACK_LOADING : STATE_TRACK_UNLOADING;
auto oldState = m_state.fetchAndStoreAcquire(newState);

Expand All @@ -220,7 +224,11 @@ void CachingReader::newTrack(TrackPointer pTrack) {
kLogger.warning()
<< "Loading a new track while loading a track may lead to inconsistent states";
}
#ifdef __STEM__
m_worker.newTrack(std::move(pTrack), stemMask);
#else
m_worker.newTrack(std::move(pTrack));
#endif
}

// Called from the engine thread
Expand Down
4 changes: 4 additions & 0 deletions src/engine/cachingreader/cachingreader.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,11 @@ class CachingReader : public QObject {
// Request that the CachingReader load a new track. These requests are
// processed in the work thread, so the reader must be woken up via wake()
// for this to take effect.
#ifdef __STEM__
void newTrack(TrackPointer pTrack, mixxx::StemChannelSelection stemMask = {});
#else
void newTrack(TrackPointer pTrack);
#endif

void setScheduler(EngineWorkerScheduler* pScheduler) {
m_worker.setScheduler(pScheduler);
Expand Down
29 changes: 28 additions & 1 deletion src/engine/cachingreader/cachingreaderworker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,20 @@ ReaderStatusUpdate CachingReaderWorker::processReadRequest(
}

// WARNING: Always called from a different thread (GUI)
#ifdef __STEM__
void CachingReaderWorker::newTrack(TrackPointer pTrack, mixxx::StemChannelSelection stemMask) {
#else
void CachingReaderWorker::newTrack(TrackPointer pTrack) {
#endif
{
const auto locker = lockMutex(&m_newTrackMutex);
#ifdef __STEM__
m_pNewTrack = NewTrackRequest{
pTrack,
stemMask};
#else
m_pNewTrack = pTrack;
#endif
m_newTrackAvailable.storeRelease(1);
}
workReady();
Expand All @@ -113,16 +123,25 @@ void CachingReaderWorker::run() {
// Request is initialized by reading from FIFO
CachingReaderChunkReadRequest request;
if (m_newTrackAvailable.loadAcquire()) {
#ifdef __STEM__
NewTrackRequest pLoadTrack;
#else
TrackPointer pLoadTrack;
#endif
{ // locking scope
const auto locker = lockMutex(&m_newTrackMutex);
pLoadTrack = m_pNewTrack;
m_pNewTrack.reset();
m_newTrackAvailable.storeRelease(0);
} // implicitly unlocks the mutex
#ifdef __STEM__
if (pLoadTrack.track) {
// in this case the engine is still running with the old track
loadTrack(pLoadTrack.track, pLoadTrack.stemMask);
#else
if (pLoadTrack) {
// in this case the engine is still running with the old track
loadTrack(pLoadTrack);
#endif
} else {
// here, the engine is already stopped
unloadTrack();
Expand Down Expand Up @@ -168,7 +187,12 @@ void CachingReaderWorker::unloadTrack() {
m_pReaderStatusFIFO->writeBlocking(&update, 1);
}

#ifdef __STEM__
void CachingReaderWorker::loadTrack(
const TrackPointer& pTrack, mixxx::StemChannelSelection stemMask) {
#else
void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
#endif
// This emit is directly connected and returns synchronized
// after the engine has been stopped.
emit trackLoading();
Expand All @@ -190,6 +214,9 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {

mixxx::AudioSource::OpenParams config;
config.setChannelCount(m_maxSupportedChannel);
#ifdef __STEM__
config.setStemMask(stemMask);
#endif
m_pAudioSource = SoundSourceProxy(pTrack).openAudioSource(config);
if (!m_pAudioSource) {
kLogger.warning()
Expand Down
18 changes: 18 additions & 0 deletions src/engine/cachingreader/cachingreaderworker.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ class CachingReaderWorker : public EngineWorker {
~CachingReaderWorker() override = default;

// Request to load a new track. wake() must be called afterwards.
#ifdef __STEM__
void newTrack(TrackPointer pTrack, mixxx::StemChannelSelection stemMask);
#else
void newTrack(TrackPointer pTrack);
#endif

// Run upkeep operations like loading tracks and reading from file. Run by a
// thread pool via the EngineWorkerScheduler.
Expand All @@ -122,6 +126,12 @@ class CachingReaderWorker : public EngineWorker {
void trackLoadFailed(TrackPointer pTrack, const QString& reason);

private:
#ifdef __STEM__
struct NewTrackRequest {
TrackPointer track;
mixxx::StemChannelSelection stemMask;
};
#endif
const QString m_group;
QString m_tag;

Expand All @@ -134,7 +144,11 @@ class CachingReaderWorker : public EngineWorker {
// lock to touch.
QMutex m_newTrackMutex;
QAtomicInt m_newTrackAvailable;
#ifdef __STEM__
NewTrackRequest m_pNewTrack;
#else
TrackPointer m_pNewTrack;
#endif

void discardAllPendingRequests();

Expand All @@ -147,7 +161,11 @@ class CachingReaderWorker : public EngineWorker {
void unloadTrack();

/// Internal method to load a track. Emits trackLoaded when finished.
#ifdef __STEM__
void loadTrack(const TrackPointer& pTrack, mixxx::StemChannelSelection stemMask);
#else
void loadTrack(const TrackPointer& pTrack);
#endif

ReaderStatusUpdate processReadRequest(
const CachingReaderChunkReadRequest& request);
Expand Down
16 changes: 7 additions & 9 deletions src/engine/channels/enginedeck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@

#ifdef __STEM__
namespace {
constexpr int kMaxSupportedStems = 4;

QString getGroupForStem(const QString& deckGroup, int stemIdx) {
DEBUG_ASSERT(deckGroup.endsWith("]"));
return QStringLiteral("%1Stem%2]")
Expand Down Expand Up @@ -74,18 +72,18 @@ EngineDeck::EngineDeck(
m_pStemCount = std::make_unique<ControlObject>(ConfigKey(getGroup(), "stem_count"));
m_pStemCount->setReadOnly();

m_stemGain.reserve(kMaxSupportedStems);
m_stemMute.reserve(kMaxSupportedStems);
for (int stemIdx = 1; stemIdx <= kMaxSupportedStems; stemIdx++) {
m_stemGain.reserve(mixxx::kMaxSupportedStems);
m_stemMute.reserve(mixxx::kMaxSupportedStems);
for (int stemIdx = 0; stemIdx < mixxx::kMaxSupportedStems; stemIdx++) {
m_stemGain.emplace_back(std::make_unique<ControlPotmeter>(
ConfigKey(getGroupForStem(getGroup(), stemIdx), QStringLiteral("volume"))));
ConfigKey(getGroupForStem(getGroup(), stemIdx + 1), QStringLiteral("volume"))));
// The default value is ignored and override with the medium value by
// ControlPotmeter. This is likely a bug but fixing might have a
// disruptive impact, so setting the default explicitly
m_stemGain.back()->set(1.0);
m_stemGain.back()->setDefaultValue(1.0);
m_stemMute.emplace_back(std::make_unique<ControlPushButton>(
ConfigKey(getGroupForStem(getGroup(), stemIdx), QStringLiteral("mute"))));
ConfigKey(getGroupForStem(getGroup(), stemIdx + 1), QStringLiteral("mute"))));
}
#endif
}
Expand All @@ -98,7 +96,7 @@ void EngineDeck::slotTrackLoaded(TrackPointer pNewTrack,
}
if (m_pConfig->getValue(
ConfigKey("[Mixer Profile]", "stem_auto_reset"), true)) {
for (int stemIdx = 0; stemIdx < kMaxSupportedStems; stemIdx++) {
for (int stemIdx = 0; stemIdx < mixxx::kMaxSupportedStems; stemIdx++) {
daschuer marked this conversation as resolved.
Show resolved Hide resolved
m_stemGain[stemIdx]->set(1.0);
m_stemMute[stemIdx]->set(0.0);
;
Expand Down Expand Up @@ -206,7 +204,7 @@ void EngineDeck::cloneStemState(const EngineDeck* deckToClone) {
VERIFY_OR_DEBUG_ASSERT(deckToClone) {
return;
}
for (int stemIdx = 0; stemIdx < kMaxSupportedStems; stemIdx++) {
for (int stemIdx = 0; stemIdx < mixxx::kMaxSupportedStems; stemIdx++) {
m_stemGain[stemIdx]->set(deckToClone->m_stemGain[stemIdx]->get());
m_stemMute[stemIdx]->set(deckToClone->m_stemMute[stemIdx]->get());
}
Expand Down
16 changes: 16 additions & 0 deletions src/engine/engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,22 @@ static constexpr audio::ChannelCount kEngineChannelOutputCount =
audio::ChannelCount::stereo();
static constexpr audio::ChannelCount kMaxEngineChannelInputCount =
audio::ChannelCount::stem();
// The following constant is always defined as it used for the waveform data
// struct, which must stay consistent, whether the STEM feature is enabled or
// not.
constexpr int kMaxSupportedStems = 4;
#ifdef __STEM__
enum class StemChannel {
First = 0x1,
Second = 0x2,
Third = 0x4,
Fourth = 0x8,

None = 0,
Copy link
Member

Choose a reason for hiding this comment

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

Does None mean the stereo track?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like. At least it is zero if we have no stem file, right. Did you consider to implement #13700 right away.
Is there a use case for "None". Probably in future so we may want to shift Everything by 1 to free up 0x01 for stereo and model the Stem file layout?

Copy link
Member

Choose a reason for hiding this comment

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

Loading the premastered stereo track is clearly out of scope of this PR!
But it's possible to assign any unused value for it. The current implementation of 2^stemNo ensures simple understandable and unique stem masking accross the entire code base.
Therefore I think this is the best possible code design here!

Copy link
Member Author

@acolombier acolombier Sep 30, 2024

Choose a reason for hiding this comment

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

Does None mean the stereo track?

None means no stem track selected, so the deck will be loaded as stem deck, except for samplers, as they cannot load stems. All means load the stereo track

Did you consider to implement #13700 right away.

Please see this comment and this one. TL; DR: loading the pre-mastered stereo is not possible yet.

Probably in future so we may want to shift Everything by 1 to free up 0x01 for stereo and model the Stem file layout?

I don't think we need to - AFAIK, there is 16 usecases, all covered by the flag:

  • 0: load as stem deck
  • 1..14: load as stereo with only the selected stems
  • 15: load as stereo (currently mixed manually to address DSP limitation, using the pre-mastered track when we have the DSP)

Copy link
Member

Choose a reason for hiding this comment

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

If "None" load as "stem deck" for a stem file and the stereo track in case of a normal file, we need to rename it to "Default" or such.

Loading the stereo track IS a valid use case.
Will it be used when all stems are selected or will we than just select all stems?

This question is uncritical if we remove the exposal to the co interface from this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

If "None" load as "stem deck" for a stem file and the stereo track in case of a normal file, we need to rename it to "Default" or such.

Sounds fair, I will change that

Loading the stereo track IS a valid use case.

it is

Will it be used when all stems are selected or will we than just select all stems?

Currently, option 2 because no DSP support. In the future Option 1.

All = First | Second | Third | Fourth
};
Q_DECLARE_FLAGS(StemChannelSelection, StemChannel);
#endif

// Contains the information needed to process a buffer of audio
class EngineParameters final {
Expand Down
15 changes: 14 additions & 1 deletion src/engine/enginebuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1540,12 +1540,25 @@ void EngineBuffer::hintReader(const double dRate) {
}

// WARNING: This method runs in the GUI thread
void EngineBuffer::loadTrack(TrackPointer pTrack, bool play, EngineChannel* pChannelToCloneFrom) {
#ifdef __STEM__
void EngineBuffer::loadTrack(TrackPointer pTrack,
mixxx::StemChannelSelection stemMask,
bool play,
EngineChannel* pChannelToCloneFrom) {
#else
void EngineBuffer::loadTrack(TrackPointer pTrack,
bool play,
EngineChannel* pChannelToCloneFrom) {
#endif
if (pTrack) {
// Signal to the reader to load the track. The reader will respond with
// trackLoading and then either with trackLoaded or trackLoadFailed signals.
m_bPlayAfterLoading = play;
#ifdef __STEM__
m_pReader->newTrack(pTrack, stemMask);
#else
m_pReader->newTrack(pTrack);
#endif
atomicStoreRelaxed(m_pChannelToCloneFrom, pChannelToCloneFrom);
} else {
// Loading a null track means "eject"
Expand Down
11 changes: 10 additions & 1 deletion src/engine/enginebuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,16 @@ class EngineBuffer : public EngineObject {
// Request that the EngineBuffer load a track. Since the process is
// asynchronous, EngineBuffer will emit a trackLoaded signal when the load
// has completed.
void loadTrack(TrackPointer pTrack, bool play, EngineChannel* pChannelToCloneFrom);
#ifdef __STEM__
void loadTrack(TrackPointer pTrack,
mixxx::StemChannelSelection stemMask,
bool play,
EngineChannel* pChannelToCloneFrom);
#else
void loadTrack(TrackPointer pTrack,
bool play,
EngineChannel* pChannelToCloneFrom);
#endif

void setChannelIndex(int channelIndex) {
m_channelIndex = channelIndex;
Expand Down
6 changes: 5 additions & 1 deletion src/library/analysis/analysisfeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@ void AnalysisFeature::bindLibraryWidget(WLibrary* libraryWidget,
&DlgAnalysis::loadTrackToPlayer,
this,
[=, this](TrackPointer track, const QString& group) {
emit loadTrackToPlayer(track, group, false);
emit loadTrackToPlayer(track, group,
#ifdef __STEM__
mixxx::StemChannelSelection(),
#endif
false);
});
connect(m_pAnalysisView,
&DlgAnalysis::analyzeTracks,
Expand Down
13 changes: 12 additions & 1 deletion src/library/autodj/autodjprocessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,14 @@ class AutoDJProcessor : public QObject {
AutoDJError toggleAutoDJ(bool enable);

signals:
#ifdef __STEM__
void loadTrackToPlayer(TrackPointer pTrack,
const QString& group,
mixxx::StemChannelSelection stemMask,
bool play);
#else
void loadTrackToPlayer(TrackPointer pTrack, const QString& group, bool play);
#endif
void autoDJStateChanged(AutoDJProcessor::AutoDJState state);
void autoDJError(AutoDJProcessor::AutoDJError error);
void transitionTimeChanged(int time);
Expand Down Expand Up @@ -230,7 +237,11 @@ class AutoDJProcessor : public QObject {
protected:
// The following virtual signal wrappers are used for testing
virtual void emitLoadTrackToPlayer(TrackPointer pTrack, const QString& group, bool play) {
emit loadTrackToPlayer(pTrack, group, play);
emit loadTrackToPlayer(pTrack, group,
#ifdef __STEM__
mixxx::StemChannelSelection(),
#endif
play);
}
virtual void emitAutoDJStateChanged(AutoDJProcessor::AutoDJState state) {
emit autoDJStateChanged(state);
Expand Down
7 changes: 7 additions & 0 deletions src/library/autodj/dlgautodj.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,14 @@ class DlgAutoDJ : public QWidget, public Ui::DlgAutoDJ, public LibraryView {
signals:
void addRandomTrackButton(bool buttonChecked);
void loadTrack(TrackPointer tio);
#ifdef __STEM__
void loadTrackToPlayer(TrackPointer tio,
const QString& group,
mixxx::StemChannelSelection stemMask,
bool);
#else
void loadTrackToPlayer(TrackPointer tio, const QString& group, bool);
#endif
void trackSelected(TrackPointer pTrack);

private:
Expand Down
13 changes: 13 additions & 0 deletions src/library/library.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -552,14 +552,27 @@ void Library::slotLoadLocationToPlayer(const QString& location, const QString& g
auto trackRef = TrackRef::fromFilePath(location);
TrackPointer pTrack = m_pTrackCollectionManager->getOrAddTrack(trackRef);
if (pTrack) {
#ifdef __STEM__
emit loadTrackToPlayer(pTrack, group, mixxx::StemChannelSelection(), play);
#else
emit loadTrackToPlayer(pTrack, group, play);
#endif
}
}

#ifdef __STEM__
void Library::slotLoadTrackToPlayer(TrackPointer pTrack,
const QString& group,
mixxx::StemChannelSelection stemMask,
bool play) {
emit loadTrackToPlayer(pTrack, group, stemMask, play);
}
#else
void Library::slotLoadTrackToPlayer(
TrackPointer pTrack, const QString& group, bool play) {
emit loadTrackToPlayer(pTrack, group, play);
}
#endif

void Library::slotRefreshLibraryModels() {
m_pMixxxLibraryFeature->refreshLibraryModels();
Expand Down
Loading