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]

I've been testing the rescan album command. It does populate the scanned_pics table - this might have been the cause of the duplicates you saw. I've made the full_path column unique as you suggested and amended the INSERT... statements to INSERT OR IGNORE....

Now we have a choice:

1. Keep scanned_pics around after full/n&c scans but remove rows related to the rescanned album in Slim::Commands::rescanCommand as it already does for scanned_files; or
2. Clear scanned_pics after full/n&c scans (if we choose this, need to take care that it would happen even after a failed scan).

What do you think?

After resolving this question, I'll push changes which hopefully cover all the comments I've resolved in the thread.

[michaelherger]

1. Keep scanned_pics around after full/n&c scans but remove rows related to the rescanned album in Slim::Commands::rescanCommand as it already does for scanned_files; or
2. Clear scanned_pics after full/n&c scans (if we choose this, need to take care that it would happen even after a failed scan).

If I understand your options correctly, then I'd prefer #2. I understand this would drop the table after the scan. 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.

BTW: I know, I asked you to keep the old updateStandaloneArtwork around. But I changed my mind. Just drop it. Git doesn't forget: if we need to go back, we can always compare back in history. Thanks!

[darrell-k]

1. Keep scanned_pics around after full/n&c scans but remove rows related to the rescanned album in Slim::Commands::rescanCommand as it already does for scanned_files; or
2. Clear scanned_pics after full/n&c scans (if we choose this, need to take care that it would happen even after a failed scan).

If I understand your options correctly, then I'd prefer #2. I understand this would drop the table after the scan. 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.

BTW: I know, I asked you to keep the old updateStandaloneArtwork around. But I changed my mind. Just drop it. Git doesn't forget: if we need to go back, we can always compare back in history. Thanks!

I'm thinking that for consistency, we should treat scanned_pics the same as scanned_files.

At present, scanned_files remains permanently in existence. It is only cleared before a full rescan, together with the rest of the database, in schema-clear.sql. For n&c scans and main process scans initiated by Slim::Control::Commands::rescanCommand only the rows relating to the folder(s) being scanned are removed.

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

[michaelherger]

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?

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

[michaelherger]

• 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

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