Only supported by the pdf backend if using poppler >= 0.63
Sadly had to change the generator API
Text cancellation is a bit wobbly still since poppler has large parts
of the code where doesn't check for the cancellation flag, but that
is something that will get automagically fixed for us if the poppler
side is improved
| No Linters Available |
| No Unit Test Coverage |
I tested this a bit and find it really nice that rendering of the actual page (rather than the thumbnail) usually starts at once when re-rendering is needed (e.g. when zoom level is changed).
Also, closing Okular now works fast, even if rendering is still in progress. (The shell prompt is shown almost immediately after closing Okular when it was started from the command line.)
While the above is true most of the time, it happens from time to time that rendering does not start at once.
I can reproduce this e.g. the following way:
While re-rendering is triggered most of the times at once, the page sometimes remains "blank" for more than 30 seconds.
In those cases where the page is "blank", it also takes long for the Okular process to finish when closing the program at that point in time.
I (temporarily) uploaded a screencast here: https://ombx.io/4SDa42mm
Another thing I realized is that re-rendering is now triggered every time the zoom level is changed.
Without this change, rendering the page is sometimes just continued at the place where it was before changing the zoom level (which results in the final page appearing earlier when no re-rendering is needed).
That seems like a bug, it shouldn't do that, i'll try to reproduce here.
Another thing I realized is that re-rendering is now triggered every time the zoom level is changed.
Without this change, rendering the page is sometimes just continued at the place where it was before changing the zoom level (which results in the final page appearing earlier when no re-rendering is needed).
Yes and no, what happened before is that after the first rendering finished a second one would start with the correct quality for the current zoom, but since the first one was probably "good enough" you got a scaled version while the second one was rendering, which sometimes it may be good, some other times it may be ultra pixelated, now we just cancel the first one and start the second one right away, so the time to "best/correct rendering" is shorter but the time to "something renderer" is a bit larger. I do think that this approach is better.
@michaelweghorn can you give it a try with the new version of the patch? This should make it better.
@ngraham @rkflx It would be great if you could find some time "soon" to review this
i have work-work-time assigned for this, but as far as i understand only this month, so if you find there's changes needed it would be great if it could be *now* so i can put the time to do it *now* too and not when i find hobby-time. Not sure if i'm making sense :D
First things first: The patch marks an impressive improvement in (perceived) drawing performance for slow-rendering PDFs, I'm glad your hard work payed off.
See below for some minor comments regarding the code, but obviously I lack Okular/Poppler knowledge to review this in depth. I noticed D8379 added some autotests, but here we get nothing in that regard. Do you think there might be a way to autotest the intended behaviour of the rendering a bit, e.g. correct order of main canvas / thumbnail rendering, cancelling on zoom/resize/pan etc.? Just asking, not a requirement for acceptance of course.
As for testing:
#0 0x00007ffff2306893 in _int_free (av=0x7ffff2639c20 <main_arena>, p=0x7affb0, have_lock=0) at malloc.c:4184
#1 0x00007ffff233bba5 in tzset_internal (always=1) at tzset.c:403
#2 0x00007ffff233bd68 in tzset_internal (always=1) at tzset.c:553
#3 __tzset () at tzset.c:555
#4 0x00007ffff233ac29 in __GI_mktime (tp=0x7fffff7ff0e0) at mktime.c:588
#5 0x00007ffff3019291 in qt_mktime(QDate*, QTime*, QDateTimePrivate::DaylightStatus*, QString*, bool*) ()
from /usr/lib64/libQt5Core.so.5
#6 0x00007ffff301a109 in refreshDateTime(QDateTime::Data&) () from /usr/lib64/libQt5Core.so.5
#7 0x00007ffff301a3b0 in setDateTime(QDateTime::Data&, QDate const&, QTime const&) () from /usr/lib64/libQt5Core.so.5
#8 0x00007ffff301cfb9 in QDateTime::setMSecsSinceEpoch(long long) () from /usr/lib64/libQt5Core.so.5
#9 0x00007ffff301f5a7 in QDateTime::fromMSecsSinceEpoch(long long, Qt::TimeSpec, int) () from /usr/lib64/libQt5Core.so.5
#10 0x00007ffff301f8c8 in QDateTime::currentDateTime() () from /usr/lib64/libQt5Core.so.5
#11 0x00007ffff301f933 in QTime::currentTime() () from /usr/lib64/libQt5Core.so.5
#12 0x00007fffda79034b in Okular::DocumentPrivate::getFreeMemory (this=0x7c7d20, freeSwap=0x0)
at okular/core/document.cpp:430
#13 0x00007fffda78f671 in Okular::DocumentPrivate::calculateMemoryToFree (this=0x7c7d20)
at okular/core/document.cpp:229
#14 0x00007fffda7952ae in Okular::DocumentPrivate::sendGeneratorPixmapRequest (this=0x7c7d20)
at okular/core/document.cpp:1248
#15 0x00007fffda7a0bc2 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...)
at okular/core/document.cpp:3325
#16 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...)
at okular/core/document.cpp:3177
#17 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0)
at okular/ui/thumbnaillist.cpp:634
#18 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1)
at okular/ui/thumbnaillist.cpp:361
#19 0x00007fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...)
at okular/core/document.cpp:3328
#20 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...)
at okular/core/document.cpp:3177
#21 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0)
at okular/ui/thumbnaillist.cpp:634
#22 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1)
at okular/ui/thumbnaillist.cpp:361
#23 0x00007fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...)
at okular/core/document.cpp:3328
#24 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...)
at okular/core/document.cpp:3177
#25 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0)
at okular/ui/thumbnaillist.cpp:634
#26 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1)
...
#495 0x00007fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...)
at okular/core/document.cpp:3328
#496 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...)
at okular/core/document.cpp:3177
#497 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0)
at okular/ui/thumbnaillist.cpp:634
#498 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1)
at okular/ui/thumbnaillist.cpp:361
#499 0x00007fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...)
at okular/core/document.cpp:3328
#500 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...)
at okular/core/document.cpp:3177
#501 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0)
at okular/ui/thumbnaillist.cpp:634
#502 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1)
...ASSERT: "(*rIt)->observer() == requesterObserver" in file okular/core/document.cpp, line 3204 Aborted (core dumped)
| core/document.cpp | ||
|---|---|---|
| 3125 | has higher priority | |
| 3129 | has lower priority | |
| 3133 | has same priority as | |
| 3311 |
| |
| 3349 | Add { }. | |
| generators/chm/generator_chm.cpp | ||
| 415 | const | |
| generators/djvu/generator_djvu.cpp | ||
| 212 | const | |
| generators/dvi/generator_dvi.cpp | ||
| 252 | const | |
| generators/poppler/generator_pdf.cpp | ||
| 1183 | const | |
| generators/xps/generator_xps.cpp | ||
| 2123 | const (not really...) | |
There's no autotests in D8379, no?
Do you think there might be a way to autotest the intended behaviour of the rendering a bit, e.g. correct order of main canvas / thumbnail rendering, cancelling on zoom/resize/pan etc.? Just asking, not a requirement for acceptance of course.
It is inside the realms of doable, but doesn't seem trivial sadly, not sure if i have enough time to devote to adding them
As for testing:
- I'm using Qt 5.10.0, Okular master and Poppler master (Poppler patch did not apply cleanly, though) with dublin-center-street.pdf for now.
I've updated the poppler patch now.
- So far I could not yet perform very extensive test runs, but I thought I'd share the first problems I found so you can start looking into it. Will do more testing in a bit. Let me know if you think the problem is somewhere on my side.
- Segfault when resizing window. Backtrace:
Right, this should be fixed with the new patch i'll upload
- OOM killed when changing sidebar size.
Can not reproduce, maybe fixed by the fix to 1) since it was sidebar related. What i can reproduce is the UI getting a bit stuck if you resize the sidebar while the page is rendering, but that already happens without this patch so at least it's not a regression, i had a quick look and can't really figure out what is going on, i'll try to have another look, but don't think we should block on this
- ASSERT when deselecting "parking (level 7)" in Layers sidebar:
Right, fixed.
- No text rendering in some situations (observed this multiple times when wildly zooming and jumping around via the thumbnail view, sadly don't have a concise video yet).
That's really weird since text is "just" part of the regular rendering, you should either get no rendering at all or it working, but having no text doesn't make much sense. It may be that the tile got cancelled and wasn't cleared. I'll investigate a bit more and see if i can reproduce it.
| core/document.cpp | ||
|---|---|---|
| 3349 | Nope, not okular style (if it has any :D) see 5 lines below | |
Better, but not perfect yet ;)
There's no autotests in D8379, no?
Seems I mixed it up with https://phabricator.kde.org/D8642, sorry.
- OOM killed when changing sidebar size.
I suspect there are multiple issues triggered by sidebar resizing:
I'm not sure about the next one, maybe you could comment whether this is related to the patch and if it is intentional or not:
I'll finalize testing today or tomorrow.
| generators/poppler/generator_pdf.cpp | ||
|---|---|---|
| 1183 | Any reason you changed it everywhere but here? | |
You got this one changing the size of the sidebar? or doing what?
I'm not sure about the next one, maybe you could comment whether this is related to the patch and if it is intentional or not:
- Text page generation not cancellable. This can be observed by comparing the reaction to F5 being pressed while doing incremental rendering vs. shortly after that. Alternatively, zoom in after rendering finished but thumbnail rendering did not start yet, where now you'd have to wait a while until rerendering triggers again.
It is cancellable, but wasn't being cancelled on closing the document (F5), I've fixed that now.
- ASSERT: "page()" in file okular/core/generator_p.cpp, line 129 (Happened two times already, but not everytime.)
You got this one changing the size of the sidebar?
That's correct. Now I even have somewhat reliable steps to reproduce (you'd still need a bit of luck, though). Open the document and while it is rendering resize the sidebar so X gets stuck, then wait for the ASSERT:
Also, the ASSERT problem is not unique to vector heavy documents like dublin-center-street.pdf, I get it too for an image heavy 50 page document while holding Space to scroll until it crashes. After reopening the document, Okular would even crash immediately then, meaning this probably has nothing to do with the stuck sidebar (which is only the trigger in one case).
In fact, scrolling in almost every multipage document triggers the problem, which is quite serious. This was not there before (I checked specifically by only undoing the Okular patch).
- Text page generation not cancellable
cancel text extraction on document close
I'm still not sure whether it's not working correctly or I'm not understanding what "cancelling" means. In the following video, I Close during rendering (immediate reaction) and then I Close during text extraction (long delay):
To me this looks like text extraction in just one huge uninterruptible chunk, and only after it has finished Okular will react with whatever I wanted to do (in this case Close). The same happens for Zoom In, where text extraction is not "stopped" and thus I get to see the pixelated rendering for quite a while (I did not test any other actions yet):
I'll finalize testing […] tomorrow.
Done. Other than the issues noted above I did not find any additional oddities or crashes when testing more extensively (different documents, more features).
(In HiDPI mode we still have those double redrawing / no redrawing problems, but those are unrelated to this patch. Mentioning it only so that nobody testing in HiDPI mode gets confused.)
I'm setting the status to "Changes requested" for now, because the crashes happen for almost every scientific document I tested (we have lots of users in this area) and apply to a Release build too. Apart from that, the overall state of the patch is quite good.
Ok, i found how to reproduce too and this should be fixed now.
- Text page generation not cancellable
cancel text extraction on document close
I'm still not sure whether it's not working correctly or I'm not understanding what "cancelling" means. In the following video, I Close during rendering (immediate reaction) and then I Close during text extraction (long delay):
Right, i did some more testing and sometimes it doesn't cancel "as interactively" as you'd want, the problem is in two areas:
I tried adding some more "should i cancel" to the poppler patch, and ended up with the second problem.
To simplify a bit text extraction is two things, "drawing" the text and then rearranging it in something that makes sense as sentences.
The "drawing" part is cancellable since it shares code with the actual drawing, but the rearranging doesn't really have support for it, and i've played with it a bit and it's not immediately obvious how to make it exit early without leaking. So I would really prefer if we would delay this as a future improvement.
To me this looks like text extraction in just one huge uninterruptible chunk, and only after it has finished Okular will react with whatever I wanted to do (in this case Close). The same happens for Zoom In, where text extraction is not "stopped" and thus I get to see the pixelated rendering for quite a while (I did not test any other actions yet):
This is "as designed", basically at that point the pixmap has been generated, the text is being generated and now we get a new pixmap request, but you already have "something to see" so in that case i decided to not cancel the text generation.
Can confirm, and I just noticed that simply running make test would've found this problem too (I kinda assumed you ran this, so did not check again).
However, the autotests still have lots of issues (at least for me). As an example, ./autotests/parttest testReload results in
Thread 1 "parttest" received signal SIGSEGV, Segmentation fault.
0x00007ffff74c1930 in Okular::TextPageGenerationThread::abortExtraction (this=0x0)
at okular/core/generator_p.cpp:113Could you recheck?
- Text page generation not cancellable
[…]
Ok, thanks for the explanation, makes sense. It's not ideal, but good enough for now in my book. Maybe add a note to the commit message?
Wops, the code assumed that there would be always a text generation thread but that's not necessarily true on the tests. fixed.
Also fixed a different crash that happened on the tests (and at least this one seems like a real bug).
So good thing we have tests.... i should run them more often :D
- Text page generation not cancellable
[…]
Ok, thanks for the explanation, makes sense. It's not ideal, but good enough for now in my book. Maybe add a note to the commit message?
Ok, added a small blurb.
Yay, make test looks much better now. (I still get some failures, but those are also present without the patch – no time to bisect right now though, sorry. Let's see what the CI says.) Short manual testing was fine too.
…like on every arc diff? ;) See arc unit. I suspect this way it would be possible to light up the green star in Phab's Diff Detail → Unit section. Peter did something similar recently for arc lint (see https://phabricator.kde.org/D9943#193322).