Rely on a DB table for artwork scans to prevent repeated disk access - #1621
Rely on a DB table for artwork scans to prevent repeated disk access#1621michaelherger wants to merge 6 commits into
Conversation
When we crawl the file system for music files, keep a list of artwork files as well. This way we don't have to repeatedly read full folders to find potential artwork. * add `scanned_pics` table in the scanner while looking for audio files * when looking for file based artwork query this table instead of reading the file system * look for an exact template match in the music file's folder first (if defined), then `cover`, `album`, `folder`, `thumb` * check the template in the artwork folder (if defined) second * last check any artwork file in the music file's folder Signed-off-by: Michael Herger <michael@herger.net>
Signed-off-by: Michael Herger <michael@herger.net>
|
@michaelherger Makes sense to separate this out from the box sets work. It would be good if @mikeysas updated #1430 with the additional requirements as he offered yesterday in his #1536 comment. Then we can take that issue as the statement of box set requirements.
I wouldn't say there were repeated lookups in #1536 (at least there shouldn't be!), but we do need to look for candidates for each folder and disc number within an album to potentially update the albums/tracks tables with new/different image urls. The logic for the box sets enhancement is, I believe, all there in #1536. I'll try to merge these changes with that. Unless you want to take over from here? Some questions on this PR:
|
|
|
||
| DROP TABLE IF EXISTS scanned_pics; | ||
| CREATE TABLE scanned_pics ( | ||
| url text NOT NULL COLLATE NOCASE, -- URL must be case insensitive, or we might duplicate tracks if the filename changes case only (https://github.com/LMS-Community/slimserver/issues/705#issuecomment-1026229542) |
There was a problem hiding this comment.
I thought that here we would want to recognise case differences?
There was a problem hiding this comment.
I copied this 1:1 from schema_11_up.sql. It's what we've had before.
There was a problem hiding this comment.
Yes, I did the same to start with, but I don't think the issue mentioned in the comment applies. And on a non-Windows system we could potentially have Cover.jpg and cover.jpg in an album folder. Maybe it wouldn't matter in that case which one we picked up , but I don't think case sensitivity would do any harm.
There was a problem hiding this comment.
...and I'm handling upper/lower case file extensions in code already. I'll remove this. Thanks!
Good question! I just don't know how to test it 🤦🏻. I think it's Linux only, and under very specific circumstances, too? I'd have to test in a VM or container. Would you know how to easiest test this?
No. Unlike the audio files, which we process one after the other until all have been "consumed", we don't iterate over that list. And the same file can be used by multiple tracks. There's actually a nice use case: if you set the cover format to
To be verified indeed. Thanks. Edit: the code would currently only use the DB when run in the scanner. It should still work in other cases, but then checking against the file system, rather than the DB. Single albums are run in the server process (IIRC). Therefore this should not be impacted by this change. |
|
Would https://github.com/LMS-Community/slimserver/blob/public/9.2/Slim/Schema.pm#L1376-L1382 be a candidate where to look for "box" or album level artwork? |
No. But I'm all Linux here, so I'll investigate.
Understood. I also saw that you're clearing the new table in the optimize SQL script but of course there's a possibility that doesn't run (crashed or cancelled scan) and I'm thinking it should always reflect the current images on disc, so should always be empty at the start of the scan.
You're keeping the code which finds images directly from the disk? |
No, because we need to scan for new/changed artwork for all tracks/albums, not just those that have changed in a n&c scan. The driver is this: slimserver/Slim/Music/Artwork.pm Lines 273 to 286 in 2f7076c which in my branch is enhanced to this: So we get a result set row returned for every album/discnumber/directory combination. I then have further logic where the The bigger size of this result set, and the extra calls for each album, are the reasons why there are many more calls of I did wonder if we could use the mtime from the new table to loop through that in order to determine which albums /tracks needed updating, but that wouldn't handle the case where an image had been removed, and we'd still need to be processing such tracks/albums. I can zip up my test data with an explanation of each test case, but it won't be today. Can I take it that you are taking over the coding the box set functionality? I'm perfectly fine with that, and of course remain on hand for support and testing. |
Done. #1430 (comment) |
|
I'm still trying to invoke the AIO scan, it looks like we need to trigger a directory scan in the main process, but I can't yet work out how to do that. But I found a bug: when |
I think one way is to use the AutoRescan plugin.
Thanks! Looking into this. Are you using standalone artwork for that particular file? Any custom artwork format string? Could you please enable logging for |
Basically yes. It's not the exact same code, but functionally it should be. Under certain circumstances it doesn't make sense to initialise the DB and everything, eg. when dealing with a single, known file. |
Why would we have to do so? |
We already do. External artwork can change without music file changes. |
I already tried that, it runs in a separate scanner process which doesn't look for AIO capability.
There is a single image in the album directory, with a random name (ie not one of the preferred names). No custom format. Not near a computer right now, but I guess it's going into the "else" after "if scanner" block which uses the new table. |
Disable all plugins that provide scanner features... I guess we can assume it's a rare case 🤣.
Oh, good catch! The disk lookup for random name got lost in my re-factor... Great catch, thanks! |
Signed-off-by: Michael Herger <michael@herger.net>
…thing else fails. Signed-off-by: Michael Herger <michael@herger.net>
This should be fixed. Thanks! |
| url text NOT NULL, | ||
| timestamp int(10), | ||
| filesize int(10) | ||
| ); |
There was a problem hiding this comment.
I wasn't suggesting removing the nocase from scanned_files. Does #705 no longer apply?
There was a problem hiding this comment.
Heh... what were you suggesting then?
There was a problem hiding this comment.
Just remove it from scanned_pics.
There was a problem hiding this comment.
Ok. Got it. Thanks for the clarification.
… `LIKE` query by using a range. This allow SQLite to still use the index, despite searching for a sub string. Signed-off-by: Michael Herger <michael@herger.net>
|
Any objections to merging this? |
|
I haven't tested all scenarios (eg album rescan, and still haven't managed to trigger the AIO scan). |
|
Also I haven't looked at the variable artwork naming. |
That part actually is in 9.2 already. |
Album rescan does work. I was confused by "boxset" idea, which is not part of this PR yet. |
Signed-off-by: Michael Herger <michael@herger.net>
|
Would it be worth looking at Could we use the new table instead? (the code in |
|
Here's an odd issue: I've got two albums (in adjacent folders) which have the same image (different file paths, same image name/size) and under this PR the tracks for both albums have |
| # doing a range search helps us avoid a LIKE query, which would result in a scan | ||
| $sql .= '>= ? AND url < ?'; | ||
| my $pathUrl = Slim::Utils::Misc::fileURLFromPath($parentDir); | ||
| push @candidates, $pathUrl, $pathUrl . chr(0xff); |
There was a problem hiding this comment.
The problem I just mentioned in the main thread is here. I reverted to LIKE (just for an experiment) and the issue was resolved.
The directories in question are:
/home/darrell/Music/testmusic/stripped_test3/Bill Evans/Waltz For Debby (Copy)
and
/home/darrell/Music/testmusic/stripped_test3/Bill Evans/Waltz For Debby
There was a problem hiding this comment.
The problem with LIKE is that it would find everywhere below a given folder. You at some point asked whether there was any harm walking up the tree more than three levels (in the box set PR, I guess). And that is one reason where it might hurt. If you walked up "infinite" levels if needed, you might end up with all artwork files repeatedly, having to process, hundreds, thousands of entries all the time.
Having these two distinct PRs is becoming more and more difficult to handle: I've already modified this check in the box set PR. Could you please check whether you see the same there?
Edit: That other change I just mentioned is only dealing with not going deeper, but not with folders of the same prefix.
There was a problem hiding this comment.
In my implementation of the pics table I had a separate column holding the directory path, which would allow a WHERE =
There was a problem hiding this comment.
Edit: That other change I just mentioned is only dealing with not going deeper, but not with folders of the same prefix.
In fact that change also prevents picking up ./Waltz For Debby (Copy)/ when processing ./Waltz For Debby/ as it finds the / in ./Waltz For Debby (Copy)/, because it's out of range of the substring.
There was a problem hiding this comment.
I have to confirm that by now I have come to the same conclusion that a distinct folder column would be the way to go. I'm sorry it took me so long to answer. I hoped to get back to this PR sooner - because I want to use the DB for the artist picture scan, too! BTW: I believe the artist picture scan will face some of the same challenges as the box set scan: it does look in common parent folders. So that functionality should come in handy.
Use the table instead of checking the file status? This might work in some cases, but fail in others. I think it's also called whenever you bring up eg. the track info page. But the table (currently) is only available while in the scanner. I guess we'd have to make the same distinction there too: if in scanner, use the table, otherwise file system check?
Yeah, saw that too. I might even have amended some of these in the box set branch. |
|
Yes it would have to be scanner only. |
|
It's not changed in this PR, but I'm trying to understand this code: slimserver/Slim/Music/Artwork.pm Lines 327 to 328 in b02ba54 It is testing Am I missing something? |
Yes, that's wrong. This would always update online artwork. The check should be against |
4 participants
When we crawl the file system for music files, keep a list of artwork files as well. This way we don't have to repeatedly read full folders to find potential artwork.
scanned_picstable in the scanner while looking for audio filescover,album,folder,thumb@darrell-k I took inspiration from
cffd307(#1536) to further look into using a db table to store artwork information. Thanks for that work!At this point (without other modifications to the scanner) I can't see any performance improvement or decrease. It's almost identical in my current test environment (much faster NAS than last week's tests), with none being consistently faster or slower. Most likely this is because there's none of that repeated artwork lookup like with the changes for box sets.
I believe the DB table approach might prove to be super helpful for other use cases, too, eg. contributor picture lookups. But I haven't gone there yet.
I'll use this branch to further look into the box set challenge. I don't know yet where exactly that will best be implemented...
Signed-off-by: Michael Herger michael@herger.net