#1555 - Use plural ALBUMARTISTS / ARTISTS as authoritative source of individual contributors

[Rouzax]

Summary

Structural fix for #1555. Reads the Picard-convention plural tags ALBUMARTISTS and ARTISTS as arrayrefs and uses them as the authoritative individual-contributor list, so the name <-> MBID pairing stops depending on splitList heuristics over joined display strings. The singular ALBUMARTIST / ARTIST tag remains the album / track display string.

Stacks on the stop-gap PR #1556 (the length-mismatch guard in Contributor::add). The guard remains in place as the safety net for the legacy single-string fall-back path and for files where the plural tag is absent or scalar.

Contributes to #1555 (does not close it; closing waits until both PRs land). Also addresses the long-standing symptom reported in the closed #1052.

References:

• Picard tag mapping: https://picard-docs.musicbrainz.org/en/latest/appendices/tag_mapping.html
• Picard basic variables: https://picard-docs.musicbrainz.org/en/latest/variables/tags_basic.html

This is a behavioural change to the scanner. Design feedback welcome before merge. Specifically: the plural-detection logic lives inside the contributor loop and keys off $tag . 'S' (only firing for ARTIST / ALBUMARTIST / TRACKARTIST). An alternative is a pre-loop expansion pass that unwraps the plural arrayref onto the singular slot. I picked the in-loop form because it keeps the TRACKARTIST compilation transform the only place that mutates $attributes pre-loop. Happy to refactor either way.

Scope of benefit (read this first)

Confirmed against the Picard docs:

• ARTISTS (plural track artists) is written by default by Picard (since Picard 2.x, as a standard tag across Vorbis / ID3v2 TXXX / MP4 freeform / ASF). So the track-level ARTIST role gets the full plural-path benefit for any Picard-tagged library without user configuration. This is the primary user-visible win, and it covers the "Artist A ft. Artist B" track-credit case that prompted the follow-up report on Scanner binds unaligned MusicBrainz album-artist IDs to joined-display contributor, poisoning later albums #1555.
• ALBUMARTISTS is NOT a standard Picard tag. It is not in Picard's tag mapping. Users who want it must add a small Picard script ($setmulti(albumartists,%_albumartists%)) or use another tagger / workflow that writes it (Jellyfin- and Navidrome-friendly Mp3tag templates, beets with the mb plugin, etc.). For vanilla-Picard libraries the ALBUMARTIST role falls back to the legacy singular path, which is still protected from MBID poisoning by #1555 - Stop-gap: drop MusicBrainz IDs when artist name and MBID counts differ #1556's length-mismatch guard but does not get the per-individual split.

We cannot recover the per-individual ALBUMARTIST split from LMS's side when the plural tag is absent: Picard's join phrases are configurable, and guessing from the singular joined string is exactly what splitList already does best-effort. This PR is forward-compatible: as soon as a user's library carries the tag, the fix kicks in.

Design notes

1. Precedence. Plural-path triggers only when $attributes->{$tag.'S'} is a non-empty arrayref with at least one non-blank entry. A scalar plural value falls through to the legacy singular path. Single-element plural arrayref counts (Picard signal "this is one authoritative individual").
2. Display string. The singular ALBUMARTIST / ARTIST tag remains the album / track display string. Skin-visible names are unchanged.
3. Primary contributor. First entry of the plural arrayref.
4. Sort tag. Picard does not define a new plural sort tag name, but the Picard internal variables %_albumartists_sort% and %_artists_sort% do carry per-individual sort data from MusicBrainz, and users can script those onto the existing ALBUMARTISTSORT / ARTISTSORT tags as multi-value content (e.g. $setmulti(albumartistsort,%_albumartists_sort%)). When the plural path is used, this PR checks whether the sort tag is an arrayref whose length matches the filtered plural names and, if so, uses it positionally for per-individual sort (e.g. van Buuren, Armin for Armin). Otherwise it passes sortBy => undef, which falls back to sort-by-name. Passing the joined singular sort string would pollute only the first contributor's sort with the whole joined value and leave the rest falling back to name; the symmetric name-fallback is strictly better.
5. Empty MBID slots. The Picard "this individual has no MBID" convention (empty string at the aligned index) is preserved positionally by this PR. Contributor::add's existing if ($mbid) guard handles empty slots. Note: LMS's existing Slim::Formats::sanitizeTagValues rejects any MUSICBRAINZ_*ID tag containing a non-UUID element, including empty strings, and strips the whole tag. That stripping is pre-existing and orthogonal to this PR; a separate sanitizer relaxation would be needed to make empty slots end-to-end functional.
6. Empty / whitespace-only name slots in the plural arrayref. Trimmed and dropped along with their aligned MBID slot before passing to Contributor::add.
7. Roles. ARTIST, ALBUMARTIST, and TRACKARTIST only. COMPOSER / CONDUCTOR / BAND etc. are unchanged per scope.
8. TRACKARTIST compilation transform (Bug 2317 / 2638) is extended to also migrate ARTISTS to TRACKARTISTS, symmetric with the existing singular and MBID migrations.
9. Fall-back is unchanged. Files without plural tags, or with plural tags as scalars, follow the existing path including splitList splitting, byte-for-byte. #1555 - Stop-gap: drop MusicBrainz IDs when artist name and MBID counts differ #1556's length-mismatch guard is the safety net there.

Patch surface

• Slim/Schema.pm: three hunks (_preCheckAttributes deferred-list addition; _mergeAndCreateContributors plural-aware loop with sort-tag alignment; TRACKARTIST transform extension).
• Changelog9.html: one bullet under the in-progress release.

