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

[DaveTBlake]

First step in improving browsing for classical music is to process the standard tags for composer (TCOM), conductor (TPE3) and Ensemble. This could be considered a work in progress, but could be merged as an initial phase providing limited user improvements.

Unlike popular music albums which usually have one album artist, classical music frequently has multiple artists - soloists, conductor, orchestra and composer. The composer is the primary element used when selecting what you want to play, and although not a performer most classical music files will have composer name in the artist (or album artist) tag just to make them accessible on the various players (inc. Kodi). Musicbrainz also treats composer as an artist.

Once the artist list includes a mix of composers, orchestras, conductors, soloists (as well as pop artists) it is useful to be able to see just one kind of artist at a time. Lack of player support for these tags means that users will have incorporated them in the artist and album artists tags. Rather than have extra tables or song attributes for these tags, my idea is to use these tags to identify the "role" that an artist has in the recording. An approach that could easily be extened to other as yet unsupported tags, but wanting to try the concept first. This means a database change adding "role" as a attribute of the song_artist and album_artist tables.

The UI aspect of this change is made available through folder type custom nodes. For example

<?xml version='1.0' encoding='UTF-8'?>
<node order="1" type="folder">
    <label>Composers</label>
    <path>musicdb://genres/-1/?roleid=3</path>
</node>

Implementing this change I have looked at the inital work on artist credits discussion between Night199UK and JMarshall. The original intentions to use JoinPhrase to construct artist names and get rid of the various strArtist and use of strutils::Join everywhere. This did not happen, and despite being used to store " / " JoinPhrase is not used, neither is bFeatured. I have therefore removed these attributes (replacing them with role). I really have checked carefully before doing that.

Not sure how much more of the design I should describe at this point, or if there is somewhere better for design documentation? I am hoping we can move the music library forward despite the lack of experienced dev team members even using the music side let along having time to consider design changes.

From other PR it seems that @Montellese, @evilhamster, @razzeee, and @zag2me are most likey to have an interest in music library/tags. Lots to discuss I am sure!!

[zag2me]

Excellent, thanks for getting stuck into this @DaveTBlake.

The design and concept is good, and I like the way you are thinking about the different cases for tagging. The new "Role" DB field is an great idea and could work with electronic artists as well. I think in general we should follow how musicbrainz classifies artists.

As always, can't comment too much on the code but the change in general looks good.

One small thing is that maybe its a good idea to add new tags in separate commits.

[evilhamster]

You should remove the two commits "Change the way the (album)artists tag are handled (#7486 not yet merged" and "Various artists" album artist in the artists list (#7938 not yet merged)" from the PR since one of them have already been merged and the other one is handled in an other PR.

For readability it might be a good idea to split into several commits (maybe one for removing strArtist /JoinPhrase and one for adding roles)?

As we discussed over PM's, I would prefer if the role are not defined by enum's but rather as strings (the idRole could reference a table with the different roles description). There are some use cases where this would be nneded; TMCL tags would be one, being able to add relationships from musicbrainz would be an other.

Haven't been able to read all the code yet, but started with the tagloader lib parts and it looks like you only load the first entry of a tag even though they should be able to contain a list of items.

Also it looks like you are changing the upgrade steps for db version 54, isn't it a better idea to bump db version to 55 so you can separate your changes (and make sure they execute for the users that are on 54)?

Will try to check more later.

[jjd-uk]

@evilhamster is your provided forum email address up to date, something was sent to you on behalf on the Team and just wanted to check if you got it.

[DaveTBlake]

Thanks for checking this out. Will make code corrections as suggested, keep them comming!

@evilhamster
As for the first 2 commits covered by other PR, I just don't know how else to handle other changes to the same piece of code. As it is this will have to be manually merged anyway, and other music library changes are bound to overtake it too (pulled, built and merged while we are still considering this). I don't want to remove those commits because I need the code to be like they leave it.

I understand that smaller commits are easier to review, but again I don't see how to get there from here. Adding roles without removing JoinPhrase I would have had to add processing for JoinPhrase, removeal made the most sense. This is as small a commit I could create and still have a working result that did something.

Again database version changed like quick sand under my feet!! Did my best to keep aligned, probably still made an inconsistent result. Github novice struggling to get it right!

[Montellese]

Having commits from other open PRs is ok if the changed depend on them. Once they have been merged the branch can be rebased and the commits will (more or less) automatically disappear. We just need to remember that this can only be merged after the other PRs.

[DaveTBlake]

Thanks @Montellese for clarifying. I will leave the first 2 commits as a clear reminder of the merge dependency.

As this work becomes increasing out of date with other smaller music changes being merged in the meantime (it feels like a continuous stream at the moment), is there anything I can do that will help with the review process and the (hopefully) subsequent merge? For example if a give my branch a new DB version, as Evilhamster and Razzee point out, do I also bring it inline with the current master, or do I pretend I don't know there are other changes? Advice appreciated.

[razzeee]

@DaveTBlake
You will need to rebase this so that it is mergeable again.
Are you using any tool for git or git itself? If I may ask?

That will also pull in this work: #8012 you should then have a look at the code again, as your code should be in the same format.

[DaveTBlake]

@razzeee
I have installed TortoiseGit, but might be better just using command line. Handholding always appreciated :)

