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

BeatDetectionSettings: change minimum BPM to 60 #2693

Closed
wants to merge 1 commit into from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Apr 21, 2020

I have a few tracks that are really between 60-69.9 BPM, but I have none that are actually below 60.

This requires #2692 to work.

@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 21, 2020

Only 2 of the 12 tracks I had detected as between 60-69.9 BPM were actually double that tempo. The rest were dub, blues, and ambient that is actually 60-69.9 BPM.

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.

I think we need to clarify the implications of this setting.

Here:
https://www.musical-u.com/learn/rhythm-tips-for-identifying-music-genres-by-ear/
I see a much wider range if Bpms

My hardware metronome had the range from 40 to 208.

I think we have original picked 70 to 140, because this will move all tracks in a region where they can be beatmatched. Your 60 Bpm track will become 120 and works perfect with the maxority of the other tracks.

With 60 as start value this "feature" is broken.

The limits are only applied for const beatgrids, and since we want to move away from it. This will be soon not relevant anymore.

@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 21, 2020

I think we have original picked 70 to 140, because this will move all tracks in a region where they can be beatmatched. Your 60 Bpm track will become 120 and works perfect with the maxority of the other tracks.

Intentionally making data incorrect is a horribly ugly hack if this is the intended use case! I want my 68 BPM tracks sorted by my 72 BPM tracks, not by my 136 BPM tracks. If we actually want to support such a use case, let's make an easy way for users to identify tracks that are near double or half a BPM. But that is beyond the scope of this PR.

I have a few tracks that are really between 60-69.9 BPM, but I
have none that are actually below 60.
@daschuer
Copy link
Member

I use non-const beat grids so I don't mind about this feature.

I have just explained why this was introduced. It was designed before we had a sync engine that can handle double and half syncing.

The point here is that if we change the minimum to 60, we breaks the original feature of showing compatible tracks side by side.

If we want to show the real tempo in the library, we also need to adjust the upper limit to a value from the link.

Maybe it is reasonable to keep the 70 to 140 range and disable the limits by a checkbox or something.

@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 21, 2020

I disagree about the maximum BPM feature. It is not to intentionally make data incorrect, it is a hack around the analyzer's incorrect data. Most my tracks detected as >140 BPM are actually half the detected tempo. A few are really >140 BPM. The converse is not true; most detected tracks <70 BPM are actually <70 BPM. For those that are actually >140 BPM, I can manually double the BPM, which is finally easy to do since #2612.

The limits are only applied for const beatgrids, and since we want to move away from it. This will be soon not relevant anymore.

I disagree. This is likely just as relevant for variable tempo beatgrids.

Anyway, these points are all beyond the scope of this simple change to only the minimum BPM.

@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 21, 2020

Maybe it is reasonable to keep the 70 to 140 range and disable the limits by a checkbox or something.

We literally just removed this unexposed setting in #2692. There is no need for it. The user can just enter a very wide range for the BPM values if they want.

@daschuer
Copy link
Member

I disagree...

There is nothing to agree and disagree about.

It is the 2.2 case that currently all tracks are by default artificially moved into the same region of 70 to 140 = 2 * 70. This implicit help to find matching tracks.

I don't think this is a good design choice for Mixxx 2.3.

If we change the default away from this, we need to choose a realistic range that covers all possible bpm, and sort out unplausible an unhandy values.
This can be Like 60 to 240, or something, with x 4. The limit of our sync engine.

If we want to fix a bug in the beat detector as you mentio it should be not exposed to the user.

So I think we can agree to one of these alternatives:

  • 70 - 140
  • Min - min * 4
  • Limits from music theory.

And decide for a hidden maximum fix.

@uklotzde
Copy link
Contributor

I don't see the point why we need to discuss hypothetical limits for the default limit? It should sufficiently fit the Top 40 use case. Everyone is free to adjust this setting depending on their musical style.

  • Serato Pro: None
  • Rekordbox: 70-180 bpm
  • Engine Prime: 78-155 bpm

All competitors only allow to select among predefined ranges, no support for custom ranges.

I'm fine with the lowering the limit to 60. A default range of 60-180 bpm (min = 3x max) would cover even more cases, but on the other hand might get the tempo of many slower tracks wrong.

@daschuer
Copy link
Member

It is all about the what we like to acheve by default:

  1. show the output of the detector
  2. limit the Bpm range to the capacity of the sync engine.
  3. group compatible tracks.

In 2.2 we do 3) for the not existing legacy reason of the sync capacity. So the basic idea of this PR to remove it is good.

So we just need to decide between 1) or 2)

I would vote for 1).
Because I like to find my slow am crazy fast tracks in the library.

The issue we have to check what happens with the detector, since these values are input values as well.

If high results are unpredictable, they do not become predictable if we devide the result by two. This just obfusticates an isssue.
It is up to the user to select the right factor.

Listing the high and low values in the library helps to find the tracks that needs some more love.

@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 21, 2020

It is all about the what we like to acheve by default:
show the output of the detector
limit the Bpm range to the capacity of the sync engine.
group compatible tracks.

None of these are what I want or expect. I want the true BPM. That is it. Don't overthink it. Whatever we can do to get close to the true BPM is what we should do.

If high results are unpredictable, they do not become predictable if we devide the result by two. This just obfusticates an isssue.

This is not what I found when analyzing my whole library without any limits. Most of the tracks >140 BPM were detected as exactly double their true BPM. Again, the reason for this PR is that the converse is not true. Most tracks detected as <70 BPM are actually slow tracks.

@JosepMaJAZ
Copy link
Contributor

So... what has happened from what we talked on https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/BPM.20analysis.20improvements ?

Your last comment was "Maybe we should disable the range by default", because you were considering that a few more bad cases shouldn't force other possibly correct results to be wrong.

As Daniel said, the 70-140 range (70-139 better) just allows the detector to not overestimate tracks (underestimate is rare, as you also saw).
That's why my range is 90-179, that also prevents overestimation (above 170). Of course it overestimates if the track is actually below 90.

But even with this, I also have to fix incorrectly detected tracks, so which is really the default user experience?

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 19, 2020
@Be-ing Be-ing closed this Nov 25, 2020
@Be-ing Be-ing deleted the branch mixxxdj:main November 25, 2020 15:51
@Be-ing Be-ing reopened this Nov 25, 2020
@Be-ing Be-ing changed the base branch from master to main November 25, 2020 16:06
@ronso0 ronso0 marked this pull request as draft February 14, 2021 21:55
@Holzhaus
Copy link
Member

Holzhaus commented Apr 2, 2021

Minimum BPM has been removed from the preferences, but there are not merge conflicts so min/max BPM values still exist in our code. Is this still relevant?

@daschuer
Copy link
Member

daschuer commented Apr 2, 2021

No idea why GitHub does not show a conflict. At least the changed line is gone. So we can close this.
See:

DEFINE_PREFERENCE_HELPERS(FixedTempoAssumption, bool,

@daschuer daschuer closed this Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine stale Stale issues that haven't been updated for a long time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants