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:15:12Z

+  dir text,
+  path text NOT NULL,


What is dir vs. path? Is the latter the full path incl. filename?

Maybe this is just my non-EN native speaker problem. But would there be names which better describe what those columns are? Even if it was file_path?

Column name changes pushed.

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:33:26Z

-  filesize int(10)
+  filesize int(10),
+  coverid char(8),
+  status char(1)


please document the possible status values

michaelherger · 2026-08-16T06:41:10Z

-  filesize int(10)
+  filesize int(10),
+  coverid char(8),
+  status char(1)


Please document the possible status values.

I also talked to my bot buddy about this, and how to potentially optimise this. A first suggestion was defining the possible values:

status TEXT NOT NULL CHECK (status IN ('D', 'E', 'N'))

(or whatever the flags!)

Not that it added much to readability or performance, but DB level validation. We probably need NULL, but you get the point.

I was also wondering about using number, and then human readable constants in code. But that would make the query definitions somewhat more cumbersome.

Anyway: if the status is well defined somewhere even I should be able to learn the few characters.

michaelherger · 2026-08-16T06:43:32Z

+				WHERE NOT EXISTS (
+					SELECT path FROM scanned_pics
+					WHERE scanned_pics.path = tracks.cover
+				)


Would we need a clause here to exclude online pictures (imported from music service) from being considered deleted?

It's there immediately below. Also excludes embedded covers (the numeric check) and includes only tracks within the currently processing base directory:

AND cover NOT LIKE 'https%' AND CAST(CAST(cover AS INTEGER) AS TEXT) <> cover AND url LIKE '$basedir%'

Oh my... 🤦🏻.

Hopefully SQLite is smart enough to do these cheap checks before doing the path lookup in scanned_pics.

Why would you have to do the double casting?

Oh my... 🤦🏻.

Hopefully SQLite is smart enough to do these cheap checks before doing the path lookup in scanned_pics.

Why would you have to do the double casting?

The is no 'is numeric' function in SQLITE. But if the value survives being cast to integer and back again, it is numeric.

michaelherger · 2026-08-16T06:48:38Z

 		VALUES
-		(?, ?, ?)
+		(?, ?, ?, ?, ?,
+		CASE WHEN (SELECT COUNT(*) FROM tracks WHERE tracks.cover = ?) = 0 THEN 'N' ELSE 'E' END


I guess that's the reason for the new index on cover?

Doing a full count might be easier to read, but it's somewhat wasteful, as the DB would always have to count all the occurrences, even if we're only interested in the existence of at least one record.

CASE WHEN ( SELECT EXISTS ( SELECT 1 FROM tracks WHERE tracks.cover = ? ) ) THEN 'N' ELSE 'E' END;

Supposedly is more efficient.

Yes, I usually would use EXISTS, don't know what happened here. But this needs changing anyway because we need to check the image hasn't been updated with another of the same name. I'll be pushing a fix soon.

michaelherger · 2026-08-16T06:57:18Z

+				$file,
+				$mtime,
+				$size,
+				substr( safe_md5_hex( $file . $mtime . $size ), 0, 8 ),


Could you please add another helper for this in Artwork.pm, and use it wherever we do this calculation? It's so specific and non-obvious, having an understandable function name would not only help making sure we're always doing the same thing, but also reading the code. I had to search existing code to understand what this was.

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.

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

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>

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

Uh oh!

Conversation

darrell-k commented Aug 15, 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!

darrell-k Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

darrell-k Aug 16, 2026 •

edited

Loading