At what point do I rebase? Music changes seem to be comming thick and fast at the moment, so do I do it now or wait for more comments (and commits) I know that you and evilhamster have only just started looking at this.

I spotted #8012, but sure other music work could get past me.

[razzeee]

You might want to backup your folder the first few times you do this.
Rebases will probably be easier if you do them as often as possible and with as few commits as possible.

Just choose 'Rebase...' from the context menu on your folder. In that dialog, be sure that you are on this branch. If you don't have the official kodi repo as remote configured hit '...' and add a new remote with the url https://github.com/xbmc/xbmc.git, be sure to fetch it. Then you can select whatever-you-named-it/master as Upstream.
You should now see your three commits, we're basically getting a fresh master and adding your three commit ontop of it. Hit 'Start rebase', if you hit conflicts, resolve them and tell tortoise to move on (same button position as start) and if your done, check the log and try to compile. Just so we're sure everything is fine.
If you have done that and everything is fine, just push and select the checkbox for 'unknown changes'. Double check your remote, it should be your remote NOT the kodi remote. (you shouldn't have rights but it's always good to know where your pushing to)

[DaveTBlake]

@evilhamster

... the tagloader lib parts and it looks like you only load the first entry of a tag even though they should be able to contain a list of items.

A song will definitely only have one conductor, any extra tag is a mistake and reasonable to ignore. I was going to say that a song only has one composer, but now I think about it whilst this is true for many classical works it is not globally true. Lenon & Macartney spring to mind (not that popular music listeners usually bother with composer, they might start), or Gilbert & Sulliven for opereta, so you have a point. But is TCMP multivalue?? Have to check, FlAC/Vorbis/Xiph will cope.
I guess ensemble could be multiple, if you lump orchestra and choir together for say Beethoven's 9th, have to find some examples.

It could make to a long track description to display during play, but good catch.

[evilhamster]

@jjd-uk
Yupp, I got it but somehow I manage to miss to send a reply, thanks for reminding me!

