KPreviewJob : Support for DeviceRatioPixel
Needs ReviewPublic

Authored by meven on May 4 2020, 6:26 AM.

Details

Reviewers
dfaure
broulik
ngraham
Group Reviewers
Frameworks
Summary

Allow users of KPreviewJob to request a devicePixelRatio for generated thumbnails.
The result image is not guaranteed to have this ratio, and users should paint it accordingly.

Adds a convenient setDefaultDevicePixelRatio to allow an application to set its device pixel ratio preferred with a one liner patch. See dolphin's D29525

The images using a pixel ratio are stored in cache in ~/.cache/thumbnails/normal@2x for 256*256 (i.e 128*128@2x) and ~/.cache/thumbnails/large@2x for 512*512 (i.e 256*256@2x)

See kio-extras patch D29526

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D29397
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27068
Build 27086: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
dfaure added inline comments.May 5 2020, 8:29 AM
src/widgets/thumbcreator.h
191

5.70 is tagged already

meven added inline comments.May 5 2020, 9:59 AM
src/widgets/previewjob.cpp
446

Maybe make this static, so that apps have to do it once per app sort of like we do with Qt::AA_UseHighDpiPixmaps, rather than by KPreviewJob.

meven updated this revision to Diff 81982.May 5 2020, 9:59 AM

Improve doc, fix @since

meven marked an inline comment as done.May 5 2020, 9:59 AM

Allow users of KPreviewJob to request a devicePixelRatio for generated thumbnails.

At the risk of asking a stupid question, why?

As opposed to just having a width and height always be in device pixels. We're always working with pixmaps is there a reason to do anything with logical sizes?
That's how I thought the current design worked.

At the risk of asking a stupid question, why?

The text thumbnailer should be able to produce readable text on high dpi, or the folder thumbnailer should render the folder icon sharply and the picture frames non-pixelated

meven added a comment.May 5 2020, 12:54 PM

At the risk of asking a stupid question, why?

The text thumbnailer should be able to produce readable text on high dpi, or the folder thumbnailer should render the folder icon sharply and the picture frames non-pixelated

And this also makes the job of Applications easier, not having to make width * devicePixelRatio, height * devicePixelRatio everywhere but a single setDevicePixelRatio similar to Qt own functions.

meven added a comment.May 6 2020, 7:15 AM

Oh, I thought it was sent as an int. But 8 is QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?

No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and the format is forced to QImage::Format_ARGB32 "5", so this works.
From kio-extra thumbnails :

if( img.format() != QImage::Format_ARGB32 ) { // KIO::PreviewJob and this code below completely ignores colortable :-/,
    img = img.convertToFormat(QImage::Format_ARGB32); //  so make sure there is none
}
// Keep in sync with kio/src/previewjob.cpp
meven updated this revision to Diff 82056.May 6 2020, 7:22 AM

Add a static setDefaultDevicePixelRatio method to PreviewJob

broulik added inline comments.May 6 2020, 7:24 AM
src/widgets/previewjob.cpp
446

Not a fan. You can have different dpi per screen.
Maybe instead we should have a QWindow* method or constructor to take dpr from

meven marked 2 inline comments as done.EditedMay 6 2020, 7:30 AM

I have pretty much the patch in kio-extras ready.

src/widgets/previewjob.cpp
446

Not a fan. You can have different dpi per screen.

What you imply is incorrect.
In Wayland, QGuiApplication::devicePixelRatio always returns the maximum scale of any screens (and ceiled up, so my 1.25 scale is returned as 2), and always the same value regardless on the scale of the screen where the app is. Only at painting does the scale makes a difference per screen.

In X, well there is only one scale anyway.

dfaure added a comment.May 6 2020, 8:45 AM

Oh, I thought it was sent as an int. But 8 is QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?

No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and the format is forced to QImage::Format_ARGB32 "5", so this works.

OK, this works today. But if one day we want to start actually using other image formats, we'll end up with a clash here.
Why not use 0x80 in order to stay away from valid image format values? This seems safer to me, in the long run.

meven updated this revision to Diff 82071.May 6 2020, 9:02 AM
meven marked an inline comment as done.

Add @since to setDefaultDevicePixelRatio

meven planned changes to this revision.May 6 2020, 9:32 AM

Oh, I thought it was sent as an int. But 8 is QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?

No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and the format is forced to QImage::Format_ARGB32 "5", so this works.

OK, this works today. But if one day we want to start actually using other image formats, we'll end up with a clash here.
Why not use 0x80 in order to stay away from valid image format values? This seems safer to me, in the long run.

I did wrong calculation about how big a 8-bit integer is, believing 0x8 was the most significant bit, while you correct me here it is 0x80, thanks.
Will update accordingly.

meven updated this revision to Diff 82079.May 6 2020, 11:31 AM

Fix bitmask check

meven updated this revision to Diff 82255.May 8 2020, 10:44 AM
meven marked an inline comment as done.

Improve naming of a variable, fix scaling of the resulting preview

For another stupid question (the first one was already asked by someone else and answered :) ):
Given some generated thumbnails are cached, does the thumbnail cache specification support logical resolution? How would cached thumbnails work cross-screen?

meven added a comment.May 8 2020, 11:35 AM

For another stupid question (the first one was already asked by someone else and answered :) ):
Given some generated thumbnails are cached, does the thumbnail cache specification support logical resolution?

I am evolving here the specification : the images stored are stored with their HiDpi size (width * dpr, height * dpr), and then some metadata is written.
When reading the cached version for large, the user expects a 256*256 (modulo aspect ratio) and can find a (256 * dpr, 256 * dpr) image (or can read the devicePixelRatio from metadata)
You can see in https://phabricator.kde.org/D29526#C575294OL738 and line 582 how to detect and handle such a case.

How would cached thumbnails work cross-screen?

Cache thumbnails with low dpr will look blurry on hidpi screen, other than that's not an issue.
We would need applications to update to use Hidpi thumbnails to renew thumbnails.
I might want to evict low dpr thumbnails.

For another stupid question (the first one was already asked by someone else and answered :) ):
Given some generated thumbnails are cached, does the thumbnail cache specification support logical resolution?

I am evolving here the specification : the images stored are stored with their HiDpi size (width * dpr, height * dpr), and then some metadata is written.

Specification as in, https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html? So there is some work on extending the specs not yet reflected on that page?
How is the metadata written?

How would cached thumbnails work cross-screen?

Cache thumbnails with low dpr will look blurry on hidpi screen, other than that's not an issue.

Cached thumbnails rendered for hidpi (so e.g. being 256x256 with bigger details due to hidpi context) , but used for lowdpi elsewhere will be an issue as well. E.g. when it comes to text rendered with minfontsize like in case of plain text documents, it will be too large then.

I wonder if the thumbnail cache would not need the same extension like the icon spec had, with a @2x variant, to make up for that. There are still a lot of lowdpi devices out there, and they are getting mixed.

dfaure requested changes to this revision.May 8 2020, 1:37 PM
dfaure added inline comments.
src/widgets/previewjob.cpp
173

coding style: '{' on separate line

src/widgets/thumbcreator.cpp
54

'{' on its own line for methods

src/widgets/thumbcreator.h
215

docu?

This revision now requires changes to proceed.May 8 2020, 1:37 PM
meven updated this revision to Diff 82290.May 8 2020, 4:41 PM
meven marked 2 inline comments as done.

Improve documentation, code style

meven updated this revision to Diff 82292.May 8 2020, 4:45 PM
meven marked an inline comment as done.

Code style

meven added inline comments.May 8 2020, 4:54 PM
src/widgets/thumbcreator.h
215

I wonder about moving qreal devicePixelRatio before img parameter

meven added a comment.May 8 2020, 5:06 PM

For another stupid question (the first one was already asked by someone else and answered :) ):
Given some generated thumbnails are cached, does the thumbnail cache specification support logical resolution?

I am evolving here the specification : the images stored are stored with their HiDpi size (width * dpr, height * dpr), and then some metadata is written.

Specification as in, https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html? So there is some work on extending the specs not yet reflected on that page?

Thanks for the reference.
I meant I am beyond the spec, adding some by facts, that is not great :/
This spec is showing its age, and would need to evolve to cover HiDPi use case, but I am not editing it ATM.

How is the metadata written?

line 751 of PreviewJob.

How would cached thumbnails work cross-screen?

Cache thumbnails with low dpr will look blurry on hidpi screen, other than that's not an issue.

Cached thumbnails rendered for hidpi (so e.g. being 256x256 with bigger details due to hidpi context) , but used for lowdpi elsewhere will be an issue as well. E.g. when it comes to text rendered with minfontsize like in case of plain text documents, it will be too large then.

I wonder if the thumbnail cache would not need the same extension like the icon spec had, with a @2x variant, to make up for that. There are still a lot of lowdpi devices out there, and they are getting mixed.

That looks like the way to go, I would favor adding normal@2x and large@2x in .cache/thumbnails

meven updated this revision to Diff 82331.May 9 2020, 6:23 AM

Store thumbnails with dpr 2 in @2x folders

meven updated this revision to Diff 82335.May 9 2020, 8:20 AM

Fix thumbpath when in devicepixel subdir

meven updated this revision to Diff 82339.May 9 2020, 9:38 AM

Rebasing

meven updated this revision to Diff 82709.May 13 2020, 6:36 AM

Ensure to load images with the right devicePixelRatio

meven edited the summary of this revision. (Show Details)May 13 2020, 6:37 AM
meven added a comment.May 16 2020, 6:08 AM

ping reviewers

src/widgets/previewjob.cpp
757

Maybe add it optionally, when different from 1

Would it make sense to initialize devicePixelRatio to QGuiApplication::devicePixelRatio() and add an API to change it (if desired) ?

This way we wouldn't need to call a static method in the main of every client app (i.e. D29525 wouldn't be needed).

meven added a comment.May 18 2020, 6:37 AM

Would it make sense to initialize devicePixelRatio to QGuiApplication::devicePixelRatio() and add an API to change it (if desired) ?

