Adding RadioLanewayCrossfade transition #13750
Conversation
Adding RadioLanewayCrossfade transition. See mixxxdj#13716
(Wow this is fussy!).
More style issues.
Yet more code style issues (Space at end of line.)
More Code tidy Missing case in cueinfo.cpp
Code style
Spelling
avoiding a DEBUG_ASSERT(!"Unknown Loop Type");
fix up tooltip
src/analyzer/analyzersilence.cpp
Outdated
| 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; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
this is a copy of the above for very little reason. Consider deduplicating by simply taking the Threshold as a parameter.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
std::find_if only requires a forward iterator.
There was a problem hiding this comment.
If you like I'll refactor all of those into one, including the db60.
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I currently don't have the time to give this yet another proper review. Please consult with @daschuer on the next steps.
| "volume last falls below -12Db or at the spin box setting which ever\n" | ||
| "is lower, and potentially starts the next earlier if it starts below\n" | ||
| "-27Db."); |
There was a problem hiding this comment.
its very easy to change the threshold in the analyzer but forget to update it here. Do you think its valuable to have the thresholds documented here or can we just omit them?
There was a problem hiding this comment.
I nearly made it a more general statement (e.g., 'a certain level'), but I figured someone would want to know the exact values. It may well be worth making it more general for the reason you suggested.
There was a problem hiding this comment.
We had also the demand to configure the threshold.
But thats is IMHO only a workaround for the replay gain/loudness issue. A perfect solution would be to use a certain Loudness level after applying replay gain. Did you consider to implement that?
I suggest you do. This transition is groundbreaking. |
and removal of commented code.
Yet more code format issues.
Yet even more code style issues.
Comment correction
Adjust lasttSampleBeforeFadeOut (and avoid a regression with lastSoundSample) to bring it into line with mixxxdj#13545 (I assume I was working off an older code base.)
|
@daschuer ping |
|
I am a bit puzzled how this iteracts with manual set intros and outros. While this works reasonable good by default for radio mixes, it will probably fail for tracks with club intros and outros or track with ambience noise at the beginning or end. This is IMHO a perfect solution for the default case where a track has only an IntroStrat and an OutroEnd Cue set. Did you consider to priorize the manual set intro and outro, to give the user the power to adjust exceptional tracks? |
src/analyzer/analyzersilence.cpp
Outdated
| // 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
src/analyzer/analyzersilence.cpp
Outdated
| return ret; | ||
| } | ||
|
|
||
| // Find the number of first sound sample where the sound is above kFadeInThreshold (-27db) |
There was a problem hiding this comment.
"Number" is ambiguous here better "index"?
| "volume last falls below -12Db or at the spin box setting which ever\n" | ||
| "is lower, and potentially starts the next earlier if it starts below\n" | ||
| "-27Db."); |
There was a problem hiding this comment.
We had also the demand to configure the threshold.
But thats is IMHO only a workaround for the replay gain/loudness issue. A perfect solution would be to use a certain Loudness level after applying replay gain. Did you consider to implement that?
src/track/cueinfo.h
Outdated
| FadeIn = 9, // First time sound reaches a certain level | ||
| // (cf analyzersilence.cpp); not shown to user | ||
| FadeOut = 10 // Last time sound reaches a certain level | ||
| // (cf analyzersilence.cpp); not shown to user |
There was a problem hiding this comment.
Please add also a comment about the level itself.
Is it the last time of full volume, or just an alternative silence threshold?
There was a problem hiding this comment.
A perfect solution would be to use a certain Loudness level after applying replay gain. Did you consider to implement that?
I did, but I was struggling to see how the track could have silence markers and not be normalised. I haven't quite understood the order of operation, and how often the analysis section is called. And importantly if normalisation is before or after silence detection. It seems to be resolved several times as far as I can tell... (mostly from the spurious results I was getting while a track was being loaded). All of which says this probably needs to be considered well before implementation. Right now, I'm not sure I know enough. But I am happy to take a look. Should this be a discussion in chat, since this is a separate topic?
Please add also a comment about the level itself.
Is it the last time of full volume, or just an alternative silence threshold?
It is the last or first time the sound reaches a certain level. Since full volume is subjective, I thought this was more precise. It is like an alternative silence threshold but is different because there are two different threshold values.
(I guess this isn't clear - so perhaps I should add something like: So to be clear, this is the last sample in the track where the volume was above the FadeOut level, and the first sample in the track where the sound volume was above the FadeIn level.)
There was a problem hiding this comment.
FadeIn = 9, // Range from the start of the track to the first sample in
// the track where the sound volume was above the
// kFadeInThreshold (cf analyzersilence.cpp);
// not shown to user
FadeOut = 10 // Range from the last sample in the track where
// the sound volume was above the kFadeOutThreshold
// to the end of the track (cf analyzersilence.cpp);
// not shown to user
src/analyzer/analyzersilence.cpp
Outdated
| #define N_25DB_FADE_THRESHOLD 0.0562f | ||
| #define N_27DB_FADE_THRESHOLD 0.0447f | ||
| #define N_30DB_FADE_THRESHOLD 0.0316f | ||
| #define N_40DB_FADE_THRESHOLD 0.01f |
There was a problem hiding this comment.
We need to consider how to handle future changes.
Since the thresholds are used fully automated without any user feedback a layer change if the values will mess up the database with old and new values.
Therfore a kind of "versioning" is require. That on case of a value change the old one found in database is no longer used.
We can easily add new cue type for this. But we need to prepare the code in a way that this is clear for future reader.
There was a problem hiding this comment.
(My intent with those comments was to have a placeholder for the values while I played around. Assume those are deleted by the time someone is making a change here. They really are not necessary now)
But I understand the main point. It is the same problem as the -60db setting.
There would be a cost if we do a lot of processing every time a track is loaded. Should we have a Cue value that is simply a version flag associated with every track, which could be checked by a future version to fix any issues? No cue = 4.x or earlier?
For the user adjustment, I suggest that the next step would be to add some UI elements to make this all visible and allow the user adjust. We also discussed this. Happy to work on it as a next step (when I get time). But I think this might be a future version change, based on any perceived need.
There was a problem hiding this comment.
As I sent that I realised 'Cue value' may not make sense. But at least have a flag with the track data that shows a version number. Or, alternatively have a cueinfo.version setting for each cue (though that smells of data duplication). After a very quick look at the code base - noting exceptions for things like "Serato's tag data", I realise even this might be problematic. So this too is also, worthy of a chat discussion.
There was a problem hiding this comment.
I don't know what changes that have been requested remain incomplete at this stage.
Fix some comments
Line endings yet AGAIN!
Comment changes
RadioLanewayCrossfade transition.
See AutoDj: New transition mode 'Radio Laneway' #13716.
This transition mode is designed for social functions, especially playing popular music, and is intended to sound like a commercial radio station. For this purpose, it is vastly superior to existing transmission modes, and for cold endings especially, it consistently performs better than most people I suspect (well, me at least!).
This adds two new Cues, FadeIn and FadeOut. (cf analyzersilence.cpp pN60dBSound and kSilenceThreshold.) The FadeIn range is the time from the Main cue point (or first sound if the MainCue is not set) to the first time a track reaches kFadeInThreshold which is set to -27 dBV (or 0.0447f). The distance between these is the fadeInLength. The FadeOut range is from the last time a track was at kFadeOutThreshold which is set to -12 dBV (or 0.2511f), to the last sound. The distance between these is the fadeOutLength. The values are selected based on "sounding good" while reducing the chance of pathological times for fade-ins or outs. Note that tracks always start a full volume (cf. TransitionMode::FixedStartCenterSkipSilence #13628).
This transition mode does the following:
If a playing (from) track has reached the FadeOut cue start, then a crossfade starting center is initiated, with the transmission time taking the fadeOutLength, or value from the spin box (fixed value), whichever is smaller. This fits most cases and provides a smooth crossfade on tracks with a fadeout, and excellent timing when a track has a sharp or cold ending.
If the next (to) track has a fadeInLength greater than 0.25 seconds then the next (to) track starts a fadeInLength value (playNextPos) earlier than the current (from) deck fadeOutStart. This covers the case where we have a slow fade-in of a song, for example, when Boston's "More Than A Feeling", comes next after a song with a cold start, removing the perception of a gap, while preserving the cold ending. (In this example it is about 3 seconds earlier, though the new track is not perceptible during a cold ending). Note that the crossfader holds in the center while the next (to) track starts, and starts moving the fader once fadeOutStart is reached. If the fadeOutLength + playNextPos difference is greater than the spin box, then revert to item 1 above using (fixed value) calculated from fadeOutLength.
If the next (to) track has a duration of less than 15 seconds then this track is likely a jingle or sample. In this case, the fadeInLength is 0 and the fadeOutLength is 1. This way we have a fast neat transition while maximizing the time for the next track to load.