redesign of updateStandaloneArtwork() by darrell-k · Pull Request #1637 · LMS-Community/slimserver

darrell-k · 2026-08-15T20:27:26Z

As discussed. I hope it all makes sense.

The diff generated by git for updateStandaloneArtwork() is a bit of a mess, probably best to view the new routine as a complete replacement for the old one.

This redesign enhances the new scanned_pics table so that it can drive updateStandaloneArtwork().

I've added acoverid column so that we can read it directly from the table (in the scanner process) when we need to update tracks or albums. In order for this to work, all external coverid generation will now use the image path, not the music file URL.
There is a new status column so we can differentiate new, existing and deleted images.
The url column is renamed to path as it will now hold the file system path of the image, not a file:// URL. This makes things much easier.
There is a new dir column as discussed.

In performance testing, this runs faster, even though we are now calling findStandaloneArtwork() for every track where an image change has been detected, rather than only once for each album/image group.

This change enables TitleFormatter to do its work correctly in cases when the user has specified a variable cover id which includes a "sub-album" field like discnumber or grouping. This means that disc or grouping-specific images can be applied to tracks using this existing mechanism when everything for the album is in the same directory.

I've added some comments to new/changed code in order to aid understanding.

I'm sure at this stage there is stuff I've missed.

Signed-off-by: darrell-k <darrell@darrell.org.uk>

michaelherger

Thanks a lot! I hope to find time to actually test this later today. All my comments are just of theoretical nature. Haven't even pulled this change yet. Bear with me.

michaelherger · 2026-08-16T06:31:08Z

 		# XXX how best to delete files in non-recursive mode?
 		# Delete the directory itself and all children
 		$dbh->do("DELETE FROM scanned_files WHERE url = '${file}' OR url LIKE '${file}/%'");
+		$dbh->do("DELETE FROM scanned_pics WHERE dir LIKE '${path}/%'");


Is there a need for this? I believe the pictures table has a different use than the files: the latter really is there to iterate over and process all records. The former (scaned_pics) is a helper to look up things for the tracks. In my plans/ideas this will be more than just cover artwork, but eg. artist pictures too. We shouldn't delete that data before we're really done. Wouldn't we potentially need it at a later stage to look up box set artwork, too?

BTW: I first wanted to complain about the use of variables in the SQL statement, instead of using prepared statements. That's a typical target for SQL injection. A folder name of drop table <table name>; -- or similar could potentially cause harm... something we should probably clean up at some point. But please try to avoid using variables potentially containing user data as much as possible.

Probably not. Also with the introduction of schema_scanner.sql you're also recreating scanned_files so the existing DELETE could be removed, too.

But this has prompted a thought: without the change which I assumed was temporary for debugging, to not run schema_scanner.sql unless we're in the scanner process, we'll also clear scanned_files when schema.pm is initialised in the main process. At the moment scanned_files remains populated until a full rescan. Might this affect things like autorescanning?

michaelherger · 2026-08-16T06:58:31Z

+### I might have missed it, but I can't see where this might be called in main process async mode.
+### If it is, we'll need more work to populate scanned_pics in the main process or just keep a version of the old subroutine for that use.


Please don't remove this just yet... I'm a bit anxious we might be missing something. I want to double check this.

Please leave that code in, commented out if you want. Until we are certain.

Signed-off-by: darrell-k <darrell@darrell.org.uk>

michaelherger · 2026-08-18T15:14:01Z

+CREATE INDEX scannedPicDirIndex ON scanned_pics (folder);
+create index scannedPicStatusidx on scanned_pics(status);
+
+CREATE INDEX IF NOT EXISTS trackscoveridx ON tracks(cover);


Should we add this to one of the versioned files, too? If it's only used in the scanner (for now) we can probably get away adding it to the latest existing up files, avoiding another full wipe & rescan.

That reminds me, I changed the INSERT to check tracks using coverid rather than cover, in case the user has updated an image without changing the file name. So I don't think this index is required any more.

https://github.com/darrell-k/slimserver/blob/4da574ef3d256ac970f2baeb026895dd28f535b8/Slim/Utils/Scanner/Local/Async.pm#L50-L57

