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

[Fix]jsonrpc getting consistent song and album artist data - names, IDs and MusicbrainzIDs #8306

Merged
merged 1 commit into from Oct 31, 2015

Conversation

Projects
None yet
9 participants
@DaveTBlake
Copy link
Member

DaveTBlake commented Oct 27, 2015

This fixes the issue that JSON may in some circumstances return inconsistent artist name and ID arrays for song and albums. It also correctly provides Musicbrainz IDs, that would never have been working properly before.

Since adding processing for ARTISTS tag #7486 it is possible that the splitting the artists string may not give the same artists as held in the song_artist or album_artist tables, so could lead to discrepencies. In particular audioLibrary::GetAlbums and GetSongs the array of artists for an album may not match the array of artistIds. This was discussed under #8268

It ensures that when JSON gets albums or songs the underlying query uses song_artist or album_artist tables to fully populate artist credits. Hence artist names and id arrays will be consistent. As a bonus now gets MBID (as a vector to match names and ids)

It bumps JSON version for now artist data returned is consistent.

This is a fix, not an improvement, but does make changes to underlying queries so could be considered major. I have deliberately avoided impacting the main music navigation SQL, but longer term we should be getting artist data from the song_artist table in more places.

Even if it does not make Jarvis alpha since it is JSON related @Montellese I am hoping you can have a look at this before you are away for a month, and at least approve the approach. It needs some more testing with live data, so I am hoping a team member will build it so this can happen.

@Tolriq see I did as I promised. Hope that you can test it with some Yatse data.
@Razzeee and @evilhamster have a look please since it is music.

if (albumartists.empty() && !strArtistDesc.empty())
albumartists = StringUtils::Split(strArtistDesc, g_advancedSettings.m_musicItemSeparator);
// if (albumartists.empty() && !strArtistDesc.empty())
// albumartists = StringUtils::Split(strArtistDesc, g_advancedSettings.m_musicItemSeparator);

This comment has been minimized.

@Razzeee

Razzeee Oct 27, 2015

Member

Is this ment to be commented out? Either you forget to remove it or work on it some more, I guess?

This comment has been minimized.

@DaveTBlake

DaveTBlake Oct 27, 2015

Author Member

I introduced those lines as a temp fix a week ago, as the comment above them says. Here I am trying with them out. I think I have fixed the issue, but I want to get some real data testing going to be absolutely sure. Don't want to make the same mistake twice!

This comment has been minimized.

@Razzeee

Razzeee Oct 27, 2015

Member

Sure. Don't get me wrong, but let's just get rid of them and probably the comments, if it doesn't apply anymore. After all this code is replacing/improving it.

"limit 100 ";
// Get data from album and album_artist tables to fully populate albums
std::string strSQL = "SELECT albumview.*, albumartistview.* FROM albumview "
"LEFT JOIN albumartistview ON albumview.idAlbum = albumartistview.idAlbum "

This comment has been minimized.

@Razzeee

Razzeee Oct 27, 2015

Member

Not sure why this is a left join. I don't think albumartistview can have data that doesn't match (haven't tested, but we should check this). If it has, do we have logic in place to handle with the NULL values that will be thrown our way?

This comment has been minimized.

@DaveTBlake

DaveTBlake Oct 27, 2015

Author Member

Left join because we can have albums without artists, and we still want to see them. It was discussed in another PR, and @Tolriq confirmed that we do have albums and songs without artists sometimes. An abberation but could happen with tags as data source. I don't see absence of artist causing an issue here.

This comment has been minimized.

@Razzeee

Razzeee Oct 27, 2015

Member

Weird, case but then we can't do anything about that. Thanks, for checking.

@@ -349,12 +349,14 @@ class CMusicDatabase : public CDatabase
bool GetCommonNav(const std::string &strBaseDir, const std::string &table, const std::string &labelField, CFileItemList &items, const Filter &filter /* = Filter() */, bool countOnly /* = false */);
bool GetAlbumTypesNav(const std::string &strBaseDir, CFileItemList &items, const Filter &filter = Filter(), bool countOnly = false);
bool GetMusicLabelsNav(const std::string &strBaseDir, CFileItemList &items, const Filter &filter = Filter(), bool countOnly = false);
bool GetAlbumsNav(const std::string& strBaseDir, CFileItemList& items, int idGenre = -1, int idArtist = -1, const Filter &filter = Filter(), const SortDescription &sortDescription = SortDescription(), bool countOnly = false);
bool GetAlbumsNav(const std::string& strBaseDir, CFileItemList& items, int idGenre = -1, int idArtist = -1, const Filter &filter = Filter(), const SortDescription &sortDescription = SortDescription(), bool countOnly = false);

This comment has been minimized.

@Razzeee

Razzeee Oct 27, 2015

Member

Unneeded whitespace change.

if (songartists.empty() && !strArtistDesc.empty())
songartists = StringUtils::Split(strArtistDesc, g_advancedSettings.m_musicItemSeparator);
//if (songartists.empty() && !strArtistDesc.empty())
// songartists = StringUtils::Split(strArtistDesc, g_advancedSettings.m_musicItemSeparator);

This comment has been minimized.

@Razzeee

Razzeee Oct 27, 2015

Member

More commented out code. I'm not really a fan. I know this from work, usually you forget about it and half a year later someone looks at it and has no clue why it's commented out. :/

This comment has been minimized.

@DaveTBlake

DaveTBlake Oct 27, 2015

Author Member

Not something I would normally do, and I would expect to remove before merge. There is just a doubt in my mind that some other part of Kodi is using the description string to make artists. By commenting out it will show up in testing. I really want this built so others can test it, and @Montellese to have some chance to look at it. Not suggesting we merge before that has happened.

This comment has been minimized.

@Razzeee

Razzeee Oct 27, 2015

Member

I'll do tests build asap. (Need read up on how)

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:JSONConsistentArtistData branch from 8f2faa5 to 20bb037 Oct 28, 2015

@kib

This comment has been minimized.

Copy link
Member

kib commented Oct 28, 2015

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

DaveTBlake commented Oct 30, 2015

Thanks for the builds @kib
I have had some testing feedback from @Tolriq. Don't know if @ronie would like to look at this having caught previous issues with GetAlbums.
@Montellese any chance of a comment before you go? Would like to merge this while you are away, so a thumbs up from you (even with reservations if you have not had time to test) would be good.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Oct 30, 2015

will give it a spin in the next few days.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Oct 30, 2015

You don't alter the API definition so nothing that can go wrong there. Otherwise I'm not really into how the music library and that musicbrainz stuff works at all so not much I can review.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Oct 31, 2015

tested this PR and didn't spot any breakage.

i did however (off topic to this PR) notice an issue with the 'artist mood' and 'album mood' field as returned by json. previously it was returned as a list: [mood1, mood2, mood3]
but now it's returned as a string: u'mood1 / mood2 / mood3'

@Tolriq

This comment has been minimized.

Copy link
Contributor

Tolriq commented Oct 31, 2015

According to JSON definition this have not changed and should still be an array.

My code is change proof for those since so many changes in previous versions. Did not notice this one.

No time this week end but maybe

    "instrument": { "$ref": "Array.String" },
      "style": { "$ref": "Array.String" },
      "mood": { "$ref": "Array.String" },
      "yearsactive": { "$ref": "Array.String" },

Should be checked too

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

DaveTBlake commented Oct 31, 2015

@ronie, @Tolriq what has changed in Mood isn't anything to do with this PR, but I will have a dig and see if I can spot what and when. CMusicDatabase::SetPropertiesFromArtist has both Mood and Mood_Array properties.

Could be down to #8012 where SetMood(StringUtils::Join(album.moods, g_advancedSettings.m_musicItemSeparator)) was added to CMusicInfoTag::SetAlbum

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

DaveTBlake commented Oct 31, 2015

I am certain that the changes you are seeing with mood are down to #8012 . It would seem @Montellese inadvertently impacted the interface format.

Just considering albums for a moment, the mood data is in the album table as a single string with "/" separators. It is read into CAlbum split into a vector of strings. Then CMusicDatabase::SetPropertiesFromAlbum stores it in "album_mood" and "album_mood_array" properties (outside the item.MusicInfoTag structure). But CFileItem::SetFromAlbum calls both SetPropertiesFromAlbum and GetMusicInfoTag()->SetAlbum(album). The latter is changed by #8012 to set the mood string CMusicInfoTag.m_strMood, so now we have mood in 2 places in CFileItem. Subsequently CFileItemHandler::HandleFileItem calls FillDetails calls GetField that first tries to get mood from serialised value of CMusicInfoTag ("mood1 / mood2 /...") and then if that is empty looks in property "album_mood_array" ("Mood1", "Mood2" etc.) and finally if that is empty too tries "album_mood" ("mood1 / mood2 /...").

Bottom line is I understand why you are seeing what you are. Question is what, if anything, do we do about it?

@Tolriq

This comment has been minimized.

Copy link
Contributor

Tolriq commented Oct 31, 2015

Well there's need to fix things in one way or another :(

Would be @Montellese choice as I have no big picture vision. But currently if returned value is a String and JSON definition says it's an Array there's an API error and either mood should always return a String and we update the JSON definition or we keep current definition (best IMO as earlier most concatened String returns where switched to Array so I suppose there's no will to return to concatened Strings.) and ensure that the field is always an array by splitting the String if not an Array, or ensuring correct array data is always used / set.

JSON when getting album or song ensure underlying query uses song_art…
…ist or album_artist tables to fully populate artist credits. Hence artist names and id arrays will be consistent. As a bonus now gets MBID (as a vector to match names and ids)

In CMusicInfoTag set "FieldArtist" = GetArtistString so sorts work on strArtists from Album and song tables. Hence not need fix in CAlbum or Csong populating artist vector from artistDesc when artist credits empty.

Rework underlying queries for GetAlbums, GetSongs, GetRecentlyAddedAlbums, GetRecentlyPlayedAlbums, GetRecentlyPlayedAlbumSongs, GetRecentlyAddedAlbumSongs, GetTop100Albums to fully populate artist credits

Bump JSON version for now artist data returned is consistent

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:JSONConsistentArtistData branch from 20bb037 to 3830fa3 Oct 31, 2015

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

DaveTBlake commented Oct 31, 2015

Mood fix is another PR
This PR @Razzeee has checked over the code (@evilahamster seems to be away). @Montellese had no comment to make before going on holiday since it is music and doesn't change API. @ronie and @Tolriq have tested with real data. I think we are good to go :)

Wonder if I can do this myself....

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

DaveTBlake commented Oct 31, 2015

Lack of lower case ends my surge to dominate the universe :)
Learning lots today thanks for bearing with me.

@jjd-uk

This comment has been minimized.

Copy link
Member

jjd-uk commented Oct 31, 2015

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the Jarvis 16.0-alpha4 milestone Oct 31, 2015

DaveTBlake added a commit that referenced this pull request Oct 31, 2015

Merge pull request #8306 from DaveTBlake/JSONConsistentArtistData
[Fix]jsonrpc getting consistent song and album artist data - names, IDs and MusicbrainzIDs

@DaveTBlake DaveTBlake merged commit a4aad14 into xbmc:master Oct 31, 2015

1 check passed

default Merged build finished.
Details
@hudokkow

This comment has been minimized.

Copy link
Member

hudokkow commented Oct 31, 2015

Welcome and congrats on your first merge. 😉

@Tolriq

This comment has been minimized.

Copy link
Contributor

Tolriq commented Oct 31, 2015

@DaveTBlake since I see the PR for bump for Beta 1 is already at the corner let's try to not forget for release :(

@DaveTBlake DaveTBlake deleted the DaveTBlake:JSONConsistentArtistData branch Nov 2, 2015

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.