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

Add support for stem in the engine #13070

Merged
merged 17 commits into from
Jun 25, 2024

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented Apr 7, 2024

This PR refactors the engine to prepare for stem mixing.

Currently, all stems are mixed at the deck level, and continue into the mixer and affect manager as a stereo channel.

Future work will allow the deck to embedded its own effect manager so it can be applied to stem individually.

Depends on #13044

@acolombier acolombier changed the title Add ssupport for stem in the engine Add support for stem in the engine Apr 7, 2024
@daschuer
Copy link
Member

daschuer commented Apr 7, 2024

Does it make sense to allocate new memory if a STEM track is loaded and discard it if another track is loaded and than allocate it again if a new STEM track is loaded?

@acolombier
Copy link
Member Author

acolombier commented Apr 7, 2024

I guess it depends.
In the one hand, I guess we could always allocate memory assuming that maximum number of supported channel, but in the other hand, it feels like a waste to always allocate this max, while many user will probably not use STEM in the early days of the new feature.

Perhaps a good middle ground would be to allocate stereo, them if a STEM gets loaded, re-allocate memory to support stem and keep it as is going forward?

@daschuer
Copy link
Member

daschuer commented Apr 7, 2024

Yes, you are probably right. Than let's stick with the dynamic allocation. This is beneficial if we think to 64 Sampler skins.

@acolombier acolombier force-pushed the feat/add-engine-support-for-stem branch 3 times, most recently from 7012b36 to 20eb0c4 Compare April 11, 2024 21:50
@acolombier acolombier force-pushed the feat/add-engine-support-for-stem branch 4 times, most recently from 142a707 to bee20d5 Compare April 12, 2024 17:25
@acolombier acolombier force-pushed the feat/add-engine-support-for-stem branch from bee20d5 to b31e801 Compare April 12, 2024 18:19
@acolombier acolombier marked this pull request as ready for review April 12, 2024 20:28
@acolombier acolombier mentioned this pull request Apr 13, 2024
@acolombier acolombier force-pushed the feat/add-engine-support-for-stem branch 2 times, most recently from 37573c7 to c70dd17 Compare April 16, 2024 20:11
This was referenced Apr 16, 2024
@acolombier acolombier force-pushed the feat/add-engine-support-for-stem branch 4 times, most recently from b8c19a0 to 2f0518d Compare April 18, 2024 16:19
@acolombier
Copy link
Member Author

That is not a question IMHO, because mono Rubberband destroys the stereo effect and makes no compatibility.

Isn't it a problem we accepted to go with anyway in #13143 and with the user prompt about it? My thoughts was, if the benchmark shows better result, have mono processing for both stereo and stem track when "multi threaded RB" is on, only stereo processing on stem otherwise.

@daschuer
Copy link
Member

daschuer commented Jun 8, 2024

Isn't it a problem we accepted to go with anyway in #13143 and with the user prompt about it?

Yes.

My thoughts was, if the benchmark shows better result,

I am pretty sure benchmarks will show better results, especially on high number of cores, but since the quality is worse and the user expects the best quality when using Rubberband, the default should be stereo processing with four threads in case of stems. The rest can be optional, along with explanations.

Co-authored-by: Daniel Schürmann <[email protected]>
Signed-off-by: Antoine C <[email protected]>
@acolombier acolombier force-pushed the feat/add-engine-support-for-stem branch from da3e4a4 to ccd8a22 Compare June 9, 2024 14:42
@acolombier acolombier force-pushed the feat/add-engine-support-for-stem branch from eb1d773 to f340b1b Compare June 9, 2024 15:26
@acolombier
Copy link
Member Author

I have added the multi threaded rubberband logic:

  • If Multi threaded is off, stereo channel will be treated together
  • If on, stereo and stem decks will be treated in mono

I have also added more logic to try and optimise the channel per worker depending of the core, as no every setup would have a 8 cores or more

@acolombier acolombier requested a review from daschuer June 9, 2024 17:14
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Here some comments

src/util/sample.cpp Outdated Show resolved Hide resolved
src/util/sample.cpp Show resolved Hide resolved
src/util/sample.cpp Show resolved Hide resolved
src/util/sample.cpp Show resolved Hide resolved
src/util/sample.cpp Outdated Show resolved Hide resolved
src/util/sample.cpp Show resolved Hide resolved
src/util/sample.cpp Outdated Show resolved Hide resolved
src/preferences/dialog/dlgprefsounddlg.ui Outdated Show resolved Hide resolved
src/preferences/dialog/dlgprefsounddlg.ui Show resolved Hide resolved
@acolombier acolombier force-pushed the feat/add-engine-support-for-stem branch from 4621cdc to f23f1b2 Compare June 23, 2024 12:20
@daschuer
Copy link
Member

Thank you. Just left some final comments. We have also a conflict.

@acolombier acolombier requested a review from daschuer June 24, 2024 17:53
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@daschuer daschuer merged commit 6b62210 into mixxxdj:main Jun 25, 2024
13 checks passed
@@ -303,6 +306,7 @@ DlgPrefSound::DlgPrefSound(QWidget* pParent,
void DlgPrefSound::slotUpdate() {
m_bSkipConfigClear = true;
loadSettings();
settingChanged();
Copy link
Member

Choose a reason for hiding this comment

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

@acolombier Why was this change necessary?
Thing is, with #12194, Apply will reapply the soundconfig unexpectedly for no obvious reason which causes a playback pause.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was necessary to update the dual threading components, but I guess it could have been in loadSettings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants