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

Auto dj cross fader center #13628

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

davidlmorris
Copy link

@davidlmorris davidlmorris commented Sep 4, 2024

And attempt to fix my understanding of git, by creating a new branch to fix code format 'issues'.

Option to set the Crossfader to the center as AutoDj starts a crossfade so the track starts at full volume.
@daschuer
Copy link
Member

daschuer commented Sep 5, 2024

pre-commit is complaining.
I assume you have installed it locally.

You may test it by adding a space at the end of the affected lines that are shown following "Details" above.

If everything works you need to commit twice.
git commit -a -m"Fix code style"
The first commit is rejected by pre-commit on purpose and changes are automatically applied in the second commit.

please squash this commit into the two earlier ones.
@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 6, 2024

@davidlmorris took the liberty to fix it for you. make sure you git pull before making new changes so your local branch is the same as the upstream one I changed.

@ronso0
Copy link
Member

ronso0 commented Sep 6, 2024

... and please "sync" this with #13619 (updates after reviews).
Or fix #13619.

Either way, let's decide where to continue.

@ronso0
Copy link
Member

ronso0 commented Sep 6, 2024

I wonder if it's worth adding this option to the controls in the GUI.
(could be tricky to find an icon for this option)

I mean: is this something you'd want to toggle quickly while mixxxing? without having to go to the preferences

@davidlmorris
Copy link
Author

I wonder if it's worth adding this option to the controls in the GUI.

I would say no. It is what I originally set out to do, but basically, this setting could/would apply to all AutoDj options, so we would end up with 8 rather than 4, which would make that GUI setting too busy and too confusing.

