Skip to content

Conversation

stkw0
Copy link
Contributor

@stkw0 stkw0 commented Sep 27, 2024

When playing random songs supersonic had a hardcoded value of 100. This was very small for my taste. This PR add the option to configure it.

I found out, however, that the current algorithm is worryingly slow. With a value of 1000 it takes more than 20 seconds on my computer. Recovering the queue when restarting supersonic is also very slow.

@dweymouth
Copy link
Owner

I am planning on working on an alternate approach to this soon-ish - continuous queuing mode. The idea would be that when playing random songs, it would load 100, but then when it gets to the last song in the queue, load the next 100, and so-on. This would get rid of the slowness of loading 1000s of songs from the server at once, and it would also be a mode you could enable when listening to an album, for example, where once it finishes the album it plays similar songs automatically

@stkw0
Copy link
Contributor Author

stkw0 commented Sep 28, 2024

I know popular applications that also do this, and while I think it will be a nice addition, it's not something I personally like.
I like to be able which songs will play next, and sometimes I sort them or remove one of them. Having a big playlist that I can keep playing each day makes it easier.

I think the slowness is a different issue. Yesterday I made a flamegraph and it showed that most of the time is wasted doing Unmarshal of XML. For android I use symfonium music player, and it is capable of creating a playlist with 6000 songs in less than a second and recovering it (when restarting the application) in less than five seconds. I will be truly happy if supersonic could get close to that in the future.

@@ -296,6 +309,10 @@ func (s *SettingsDialog) createGeneralTab(canSaveQueueToServer bool) *container.
saveQueueHBox,
trackNotif,
albumGridYears,
container.NewHBox(
widget.NewLabel(lang.L("Queue size: ")),
Copy link
Owner

Choose a reason for hiding this comment

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

I think this needs a more descriptive settings name. "Queue size" is kind-of ambiguous, maybe something like "Random queue size"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although now that I remember, that setting also applies for "play similar artists". Should be better to remove it?

Copy link
Owner

Choose a reason for hiding this comment

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

We could remove it from the settings dialog but leave it in the config file. There are already a few settings that are config-file-only (which most people probably won't need to change)

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 think some users could find it useful to easily change it, but maybe it's just my personal preference. I remove it from the UI.

btw, there is plans to improve the performance regarding the lists?

Copy link
Owner

Choose a reason for hiding this comment

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

Well aside from switching away from Go's standard XML parser, or reworking the whole subsonic library to use JSON (including the work to handle date idiosyncracies across different servers), there's not much to be done. Switching to JSON would be something I would review if someone else were to do the work but it will be low on my personal priority list.

Copy link
Owner

Choose a reason for hiding this comment

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

To add onto this, it would not be a very large amount of work to switch to JSON, but the custom marshalXML/unmarshalXML implementations for the xsdDateTime type in here would need to be implemented and tested with the JSON parser as well. All the workarounds for things like timestamps with and without timezone suffixes would have to continue to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it would be feasible and easier to use concurrent jobs?

Copy link
Owner

Choose a reason for hiding this comment

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

Queuing random/similar tracks from the server is a single HTTP call so I don't think it's really parallelizable. On the other hand, loading the saved play queue currently issues one HTTP call per track, to get updated play count and other metadata, so concurrent calls could likely speed that up significantly

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 see. Thank you.

btw I changed the config name as requested

@stkw0 stkw0 force-pushed the main branch 2 times, most recently from e6f2bab to a4a7f31 Compare September 29, 2024 10:55
@dweymouth dweymouth merged commit 3fd3c40 into dweymouth:main Oct 1, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants