Skip to content

Loading…

Music thumbs to cache #1109

Merged
merged 46 commits into from

6 participants

@jmarshallnz
Team Kodi member

This is in no way complete - I'm sure there's areas that will no longer have thumbs after this, as music thumbs were all over the show all using slightly different techniques - I'll continue testing it and adding any fixes on top (to be rebased at the end). Hopefully I hit the majority, but I can't test everything, right? Further, with #1040 being cleaned up to go in, it makes sense to get this in at the same time so that things can be integrated.

Things to discuss are:

  1. This needs a rescan, which currently isn't implemented. The trick is going to be sorting out when the rescan can occur. ATM we update the db on first open, which could potentially occur before the skin is fully up (startup scripts) so this will need a bit of thought. Also, we only want to scan tags, not download album/artist info, so it needs a bit of work on the scanner (basically the scanner should take in a set of flags as to what we want it to do).

  2. This will almost certainly be slower (from the users perspective) at processing thumbs (though it's done off-thread) - in particular, items later in the list will wait on items earlier - this would almost certainly be noticeable in the Songs node which even in modest collections are quite long. There are several potential solutions to this:

    • Having an extra field in the songview/albumviews with the unnormalised art. This would bring it back to how the musicdb thumbs used to operate, though we're now capable of setting more than just the thumb.
    • Changing the bg thumbloader to run from the item during processing, rather than on the list from start to finish (i.e. when processing the UI during the render loop we'd run the thumbloader on the item if it hadn't already been run)
    • We could have a caching layer on the thumbloader that hits ram rather than the db.
  3. There's areas where we use only the pre-cached file thumb rather than the library thumb (upnp for ex) which will need some work - same applies to videos here - figured that @alcoheca would be changing things in this area so didn't spend much time here (plus have no way of testing easily atm).

  4. This deprecates two HTTP-API functions (fetching of album thumb based on album+albumartist, take screenshot).

Comments/criticisms most welcome.

@arnova
Team Kodi member

1) Can't we update when the user opens the Music window/library view?
2) Having the background loader run for each item totally makes sense. This would also improve the overall performance (and feel) for the video views as well, since certain items have to wait now before others eg. finished fetching/caching their thumbs. Also having some sort of caching would probably help a lot although this may cause problems on platforms which don't have an awful lot of ram(?)
3) We already noticed this in ticket #13052 as well ;-)
4-5) No comment.

@jmarshallnz
Team Kodi member

@arnova for 1, we could, but shitloads of stuff hits the thumbs (recently added + lots of scripts for example) before this. The problem is we need to know that an update is required, and the best place we know that is where we update the db. We could save a bool in guisettings.xml I guess, and then reset that bool once the scan is finished. This would also allow for the scan being interrupted by the user - we'd prompt again each time they enter music until the scan is finished. I suspect this is the best solution, so will pop the bools in place for both the music and video db's ready to go for later on when the rescan code is in place.

For 2 it's quite tricky to get this right - The denormalised approach doesn't work well I think as you need to have the artist and album art for songs as well, so denormalising at the song level doesn't buy you much unless you also store the artist and album song at that level as well, which would become a maintenance nightmare I suspect, so that rules out the first option. Having the item loader run per-item when the UI demands it has a lot of benefits, but needs some careful thinking to get it right - in particular, it's more efficient for music items that aren't in the library to run per path rather than per item - you need to do this in order to assign art with any accuracy for example. For library based items it's no problem per item.

@jmarshallnz
Team Kodi member

@theuni the last 2 commits restrict the size of cached images to 720, except for if the user has set to 1080 and there's a 1080p image being cached. On devices with smallers screens, it would make sense to set this to something smaller (eg ATV2 can set to 720). There's some argument to restrict to largest screen res as well perhaps, but I'm not sure we can reliably determine this (user runs xbmc in windowed mode for example, then later hooks up a 2560x1600 monitor)

@arnova 7ed96cc adds some settings so we have something to potentially check in the music window to queue up a scan. Will work on adding the necessary stuff to allow the rescan.

@Montellese, @night199uk, 4d541d9 removes idArtist from CSong/CMusicInfoTag. This means that retrieving fanart for items is a little more complicated (separate routines in CMusicDatabase for them). Further, it may have implications for JSON-RPC as some clients might be using them. An alternate is to define this as the primary artistid and joining the album_artist/song_artist tables in the respective views. Thoughts?

@ghost

i've been through it. no eye-brow-raising stuff found, but it's a lot of code... think we need to do as we did with vids. hit it..

