Thumbnails: make thumbnail generation dpr-aware
Needs RevisionPublic

Authored by meven on May 8 2020, 10:37 AM.

Details

Reviewers
dfaure
broulik
sitter
ngraham
bruns
Group Reviewers
Frameworks
Summary

Thumbnail ioslave can now receive a devicePixelRatio metadata parameter, that it will pass to its thumbnail creators plugins implementing ThumbCreatorV3.
It updates the directory thumbnail generation.

Port image, jpeg, djvu, krita, svg, text to new ThumbCreatorV3.

Depends on D29397

Test Plan

Manualy tested
With dolphin D29525

Diff Detail

Repository
R320 KIO Extras
Branch
thumbnail-dpr
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26597
Build 26615: arc lint + arc unit
meven created this revision.May 8 2020, 10:37 AM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptMay 8 2020, 10:37 AM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.May 8 2020, 10:37 AM
meven updated this revision to Diff 82332.May 9 2020, 6:24 AM

Store thumbnails with dpr 2 in @2x folders

You do seem to calculate the deviceWidth and height an awful lot, it makes reading a bit clunky. I'd much rather have the multiplication done once and then always use the var instead. In fact, perhaps it'd make sense to have createV3 feed the values into the implementations? Currently they all repate the same two lines over and over.

thumbnail/djvucreator.cpp
61

Surely multiplication results need converting to int. That being said, you do multiply below again, so maybe just move maxWidth/Height up here.

thumbnail/thumbnail.cpp
774

Var naming is a bit inconsistent across the code base now. In the implementations there are maxWidth/H that are device-adjusted but in here they are not. Not a huge concern, just noticed.

meven added inline comments.May 13 2020, 3:05 PM
thumbnail/thumbnail.cpp
774

Will take care

791

Not used can be removed.

794

Because of line above scaleDownImage can never happen.
Can probably be removed.

meven added a comment.Sep 7 2020, 9:39 AM

I just sent a specification evolution to cover this use case : https://gitlab.freedesktop.org/xdg/xdg-specs/-/merge_requests/35

bruns requested changes to this revision.Sep 7 2020, 3:19 PM
bruns added a subscriber: bruns.

For all but text the DPR is completely irrelevant, large@1 is identical to normal@2.

This revision now requires changes to proceed.Sep 7 2020, 3:19 PM
bruns added inline comments.Sep 7 2020, 3:51 PM
thumbnail/thumbnail.cpp
143

Has to be static or in an anonymous namespace.

149

width * dpr, height * dpr gives the wanted result even without explicit support for DPR for the majority of the thumbnailer.

bruns added inline comments.Sep 7 2020, 3:54 PM
thumbnail/imagecreator.cpp
48

This whole hunk should not be part of this submission. Also, the comment ("jpeg") is likely wrong.

meven added a comment.Sep 8 2020, 10:51 AM

For all but text the DPR is completely irrelevant, large@1 is identical to normal@2.

Yes and that's up to thumbnail creators to decide. To take advantage of this, we would need to introduce some ThumbnailCreator type that would say whether or not generated thumbnail might be influenced by DPR (i.e) text. That would necessitate change the ThumbnailCreator API.

But the implementation will stay a lot simpler if we don't this level of complexity and adding will have a limited interest. Storing twice large@1 would happen only when a user would change DPR, thumbnails cache size limit will stay enforced.

bruns added a comment.Sep 8 2020, 1:29 PM

For all but text the DPR is completely irrelevant, large@1 is identical to normal@2.

Yes and that's up to thumbnail creators to decide. To take advantage of this, we would need to introduce some ThumbnailCreator type that would say whether or not generated thumbnail might be influenced by DPR (i.e) text. That would necessitate change the ThumbnailCreator API.

No, just add a "DisplayPixelRatioDependent=true" key to the service data of the thumbnailer.

But the implementation will stay a lot simpler if we don't this level of complexity and adding will have a limited interest. Storing twice large@1 would happen only when a user would change DPR, thumbnails cache size limit will stay enforced.

You are changing every single thumbnailer implementation, although the results are binary identical for normal@2 and large@1. And that are only the thumbnailers in kio-extra.

And on top of all this, the only thumbnailer which is DPR dependent (text) does not even cache the data on disk.