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

mbsync: fix updating album with invalid first track MBID #2920

Merged
merged 1 commit into from
May 10, 2018

Conversation

bjin
Copy link
Contributor

@bjin bjin commented May 9, 2018

MBID of recording could become invalid after merging. The existing
code always copies metadata from first track after updating. But for
albums with invalid track MBID that happens to be the first track,
MusicBrainz changes won't be applied to whole album, only whose
tracks with valid MBID. This is particularly annoying since those
changes are actually displayed for every beet mbsync run, but never
get applied.

Fix this issue by finding any track that get MusicBrainz updates, and
apply it to whole album.

Some additional comments:

  1. While mbsync: Map tracks using release track MBIDs instead of recording MBIDs #2917 seems being able to solve beet mbsync issues in long term. It still can't handle existing collection with invalid track MBID. Since those tracks won't receive any updates (including mb_release_trackid), and becomes "zombie". This pull request won't solve the whole "track MBID missing" issue, just the most annoying part of it.
  2. This issue actually caused mbcollection: 'remove' option inadvertently removes merged releases #2914 (partially) for me. It stops merged album MBID from being updated during beet mbsync run. The issue of mbcollection: 'remove' option inadvertently removes merged releases #2914 still exists, but with this pull request at least beet mbsync could become a work around.

@jdetrey
Copy link
Contributor

jdetrey commented May 9, 2018

Hi! Thing is, with this change, if I'm not mistaken, tracks with an invalid mb_trackid will remain "zombie", right? It seems to me that this happens when the candidates list in line 133 of mbsync.py is empty: then mapping fails to map the corresponding Item to a TrackInfo object.

Note however that, when there are multiple candidates, mbsync falls back to identifying tracks based on medium and track numbers.

Maybe a solution when there is no available candidate for a given track could be to go through all elements of album_info.tracks and identify the missing track using medium and track numbers. See commit jdetrey/beets@1b929b5 for a possible fix.

@bjin
Copy link
Contributor Author

bjin commented May 9, 2018

Hi! Thing is, with this change, if I'm not mistaken, tracks with an invalid mb_trackid will remain "zombie", right? I

Yes. With invalid track MBID there are no candidates availble for selection later.

Maybe a solution when there is no available candidate for a given track could be to go through all elements of album_info.tracks and identify the missing track using medium and track numbers. See commit jdetrey/beets@1b929b5 for a possible fix.

This fix doesn't seem to be robust enough. I actually did a few attempts to clean up all tracks with invalid MBID (around 5% of all my collection). None of those attempts managed to figuring out all mappings correctly with high confidence.

If we want to stay on the relatively safe side, a c.title == item.title in addition to c.medium_index == item.track and c.medium == item.disc helped me fixing about 70% of those zombie tracks. The other "smart" approach I tried all brings some wrong mapping.

@jdetrey
Copy link
Contributor

jdetrey commented May 9, 2018

By wrong mapping, do you mean that tracks have been renumbered in MusicBrainz, and that a mapping based on medium and track numbers only misses this renumbering and thus wrongly identifies the tracks?

Wow, I didn't think this could happen: I would've thought disc and track numbers were not prone to frequent changes. But sure, adding an extra check based on title would definitely help avoiding such mixups (even though mbsync would then miss the tracks whose title has changed; but I guess that's probably the price to pay, here).

@bjin
Copy link
Contributor Author

bjin commented May 9, 2018

Not just shuffling, some times tracks could also get removed or added. MusicBrainz, after all, is created by human. Medium list could be wrong and be changed later in any way one can think of.

On the other hand, leaving zombie track doesn't hurt much. It's not visible to user (after this PR merged), and won't hurt user in another way later (unless we attempted to fix in an aggressive way and created wrong track mapping). So I would suggest the strict approach if we are going to fix the zombie tracks issue in one way or another.

@jdetrey
Copy link
Contributor

jdetrey commented May 9, 2018

Yes, you're absolutely right. Leaving zombie tracks isn't that much of a problem indeed, especially since albums can still be updated (unless all of their recording IDs have changed) thanks to your PR. Thanks!

@sampsyo
Copy link
Member

sampsyo commented May 10, 2018

Sounds like a good idea to me; thanks!

Could you please add a changelog entry describing the change? Also, is there an easy way to write a test showcasing the problem?

@bjin bjin force-pushed the fix-invalid-first-track-mbid branch from 00c621d to 4a40369 Compare May 10, 2018 07:25
bjin added a commit to bjin/beets that referenced this pull request May 10, 2018
@bjin
Copy link
Contributor Author

bjin commented May 10, 2018

rebased and added changelog entry.

Also, is there an easy way to write a test showcasing the problem?

Hmm, I don't think there is an easy way. However, the fix is actually pretty self explained. I also tested it while fixing zombie tracks from my collection. It works as expected.

MBID of recording could become invalid after merging. The existing
code always copies metadata from first track after updating. But for
albums with invalid track MBID that happens to be the first track,
MusicBrainz changes won't be applied to whole album, only whose
tracks with valid MBID. This is particularly annoying since those
changes are actually displayed for every `beet mbsync` run, but never
get applied.

Fix this issue by finding any track that get MusicBrainz updates, and
apply it to whole album.
@bjin bjin force-pushed the fix-invalid-first-track-mbid branch from 4a40369 to 69d6dfe Compare May 10, 2018 08:39
@sampsyo
Copy link
Member

sampsyo commented May 10, 2018

OK, no big deal. Thanks for looking into it, and thanks for adding the changelog entry too!! ✨

@sampsyo sampsyo merged commit 69d6dfe into beetbox:master May 10, 2018
sampsyo added a commit that referenced this pull request May 10, 2018
mbsync: fix updating album with invalid first track MBID
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants