Skip to content

fix: handle cached image load failures correctly - #345

Merged
htkhiem merged 2 commits into
htkhiem:mainfrom
aaravrav:fix/cache-image-errors
Aug 31, 2026
Merged

htkhiem merged 2 commits into
htkhiem:mainfrom
aaravrav:fix/cache-image-errors

Conversation

@aaravrav

Copy link
Copy Markdown
Contributor

When GTK fails to load a cached image, such as a WebP file for which GdkPixbuf may have no loader for, or a damaged or inaccessible file, Euphonica incorrectly treats the error as if the file were missing. It removes the database entry and downloads the cover again without deleting the existing file, so the cycle repeats on subsequent disk loads.

This PR fixes the cache behavior:

  • Now it database entries only for actually missing files while correctly propagateing other loading errors.
  • Reuses images loaded from disk instead of decoding them again on later requests by not forgetting to add them to the memory cache

This issue surfaced at NixOS/nixpkgs#557510.

@htkhiem

htkhiem commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Oh damn I accidentally removed the cache put operation on the normal codepath :( Thanks for putting it back in.

Regarding the other fixes though, looks like that'll disable the instant-retry logic when we fall into the "mentioned in DB but doesn't exist on disk" case, i.e. it requires a second image request to fill the image in; this would then leave placeholders on the screen without further retry/load attempts until the user at least scrolls away and back (correct me if I understood it wrongly). Not sure if it's necessary to restore that since I'm kinda biased toward what I wrote, but I'd like to have your confirmation that you are aware of this and think it's a fair removal.

I was totally unaware of the WebP issue though. It's weird as on my Arch Linux machine, GTK4 apps load WebP perfectly fine without webp-pixbuf-loader and the check the original issue used in the nixpkgs issue you mentioned also returned nothing for me (but I do have libwebp). I think I'll just add webp-pixbuf-loader to the AUR package's deps to be extra sure.

@aaravrav

aaravrav commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor Author

Regarding the other fixes though, looks like that'll disable the instant-retry logic when we fall into the "mentioned in DB but doesn't exist on disk" case, i.e. it requires a second image request to fill the image in

It removes the recursive instant retry, but that retry only repeated the database lookup as in it was not responsible for fetching the replacement. When the file is missing, get_image_internal removes its database entry and returns Ok(None). The same get_cover_internal call then continues directly to the MPD and external-provider paths. The replacement is still fetched immediately, without a second image request.

It's weird as on my Arch Linux machine, GTK4 apps load WebP perfectly fine without webp-pixbuf-loader and the check the original issue used in the nixpkgs issue you mentioned also returned nothing for me

Arch already provides GTK with WebP support through the glycin loader built into GdkPixbuf, so it is not listed separately. libwebp alone is not what enables WebP support. Nixpkgs disables glycin and therefore needs webp-pixbuf-loader, so you don't need to add it the deps of the AUR package

@htkhiem

htkhiem commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Arch already provides GTK with WebP support through the glycin loader built into GdkPixbuf

That's a relief to hear. I'll leave the deps unchanged then.

The rest looks good so I'll be merging now. Thanks a lot for both fixes!

@htkhiem
htkhiem merged commit d24a24b into htkhiem:main Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants