Alternative artwork/box set approach

[darrell-k]

Here is an alternative approach to #1513

It fixes a couple of existing new&changed artwork scanning problems including embedded artwork not being removed after being removed from the music file and issues I found with albums in multiple directories.

I've tried to implement the functionality of #1513 but I have not yet coded the "file/folder name to disc number" heuristics or the discsubtitle processing. But these should be relatively straightforward additions to the code structure in this PR.

I've also ensured that for box sets, if a particular disc doesn't have specific artwork, we fall back to the overall album artwork.

Needs much more testing, not just by me!

Comments please.

[michaelherger]

Copilot's thoughts (Concern #4 is what made me raise an eyebrow):

Concerns & Observations:

1. Database query complexity - The SQL GROUP BY now includes dirname and tracks.disc, which could significantly change grouping behavior and potentially impact performance. This warrants testing with large libraries.

   GROUP BY tracks.cover, tracks.album, dirname, tracks.disc

2. Unclear variable initialization - In updateStandaloneArtwork(), $newCover and $newAlbumCover are declared but not always initialized before use. This could lead to undefined behavior.

3. Incomplete feature - As noted in the PR description, the "file/folder name to disc number" heuristics aren't implemented yet. This makes the current solution partially functional.

4. CoveridGeneration refactoring - The changes to generateImageId() logic in Slim/Schema.pm and Slim/Schema/Track.pm distinguish between embedded vs. external artwork, which is good for deduplication. However, this logic is duplicated across two files—could be consolidated.

5. Potential query performance issue - The new $sth_album_urls query executes for each album in the loop. Consider caching results or using a larger batch query to avoid N+1 queries.

6. Edge case: empty parent artwork - When _findCommonParent() returns undef for multiple directory paths, the code sets $parentArtwork = undef and $parentArtworkId = undef. The downstream logic should clearly handle this case.

Testing Recommendations:

• Box sets with multiple directories - Critical test case
• Disc-specific artwork detection - Verify the naming patterns are recognized
• Cache behavior - Ensure the new disc-specific caching doesn't cause memory issues
• Performance - Run on a large library to catch any query bottlenecks
• Embedded vs. external artwork - Verify the cover ID deduplication works correctly

Minor Issues:

• Line in updateStandaloneArtwork(): The condition $urlDir ne dirname($cover) assumes $cover is always a valid file path—add safety checks.
• Missing comments explaining the new $commonParent parameter's purpose in findStandaloneArtwork().

Overall, this is a thoughtful approach to a complex problem, but it would benefit from the noted refinements before merging.

[darrell-k]

On the concerns:

1. Definitely. But some performance hit is inevitable if we're going to go down this road.
2. As I understand it, by declaring them at the top of the fetch loop, they're always starting of undef each time through, which is what we want. But I could declare them explicitly undef it that makes things clearer.
3. Yes, it's just telling me what I already said!
4. It's a simple enough if statement, but yes, why not consolidate it? I'll do this.
5. A memory cache of all albums to be processed could be very large in updateStandaloneArtwork, smaller in precacheAllArtwork. We have an index on tracks.album so I suggest leaving it alone unless performance testing highlights any big problems.
6. Would only happen if the user has an album split across different file systems. Though considering this has made me wonder how dirname() handles symbolic links. Having said that, _findCommonParent needlessly returns undef if called with a single path. I might change this so that it just returns the input parameter in that case - safer if someone in the future uses it a bit carelessly.

On testing:

Except for performance testing, I've tested the other stuff to the point of boredom, but I intend to do some more testing and it's a good checklist.

Given the almost infinite combinations of adding/removing/changing artwork both at the disc/track level and album parent level, we can only do our best.

Once we've verified performance is OK and it passes regression testing (ie current behaviour for the simple cases of changing artwork in a single album folder and embedded artwork) I'd suggest merging it to 9.2 for user feedback.

On minor issues:

$urlDir : the test is already protected by Slim::Music::Info::isFileURL($url).
Missing comments: yes that should be explained.

[mikeysas]

@darrell-k - happy to help testing when you are ready for that.

I have some test cases set up from before. I was testing with embedded artwork where I had one with artwork embedded that varied by disc and another with embedded artwork varied by Grouping all with the “box set” artwork as cover.jpg.

[darrell-k]

@darrell-k - happy to help testing when you are ready for that.

I have some test cases set up from before. I was testing with embedded artwork where I had one with artwork embedded that varied by disc and another with embedded artwork varied by Grouping all with the “box set” artwork as cover.jpg.

Thanks for the offer.
Replace these files in your 9.2 installation

• Slim/Control/Commands.pm
• Slim/Music/Artwork.pm
• Slim/Schema.pm
• Slim/Schema/Track.pm

with the versions from my branch https://github.com/darrell-k/slimserver/tree/artwork

Start with a full rescan to ensure a good starting point. Then put the new & changed scan through its paces.

[darrell-k]

@mikeysas I should have reminded you that discsubtitle matching and file name heuristics are not implemented yet. I'm working on it now.

[michaelherger]

Wow... that guy's tricky! Could you please uppercase SQLite keywords/functions for readability? Thanks!

[michaelherger]

BTW: this seems to be incompatible with MySQL. I know... we kind of no longer support it. There's no reasonable way to handle this in Perl, is there? Maybe that's the final nail in the coffin of MySQL support.

[darrell-k]

Apparently, something like
SELECT LEFT(url, CHAR_LENGTH(url) - LOCATE('/', REVERSE(url))) from tracks;
would work in MySql, but not Sqlite.

Or we could write a perl UDF.

[michaelherger]

Don't know whether MySQL (or the Perl MySQL driver) supports UDFs. But let's ignore this for now. We can always try to fix this with the help of the guy who insists on using MySQL.

[darrell-k]

Initial performance tests show quite a hit (40 seconds combined for updateStandaloneArtwork and precacheArtwork combined for the 40,000 track test library, compared to 1 second for current 9.2!

I'll work on this tomorrow.

[mikeysas]

I saw this when testing the previous version also and suspect this needs to be addressed in Material....

NOTE:
cover.jpg

Embedded artwork in the first track

When you click on box set release from any of the browse menus or search results, it displays correctly:

When you click on the release name in the Now Playing screen or on the playlist queue note that the embedded artwork from first track displays on the top left:

[darrell-k]

@mikeysas Yes, this can be resolved by Material sending an extra tag (J) in the tracks command and then using the returned album image id (which is confusingly called artwork_track_id).

There's a related issue: the artist shown in the album heading is the lead artist of the track, rather than the lead artist of the album. This will require an LMS change, and possibly another Material change, so I'll hold off creating a PR for Material until I've decided how to approach the LMS query amendment, which is probably going to need an extra join to the contributors table for the name.