@Montellese
Team Kodi member

Concerning idArtist in CMusicInfoTag et. al.: I added this specifically (on requests from JSON-RPC client developers) because it is the only way for JSON-RPC clients to cross-reference artists, albums and songs short of looping through all the albums and checking if an artists name is part of an album's list of artists. As an example if XBMC starts playing a song it sends an announcement with the songs ID. The JSON-RPC client can then call AudioLibrary.GetSongDetails and will be able to retrieve the albumid and (primary) artistid belonging to this song. Without the artistid, the client has to call AudioLibrary.GetArtists to retrieve a list of all artists and cross-check their names with the list of artists returned for the specific song (which is obviously more difficult to implement and also slower because of the extra request and the cross-checking. So this would break backwards compatibility with JSON-RPC as published in Eden (which is bound to happen in pre-Frodo) but not because there's an easier/better way to get the same thing done.

Is there any benefit in removing it or what is the motive behind it? (I'm guessing it's because there can be multiple artists linked to an album/song).

@night199uk

Can you get the ArtistID back somehow instead of using GetArtistsByName? (dup artists)

@Montellese for JSON-RPC can we replicate the new MusicDatabase.GetArtistsBySong and MusicDatabase.GetArtistsByAlbum calls through to JSON-RPC? So that the JSON-RPC client flow is like this:
AudioLibrary.GetSongDetails
AudioLibrary.GetArtistsBySong (or Album)...
AudioLibrary.GetArtistDetails

Dup artists & multiple artists per song/album is the motivation... Later we'll be able to parse stuff like this:
Mark Knopfler & Chet Atkins feat. Blah
(two primary artists and a featuring artist)

@jmarshallnz
Team Kodi member

@Montellese The alternate is to have a vector of artist id's (or a vector of id,name pairs) for the artists available on request from a song - it's an additional query as @night199uk points out.

