Skip to content

Commit 2cd9ea5

Browse files
RouzaxRouzax
committed
#1555 - Use plural ALBUMARTISTS / ARTISTS as authoritative source of individual contributors
When Picard-style plural tags (ALBUMARTISTS, ARTISTS) are present as arrayrefs on the scanned $attributes hash, use them as the authoritative list of individual contributors rather than parsing the joined singular ALBUMARTIST / ARTIST display string with splitList. The singular tag remains the display string on the track / album row. This is the structural follow-up to the stop-gap in 0dbbb98. With aligned plural tags now feeding Slim::Schema::Contributor->add as arrayrefs, the positional MBID zip is no longer a guess: it is an index provided by the tagger. The length-mismatch guard from the stop-gap stays as the safety net for the legacy single-string path. - Slim/Schema.pm _preCheckAttributes: defer ARTISTS, ALBUMARTISTS, TRACKARTISTS so they reach _mergeAndCreateContributors via the newTrack path (previously they were left in the regular attributes hash and lost before contributor processing). - Slim/Schema.pm _mergeAndCreateContributors: prefer \$attributes->{\$tag.'S'} as the contributor source when it is a non-empty arrayref, for the ARTIST / ALBUMARTIST / TRACKARTIST roles only. Other roles (COMPOSER, BAND, CONDUCTOR, ...) are unchanged. Trim plural names, drop empty-name slots and their aligned MBID slot, tolerate arrayref \$contributor in the whitespace-strip step, and loosen the early skip so a track with plural but no singular still produces rows. - Slim/Schema.pm TRACKARTIST compilation transform (bug 2317 / 2638): also migrate the plural ARTISTS tag to TRACKARTISTS, symmetric with the existing ARTIST and MUSICBRAINZ_ARTIST_ID migration. Picard tag mapping reference: https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html Backwards compatibility: files without plural tags, or with plural tags as scalars, fall through to the existing splitList path. No schema change. No format-parser change. Already-poisoned libraries from pre-stop-gap scans need a wipecache. Users who tuned splitList to work around the absence of plural-tag reading may find it is no longer needed, but it is not removed. Known limitation for MP3: Audio::Scan returns only the first value of multi-value ID3v2.4 TXXX text frames (which is how Picard writes plural tags into MP3 files). Until that is addressed, MP3 files see the poisoning gone but do not get the full multi-contributor benefit. FLAC, Ogg, Opus and MP4 are unaffected and gain the full benefit. Signed-off-by: Rouzax <GitHub@mgdn.nl>
1 parent 6e40c98 commit 2cd9ea5

2 files changed

Lines changed: 78 additions & 8 deletions

File tree

‎Changelog9.html‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ <h2><a name="v9.2.0" id="v9.2.0"></a>Version 9.2.0</h2>
3131
<li><a href="https://github.com/LMS-Community/slimserver/pull/1517">#1517</a> - Fix work images and artwork precaching (@darrell-k)</li>
3232
<li><a href="https://github.com/LMS-Community/slimserver/pull/1553">#1553</a> - Allow plugins to shut down before closing the database (@SamInPgh)</li>
3333
<li><a href="https://github.com/LMS-Community/slimserver/issues/1555">#1555</a> - Scanner: stop-gap to prevent contributor row poisoning when the MusicBrainz ID count does not match the artist name count (e.g. "Artist A &amp; Artist B" or "Artist A ft. Artist B" with multi-value MBIDs). The ambiguous MBIDs are dropped rather than mis-bound. Full plural-tag support to follow. (@Rouzax)</li>
34+
<li><a href="https://github.com/LMS-Community/slimserver/issues/1555">#1555</a> - Scanner: read the Picard-convention plural tags ALBUMARTISTS and ARTISTS as the authoritative list of individual contributors for the ALBUMARTIST and ARTIST roles, zipped positionally with MUSICBRAINZ_ALBUMARTIST_ID / MUSICBRAINZ_ARTIST_ID. The singular tag still provides the album/track display string. Behaviour for libraries without plural tags is unchanged. Run <code>wipecache</code> to benefit on already-scanned libraries. (@Rouzax)</li>
3435
<li></li>
3536
</ul>
3637
<br />

‎Slim/Schema.pm‎

Lines changed: 77 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2842,6 +2842,7 @@ sub _preCheckAttributes {
28422842
MUSICBRAINZ_ARTIST_ID MUSICBRAINZ_ALBUMARTIST_ID MUSICBRAINZ_ALBUM_ID
28432843
MUSICBRAINZ_ALBUM_TYPE MUSICBRAINZ_ALBUM_STATUS RELEASETYPE
28442844
ALBUM_EXTID ARTIST_EXTID WORK WORKSORT
2845+
ARTISTS ALBUMARTISTS TRACKARTISTS
28452846
))
28462847
{
28472848

@@ -3126,34 +3127,102 @@ sub _mergeAndCreateContributors {
31263127
$attributes->{'TRACKARTISTSORT'} = delete $attributes->{'ARTISTSORT'};
31273128
$attributes->{'MUSICBRAINZ_TRACKARTIST_ID'} = delete $attributes->{'MUSICBRAINZ_ARTIST_ID'} if $attributes->{'MUSICBRAINZ_ARTIST_ID'};
31283129

3130+
# Issue #1555 - carry the plural ARTISTS tag across too, so the
3131+
# per-role plural lookup below finds it under TRACKARTISTS.
3132+
$attributes->{'TRACKARTISTS'} = delete $attributes->{'ARTISTS'} if $attributes->{'ARTISTS'};
3133+
31293134
main::DEBUGLOG && $isDebug && $log->debug(sprintf("-- Contributor '%s' of role 'ARTIST' transformed to role 'TRACKARTIST'",
3130-
$attributes->{'TRACKARTIST'},
3135+
ref($attributes->{'TRACKARTIST'}) eq 'ARRAY'
3136+
? join(' / ', @{$attributes->{'TRACKARTIST'}})
3137+
: $attributes->{'TRACKARTIST'},
31313138
));
31323139
}
31333140
}
31343141

