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.

[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.

I've got it down to 5 seconds for this test case.

I conditioned a couple of SQL UPDATE statements to stop them being issued unnecessarily, but the big improvements are:

• Just pass the track url & disc number to findStandaloneArtwork() rather than instantiating a Track instance. If the user has a wildcard image name preference, a Track will still be instantiated by Slim::Music::TitleFormatter::infoFormat() but no longer in other cases.
• Process the album level artwork after all rows for an album have been fetched, rather than when we first see a new album. This allows us to build an array of album folder paths on the way through rather than hitting the database with an extra fetch for each album to get them.

I also tried replacing the dirname SQL grouping with Perl code, but it added a lot of complication and only saved 1 second. Turns out Sqlite is quite good at such things!

I'm going to look at whether similar efficiencies are useful in precacheAllArtwork() but pushing these changes now to allow review and testing of the new & changed scan.

@mikeysas please check I haven't broken anything with the latest changes if you get the chance. You only need to refresh your Slim/Music/Artwork.pm from my branch to pick up this change.

I also need to make sure I haven't broken online albums, and also test the async call from the main process.

[michaelherger]

How did you profile? I've found NYTProf to be super helpful. Once you've installed it on your system you do:

perl -d:NYTProf slimserver.pl

[run a scan, then stop LMS - the data can be huge!]

nytprofhtml -f NYTProf/nytprof.out -o NYTProf/smallartwork --open

Which should give a web page where you can drill down to hot spots etc.

DBIx unfortunately is known to tend to be slow. You could avoid it in infoFormat() if you provided metadata known from the scan. I see that you sometimes do this. Maybe it would be worth using raw SQL to get it for the other cases, rather than rely on infoFormat. That really was intended to render title information of the currently playing track - which doesn't change often.

[darrell-k]

Timings from the scan messages. Experimented by commenting out different bits of the code. As you say, the DBIx stuff was a prime suspect. Thanks for the tool recommendation, I'll try it out.

I'll look at further changes to ensure that DBIx is never used in findStandaloneArtwork().

[darrell-k]

DBIx unfortunately is known to tend to be slow. You could avoid it in infoFormat() if you provided metadata known from the scan. I see that you sometimes do this. Maybe it would be worth using raw SQL to get it for the other cases, rather than rely on infoFormat. That really was intended to render title information of the currently playing track - which doesn't change often.

I've been struggling a bit with this, trying to create a compatible hash with raw SQL, but I just found that the variable artwork name stuff (I'm testing with an artwork format of %ARTIST - ALBUM) doesn't work in current 9.2 with the metadata hash provided by Slim::Music::Artwork::updateStandaloneArtwork:

[26-04-02 17:00:52.4029] Slim::Music::Artwork::findStandaloneArtwork (113) No variable cover Slim::Schema::Album=HASH(0x564720957b08).jpg found from ARTIST - ALBUM

Or am I missing something?

[michaelherger]

The failure: is this with stock 9.2? And 9.1 would be correct? There aren't too many changes in 9.2 yet.

[darrell-k]

I've examined the code and the source history. I can't see how the hash created in Slim::Music::Artwork::updateStandaloneArtwork can ever have worked for variable title format fields other than those in the tracks table since it was introduced 13 years ago!

I dumped the hash received by Slim::Music::TitleFormatter and the album columns are within an embedded Slim::Schema::Album object and I can't see the code ever having dealt with that. And artist/albumartist/trackartist/contributor.name are not there in any form.

[michaelherger]

Are you saying that if I define ARTIST - ALBUM as the artwork pattern, then "Elvis - Love Me Tender.jpg" would never have been found?

[michaelherger]

Oh, is the answer in the code already?

slimserver/Slim/Music/Artwork.pm

Line 86 in 9508fbe

# XXX This may break for some people as it's not using a Track object anymore

So this would have been broken for >16 years, for anyone trying to use more than just track title. The artist would not even be referenced in that data structure...

[michaelherger]

Hmm... and it's different whether you run a full wipe & rescan, or just a rescan. In the former case we'd get a flattened $meta object, including artist information. In the latter we don't. So the good news is it is not consistent, working sometimes 😁. It depends on the caller. Maybe we should look into fixing them? I guess we did too many optimisations over the years, without paying much attention to this case.

[darrell-k]

Yes, I can fix it now we've confirmed it is broken.