You still might be using it around the update album artwork to first track coverid for remote and embedded images. query. But as that's also using album ID and coverid checks, I'm not sure it has much of an impact? Did you measure performance?

In any case: if you wanted to keep it, please name it, as it's not an index on the cover ID.

And for all index statements add a space or remove it everywhere between the table name and the index field 😉 .

"idx" means index. I'll change it to "index"

Hah! Right! Keep the naming convention and upper/lower/camel casing consistent to help my aging and tired eyes and brain.

Signed-off-by: darrell-k <darrell@darrell.org.uk>

…de I used Signed-off-by: darrell-k <darrell@darrell.org.uk>

Signed-off-by: darrell-k <darrell@darrell.org.uk>

darrell-k · 2026-08-22T23:14:41Z

Some rework just pushed.

michaelherger

I'm sorry, another sh..load of comments. I haven't even test run this yet, but I believe there's potential for performance optimisations on the DB level.

michaelherger · 2026-08-24T15:38:15Z

 use Slim::Utils::OSDetect;

 use constant MAX_RETRIES => 5;
+use constant IS_SQLITE => (Slim::Utils::OSDetect->getOS()->sqlHelperClass() =~ /SQLite/ ? 1 : 0);


This is no longer used.

michaelherger · 2026-08-24T15:42:26Z

+				# Maybe a track instance was passed in, but no longer from updateStandaloneArtwork() which gives us
+				# the trackid instead, as we only need to instantiate a track if 'titleformatter' artwork naming is in use.
 				my $track = $trackAttributes && delete $trackAttributes->{_track};
+				$track ||= Slim::Schema->find('Track', $trackAttributes->{_trackid}) if $trackAttributes->{_trackid};


Can we delete the _trackid element here?

michaelherger · 2026-08-24T15:45:24Z

+### I might have missed it, but I can't see where this might be called in main process async mode.
+### If it is, we'll need more work to populate scanned_pics in the main process or just keep a version of the old subroutine for that use.


Please leave that code in, commented out if you want. Until we are certain.

michaelherger · 2026-08-24T15:47:31Z

+			UPDATE albums
+			SET artwork = tracks.coverid
+			FROM tracks
+			WHERE tracks.album = albums.id
+			AND ( tracks.cover IS NULL OR CAST(CAST(tracks.cover AS INTEGER) AS TEXT) = tracks.cover OR tracks.cover LIKE 'https%' )
+			AND ( tracks.coverid <> albums.artwork OR albums.artwork IS NULL )
+		} );


formatting police alert: please move one out to align with the $dbh->do()

michaelherger · 2026-08-24T15:47:42Z

+	### I considered adding rows to scanned_pics for these images so that they'd be processed in the loop below, but I think this is more efficient.
+	#there's a different syntax for MySql.


Remove outdated comment

michaelherger · 2026-08-24T16:29:52Z

+	my $sth_update_tracks = $dbh->prepare( qq{
+		UPDATE tracks
+		SET    cover = ?, coverid = ?, cover_cached = NULL
+		WHERE  id = ?
+	} );


Can we optimise this? We would run an update for each track individually. But couldn't we update all tracks of an album in one update query? Hopefully (to be confirmed) the check in the loop would then no longer enter the conditional update, as the cover would already be updated on the remaining tracks of the album?

If the user is using TitleFormatter for variable names, then they could be different covers across the album (DISC, PERFORMANCE, GROUPING...).

michaelherger · 2026-08-24T16:30:56Z

