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 musicmetadata handling of compilations #192
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Useful after an upgrade (or while hacking) or if something else changed which doesn't affect the timestamp of the file.
…TIST Although there is no real standard this is as described in https://picard.musicbrainz.org/docs/mappings/ and what one gets by default using the picard tool (as I do).
I was observing that I had one `music_albums` entry per track on each compilation album after the album was scanned the second time (first time it went in correctly). The issue was that on reloading from the DB the field was not being initialised so remained as `-1` when the entry came to be written back, which because the field in the DB is `unsigned` ended up being stored as `0`, so when subsequent lookups try to find the album it failed every time (since the 0 in the database matches neither -1 nor the >0 correct value) and a fresh one is inserted for every track. Fix this by adding and using `{get,set}CompilationArtistId` corresponding to the uses of `{get,set}ArtistId`. I broke out `getCompilationArtistId` from the within exiting `getArtistId` implementation.
The latter already calls the former right at the end.
Since these may have changed.
... otherwise they are never recalculated when the actual value changes.
This ensures all the fields are properly filled in. In particular it ensures that non-compilation albums have the compilation artist filled in to match the artist, otherwise they all end up with "Unknown Artist" which in turn means that albums which happen to have the same title (e.g. "Greatest Hits") all get lumped into one.
ijc
force-pushed
the
musicmetadata-compilations
branch
from
March 19, 2020 09:58
75f9d94
to
27a9c9a
Compare
linuxdude42
pushed a commit
that referenced
this pull request
Jun 6, 2020
Fixes #13585 Closes #192 Signed-off-by: David Hampton <mythtv@love2code.net> (cherry picked from commit 1236aef) ----- Squashed commit of the following: commit ca6ffb883c9f32ec8a7f1461a0b4d71914e3c210 Author: Ian Campbell <ijc@hellion.org.uk> Date: Mon Mar 16 20:18:29 2020 +0800 musicmetadata: check for empty field before dumping to db This ensures all the fields are properly filled in. In particular it ensures that non-compilation albums have the compilation artist filled in to match the artist, otherwise they all end up with "Unknown Artist" which in turn means that albums which happen to have the same title (e.g. "Greatest Hits") all get lumped into one. commit 88418b6c7c400d04440eeeaf232f571a1463b09a Author: Ian Campbell <ijc@hellion.org.uk> Date: Mon Mar 16 20:02:28 2020 +0800 musicmetadata: clear id fields when main field is set ... otherwise they are never recalculated when the actual value changes. commit a0e93004c18f3a34c2c2d450af72366860a19b4e Author: Ian Campbell <ijc@hellion.org.uk> Date: Sun Mar 15 15:35:32 2020 +0800 musicmetadata: Fully update music_albums, including name and artist Since these may have changed. commit 30898722aebbcfbd9d28557fcdbf2324379db2b6 Author: Ian Campbell <ijc@hellion.org.uk> Date: Sun Mar 15 15:33:43 2020 +0800 musicmetadata: Do not call `ensureSortFields` after `checkEmptyFields` The latter already calls the former right at the end. commit 2e4a0e93768142c05bcfcd1b58f1b7db7bbde609 Author: Ian Campbell <ijc@hellion.org.uk> Date: Fri Feb 28 07:15:41 2020 +0800 musicmetadata: ensure compilation artist id is always set I was observing that I had one `music_albums` entry per track on each compilation album after the album was scanned the second time (first time it went in correctly). The issue was that on reloading from the DB the field was not being initialised so remained as `-1` when the entry came to be written back, which because the field in the DB is `unsigned` ended up being stored as `0`, so when subsequent lookups try to find the album it failed every time (since the 0 in the database matches neither -1 nor the >0 correct value) and a fresh one is inserted for every track. Fix this by adding and using `{get,set}CompilationArtistId` corresponding to the uses of `{get,set}ArtistId`. I broke out `getCompilationArtistId` from the within exiting `getArtistId` implementation. commit 56506e477ceb815081ed05aea3ff6656413b592a Author: Ian Campbell <ijc@hellion.org.uk> Date: Wed Feb 19 20:05:22 2020 +0800 metaioflacvorbis: Handle ALBUMARTIST as a fallback for COMPILATION_ARTIST Although there is no real standard this is as described in https://picard.musicbrainz.org/docs/mappings/ and what one gets by default using the picard tool (as I do). commit 1e303b005e4b613e4b965b1a5cbdc830b64020c2 Author: Ian Campbell <ijc@hellion.org.uk> Date: Thu Feb 27 20:00:05 2020 +0800 Support `mythutil --scanmusic --force` to ignore file timestamps Useful after an upgrade (or while hacking) or if something else changed which doesn't affect the timestamp of the file.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was observing that I had one
music_albums
entry per track on each compilation album after the album was scanned the second time (first time it went in correctly).There are two things I needed to fix to make this work correctly:
Firstly in the flacvorbis code handle
ALBUMARTIST
as a fallback forCOMPILATION_ARTIST
. Although there is no real standard this is as described in https://picard.musicbrainz.org/docs/mappings/ and what one gets by default using the picard tool (as I do).Secondly ensure that the
MusicMetadata::m_compartistId
is always properly setup, I was observing that it was still-1
by the time the database writes were happening, which ends up as0
in the db (because themusic_albums.album_id
field is unsigned) which then wouldn't match on subsequent lookups, so multiple albums were created for compilations (one per track). The fix is to make sure the compilation artist id is always properly set.As well as the fixes I've also added a
--force
flag tomythutil --scanmusic
which ignores the file timestamp, so it updates everything, this is useful when testing changes like this where the files don't actually change.I've run this on my v30 production system as well as some tests (scanning and querying the db) on a sandboxes master build. FWIW the porting was mostly trivial but had a few clashes with Dave Hampton's various
tidy: *
commits in the interval v30..HEAD. They were:m_*id
renamedm_*Id
MusicFileScanner::MusicFileScanner
is different so needed minor adjustments.If this is a candidate for v31 backport and you want a backport I'm happy to make one (but I think the cherry-pick will be trivial, the above are v30..v31 conflicts not v31..HEAD I think)