Another alternate is to just store the primary artist id and make it clear that there may be more, as this can be fetched without an additional query (it's just a JOIN). The problem with this, is that the primary artist could actually be 2 or more artists (with other artists featuring on select tracks), though this is rare, and currently for fanart and the like it doesn't make much sense to do anything other than the first listed artist anyway, which is what we do.

The first alternate is more useful to the client, but is not something that can be done quickly when retrieving a bunch of songs.

Jonathan Mar... added some commits
Jonathan Marshall [musicdb] add m_type to CMusicInfoTag de75cc2
Jonathan Marshall [upnp art] fanart/thumb images over upnp to use previously cached art eb019ca
Jonathan Marshall [musicdb] add art table to the music database, and get/set art routines da03764
Jonathan Marshall [musicdb] export art from the texturecache (or as XML nodes) when exp…
…orting the music library
2592424
Jonathan Marshall [musicdb] AddSong should take a const CSong and return the id 06e7b4d
Jonathan Marshall [infoscanner] set artist artwork in the db during scan bdfbe52
Jonathan Marshall [thumbloader] we need only a single background thread for the music t…
…humbloader
3e594ad
Jonathan Marshall [thumbloader] retreive art for library music in the thumbloader a46667f
Jonathan Marshall [artist art] adds backcompat for artist art in the thumbloader 3c1178b
Jonathan Marshall [artist art] Musicvideo art is retrieved from the music database 43bdd51
Jonathan Marshall [thumbloader] use thumbloader for infomanager/recently added/json-rpc 0ce8c15
Jonathan Marshall [info dialogs] set thumb/fanart from the db in the song, album, and a…
…rtist dialogs
c089cc8
Jonathan Marshall [thumbloader] set fanart for albums/songs in the music thumbloader 24a5ea0
Jonathan Marshall [embedded art] adds EmbeddedArt members to CSong and CMusicInfoTag, a…
…nd adds art attribute to CAlbum
d21a298
Jonathan Marshall [embedded art] pass EmbeddedArt into IMusicInfoTagLoader::Load 268a0ab
Jonathan Marshall [embedded art] read coverart data in tags into the MusicInfoTag rathe…
…r than caching directly
fd0f626
Jonathan Marshall [embedded art] copy embedded art in underlying media item to cue shee…
…t items
8f9ba90
Jonathan Marshall [infoscanner] add CategoriseAlbums to MusicInfoScanner as an improved…
… version of CheckForVariousArtists
9f08d93
Jonathan Marshall [infoscanner] assign embedded art to the album if a single album is i…
…n the folder
8c4d9fb
Jonathan Marshall [infoscanner] add songs to database by album 7097064
Jonathan Marshall [infoscanner] add song and album art to the database on scan a29d994
Jonathan Marshall [infoscanner] remove old album categorisation and thumb updating func…
…tions from the infoscanner
58a38d8
Jonathan Marshall [imageloader] factor out LoadFromImage 55db267
Jonathan Marshall [imageloader] adds a static wrapper to CBaseTexture::LoadFromFile to …
…save worrying about deletion of the created texture object
cc5ed3c
Jonathan Marshall [imageloader] adds LoadFromFileInMemory to CBaseTexture 6f4ce3e
Jonathan Marshall [imageloader] switch flipped param in CTextureCacheJob to a string to…
… allow more values
06e1d96
Jonathan Marshall [imageloader] adds ability to cache embedded music art fd9df6a
Jonathan Marshall [thumbs] don't fallback to using folder.jpg for songs - it'll be done…
… during scan after reading tags if required.
840fa97
Jonathan Marshall [thumbs] song and album thumbs are retrieved using the thumbloader, s…
…o no need for them to be set when retrieving from the db.
27e3814
Jonathan Marshall [thumbs] use CMusicThumbLoader::FillThumb for setting music thumbs fo…
…r files/folders
852da99
Jonathan Marshall [thumbs] deprecate HTTP-API GetThumbFilename for albums e079b96
Jonathan Marshall [thumbs] remove unnecessary calls to SetMusicThumb from JSON-RPC/Anno…
…unceManager
dad0f19
Jonathan Marshall [thumbs] music share thumbs can use the texture database d63e8ff
Jonathan Marshall [infoloader] don't set the music thumbs in the musicplaylist OnItemLo…
…aded callback (TODO: needs to be done somewhere)
6d99ed5
Jonathan Marshall [infoloader] set album art for file items in a folder when finished l…
…oading tags for that folder
581fe30
Jonathan Marshall [httpapi] depracate TakeScreenshot that has paramaters 0f12c02
Jonathan Marshall [lastfm] no need to cache art - the texture cache will do it 452c254
Jonathan Marshall [cleanup] remove old thumb caching code as no longer used 3ae8891
Jonathan Marshall [dvd thumbs] no need to cache on insert 7e0a095
Jonathan Marshall [cleanup] drop unused functions from musicdatabase 402d5cb
Jonathan Marshall [cleanup] remove unused functions in cximage 4182f70
Jonathan Marshall [cleanup] remove unneeded Picture.h includes 24f1dc3
Jonathan Marshall [song/musicinfotag] remove artist id from CSong and CMusicInfoTag 2e56957
Jonathan Marshall [dbupdate] add settings indicating the db needs an update, so that la…
…ter update code can kick in and rescan as needed
392bf9c
Jonathan Marshall [texturecache] cache images by default at a max of 720p, except for 1…
…080p fanart images which default to 1080p - can be changed by <fanartres> and <imageres> advancedsettings - these replace the previous <fanartheight> and <imagesize> tags.
34f045a
Jonathan Marshall [jpegloader] load based on m_fanartRes/m_imageRes instead of based on…
… screen res
f878229
@jmarshallnz
Team Kodi member

Updated with fixes based on comments - thanks very much.

@night199uk, @Montellese The one thing I'd like resolved before this going in is how to handle artistId for JSON-RPC. If noone objects, I'll add it back into the song and album views for now and we can redo it later if required.

@night199uk
@jmarshallnz
Team Kodi member

@night199uk Go for it - I believe @Montellese is busy with other things. I suspect you'll need to add a vector to CSong/CMusicInfoTag and go from there? It would be nice to supplement the existing vector to vector instead if possible as I doubt there's much point having only the ids available - this would match with the cast list for movies which has both id and name (as well as other information) available.

In the meantime, I've done up a quick fix commit to get the idArtist back in for the meantime in case we find it's more difficult than initially thought - it's currently broken, after all - available in my restore_idArtist branch.

@night199uk
@jmarshallnz
Team Kodi member
@night199uk
@night199uk

yes :-)

Whats your thought on using an enum type instead of a string? makes select{ case : } simpler in core (GUIWindowMusicBase needs this). Can be translated in e.g. JSON-RPC where we want a string?

Team Kodi member

Doesn't particularly worry me either way. JSON-RPC already sets a string based on other criteria, so that doesn't really matter as either way it would need support). The db doesn't require one (it could use an int instead, which would be slightly more efficient to hash perhaps?)

