-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Intro/Outro cues and silence detection #1242
Conversation
Implement simple threshold-based silence detection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this @ninomp! I think you are also fixing this bug:
https://bugs.launchpad.net/mixxx/+bug/673515
src/analyzer/analyzersilence.cpp
Outdated
} | ||
|
||
bool AnalyzerSilence::initialize(TrackPointer tio, int sampleRate, int totalSamples) { | ||
Q_UNUSED(tio); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't enable the analyzer if the track already has an in/out cue (since the user can manually edit their cues, so we don't know if we are overwriting user data)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may consider her to move the cue points if they are finally independent from auto DJ an they are not user editable
src/analyzer/analyzersilence.cpp
Outdated
m_iTotalSamples = totalSamples; | ||
m_iFramesProcessed = 0; | ||
m_bPrevSilence = true; | ||
m_iSignalBegin = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would initialize with -1 to distinguish between not found and 0.
src/analyzer/analyzersilence.cpp
Outdated
|
||
bool bBeginPointFoundAndSet = false; | ||
bool bEndPointFoundAndSet = false; | ||
QList<CuePointer> cues = tio->getCuePoints(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should edit cues since at the point we create them they become user data and we can't tell if they were edited by the user
src/analyzer/analyzersilence.cpp
Outdated
AnalyzerSilence::AnalyzerSilence(UserSettingsPointer pConfig) | ||
: m_pConfig(pConfig), | ||
m_fThreshold(db2ratio(kSilenceThreshold)), | ||
m_iSampleRate(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused, please remove m_iSampleRate and m_iTotalSamples
} | ||
|
||
void AnalyzerSilence::finalize(TrackPointer tio) { | ||
// If track didn't end with silence, place signal end marker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle m_iSignalBegin < 0 and m_iSignalEnd < 0 explicitly here ?
src/library/dao/cue.h
Outdated
@@ -21,6 +21,8 @@ class Cue : public QObject { | |||
BEAT = 3, | |||
LOOP = 4, | |||
JUMP = 5, | |||
BEGIN = 6, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might make more sense to call these "fade in" and "fade out" cues (or autodj_in / autodj_out), since that's more aligned with the purpose.
Tracks may not have a beginning or end, and user's probably don't particularly care about having the beginning and end of the track annotated, but they do care about annotating where AutoDJ would fade in and out. Plus if they edit the cue to be located where they want AutoDJ to fade in, then it doesn't point at the track start anymore so it'd be misnamed.
src/engine/cuecontrol.cpp
Outdated
@@ -102,6 +102,9 @@ CueControl::CueControl(QString group, | |||
m_pCueIndicator = new ControlIndicator(ConfigKey(group, "cue_indicator")); | |||
m_pPlayIndicator = new ControlIndicator(ConfigKey(group, "play_indicator")); | |||
|
|||
m_pSignalBeginPosition = new ControlObject(ConfigKey(group, "signal_begin_position")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
along with renaming the cue type, please call these
fade_in_cue_point
fade_out_cue_point
or similar?
res/skins/LateNight/deck_row_5.xml
Outdated
@@ -41,6 +41,20 @@ | |||
<Color>#FF001C</Color> | |||
<TextColor>#FFFFFF</TextColor> | |||
</Mark> | |||
<Mark> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we get closer to merging, please remember to update Deere and Shade.
src/analyzer/analyzersilence.cpp
Outdated
@@ -0,0 +1,104 @@ | |||
#include "analyzer/analyzersilence.h" | |||
|
|||
const int kChannelCount = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use mixxx::AudioSource::kChannelCountStereo
src/analyzer/analyzersilence.cpp
Outdated
#include "analyzer/analyzersilence.h" | ||
|
||
const int kChannelCount = 2; | ||
const float kSilenceThreshold = -60.0f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please name it kSilenceThresholdDb to document the unit
I think we schould treat the silence start an silence end points independent from the auto DJ fade in and fade out points. In the jukebox use case, we want to play the tracks in a whole, with a equal gap of silence in between. What do you think? |
Do we need to consider the replay gain of a track or is -60 dB of full scale so quite that a replay gain does not matter? If yes, this feature should be part of the replay gain analysis: The function form the ReplayGain2 analysis is ebur128_loudness_momentary mixxx/lib/libebur128-1.1.0/ebur128/ebur128.h Line 233 in 8d191bf
it uses a 3 s window. A momentary Loudness of replay1 gain is calculated here with a 50 ms window. mixxx/lib/replaygain/replaygain.cpp Line 246 in 8d191bf
I am not sure how much we should overcomplicate this feature that much. |
Well, I was thinking about using the replay gain value to adjust the -60dB threshold value, but that means that we need multi-pass analysis (in first pass compute the RG, in second detect silence) which seems like an overkill (at least to me). For now, I would like to continue testing this simple-threshold-on-a-raw-signal silence detection and see how it works with various tracks from my library. So far, this simple algorithm seems to be performing quite good. Also, I would like to allow user to change silence detection threshold in preferences, but I'm afraid that it might cause problems if inexperienced users change it to very low or very high value. What do you think? |
Regarding the cue points, previously all cue points were set/created by user and now I am introducing cue points that are set (and moved if threshold changes) by Mixxx and which need to be modifiable by user. As I see it, we have three options:
What do you think? Which one of these is better way? |
One other thing I am also thinking of adding to this PR is automatic placement of main CUE point at the detected signal begin position if the main CUE point is not set. (I will quantize to nearest beat if quantization is enabled.) I don't think that we need a preference option for this. Do you agree? |
I agree, this would be good to do. (Pioneer CDJs have been doing this for over a decade. It's about time Mixxx does it as well. ;) ) I would go further and set it to the detected begin point also if the main CUE point is set to the very beginning of the track. |
I agree two passes are overdone. A combined path may work.
We schould try to go without a preference option. IMHO this option is somehow track depending and tweaking it for one track will destroy a good result for an other. |
since it is finally a user editable point, it does make sense to treat it as special cue point and not as a database column. An alternative would to track it along with the lock feature of the beat grid, which is user editable as well. Generally I think we need two sets of points the sound start point honoured by auto DJ in jukebox mode and the fade-in/out points used for crossfading/beatmatching. |
src/analyzer/analyzersilence.cpp
Outdated
|
||
void AnalyzerSilence::process(const CSAMPLE *pIn, const int iLen) { | ||
for (int i = 0; i < iLen; i += kChannelCount) { | ||
bool bSilence = math_max(fabs(pIn[i]), fabs(pIn[i + 1])) < m_fThreshold; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit assumption of 2 channels! Please make sure that your code is generic and works with any number of channels in kChannelCount. Otherwise document your assumptions explicitly with an assertion.
src/analyzer/analyzersilence.cpp
Outdated
return false; | ||
} | ||
|
||
void AnalyzerSilence::process(const CSAMPLE *pIn, const int iLen) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document your assumptions explicitly:
DEBUG_ASSERT((iLen % kChannelCount) == 0);
Otherwise your code might read outside the bounds of the provided array! But this is actually a flaw in the Analyzer's design which needs to be migrated to frame indexing. It should always provide complete frames, that's why the assertion is sufficient here.
@daschuer How do you distinguish crossfading from jukebox use case? |
Do we really need special begin cue point for AutoDJ? Cue points are starting to clutter on beginnings of tracks. I suggest:
Furtermore, I believe we are reaching the point when we need to introduce AutoDJ modes. Maybe something like:
|
No, AutoDJ can use one of the existing/upcoming cue points. I really think we need both, because in jukebox mode, the user want to listen a track from the first to the last tone unfortunately the first tone is unrelated to the first down beat. But I agree, that showing all cue points at the same time is somehow cluttered, since the user normally needs only one of it. Coupling it with a kind of auto DJ mode might help. This mode should select at which cue the track loaded. .. Is it finally a new or advanced track load mode? |
How about using THE CUE to set either the sound start point or the first downbeat point, depending on the selected mode? This would work almost natural from controllers and without extra cluttering controls in the GUI . |
So, we actually need four cue points for AutoDJ (all of them should be user-editable):
|
Unfortunately yes. In the major manual DJ use case, only the "Start fade in point" (alias THE CUE) point is needed. Did you consider to hide the "Begin point" in this case, or add a preference option to use one of them as "THE CUE"? We may also try to auto detect the "Start fade in point" one day. It is the first notable downbeat after the "Begin point". In the first version we may move both points to the same place. |
Well, I'm trying to find a nice and simple solution that will cover all use cases (or at least most of them 😄). Maybe we can leave everything visible for now. Furthermore, I'm concerned it might confuse users.
Maybe we can leave begin point non-quantized and quantize it as the main cue point. However, quantization will apply if user moves the begin point while quantization is enabled. |
I don't think it's clear what problem this PR is trying to solve. Could you please make that clearer in the PR description? Ideally with some "user stories" or use cases? I think we're a bit into the weeds here discussing AutoDJ modes. I'd also like us to keep in mind that Mixxx is not trying to be iTunes, so "jukebox" mode doesn't really make sense for to support. AutoDJ is a stand-in for the real DJ when they need to take a break, and I'm against scope creeping it into the music-player part of Mixxx. @daschuer -- I don't think the main cue point should play a role in AutoDJ. The main cue has a purpose defined by the user that will vary depending on the user's style of DJing. Also, if the main cue point is going to be automatically set, it should be set to the first detected beat, right? Not the first time non-silence frame in the track? (e.g. what would the benefit of that be over the first beat?) What's wrong with this PR automatically creating AutoDJ in/out cues, as requested in |
No, I think it should be set to the beginning of non-silence. I think quantize should be responsible for starting playback on beat, which it already does. An example use case for starting playback at the beginning of non-silence rather than the first beat could be mixing ambient tracks with different tempos and not beatmatching.
I think that is a good idea. These new markers shouldn't be exclusively for AutoDJ though. They are also useful as reminders set up when preparing tracks. But I'm not sure I'd want two autogenerated marks appearing on the waveform for every track... |
IMHO the scope of this PR is just to auto detect silence at the very start The discussion is about how to store, use and tweak this information. I think we are close to a conclusion, that these points schould be non The other open issues is how to use these points. "Load cue": currently we have a preference option to load a track either at "First down beat": this is a highly desired feature for me, but cannot be "End cue" this has only a value for Mixxx auto features. It can be used for "Jukebox mode": The AutoDj currently already support the jukebox mode using "Edit start point": normally this is done by The cue buttons. The "Declutter the view" a disco user probably does not care about the start |
@daschuer Thank you for summarizing the discussion.
I don't find this very intuitive. I would prefer to introduce 'source' field to cue and make AnalyzerSilence modify only those cues that have not been modified by user. However, in order to enable user to cancel his edits are make Mixxx detect these cues again, we need to make sure these cue points can be deleted.
Since I don't know how to solve this without introducing new modes or preference options, how about if we just continue using the main cue point as AutoDJ start? This way we are not only solving issues "edit start point" and "declutter the view", but we are also preserving backward compatibility for users who are currently using the main cue point for AutoDJ start. Besides, I don't think we have many users who are using same tracks for AutoDJ and manual Djing. (Am I wrong? I suppose these users are currently using Hotcues for manual Djing.) What is the conclusion with AutoDJ cues and modes? Do we really need separate cue points for the two AutoDJ use cases/modes? |
I don't find this very intuitive. I would prefer to introduce 'source'
field to cue and make AnalyzerSilence modify only those cues that have not
been modified by user. However, in order to enable user to cancel his edits
are make Mixxx detect these cues again, we need to make sure these cue
points can be deleted.
This works for me as well. We have already the (poor) cue editor. This
schould suite to delete the new cue points.
Since I don't know how to solve this without introducing new modes or
preference options, how about if we just continue using the main cue point
as AutoDJ start? Furthermore, this way we are also preserving backward
compatibility for users who are currently using the main cue point for
AutoDJ start.
We could keep the Auto DJ unchanged to continue in this PR. But in my use
case, I would use the two kind of Cue points in different situations. For
example the jukebox AutoDj mode works well for dinner background music,
while I use The Cue point for beatmatching. I am not concerned about
backward compatibility, because, we will not remove an AutoDj mode and so
the old behavior remains.
Besides, I don't think we have many users who are using same tracks for
AutoDJ and manual Djing. (Am I wrong? I suppose these users are currently
probably using Hotcues for manual Djing.)
That might be right, but we have an issue the other way round. The analyzer
will place the start point on any track.
What is the conclusion with AutoDJ cues and modes? Do we really need
separate cue points for the two AutoDJ use cases/mode.
How would the solution look like if we don't? Will the analyser move THE
Cue, instead of a new dedicated point?
I am afraid this will prevent us from a reliable an predictable solution.
|
A bug: |
Although this is a Waveform feature, I guess the option should rather be in Decks because it's there where we have the overview and the respective buttons. |
@ronso0 We already have an option in all skins that allows users to hide Intro/Outro cues on waveforms and overviews as well as buttons for setting/clearing these cues. However, these cues will still be automatically placed on all tracks. Are you looking for option to disable automatic placement of these cues?
Thank you for reporting. I'll investigate this. |
...ahemm, jup..never mind! I completely missed that as I didn't expect it there...although I maybe even suggested it myself
Nope, that's fine. An option would unnecessarily increase the complexity. I rarely use AutoDJ and if I do it's mostly personal my private jukebox, therefore my question: |
Yes, that makes sense. Without this, users will be confused why AutoDJ is behaving differently. |
not 100% reproducible though |
The idea was to quantize start markers to previous beat and end markers to next beat, so that the range would be longer. I am not sure how intuitive this is. It is easy to change this, if we decide to do so. |
I wasn't aware we can play beyond the track end. I recall there was a bug that we can create loops that go beyond the end but they'd fail to play.. |
Now Auto DJ always loads ad intro start, right? This works perfectly fot the Juke Box mode, but fails for all tracks which are already CUEed on the first beat for the DJ use case. Sorry I have not followed the discussion, but I think it currently breaks the users use case. |
I think it is up to the user to set the intro start point wherever they set the cue point previously. I am reluctant to add an option to revert to the old behavior. If this turns out to be a major issue for anyone, perhaps we could instruct them how to run a SQL query to set intro start points where they have the cue point for their whole library. But that should only be a one time issue when upgrading to 2.3, so I'm reluctant to add something to the GUI just for this. However, I have realized it would be helpful for me to load tracks at the intro start point regardless of whether AutoDJ is in use. The durations shown on the overview waveforms calculate the length of the sections. Together with the automatic silence detection, this can be misleading if the track is loaded at the start point. For example, I have a track where the automatically placed intro start point was at about 0:01. The intro end point was at 0:24. The overview showed 0:23, as it should, but the track was loaded at 0:00. So if I pressed play thinking I would have 23 seconds to mix in the track, that could throw off the mix because the intro would actually end 24 seconds later. If the track was loaded at the intro start point, that would not be a problem. |
We have clearly identified two use cases for Auto DJ. One is playing like a juke box with a small or no gap. The other is to let the tracks overlap for betmaching. Currently only one is supported, before, we have supported both. This is a regression, which is required to fix. The other issue that need to be fixed is the migration part. All DJs like me used the load to cue option have cued the tracks to the first beat. Or their desired load point. We may argue that this was a workaround, but that is the current state. We cannot ignore. The new feature is very nice and it frees up the CUE for temporary things, but we must not ignore the legacy. So let's find a solution instead of argue against it. |
A fix that allows users to switch back to the old behavior is here: #2103. |
while playing with #2205 I expected that holding in/outro cues should jump to the respective position AND keep playing as long as the cue button is hold down |
I noticed the new intro/outro cue Controls had not been documented, so I added them to the wiki and mentioned them on https://mixxx.org/wiki/doku.php/updating_controller_mappings |
We should also add information about intro/outtro markers to the mixxx manual, if that hasn't been done already. |
Yes, certainly. But let's wait until we finish #2103 and reach a consensus on how AutoDJ should use them first. |
This was introduced in PR mixxxdj#1242 but is no longer used.
This was introduced in PR mixxxdj#1242 but is no longer used.
This PR introduces simple threshold-based silence detection
https://bugs.launchpad.net/mixxx/+bug/673515
https://bugs.launchpad.net/mixxx/+bug/1106813
Controls added: https://manual.mixxx.org/2.3/en/chapters/advanced_topics.html#control-[ChannelN]-intro_end_activate
Work items:
Ideas: