redesign of updateStandaloneArtwork()

[darrell-k]

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.

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

[darrell-k]

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]

Can we delete the _trackid element here?

[michaelherger]

@darrell-k?

[darrell-k]

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]

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

[darrell-k]

I'm about to push an update.

The scanned pics SQL now identifies the albums that need processing so that all tracks can be processed together even if the album's tracks are located in multiple folders. This makes it simpler, especially when we come to enhance this to look for specific album artwork (eg box sets).

I hope I've also covered all the comments above.

I'm ready for more questions/comments!

[michaelherger]

I'm so utterly confused... does this change even touch the full wipe & rescan case? And didn't we change the code (in 9.2 or before) to use cover.jpg over a track's embedded artwork for an album? I just spent an hour or more trying to figure out why one of my albums would show the first track's cover, instead of cover.jpg. Just to then figure out that it's the same in 9.x...

[michaelherger]

This has been sitting in my computer for way too long. And I'm still rather confused... these changes only deal with updated artwork on a rescan, but not the initial scan? Is this because we're happy with the initial scan at this point (except for boxsets)?

And as I mentioned in some other comment we don't handle album artwork independently of the track artwork yet? I'm very much confused because I had retagged some of my files about a month ago to test this particular case.

EDIT: ok, it seems the album artwork vs. track artwork was part of my attempt to deal with boxsets in #1627. Aargh... just wasted half an afternoon 😞.

[michaelherger]

I did what people do nowadays: I asked the AI assistant. It told me that the easiest way to have the scan run in the main process was to do a rescan album <album_id>. I'll give that a try.

[darrell-k]

I don't think that would call updateStandaloneArtwork and Schema.pm will still calculate a coverid if it can't find the image in scanned_images (if I remember correctly!).

[darrell-k]

these changes only deal with updated artwork on a rescan, but not the initial scan? Is this because we're happy with the initial scan at this point (except for boxsets)?

Yes (until we get on to the box set changes), except that in Schema.pm we now read coverid directly from the enhanced scanned_files table rather than recalculating it.

And unless your refactoring of findStandaloneArtwork changes anything for a full scan.

[michaelherger]

This PR is still in draft mode. How do you feel about it? I think we/I should try to get things moving again. Shall I merge?

[darrell-k]

Let me address the outstanding comments first. Give me a day.

And then, you mean merge into your artwork-scan-db branch and not 9.2?