There's a slight advantage in that the type is often also related to the table name, which means slightly less code (eg see GetArtistArtByItem) though that could be done using a LUT instead easily enough.

@Montellese: your thoughts?

@Montellese
Team Kodi member

Concerning JSON-RPC: IMO it's not absolutely necessary to provide the IDs of the artists as long as there's a way to retrieve the artist based on a song/album (which could be done by adding parameters "songid" and "albumid" to AudioLibrary.GetArtists). I don't develop any JSON-RPC client myself so sometimes it's hard for me to say/know what all the clients out there do. Maybe ask joethefox if/how he handles this in the official iOS remote app? Not sure if freezy3k is still working on updating the official android remote to JSON-RPC.

All in all it sounds like this will break backwards compatibility to Eden one way or another. If so we might wanna consider also removing some of the other hacks we put in place to keep backwards compatibility.

Concerning making those types into an enum. I first considered that as well when the video thumb stuff was merged but although it's easier to have enums in the code (because you can use switch statements and there's some compile-time checking) but it also makes it less flexible. Sure you can add a new enum value when you have a new type of media but if we ever move towards a more flexible database layout (which would e.g. allow singles, compilations, soundtracks, concerts etc) those types could actually be defined in the database without having to change the code. But that's far off so just wanted to throw it in ;-)

@jmarshallnz jmarshallnz merged commit f70b7d7 into xbmc:master
@ipitcher

Just tested this. Views aren't being created when using a MySQL back-end.

@jmarshallnz
Team Kodi member

Thanks for the informative message :p

@ipitcher

Hahaha, sorry. I'll try to recreate the conditions and get a debug log.

But, in summary: I had a version 25 music database, built from git after these commits were merged, got many "the table does not exist" messages, looked at the database with mysql command line client, database existed and was populated, views weren't there, dropped the database, restarted XBMC to allow it to create a fresh database, scanned music source, views weren't there, modified advancedsettings to use sqlite, rescanned source to sqlite, looked fine and everything showed up in the gui, modified advancedsettings to use mysql, restarted XBMC, version 27 database was create and populated -- views still not there.

@jmarshallnz
Team Kodi member

Odd, as all that was changed with the views is that strThumb and the thumb table are no longer joined. Are the triggers created (song, artist, album tables have a trigger on delete to clear the art table)?

@ipitcher

Looks like tgrAlbumInfo is the only one there.

@jmarshallnz
Team Kodi member

Ah - I know what it is - silly mysql and it's text indicies needing a length (index on art table). Fixed in ae6508e

@ipitcher

Okay, thanks. That's working well now. One last thing: are Album Artist tags going to be required now? Newly scraped albums without the Album Artist tag are ending up with empty strArtists fields. I suppose I could just add the Album Artist tags, but I thought it was worth asking.

@MartijnKaijser
Team Kodi member

Yes they are required or else they end up without artist. You must also check/uncheck the compilation flag for albums or compilations

@night199uk

ipitcher: do you see the same in yesterdays master? I want to check if that is from the musicdb changes or from these changes.

@ipitcher

Yes, this was from yesterday's master. Sorry, I should have thought about the other musicdb changes before commenting.

I recall strArtists populating correctly without Album Artist tags since that music db change, but I don't remember if it was on a db upgrade or a fresh scrape.

@night199uk
@ipitcher

Sorry, the log is long gone as is the old DB. This is MySQL, and though I'm not certain, I think the same behavior was seen on SQLite.

@night199uk
@ipitcher

I've since copied the Artist tags to the Album Artist tags on all of my mp3s with empty Album Artists, but if I have some time later I can create a small test case and send you a log.

@Montellese

Am I missing something here? This was the only place that filled strThumb which is used further down to fill the ListItem.X.Thumb for recently added album songs. How is this achieved now?

Team Kodi member

Fixed in 7bb063a

@MartijnKaijser
Team Kodi member

Are these folders and subfolders still needed (because they are still generated again at every start-up)?:
Thumbnails/
generated
Music
Video

@jmarshallnz
Team Kodi member

