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

Adding RadioLanewayCrossfade transition #13750

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 112 additions & 6 deletions src/analyzer/analyzersilence.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,36 @@ namespace {
// verify that the track samples have not changed since the last analysis
constexpr CSAMPLE kSilenceThreshold = 0.001f; // -60 dB
// TODO: Change the above line to:
//constexpr CSAMPLE kSilenceThreshold = db2ratio(-60.0f);
// constexpr CSAMPLE kSilenceThreshold = db2ratio(-60.0f);

// These are in dBV expressed as Volts RMS (which seems, sensibly,
// the way Mixxx works).
// Don't change these to const as they are used only to feed the
// fade thresholds which are consts below themselves while we work
// out which one is best, and you'll just be adding to exe size.
Copy link
Member

Choose a reason for hiding this comment

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

Unused constexpr are not contribute to the EXE size. There might be only a "unused" warning.

Now we have unused macros instead. I would prefer to use constexpr and comment out the unused versions to mute warnings. This would be more expensive than the comment.

At least the comment needs to be corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole thing is a comment in case the chosen values are not the best. I should just delete the lot. I believe I have good values here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a bit puzzled how this iteracts with manual set intros and outros.

You asked about this before. It does not interact or use manual set intros and outros cues at all. It runs as discussed in AutoDj: New transition mode 'Radio Laneway' #13716

I've yet to find a track where it isn't 'good enough', and in some cases does consistently better than me (where I might miss 1 in 10 cold endings), so I'm not sure I agree with you about not working with ambient noise at the beginning or end, but since I can't claim any great familiarity with club music I will leave that for you to decide.

Why not try it and see?

#define N_10DB_FADEOUT_THRESHOLD 0.3162f
#define N_12DB_FADEOUT_THRESHOLD 0.2511f
#define N_15DB_FADEOUT_THRESHOLD 0.1778f
#define N_18DB_FADEOUT_THRESHOLD 0.1259f
#define N_20DB_FADEOUT_THRESHOLD 0.1f
#define N_24DB_FADEOUT_THRESHOLD 0.0631f
#define N_25DB_FADEOUT_THRESHOLD 0.0562f
#define N_27DB_FADEOUT_THRESHOLD 0.0447f
#define N_30DB_FADEOUT_THRESHOLD 0.0316f
#define N_40DB_FADEOUT_THRESHOLD 0.01f
Swiftb0y marked this conversation as resolved.
Show resolved Hide resolved

constexpr CSAMPLE kFadeInThreshold = N_27DB_FADEOUT_THRESHOLD;
constexpr CSAMPLE kFadeOutThreshold = N_12DB_FADEOUT_THRESHOLD;

bool shouldAnalyze(TrackPointer pTrack) {
CuePointer pIntroCue = pTrack->findCueByType(mixxx::CueType::Intro);
CuePointer pOutroCue = pTrack->findCueByType(mixxx::CueType::Outro);
CuePointer pN60dBSound = pTrack->findCueByType(mixxx::CueType::N60dBSound);
CuePointer pFadeIn = pTrack->findCueByType(mixxx::CueType::FadeIn);
CuePointer pFadeOut = pTrack->findCueByType(mixxx::CueType::FadeOut);

if (!pIntroCue || !pOutroCue || !pN60dBSound || pN60dBSound->getLengthFrames() <= 0) {
if (!pFadeIn || !pFadeOut || !pIntroCue || !pOutroCue || !pN60dBSound ||
pN60dBSound->getLengthFrames() <= 0) {
return true;
}
return false;
Expand All @@ -30,13 +52,29 @@ Iterator first_sound(Iterator begin, Iterator end) {
});
}

template<typename ForwardIterator>
ForwardIterator last_fade_in_sound(ForwardIterator begin, ForwardIterator end) {
return std::find_if(begin, end, [](const auto elem) {
return fabs(elem) >= kFadeInThreshold;
});
}

template<typename Iterator>
Iterator first_fade_out_sound(Iterator begin, Iterator end) {
return std::find_if(begin, end, [](const auto elem) {
return fabs(elem) >= kFadeOutThreshold;
});
}

Copy link
Member

Choose a reason for hiding this comment

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

this is a copy of the above for very little reason. Consider deduplicating by simply taking the Threshold as a parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second one goes backward, while the first is strictly forward. I spent a long time playing around with these, trying to determine why I was getting weird results (and so I was keen to keep them separate). It turns out AutoDj processing can take place before a track is fully analysed, so I am less invested in that now.

Copy link
Member

Choose a reason for hiding this comment

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

The functions are identical. In this context ForwardIterator and Iterator are just names that resolve to the underlying iterator implementation when called. So there is no difference in terms of what function can be called when. A std::reverse_iterator is a "bidirectional iterator" and thus also a "forward iterator".

Copy link
Member

Choose a reason for hiding this comment

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

std::find_if only requires a forward iterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you like I'll refactor all of those into one, including the db60.

Copy link
Member

Choose a reason for hiding this comment

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

Prolog isn't really a functional language though, its declaritive. If you're coming from the performance angle, I can recommend you to learn rust. Its amazing how nice code can look if its functional-ish while still being blazingly fast. The bulk of code is primarily written because its old and there was nothing better back then thats still performant. That is not the case anymore today. C++ is also a multi-paradigmatic language which includes all the necessary tools to clarify as functional as well. Anyways, it's not very productive to argue about this here. I was just trying to encourage you to broaden your horizon a little bit ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are technically correct. As I remember it at the time Prolog was considered to be like a functional language... and the way it works has similarities at least. Haskal was a thing. There was an elective for Lambda Calculus and Haskal. Looked too much like math (and formal logic which I did in philosophy) to attract my interest. I did Networking and Databases. This was the late 80's early 90's as a mature-age student.

As for Rust, this is one I have already downloaded and considered for my microcontroller projects, mostly in the wake of Counterstrike. I've backed off a little since then because it doesn't have the lib support yet (for that area). (And let's face it it is a tad faddish at the moment - yes I am saying that like it is a bad thing - even if Rust will stick.)

As for my horizons, you do realise that I am in my 60's and that I have been doing this for longer (probably) than you have been around, having first programmed on a Z-80 back in the very early 80's, right? When I started PC's weren't even a thing. The Commodore 64 was a year or two away. And, I had to build my first computer. Back then you could understand everything that was happening inside the computer. I nearly built the EDUC-8 back in 1975 when I was still in high school, but could not afford it, and I also doubled the utility of it (I was right - though it would have been a great learning exercise. And it was complex (for the time). Switches and lights... no KB. Back then I was focused on sound and mixers. And I was more results-focused. I wanted to mix and sound like the Radio (strangely that hasn't changed!) In the next seven years, computers went from unobtainable to commonplace - the Commodore 64 I purchased in 1983 was my favourite.)

There is a time in your life when you are young when the "latest and greatest" (= fashionable) always seems to be the best. And, old fogeys don't seem to adjust well. However, the 'latest and greatest' doesn't always work out, even if the latter statement is still probably true. 'Amiga' anyone?

You clearly know your stuff, much more than I did at your age. (I am making assumptions about your age - please correct me if I am wrong). Since you have taken the liberty of recommending things to me, how about you take a look at Steve McConnell's "Code Complete"?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your recommendation. I've studied some of Robert C. Martins books so far but "Code Complete" also seems interesting. I will try to get a hold of a copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've read some Uncle Bob stuff. I should check out his book(s?). What I liked about Steve's work was it was so concrete (practical), and backed up by evidence.

So to the matter at hand. Is there anything else that we need to do to get this merged?

Copy link
Member

Choose a reason for hiding this comment

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

I currently don't have the time to give this yet another proper review. Please consult with @daschuer on the next steps.

} // anonymous namespace

AnalyzerSilence::AnalyzerSilence(UserSettingsPointer pConfig)
: m_pConfig(pConfig),
m_framesProcessed(0),
m_signalStart(-1),
m_signalEnd(-1) {
m_signalEnd(-1),
m_fadeThresholdFadeInEnd(-1),
m_fadeThresholdFadeOutStart(-1) {
}

bool AnalyzerSilence::initialize(const AnalyzerTrack& track,
Expand All @@ -53,6 +91,8 @@ bool AnalyzerSilence::initialize(const AnalyzerTrack& track,
m_framesProcessed = 0;
m_signalStart = -1;
m_signalEnd = -1;
m_fadeThresholdFadeInEnd = -1;
m_fadeThresholdFadeOutStart = -1;
m_channelCount = channelCount;

return true;
Expand All @@ -70,6 +110,28 @@ SINT AnalyzerSilence::findLastSoundInChunk(std::span<const CSAMPLE> samples) {
return ret;
}

// static
SINT AnalyzerSilence::findFirstFadeOutChunk(std::span<const CSAMPLE> samples) {
// -1 is required, because the distance from the fist sample index (0) to crend() is 1,
SINT ret = std::distance(
first_fade_out_sound(samples.rbegin(), samples.rend()),
samples.rend()) -
1;
if (ret == -1) {
ret = samples.size();
}
return ret;
}

// static
SINT AnalyzerSilence::findLastFadeInChunk(std::span<const CSAMPLE> samples) {
SINT ret = std::distance(samples.begin(), last_fade_in_sound(samples.begin(), samples.end()));
// if (ret == samples.size()) {
// ret = 0;
// }
return ret;
}

// static
bool AnalyzerSilence::verifyFirstSound(
std::span<const CSAMPLE> samples,
Expand All @@ -93,10 +155,24 @@ bool AnalyzerSilence::processSamples(const CSAMPLE* pIn, SINT count) {
m_signalStart = m_framesProcessed + firstSoundSample / m_channelCount;
}
}
if (m_signalStart >= 0) {

if (m_fadeThresholdFadeInEnd < 0) {
const SINT lastSampleOfFadeIn = findLastFadeInChunk(samples);
if (lastSampleOfFadeIn < count) {
m_fadeThresholdFadeInEnd = m_framesProcessed + (lastSampleOfFadeIn / m_channelCount);
}
}
if (m_fadeThresholdFadeInEnd >= 0) {
const SINT lasttSampleBeforeFadeOut = findFirstFadeOutChunk(samples);
if (lasttSampleBeforeFadeOut < (count - 1)) {
m_fadeThresholdFadeOutStart = m_framesProcessed +
(lasttSampleBeforeFadeOut / m_channelCount) + 1;
}
}
if (m_fadeThresholdFadeOutStart >= 0) {
const SINT lastSoundSample = findLastSoundInChunk(samples);
if (lastSoundSample >= 0) {
m_signalEnd = m_framesProcessed + lastSoundSample / m_channelCount + 1;
if (lastSoundSample < (count - 1)) { // not only sound or silence
m_signalEnd = m_framesProcessed + (lastSoundSample / m_channelCount) + 1;
}
}

Expand Down Expand Up @@ -136,6 +212,36 @@ void AnalyzerSilence::storeResults(TrackPointer pTrack) {

setupMainAndIntroCue(pTrack.get(), firstSoundPosition, m_pConfig.data());
setupOutroCue(pTrack.get(), lastSoundPosition);

if (m_fadeThresholdFadeInEnd < 0) {
m_fadeThresholdFadeInEnd = 0;
}
if (m_fadeThresholdFadeOutStart < 0) {
m_fadeThresholdFadeOutStart = m_framesProcessed;
}
const auto fadeInEndPosition = mixxx::audio::FramePos(m_fadeThresholdFadeInEnd);
CuePointer pFadeIn = pTrack->findCueByType(mixxx::CueType::FadeIn);
if (pFadeIn == nullptr) {
pFadeIn = pTrack->createAndAddCue(
mixxx::CueType::FadeIn,
Cue::kNoHotCue,
firstSoundPosition,
fadeInEndPosition);
} else {
pFadeIn->setStartAndEndPosition(firstSoundPosition, fadeInEndPosition);
}

const auto fadeOutStartPosition = mixxx::audio::FramePos(m_fadeThresholdFadeOutStart);
CuePointer pFadeOut = pTrack->findCueByType(mixxx::CueType::FadeOut);
if (pFadeOut == nullptr) {
pFadeOut = pTrack->createAndAddCue(
mixxx::CueType::FadeOut,
Cue::kNoHotCue,
fadeOutStartPosition,
lastSoundPosition);
} else {
pFadeOut->setStartAndEndPosition(fadeOutStartPosition, lastSoundPosition);
}
}

// static
Expand Down
12 changes: 12 additions & 0 deletions src/analyzer/analyzersilence.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@ class AnalyzerSilence : public Analyzer {
UserSettings* pConfig);
static void setupOutroCue(Track* pTrack, mixxx::audio::FramePos lastSoundPosition);

/// returns the index of the first sample in the buffer that is above the
/// fade out threshold (e.g. -20 dB) or samples.size() if no sample is found
/// TODO CHeck if 'samples.size()' is actually meaningful in this context.
static SINT findLastFadeInChunk(std::span<const CSAMPLE> samples);

/// returns the index of the last sample in the buffer that is above that is
/// above the fade out threshold (e.g. -20 dB) or samples.size() if no
/// sample is found
static SINT findFirstFadeOutChunk(std::span<const CSAMPLE> samples);

/// returns the index of the first sample in the buffer that is above -60 dB
/// or samples.size() if no sample is found
static SINT findFirstSoundInChunk(std::span<const CSAMPLE> samples);
Expand All @@ -56,4 +66,6 @@ class AnalyzerSilence : public Analyzer {
SINT m_framesProcessed;
SINT m_signalStart;
SINT m_signalEnd;
SINT m_fadeThresholdFadeInEnd;
SINT m_fadeThresholdFadeOutStart;
};
Loading
Loading