31353142
my %contributors = ();
31363143

3144+
# Issue #1555 - roles for which a plural Picard tag (ARTISTS,
3145+
# ALBUMARTISTS, TRACKARTISTS) is the authoritative individual-contributor
3146+
# list. The singular tag remains the display string; the plural tag,
3147+
# when present as a non-empty arrayref after trimming, supersedes
3148+
# splitList parsing of the singular for contributor-row creation.
3149+
my %pluralRoles = map { $_ => 1 } qw(ARTIST ALBUMARTIST TRACKARTIST);
3150+
31373151
for my $tag (Slim::Schema::Contributor->contributorRoles) {
31383152

3139-
my $contributor = $attributes->{$tag} || next;
3153+
my $contributor = $attributes->{$tag};
3154+
my $brainzID = $attributes->{"MUSICBRAINZ_${tag}_ID"};
3155+
my $usedPlural = 0;
3156+
3157+
# Issue #1555 - prefer the plural Picard tag as the individual
3158+
# contributor list when it is a non-empty arrayref with at least
3159+
# one non-blank name. Trim, drop empty-name slots, and drop the
3160+
# corresponding MBID slot so positional alignment is preserved.
3161+
# Empty-string MBID slots (for contributors without an MBID,
3162+
# Picard convention) are kept; Contributor->add's `if ($mbid)`
3163+
# guard handles them. A scalar plural value is treated as "not
3164+
# plural" and falls through to the legacy singular path.
3165+
if ($pluralRoles{$tag}) {
3166+
my $plural = $attributes->{$tag . 'S'};
3167+
if (ref($plural) eq 'ARRAY' && @$plural) {
3168+
my $pluralMbid = $attributes->{"MUSICBRAINZ_${tag}_ID"};
3169+
my @mbids = ref($pluralMbid) eq 'ARRAY' ? @$pluralMbid : ();
3170+
my (@names, @alignedMbids);
3171+
for (my $i = 0; $i < @$plural; $i++) {
3172+
my $name = $plural->[$i];
3173+
next unless defined $name;
3174+
$name =~ s/^\s+//; $name =~ s/\s+$//;
3175+
next if $name eq '';
3176+
push @names, $name;
3177+
push @alignedMbids, $mbids[$i] if @mbids;
3178+
}
3179+
if (@names) {
3180+
$contributor = \@names;
3181+
$brainzID = @mbids ? \@alignedMbids : $brainzID;
3182+
$usedPlural = 1;
3183+
main::DEBUGLOG && $isDebug && $log->debug(sprintf(
3184+
"-- Using plural %sS (%d entries) as contributor source for role '%s'",
3185+
$tag, scalar @names, $tag,
3186+
));
3187+
}
3188+
}
3189+
}
3190+
3191+
# Skip unless we have a source of contributor names for this role.
3192+
# Empty string, undef, or an empty plural array all skip.
3193+
next unless $usedPlural || (defined $contributor && (ref $contributor || $contributor ne ''));
31403194

3141-
# Bug 17322, strip leading/trailing spaces from name
3142-
$contributor =~ s/^ +//;
3143-
$contributor =~ s/ +$//;
3195+
# Bug 17322, strip leading/trailing spaces from name. splitTag
3196+
# already trims arrayref elements, so the regex strip is scalar
3197+
# only.
3198+
unless (ref $contributor) {
3199+
$contributor =~ s/^ +//;
3200+
$contributor =~ s/ +$//;
3201+
}
31443202

31453203
# Is ARTISTSORT/TSOP always right for non-artist
31463204
# contributors? I think so. ID3 doesn't have
31473205
# "BANDSORT" or similar at any rate.
3206+
#
3207+
# Issue #1555 - when we used the plural names path, skip the
3208+
# singular joined sort tag: it is a joined display string of
3209+
# the same shape as the singular name (e.g. "van Buuren, Armin
3210+
# & KI/KI") and there is no standard plural sort variant to
3211+
# align with. Passing it would pollute the first contributor's
3212+
# sort with the whole joined string. Falling through to
3213+
# sortBy-as-name is strictly better.
31483214
push @{ $contributors{$tag} }, Slim::Schema::Contributor->add({
31493215
'artist' => $contributor,
3150-
'brainzID' => $attributes->{"MUSICBRAINZ_${tag}_ID"},
3151-
'sortBy' => $attributes->{$tag.'SORT'},
3216+
'brainzID' => $brainzID,
3217+
'sortBy' => $usedPlural ? undef : $attributes->{$tag.'SORT'},
31523218
# only store EXTID for track artist, as we don't have it for other roles
31533219
'extid' => $tag eq 'ARTIST' && $attributes->{'ARTIST_EXTID'},
31543220
});
31553221

3156-
main::DEBUGLOG && $isDebug && $log->is_debug && $log->debug(sprintf("-- Track has contributor '$contributor' of role '$tag'"));
3222+
main::DEBUGLOG && $isDebug && $log->is_debug && $log->debug(sprintf(
3223+
"-- Track has contributor '%s' of role '$tag'",
3224+
ref($contributor) eq 'ARRAY' ? join(' / ', @$contributor) : $contributor,
3225+
));
31573226
}
31583227

31593228
# Bug 15553, Primary contributor can only be Album Artist or Artist,

0 commit comments

Comments
 (0)