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

[Music]Fix delay of playback on switching song in party mode #15557

Merged
merged 1 commit into from Feb 21, 2019

Conversation

Projects
None yet
6 participants
@DaveTBlake
Copy link
Member

commented Feb 19, 2019

This fixes a regression that got into party mode around Jan 2018, causing it not only to be slow but to block playback. While playing a smart playlist using "play in party mode", then clicking on a different song in the current playlist a user was experiencing delays of ~20s in both GUI and sound.

While analysing that issue I found that there was a much older issue where even though playback started immediately, the GUI was locked until party mode had finished refilling the current playlist to hold 10 songs. Both delays only became noticeable on large music collections and with complex smart playlists, that is those where the rules produced inefficient queries, but that is exactly where party mode is needed most. Neither should be happening.

Party mode is old, and there were some obvious improvements to be made
a) Use fewer reconnections to music DB
b) Use query to return random results rather than shuffle later in memory
c) Use song ID cache (data all fetched previously but discarded) to avoid increasingly slow requeries to skip played songs

This also fixes a long standing issue with party mode playback of "mixed" smart playlists - they didn't work at all, they now do.

Finally it also ensures that if a new song selected while browsing the library during party mode playback then the "queue songs on selection" setting is observed and the selected song queued for play next rather than played immediately.

Discussed on forum
https://forum.kodi.tv/showthread.php?tid=340758
https://forum.kodi.tv/showthread.php?tid=332263 (trac ticket too)

Had some positive user test feedback as well as own testing, but will wait for more.

The heavy processing of party mode now all happens on start up while "filling glasses" dialog showing, and subsequent refilling of playlist when switching song, especially when more than 1 new song is needed (e.g. having clicked on 10th current playlist), is immediate for GUI and playback regardless of music library size or complexity of smart playlist rules.

Show resolved Hide resolved xbmc/PartyModeManager.cpp Outdated

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:PMClickFix branch from 2b1537d to 62e2ffb Feb 19, 2019

Show resolved Hide resolved xbmc/PartyModeManager.cpp Outdated
@pkerling
Copy link
Member

left a comment

Some minor style comments - can't say much about the behavior changes

Show resolved Hide resolved xbmc/PartyModeManager.cpp Outdated
Show resolved Hide resolved xbmc/PartyModeManager.h Outdated
Show resolved Hide resolved xbmc/video/VideoDatabase.cpp
Show resolved Hide resolved xbmc/music/MusicDatabase.cpp
Show resolved Hide resolved xbmc/PartyModeManager.cpp Outdated
Show resolved Hide resolved xbmc/PartyModeManager.cpp Outdated
Show resolved Hide resolved xbmc/PartyModeManager.cpp Outdated
Show resolved Hide resolved xbmc/PartyModeManager.cpp Outdated
@arnova

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@DaveTBlake : Nice work! Finally partymode is getting the love it deserves :) Btw. since you're at it: Cancelling partymode (using the cancel button in the "Mixing drinks" modal dialog) hasn't been working for ages. Any chance you know what's up?

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

Well the 20s freeze got my attention! @arnova I doubt cancel ever worked - "Mixing Drinks" / "Filling Glasses" dialog and party mode process are in the same thread so you never get to move the pointer, let alone nothing in the processing loop allows cancelling.

I think I would like to get this fix in, and look at fixing cancel separately

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:PMClickFix branch from 62e2ffb to b08d716 Feb 20, 2019

@arnova

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Well the 20s freeze got my attention! @arnova I doubt cancel ever worked - "Mixing Drinks" / "Filling Glasses" dialog and party mode process are in the same thread so you never get to move the pointer, let alone nothing in the processing loop allows cancelling.

I think I would like to get this fix in, and look at fixing cancel separately

Indeed: Afaik it never worked in the 12 years I've been working on Xbox-Media-Center/XBMC/Kodi. Your recent changes in this area just triggered me mentioning it. Obviously it's beyond the scope of this PR but it would be nice if we could fix it at some point.

if (!CDatabase::BuildSQL(strSQL, filter, strSQL))
return false;
strSQL += PrepareSQL(" ORDER BY RANDOM()");

This comment has been minimized.

Copy link
@arnova

arnova Feb 20, 2019

Member

This looks confusing as at first it doesn't seem to make sense it "orders by random" by default, but apparently GetSongIDs is only used by PartyMode. Perhaps add a big fat comment why or make random optional by adding an additional argument to GetSongIDS() to make it explicit?

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Feb 20, 2019

Author Member

CMusicDatabase::GetSongIDs fetches a vector of <1, ID> pairs. This result structure and method is only used by party mode and always wants to be randomly sorted. Starting to document what calls what in code comments, something that can easily be searched and just as easy get out of date, does not seem a good idea to me.

This comment has been minimized.

Copy link
@pkerling

pkerling Feb 20, 2019

Member

I concur with @arnova. The method name does not make it obvious at all that you will get randomly sorted results, making it quite confusing. You should either change the method name or add a parameter.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Feb 20, 2019

Author Member

Shall I call it GetRandomSongIdVectorForPartyModeOnly ?

The fact it returns an unusual vector structure makes it pretty obvious that this method is specific in use. The entire CMusicDatabase could do with Doxygen comments. I really don't get you picking on this one method, but hey ho.

This comment has been minimized.

Copy link
@arnova

arnova Feb 20, 2019

Member

I think "GetRandomSongIDs" should be fine ;-)

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Feb 20, 2019

Author Member

OK, I have renamed the partymode GetXXXIDs methods in both db and added Doxygen comments.
I also got so carried away I even removed some old unused methods too that were part of party mode years ago and got left behind. Now I want my cookie ;-)

This comment has been minimized.

Copy link
@pkerling

This comment has been minimized.

Copy link
@arnova

arnova Feb 21, 2019

Member

OK, I have renamed the partymode GetXXXIDs methods in both db and added Doxygen comments.
I also got so carried away I even removed some old unused methods too that were part of party mode years ago and got left behind. Now I want my cookie ;-)

Thanks for the effort. Like I said: this part of the code didn't get any love for a very long time. I'm fine with how it is now so I think we can push this for 18.2?

Btw. have another cookie ;-)

Fix delay of playback on switching song in party mode regression.
Refactor party mode to be faster:
a) on start up  by using fewer reconnections to music db and doing random order in query.
b) on refilling playlist when switching song,  especially when more than 1 new song is needed (e.g. click on song lower in current playlist) by using song ID cache.

Fix party mode use with "mixed" smart playlists.

Obey "queue songs on selection" setting when new song selected during party mode playback and queue song next rather than play it immediately.

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:PMClickFix branch from b08d716 to 141621b Feb 20, 2019

@scott967

This comment has been minimized.

Copy link

commented Feb 20, 2019

I have done testing on Win x64 build for song playlists, musicvideo playlists and mixed playlists, all types launched from both music and videos window. In all test cases party mode was initiated and the appropriate playlist (musicplaylist or videoplaylist based on which media window party mode was initiated from) displayed with items drawn (apparently) randomly from the playlist content. Also monitored infolabel MusicPartyMode.MatchingSongs against Container.NumItems (from the parent playlist loaded in music or videos window) for consistency.

@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

IMO good for merge. Probably got more testing than most other PRs too. @MartijnKaijser ?

@MartijnKaijser MartijnKaijser merged commit fc26066 into xbmc:master Feb 21, 2019

1 check passed

default You're awesome. Have a cookie
Details

@DaveTBlake DaveTBlake deleted the DaveTBlake:PMClickFix branch Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.