@DaveTBlake
I think they are rare, but there are cases where performances have used more than one conductor (the one I read about was https://en.wikipedia.org/wiki/Gruppen, but there are probably more).

One thing I have been thinking about, even though TPE1/TPE2 can be multi-value it's mostly used as a single string that reflects what is written on the album, do you think it could be the same problem with these tags?

tcmp is itunes compilation thingy, so I hope that one isn't multivalue :)

In std::string GetArtistString, could there be a problem with duplicate names in artistString if credits contain the same artist both as albumartist and artist for a song?

Btw, I have been thinking some more about using one additional table versus using enum definitions and I'm not completely convinced but and I'm starting to think you made the correct call in this pr (so you can disregard my comment about it in my first comment).

But it's great work, let me know if you need help with the rebasing of it

[DaveTBlake]

Rebased so now upto date with master (thanks guys for the hand holding). A build could help with getting some others testing with appropriately tagged classical music. How does that happen?

@evilhamster
I am thinking that starting simple with one composer, conductor and ensemble could be the way to go. It is a huge leap forward,and handling all multi-line possibilities could just complicate things at this point for little return.

One thing I have been thinking about, even though TPE1/TPE2 can be multi-value it's mostly used as a single string that reflects what is written on the album, do you think it could be the same problem with these tags?

Yes, almost certainly. It is down to the tagging software too. FLAC/Vorbis/Xiph comments for ARTIST can be multiple but usually you get one with separators for the many names.

TCMP was a typo, I meant TCOM of course!

In std::string GetArtistString, could there be a problem with duplicate names in artistString if credits contain the same artist both as albumartist and artist for a song?

I don't think so, different tables are involved (in that sense the separate roles are unnecessary, but I feel they help with debugging if nothing else).

However I do wonder about how well Kodi keeps the 3 ways it holds artist names in line - string, vector and artist credit objects - even without this PR. The string (and denormalisation when stored in database) does make sense as a display value, the artist credits object makes sense too. Then there is the vector, which is historic and gets passed about lots. I do wonder if there is an arguement for rationalisation or at least replacing the vector with a method.

[zag2me]

Windows test build
http://mirrors.kodi.tv/test-builds/win32/KodiSetup-20150916-0fe4fef-ArtistRoles.exe

[razzeee]

Whats the status here? Has there been any testing? Any more work you want to do?
This also needs a rebase.

[DaveTBlake]

@razzeee it has received some testing and been OK, but probably needs a second team member to sign off on it. Happy to rebase, but I feel it would be better to get #8158 done first so my focus is on that, and that could provoke some modifications.

Meanwhile there is a build that can be used for testing further, just need some classical music users to engage with suitably tagged music (otherwise you don't see any difference).

[DaveTBlake]

Have rebased just to show proof of life!! I would like to rebase again to include #8158 (as yet unmerged) so will probably keep that on my repo to avoid confusion. Either that or wait... :(

[razzeee]

Can you rebase and squash it to one commit?

[DaveTBlake]

Tried to rebase again, hard to keep up! Manually merging was painful and I ended up creating a new branch on my repo (with an uptodate master). Working through manually reapplying changes led me to modify the design a little for the better, so worthwhile. The new, mergeable, version of this change is in a new branch on my repo https://github.com/DaveTBlake/xbmc/tree/ClassicalContributors, just not sure how to push it here?

[razzeee]

Make a backup.
Switch to this branch.
Hard reset it to the last version without any of your changes.
Rebase against your new branch. (you could also Cherrypick the commits)

Might work, haven't tried obviously. Using any git gui or just command shell?

[DaveTBlake]

jenkins build this please

[DaveTBlake]

@Tolriq thanks for your helpful comments about JSON API. I have added GetContributorRoles method and role as a filter parameter to GetSongs. I agree that the role criteria use needs description, but not sure where to put it?

When it comes AudioLibrary.GetArtists returning denormalised data, such as flagging the contributor only artists, or even the roles that artist plays, in the same way as you want it to return a compilationartist flag, that seems are far bigger issue than the scope of this PR. Currently the "compilationartist" field is always false, fixing that and returning roles too is something for another PR.

[DaveTBlake]

@Montellese your overview of the JSON additions would be appreciated.

[Montellese]

JSON-RPC changes look ok apart from the minor. You're also missing the JSON-RPC API bump.

[DaveTBlake]

Thanks @Montellese will fix enum order (and those added by #8405 in the middle too). Must have lost the API bump in the rebase, I remember doing it.

[Tolriq]

Just so that there's a public record about it :)

I do not agree on usability of current JSON API exposition of this, to be really useful for remote makers :)

[DaveTBlake]

Have fixed enum order, bumped JSON. Also added a provisional SongInfoDialog (thanks @HitcherUK ) and node files that make the additional filtered artist listings and roles data visible.

Can we merge with this provisonal Dialog? Functionallity it has the thumbs up and is good to go, just a question of the UI aspects. @razzeee haven't heard from you in a while.

I will get a test build up on the mirrors if anyone would like to try it and see what I am talking about.
Here http://mirrors.kodi.tv/test-builds/win32/KodiSetup-20160116-3a80bb8-HEAD.exe

[DaveTBlake]

A little bit more of a twiddle with the song info dialog (thanks @HitcherUK )
jenkins build this please

[Tolriq]

So as I feared this was pushed without @DaveTBlake reporting any of my concerns so that @Montellese have the correct information to validate ....

Why the changes to GetArtists is wrong :

1. It adds new filter role and roleid on data that is not part of artists and can not be get by anyone. This is as an API definition quite wrong. (And the fact that there's a wrong precedence does not validate the fact that new bad things should be added !)

2. It adds an hidden undocumented filter on roleid = 1. So now the GetArtists do not return all Artists from the table but only some based on a filter that no one is aware of.

3. To get all artists you need to filter on roleid = -1, first it's not documented, but more wrong : You need to use an undocumented internal magic number to do a filter on a public API. Completely insane.
   (And not talking about the fact that doing that filter returns data that you can't really use since you can't apply the filter manually since you do not have access to roledid and role.)

About naming : There's some major incoherence too, it seems it's all over contributor roles, but then it's Library.Fields.Role so not related to audio or contributors and not really clear as it can have many meanings.

"Audio.Contributors" do not include or return roleid only the role string, also not cool for any proper usage of the API. Come on it means API consumers can not recreate a proper joint and need to use
join on Strings, really on strings ? (Specially when due to current api limitation this joint table is mandatory to recreate correct data for offline use ?)

This is IMO a lot of things that should have been at least discussed with the proper people, and not hidden.

Really disappointed that JSON like that was merged, this open doors to anything in future and is not toward a better base, unlike what some others guys from the team tries to achieve !

[Tolriq]

Another funny thing : The GetSong part , you can filter on role and roleid (but again roleid not returned in contributor filed).

So basically you can query all songs that have a composer or a lyricist.

If you use 2 filter + "and" you'll have all songs where toto is an artist and there's a composer (but composer could be anyone).

@DaveTBlake when you ask for advice to someone that have the knowledge and despite more than 20 PM / Mails you decide to ignore all the messages about your changes being bad and / or useless you should at least have taken time to ask someone else for confirmation.

@Montellese do not use / care about music so he only checked syntax for JSON part without having all the necessary element to understand all the problems you integrated. And since you were aware of the problems but decide to merge, this is called sabotage.

Sorry to go public about our exchanges but Kodi integrity is important at least to me.

[DaveTBlake]

@Tolriq I am sorry if I have upset you, but I am surprised and very disappointed in your reaction too and the accusations you make.

I had a long email exchange with Tolriq over the JSON aspects of this PR, and incorporated a number of improvements. However we had some remaining disagreements. Role is much like genre, so I have implemented the API in a similar manner. It turns out that genre and the filters in GetArtists do not work in the way Tolriq thought they did (neither does the core querying of Kodi). He demanded that I treat role differently and said that genre was broken (in fact that whole area of Kodi), and I declined. I feel that the kind of changes needed to get the API to do what wants are beyond the scope of this PR.

As I said to him before I will happily discuss the whole music API design issue in pubic, I will even do any work needed, but I just don't see it belongs in this PR.

[razzeee]

It's pre alpha, there is enough time to improve upon this.

On Mon, Jan 18, 2016, 09:22 Tolriq notifications@github.com wrote:

Another funny thing : The GetSong part , you can filter on role and roleid
(but again roleid not returned in contributor filed).

So basically you can query all songs that have a composer or a lyricist.

If you use 2 filter + "and" you'll have all songs where toto is an artist
and there's a composer (but composer could be anyone).

@DaveTBlake https://github.com/DaveTBlake when you ask for advice to
someone that have the knowledge and despite more than 20 PM / Mails you
decide to ignore all the messages about your changes being bad and / or
useless you should at least have taken time to ask someone else for
confirmation.

@Montellese https://github.com/Montellese do not use / care about music
so he only checked syntax for JSON part without having all the necessary
element to understand all the problems you integrated. And since you were
aware of the problems but decide to merge, this is called sabotage.

Sorry to go public about our exchanges but Kodi integrity is important at
least to me.

—
Reply to this email directly or view it on GitHub
#8015 (comment).

[Tolriq]

@DaveTBlake you mixed 2 things : The ask for something when you asked me about the API.

And the last mails exchanges where I did warn you about the breaking you introduce and the fact that API was not finished and should not be merged.
You on purpose refused to take time to think about API and force merge. (Removing JSON part and thinking 2 seconds about it would have at least ensure that there's something that can be used...)

Leading to an API that does not even work for the basics (GetSongs that have composers ?) and that introduce breaking to all JSON convention.
Changes to GetArtist are bad (filter on data not on filtered data, implicit filter, magic number to remove a filter) and you were aware of that. This is against the whole JSON handling in Kodi ....

Do not mix with the rest of the discussion.

What you merged does not even provide any way to achieve what you want, offline remote or not....

@razzeee breaking things on purpose is bad, you know that once things are merged it's complicated, since the API needs change it means a new major bump of JSON just to fix a too quick merge when the author was aware of the problem ?

[Tolriq]

@DaveTBlake even better : you can filter songs by roleid 50 (all songs are returned) even when there's no roleid 50 existing ;) (And of course filtering by roleid have no purpose since you need to be able to filter by artist + role together to list correct songs)

And you can not use filter roleid -1 for artist as the definition does not allow it and it returns an error.

So 100% of the JSON part is broken and you wonder why I'm upset and react like that ? The JSON part should never have been merged ....