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

AutoDJProcessor: Calculate and expose the number of tracks and accurate total duration in the Auto DJ queue #13183

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

Conversation

cr7pt0gr4ph7
Copy link
Contributor

@cr7pt0gr4ph7 cr7pt0gr4ph7 commented Apr 28, 2024

The calculated total duration take into account the transition modes, transition time etc. (in fact, it reuses the code that performs the actual transition during queue playing).

The queue_duration time_remaining and queue_tracks tracks_remaining are exposed as ControlObjects so they can be consumed by skins.

The time_remaining and tracks_remaining controls in the [AutoDJ]
can be used to display the information about the Auto DJ queue
in custom skins.

The current calculation method for the time_remaining control
only considers the total duration of the tracks by themselves,
and does not take the transition times between tracks into account.
This is a first step in generalizing the transition logic in
AutoDJProcessor to enable it to also be applied to tracks in
the queue instead of just the current decks.
This is another step in generalizing the transition logic
in AutoDJProcessor to enable calculating transition times
for the whole queue.
This is the final step in generalizing the core transition timing
logic in AutoDJProcessor, so that it can be applied to a whole
playlist instead of just the current decks.
…n relevant

The following data points are used as inputs for the "remaining time"
calculation, and should therefore trigger a recalculation:

* The list of tracks in the Auto DJ playlist (both
  the set of tracks and their order are important).

  We subscribe to PlaylistTableModel::tracksChanged
  to receive the relevant notifications.

* The AutoDJ transition mode and transition time settings.
  We will be notified via ::setTransitionMode
  and ::setTransitionTime, respectively.

* The Intro, Outro & N60dBSound cues of all tracks
  that are contained in the Auto DJ queue.

  We subscribe to TrackCollection::tracksChanged
  and TrackCollection::multipleTracksChanged to
  receive notifications about possible changes.

  As of now, we do not filter the notifications,
  and simply trigger a recalculation when ANY
  track has changed.
@cr7pt0gr4ph7 cr7pt0gr4ph7 force-pushed the autodj-time-remaining branch from dca71bf to 1923d12 Compare May 19, 2024 16:30
@cr7pt0gr4ph7 cr7pt0gr4ph7 force-pushed the autodj-time-remaining branch from 1923d12 to 2f5f0ea Compare May 19, 2024 17:24
@cr7pt0gr4ph7
Copy link
Contributor Author

@ninomp Maybe you can have a look at reviewing this PR

Copy link

github-actions bot commented Sep 7, 2024

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Sep 7, 2024
@daschuer
Copy link
Member

daschuer commented Sep 9, 2024

Oh sorry, a conflict has developed.

@daschuer daschuer added needs review and removed stale Stale issues that haven't been updated for a long time. labels Sep 9, 2024
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.

This is a good start. I am afraid the calculation is not yet correct in any case. Did you consider to reuse the original transition calculation code?

This PR includes a lot of useful refactorings.
I have interest to merge them first in a separate PR. Can you prepare that?

What is the use case of the new duration controls?

src/library/autodj/autodjprocessor.cpp Outdated Show resolved Hide resolved
@cr7pt0gr4ph7
Copy link
Contributor Author

cr7pt0gr4ph7 commented Sep 9, 2024

Thanks for having a look at reviewing this 👍

Oh sorry, a conflict has developed.

Thats one reason why I want it merged into upstream, at least at one point in the future 😅 I will have a go at resolving the conflicts as soon as I find time again.

This is a good start. I am afraid the calculation is not yet correct in any case. Did you consider to reuse the original transition calculation code?

Uh, I am not sure what you mean. Just to make sure that I understand you correctly:

I actually use the original transition calculation code, but had to refactor it so that I could apply it not only to tracks already loaded into a deck, but to playlist tracks as well. Mistakes not withstanding, of course.

These changes are isolated into the commits 5c6ce7a (review link), b646631 (review link) and 3eba5f2 (review link), which I tried to keep small and self contained.

I have also tested the feature with different transition modes, transition times etc., and haven't noticed any problems.

Is there a specific part of the code that worries you?

This PR includes a lot of useful refactorings. I have interest to merge them first in a separate PR. Can you prepare that?

Sure. I actually already structured the commits in a way that would make this possible. Which changes would you like to get in a separate PR?

What is the use case of the new duration controls?

