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

make better use of intro/outro markers with AutoDJ #2103

Merged
merged 225 commits into from Nov 10, 2019

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented May 6, 2019

and add a checkbox to revert to the old behavior that does not use the
new intro/outro markers at all. With the box checked (which is the
default), AutoDJ uses the outro of the outgoing track or the intro of
the incoming track for the transition time, whichever is longer. If only
one track has these marked, use that track's marked transition time. If
neither track has the intro/outro range marked, fall back to the fixed
number of seconds specified by the spinbox. If users do not want to
interrupt the workflow they are accustomed to when upgrading to Mixxx
2.3, they can simply uncheck the new box.

and add a checkbox to revert to the old behavior that does not use the
new intro/outro markers at all. With the box checked (which is the
default), AutoDJ uses the outro of the outgoing track or the intro of
the incoming track for the transition time, whichever is longer. If only
one track has these marked, use that track's marked transition time. If
neither track has the intro/outro range marked, fall back to the fixed
number of seconds specified by the spinbox. If users do not want to
interrupt the workflow they are accustomed to when upgrading to Mixxx
2.3, they can simply uncheck the new box.
@Be-ing Be-ing mentioned this pull request May 6, 2019
15 tasks
@Be-ing
Copy link
Contributor Author

Be-ing commented May 6, 2019

With this new behavior, if a user does not put in the effort to manually mark intro end and outro start markers, AutoDJ works just as it did before but with the added benefit of cutting out silence.

@uklotzde
Copy link
Contributor

uklotzde commented May 6, 2019

I suggest using the shorter duration of both outro and intro instead. Otherwise you risk to cut off the main part from one of the tracks. The silence part after the outro end and before the intro start is already excluded, so you will never get silence during a transition.

@uklotzde
Copy link
Contributor

uklotzde commented May 6, 2019

Counter example when using the longer duration: A track with a long instrumental outro (30 sec) will play over the first verse of the next track with a short intro (3 sec).

@Be-ing
Copy link
Contributor Author

Be-ing commented May 6, 2019

Otherwise you risk to cut off the main part from one of the tracks.

I don't understand. In what scenario could anything be cut off?

Counter example when using the longer duration: A track with a long instrumental outro (30 sec) will play over the first verse of the next track with a short intro (3 sec).

Yes, that is what I intended. I initially tried choosing the shorter duration, but thought the longer one generally worked better. I suppose there are use cases for both, depending on the intended effect. How about turning the new checkbox into a 3 state combobox?

  • Do not use intro/outro markers
  • Use shorter of intro/outro sections
  • Use longer of intro/outro sections

@uklotzde
Copy link
Contributor

uklotzde commented May 6, 2019

The crossfade phase should be short since we don't have beat nor phrase synchronization. If I enable AutoDJ I don't expect long, sophisticated mixes, but instead predictable behavior and pleasant results without periods of silence while playing unattended.

I don't expect that the long duration will be used frequently, but the short duration is mandatory for me. How about offering both and collecting feedback? Removing an option later is not very user-friendly, but I don't want to exclude any of those options upfront.

@uklotzde
Copy link
Contributor

uklotzde commented May 6, 2019

The longer duration may work for extend mixes or ambient styles, but definitely not for a mixture of short radio edits and only a few tracks with extended intros or outros (as found on album versions).

@daschuer
Copy link
Member

daschuer commented May 6, 2019

For me it is unclear what to mark as intro. I think we need a clear definition to make subsumptions how the Auto DJ should deal with it.

The markers are named "intro start" and "intro end" where "intro start" id by default the first notable sound. You reuse the markers her as fade start and fade end. This is a fundamental difference.

A extreme is for example:
fury in the slaughterhouse - time to wonder
https://www.youtube.com/watch?v=qk7Xt4X0qyw

The intro is 1:06 than guitar to 1:38 and than vocal starts.

In Juke box mode may I like to play the track from the start. For DJ-Ing I have a CUE point placed at the first beat after 1:06. All works fine in Mixxx 2.2.

  • -2 s transition and load at start for Juke box
  • 4 s transition and load at CUE for DJ-ing

How can I achieve the same in Mixxx 2.3?

Starting from the first sound instead of 0:00 is a nice feature. No manual track editing required.
If I am annoyed from a long intro even in Juke-Box mode I can shift the "intro start".

What is the "intro end" then?
I may place it at 1:06, and it becomes a "DJ-Mode Load CUE"
Or I can place it at 1:38 and it becomes a "End transition here!" warning
Or I can place start and end marker after 1:06 .. 1:10 to define an individual fade in region.

@ronso0
Copy link
Member

ronso0 commented May 6, 2019

I suggest using the shorter duration of both outro and intro instead. Otherwise you risk to cut off the main part from one of the tracks

..except when both outro and intro are equally long AutoDJ would always play over the main part of one track. I can't imagine how this could be intended.
What are the use cases for using the longer in/outro?

