Optionally use images' embedded thumbnails for greater performance, even if they're too small
ClosedPublic

Authored by tommo on May 22 2019, 5:20 AM.

Details

Summary

Only generate high resolution thumbs if the user cares to not throw them away when closing gwenview, that is when deleteThumbnailCacheOnExit() is set to false. If set to true, prefer speed over quality: always use the embedded thumbnail regardless of its size.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
tommo requested review of this revision.May 22 2019, 5:20 AM
tommo created this revision.
tommo added a comment.EditedMay 22 2019, 5:36 AM

Attached is a screenshot demonstrating this suggestion. Even at 512px big thumbs, the embedded thumbnail (160x120) is big enough to get a rough idea of what's on the picture: A landscape, an animal, maybe which one? (not sure if you can tell the difference between a horse and a donkey, but IMO this isn't the purpose of a thumbnail preview anyway.)

Note that this feature is esp. useful when GwenviewConfig::deleteThumbnailCacheOnExit() is true.

One might argue, that those blurred thumbs are not pleasant to look at. In this case one might think about switching off the smooth scaling interpolation then and just show the pixelated preview, which would look like:

P.S.: The unit test fails expectedly. However, I'd first like to get some feedback before fixing it.

I think blurry is better than pixellated.

Speed is indeed important, but so is quality. I'm worried that if we do this, we'll get a huge flood of bugs complaining about blurry or pixellated thumbnaile. As a user, I would file one such bug.

What might make more sense is to load the small thumbs (if available) immediately for maximum speed, and then once that's done, begin lazily loading larger and more detailed thumbnails for any image where a small one is being used. That way we'll get the best of both worlds: speed and quality! What do you think?

tommo added a comment.EditedMay 22 2019, 4:48 PM

I really can't get used to high quality thumbnails. The problem with your approach is that it puts (IMO unnecessary) load on memory bus and CPU: First we would crawl through all files in the directory to quickly display the embedded thumbnail and in a second run we open and decode picture by picture just for generating nice high quality thumbs. This reminds me of https://bugs.kde.org/show_bug.cgi?id=331435

Counter-proposal: Only generate high resolution thumbs if the user cares to not throw them away when closing gwenview, that is when deleteThumbnailCacheOnExit() is set to false. If set to true, always use the embedded thumbnail regardless of its size.

Oh, and I would prefer pixelated.

Counter-proposal: Only generate high resolution thumbs if the user cares to not throw them away when closing gwenview, that is when deleteThumbnailCacheOnExit() is set to false. If set to true, always use the embedded thumbnail regardless of its size.

Okay, that seems reasonable to me since it's an option that's not on by default. With this proposal, that option basically turns into a high performance/low resource usage toggle, which I can get behind. We might want to adjust the wording in the settings dialog to mention the new behavior. Maybe something like this:

Thumbnails: [ ] Low resource usage mode
                Don't store thumbnails on disk, and prefer lower-quality
                thumbnails that are faster to load, when available

                Be careful: blablabla

BTW:

I really can't get used to high quality thumbnails.
Oh, and I would prefer pixelated.

You may be a bit different from Gwenview's target user, JFYI. :) That's just fine, most of us software developers are weirdos with very different preferences than our users, but it's important to keep that in mind. :) As a general rule most people prefer smoothness and quality over blistering speed, when a choice must be made for one or the other. We should default to beauty, even if the quality/speed trade-off is configurable.

tommo updated this revision to Diff 58529.May 23 2019, 6:30 AM

Docs updated. Need to apply https://phabricator.kde.org/D21329 first, to avoid conflict.

Restricted Application added a project: Documentation. · View Herald TranscriptMay 23 2019, 6:30 AM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript

Thanks in advance for fixing this minor typo.

lib/thumbnailprovider/thumbnailgenerator.cpp
123

Typo: thumnail -> thumbnail

tommo updated this revision to Diff 58530.May 23 2019, 6:57 AM

Sry, I should really enable Kate's spell checking.

Thanks. To account for this behavioral change, I think we need to adjust the UI strings in the settings window as well. Did my suggestion above make sense?

Thumbnails: [ ] Low resource usage mode
                Don't store thumbnails on disk, and prefer lower-quality
                thumbnails that are faster to load, when available

                Be careful: blablabla
app/advancedconfigpage.ui
51

avoid to store -> avoid storing

doc/index.docbook
849

Same ^^

tommo updated this revision to Diff 58573.May 24 2019, 5:55 AM
tommo edited the summary of this revision. (Show Details)
tommo set the repository for this revision to R260 Gwenview.

Sry, I missed that.

tommo updated this revision to Diff 58583.May 24 2019, 8:45 AM

Fix unit test.

ngraham retitled this revision from Use the embedded thumbnail for preview even if it's too small to Optionally use images' embedded thumbnails for greater performance, even if they're too small.May 24 2019, 3:04 PM

Thanks, just one last little thing...

tests/auto/thumbnailprovidertest.cpp
226 ↗(On Diff #58583)

space after the if

tommo updated this revision to Diff 58606.May 24 2019, 3:28 PM

You guys really have to setup clang-format or astyle to automate those whitespace formatting issues, IMO.

ngraham accepted this revision.May 24 2019, 4:36 PM
This revision is now accepted and ready to land.May 24 2019, 4:36 PM
This revision was automatically updated to reflect the committed changes.
cfeck added a subscriber: cfeck.May 24 2019, 4:55 PM
cfeck added inline comments.
app/advancedconfigpage.ui
51

I haven't checked which folder Gwenview actually tries to delete, but the thumbnails are stored in ~/.cache/thumbnails instead of ~/.thumbnails.

tommo added a comment.May 25 2019, 3:32 AM

I haven't checked which folder Gwenview actually tries to delete, but the thumbnails are stored in ~/.cache/thumbnails instead of ~/.thumbnails.

Indeed, the thumbnail cache location was changed in 574ff87ed9f1e761cd958045a57f937c0c2becac to ~/.cache/thumbnails which will be deleted in case.

Regarding astyle, see

Oh, I see, thanks!