To display them on DJ controllers and/or in skins and/or in a status bar (see also #13176 for an example of how it could be done).

@github-actions github-actions bot added the build label Oct 4, 2024
@cr7pt0gr4ph7
Copy link
Contributor Author

cr7pt0gr4ph7 commented Oct 4, 2024

@daschuer I've resolved the merge conflicts and moved TrackOrDeckAttributes and its subclasses into separate files.

As mentioned in the previous comment, this reuses the original transition code for both the actual transitions, and the ahead-of-time playlist length calculations (see autoprocessor.cpp:L1239-L1503). I've only renamed variables like pFromDeck -> fromTrack, and added TrackOrDeckAttributes and TrackAttributes to generalize code that was previously using DeckAttributes where possible. In every other regard, the transition code should behave 1:1 as before.

(Update 1: Negative transition times (i.e. silence between tracks) are currently not correctly accounted for in all cases. I'm working on a solution) (Update 2: Fixed.)

There are one two (small) inconsistencies, which are currently by design:

  1. The duration of the selected tracks (which is also displayed in the Auto DJ view) is always calculated from the full duration of the tracks, because if you are selecting multiple non-continguous sequences of tracks using Ctrl, you can only calculate the transitions within each contiguous sequence, but not between non-adjacent sequences.

    • One alternative solution would be to actually calculate the duration for each contiguous part of the selection, and then sum them up to get a total.
      • This is more complicated (and slower, but probably not noticeably), but has the benefit of avoiding a difference between the total duration of the queue and the duration displayed when all tracks in the queue are selected.
      • The only remaining inconsistency here would be: The duration displayed for [A B] C [D E] ([] shows the current selection) would be different than the one displayed for [A B D E] (same selected songs, but with C removed altogether).
    • The other alternative would be to treat the non-continguous selection as a continguous playlist, and calculate the transitions + total duration based on that assumption. This solution has its merits, too.
    • Maybe it makes sense to combine one of these with the current solution, and display something like: [full duration of selected tracks] ([# of selected tracks]) ([duration of selected tracks] with transitions)) / [duration of queue] ([# of queued tracks]), e.g. 10:15 (3) (08:54 with transitions) / 20:11 (8).
  2. The Remaining Queue Playtime calculation is done based solely on the contents of the Auto DJ queue and the configured transition time times between its tracks. In particular, it does not take the currently playing track(s)1 or the transition time from the currently playing track to the next track in the queue into account.

After thinking about the latter issue, there are two different usecases, only one of which is fully solved here - and it might make sense to rename our control objects to allow implementing the other one in the future:

Use Case 1 Use Case 2
What? Length/Duration of Auto DJ queue Remaining playtime in Auto DJ mode
When is it relevant? (Use case) Preparing an Auto DJ queue while something else/unrelated is playing: "How long will this list of tracks be playing after I start it?" While playing using Auto DJ: "How much time do I have until music stops?"
When does this number change? Discrete: Whenever the track list changes, i.e. when loading the next track Continuously: All the time (while playing)
Solved by this PR? Yes Partially (the user has to manually add the remaining track time to the remaining queue time in their head)
Suggested CO names [AutoDJ] queue_duration and [AutoDJ] queue_tracks [AutoDJ] autodj_time_remaining

Footnotes

  1. Sidenote: Determining the "current track" from which we will transition to our next track in the queue is only possible when Auto DJ is either already active, or could be activated. It is not possible when e.g. multiple tracks are currently playing.

@cr7pt0gr4ph7 cr7pt0gr4ph7 force-pushed the autodj-time-remaining branch from 99c257f to 7a00a77 Compare October 4, 2024 15:22
@cr7pt0gr4ph7
Copy link
Contributor Author

As it turns out, the transition calculation code seems to work fine, but I had messed up the small piece of new code that calculates how a given transition between two tracks affects their total playtime... fixed now.

@cr7pt0gr4ph7 cr7pt0gr4ph7 requested a review from daschuer October 31, 2024 14:21
@cr7pt0gr4ph7
Copy link
Contributor Author

cr7pt0gr4ph7 commented Oct 31, 2024

Can I do anything to move this PR forward/get it merged? It's been one month....

Implementation is complete, it is useful and follows existing UI conventions in Mixxx (with respect to displaying playlist and crate lenghts), and I'm already running this in my production setup and haven't encountered any problems so far.

PS: If you want to quickly try it out without having to build it yourself, you can use the io.github.cr7pt0gr4ph7.Mixxx Flatpak published at cr7pt0gr4ph7/mixxx-flatpak that includes this and a few other changes. Parallel installation is possible, i.e. any existing official or Flatpak installation of Mixxx will not be affected:

sudo flatpak remote-add --if-not-exists mixxx-flatpak https://cr7pt0gr4ph7.github.io/mixxx-flatpak/default.flatpakrepo
sudo flatpak install io.github.cr7pt0gr4ph7.Mixxx

@ninomp
Copy link
Contributor

ninomp commented Oct 31, 2024

Hi @cr7pt0gr4ph7 Sorry for delay. I actually did go through this PR some time ago. I also gave it a test and couldn't find anything wrong, so as far I'm concerned, this can be merged.

I would like to give it one more look and test before giving final LGTM. I'll try to do it soon.

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.

3 participants