20fcaae removes those @MartijnKaijser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 3, 2012
  1. [musicdb] add m_type to CMusicInfoTag

    Jonathan Marshall committed
  2. [musicdb] export art from the texturecache (or as XML nodes) when exp…

    Jonathan Marshall committed
    …orting the music library
  3. [musicdb] AddSong should take a const CSong and return the id

    Jonathan Marshall committed
  4. [infoscanner] set artist artwork in the db during scan

    Jonathan Marshall committed
  5. [thumbloader] we need only a single background thread for the music t…

    Jonathan Marshall committed
    …humbloader
  6. [thumbloader] retreive art for library music in the thumbloader

    Jonathan Marshall committed
  7. [artist art] adds backcompat for artist art in the thumbloader

    Jonathan Marshall committed
  8. [artist art] Musicvideo art is retrieved from the music database

    Jonathan Marshall committed
  9. [info dialogs] set thumb/fanart from the db in the song, album, and a…

    Jonathan Marshall committed
    …rtist dialogs
  10. [embedded art] adds EmbeddedArt members to CSong and CMusicInfoTag, a…

    Jonathan Marshall committed
    …nd adds art attribute to CAlbum
  11. [embedded art] pass EmbeddedArt into IMusicInfoTagLoader::Load

    Jonathan Marshall committed
  12. [embedded art] read coverart data in tags into the MusicInfoTag rathe…

    Jonathan Marshall committed
    …r than caching directly
  13. [embedded art] copy embedded art in underlying media item to cue shee…

    Jonathan Marshall committed
    …t items
  14. [infoscanner] add CategoriseAlbums to MusicInfoScanner as an improved…

    Jonathan Marshall committed
    … version of CheckForVariousArtists
  15. [infoscanner] assign embedded art to the album if a single album is i…

    Jonathan Marshall committed
    …n the folder
  16. [infoscanner] add songs to database by album

    Jonathan Marshall committed
  17. [infoscanner] add song and album art to the database on scan

    Jonathan Marshall committed
  18. [infoscanner] remove old album categorisation and thumb updating func…

    Jonathan Marshall committed
    …tions from the infoscanner
  19. [imageloader] factor out LoadFromImage

    Jonathan Marshall committed
  20. [imageloader] adds a static wrapper to CBaseTexture::LoadFromFile to …

    Jonathan Marshall committed
    …save worrying about deletion of the created texture object
  21. [imageloader] adds LoadFromFileInMemory to CBaseTexture

    Jonathan Marshall committed
  22. [imageloader] switch flipped param in CTextureCacheJob to a string to…

    Jonathan Marshall committed
    … allow more values
  23. [imageloader] adds ability to cache embedded music art

    Jonathan Marshall committed
  24. [thumbs] don't fallback to using folder.jpg for songs - it'll be done…

    Jonathan Marshall committed
    … during scan after reading tags if required.
  25. [thumbs] song and album thumbs are retrieved using the thumbloader, s…

    Jonathan Marshall committed
    …o no need for them to be set when retrieving from the db.
  26. [thumbs] use CMusicThumbLoader::FillThumb for setting music thumbs fo…

    Jonathan Marshall committed
    …r files/folders
  27. [thumbs] deprecate HTTP-API GetThumbFilename for albums

    Jonathan Marshall committed
  28. [thumbs] remove unnecessary calls to SetMusicThumb from JSON-RPC/Anno…

    Jonathan Marshall committed
    …unceManager
  29. [thumbs] music share thumbs can use the texture database

    Jonathan Marshall committed
  30. [infoloader] don't set the music thumbs in the musicplaylist OnItemLo…

    Jonathan Marshall committed
    …aded callback (TODO: needs to be done somewhere)
  31. [infoloader] set album art for file items in a folder when finished l…

    Jonathan Marshall committed
    …oading tags for that folder
  32. [httpapi] depracate TakeScreenshot that has paramaters

    Jonathan Marshall committed
  33. [lastfm] no need to cache art - the texture cache will do it

    Jonathan Marshall committed
  34. [cleanup] remove old thumb caching code as no longer used

    Jonathan Marshall committed
  35. [dvd thumbs] no need to cache on insert

    Jonathan Marshall committed
  36. [cleanup] drop unused functions from musicdatabase

    Jonathan Marshall committed
  37. [cleanup] remove unused functions in cximage

    Jonathan Marshall committed
  38. [cleanup] remove unneeded Picture.h includes

    Jonathan Marshall committed
  39. [dbupdate] add settings indicating the db needs an update, so that la…

    Jonathan Marshall committed
    …ter update code can kick in and rescan as needed
  40. [texturecache] cache images by default at a max of 720p, except for 1…

    Jonathan Marshall committed
    …080p fanart images which default to 1080p - can be changed by <fanartres> and <imageres> advancedsettings - these replace the previous <fanartheight> and <imagesize> tags.
  41. [jpegloader] load based on m_fanartRes/m_imageRes instead of based on…

    Jonathan Marshall committed
    … screen res
Something went wrong with that request. Please try again.