I have an idea though for my 'next change' which would require a much bigger effort to dig through and understand the code: Add more 'unseen' cue points, being the first and last (and here I'm just guessing - this needs to be thought out) say: -6db (from ref), - 10db, -20db and maybe others. Then have an AutoDJ option that works like the 'skip silence' option, except that it picks up the timing from, for example, the -6db point (selectable in options), called 'Selectable Fade' or 'Radio Fade' so that you can get much better crossfades (or radio fades) on songs where you don't have to add start and end clue markers. This way a fade-out would be merged better with a cold start. So a cold (abrupt) end would transition perfectly (and tightly) into a cold opening.

Naturally which cue marker you used would be selectable in AuduDJ preferences. (I am assuming that adding cue marks would not be a huge pain or overhead- given they would need to be checked and loaded with each song - and I presume updated in the DB ... lots here I don't know about that I (or someone) would need to check).

@davidlmorris
Copy link
Author

took the liberty to fix it for you. make sure you git pull before making new changes so your local branch is the same as the upstream one I changed.

@Swiftb0y Thankyou! Brilliant! (I have done the pull. But until I am confident I won't break things further, will abstain from any other git shinanigans.)

@ronso0
Copy link
Member

ronso0 commented Sep 8, 2024

this setting could/would apply to all AutoDj options, so we would end up with 8 rather than 4, which would make that GUI setting too busy and too confusing.

What I meant was: have this option in the Auto DJ controls row, too. (this does not require duplicating all transistion modes)
It could be off for tracks where outro and intro blend nicely, and on if there's a track that starts with a vocal that you want with full volume.

@daschuer
Copy link
Member

daschuer commented Sep 9, 2024

I am in doubt that this option suits for every transition mode. For instance if you have defined an into of a track you may not want to start it immediatly at full volume. In the other hands there are tracks that start with a significant sound like a signature vocal. that shall not be fadet in, because it is silenced at the beginning. In addition I want not fade in Track started at 0:00, because the usually have their own "intro". Sometimes I fade the previous track out to make the signature sound of the new track even more notable. So It is a case by case decision.

So we may define these rules:

  • Full Intro + Outro:
    • If intro not defined or zero start new track at full volume.
  • Fade At Outro Start:
    • If intro not defined or zero start new Track at full volume.
  • Full Track:
    • Here to conditional option may make sense "Full Track, no Fade-In"
    • Since this is an "unprepared" mode any intro dependency doe not make sense
  • Skip Silence
    • Same here. Maybe we can rethink both. Does playing a track with silence ever make sense?
    • Maybe we need a gepless mode instead that just plays the tracks like pressed on the vinyl or live recordings.

What is your use case? How we can distinguish the intro types?

@ronso0
Copy link
Member

ronso0 commented Sep 9, 2024

Thanks, that's excactly why I proposed to have the option in the skins, not only in the preferences.

  • Full Intro + Outro:
    • If intro not defined or zero start new track at full volume.
  • Fade At Outro Start:
    • If intro not defined or zero start new Track at full volume.

I don't think we should assume the track has no intro/outro just because users didn't configure it.
IIUC the center mode is most helpful for Full Track and Skip Silence. I imagine there are users who don't use intro/outro but run Auto DJ with these modes in "manual" fade, ie. trigger Fade Now when it seems appropriate.

@daschuer
Copy link
Member

daschuer commented Sep 9, 2024

Yes, I can confirm that. On the other hand I like to be able to set the behavior track by track, like we do with the intros.

My rational was that: If a track has an optionsl intro set, it is assumed to faded in. We have no other option yet.
The track is never faded after intro end.
We may reuse this assumption.

In addition I think the default should be no fade in.
This fits better to most of my tracks, because they have a natural intro, or start with lyrics right away.

@davidlmorris
Copy link
Author

From a radio perspective, there is almost NO 'use case' for when you would want to fade a track in. (Ok there is one: when you fade an instrumental in from the middle so you can time a cold ending to the top of the hour where you have time pips like BBC World Service or a syndicated news bulletin... but this isn't an AutoDj thing.) I've never worked in or seen a radio studio with a 'crossfader'. It just isn't (wasn't?) a thing. Every announcer I've seen starts the track at (or near full volume) talking over the intro, or running a sweeper, or doing nothing as required. On some public and community radio stations I have heard the occasional fade-in, but it sounds unprofessional and messy and was probably a mistake. Perhaps this is different in other parts of the world?

So from my perspective in my use case, I would have the option checked all the time. I suspect this would be true for most people using it this way. So if a song has an intro specified (and the previous has an outro or not) I would still want to start it at full volume. (In my case when I use the intro/outro I use it as an indicator for when it is safe to voice over music... which still mostly works for AutoDJ) To be clear this is not a full solution to the problem of having the ability to do a 'full radio mix' (more on that later), but at the risk of a pun, it does take us halfway there.

I have enormous respect for those that are using Mixxx (and the rest) as musical instruments, manually matching beats, and fading bits and pieces in and out to create something new. That is a skill set I just don't have. And of course, the requirements there are very different. In those cases, I can't imagine AutoDJ is used at all, and a cross-fader might be used a lot. But perhaps Electro and House music lends itself to a fade-in and so for those having that as an option would be useful. Not my area of expertise (at all!).

I have no objection to it being part of the UI bar for AutoDJ, but right now, my suggestion would be to include it as the option is proposed. And then as a separate pull request if someone needs it have it as an option of the AutoDJ UI 'bar'.

I would be keen to have the feature included as soon as possible, rather than stalled out trying to make it perfect. After all, there is simply no option at the moment that allows a track to start at full volume in AutoDj.

@davidlmorris
Copy link
Author

I might add, that I use the 'Skip Silence' with the cross-over time set to 5 seconds currently. This works with the center option as well as you would expect (and a whole lot better than a fade-in). Even using 'Full Track' and adding silence would work better for me since you are hearing the full track as the creator intended (like an LP or CD) without a fade-in.

@daschuer
Copy link
Member

... there is almost NO 'use case' for when you would want to fade a track in ...

I can confirm.

rather than stalled out trying to make it perfect.

We are in a good progress to make it perfect. I like rather include only your use case as a special transition than increase the complexity of the Auto DJ by making it a separate option.

So it looks like we already have a conclusion ..

Skip Silence is the same as the two intro/outro modes in case no intros and outros are defined. Do you have some defined?

My guess is if a track has defined an outro, you want to use it, right? If not, you like the 5 second fade out as described.

This is "Fade At Outro Start". Which has currently the IMHO buggy behaviour that it fades a track into lyrics or such if you have a long outro and no intro defined.

So I think this can be fixed by your mode. In case the is no intro, start with full volume at sound start.

I don't think there is any use case for an uncontrolled long intro in that case.
This is probably a minimal solution without any config option.

Does it work for you?

@davidlmorris
Copy link
Author

Skip Silence is the same as the two intro/outro modes in case no intros and outros are defined. Do you have some defined?

I do. While most work as advertised (as you stated), occasionally I marked some from the last vocal with a very extended outro, mostly so I have a mark for starting an extended talk break over the tail. Unfortunately, this sometimes makes the crossfade seem a bit messy, making the song seem like it has been cut short - which it has, so I just leave it on skip silence.

I have tested the change against all or the options, and they all seem to work. So to be clear this works for every type of AutoDj transition currently, and I would be against limiting it to any particular one.

So I would be keen to have the change (as it is) included as soon as possible.

The only reason that I would not propose making it a default, is that I don't think that changes that 'break expectations' or 'break behavior' should be introduced as a default, even if I suspect most people will end up using it that way.

@daschuer
Copy link
Member

The issue I have with this extra toggle, that this is clearly a "per track" option. You can either have club tracks with an intro you like to fade in and a track that starts with vocals that should be started with full volume.

I want a feature that allows unattended mixes as well.
Even the two intro/outro modes are not good in this regards and only a compromise, because we could not decide for one or another approach.

I am not afraid to break a feature in favor of it. We will learn if this was good or bad during the beta period and may change the implementation.

So how about doing both, add another skip silence option with only fade out and alter the "Fade At Outro Start" feature as described?

@davidlmorris
Copy link
Author

I thought AutDJ was really only for unattended mixes (although I also use it as a 'current' playlist), so a per-track option is not a 'live' option unless you have gone through each song on the playlist and set this up, right? Or have I misunderstood what you are suggesting?

This doesn't mean I would be against another separate option. My solution is just a quick fix for the problem of tracks not starting at full volume when using AutoDj, regardless of what setting you are using. One I would be very keen to see implemented ASAP.

@daschuer
Copy link
Member

Auto DJ shall be usable for complet unattanded mixing but also allow seamless interaction with a real DJ.

So a per-track option is not a 'live' option unless you have gone through each song on the playlist and set this up, right?

Yes, right. For many of my track I have only set the cue point, used as load cue. Some have also a difined intro and outro.
Most club mixes have a reasonable long intro that it works by default using the fade time. Somtimes I set an intro to protect the track from a transition into vocals or skipping unusable embient noise. That it works, but than there are Tracks like "Draft Punk - Technologic" which really shall be used without any fade in. Without an intro set, the probably long outro is used. With short intro the bevious track is cut off unleasant. I need a way to fix it in an unattenden mix.

My solution is just a quick fix

The issue with the extra option for all transition is that it is more than a quick fix for your particular use case, but it still does not help for my use case. It may limit use for future improvments because some useres will get used to this option and we may hasitate to remove it.

@daschuer
Copy link
Member

Proposal:
Only do one step: Add a new "Skip Silence, fade out" transition to fix you use case.

Later or in addition, we may also add the workaround for my use case.

@davidlmorris
Copy link
Author

It may limit use for future improvments because some useres will get used to this option and we may hasitate to remove it.

You say that like it is a bad thing?

Proposal:
Only do one step: Add a new "Skip Silence, fade out" transition to fix you use case.

Why limit this? It is after all a setting in preferences, that doesn't affect extant behavior at all. If you want less, then don't set the check box. If you want more... then create a new feature that can be implemented later, right? What problem are we trying to prevent here?

@daschuer
Copy link
Member

You say that like it is a bad thing?

Yes, I mean it actually: https://www.youtube.com/watch?v=glZ1C-Yu5tw&t=206s

Why limit this?

because it breaks the concept of full auto, for the intro/outro related modes.

@davidlmorris
Copy link
Author

because it breaks the concept of full auto, for the intro/outro related modes.

How? Just don't set the checkbox (like it is now).

Are we trying to do too many things here?

@davidlmorris
Copy link
Author

Yes, I mean it actually: https://www.youtube.com/watch?v=glZ1C-Yu5tw&t=206s

I watched this. I did find him a tad hyperbolic. It is an argument of usability and functionality over the complexity of development. These things are, of course, a trade-off. In my view, not an argument for an incomplete user experience.

However, this isn't my software - it is yours or your team's. If you think this is a bad proposal, then I understand that you won't include it, even if I don't understand the reasoning. Do let me know, since I would like to adapt it for my purposes (since I do think Mixxx is great software), and I would like to fork a more stable version than the nightly build.

@daschuer
Copy link
Member

daschuer commented Sep 12, 2024

Oh sorry, it was not my intention to reject your work. It was probably also a bad idea to post the video, which shows the issue exaggerated. I highly appreciate your work and the fader trick you demonstrate here is really a missing feature.

@daschuer
Copy link
Member

daschuer commented Sep 12, 2024

Let's summe up:

  • You need a new mode like "skip silence" without fade in. And you will use it permanently.
  • I need no fade in options for certain tracks in "Fade At Outro Start"

I am happy to merge your demand.
Adding this center mode to all transitions would create features where we have no demand for. It is always difficult to test and discuss edge cases for a "feature" that is not used anyway and only a by-catch.

That's why I have proposed to go in small steps and only solve your problem, which is for my understanding covered with ""Skip Silence, fade out".

Once this is merged, we can build on that in any direction.

@davidlmorris
Copy link
Author

Thanks. (I was trying to reduce complexity, while not messing with other user's expectations.)

For the most part "Skip Silence, fade out" is at least good enough for now (and certainly better than nothing!). I can see situations where I might not be skipping silence (dinner party background et al) or using the into and outro, but "Skip Silence" is certainly my most pressing requirement.

I have a further more detailed idea which I hinted at above... which would require a much bigger effort to dig through and understand the code, the biggest part probably is to add more 'unseen' cue points, being the first and last -6db (from ref), - 10db, -20db and maybe others (I would need to play around and hear the results).

What would you see as the biggest problem with adding hidden cue points like the existing 60db point? (e.g. Database reformat?)

@davidlmorris
Copy link
Author

One other thing worth mentioning is the potential unintended consequence for someone who has set (under preferences Mixer) the Crossfader Curve to Constant power.

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.

The new mode does not work well with const power mode, which is also usable for radio. Because it avoids loudness peaks during the transition. In this case we have an immediately loudness drop in the outgoing track. Original this double loudness issue was only short, during crossing the center. Now we have it during the full first half. A real DJ will use EQ or filter to avoid blowing the speakers.

What could be the approach here?

Fade out the outgoing track and delay the start of the incoming track? Ideas?

@@ -832,6 +836,7 @@ void AutoDJProcessor::playerPositionChanged(DeckAttributes* pAttributes,
double crossfaderTarget;
if (m_eState == ADJ_LEFT_FADING) {
crossfaderTarget = 1.0;
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this extra line

@@ -859,17 +864,31 @@ void AutoDJProcessor::playerPositionChanged(DeckAttributes* pAttributes,
double transitionProgress = (thisPlayPosition - thisDeck->fadeBeginPos) /
(thisDeck->fadeEndPos - thisDeck->fadeBeginPos);
double transitionStep = transitionProgress - m_transitionProgress;

if ((transitionProgress == 0.0) &&
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if the == 0.0 state is always reached, because of rounding issues.i think it is more reliable to move this in an else branch of the condition below.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed, this condition is never true and can be removed.

Copy link
Author

Choose a reason for hiding this comment

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

After some testing (and an assert), I couldn't find this state being reached either. So I agree it can be removed.

// we move the crossfader linearly with
// movements in this track's play position.
setCrossfader(currentCrossfader + adjustment);
if ((m_crossfaderStartCenter) &&
Copy link
Member

Choose a reason for hiding this comment

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

Auto DJ should allow to adjust the Crossfader during a transition. So the code must be able to continue fading from any position. In this case I can imagine a emergency condition where the user realizes that he wants fade in and moves the xfader rapidly to the beginning.

Copy link
Author

Choose a reason for hiding this comment

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

This only happens once and the crossfader is now at the new position, so the user can adjust.

((thisDeck->fadeEndPos -
thisDeck->fadeBeginPos) /
2))) {
// We want the cross fader to remain in the middle.
Copy link
Member

Choose a reason for hiding this comment

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

This will constantly fight with the user. I think it is better to just not touch the crossfadere here. It will feel naturally if we fall back to the default mode when xfader is no longer in the expected center position.

Copy link
Author

Choose a reason for hiding this comment

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

This should now be:

transitionProgress <= std::midpoint(thisDeck->fadeBeginPos, thisDeck->fadeEndPos) {

to make it clearer what is going on.

In my testing, neither constantly fights the user. If the user moves the fader it goes there and completes the transition.

Copy link
Member

Choose a reason for hiding this comment

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

I can confirm that this readjust the user tweaks in some conditions. It happens because you compare here fractions of the full track length with fractions of the transition. I think the best solution is to move this code to Line 823 as

                if (m_crossfaderStartCenter) { 
                    setCrossfader(0.0);
                } else if (thisDeck->fadeBeginPos >= thisDeck->fadeEndPos) {
                    setCrossfader(thisDeck->isLeft() ? 1.0 : -1.0);
                }

Copy link
Author

Choose a reason for hiding this comment

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

Please show the conditions. I can't find any in my testing.

Copy link
Author

Choose a reason for hiding this comment

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

I've had a play around with this. And I think you are correct. It seems you only need to set the position of the crossfader to the middle once. And the best position would be just before the other desk starts playing. So I would propose this at around line 820:

                if (m_crossfaderStartCenter) {
                    setCrossfader(0.0);
                } else if (thisDeck->fadeBeginPos >= thisDeck->fadeEndPos) {
                        setCrossfader(thisDeck->isLeft() ? 1.0 : -1.0);
                }

                if (!otherDeckPlaying) {
                    otherDeck->play();
                }

The reason is that I noticed that occasionally the audio seems to just miss the very start of a track (I noticed it on a cold vocal start). This may have been a glitch in my system, so I cannot attest to this, but it seems the best thing to do in any event.

@@ -1343,6 +1362,7 @@ void AutoDJProcessor::calculateTransition(DeckAttributes* pFromDeck,
}

switch (m_transitionMode) {
// case TransitionMode::RadioFullIntroOutro:
Copy link
Member

Choose a reason for hiding this comment

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

What is that?

Copy link
Author

Choose a reason for hiding this comment

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

As I said elsewhere that is a comment form an earlier experiment, that I neglected to delete.

@daschuer
Copy link
Member

I can see situations where I might not be skipping silence (dinner party background et al)

Crossfading in full track mode is probably another by catch without a use case. We kept this mode, because it was the default before introducing silence detection.
I am happy to alter it to the real use cases like gapless life recording / symphony mode.

Do you need the fade out part at dinner?

Or using the into and outro ...

Can you confirm my demand in that case?

@daschuer
Copy link
Member

There is no issue to add more hidden cue points as long at they are auto detected.

@davidlmorris
Copy link
Author

My changes left the existing behavior completely intact unless you had the option check box ticked. If you move the fader after it starts the crossfade it just goes to where you put it as you would expect (like when the option box is unchecked).

Fade out the outgoing track and delay the start of the incoming track? Ideas?

My idea originally was the option check box, as a follow-up to another change to use the same message as a reminder of the side effect. See #13619 for an illustration.

I think you are making significant changes to what I proposed. I suspect that some of these are breaking changes, and since you know the software better than me, then I think you ought to be making them rather than me. (That and my lack of confidence in my understanding of Git). I am still unclear if you are adding another option to the drop-down menu, or changing an existing one.

And, I can't see what you are doing to test it. Have you tested my changes?

There is no issue to add more hidden cue points as long at they are auto detected.

That is brilliant news. These would and should be auto-detected and hidden just like the -60db silence marker. But that is for another time in the future (I suspect a distant future, now).

@daschuer
Copy link
Member

My changes left the existing behavior completely intact unless you had the option check box ticked. If you move the fader after it starts the crossfade it just goes to where you put it as you would expect (like when the option box is unchecked).

I can confirm this. In a new test I just did.

I think you are making significant changes to what I proposed.

My demand for this PR is to turn you option in to a new transition mode as discussed. Any other changes can be done in follow up PRs.

I think you ought to be making them rather than me.

I have currently other priorities, like releasing 2.4.2. Sorry.

And, I can't see what you are doing to test it. Have you tested my changes?

I have tested your original PR and did a review here without testing. I have probably misread your code.

@@ -566,6 +568,8 @@ AutoDJProcessor::AutoDJError AutoDJProcessor::toggleAutoDJ(bool enable) {
setCrossfader(1.0);
}
}
m_crossfaderStartCenter = m_pConfig->getValue<bool>(ConfigKey(kConfigKey,
Copy link
Member

Choose a reason for hiding this comment

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

m_crossfaderStartCenter should be also initialized in the constructor.
Currently we need to restart auto DJ to switch the modes. When it becomes a transition mode, it can be already changes on the fly.

Copy link
Author

Choose a reason for hiding this comment

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

As far as I can tell AutoDj is instantiated just once at the program start, so I've made the change even if I think is a tad redundant for completeness. "Currently we need to restart auto DJ to switch the modes.", is true, like a lot of changes made in options.

@davidlmorris
Copy link
Author

While I have made the changes I mentioned above to my code. I won't be making any further commits until I understand git better and have understood the implications of other changes, some of which as I said before I am uncomfortable with.

(I've been playing with it this morning, and it seems to make even cold cuts sound 'ok-ish' [as in so, so much better] with a 4-second crossfade time - so I'll modify version 2.4 for my use in the meantime, and maybe 2.5 when it comes out).

@davidlmorris
Copy link
Author

Ok, I pushed this to my repository... forgetting that it was connected to this pull... hopefully, this is not an 'ooops' situation again. These are changes made or suggested above.

@daschuer
Copy link
Member

My demand for this PR is to turn you option in to a new transition mode as discussed. Any other changes can be done in follow up PRs.

Can you also re-introduce the the new transition mode rather than the preferences option? Than this is immediately merge-able IMHO.

Reintroduction of a new transition mode: called "Skip Silence Start Full Volume".
Removal of preferences option.
@davidlmorris
Copy link
Author

As "demanded":
Reintroduction of a new transition mode: called "Skip Silence Start Full Volume".
Removal of preferences option.

I've left in the global m_crossfaderStartCenter and supporting code in autodjprocessor.cpp. This can be used to extend other transmission modes, when or if required.

Not included: is a much much better transition, which (I assume) I should do as a separate pull request, that creates some hidden cue points, which correspond to the first and last sound in the track at specific volumes, and then uses those to start the fade out, up to a maximum set by the user in the spin box. This means that cold endings and cold starts can mesh perfectly, and fade-outs happen sensibly. Other than a few gremlins which I "think" I have sorted out - but will test more, I think it now does a better job "mixing tracks" than I do especially on cold endings!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants