Bug 460681

Summary: Laggy scrolling on high DPI display - profiling indicates excessive copying in PagePainter::paintCroppedPageOnPainter
Product: [Applications] okular Reporter: maximumsomething <maximumsomething>
Component: generalAssignee: Okular developers <okular-devel>
Status: RESOLVED FIXED    
Severity: normal CC: oliver.sander
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Debian testing   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:

Description maximumsomething 2022-10-18 19:32:34 UTC 
SUMMARY
On my computer - a laptop with a 4k display set to 250% scaling - Okular scrolls through PDFs at ~30 FPS and consumes 100% CPU while doing so. I did some profiling, and the main thread was spending 80% of its time on PagePainter::paintCroppedPageOnPainter. 

Looking at the source code, it's doing a few things very inefficiently:
(1) It asks the input page for a QPixmap, then dereferences it and calls QPixmap::setDevicePixelRatio, which creates a deep copy of the pixmap;
(2) Whenever it wants to draw a cropped scaled rectangle of the pixmap, it first creates a scaled copy of the pixmap then draws it rather than drawing it directly.

(1) is specifically related to having a non-1 scale factor, while (2) is I suspect just exacerbated by the high pixel count of the display.

I was going to go ahead and fix these things myself, but I was wondering if anyone familiar with the code knows why it was written this way/any gotchas that might apply if the deep copies are not made.

STEPS TO REPRODUCE
1. Open any PDF on a display with a high scale factor.
2. Scroll around.


SOFTWARE/OS VERSIONS
Linux/KDE Plasma:
KDE Plasma Version:  5.25.5
KDE Frameworks Version: 5.98.0
Qt Version: 5.15.4
Graphics Platform: X11
Comment 1 Oliver Sander 2022-10-19 06:43:42 UTC 
Some of the calls to `setDevicePixelRatio` where probably introduced by me. The deep-copying was not intentional.  I am not very proficient in Qt programming, and I wasn't even aware that these calls trigger a deep copy.

Does https://invent.kde.org/graphics/okular/-/merge_requests/612 fix your issue by any chance?
Comment 2 maximumsomething 2022-10-19 17:36:58 UTC 
It would fix the biggest problem, yes. However, it turns out setting the dpr of the source pixmap isn't necessary at all. I went ahead and made a merge request that addresses both problems. See https://invent.kde.org/graphics/okular/-/merge_requests/665 .
Comment 3 Laura David Hurka 2022-10-20 16:30:10 UTC 
(In reply to maximumsomething from comment #0)
> I was going to go ahead and fix these things myself, but I was wondering if
> anyone familiar with the code knows why it was written this way/any gotchas
> that might apply if the deep copies are not made.

I thought I would just mention that I have made https://invent.kde.org/graphics/okular/-/merge_requests/240 long time ago.
The intention was to simplify the code, just like the title suggests.
I also tried to understand the code, and prepare it for peformance improvements.

I was not aware of deep copying, actually.
My patch still uses setDevicePixelRatio(), so it actually does not fix this problem.

Honestly I lost interest in this.
If you want, you may continue the work, or just look at it if it helps you to understand anything. :)
Comment 4 Oliver Sander 2022-12-08 11:59:07 UTC 
Fixed by merging https://invent.kde.org/graphics/okular/-/merge_requests/665 .