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
davidedmundson | |
hetzenecker | |
ngraham | |
rkflx |
Gwenview |
Initial support for HiDPI-scaling of documents in RasterImageView.
This patch scales up images to display them correctly on HiDPI-enabled screens.
TODO:
BUG: 373178
No Linters Available |
No Unit Test Coverage |
Buildable 10434 | |
Build 10452: arc lint + arc unit |
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.
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
lib/document/documentloadedimpl.cpp | ||
---|---|---|
68–71 ↗ | (On Diff #18870) | This fragment of logic is repeated again and again, just make a static function that do the calculation. |
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 unfortunately not yet, but still plan on getting back to it sooner than later..
@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.
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:
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: coordinats → coordinates | |
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. |
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:
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.
@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.
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™.
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 ;)
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!
@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
@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.
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 (?).
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?
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.
@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. :)
At this point, the patch will need to be rebased since a lot has happened since August of 2017 (!). Either @hetzenecker will need to return and finish the work, or another interested party will need to take it over.
I agree that it sure would be nice to get this in, even if full high DPI support is not 100% complete yet. Something is better than nothing. :)
Rebasing against 19.04 branch was possible with minor conflicts in 5 files, subsequently tested it at runtime on top of 18.12.3 where it applied unchanged, at least on a regular non-HiDPI screen it did not see a difference which is probably also a good thing.
Thanks @asturmlechner! Any chance you want to Commandeer this patch (Add Action... → Commandeer Revision), rebase it, fix the conflicts, and finish it up? It such a shame to let work that's 90+% done just sit around. :)
lib/document/document.cpp | ||
---|---|---|
355 ↗ | (On Diff #22415) | dprScaledSize? |
Would you like to take over this patch and address the few remaining issues so we can finally get it landed?
Thanks! That would be fantastic. @rkflx found a bunch of little things here: https://phabricator.kde.org/D7581#163655
He also mentioned that those are minor issues that coulbe be fixed later.
This change is rather big and complicated, so it would be better to leave it with fixing one main issue on HiDPI displays.
@hetzenecker wrote: "Extracted QRect scaling to helper methods (there seems to be rounding errors now, those still have to be fixed)",
so I guess this is the only thing that needs to be checked for this patch.
Currently there is a bug in Qt which makes QGraphicsOpacityEffect not working: https://codereview.qt-project.org/#/c/257946/
lib/imagescaler.cpp | ||
---|---|---|
49 | Avoid the term "scaled" in any code anywhere. Things are scaled from logical -> native Without knowing which way you're transforming the word means nothing. In this particular case you're going from logical to native. |
lib/imagescaler.cpp | ||
---|---|---|
49 | scaledRect() is also used for zoom. It's like operator*() for rects. |
Nice to see this. It's definitely better than nothing, but HiDPI support for the thumbnails would be nice too. :)
A surprisingly clean diff. Very nice.
lib/documentview/documentview.cpp | ||
---|---|---|
477 | Can you add a comment explaining this, with a link to the relevant Qt bug report or patch? |
Wow, it's so great to see this finally merged.
@volkov - a huge thank you that you took over this patch and for all the work you put in to finally resolve this.
I didn't really follow the conversation anymore, so only noticed the messages just now (after seeing the entry in @ngraham s blog) - so sorry that I didn't have the time to complete the work, life was really hectic after the GSoC :-( But, again, thank you that you did 😊
You are welcome!
I started to implement highdpi support from scratch, but then I found this change and it was a huge help!