Existing Album Artist changes to VA in new & changed scan

[darrell-k]

Analysis for https://forums.lyrion.org/forum/developer-forums/developers/1756257-albums-contributor-on-compilations-is-always-set-to-va-if-albumartist-is-changed

Looks like the problem is here:

slimserver/Slim/Utils/Scanner/Local.pm

Lines 1112 to 1120 in ef5c4b9

	# Reset compilation status as it may have changed
	if ( $album ) {
	# XXX no longer works after album code ported to native DBI
	$album->compilation(undef);
	$album->update;
	# Auto-rescan mode, immediately merge VA
	Slim::Schema->mergeSingleVAAlbum( $album->id );
	}

As far as I can tell, Slim::Schema::_createOrUpdateAlbum has already called mergeSingleVAAlbum where neccessary and done the work, so I would propose removing the above code.

Thoughts?

[michaelherger]

Could you please provide the necessary description and information needed to reproduce and understand the problem here? Maybe ask ChatGPT to summarize the linked discussion 😁.

[michaelherger]

Well, I gave it a try:

In this discussion, users are experiencing an issue with the LMS (Logitech Media Server) when it comes to changing the "album artist" tag for compilation albums. The original poster (frank1969) notes that when they change the "album artist" on an existing compilation in their database, it defaults to "Various Artists" (VA) regardless of the new value they set. This behavior persists when the album is already in the database, unlike when adding a new album where the album artist is correctly set.
Michael, another user, explains that this behavior is by design, as LMS traditionally treats compilations as VA albums. Frank1969 disagrees, stating that this behavior seems new and inconsistent with their long-term experience using LMS.
Frank1969 further describes how new albums work correctly when added, but changing an album artist for an existing album causes it to revert to VA. They use different categories for compilations and explain their tagging strategy.
Darrell confirms frank1969's analysis, agreeing that the "new" scan is correct and suggesting that the "changed" scan should be modified to respect the user's input for album artist, regardless of the compilation flag. The conversation also touches on tagging strategies for classical music collections within LMS, providing guidance for organizing and tagging such collections effectively.

[michaelherger]

Haha... told it about the name change. To which it responded:

Thank you for the clarification. In the context of your discussion, users are encountering an issue with Lyrion Music Server (formerly known as Logitech Media Server) regarding the handling of album artist tags for compilation albums....

[darrell-k]

That's horribly confusing. Perhaps logically correct, but it just goes to show there's no "I" in "AI", just blind textual analysis. Maybe it works for some things...

My summary:

1. Create a new (to LMS) album, tagged with COMPILATION=1 and with an ALBUMARTIST and import to LMS (new and changed scan)
   Result: albums.contributor is set to the ALBUMARTIST.

2. Change a different tag on the album (or just "touch" the files), do another "new and changed" scan.
   Result: albums.contributor is changed to the VA ID.

The cause is clear: the code I quoted above only runs for changed tracks, and undoes what was previously done in Schema.pm, updating the albums row again.

[michaelherger]

I run a few tests with @frank1969b's "Carousel" album. The findings are slightly confusing:

• mergeSingleVAAlbum is not called twice, as $isCompilation in Schema.pm around line 1372 is already set
• _createOrUpdateAlbum would handle the compilation flag?
• if I disable above call to mergeSingleVAAlbum, the compilation flag would be reset due to the few lines before that
• looking into mergeSingleVAAlbum I see that it's checking for the ARTIST role:

slimserver/Slim/Schema.pm

Line 2183 in dc10f66

my $role = Slim::Schema::Contributor->typeToRole('ARTIST');

• if I changed the above to ALBUMARTIST, the compilation flag would be reset, ignoring the metadata's compilation flag
• this is not a regression in LMS9
• there are too many too confusing comments... "no longer works after album code ported to native DBI"?!?

Based on the last comment in the code snippet you posted I disabled that block for the scanner - which fixed our case. But I'm not sure about the consequences for other cases...

Eg. I used the same album but removed the compilation flag. LMS did scan it as attributed to the album artist, with no compilation flag set. Setting it in the metadata and running a rescan would correctly set the flag in LMS, too. But the opposite (clean scan with the flag, then remove the flag) would not be picked up, as the logic in LMS would only respect defined values from the metadata. But my tagger would not set compilation=0, but rather remove the flag...

I doubt we'll be able to fix all these cases without major work. In fact I believe that due to the complexity we already have issues which were created by fixing something else, but not consistently etc. One issue I'm seeing with the compilation handling is metadata vs. our own logic. In particular if a tagger would not explicitly set compilation=0 (or false) our logic would kick in, not allowing the user to override it.

TL;DR

The following change "fixes" this issue for me, but would still not fix the case where an album had the compilation flag set, and we remove the flag without a full wipe & rescan:

diff --git a/Slim/Utils/Scanner/Local.pm b/Slim/Utils/Scanner/Local.pm
index 602fde279..5e4c785aa 100644
--- a/Slim/Utils/Scanner/Local.pm
+++ b/Slim/Utils/Scanner/Local.pm
@@ -1110,7 +1110,7 @@ sub changed {
                        }
 
                        # Reset compilation status as it may have changed
-                       if ( $album ) {
+                       if ( $album && !main::SCANNER ) {
                                # XXX no longer works after album code ported to native DBI
                                $album->compilation(undef);
                                $album->update;

It's based on the comment which mentions auto-rescan mode, which would be run in the server process vs. regular scan run in an external process.

Why on earth would Github not recognize @frank1969b as a Github user while typing the name?...

[darrell-k]

• mergeSingleVAAlbum is not called twice, as $isCompilation in Schema.pm around line 1372 is already set
• _createOrUpdateAlbum would handle the compilation flag?

I mean't potentially call mergeSingleVAAlbum.

• if I disable above call to mergeSingleVAAlbum, the compilation flag would be reset due to the few lines before that

You mean $album->compilation(undef);? I was suggesting getting rid of everything in the snippet above. But I just noticed mergeSingleVAAlbum also called in the deleted sub so that would need looking into.

• looking into mergeSingleVAAlbum I see that it's checking for the ARTIST role:

slimserver/Slim/Schema.pm

Line 2183 in dc10f66

my $role = Slim::Schema::Contributor->typeToRole('ARTIST');

• if I changed the above to ALBUMARTIST, the compilation flag would be reset, ignoring the metadata's compilation flag

I don't think that would be a good idea, as that looks like the bit of code which sets albums.compilation if there's no COMPILATION tag in the track. which might be why it's called from Local.pm. I'll investigate further.

• this is not a regression in LMS9
• there are too many too confusing comments... "no longer works after album code ported to native DBI"?!?

Based on the last comment in the code snippet you posted I disabled that block for the scanner - which fixed our case. But I'm not sure about the consequences for other cases...

Eg. I used the same album but removed the compilation flag. LMS did scan it as attributed to the album artist, with no compilation flag set. Setting it in the metadata and running a rescan would correctly set the flag in LMS, too. But the opposite (clean scan with the flag, then remove the flag) would not be picked up, as the logic in LMS would only respect defined values from the metadata. But my tagger would not set compilation=0, but rather remove the flag...

I doubt we'll be able to fix all these cases without major work. In fact I believe that due to the complexity we already have issues which were created by fixing something else, but not consistently etc. One issue I'm seeing with the compilation handling is metadata vs. our own logic. In particular if a tagger would not explicitly set compilation=0 (or false) our logic would kick in, not allowing the user to override it.

Yes, could be the auto-setting of albums.compilation in the absence of a COMPILATION tag which we'd lose. I'll look into this further. My gut feeling is that this should already be done in _createOrUpdateAlbum and if we were able to remove that call to mergeSingleVAAlbum there might be a noticeable performance improvement as it's running multiple SQLs for every track.

I agree that this is complicated and if we change it at all it should be for 9.1 only.

[darrell-k]

Attached is a spreadsheet containing before and after test results (with the change I suggested at the top - remove all the code in that snippet).

I think I've covered all scenarios when rescanning with null or unrelated tag changes.

My worry about bypassing automatic determination of compilations when there is no Compilation tag was unfounded.

This is not the end of it though, I still need to test actual artist/album artist tag changes between the new and changed scans (and deleted tracks) because then we might actually want to change a compilation to not-a-compilation or vice versa.

But this is enough for today, I think.

LMS Scanner compilations test.ods

[michaelherger]

Copy/pasted the file's content to a tab-to-markdown converter:

Tags			new albums		current code changed albums		proposed code changed albums
Compil.	Album Artist	Common Artist	compil.	name	compilation	name	compil.	name
0	no	no	0	artist (last scanned)	1	Various Artists	0	artist (last scanned)
0	no	yes	0	artist	0	artist	0	artist
0	yes	no	0	album artist	0	album artist	0	album artist
0	yes	yes	0	album artist	0	album artist	0	album artist
1	no	no	1	Various Artists	1	Various Artists	1	Various Artists
1	no	yes	1	Various Artists	0	Various Artists	1	Various Artists
1	yes	no	1	album artist	1	Various Artists	1	album artist
1	yes	yes	1	album artist	0	album artist	1	album artist
none	no	no	1	Various Artists	1	Various Artists	1	Various Artists
none	no	yes	0	common artist	0	common artist	0	common artist
none	yes	no	0	album artist	0	album artist	0	album artist
none	yes	yes	0	album artist	0	album artist	0	album artist

Notes:

1. Inconsistency between new scan and changed scan, see rows 3 and 9 (italic).
2. The proposed change introduces consistency between scanning new albums and scanning changed albums.
3. Not sure about row 8: should having a common artist be treated the same as having an album artist (ie rows 9 and 10)
4. Row 9 is the problem reported by Frank.

[darrell-k]

Nice! But need to edit the notes as they're referring to spreadsheet rows/cells.