Skip to content

Fix work images and artwork precaching #1517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
michaelherger merged 2 commits into from
Feb 26, 2026
+6 −2
Merged

Fix work images and artwork precaching #1517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2418a4
Fix work images
darrell-k Feb 23, 2026
087f8cc
Fix artwork precaching
darrell-k Feb 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Slim/Control/Queries.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5149,6 +5149,10 @@ sub worksQuery {

}

push @{$w}, "NOT EXISTS (SELECT 1 FROM tracks t2 WHERE tracks.album=t2.album AND tracks.work=t2.work
AND (tracks.performance IS NULL AND t2.performance IS NULL OR tracks.performance=t2.performance)
AND tracks.id > t2.id)";

if (defined $libraryID) {
push @{$w}, 'EXISTS (SELECT 1 FROM library_album WHERE library_album.album = albums.id AND library_album.library = ?)';
push @{$p}, $libraryID;
Expand Down
4 changes: 2 additions & 2 deletions Slim/Music/Artwork.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,7 @@ sub precacheAllArtwork {
}
. ($force ? '' : ' AND tracks.cover_cached IS NULL')
. qq{
GROUP BY tracks.cover
GROUP BY tracks.cover, tracks.album
};

my $sth_update_tracks = $dbh->prepare( qq{
Expand Down Expand Up @@ -816,7 +816,7 @@ sub precacheAllArtwork {
FROM tracks
WHERE tracks.album = ?
AND tracks.coverid IS NOT NULL
ORDER BY tracks.disc, tracks.tracknum
ORDER BY CASE WHEN CAST(CAST(tracks.cover AS INTEGER) AS TEXT) = tracks.cover THEN '1' ELSE '0' END, tracks.disc, tracks.tracknum
Copy link
Copy Markdown
Member

@michaelherger michaelherger Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prioritisation of external images would only work if the hash didn't happen to be an all numerical value, right? Is it worth the casting overhead? Did you measure the overhead?

Copy link
Copy Markdown
Contributor Author

@darrell-k darrell-k Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added logic to the order by in $sth_get_album_art so that any external artwork is preferred to that embedded in the first track.

In more detail: the double cast converts the column's value to numeric and then back to text. we then compare it to the original value and if they're equal, we know that the original value was an integer, ie the size of the embedded artwork rather than a path to the external image.

This is then enclosed in a CASE statement so as to return 1 if the artwork is embedded and 0 if it is external.

The whole thing is used as the first sort key so that an external image will always take priority over an embedded image.

The existing code just uses the first track's image even if it was embedded and there is also an album-level image in the album's folder.

Copy link
Copy Markdown
Member

@michaelherger michaelherger Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I only saw your initial description after I posted... and then edited my comment to be more specific. And now I see that I confused cover and coverid...

Copy link
Copy Markdown
Member

@michaelherger michaelherger Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run a quick test listing tracks using those orders on my Pi4, with about 22k tracks. Overhead is negligible.

Copy link
Copy Markdown
Contributor Author

@darrell-k darrell-k Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good. What do you say about further work to bring Slim::Music::Artwork::updateStandaloneArtwork and Slim::Music::Artwork::precacheAllArtwork into line?

Copy link
Copy Markdown
Member

@michaelherger michaelherger Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align how the two work? I'm all for that. But this PR started as a fix, which I believe should even go into 9.1. Further alignment might be beyond a stable scope. What do you think?

Copy link
Copy Markdown
Contributor Author

@darrell-k darrell-k Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might do and definitely this fix should go into 9.1. Leave it with me for now.

I'm actually having ideas about #1513 now I have studied Slim::Music::Artwork. I think that PR can be simplified.

Copy link
Copy Markdown
Member

@michaelherger michaelherger Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, shall we merge this one, and back port in a few if good?

And yes, #1513 needs some cleanup IMHO. There were some no-op changes and probably cruft left behind by the AI assistant forgetting what it had done before...

Copy link
Copy Markdown
Contributor Author

@darrell-k darrell-k Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check whether the sth_get_album_art processing needs adding to Slim::Music::Artwork::updateStandaloneArtwork. I'm wondering if it needs to be for a album rescan.

Copy link
Copy Markdown
Contributor Author

@darrell-k darrell-k Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelherger Let's go with this as it is.

LIMIT 1
});

Expand Down