Intro/Outro cues and silence detection #1242
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| #include "analyzer/analyzersilence.h" | ||
|
|
||
| const int kChannelCount = 2; | ||
|
There was a problem hiding this comment. please use mixxx::AudioSource::kChannelCountStereo |
||
| const float kSilenceThreshold = -60.0f; | ||
|
There was a problem hiding this comment. please name it kSilenceThresholdDb to document the unit |
||
|
|
||
| AnalyzerSilence::AnalyzerSilence(UserSettingsPointer pConfig) | ||
| : m_pConfig(pConfig), | ||
| m_fThreshold(db2ratio(kSilenceThreshold)), | ||
| m_iSampleRate(0), | ||
|
There was a problem hiding this comment. unused, please remove m_iSampleRate and m_iTotalSamples |
||
| m_iTotalSamples(0), | ||
| m_iFramesProcessed(0), | ||
| m_bPrevSilence(true), | ||
| m_iSignalBegin(-1), | ||
| m_iSignalEnd(-1) { | ||
| } | ||
|
|
||
| AnalyzerSilence::~AnalyzerSilence() { | ||
| } | ||
|
|
||
| bool AnalyzerSilence::initialize(TrackPointer tio, int sampleRate, int totalSamples) { | ||
| Q_UNUSED(tio); | ||
|
There was a problem hiding this comment. 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. we may consider her to move the cue points if they are finally independent from auto DJ an they are not user editable |
||
|
|
||
| m_iSampleRate = sampleRate; | ||
| m_iTotalSamples = totalSamples; | ||
| m_iFramesProcessed = 0; | ||
| m_bPrevSilence = true; | ||
| m_iSignalBegin = 0; | ||
|
There was a problem hiding this comment. I would initialize with -1 to distinguish between not found and 0. |
||
| m_iSignalEnd = 0; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool AnalyzerSilence::isDisabledOrLoadStoredSuccess(TrackPointer tio) const { | ||
| Q_UNUSED(tio); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| void AnalyzerSilence::process(const CSAMPLE *pIn, const int iLen) { | ||
|
There was a problem hiding this comment. Please document your assumptions explicitly: 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. There was a problem hiding this comment. Regarding coding guidelines: |
||
| for (int i = 0; i < iLen; i += kChannelCount) { | ||
daschuer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| bool bSilence = math_max(fabs(pIn[i]), fabs(pIn[i + 1])) < m_fThreshold; | ||
|
There was a problem hiding this comment. 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. |
||
|
|
||
| if (m_bPrevSilence && !bSilence) { | ||
| if (m_iSignalBegin <= 0) { | ||
| m_iSignalBegin = m_iFramesProcessed + i / kChannelCount; | ||
| } | ||
| } else if (!m_bPrevSilence && bSilence) { | ||
| m_iSignalEnd = m_iFramesProcessed + i / kChannelCount; | ||
| } | ||
|
|
||
| m_bPrevSilence = bSilence; | ||
| } | ||
|
|
||
| m_iFramesProcessed += iLen / kChannelCount; | ||
| } | ||
|
|
||
| void AnalyzerSilence::cleanup(TrackPointer tio) { | ||
| Q_UNUSED(tio); | ||
| } | ||
|
|
||
| void AnalyzerSilence::finalize(TrackPointer tio) { | ||
| // If track didn't end with silence, place signal end marker | ||
|
There was a problem hiding this comment. handle m_iSignalBegin < 0 and m_iSignalEnd < 0 explicitly here ? |
||
| // on the end of the track. | ||
| if (!m_bPrevSilence) { | ||
| m_iSignalEnd = m_iFramesProcessed; | ||
| } | ||
|
|
||
| bool bBeginPointFoundAndSet = false; | ||
| bool bEndPointFoundAndSet = false; | ||
| QList<CuePointer> cues = tio->getCuePoints(); | ||
|
There was a problem hiding this comment. 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 |
||
| foreach (CuePointer pCue, cues) { | ||
| if (pCue->getType() == Cue::BEGIN) { | ||
| pCue->setHotCue(0); | ||
| pCue->setLabel("BEGIN"); | ||
| pCue->setLength(0); | ||
| pCue->setPosition(kChannelCount * m_iSignalBegin); | ||
| bBeginPointFoundAndSet = true; | ||
| } else if (pCue->getType() == Cue::END) { | ||
| pCue->setHotCue(1); | ||
| pCue->setLabel("END"); | ||
| pCue->setLength(0); | ||
| pCue->setPosition(kChannelCount * m_iSignalEnd); | ||
| bEndPointFoundAndSet = true; | ||
| } | ||
| } | ||
|
|
||
| if (!bBeginPointFoundAndSet) { | ||
| CuePointer pCue = tio->createAndAddCue(); | ||
| pCue->setType(Cue::BEGIN); | ||
| pCue->setHotCue(0); | ||
|
There was a problem hiding this comment. please don't set a hotcue since it might conflict with other hotcues |
||
| pCue->setLabel("BEGIN"); | ||
|
There was a problem hiding this comment. label is meant for user tagging / notes to themselves, so I wouldn't set the label |
||
| pCue->setLength(0); | ||
| pCue->setPosition(kChannelCount * m_iSignalBegin); | ||
| } | ||
|
|
||
| if (!bEndPointFoundAndSet) { | ||
| CuePointer pCue = tio->createAndAddCue(); | ||
| pCue->setType(Cue::END); | ||
| pCue->setHotCue(1); | ||
|
There was a problem hiding this comment. please don't set a hotcue since it might conflict with other hotcues |
||
| pCue->setLabel("END"); | ||
|
There was a problem hiding this comment. label is meant for user tagging / notes to themselves, so I wouldn't set the label |
||
| pCue->setLength(0); | ||
| pCue->setPosition(kChannelCount * m_iSignalEnd); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| #ifndef ANALYZER_ANALYZERSILENCE_H | ||
| #define ANALYZER_ANALYZERSILENCE_H | ||
|
|
||
| #include "analyzer/analyzer.h" | ||
| #include "preferences/usersettings.h" | ||
|
|
||
| class AnalyzerSilence : public Analyzer { | ||
| public: | ||
| AnalyzerSilence(UserSettingsPointer pConfig); | ||
| virtual ~AnalyzerSilence(); | ||
|
There was a problem hiding this comment. please mark the destructor override too |
||
|
|
||
| bool initialize(TrackPointer tio, int sampleRate, int totalSamples) override; | ||
| bool isDisabledOrLoadStoredSuccess(TrackPointer tio) const override; | ||
| void process(const CSAMPLE *pIn, const int iLen) override; | ||
| void finalize(TrackPointer tio) override; | ||
| void cleanup(TrackPointer tio) override; | ||
|
|
||
| private: | ||
| UserSettingsPointer m_pConfig; | ||
| float m_fThreshold; | ||
| int m_iSampleRate; | ||
| int m_iTotalSamples; | ||
| int m_iFramesProcessed; | ||
| bool m_bPrevSilence; | ||
| int m_iSignalBegin; | ||
| int m_iSignalEnd; | ||
| }; | ||
|
|
||
| #endif // ANALYZER_ANALYZERSILENCE_H | ||
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.