No DB schema change. No format-parser change. No change to Slim/Schema/Contributor.pm (the stop-gap from #1556 stays intact).

Backwards compatibility

• No schema change, no automatic migration.
• Files without plural tags, or with plural tags as scalars, see zero behavioural difference.
• Users must wipecache to benefit on already-scanned libraries.
• Users who tuned splitList to work around the absence of plural-tag reading may find it is no longer needed. It is not removed; they can keep or clear it.
• The "primary contributor" semantic is defined as "first entry of the plural list". In rare edge cases where existing databases relied on an arbitrary order, the ordering may shift on rescan.

Verification

Manual end-to-end verification against a hand-built fixture set (silent Opus / FLAC / MP3 / MP4 files tagged with mutagen). Pre-fix vs post-fix scan dumped from the SQLite library DB. Diff captured.

Fixture matrix (Opus, 11 scenarios)

#	Scenario	Pre-fix	Post-fix
1	Plural + aligned MBIDs (the #1555 reproduction)	"Armin van Buuren & KI/KI" + first MBID (poisoned)	Two rows, each individual with their own MBID
2	Plural without explicit MBIDs	"A & B" joined (one row)	A, B as separate rows
3	Plural with three aligned MBIDs	"A, B & C" joined, MBIDs dropped	A, B, C each with their own MBID
4	Singular only (regression guard)	A (one row)	A (unchanged)
5	Joined singular + splitList (regression guard)	A, B via splitTag	A, B unchanged
6	Plural + COMPILATION=1	"A & B" joined ARTIST + first MBID	A, B as separate ARTIST rows; ALBUMARTIST=Various Artists preserved
7	ft. track-artist (the follow-up case)	ALBUMARTIST and TRACKARTIST both joined; one MBID stuck on ALBUMARTIST	ALBUMARTIST individuals + 2 TRACKARTIST rows, all with correct MBIDs
8	Plural with empty name slot	"A, , B" joined	A, B (empty slot dropped + aligned MBID dropped)
9	Plural present, ALBUMARTIST singular missing	"No Artist" (plural ignored)	A, B as separate rows
10	Scalar plural value (buggy tagger)	A & B joined	A & B singular fall-through (unchanged)
11	Plural length != MBID length (existing #1556 guard)	"A & B" + first MBID	A, B separate rows; #1556 guard drops the unaligned MBID

Multi-track album

3-track album, all tracks share ALBUMARTISTS=[Armin, KI/KI] plural; track 1 has single ARTIST=Armin, track 2 single ARTIST=KI/KI, track 3 plural ARTISTS=[Armin, KI/KI]. Album-level resolves to two ALBUMARTIST rows aggregated across tracks; per-track ARTIST / TRACKARTIST rows correctly reflect each track's own credits. No joined rows at any level.

Various Artists compilation

3-track VA compilation with canonical MB Various Artists MBID (89ad4ac3-39f7-470e-963a-56509c546377), COMPILATION=1. Each track has its own ARTIST + plural ARTISTS + MBIDs; track 2 is a two-artist collab. Album row resolves to "Various Artists" primary; contributor_album aggregates all individual track artists as ARTIST rows; per-track ARTIST rows correctly reflect each track's credits.

Sort-tag alignment

Verified in two modes by inspecting contributors.namesort after rescan:

• No plural sort tag present: each plural-path contributor gets its own name-based sort (ARMIN VAN BUUREN, KI KI). The joined ALBUMARTISTSORT string does not leak into the first contributor.
• Aligned plural sort tag present (user scripted %_albumartists_sort% onto ALBUMARTISTSORT as multi-value, per the Picard convention described above): each contributor gets its own per-individual sort from the aligned slot (VAN BUUREN ARMIN for Armin, not name-fallback ARMIN VAN BUUREN).

Cross-format

Same happy-path tags written to FLAC, MP4 (M4A), and MP3:

Format	Result
FLAC (Vorbis Comments)	Works. Two individual ALBUMARTIST rows with MBIDs.
MP4 / M4A (iTunes ----:com.apple.iTunes:ARTISTS freeform)	Works. Two individual ALBUMARTIST rows with MBIDs.
MP3 (ID3v2.4 TXXX)	Partial. Poisoning gone, but only the first individual recovered. See "Known limitations" below.
ASF / WMA (WM/ARTISTS)	Not tested here. Likely depends on Audio::Scan's WMA reader, may or may not surface multi-value. Flagging for a reviewer with a WMA test library.
APEv2	Not tested here. Rare in practice.

CI / smoketest

bash t/00_smoketest.sh passes (server starts, jsonrpc responds with version 9.2.0).

Known limitations

MP3

Picard writes plural tags into MP3 files as ID3v2.4 multi-value text frames (one TXXX frame per descriptor, multiple values null-separated within the frame's text field, per the spec). The vendored Audio::Scan module (CPAN/arch/<perl>/Audio/Scan.{pm,so}, sourced from slimserver-vendor) returns only the first value of multi-value TXXX frames, truncating at the first null. By the time MP3.pm sees the tag, the second and subsequent values are already lost.

Effect with this PR:

• Before: Armin van Buuren & KI/KI joined contributor row with first MBID bound (poisoned).
• After: Armin van Buuren row alone with first MBID; KI/KI is missing entirely from the MP3 album.

This is an improvement (no more poisoning) but is not the full multi-contributor benefit. Two paths to a complete MP3 fix:

1. Patch Audio::Scan upstream (XS / C source in slimserver-vendor) to fully support ID3v2.4 multi-value text frames. Cleanest, most correct, but requires upstream coordination and rebuilds across all platforms.
2. Add a small Perl-level ID3v2.4 TXXX re-parser inside Slim/Formats/MP3.pm for ALBUMARTISTS / ARTISTS / MUSICBRAINZ_ALBUMARTISTID / MUSICBRAINZ_ARTISTID only. ~50-100 lines using pack / unpack on the raw frame bytes; opens the file a second time per scan. Separate small follow-up PR.

Recommend shipping this PR as-is and addressing MP3 in a follow-up. Happy to do either.

Default-Picard ALBUMARTIST libraries

As noted in "Scope of benefit", vanilla Picard does not write ALBUMARTISTS, so default-Picard libraries see the ARTIST-role win but retain the legacy ALBUMARTIST singular-path behaviour (now safe under #1556's guard). Users who want the full album-level split should add the Picard script referenced above, or switch to a workflow that writes ALBUMARTISTS.

FLAC, Ogg, Opus, and MP4 / M4A are unaffected by the Audio::Scan limitation and gain the full benefit whenever the plural tag is present.

Test plan

• Smoketest (bash t/00_smoketest.sh).
• Opus fixture matrix (11 scenarios) before / after diff.
• Multi-track album fixture before / after.
• Various Artists compilation fixture before / after.
• Cross-format fixtures (FLAC, MP3, MP4).
• Sort-tag alignment verification (namesort inspection).
• CI on push.
• Maintainer review of the design (in-loop plural detection vs pre-loop expansion).
• Optional: WMA test fixture.

[michaelherger]

I don't like how some variables can be either a scalar or a list ref. Can't we have separate variables for them? And if they were list refs, wouldn't we have to run the cleanup on all of its items?

I see this pattern in other places, like line 3135.

[Rouzax]

Thanks, both fair points. Taking them separately:

Scalar-or-arrayref variables. Agreed that it's awkward, but the polymorphism predates this PR: $contributor = $attributes->{$tag} has always been able to be either shape depending on the tag reader, and the ref(...) eq 'ARRAY' ? join(...) : $scalar pattern (e.g. the debug log at the old line 3135 you pointed at) is legacy. Splitting into typed variables ($contributorScalar vs $contributorList, same for $brainzID / $sortBy) is a worthwhile cleanup but it touches the whole _mergeAndCreateContributors loop and the Contributor->add call shape. I'd prefer to keep this PR scoped to #1555 and open a follow-up refactor issue for the variable split. Happy to file that and link it from here if you agree.

Cleanup on arrayref items. This one is a real gap and I'll fix it in this PR. The existing unless (ref $contributor) guard relies on the older "splitTag already trims arrayref elements" invariant (see the comment just above it). The new plural-tag path I added trims explicitly at the build step, so arrayrefs from that source are clean, but an arrayref coming through the legacy path isn't guaranteed to be. I'll replace the scalar-only strip with a shape-aware version:

if (ref $contributor eq 'ARRAY') {
    for my $n (@$contributor) {
        next unless defined $n;
        $n =~ s/^\s+//;
        $n =~ s/\s+$//;
    }
} else {
    $contributor =~ s/^ +//;
    $contributor =~ s/ +$//;
}

That way the cleanup runs regardless of which path populated $contributor.

Re: the line 3135 reference, that one is a debug-log formatter rather than a cleanup site, so I don't think it needs a behavioural change, but I'll fold it into the follow-up refactor along with the other polymorphism sites.

[michaelherger]

Why would you de-reference these two variables and not just use them directly?

[Rouzax]

The dereference is there to normalise the shape once so the rest of the loop can treat it as a plain list. Two concrete reasons:

• $attributes->{"MUSICBRAINZ_${tag}_ID"} / ...SORT can arrive as a scalar (single MBID from a legacy singular tag), an arrayref (Picard multi-value), or undef, independently of whether the plural names tag is an arrayref. Flattening to @mbids / @sorts collapses all the "not a usable list" cases to an empty array, so $mbids[$i] at line 3189 is safe regardless of input shape.
• The boolean checks if @mbids (3189) and @mbids ? ... : $brainzID (3194) mean "has at least one element". Using $pluralMbid directly has the wrong truthiness: an empty arrayref is truthy, so if ($pluralMbid) would wrongly enter the branch for []. I'd need ref($pluralMbid) eq 'ARRAY' && @$pluralMbid at every use site.

If you'd rather keep arrayrefs throughout for consistency with the rest of the function I'm happy to switch, but the guards at every use site would get noisier.

[Rouzax]

PS: I'm signing out for the day and will be away the coming days so won't be able to respond directly

[mikeysas]

Per my comment on your issue #1555, I believe there is more to consider than just the scanner (if I am understanding your proposal here.

I added my thoughts in reply to your forum post here (for the broader audience to pick apart):
https://forums.lyrion.org/forum/user-forums/ripping-encoding-transcoding-tagging/1816764-lms-metadata-scaan-overrides-local-tags?p=1819374#post1819374

[Rouzax]

Pausing this PR while a design question plays out on the forum. Summary below; full discussion and feedback is on the forum thread referenced in #1555.

What I got wrong: PR body design note 2 claimed "skin-visible names are unchanged". I verified the DB contents after the scanner changes but not the rendered output. Tested the live JSON API against fixture 01 (ALBUMARTIST="Armin van Buuren & KI/KI", ALBUMARTISTS=["Armin van Buuren","KI/KI"]): the albums query with tag a returns "artist": "Armin van Buuren", not "Armin van Buuren & KI/KI". The user-written joined credit is dropped.

Why the regression exists (verified in code, not speculated):

• Slim/Control/Queries.pm:902 — album a tag emits contributors.name from the single-FK join JOIN contributors ON contributors.id = albums.contributor (line 649).
• Slim/Schema.pm:1289 — scanner sets albumHash->{contributor} to the first entry of the returned contributor list, which after this PR is the first of plural ALBUMARTISTS.
• Slim/Control/Queries.pm:6515 — tracks a tag reads tracks.primary_artist (a stored FK column). Same shape.
• Slim/Schema.pm:1798 — scanner sets primary_artist from $contributors->{ARTIST}->[0] or $contributors->{TRACKARTIST}->[0], again first of plural.

There is no existing column on albums or tracks that stores the raw display string, and no method anywhere that reconstructs a joined display from the individual contributors (LMS does not store joinphrases). So the scanner has no way to preserve the credited display short of a schema addition.

What is already fine: Classic Skin templates (HTML/Default/status_list.html, HTML/Default/browsedb.html) iterate over album.contributors / artistsWithAttributes, which go through the contributor_album many-to-many. They already render the individual contributors correctly after this PR's scanner changes. The aa / A / S tag variants in the API also return multi-contributor lists and are unaffected. The regression is specific to the single-valued a tag consumed by Material Skin, jsonrpc clients, and the CLI.

Proposed redesign (open for feedback):

• Add columns albums.display_artist and tracks.display_artist (schema bump, migration).
• Scanner writes the raw singular ALBUMARTIST / ARTIST string verbatim into the new columns.
• Scanner continues to use plural ALBUMARTISTS / ARTISTS as the authoritative contributor index (what this PR already does well).
• albumsQuery / titlesQuery wire the a tag output to COALESCE(display_artist, contributors.name). Primary contributor FK unchanged.
• Migration populates display_artist from the current primary-contributor name on first upgrade, then rescans refine it from the raw tag. Non-destructive.

This supports the two use cases raised on the forum: the common "Bill Evans Trio with Scott LaFaro & Paul Motian" migration case where the joined credit is the display string and the three individuals are the indexable contributors; and the "Prince" vs "Prince and the Revolution" case where the user deliberately diverges index from display.

Design questions are on the forum thread (linked from #1555). Looking for feedback on column naming, storage policy (always-store vs store-only-when-different), and scope before writing code.

Status of the two PRs:

• #1555 - Stop-gap: drop MusicBrainz IDs when artist name and MBID counts differ #1556 (length-mismatch stop-gap) is already merged, so the defensive mitigation is in tree independent of this discussion.
• This PR (#1555 - Use plural ALBUMARTISTS / ARTISTS as authoritative source of individual contributors #1557) will be replaced by the structural fix once the design is agreed. Keeping it open as the reference for the scanner-side plural-tag logic that stays in the replacement PR.

[darrell-k]

See my comments on the forum: https://forums.lyrion.org/forum/user-forums/ripping-encoding-transcoding-tagging/1816764-lms-metadata-scaan-overrides-local-tags?p=1819451#post1819451

I'm not trying to pour cold water on this, just making sure we've thought everything through.

[Rouzax]

See my comments on the forum: https://forums.lyrion.org/forum/user-forums/ripping-encoding-transcoding-tagging/1816764-lms-metadata-scaan-overrides-local-tags?p=1819451#post1819451

I'm not trying to pour cold water on this, just making sure we've thought everything through.

Please do, I thought I had thought about everything but @mikeysas already pointed out a big gap I missed.
Would prefer to get all feedback and angles that I did not tbink about.