+				if ( $track->{cover} ne $newCover ) {
+					my ($newCoverid) = $dbh->selectrow_array($sth_scanned_pics, undef, $newCover);
+					$sth_update_tracks->execute( $newCover, $newCoverid, $track->{id} );


See my comment where we define $sth_update_tracks: if we updated all the tracks of the album in a single update query, wouldn't this condition skip processing all the tracks individually?

michaelherger · 2026-08-24T16:34:31Z

 		# XXX how best to delete files in non-recursive mode?
 		# Delete the directory itself and all children
 		$dbh->do("DELETE FROM scanned_files WHERE url = '${file}' OR url LIKE '${file}/%'");
+		$dbh->do("DELETE FROM scanned_pics WHERE folder LIKE '${path}%'");


Is there really a need to delete this? I'd really like to use the same table for other purposes to avoid another scan for images for the contributor artwork.

Probably not. But see my previous comment regarding scanned_files: #1637 (comment)

michaelherger · 2026-08-24T16:35:11Z

 	Slim::Utils::SQLHelper->executeSQLFile(
 		$driver, $class->storage->dbh, "schema_scanner.sql"
-	);
+	) if main::SCANNER; ### temporary, so we keep the contents from the last scan for debugging


still needed?

It's still proving useful for development. Also see #1637 (comment) (which I just mentioned elsewhere as well)

michaelherger · 2026-08-24T16:35:41Z

 		$columnValueHash{cover} = $cover;
 	}

+#	if ( $columnValueHash{cover} =~ /^https?/ || $columnValueHash{cover} =~ /^\d+$/ ) { ###combine the regex if this works!!!


Obsolete idea, needs removing!

darrell-k · 2026-08-24T17:12:07Z

Just to let you know, I'm currently testing the TitleFormatter variable artwork stuff and have found two problems with DISC:

In a full rescan when findStandaloneArtwork is called from Schema.pm, if total discs/disc count/discc is not in the tags therefore not in $deferredAttributes, TitleFormatter doesn't return anything for the DISC variable. It's OK in a n&c scan because we've got a database object by the time it's called via updateStandaloneArtwork and have written albums.discc regardless of whether it was tagged or not.
I've got some test files with a leading zero in the disc number tag, which also screws it up in a full rescan (no artwork returned again with either eg 1.jpg or 01.jpg in the album folder).

darrell-k · 2026-08-24T17:21:26Z

I'm going to resolve some of the comments in this thread, it's getting hard to follow!

redesign of updateStandaloneArtwork()

50c8587

Signed-off-by: darrell-k <darrell@darrell.org.uk>

michaelherger reviewed Aug 16, 2026

View reviewed changes

darrell-k added 2 commits August 16, 2026 15:09

some fixes

af8169b

Signed-off-by: darrell-k <darrell@darrell.org.uk>

rename scanned_pics columns

4da574e

Signed-off-by: darrell-k <darrell@darrell.org.uk>

michaelherger reviewed Aug 18, 2026

View reviewed changes

darrell-k added 3 commits August 18, 2026 17:31

add MySql support

6a5a6bd

Signed-off-by: darrell-k <darrell@darrell.org.uk>

simplify SQLITE online artwork update, I misunderstood the example co…

7c62c4d

…de I used Signed-off-by: darrell-k <darrell@darrell.org.uk>

fix addition & removal of embedded artwork

faafb9f

Signed-off-by: darrell-k <darrell@darrell.org.uk>

darrell-k commented Aug 19, 2026

View reviewed changes

Comment thread Slim/Music/Artwork.pm Outdated

darrell-k commented Aug 19, 2026

View reviewed changes

Comment thread Slim/Schema.pm

michaelherger reviewed Aug 20, 2026

View reviewed changes

Comment thread Slim/Music/Artwork.pm Outdated

Some rework

d6932bd

Signed-off-by: darrell-k <darrell@darrell.org.uk>

michaelherger reviewed Aug 24, 2026

View reviewed changes

		### I might have missed it, but I can't see where this might be called in main process async mode.
		### If it is, we'll need more work to populate scanned_pics in the main process or just keep a version of the old subroutine for that use.

		### I considered adding rows to scanned_pics for these images so that they'd be processed in the loop below, but I think this is more efficient.
		#there's a different syntax for MySql.

Uh oh!

Conversation

darrell-k commented Aug 15, 2026

Uh oh!

michaelherger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darrell-k commented Aug 22, 2026

Uh oh!

michaelherger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrell-k commented Aug 24, 2026

Uh oh!

darrell-k commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants