[music] Change the way the (album)artists tag are handled #7486

evilhamster · 2015-07-11T20:05:22Z

In #7281 I added some basic support for the artists tag to sort the problems reported in http://forum.kodi.tv/showthread.php?tid=198149

There where some comments that it was preferred if information in the artist tag could be retained for display purposes and the contents of the artists tag could be used when adding the artist to the database. This commit tries to solve this and from my testing it seems to be working fine, but more testing is required.

There is a problem with albums where there are artists credited for the album but are not credited on any song (like https://musicbrainz.org/release/5a216f06-5ed1-4ad4-a876-c1d5145ea6c8). They will be added with the mbrainz id to the database. I have added two things that should mitigate this problem, support for albumartists (not a standard in picard yet but will hopefully be) and changed the addArtist function so that if the artist has a good mbrainzid / name match from another release it will update the name.

xbmc/music/Album.cpp

-      if (!artist.empty())
-        artistName = (i < artist.size()) ? artist[i] : artist[0];
+
+      if ( i < musicBrainAlbumArtistHints.size() )


zag2me · 2015-07-14T11:22:32Z

Will need a test build to run it through my collection

mkortstiege · 2015-07-14T11:29:20Z

win32 test build is building and should be on the mirrors within the next hour.

zag2me · 2015-07-14T12:49:40Z

Thanks the test build link is here

http://mirrors.kodi.tv/test-builds/win32/KodiSetup-20150712-439b01e-addartistdescfield.exe

@steve1977 @scott967 @GitrianDK can you test?

steve1977 · 2015-07-17T13:36:01Z

Happy to help testing, but would need your help with an OSX built (no Windows machine running at the same). Any chance?

MartijnKaijser · 2015-07-19T10:28:32Z

builds can be found on http://mirrors.kodi.tv/test-builds/ with ff6de6c in the filename

scott967 · 2015-07-22T05:41:50Z

Did some preliminary test on Kodi 16 150721 nightly. Had some problem with ID3v2.3 tags, but they are problematic anyway. Retagged to v2.4 and it seemed to work as intended viz:
Song with "artists" tag and matching artist MBID tag loaded each artist in artists into both song_artist and artist table with MBID. Song with "artists" tag and no artist MBID tag loaded each artist in artists into both song_artist and artist tables without MBID. Song with no artists tag no artist MBID loaded the TPE1 performer into both song-Artist and artist tables without MBID.

As expected, the song_artist "credit" as defined by MB for TPE1 is lost when TXXX "artists" tag is used.

This was go-path testing. Need to do some negative path testing next, also will look at APE tags and FLAC(Xiph) tags.

steve1977 · 2015-08-04T13:33:27Z

This is really great. The only missing piece now is to have Picard (or Kodi?) fix incompatibility of artists tag in MP4/M4A files? http://tickets.musicbrainz.org/browse/PICARD-376

evilhamster · 2015-08-14T18:27:55Z

@scott967
Do you have a copy of a problematic file, then I can take a look at it and see if I can figure it out?

Have you been able to test any build with this patch applied (I don't think that daily had it).

@steve1977
The next logical step would be to start looking into the scraper and make sure they are also able to retain the artist credits and not use artist1 / artist2 (if that is the case haven't been using prefer online information for a long time).

Has anyone else had time to test this patch? Any remarks of comments?

Paxxi · 2015-08-14T23:11:35Z

No clue about functionality but code looks good @evilhamster

DaveTBlake · 2015-08-24T10:26:32Z

Should m_musicBrainzArtistHints and m_musicBrainzAlbumArtistHints be added to CMusicInfoTag::Serialize? I am not saying they should, but just think it may need checking.

If I have understood correctly the presence of MBIDs and ARTISTS, the value of the ARTIST tag becomes a song artist description (stored in strArtists field of Song table) that is displayed on play etc. That seems fine to me in principle, but I note that strArtists is not included in searches and that could be a disadvantage when ARTISTS and ARTIST tags contain different variants of artists names e.g. abbreviation "SNO" rather than "Scottish National Orchestra". Users could rightly expect to be able to find the text they see when a song is played regardless of how Musicbrainz has correctly named the artists.

evilhamster · 2015-08-26T21:47:45Z

@Paxxi
Thanks for checking the code (after you commented I removed some bools and checks that I added #7281 that are of no use after this commit)

@DaveTBlake
I don't think the Hints variables should be added to the Serialize function, their only use is when trying to match mbrainzid <> artistname when scanning a new item. There could be an argument made to change value["displayartist"](in CMusicInfoTag::Serialize) to the desc item if it exists (and add one for albumartist). But for that to make sense you should probably update CSong CMusicDatabase::GetSongFromDataset so that the desc values get set there as well, I think that should be safe todo but not sure. Any ideas? (thanks for getting me to check serialize btw!)

Good point about searching, that could be done in two ways:
a) Add a alias table that keeps track of an artists alias:es.
b) Update SearchSongs to something like: select * from songview where strTitle like '%%%s%%' OR strArtists like '%%%s%%';

I would prefer option a, but that should probably be done in a separate pr since that could grow in size.

What do you all think?

DaveTBlake · 2015-08-27T12:04:42Z

I'm eager to get this patch merged so that I can fork from it with other music library changes (re: classical music), so would not want to complicate it further and cause delay.

I am also still finding my way around the code so not sure how much my opinion counts, but I am an avid music library user here goes!

Support for artist alias names would be great, as would sort name so we could have surname order for individuals rather than first name, but beyond the scope of this PR really. Searching in strArtists could be a quicker fix.

More generally I wonder if the use of original strings and names + separators in the music library could do with some rationalisation. For example where does the strJoinPhrase and BoolFeatured in the song_artist and album_artist tables get used? If we have the original string and the separate names why does some output reconstruct the string, and elsewhere split the names again (using global setting separator)?

CMusicDatabase::GetSongFromDataset should set strArtistDesc shouldn't it, it is part of CSong and is held in the DB?

evilhamster · 2015-08-30T15:05:45Z

Commited some updates:

When updating an album or song, use the desc field if is not empty.
When getting an album or song from a dataset, set the desc fields.
Reads the ----:com.apple.iTunes:ARTISTS, ----:com.apple.iTunes:ALBUMARTISTS flags from M4A/MP4 files, from my testing it works fine and kodi is able to read picard tags (only tested on linux).
Updated CMusicInfoTag::Serialize to use the desc values if they exists. Added displayalbumartist, but not change anything in json-rpc so it will currently not be used (don't know enough about the json interface to start doing changes there). But it felt we should have a variable like since m_albumArtist is a vector.
Did not update CSong::Serialize since it didn't contain any display variable for artist.

No updates in regards to the search part since I'm worried that doing the SearchSongs variant posted above, that will cause to many hits.

Let me know if you have any feedback!

I have added the last commit as a separate commit to make it easier to see the changes. If approved for merging, let me know and I'll squash it!

@DaveTBlake
Don't know, I have wondered the same things.

evilhamster · 2015-09-03T03:18:44Z

Fixed conflicts that prevented merging, squashed the commits into one (looked over them and it didn't make any sense to keep them separated).

Anything I can do to speedup the possible merger of this PR? If possible I would like kodi16 to include this one, since it adds asked about features that where missing from #7281

mkortstiege · 2015-09-03T07:57:44Z

Could you please try to factor out:

std::string artistDesc;
if ( ! album.strArtistDesc.empty() )
  artistDesc = album.strArtistDesc;
else
  artistDesc = GetArtistString(album.artistCredits);

Since it's called multiple times now it makes no sense to keep that dupe code block all around.

xbmc/music/MusicDatabase.cpp

@@ -1178,7 +1201,16 @@ int CMusicDatabase::AddArtist(const std::string& strArtist, const std::string& s
      if (m_pDS->num_rows() > 0)
      {
        int idArtist = (int)m_pDS->fv("idArtist").get_asInt();
+        bool update = false;


steve1977 · 2015-09-03T08:23:26Z

Thanks evilhamster and Dave for driving this forward. This is truly exciting.

I finally got around testing (using Kodi 16 Alpha 2 on Windows):

MP3: nicely using artists tag now splitting artist 1 / artist 2. Would be great still to use artist like a display artist not to lose the "feat." information.
M4A: unfortunately, this is not working for me yet, but should be related to a Picard bug? (http://tickets.musicbrainz.org/browse/PICARD-376). When scanning M4A files, Kodi now reverts back to using artist tag and does not use artists tag. Reading to what evilhamster writes this may be fixed, but I cannot test yet as not in Alpha2? Could I test with latest nightly?
Both MP3 and M4A: I have some files where I manually changed the albumartist (but kept the MBIDs as is). For these cases, the old issue still remains. I am ok with the work-around just to have Picard remove/unset all MBIDs, but still want to report the issue.

Has albumartists (with "s") already been implemented? If so, let me also test. Currently don't have any albumartists (with "s") tags, so need to manually tag.

Thanks again @evilhamster and @DaveTBlake. This is really getting nice and fantastic to see attention on the music library.

DaveTBlake · 2015-09-03T11:39:22Z

@steve1977
For 3). Since you have manually changed the albumartist, why not manually add ALBUMARTISTS (note "s") tags that match artists in the MBIDs as a better workaround. Not sure about Picard but you can add tags in MP3tag. Just a thought, then your tagging job will be done.

steve1977 · 2015-09-03T11:53:07Z

Thanks. Would this solve my issue? This would indeed be a much better work-around. I can currently not try as the fix for "artists" / "albumartists" (both with "s") for M4A files is not merged yet. But I am happy to test whether this works once this it's in. I have also added to the forum thread for wider visibility of the community (http://forum.kodi.tv/showthread.php?tid=235259&page=2). Thanks again Dave!!!

evilhamster · 2015-09-03T13:27:54Z

@mkortstiege
Thanks for the review, I have updated the commit to reflect your comments. Moved the duplicate code into the GetArtistString, since it wasn't used in any other place in the code. I found some other place in the code where i had written "if ( expr )" instead of "if (expr)" and have updated them as well.

@steve1977

added in this pr.
added in this pr.
Albumartists / Album artists in this PR

Since this pr has not been accepted and merged, it is not included in the nightly's and the only way to test it is to manually compile it.

I think it would be great if we could keep discussions that are not strictly related to this PR on the forum instead of github, to keep the numbers of emails down for the kodi devs.

mkortstiege · 2015-09-03T15:20:54Z

New test build: http://mirrors.kodi.tv/test-builds/win32/KodiSetup-20150903-8da40a8-addartistdescfield.exe

@Paxxi, @Montellese please have a look as well if you get a chance so we can get this in within this merge window.

…merged)

xbmc/music/MusicDatabase.cpp

@@ -369,8 +369,10 @@ int CMusicDatabase::AddAlbumInfoSong(int idAlbum, const CSong& song)
  }
 }

-std::string GetArtistString(const VECARTISTCREDITS &credits)
+std::string GetArtistString(const std::string desc, const VECARTISTCREDITS &credits)


Paxxi · 2015-09-05T12:22:46Z

No clue about functionality, code looks good. No objections to merging this asap

…field is used to keep the information from the tags for display purposes.

evilhamster · 2015-09-07T15:17:13Z

@Paxxi
Missed that, thanks for noticing and I have updated the pr!

[music] Change the way the (album)artists tag are handled

razzeee reviewed Jul 12, 2015
View reviewed changes

xbmc/music/Album.cpp

if (!artist.empty())

artistName = (i < artist.size()) ? artist[i] : artist[0];

if ( i < musicBrainAlbumArtistHints.size() )

This comment was marked as spam.

Sign in to view

evilhamster force-pushed the addartistdescfield branch from 6aa52cc to 439b01e Compare July 12, 2015 20:36

evilhamster force-pushed the addartistdescfield branch from 439b01e to d856ed7 Compare August 15, 2015 09:27

evilhamster force-pushed the addartistdescfield branch from 2e39ff6 to 5aa4bc8 Compare September 3, 2015 02:55

mkortstiege reviewed Sep 3, 2015
View reviewed changes

xbmc/music/MusicDatabase.cpp

@@ -1178,7 +1201,16 @@ int CMusicDatabase::AddArtist(const std::string& strArtist, const std::string& s

if (m_pDS->num_rows() > 0)

{

int idArtist = (int)m_pDS->fv("idArtist").get_asInt();

bool update = false;

This comment was marked as spam.

Sign in to view

evilhamster force-pushed the addartistdescfield branch from 5aa4bc8 to 8da40a8 Compare September 3, 2015 13:05

DaveTBlake added a commit to DaveTBlake/xbmc that referenced this pull request Sep 5, 2015

Change the way the (album)artists tag are handled (xbmc#7486 not yet …

c35fe26

…merged)

Paxxi reviewed Sep 5, 2015
View reviewed changes

[music] Added a desc field for (album)artists to Song and Album, the …

c37a02b

…field is used to keep the information from the tags for display purposes.

evilhamster force-pushed the addartistdescfield branch from 8da40a8 to c37a02b Compare September 7, 2015 15:15

Paxxi added a commit that referenced this pull request Sep 7, 2015

Merge pull request #7486 from evilhamster/addartistdescfield

6adfe05

[music] Change the way the (album)artists tag are handled

Paxxi merged commit 6adfe05 into xbmc:master Sep 7, 2015

MartijnKaijser modified the milestone: Jarvis 16.0-alpha3 Sep 8, 2015

evilhamster mentioned this pull request Sep 11, 2015

[MusicDB] Add artist roles to handle Composer, Conductor, DJMixer etc. Tags #8015

Merged

DaveTBlake mentioned this pull request Oct 10, 2015

[Music]Fix More artists than MBID in song and album #8191

Closed

DaveTBlake mentioned this pull request Oct 27, 2015

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

Merged

DaveTBlake mentioned this pull request Nov 5, 2015

Fix missing artist names in legacy Python music interface #8354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[music] Change the way the (album)artists tag are handled #7486

[music] Change the way the (album)artists tag are handled #7486

evilhamster commented Jul 11, 2015

This comment was marked as spam.

zag2me commented Jul 14, 2015

mkortstiege commented Jul 14, 2015

zag2me commented Jul 14, 2015

steve1977 commented Jul 17, 2015

MartijnKaijser commented Jul 19, 2015

scott967 commented Jul 22, 2015

steve1977 commented Aug 4, 2015

evilhamster commented Aug 14, 2015

Paxxi commented Aug 14, 2015

DaveTBlake commented Aug 24, 2015

evilhamster commented Aug 26, 2015

DaveTBlake commented Aug 27, 2015

evilhamster commented Aug 30, 2015

evilhamster commented Sep 3, 2015

mkortstiege commented Sep 3, 2015

This comment was marked as spam.

steve1977 commented Sep 3, 2015

DaveTBlake commented Sep 3, 2015

steve1977 commented Sep 3, 2015

evilhamster commented Sep 3, 2015

mkortstiege commented Sep 3, 2015

This comment was marked as spam.

Paxxi commented Sep 5, 2015

evilhamster commented Sep 7, 2015

[music] Change the way the (album)artists tag are handled #7486

[music] Change the way the (album)artists tag are handled #7486

Conversation

evilhamster commented Jul 11, 2015

This comment was marked as spam.

zag2me commented Jul 14, 2015

mkortstiege commented Jul 14, 2015

zag2me commented Jul 14, 2015

steve1977 commented Jul 17, 2015

MartijnKaijser commented Jul 19, 2015

scott967 commented Jul 22, 2015

steve1977 commented Aug 4, 2015

evilhamster commented Aug 14, 2015

Paxxi commented Aug 14, 2015

DaveTBlake commented Aug 24, 2015

evilhamster commented Aug 26, 2015

DaveTBlake commented Aug 27, 2015

evilhamster commented Aug 30, 2015

evilhamster commented Sep 3, 2015

mkortstiege commented Sep 3, 2015

This comment was marked as spam.

steve1977 commented Sep 3, 2015

DaveTBlake commented Sep 3, 2015

steve1977 commented Sep 3, 2015

evilhamster commented Sep 3, 2015

mkortstiege commented Sep 3, 2015

This comment was marked as spam.

Paxxi commented Sep 5, 2015

evilhamster commented Sep 7, 2015