I think we should do the box set stuff before merging to 9.2 (it should be easy to incorporate into updateStandaloneArtwork with the new logic, I've already started, but am holding back for a separate PR after we got the basic functionality agreed/finalised).

[darrell-k]

So I'm much preferring option 1. above. What are the problems you foresee with that approach?

Quoting what you already quoted:

As I mentioned before I'd like to use the same table for contributor picture lookups, where we currently do a ton of directory reading etc. Using the database should be so much more cleaner and efficient.

If we removed data from that table, the contributor picture scan would have to populate it again. It's too early to drop the data. We could drop it as part of the "optimize" step?

I'm not understanding.
Option (2): clearing the table at the end of the scan means it starts off empty for all scan types.

Option (1) would follow what 9.2 is doing for scanned_files:

• the table is only cleared at the start of a full rescan.
• in a n&c scan it stays populated but rows are deleted for the folders being scanned before they are scanned/added to the table.

Either way, the data would be available to the contributor picture scan.

• in a n&c scan it stays populated but rows are deleted for the folders being scanned before they are scanned/added to the table.

Are you referring to this line? https://github.com/LMS-Community/slimserver/blob/public/9.2/Slim/Utils/Scanner/Local.pm#L66

Yes, and also

slimserver/Slim/Control/Commands.pm

Lines 2760 to 2778 in c96f8f2

	my $dbh = Slim::Schema->dbh;
	my $sth = $dbh->prepare_cached('DELETE FROM scanned_files WHERE url = ?');
	my @paths = Slim::Utils::Misc::uniq(
	map {
	# reset the track's timestamp so changes are certainly picked up
	$_->timestamp(0);
	$_->update;
	# delete entry in scanned_files - otherwise rescan doesn't handle deletions for non-recursive scans
	$sth->execute($_->url);
	dirname(Slim::Utils::Misc::pathFromFileURL($_->url));
	} @tracks
	);
	# need to delete the entry for the folder, too
	foreach (@paths) {
	$sth->execute(Slim::Utils::Misc::fileURLFromPath($_));
	}

[michaelherger]

Ok, a month or two later I finally get it 🤦🏻.

I'm not sure we should just add things there as deletion of found artwork references might actually be a bit more complex than tracks once we add box set handling, works etc. as we will have to look at parent folders too. We should probably not delete by folder, but by cover of the tracks inside a folder?

Do we populate the scanned_pics during a single album or track (non-recursive) scan?

[darrell-k]

Ok, a month or two later I finally get it 🤦🏻.

I'm not sure we should just add things there as deletion of found artwork references might actually be a bit more complex than tracks once we add box set handling, works etc. as we will have to look at parent folders too. We should probably not delete by folder, but by cover of the tracks inside a folder?

If we were to clear the data from scanned_pics in schema_optimize, when it hits those two points there'll be no rows for the folder(s) in question anyway.

Do we populate the scanned_pics during a single album or track (non-recursive) scan?

Yes for a single album scan, no for a single track scan, because we bypass Slim::Utils::Scanner::Local::Async->find() in that case:

slimserver/Slim/Utils/Scanner/Local.pm

Lines 71 to 110 in c96f8f2

	if ( -f _ ) {
	# A single file was passed in, handle it directly here
	my $types = Slim::Music::Info::validTypeExtensions( $args->{types} || 'audio' );
	if ( Slim::Utils::Misc::fileFilter( dirname($path), basename($path), $types, 0 ) ) {
	# Add single file to scanned_files
	my $sth = $dbh->prepare_cached( qq{
	INSERT INTO scanned_files
	(url, timestamp, filesize)
	VALUES
	(?, ?, ?)
	} );
	$sth->execute(
	$file,
	(stat _)[9], # mtime
	(stat _)[7], # size
	);
	# Callback that we found 1 file
	$cb->(1);
	}
	else {
	$cb->(0);
	}
	return;
	}
	elsif ( -d _ ) {
	# Scan the directory for files
	if ( $args->{no_async} ) {
	# Force the use of the async find class if not in async mode
	# (it can run it a tight loop, AIO can't)
	require Slim::Utils::Scanner::Local::Async;
	Slim::Utils::Scanner::Local::Async->find( $path, $args, $cb );
	}
	else {
	$findclass->find( $path, $args, $cb );
	}
	}

For the single album scan we are going to have to consider the parent folder question when we get on to box sets.

For a single track scan, I think it's reasonable that artwork changes (unless embedded) is ignored.

I would vote that for this PR we should go with the simple solution I'm suggesting and if it needs further amendment for box sets we can enhance it when we code that.

NOTE: I thought I'd have a look at what 9.2 was doing and just found that it's not handling changed artwork properly anyway! Something else to incorporate in this PR!!!

[michaelherger]

Should we merge what we have, and add more as we get more exposure? We don't have to fix everything in one go.

[darrell-k]

Probably - I'll just check I haven't made anything worse.

[darrell-k]

OK, the artwork problem in rescan album is hopefully fixed by calling Slim::Music::Artwork->precacheAllArtwork after Slim::Utils::Scanner::Local->rescan has run in Slim::Control::rescanCommand.

I'm about to push the changes, hopefully this is ready to merge now, but I want to run all my tests again before I mark this "ready for review". That will be tomorrow afternoon.

[michaelherger]

They say that if you want to ignore only if it already exists, you should use INSERT ... ON CONFLICT DO NOTHING, rather than INSERT OR IGNORE. The latter would silently swallow other failures, like empty key values, which we might be interested to learn about. See eg. https://hoelz.ro/blog/with-sqlite-insert-or-ignore-is-often-not-what-you-want

[michaelherger]

I know, this has been around for 15+ years. But maybe it's time to do this correctly, using a prepared statement? Some paths when expanded this way might fail the query, eg. if they had quotes in them etc.

[michaelherger]

This is a new change. As these are all purely scanner related, do we really want to bump the schema version? Keep in mind that this will force a full new rescan - for the only reason to run new scanner code. That's kind of stupid if all we change is the scanner process, but not the result (hopefully, anyway).

[darrell-k]

On the other hand, IMO it's the "right" way to do it (following scanned_files). But I'm open to other ideas if we can ensure it's only created once, not dropped and recreated.

I could reinstate scannerInitialiseDB (or whatever I called it) from a previous commit. That could check for the table's existence and do nothing if it already exists. Checking for a table's existence is easier now we don't have to worry about mysql, but is it worth it?

[darrell-k]

Or just CREATE TABLE IF NOT EXISTS, of course!

[michaelherger]

You're right: a new schema version file the right thing to do. 9.2 is still in development.

Do we really need the additional index on tracks.cover? I think we never use that criteria without another more restrictive condition like eg. the album. Maintaining the index might take more time than we gain checking all tracks of an album with the help of the index vs. without?

[darrell-k]

I merged in 9.2 locally before this latest push, which was probably a mistake, I forgot this PR was against the https://github.com/LMS-Community/slimserver/tree/artwork-scan-db branch. It also seems to have caused a DCO problem with some of the commits that were in that merge.

And I still have to address the two recent comments about SQL changes.

[michaelherger]

You could git reset <previous commit> --hard or similar, then force push. Or I can do it if you prefer.

[darrell-k]

You could git reset <previous commit> --hard or similar, then force push. Or I can do it if you prefer.

Done. looks better now.

[darrell-k]

@michaelherger I found an inconsistency with TitleFormatter image retrieval using DISC and/or DISCC:

Consider a music file with the following tags:
Disc Number = "01"
Disc Total = "02"
(taggers can add leading zeros, sometimes even if the user doesn't enter the leading zero!)

Now, let's look at 3 scanning scenarios:

1. An unchanged track being processed by Slim::Music::Artwork::updateStandaloneArtwork():
   findStandaloneArtwork() is called and it will obtain a track object from the database if required for TitleFormatter. Because the relevant database columns are numeric, there will be no leading zeros returned.

2. A new track (or all tracks in a full rescan):
   Slim::Schema::_newTrack will call findStandaloneArtwork() with the two hashes: %columnValueHash and $deferredAttributes which hold the raw tag values including any leading zeros.

3. A changed track (N&C scan):
   findStandaloneArtwork() is called by Slim::Utils::Scanner::Local::changed() after it has called Slim::Schema->updateOrCreate() to get a track object. It passes this track object into findStandaloneArtwork(). However this $track is the one being built from the rescanned tags and so will contain leading zeros in $track->disc (but no leading zeros in the inflated album->_column_data).

Therefore, as things stand, there is no correct image naming possible when using DISC and/or DISCC because depending on the scan type we'll be looking for different names.

I've fixed this in Slim::Music::Artwork::__findStandaloneArtwork() by zero-suppressing the filename checks. It will of course zero-suppress all numeric chunks in the strings being compared because there's no easy way to isolate DISC and DISCC. But I don't think this matters.

I am about to push the change to this PR. Please let me know what you think. I've added some comments in the code which hopefully explain my approach. My code could well require some tidying up. Am I doing something crazy?

[michaelherger]

I've fixed this in Slim::Music::Artwork::__findStandaloneArtwork() by zero-suppressing the filename checks. It will of course zero-suppress all numeric chunks in the strings being compared because there's no easy way to isolate DISC and DISCC. But I don't think this matters.

I think you're not happy with this yourself...

We know the title format. So we can do an educated guess: if DISC or DISCC are part of the format string, create two file name candidates (with and without leading zeros)? After all we're building a list of file name candidates. We should consider them candidates like all the other file names, too. How does that sound?

And we not only have the format, we can also "massage" the data as we like, convert numbers to strings etc.

[michaelherger]

Hardly tested - I hope you get the idea:

diff --git a/Slim/Music/Artwork.pm b/Slim/Music/Artwork.pm
index cf328a03a..c7d0938b9 100644
--- a/Slim/Music/Artwork.pm
+++ b/Slim/Music/Artwork.pm
@@ -100,16 +100,33 @@ sub findStandaloneArtwork {
 				if ( my $coverName = Slim::Music::TitleFormatter::infoFormat( $track, $formatStr, undef, $meta ) ) {
 					$coverName .= $suffix;
 
-					if ( main::ISWINDOWS ) {
+					my $addCleanCoverName = sub {
 						# Remove illegal characters from filename.
-						$coverName =~ s/\\|\/|\:|\*|\?|\"|<|>|\|//g;
-					}
+						$coverName =~ s/\\|\/|\:|\*|\?|\"|<|>|\|//g if main::ISWINDOWS;
+
+						# Generating a pathname from tags is dangerous because the filesystem
+						# encoding may not match the locale, but that is the best guess that we have.
+						unshift @files, Slim::Utils::Unicode::encode_locale($coverName) if $coverName;
+					};
+
+					$addCleanCoverName->();
 
-					# Generating a pathname from tags is dangerous because the filesystem
-					# encoding may not match the locale, but that is the best guess that we have.
-					$coverName = Slim::Utils::Unicode::encode_locale($coverName);
+					# DISC/DISCC can come with or without leading zeros, so we need to format them to match the template
+					if ($coverFormat =~ /%DISC/) {
+						$meta ||= { $track->get_columns };
 
-					unshift @files, $coverName;
+						my $formatNumbers = sub {
+							return $_[0] * 1 if $_[0] =~ /^0\d+$/;
+							return sprintf("%02d", $_[0]);
+						};
+
+						$meta->{DISC} = $meta->{disc} = $formatNumbers->($meta->{disc}) if ($meta->{disc});
+						$meta->{DISCC} = $meta->{discc} = $formatNumbers->($meta->{discc}) if ($meta->{discc});
+
+						$coverName = Slim::Music::TitleFormatter::infoFormat( undef, $formatStr, undef, $meta );
+						$coverName .= $suffix;
+						$addCleanCoverName->();
+					}
 
 					if ( $artDir && -d $artDir ) {
 						$candidateForArtfolder = $coverName;

With a format of %DISC - ALBUM this would create a candidates list of eg.:

  "01 - The Best Of 1980-1990 & B-Sides - CD 1 (1998)",
  "1 - The Best Of 1980-1990 & B-Sides - CD 1 (1998)",
  "cover",
  "album",
  "folder",
  "thumb",

[darrell-k]

I've fixed this in Slim::Music::Artwork::__findStandaloneArtwork() by zero-suppressing the filename checks. It will of course zero-suppress all numeric chunks in the strings being compared because there's no easy way to isolate DISC and DISCC. But I don't think this matters.

I think you're not happy with this yourself...

We know the title format. So we can do an educated guess: if DISC or DISCC are part of the format string, create two file name candidates (with and without leading zeros)? After all we're building a list of file name candidates. We should consider them candidates like all the other file names, too. How does that sound?

And we not only have the format, we can also "massage" the data as we like, convert numbers to strings etc.

Well, I'm quite happy. I actually enjoyed coding that :) and I don't think ignoring leading zeros in other format elements would be a problem. Would people really tag "foobar 1" and "foobar 01" in, say, GROUPING and use in their image filenames?

I didn't want to limit it to a specific number of leading zeros. But then we're storing DISC and DISCC in tinyint columns, so I suppose coding for up to two leading zeros would be enough.

I'll look at doing this by passing an array back from Slim::Music::TitleFormatter::infoFormat(). I suppose wantarray would be useful in doing that, so as not to worry about other uses of that function?

[darrell-k]

@michaelherger just saw your suggested code. I'll run it through my tests.

[darrell-k]

I did a little testing,

I had to replace

my $meta = { %{$trackAttributes}, %{$deferredAttributes} } unless $track;

with

my $meta = $track ? undef : { %{$trackAttributes}, %{$deferredAttributes} };

because the extra use of $meta in the new code was somehow leaving it as a ref to its previous incarnation and leading to slightly confusing behaviour (it's a return value from infoFormat()). Is this a quirk of my .... unless with a hash ref? I'd love to know!

And I also had to retrieve $track->album->discc, replacing

$meta->{DISCC} = $meta->{discc} = $formatNumbers->($meta->{discc}) if ($meta->{discc});

with

my $discc = $meta->{discc} || $track->album->discc;
$meta->{DISCC} = $meta->{discc} = $formatNumbers->($discc) if ($discc);

because discc isn't a track column.
I don't think this is enough, we're also missing eg album name, artist names...
I think we really need to be passing an actual track object to infoFormat() when we haven't had $deferredAttributes passed in.

There's also a problem when using DISC and DISCC:

For a new track we get:
("1 - 2", "01 - 02", "cover", "album", "folder", "thumb")
But for an existing changed track we get:
("1 - 02", "01 - 2", "cover", "album", "folder", "thumb")

Anyway, that's enough for tonight, it's bedtime!

[michaelherger]

Maybe we are overthinking this and should just accept the risk of failing some specific formats... because there's more oddities: I believe the title formatter would strip out DISC if there was no DISCC, or no DISC > 1. There's too much logic built in to this function to optimise for the display which don't really fit our needs here. But this has been around for a while - nobody complained. Maybe we can just ignore the leading issue all together?

Would people really tag "foobar 1" and "foobar 01" in, say, GROUPING and use in their image filenames?

You can expect people to do anything, really. Maybe it's "party hits ('01-'09)" referring to years? But sometimes they'll just have to live with certain side-effects of their doing 😉. I'd rather simplify our code and not deal with leading zeros at this point. Or we'll never end this PR trying to cover each and every edge case.

[michaelherger]

Please use scalar for improved readability:

rank{ scalar fileparse($b, qr/\.[^.]*/) } // 999_999;

fileparse would return the first element in scalar context.

[darrell-k]

Maybe we are overthinking this and should just accept the risk of failing some specific formats... because there's more oddities: I believe the title formatter would strip out DISC if there was no DISCC, or no DISC > 1. There's too much logic built in to this function to optimise for the display which don't really fit our needs here. But this has been around for a while - nobody complained. Maybe we can just ignore the leading issue all together?

Would people really tag "foobar 1" and "foobar 01" in, say, GROUPING and use in their image filenames?

You can expect people to do anything, really. Maybe it's "party hits ('01-'09)" referring to years? But sometimes they'll just have to live with certain side-effects of their doing 😉. I'd rather simplify our code and not deal with leading zeros at this point. Or we'll never end this PR trying to cover each and every edge case.

I'm thinking of box sets. After discussion we decided we should use the existing TitleFormatter functionality instead of invent a new mechanism and template rules for dealing with multiple artwork files in the same folder.

DISC would be a very important attribute in that scenario.

"party hits ('01-'09)" would only be a problem if the user had both "party hits ('01-'09).jpg" and "party hits ('1-'9).jpg" in the same folder. In that case, with my zero-suppression technique we'd match both and use the first we found. I think this really is an edge case.

I also think my latest commit has another minor advantage - it simplifies the scanned_pics SQL.

But if you really don't like that approach, I would propose changing TitleFormatter to always zero-suppress DISC and DISCC elements in the returned template.

This would potentially change current behaviour, mainly in a full rescan, but at least it would be consistent.

[michaelherger]

I'm thinking of box sets. After discussion we decided we should use the existing TitleFormatter functionality instead of invent a new mechanism and template rules for dealing with multiple artwork files in the same folder.

DISC would be a very important attribute in that scenario.

Ok, gotcha. I need some more time to look into/think about this.

What I didn't like about your change as the heavy manipulation back and forth of those filenames. I was wondering whether we should have something similar to the NAMESEARCH column, where we "normalise" values. The leading zero might be one such candidate. That way the scan might be heavier, but the search cleaner...

[darrell-k]

I'm thinking of box sets. After discussion we decided we should use the existing TitleFormatter functionality instead of invent a new mechanism and template rules for dealing with multiple artwork files in the same folder.
DISC would be a very important attribute in that scenario.

Ok, gotcha. I need some more time to look into/think about this.

What I didn't like about your change as the heavy manipulation back and forth of those filenames. I was wondering whether we should have something similar to the NAMESEARCH column, where we "normalise" values. The leading zero might be one such candidate. That way the scan might be heavier, but the search cleaner...

To be honest, looking at TitleFormatter I would think that the manipulation I'm doing would be cheaper than multiple calls to TitleFormatter.

However, I'm also finding other inconsistencies in TitleFormatter, for example ARTIST:

• when processing the hashes from Slim::Schema::_newTrack it takes the first of artist, albumartist, trackartist or contributors.name.
• but when processing a track object, is uses Slim::Schema::Track::artists which returns a list of artists and trackartists for the track, which it then concatenates.

I'm working on avoiding all this pain by NOT calling Slim::Music::Artwork::findStandaloneArtwork() at all from Slim::Schema::_newTrack() and Slim::Utils::Scanner::Local::changed(), but instead relying on Slim::Music::Artwork->updateStandaloneArtwork() to do all required artwork processing.

Then we would always be able to retrieve clean Track, Album etc objects for TitleFormatter which would introduce much needed consistency. I also have the feeling it would be more efficient.

It would mean having Slim::Music::Artwork->updateStandaloneArtwork() run in a full scan so we might even be able to combine it with Slim::Music::Artwork->precacheAllArtwork(), saving a scanner step.

This would also make the future addition of looking for images in the common parent folder for multiple folder albums easier, because there would be a single method doing all the work in all circumstances.

I'll create a PR against my own branch so you can have a look.

[michaelherger]

I'm sorry, you're moving too fast. I've been told to slow down. I can't keep up with your pace.

[darrell-k]

I'm not trying to put pressure on. Sorry if it seems that way. I really value your input, but if you would prefer to put this to one side until I mark the PR "Ready for review", that's fine too.

Thanks again.