Bump max thumbnail size to 512px
ClosedPublic

Authored by sandsmark on Jun 3 2017, 7:50 PM.

Details

Summary

256 really isn't all that big on modern hidpi/4k displays.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sandsmark created this revision.Jun 3 2017, 7:50 PM
gateau edited edge metadata.Jun 6 2017, 11:56 AM

I love the idea, but what about caching? Cached thumbnails are supposed to follow https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html#THUMBSIZE, which still, sadly, says 256.

ngraham added a subscriber: ngraham.Nov 2 2017, 9:39 PM
rkflx added a subscriber: rkflx.Jan 10 2018, 9:36 PM

I love the idea, but what about caching? Cached thumbnails are supposed to follow https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html#THUMBSIZE, which still, sadly, says 256.

it says the sizes are just suggestions, so I don't think bumping it up a bit would violate this.

rkflx requested changes to this revision.Jul 16 2018, 10:15 PM
rkflx added a project: Gwenview.
rkflx edited reviewers, added: Gwenview; removed: Konsole, tcanabrava, dhaumann.

Thanks for coming back to this. Yes, HiDPI support is still in progress. If you could help with the thumbnail-spec/caching side of things that would be great!

I love the idea, but what about caching? Cached thumbnails are supposed to follow https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html#THUMBSIZE, which still, sadly, says 256.

it says the sizes are just suggestions, so I don't think bumping it up a bit would violate this.

Quotes from the spec:

  • Normal: […]The image size must not exceed 128x128 pixel.
  • Large: […] must be 256x256 pixel.
  • However, these are suggestions. Implementations should cope also with images that are smaller […]

I don't see anything in there about larger thumbnails.

Some observations running Gwenview with this patch:

  • Even with the size slider set to 512px, the thumbnails being written to the cache are still 256px. I guess this makes sense, since the "normal" and "large" folders of the cache are meant for 128px and 256px thumbnails respectively as per the spec. However, what's the point of the cache then? Perhaps the spec should be extended to allow for something like a "huge" folder with 512px thumbnails?
  • The pixmap used for the thumbnail is still only 256px, this patch merely allows scaling that up to 512px, making it look blurry (can be seen easily by viewing the thumbnail of a screenshot and setting MaxThumbnailSize = 2048). You can achieve the same effect with the default slider maximum of 256px and QT_SCALE_FACTOR=2.
  • For large sizes, using the zoom buttons or scrolling over the slider becomes really tedious. It's still acceptable around 256px, but much too slow up to 512px. This should work more like the progressive zoom slider in View mode.

I'm all for allowing larger thumbnail sizes, but I'm not yet comfortable approving the patch. Is showing a large blurry thumbnail really what we want, or should we rather fix things in the proper places? Ideally the spec could be extended, but in any case we'd have to display and (somewhere) store proper 512px thumbnails (which this patch is not implementing – yet?).


@ngraham added reviewers: Konsole, tcanabrava, dhaumann.

Sorry to disagree with you there. Yes, the repo is set wrongly, but title, file list and the diff are quite clear in this case.

This revision now requires changes to proceed.Jul 16 2018, 10:15 PM

Whoops, my bad! It has been so long since I last saw this that I forgot completely what it was about and trusted the Phabricator metadata.

rkflx resigned from this revision.Aug 25 2018, 8:36 AM

Seems like gnome just went with their own private folder ($XDG_CACHE_HOME/gnome-photos/thumbnails/$SIZE-$GENERATION), though not only because of the size issue:

https://debarshiray.wordpress.com/2018/01/29/gnome-photos-an-overview-of-thumbnailing/

sandsmark updated this revision to Diff 48020.Dec 22 2018, 5:31 PM

Make sure we avoid the disk cache if we use thumbnails larger than the XDG spec allows.

ngraham accepted this revision.Dec 31 2018, 10:36 PM

I am inclined to accept this. It's a nice enhancement that seems to work fine in my testing, and while it doesn't strictly speaking follow the letter of the XDG spec, I don't think there's any sense letting the perfect being the enemy of the good here. We all know how that game would end: tons of wrangling in the FDO world and no official support for 512px thumbnails for years while we wait. I say let's do this now.

This revision is now accepted and ready to land.Dec 31 2018, 10:36 PM
gateau accepted this revision.Jan 6 2019, 1:23 PM

I agree, unless the XDG spec evolves, it is becoming more and more useless anyway.

This revision was automatically updated to reflect the committed changes.