| davidedmundson | |
| aacid |
HiDPI Support: This is my initial draft that enables HiDPI throught the application
Every pixmap is multiplied by the devicePixelRatioF
QPainter code is ajusted to take the DPR value into account
Whereever possible, code was simplified to make use of modern QPainter features
BUG: 362856
BUG: 383589
| Lint Skipped |
| Unit Tests Skipped |
"Test plan" simply needs to be a list of things you've tried doing so the maintainers can point out if there's any parts you've not thought about.
So saying if you tested annotations and what types of files you opened, checked with zoom = 100%..that sort of thing.
| ui/pagepainter.cpp | ||
|---|---|---|
| 75–76 | Why? (maybe ceil() ? | |
| 410 | If the code below works, I'd avoid changing it. Painting something on top isn't quite the same as the exitsing multiplying the two values together, and the old code has that //for odt or epub hack swapping black pixels for white ones. | |
| 911 | Can you explain on Phabricator the changes you've made in PagePainter, and that benchmarking you did. | |
| ui/pageview.cpp | ||
|---|---|---|
| 5324 | This introduce a regression, since pointer can stay uninitialized or nullptr, cause a crash in line 5334 | |
For me the patch didn't quite apply to the latest okular git master. Easy to fix, but still. The problem was the cosmetic changes in shell/main.cpp.
After brief testing, the patch appears to do what it is supposed to do in normal page view mode. In presentation mode, however, the documents still look blurry.
Strangely, with the patch applied, I cannot change slides anymore in presentation mode. Does anybody else see this behavior?
Thanks for all your feedback so far, I promise to address it soon in detail; just want to get another patch out before continuing with this one
I've rebased my branch to master
After brief testing, the patch appears to do what it is supposed to do in normal page view mode. In presentation mode, however, the documents still look blurry.
Strangely, with the patch applied, I cannot change slides anymore in presentation mode. Does anybody else see this behavior?
I could reproduce this and included presentation mode in my test plan. Will look into this for the next revision
I've defined my test cases here (sorry for using an external service, i wanted to show the test cases in a matrix/spreadsheet; you don't need an account for viewing)
and will look into fixing presentation mode and some remaining zooming issues for the next revision
I would very much prefer if you didn't break the ABI of the files in core/
Also not sure why you need to pass the devicepixel ratio up and down, isn't it the same basically in all the app?
Thanks for your feedback.
Yes and no. While it would be possible to use qApp->devicePixelRatio() (as I've done in mobile/components), this would return the highest pixel ratio in the system, in case there are more physical screens with different Scale Factors connected.
So, as alternative QScreen::devicePixelRatio could be used to return the DPR of the screen the application is shown. I'm not sure yet if this is possible without changing the ABI, but I'll try to.
qApp->DPR can change within the lifespan of the app; for example plugging in a new monitor can change things.
This does mean it's very important to keep the metadata of the pixmap's dpr with the pixmap. It ends up simpler and safer overall.
Doesn't mean we need to break ABI though.
There are two breaks RequestPixmap::RequestPixmap and Page::_o_nearest
For requestPixmap we're sure to not process any screen events in the meantime, so you can use qApp->devicePixelRatio() in the two places in document and in the line in generator.
For use of Page::_o_nearest you can't.
However, there is an easy way to keep ABI for this sort of thing.
If you have
someMethod(int a) and want to change it to someMethod(int a, qreal dpr)
keep both, but make the first method simply call the second with a default parameter.
| core/generator.cpp | ||
|---|---|---|
| 106 | sure about this one? | |
Marking as needs changes so it does not appear on my "needs review" list, please mark it back once you fix the comments by david
Fixed presentation mode
Fixed scaling using TileManager
All pixmaps get cached with the highest DPR of all screens. When moving the application to another screen, the cache doesn't have to be invalidated. This also simplified the code, because all PixmapRequests can be upscaled by the highest DPR. No more ABI breakage, because my changes can now be isolated to PixmapRequest and PagePainter/PresentationWidget
| ui/pagepainter.cpp | ||
|---|---|---|
| 70 | can you add some comments on your "d" prefix. | |
| 113 | we don't want to be doing this - copy does a deep copy which is expensive QPixmap pixmap = *p would probably work as well, and that shares the underlying image data. or we could just use p for this section, it doesn't seem like pixmap is used outside here. | |
| ui/presentationwidget.cpp | ||
| 842 | ... | |
I just tested again, and it looks much better than before. Thanks!
There is now a problem with annotation drawing, though. When annotating a document, no annotation will actually be drawn until a repaint is triggered.
Steps to reproduce:
Incidentally, I found another Okular hidpi problem:
https://bugs.kde.org/show_bug.cgi?id=383589
:-)
I do see minor rendering artifacts every now and then. Single rows of pixels seem to occur twice---possibly on tile boundaries. See the attached example image: the center line of text is slightly stretched. This happens at certain zoom levels. I don't really know how to reproduce it reliably.
Addressed the previous comments, change includes in particular:
Hi Lukas, I tested and all my previous complaints seem to be resolved. Thank you!
But of course I also found yet another issue: :-) When drawing in presentation mode, the resolution of the drawing is too low. The lines look pixelish.
But of course I also found yet another issue: :-) When drawing in presentation mode, the resolution of the drawing is too low. The lines look pixelish.
Can I confirm that that's not a regression from the pre-patch status but just something else that could be improved upon.
From my POV (pending a reply from Sander) this is good to go.
| ui/pageviewannotator.cpp | ||
|---|---|---|
| 1122 | that's a redundant else branch, and it won't get optimised out as technically QString() is different from QString(""); | |
I don't know. But as this thread's title is "HiDPI Support for Okular", and as Lukas has turned into an Okular rendering expert I just mention here everything I notice. Feel free to ignore me.
Oh I didn't mean it like that. Your input has been really really valuable.
I was just trying to work out if we should block this and get it fixed, or try to merge this and then work on any remaining improvements.
Midair collision. I'm not saying it is not good to go. I am just saying that there are more HiDPI issues left.
Another one is
https://bugs.kde.org/show_bug.cgi?id=383943
But as noted there, it is not a regression, and should be handled separately.
Regarding the annotation toolbar (BUG 383589): Icons looking much better now, thanks! Two observations, though:
Adressed raised problems by sander and David:
And a huge thanks to all of you for your valuable feedback, this really helps to stay sane :-)
Oh, and before I forget:
TIFF files are generally broken, there is a bug report for that: https://bugs.kde.org/show_bug.cgi?id=371828 (noticed that after endless hours of debugging ;) )
| conf/widgetconfigurationtoolsbase.cpp | ||
|---|---|---|
| 29 | The size for the pixmaps is hardcoded to 32 in pageviewannotator.cpp, so I don't quite understand how this could have worked before.. | |
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 ;)
Enjoy!
I may have take on at https://bugs.kde.org/show_bug.cgi?id=384143 myself. Any ideas on where to start looking?
I just had a look at it.. and no, sorry, I didn't have enough time to spot the error. Guess you are on your own :/
I started running with this branch, and it enables me to use Okular again, without this the rendering is so blurry that I switched to another PDF viewer. So thanks a lot for all the work that went into this :)
Nice!
Can we close this out so the bugs mentioned in the Summary get updated and marked as resolved?
Closing this won't close the bugs.
Unfortunately the syntax used for the committed change is incorrect:
Too late now. The bugs should be closed manually.
Thanks for all your comments!
This was merged with commit ecc1141e0293e1a30e0f8787d86dcc6309ffb0c0 ( https://cgit.kde.org/okular.git/commit/ui?id=ecc1141e0293e1a30e0f8787d86dcc6309ffb0c0 )
Unfortunately I've used the wrong REVIEW tag, so this didn't get closed automatically.