Can we use radio buttons O CUE point [spinbox] sec. and O Intro/outro range and put both choices in the AutoDJ feature in the skin?

@ninomp
Copy link
Contributor

ninomp commented May 6, 2019

@Be-ing I don't see that you change SeekOnLoad mode of decks -- tracks will still be loaded at Intro Start.

Be-ing added 3 commits May 6, 2019 21:20
This allows for completely ignoring the intro/outro markers with AutoDJ.
and rename some code for clarity
@Be-ing
Copy link
Contributor Author

Be-ing commented May 7, 2019

@daschuer:
Mark the part of the track you want to mix in as the intro. That can be anywhere in the track; it is up to you. If you want AutoDJ to ignore the intro & outro markers, choose that mode in the new combobox in the AutoDJ library feature. When this mode is selected and AutoDJ is active, tracks load now at the beginning (as they did before #1242 was merged). The preference option to choose where to seek tracks upon loading is overridden by AutoDJ when AutoDJ is enabled. I have added a third option for this preference so tracks can be loaded at the intro start for manual DJing.

If you want AutoDJ to use the main cue points you have already set, I think the best way to do that would be setting the intro start point to those cue points. Perhaps you could add a button in the AutoDJ preference window to do this for the whole library. We could then remove the button for 2.4 as it should only be needed for migrating to 2.3. We should not assume how users have used the main cue point in the past, so I think this should be manually triggered by the user. Going forward, I don't think AutoDJ should have anything to do with the main cue point (which is the current state in this branch).

@uklotzde, @ronso0:
I still do not understand what you mean by cutting off parts of tracks. If you can find a way to make that happen with this branch, please explain. Screenshots would be helpful.

@Be-ing
Copy link
Contributor Author

Be-ing commented May 7, 2019

The default for the new combobox in the AutoDJ library feature is to use the shorter of the intro/outro. Using the longer of the intro/outro is still an option. After factoring out some repeated code into functions, maintaining both is rather simple.

(I know the tests are not working now. I want to reach a consensus on the desired behavior before fixing those.)

case SeekOnLoadMode::Beginning:
// This allows users to load tracks and have the needle-drop be maintained.
if (!(m_pVinylControlEnabled->get() &&
m_pVinylControlMode->get() == MIXXX_VCMODE_ABSOLUTE)) {
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'm not sure if this should be checked for any other SeekOnLoadModes. @ywwg maybe you can clarify?

@Be-ing
Copy link
Contributor Author

Be-ing commented May 7, 2019

Maybe we should call AutoDJ SemiautoDJ now :P IMO this is more useful than what any other DJ software does.

@daschuer
Copy link
Member

daschuer commented May 7, 2019

Mark the part of the track you want to mix in as the intro. That can be anywhere in the track; it is up to you.

This way the term intro is just wrong. In the DJ use case, I don't want to use a long intro, just because the previous track as a short outro for transition.

This way we reuse the intro start amend markers as "transition start" and "transition end".

We may rename these markers, but in this case we loose the original intro region, which is useful for the juke box use case and for visualisation during DJing.

If you want AutoDJ to ignore the intro & outro markers, choose that mode in the new combobox in the AutoDJ library feature.

I have no option there for load at "first sound" if we reuse the markers like above.

The preference option to choose where to seek tracks upon loading is overridden by AutoDJ when AutoDJ is enabled.

I think changing the load behavior according to the AutoDj was a bad decision.
We always use the AutoDJ as it would be a real DJ. We have even a Bug to have Auto DJ always on. I strongly recommend to follow this paradigm. This keeps finally the ability to interact smoothly with the AutoDj without hidden magic.

I have added a third option for this preference so tracks can be loaded at the intro start for manual DJing.

Thank you.

If you want AutoDJ to use the main cue points you have already set, I think the best way to do that would be setting the intro start point to those cue points.

Nooo. You cannot require every user to do that for all tracks in Library. Imagine somone installs Mixxx 2.3 without doing this. The gig will be a mass.

We need a Auto DJ mode loading at Cue.
And thus should be default, if load at cue was selected in 2.2.

Perhaps you could add a button in the AutoDJ preference window to do this for the whole library.

This breaks the new and IMHO important silent detection feature.

We should not assume how users have used the main cue point in the past, so I think this should be manually triggered by the user.

We can if the track was loaded at cue. It was used as load cue than.

Going forward, I don't think AutoDJ should have anything to do with the main cue point (which is the current state in this branch).

It should. It is my use case. How do you use the cue point? Something else than cue-ing a new track in? Please explain, so I can understand your needs.

How will the Juke box use case work finally?

@Be-ing
Copy link
Contributor Author

Be-ing commented May 7, 2019

I have no option there for load at "first sound" if we reuse the markers like above.

I think the only way to do that would be to add new cues for the autodetected first/last sounds independent of the intro/outro ranges and set the intro start/outro end to these autodetected points by default (as they are currently). These new first/last sound points could potentially be hidden and read-only to avoid complicating the UI. If AutoDJ uses those when not using the intro/outro ranges, that breaks one of my use cases (playing an album straight through with transition time 0, including all silence as it is on the album), so I think a 4th AutoDJ mode would be needed.

How do you use the cue point?

The cue point is merely where I last pressed play. Nothing more. If Mixxx interprets my main cue points as having any more meaning than that, it will be wrong and likely sound bad.

@daschuer
Copy link
Member

daschuer commented May 7, 2019

How do you use the cue point?

The cue point is merely where I last pressed play. Nothing more. If Mixxx interprets my main cue points as having any more meaning than that, it will be wrong and likely sound bad.

So you load a track always at start, in 2.2. right? I think you and me are presenting the two different typical users then.

Do you use Denon Cue mode, for moving cue with play?

I am in a good mood that it should be able to find a solution for both with a limited number of extra options and no addition GUI clutter.

Be-ing added 5 commits May 7, 2019 11:31
"tio" was a legacy name for TrackInfoObject, which is now just Track
This will allow for adding a new AutoDJ fade mode that skips silence
regardless of where the intro/outro markers are.
@Be-ing
Copy link
Contributor Author

Be-ing commented May 7, 2019

I have added new FirstSound and LastSound cue types independent of the Intro/Outro cues. The intro start/outro end are placed at the same point as these by AnalyzerSilence, but the FirstSound and LastSound cues do not move when the user adjusts the intro/outro cues. I have added a new AutoDJ mode to skip silence with a fixed fade time. This is the only change to the GUI for these new cue types; they are not shown on the waveforms or editable by the user.

@daschuer
Copy link
Member

daschuer commented May 7, 2019

Thank you for the work. I an not sure if this is a good solution, it might be finally. But let's first define a solution that works for us both. Currently I am in doubt that these new hidden cues work. I will prepare a post with the requirements we have so far.

@daschuer
Copy link
Member

Another try:
Be-ing#38

quantisation fix + cue fix + git-clang-format
@daschuer
Copy link
Member

Everything green finally. LGTM

@daschuer daschuer merged commit b4c9ca1 into mixxxdj:master Nov 10, 2019
@uklotzde
Copy link
Contributor

What's the different between HotCue and Jump? From my understanding these terms are used synonymously. Cue::Type::Jump is unused in Mixxx.

In aoide I prefer to use the shorter term Jump for HotCue position markers. Just noticed this while adjusting the mapping.

@Be-ing Be-ing deleted the autodj_intro_outro branch November 11, 2019 21:53
@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 11, 2019

You're right, Cue::Type::Jump is unused. We may as well rename it to Cue::Type::Placeholder.

@uklotzde
Copy link
Contributor

I've chosen a different naming for the position markers in aoide, but the mapping is straightforward:

//const int kPositionMarkerCustom = 0; // unused/unsupported
const int kPositionMarkerLoad = 1; // = Cue::Type::MainCue
const int kPositionMarkerMain = 2; // = Cue::Type::AudibleSound
const int kPositionMarkerIntro = 3;
const int kPositionMarkerOutro = 4;
const int kPositionMarkerJump = 5; // = Cue::Type::HotCue
const int kPositionMarkerLoop = 6;
//const int kPositionMarkerSample = 7; // unused/unsupported

The most notable difference is that in aoide Load is the "main" cue point with the initial position after loading while Main is supposed to be a range, i.e. the audible/main part of the track.

@daschuer
Copy link
Member

Cue::Type::Jump was planed to be a jump cue, a kind of reverse loop to automatically jump over a region, in the track.

Where do the aoide definitions come from?

@uklotzde
Copy link
Contributor

Intuition, practical experience, some research. Unfortunately, I have not collected the sources. The intention was to cover the common use cases without introducing any conflicting or redundant definitions.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 11, 2019

Can we continue this conversation on Zulip? This is off topic from this PR.

@uklotzde
Copy link
Contributor

So Mixxx::Jump == Serato::Flip. Not supported by aoide yet.

Closed.

@Holzhaus
Copy link
Member

Serato Flip works differently, it supports multiple jumps at different positions.

ehendrikd added a commit to ehendrikd/mixxx that referenced this pull request Nov 12, 2019
@Be-ing Be-ing mentioned this pull request Jan 18, 2020
1 task
@Holzhaus Holzhaus added this to Done in 2.3 release Mar 14, 2020
// outro of the fromDeck track is longer than the whole toDeck
// track
outroLength = pToDeck->fadeBeginPos - introStart;
VERIFY_OR_DEBUG_ASSERT(outroLength > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This debug assertion is triggered inside a lot of the AutoDJ processor tests: https://travis-ci.org/github/mixxxdj/mixxx/jobs/705774858#L2692
You notice it when compiling with #2911.

@daschuer You introduced this assertion in 010fd5c and know the AutoDJ code better than I do. Can you have a look?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe @Be-ing check this?

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

Successfully merging this pull request may close these issues.

None yet

10 participants