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

Better handling of mis-matching musicbrianz album artist id and hints tags #9625

Merged
merged 1 commit into from Apr 19, 2016

Conversation

@DaveTBlake
Copy link
Member

commented Apr 14, 2016

A follow on from #8904 with more improved handling of mis-matching numbers of Musicbrainz (Album) Artist Ids and (Album) Artist tags when creating songs and albums.

Primarily this was to fix an issue where missing hints (ALBUMARTISTS tag) was causing the album artist tag to be ignored (even when matched mbid) and artist hints and names to be used instead to get album artist names to match the number of mbid. If there was a mis-match of artist names and mbid then the results were quite confusing.
For example:
ARTIST = "artist1 feat. artist2"
MUSICBRAINZ ARTISTID = mbid1
MUSICBRAINZ ARTISTID = mbid2
ALBUMARTIST = "artist1"
MUSICBRAINZ ALBUMARTISTID = mbid1

resulted in an artist named "artist1 feat. artist2" with mbid1 for the album even though the number of album artist names and mbids matched, and the lack of ARTISTS tag and mis-match of mbid <-> name count would have been repaired for the song.

Also a user managed to have ARTISTS and ALBUMARTISTS tags present but as single string with an unexpected separator. If one user can do it so can others, so a more flexible approach to parsing both (ALBUM)ARTIST and (ALBUM)ARTISTS tags when the number of names does not match the number of Musicbrainz ids seems worthwhile.

Hence when an inconsistency in the number of artist names to mbid occurrs this applies improved parsing to both hints and names, using multiple item separators progressively, in an attempt to correctly interpret the way music files have been tagged.

@Razzeee got some time to look at this? The sooner it is out there getting more real user data through it the better


// Vector of possible separators in the order least likely to be part of artist name
const char *sepinit[] = { " feat. ", ";", ":", "|", "#", "/", ",", "&" };
std::vector<std::string> separators(sepinit, std::end(sepinit));

This comment has been minimized.

Copy link
@phate89

phate89 Apr 14, 2016

Contributor

Is sepinit used only to initialize the vector? in c++11 you should be able to do
std::vector<std::string> separators { " feat. ", ";", ":", "|", "#", "/", ",", "&" };
Same thing in Song.cpp
Also not essential but better to set it as const

if (input.empty())
return results;

for (std::vector<std::string>::const_iterator it = input.begin(); it != input.end(); ++it)

This comment has been minimized.

Copy link
@phate89

phate89 Apr 14, 2016

Contributor

it's the same thing but imo it's cleaner to use range based iterators to iterate in vectors like this:

for (const auto& item: input)
  results.push_back(item);
@@ -759,6 +759,61 @@ std::vector<std::string> StringUtils::Split(const std::string& input, const char
return results;
}

std::vector<std::string> StringUtils::Split(const std::vector<std::string> &input, const std::vector<std::string> &delimiters, unsigned int iMaxStrings /* = 0 */)

This comment has been minimized.

Copy link
@phate89

phate89 Apr 14, 2016

Contributor

Isn't SplitMulti a better name? There's already a SplitMulti and it's similar to that since it takes more than one separator

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Apr 17, 2016

Author Member

Not sure. SplitMulti had to have a different name to differentiate between split with a string that should be applied as a single delimter, and a string where each character is a deliimter. SplitMulti is also unused now, I wrote it to do the initial version of tag repair, so do I remove it? Or do I replace it?

This comment has been minimized.

Copy link
@phate89

phate89 Apr 18, 2016

Contributor

IF it's unused you should remove it.. Imho is better to have a name distinction between a regular split and this complex split..

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

Thanks @phate89 for the code suggestions. My dev env is in pieces at the moment, attempting to back out of VS2015 (seems incompatible with my old PC) which has brought me to a halt.

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:FixArtistTagRobust branch from ee042b5 to 9c3fb76 Apr 17, 2016
return results;

for (const auto& item : input)
results.push_back(item);

This comment has been minimized.

Copy link
@phate89

phate89 Apr 18, 2016

Contributor

I know Isuggested already a change here but you can avoid the loop initializing result with the current vector:

if (input.empty())
  return std::vector<std::string>(); // or even return {};

std::vector<std::string> results(input);
…t Ids and (Album) Artist tags when creating songs and albums.

Fix missing hints (ALBUMARTISTS tag) causing artist hints to be used to get album artist to match mbid rather than try album artist itself. Made mess when artist hints are broken and repair happening.
@DaveTBlake DaveTBlake force-pushed the DaveTBlake:FixArtistTagRobust branch from 9c3fb76 to 9b37330 Apr 18, 2016
@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2016

Thanks for the improvements @phate89

jenkins build this please

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

Compile error not related to this PR

@DaveTBlake DaveTBlake merged commit e31538d into xbmc:master Apr 19, 2016
1 of 3 checks passed
1 of 3 checks passed
default Merged build finished.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@DaveTBlake DaveTBlake added this to the Krypton 17.0-alpha1 milestone Apr 19, 2016
@DaveTBlake DaveTBlake deleted the DaveTBlake:FixArtistTagRobust branch Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.