It would make KPreviewJob casting a QCoreApplication::instance to QGuiApplication::devicePixelRatio that would be tolerable.

But this would make the feature opt-out : apps could get broken when their previews double their size just when changing KF version.

This way we wouldn't need to call a static method in the main of every client app (i.e. D29525 wouldn't be needed).

This is a single static call for the lifetime of the app, not that hard.
This makes the feature opt-in, meaning the developer has tested it.

meven added a comment.May 20 2020, 6:12 AM

ping dear reviewers

src/widgets/previewjob.cpp
588

Missing const

src/widgets/previewjob.h
192

Did you mean @p dpr?

(unless doxygen is smart enough to figure out the name of the parameter?)

268

Typo: devicePixelRatio.

meven updated this revision to Diff 83139.May 24 2020, 5:37 AM
meven marked 3 inline comments as done.

Typo, const, doxygen fix

meven updated this revision to Diff 83140.May 24 2020, 7:39 AM

Rebasing

Overall seems sane. Two questions though:

src/widgets/previewjob.cpp
401

Is this @2x suffix standardized? What happens if I'm using 125% scaling, generate some previews, and then switch back to 100% (no scaling?)

meven marked an inline comment as done.May 25 2020, 4:28 AM

Overall seems sane. Two questions though:

Is this @2x suffix standardized?

No but it is already being used for icon caching.

What happens if I'm using 125% scaling, generate some previews, and then switch back to 100% (no scaling?)

When your screen is scaled 125%, the application gets a dpr of 2.
When you go back to 100%. the thumbnails in ~/.cache/thumbnails/large and ~/.cache/thumbnails/normal will be used again.
Meaning you will have two sets of previews.

The images using a pixel ratio are stored in cache in ~/.cache/thumbnails/normal@2x for 256*256 (i.e 128*128@2x) and ~/.cache/thumbnails/large@2x for 512*512 (i.e 256*256@2x)

@meven Have you already got in contact with the other users/maintainers of he thumnbail cache spec about the idea to extend it with support for high dpi? If not, please consider to do so, so things will also work cross-toolkit/platforms in the future there, by being based on an agreed & formalized specification. Not being involved here or having full understanding of the topic, but I would guess your approach with the separate x2 should run into "make sense" responses, so the additional effort might be low for the gain of being based on an official spec.

This would be discussed on https://lists.freedesktop.org/mailman/listinfo/xdg, possible best by cc:ing other potential parties interested in highdpi thumbnails, would need to be researched, possibly some Dolphin contributors know their counterparts in non-KDE FLOSS projects

The approach makes sense then. I agree that making high DPI a part of the FDO spec would be nice, but IMO that shouldn't block this. The approach currently taken is logical and it could be submitted as an extension to the spec later.

src/widgets/previewjob.cpp
401

What about if I'm using a 250% scale factor? Maybe there should be an @3x folder too.

meven added inline comments.May 25 2020, 3:12 PM
src/widgets/previewjob.cpp
401

or just trunate devicePixelRatio to int

The approach makes sense then. I agree that making high DPI a part of the FDO spec would be nice, but IMO that shouldn't block this. The approach currently taken is logical and it could be submitted as an extension to the spec later.

It might be seen logical to us from our current POV, but better to do as desktop developer generations have done before and try to get others on board from the start, once there is a first plan. Would we want others do just extend specs on their own and start to write stuff without namespacing onto common data grounds? I suspect: no :) So we better behave as well as we would like others to do.

Here's where the spec lives, FWIW: https://gitlab.freedesktop.org/xdg/xdg-specs

Expanding it is mostly just a matter of writing up a reasonable proposal in the form of a merge request and getting enough people to agree. Discussing on the mailing list first can help, to gauge people's opinions.

Thanks for starting the discussion about the spec, by what I saw on the mailinglist. Hopefully that soon gets traction by others.

To not have that block this improvement, you could in parallel for now use a "kde" namespaced directories, say "*@kde2x/", where we/you could just use the for-now-KF-only theme extension. And once there is an agreed specification extension, the code could be switched to just any non-namespace shared hidpi folder matching whatever the specification ended up to be.

meven added a comment.Jun 2 2020, 3:10 PM

To not have that block this improvement, you could in parallel for now use a "kde" namespaced directories, say "*@kde2x/", where we/you could just use the for-now-KF-only theme extension. And once there is an agreed specification extension, the code could be switched to just any non-namespace shared hidpi folder matching whatever the specification ended up to be.

Well I don't think it is needed, since those @2x are not used by anyone as far as I can tell.
I'd rather move on here without the spec finalized and then update the implementation once it is finalized.
I don't expect it to be much different from the current implementation, and I hope this implementation serve the specification process since we will have an implementation to debate on.
And this will save two commits that serves no purpose IMO, we are not Web Browsers after all, we are not breaking anyones business.

Alternatively I'd rather wait for the spec to be finalized.
Just to avoid the friction.

The proposed additions to the spec are non-controversial IMO. Let's push that forward. I left a supportive comment in the email thread, so maybe it's time to put together a patch that people can comment on.