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 analyser support for stem #13106

Merged

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented Apr 15, 2024

Make analysers compatible with multi channel and use the best channel for specific features.

As per the NI spec, the first stem is always the drum or rhythmic channel.

This means that:

  • Beat and BPM matching will always be using the first stem
  • Key will use all other channels

All other analyser perform a stereo mix, except if multichannel is supported internally.

Finally, the waveform analyser will gather EQ samples for the mixed track and per-stem samples.

Depends on #13070

@acolombier

This comment was marked as outdated.

@JoergAtGithub
Copy link
Member

I think there's no need to support the Serato format, because it's only used for Seratos internal caching. We don't know if they store additional metadata in their library.
Calculating the replay gain from the 4 stems mixed together makes sense to me. Unless we've a compressor/limiter stage, this seems to be the replay gain, that matters.

@acolombier acolombier force-pushed the feat/add-analyser-support-for-stem branch from 97cb238 to 87b5659 Compare April 20, 2024 17:20
@JoergAtGithub JoergAtGithub added this to the 2.6-beta milestone May 8, 2024
@acolombier acolombier force-pushed the feat/add-analyser-support-for-stem branch from 87b5659 to 036015c Compare May 31, 2024 11:28
@acolombier acolombier force-pushed the feat/add-analyser-support-for-stem branch from 036015c to 8923656 Compare May 31, 2024 16:20
src/waveform/waveform.cpp Outdated Show resolved Hide resolved
src/sources/soundsourcestem.cpp Outdated Show resolved Hide resolved
src/analyzer/analyzerwaveform.h Show resolved Hide resolved
src/analyzer/analyzersilence.cpp Outdated Show resolved Hide resolved
src/analyzer/analyzerkey.cpp Outdated Show resolved Hide resolved
src/analyzer/analyzergain.cpp Outdated Show resolved Hide resolved
@daschuer
Copy link
Member

Can you rebase that onto main?

@acolombier acolombier force-pushed the feat/add-analyser-support-for-stem branch from 19cf8c2 to 2445443 Compare June 25, 2024 06:42
src/analyzer/analyzerkey.cpp Outdated Show resolved Hide resolved
src/analyzer/analyzerwaveform.h Outdated Show resolved Hide resolved
src/util/sample.cpp Outdated Show resolved Hide resolved
@acolombier acolombier force-pushed the feat/add-analyser-support-for-stem branch from 1b5d009 to 1a090a4 Compare July 7, 2024 16:00
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.

The first eatDetectionSettings::StemStrategy settimg alters the analyzer results and shall therefore also be part of the analyzer metadata so that we can later deal with existing analysis of alpha testers. To not put extra work on you for this intermediate solution, it would be also OK to disable or hide the feature to now.

How are existing analysis treated by the way? They have been probably done from the stereo downmix. Do we need to deal with it?

src/analyzer/analyzerkey.cpp Outdated Show resolved Hide resolved
src/util/sample.cpp Outdated Show resolved Hide resolved
src/util/sample.h Outdated Show resolved Hide resolved
src/util/sample.h Outdated Show resolved Hide resolved
@acolombier
Copy link
Member Author

The first eatDetectionSettings::StemStrategy settimg alters the analyzer results and shall therefore also be part of the analyzer metadata

Is that already supported for the others beats and key settings such as the algorithm used for either? If yes, happy to add this as part of that PR too as well.

@daschuer
Copy link
Member

daschuer commented Jul 8, 2024

For beats, it is implemented here:

if (version == newVersion && subVersion == newSubVersion) {

I am not sure if we have similar for keys.

@acolombier
Copy link
Member Author

acolombier commented Jul 8, 2024

For beats, it is implemented here:

if (version == newVersion && subVersion == newSubVersion) {

I am not sure if we have similar for keys.

AFAICS, this is only taken protobuf serialisation version into account, this doesn't invalidate analyses with different settings. Or am I missing something?

On a side note, I'm just wondering, do we want to invalidate older analyses upon analyser changes? I'm thinking it actually sounds wrong since you may want to change your settings temporarily (e.g with our use case, to analyse a non compliant stem) without invalidate other valid analyses. In case you wanted to redo the analyses you always get the option to do so in the context menu. Wdyt?

@daschuer
Copy link
Member

daschuer commented Jul 9, 2024

We already have a preference option for this:

<string>Re-analyze beats when settings change or beat detection data is outdated</string>

But anyway, can we postpone all this and merge this with the unoptimized Analysis?

In case of beats we have the const and the non const detection and version numbers. We must not automatically reanalyze a track, because beat markers may shift away from the cue points, which may creates surprising effects when mixing.

I think we should optimize this workflow:

  • Use reasonable defaults in the preferences
  • Track settings in the analysis metadata to be able to destingush/filter tracks using a certain setting.
  • Allow to reanalyze single tracks with a different setting to fix special issues with a couple of tracks.
  • Allow to reanalyze all tracks in case of improved analyzers on demand.

I think we should not recommend to alter the preference options to fix single tracks.

@acolombier
Copy link
Member Author

can we postpone all this and merge this with the unoptimized Analysis?

Yeah that's fine - I'll hide the option in the setting, so I can still use it via config file, if that's okay with you.

Use reasonable defaults in the preferences

Do you think that Disabled is not the best option here?

I think we should not recommend to alter the preference options to fix single tracks

Agreed - but this is currently how you would deal with tracks needing different options (e.g key plugin). It is suboptimal, but reworking the whole flow is an entire feature so I guess that workaround remain the only way, and thus we should still take it into account.

@daschuer
Copy link
Member

Yeah that's fine - I'll hide the option in the setting, so I can still use it via config file, if that's okay with you.

Yes that's fine. The preferences pane needs anyway some love.

Do you think that Disabled is not the best option here?

For the vast majority of the tracks the optimized analysis will be the best option. Unfortunately we need to deal with the exceptions.
Without that in place let's keep it disabled as a safe default known good form stereo tracks.

It is suboptimal, but reworking the whole flow is an entire feature.

Yes. Since it should work without optimization, postponing is a good idea.

I see the following solution/iterations:

  1. Add a stem mode to the analysis metadata to distinguish it later. (We need it anyway for further improvements of the analyzer)
  2. Only optimize when a the first stem is reasonable labeled.
  3. Add a context menu option to the analyze menu to reanalyze problematic tracks differently.
  4. Allow to add custom Labels that enable optimizations

Does that work for you? (I don't want to put the work on you, just plan possible steps)

@acolombier acolombier requested a review from daschuer July 13, 2024 14:11
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.

Thank you. LGTM. Please give a hint when I can press merge

@daschuer
Copy link
Member

@JoergAtGithub is there something open form your review?

@acolombier
Copy link
Member Author

All good from my end @daschuer

@JoergAtGithub
Copy link
Member

Yes all is fine now! Thank you very much!

@JoergAtGithub JoergAtGithub merged commit 58e0bdf into mixxxdj:main Jul 14, 2024
13 checks passed
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.

4 participants