[JSON-RPC]Audio library Fixes #13051
Conversation
DaveTBlake
added
Type: Fix
|
@SyncedSynapse and @joethefox this is of interest to you, especially the album genre parts Test builds on the mirrors http://mirrors.kodi.tv/test-builds/windows/win64/KodiSetup-20171115-11ea951224-HEAD-x64.exe, or equivalent |
|
|
||
| // Match up artist names and mbids to make new artist credits | ||
| // Mbid values only apply if there are names | ||
| bool updateartists = false; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
DaveTBlake
force-pushed
the
JSONMusicFixes
branch
2 times, most recently
from
ee8acbc
to
acbbd57
Compare
|
LINUX-64-Wayland fail is unrelated to this PR, rest builds fine |
jenkins4kodi
added
the
Rebase needed
There was a problem hiding this comment.
First of all thanks for taking the time to update the JSON-RPC API and trying to get it cleaned up. Much appreciated!
Looks good in general apart from two minors.
Two more things:
- Would it make sense to merge the remaining "genre" and "genreid" into a "genre" proprty of type
Audio.Details.Genreswhile we are at it? - I didn't know about "albumreleasetype" so if it's only for internal use and not even properly populated we should remove it from the API.
| std::vector<std::string> artist; | ||
| CopyStringArray(parameterObject["artist"], artist); | ||
| album.strArtistDesc = StringUtils::Join(artist, g_advancedSettings.m_musicItemSeparator); | ||
| std::vector<std::string> artists, mbids; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| { | ||
| CFileItemPtr item = items[i]; | ||
| if (additionalProperties.find("genreid") != additionalProperties.end()) | ||
| for (int i = 0; i < items.Size(); i++) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@Montellese thanks for finding time to review.
I don't think so, it would unnecessarily slow API data access. The only remaining type with "genreid" and "genre" separately is Audio.Details.Song. Fetching the "genre" data for songs does not take any extra processing time, but fetching "genreid" does, and only some API consumers want genre id values. But perhaps it would be better to replace the "genreid" field in Audio.Details.Song with "songgenres" of type Audio.Details.Genres, so ids always come with their related string as a pair. However keeping the separate "genre" field (for efficient data access) means that
Will do. [See correction below] |
GetAlbums() and GetAlbumDetails(): fix genre handling SetAlbumDetails() and SetSongDetails(): fix genre, artists etc. Generally sort deprecated fields and inconsistencies. To achieve that: - Rework CMusicDatabase::UpdateSong for common processing when add song via scraper (sync with MB) or JSON - Make artist tag processing methods in CAlbum and CSong artist for common processing when scanning music files tags and process artist data from JSON.
DaveTBlake
force-pushed
the
JSONMusicFixes
branch
from
acbbd57
to
c5cead9
Compare
jenkins4kodi
removed
the
Rebase needed
|
On looking closer it seems I was mistaken about "albumreleasetype". It does get populated but by |
There was a problem hiding this comment.
I don't think so, it would unnecessarily slow API data access. The only remaining type with "genreid" and "genre" separately is Audio.Details.Song. Fetching the "genre" data for songs does not take any extra processing time, but fetching "genreid" does, and only some API consumers want genre id values.
Right I didn't know that it required extra database queries.
But perhaps it would be better to replace the "genreid" field in Audio.Details.Song with "songgenres" of type Audio.Details.Genres, so ids always come with their related string as a pair. However keeping the separate "genre" field (for efficient data access) means that GetSongs and GetSongsDetails would return song genre strings in both "genre" and "songgenres" fields. Is that data duplication acceptable?
I'm not a fan of data duplication in an API. Would have been nice to be consistent but if it means slowing responses down considerably that's not an option IMO.
Looks good to me, thanks.
As detailed in xbmc/xbmc#13051, Kodi v18 changes the way album genres are handled, as the genre (by which i mean `genreId`) ceases to be available at the album level, being only available at the songs level. This impacts Kore because `GetAlbums` and `GetAlbumDetails` won't return the `genreId` tag, from which the local AlbumGenres table was populated. This PR changes the source of the local AlbumGenres table to be the `genreId` returned at the song level (by `GetSongs`), to make it somewhat more consistent with the way Kodi will handle things from now on.
As detailed in xbmc/xbmc#13051, Kodi v18 changes the way album genres are handled, as the genre (by which i mean `genreId`) ceases to be available at the album level, being only available at the songs level. This impacts Kore because `GetAlbums` and `GetAlbumDetails` won't return the `genreId` tag, from which the local AlbumGenres table was populated. This PR changes the source of the local AlbumGenres table to be the `genreId` returned at the song level (by `GetSongs`), to make it somewhat more consistent with the way Kodi will handle things from now on.
|
Hi all, I just adapted my app to request the "songgenres" property for "AudioLibrary.GetAlbums". |
Fixes in one breaking change of JSON_RPC to a number of long standing faults in the API access to the music library. Bumping major version to 9 as this is the first such API change for v18.
Since the API is already broken in these places one could say that anything done fixing it is not a breaking change. However the way it is broken is well disguised, and leaving deprecated fields very misleading, this seems a good point to remove them fully and cleanly.
@Montellese could you review this please, I really do want to get music working better via the API.
This PR depends upon #13042 which needs to merge first (and is the first commit here)Changes in more detail:
While the inconsistencies in album genre handling within Kodi have are fixed by [Music]Fix album genre inconsistencies #13042 , that does not change the data exposed by the API. That data that is exposed is still not the matching and consistent data API consumers expect it to be.
GetAlbums()andGetAlbumDetails()both return "genre" (string array) and "genreid" (int array) fields, with the original intention/belief that they match. But "genre" comes fromAlbum.strGenres(and could be anything, although often is song genres) and "genreid" is the distinct (song) genre ids for all the songs on the album (from thegenretable viaalbum_genre) .Hence API changes:
Audio.Details.MediaAudio.Details.SongAudio.Fields.AlbumandAudio.Details.AlbumSo that "genreid" is no longer returned by
GetAlbums()andGetAlbumDetails(), but still returned byGetSongs()where it matches the song genre values returned in "genre". And "songgenres", is returned byGetAlbums()andGetAlbumDetails()with matching song genre strings and id pairs.That approach matches how
GetArtists()andGetArtistDetails()works.For artists "genre" is artist genres, for albums it is album genres and they both have "songgenres" field, for songs "genre" is song genres and songs also have a "genreid" field. This field naming is also consistent with how "artist" is handled: in album "artist" is album artists, in song it is song artists and there is an "albumartist" field. But note that
GetAlbums()filters "genreid" or "genre" are song genre id or string respectively.We want to alert all API consumers to use "songgenres" - a matching pairs of string and id - if they want genre values that enable them to replicate Kodi genre filtering. I think it best to remove "genreid" from fields returned for albums, and have calls with "genreid" fail rather than risk API consumers continue to it in a mistaken way. But also make clear what is in "genre", for albums this continues as before to return album genre strings (that may be anything not necessarily song genres).
This PR fixes the long outstanding bugs in
SetSongDetails()https://trac.kodi.tv/ticket/15247.Since Gotham and the introduction of artist credits and Musicbrainz ids, neither
SetSongDetails()norSetAlbumDetails()was modified sufficiently for them to work fully. In particular updating artists or album artists is broken, see https://forum.kodi.tv/showthread.php?tid=195711.SetSongDetails()also has album fields that should be part ofSetAlbumDetails()instead.Hence API changes:
SetAlbumDetails()SetSongDetails()SetSongDetails()Since we are making a breaking change in this area we can also drop other deprecated fields, particularly not leaving fields that only apply to albums returned blank for songs (see [JSON RPC][Music]Extra artist data with JSON API update #12963) and risking further confusion.
Hence API changes:
Audio.Details.MediaAudio.Details.AlbumsAudio.details.Song,Audio.details.ArtistandList.Item.Baseto be an array. Although for artists it will always be an array of one element, it is always serialized consistently, and correctly as an array.The above consistent definition of "musicbrainzartistid" avoids the problem that exists with "artist". JSON-RPC implicitly needs the "artist" field to always be the same type, but
Audio.Details.Mediadefines it as an array, whileAudio.Details.Artistdefines it as a single string (logical as an artist is only going to have one name, but why is the name file of an artist called "artist" not "name"?). Thus when it is serialized the mediatype is used to determine how to format - string or array.List.Item.BaseextendsAudio.Details.Mediaso expects an array, but when the fileitem list media type is "artists" a single string is returned (allaAudio.Details.Artistdefinition).This means for example that
Files.GetDirectory()on an artists type smartplaylist returns "artist" as a string, breaking the specification given inList.Item.Base.However the use of the "artist" field is so core that I am not confident that making the spec consistent (say by making "artist" always an array), is worth the disruption.
Hence no changes made, but inconsistency noted.
I also think that "albumreleasetype" should be removed fromAudio.Fields.Songs. Releasetype is an internal abum property (with values either "album" or "single") indicating if the album is a real or a fake album created for colating the singles by an artists. It is not populated when songs are quieried from the library, and only set in CMusicInfoTag (hence serialized) when mediatype is album.