HiDPI Support for Gwenview
Needs RevisionPublic

Authored by hetzenecker on Aug 28 2017, 5:57 AM.

Details

Summary

Initial support for HiDPI-scaling of documents in RasterImageView.

This patch scales up images to display them correctly on HiDPI-enabled screens.

TODO:

  • SVG documents and videos
  • Scaling of thumbnails

BUG: 373178

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
hetzenecker created this revision.Aug 28 2017, 5:57 AM
asn added a subscriber: asn.Aug 30 2017, 2:43 PM
cfeck added a subscriber: cfeck.Aug 30 2017, 5:08 PM

With Wayland, we can have different scaling factors per screen. Is it better to use QScreen::devicePixelRatio() or QWindow::devicePixelRatio() instead of QApplication::devicePixelRatio()?

In D7581#141489, @cfeck wrote:

With Wayland, we can have different scaling factors per screen. Is it better to use QScreen::devicePixelRatio() or QWindow::devicePixelRatio() instead of QApplication::devicePixelRatio()?

I briefly spoke about this with LuHe on IRC, but for prosperity here.

When generating and caching images, there is a good reason to use qApp->DPR it'll be the largest of all plugged in monitors - it can give better results when you end up moving windows about.

For rendering (which some of this code is) you should *always* use the DPR of the relevant paintDevice, that way it'll always be right no matter what DPRs are being mixed.

hetzenecker added a comment.EditedAug 30 2017, 5:22 PM
In D7581#141489, @cfeck wrote:

With Wayland, we can have different scaling factors per screen. Is it better to use QScreen::devicePixelRatio() or QWindow::devicePixelRatio() instead of QApplication::devicePixelRatio()?

That's the point David Edmundson, my GSoC mentor, already raised.
We want to generate pixmaps using QApplication::devicePixelRatio() - so the pixmaps with the highest resolution are in the cache.

For drawing you are correct, there we have to use QPaintDevice::devicePixelRatio(). This needs to be corrected before merging.

We decided to use a similar approach for Okular, testing was successful there. This allows for window moving between different screens of differing scale factors without invalidating the cache.

EDIT: David was quicker to answer, his answer is essentially the same

anthonyfieroni added inline comments.
lib/document/documentloadedimpl.cpp
68–71

This fragment of logic is repeated again and again, just make a static function that do the calculation.

cfeck added a comment.EditedAug 30 2017, 6:02 PM

Does scaledSize() need to be a QSizeF? I fear that if my biggest screen has a 4.0 DPR, then an image of size 121x233 pixels cannot be accurately rendered to a screen with 1.0 DPR because of rounding errors?

I am going on a journey this month - this unfortunately means I also won't have access to the internet.
It'd pick up work on this patch after I return though, if nobody else already has ;)

ngraham edited the summary of this revision. (Show Details)Oct 19 2017, 6:54 PM

@hetzenecker, have you had a chance to work on this any more?

@ngraham unfortunately not yet, but still plan on getting back to it sooner than later..

rkflx added a subscriber: rkflx.Nov 2 2017, 5:22 PM

@hetzenecker Great job so far, this is really appreciated. I tested your patch extensively, see below for details on things which are still off. In summary though, your changes do not regress in both normal and HiDPI setups. Even with some things left to do, the improvements for the main use case of viewing pictures are substantial.

Therefore I would strongly suggest to get this code landed in time for the next beta, which is on November 16. Considering both Ubuntu 18.04 LTS and openSUSE Leap 15 as major milestones are coming up early next year, that's kind of an important release, too. Viewing an image on a HiDPI display is an essential feature on todays desktops. Fixing the rest could be done later and on top, e.g. before the RC, in stable releases or (worst case) for our 18.04.

If you agree, it would be great if you could have a look at the code review comments from above (and my minor nits), so things get going again and we don't wait forever for the perfect patch. I you are not able to make any changes at all until then, please indicate and we'll have to find another solution.


Summary of (minor) issues found

Tested with Qt 5.9.2, KF 5.39, Plasma 5.11.2 and Gwenview master with 1.5x scaling (via QT_SCALE_FACTOR for the app, and briefly for the whole session via systemsettings). Note this was only on a single monitor and only with X (have to start somewhere…), somebody else would need to test with a multi-monitor/multi-dpi setup and on Wayland (@cfeck, perhaps?). I tried to order the findings by priority:

  1. Thumbnails pixelated (as noted above), specifically:
    • on Start page and in Browse mode: folder icons, previews on folders, previews itself, loading placeholder image
    • in View mode and in Fullscreen mode: Thumbnail Bar
    • in Gwenview Importer (likely just reuses the same component as Browse mode)
    • Surprisingly, Videos worked very well for me when testing a 1080p screencast played and recorded at native resolution. Only their thumbnails were pixelated. (But there's another problem unrelated to the patch, I filed bug 386466 – Video aspect ratio distorted for that.)
  1. QT_SCALE_FACTOR=1.5 ./importer/gwenview_importer . still shows pixelated icons all over the UI (probably just missing the general HiDPI enablement), apart from the thumbnail problem.
  1. After modifying an image (e.g. via Rotate), the icon on the overlay save button in Browse mode is pixelated as soon as mouse leaves the mouse over position.
  1. SVGs are not HiDPI aware everywhere (as noted above), but only some zoom levels and SVG sizes are affected apparently.
  1. In Full Screen, the black background texture/grain is pixelated.
  1. Image loading indicator animation is pixelated (to reproduce: open PNG (!) screenshot, assign shortcut to Flip, keep shortcut pressed).
  1. QT_SCALE_FACTOR=1.5 ./tests/auto/imagescalertest fails.
  1. Panning when zoomed in smudges the image in some cases.
  1. In Browse mode, shrinking the visual selection rectangle by moving the mouse results in artefacts (lines) on items not selected anymore.
  1. Scrolling in Recent Files tab leaves line artefact line at the bottom.
  1. Birds eye view: Outline of handle changes width when panning.
  1. Setting a large 48px cursor in systemsettings, the zoom indicator cursor (magnifying glass) is still small.

Note sure whether 9. to 11. are Gwenview's or Qt's problem, might be worth testing with Qt 5.10.

lib/documentview/abstractimageview.cpp
493

Why are you adding parentheses around (d->mImageOffset)?

513

Any reason the order of multiplication and division is different compared to above? (Mathematically the same, of course, but not sure about the compiler wrt. floating point math and the casual reader…).

lib/documentview/abstractimageview.h
94

Nit: coordinatscoordinates

lib/imagescaler.cpp
131

Could you come up with a better variable name for dRect? (not really important, though)

Also, probably worth breaking this line up into multiple lines like below (or use a static as suggested, if applicable here).

202

Line breaks here too, please, as well as potentially better d-naming.

rkflx added a comment.Nov 2 2017, 9:30 PM

Regarding the thumbnails, it might also be worth revisiting D6083: Bump max thumbnail size to 512px (Can we change/extend the FDO spec? Ignore it partly? What are other (GTK-)image viewers doing?).

@rkflx wow, thanks for your thorough review - this provides a really nice roadmap :)
I agree that this patch is essential for todays displays.

I didn't yet submit a second revision, because I tried to fix the thumbnail generator beforehand.. which, unfortunately, was again more difficult than expected.
Nevertheless, I published my current work in progress in git ( https://cgit.kde.org/gwenview.git/log/?h=gsoc2017_hidpi_support ). Thumbnails are still generated too small (which is really weird), and painted at wrong positions (which I expected).
IMHO we should at least also get this working before release, but getting it done before November 16 will be tricky.

I think I also noticed one regression during my last test, so let me add number 13:

  1. Rendering is broken with Animations set to OpenGL in Settings -> Configure Gwenview.. -> Image View
rkflx added a comment.Nov 5 2017, 10:20 AM
  1. Rendering is broken with Animations set to OpenGL in Settings -> Configure Gwenview.. -> Image View

Weird, I'm sure I tested this, but now it fails for me too, i.e. there's an offset in View mode when switching to OpenGL. However, trying without any HiDPI patch with scaling enabled this still fails, so at least this is no regression.

I published my current work in progress in git

Thanks, good to know. Tried it briefly, let's say in terms of shipping quality it is the first part of "one step backward, two steps forward" ;)

IMHO we should at least also get this working before release, but getting it done before November 16 will be tricky.

Having HiDPI thumbnails would be great, but I don't think this should prevent us from at least shipping a HiDPI-enabled View mode in the Beta. I know this is boring work, but would it be possible at all (not that I can tell you what to work on) to first do the code cleanups so the first part of the patch can land, and do the thumbnails afterwards? In terms of testing and not doing too invasive changes for RC and stable branches this would be much more preferable. Smaller patches also result in better git bisecting.

rkflx added a comment.Nov 9 2017, 11:36 PM

@hetzenecker Any news, what's your plan here? To reiterate: Having at least some HiDPI support (without HiDPI thumbnails for now, but maybe added later) is better than none at all for the complete 17.12 cycle. Please let us know if you need any help in polishing the current patch for the Beta or if we should commit on your behalf. Also, for new code please allow for at least some days of review before Thursday.

  1. Rendering is broken with Animations set to OpenGL in Settings -> Configure Gwenview.. -> Image View

Just for reference: Nate dug out a corresponding bug + duplicate in his amazing triaging of Gwenview bugs today: https://bugs.kde.org/show_bug.cgi?id=350883

@rkflx just wanted to give you a quick update so you know I haven't abandoned this.
I've made some helper functions for shrinking and enlarging QRect(F)s in PaintUtils.
Those were really useful and also simplified the zoom scaling quite a bit. Now the only thing left to do is finding out how the heck this simple change completely broke the rendering - again. ;)
But I expect to have this ready in soon™.

rkflx added a comment.Nov 15 2017, 7:22 PM

Thanks for the update, I know you won't abandon this (it's only your current state/plans I'm sometimes left wondering about :).

Just to prevent any disappointment: As the cutoff deadline is tonight, there is simply not enough time to test this properly. And as you mentioned yourself, the code is quite brittle, which is a strong indicator we should not try to bend the rules and sneak this in after the Beta.

Nevertheless, soon™ would still be appreciated (e.g. by those getting shiny HiDPI computers for Christmas and having someone to compile Gwenview from git master for them…). Also don't hesitate to ask for help, upload a WIP version so others can have a look, or maybe d_ed has some tips?


At the risk of diverting your attention from Gwenview, I want to at least mention there is a severe CPU consumption regression when panning with HiDPI scaling in Okular. See https://phabricator.kde.org/D8379#164630, grep for 1.1 (haven't got around to open a task on the Okular workboard to track remaining HiDPI issues yet, still plan to do it though).

Anyway, thanks for the update on Gwenview. I'm sure you'll solve the problem eventually ;)

hetzenecker marked 6 inline comments as done.Nov 15 2017, 9:20 PM
hetzenecker added inline comments.
lib/documentview/abstractimageview.cpp
493

removed them again, there was a lot of "playing with scaling factors" involved in these code paths..

lib/imagescaler.cpp
131

added an explanation

hetzenecker marked 2 inline comments as done.

Yes, this definitely still needs some work.

Extracted QRect scaling to helper methods (there seems to be rounding errors now, those still have to be fixed)

Hi, if you haven't already, please have a look at D9078, where I figured out HiDPI for thumbnails. I would love to have some feedback, As for the small thumbnail issue @hetzenecker was facing, I was facing that too, but I realized I needed to generate bigger thumbnails, so if I have 2x scaling, I generate 256px * 2 = 512px wide thumbnails, unlike D6083 which suggests upping the base thumbnail size itself. The HiDPI patch for Okular (thank you @hetzenecker!) landed and I'm glad I can finally switch back from evince, and it would be great to have gwenview fixed soon as well!

Any update on this?

Any update on this?

@ngraham I've been out of town on an internship since early Jan, and haven't been able to test some of the HiDPI stuff across monitors with different scaling, so I've not made a lot of progress since. I'm coming back in a few weeks, and I can catch up after that

Thanks for the update!

fvogt added a subscriber: fvogt.Jul 18 2018, 9:36 AM

Any news here? 18.08 is very close, maybe even too late...

@fvogt I haven't worked on it for quite a while. I can resume working on this in a few weeks, after my exams are done.

This patch is going to have its first birthday soon, and Gwenview is about to be released once again without HiDPI support...

@hetzenecker, are you planning to finish this work, or does someone else need to take over? @abalaji maybe? Gwenview is very actively developed for these days, so the longer this patch sits around unmerged, the more conflicts it's going to accumulate.

@ngraham I can resume next week, having my exams right now

rkflx requested changes to this revision.Aug 6 2018, 5:52 PM

Yeah, a lot has happened since last year. After the finishing touches are done, this should be checked carefully again.

Let's wait until after Akademy with taking a decision though, since I'd assume @hetzenecker has been and still is busy with that (?).

This revision now requires changes to proceed.Aug 6 2018, 5:52 PM

Unfortunately I was also busy with other work recently, so I could not persue this any further.
Perhaps it would be a good idea to get together and discuss this at Akademy (e.g. how to handle the freedesktop.org thumbnail caching spec). Will you join Akademy this year?

Unfortunately I was also busy with other work recently, so I could not persue this any further.
Perhaps it would be a good idea to get together and discuss this at Akademy (e.g. how to handle the freedesktop.org thumbnail caching spec). Will you join Akademy this year?

Yep, I am!

rkflx added a comment.Aug 7 2018, 5:24 PM

how to handle the freedesktop.org thumbnail caching spec

See D6083#293394 for some discussion.

Also as said above, I'd suggest to ignore thumbnails for this patch (which is mainly about View mode). Otherwise it might take even longer to finish it.

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

@hetzenecker, looks like we missed each other at Akademy. :-( Has there been any progress on this? The patch is about to have its first birthday in two days. :)

asn added a comment.Sat, Nov 17, 11:34 AM

Reminder ...