D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid closed this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin, rkflx
Cc: #okular, michaelweghorn, ngraham, aacid

D9328: Add support for cancellable image rendering and text extraction

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.
This revision is now accepted and ready to land.


  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.
  
  In https://phabricator.kde.org/D9328#198363, @aacid wrote:
  
  > So good thing we have tests i should run them more often :D
  
  
  …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).

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin, rkflx
Cc: #okular, michaelweghorn, ngraham, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D9328#198101, @rkflx wrote:
  
  > In https://phabricator.kde.org/D9328#197788, @aacid wrote:
  >
  > > In https://phabricator.kde.org/D9328#196555, @rkflx wrote:
  > >
  > > > >> 5. `ASSERT: "page()" in file okular/core/generator_p.cpp, line 129`
  > >
  > >
  > > Ok, i found how to reproduce too and this should be fixed now.
  >
  >
  > 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.
  >   0x774c1930 in Okular::TextPageGenerationThread::abortExtraction 
(this=0x0)
  >   at okular/core/generator_p.cpp:113
  >
  >
  > Could you recheck?
  
  
  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
  
  > 
  > 
  >  ---
  > 
  > 6. 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin, rkflx
Cc: #okular, michaelweghorn, ngraham, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid updated this revision to Diff 26246.
aacid edited the summary of this revision.
aacid removed subscribers: rkflx, ervin, michaelweghorn, ngraham.
aacid added a comment.


  Fix crash found by the tests. Good we have them!

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9328?vs=26172=26246

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/area.h
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page.cpp
  core/page_p.h
  core/rotationjob.cpp
  core/rotationjob_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  core/tilesmanager.cpp
  core/tilesmanager_p.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid, ervin, rkflx
Cc: #okular, michaelweghorn, ngraham, aacid

D9328: Add support for cancellable image rendering and text extraction

[Henrik Fehlauer]

rkflx added a comment.


  In https://phabricator.kde.org/D9328#197788, @aacid wrote:
  
  > In https://phabricator.kde.org/D9328#196555, @rkflx wrote:
  >
  > > >> 5. `ASSERT: "page()" in file okular/core/generator_p.cpp, line 129`
  >
  >
  > Ok, i found how to reproduce too and this should be fixed now.
  
  
  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.
0x774c1930 in Okular::TextPageGenerationThread::abortExtraction 
(this=0x0)
at okular/core/generator_p.cpp:113
  
  Could you recheck?
  
  ---
  
   6. 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?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin, rkflx
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D9328#196555, @rkflx wrote:
  
  > >> 5. `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?
  >
  > 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).
  
  
  Ok, i found how to reproduce too and this should be fixed now.
  
  > 
  > 
  >  ---
  > 
  >>> 6. 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):
  > 
  > F5676037: close-not-cancelling.webm 
  
  Right, i did some more testing and sometimes  it doesn't cancel "as 
interactively" as you'd want, the problem is in two areas:
  
  - Checking "should i cancel" too often will make it slower
  - If the code is not written in a way to exit early you'll end up with memory 
leaks
  
  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):
  > 
  > F5676039: zoom-not-cancelling.webm 
  
  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin, rkflx
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid updated this revision to Diff 26172.
aacid added a comment.


  Fix issues with the text thread generator

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9328?vs=25991=26172

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/area.h
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page.cpp
  core/page_p.h
  core/rotationjob.cpp
  core/rotationjob_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  core/tilesmanager.cpp
  core/tilesmanager_p.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid, ervin, rkflx
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Henrik Fehlauer]

rkflx requested changes to this revision.
rkflx added a comment.
This revision now requires changes to proceed.


  >> 5. `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`:
  
  F5676036: assert-after-resize.webm 
  
  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).
  
  ---
  
  >> 6. 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):
  
  F5676037: close-not-cancelling.webm 
  
  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):
  
  F5676039: zoom-not-cancelling.webm 
  
  ---
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin, rkflx
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid updated this revision to Diff 25991.
aacid marked an inline comment as done.
aacid added a comment.


  cancel text extraction on document close
  
  add const

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9328?vs=25886=25991

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/area.h
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page.cpp
  core/page_p.h
  core/rotationjob.cpp
  core/rotationjob_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  core/tilesmanager.cpp
  core/tilesmanager_p.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D9328#195937, @rkflx wrote:
  
  > Better, but not perfect yet ;)
  >
  > > There's no autotests in https://phabricator.kde.org/D8379, no?
  >
  > Seems I mixed it up with https://phabricator.kde.org/D8642, sorry.
  >
  > > 2. OOM killed when changing sidebar size.
  >
  > I suspect there are multiple issues triggered by sidebar resizing:
  >
  > - OOM, which seems to be solved with your latest updates.
  > - Unresponsiveness, which has been there before. Observation: This is not 
restricted to Okular, but extends to X. I cannot switch windows (or even move 
window focus via focus-follows-mouse), which is odd because normally even with 
all CPU cores busy (here some are idle, still) that's not a problem. I can move 
the mouse, but the pointer does not change. Maybe Okular has a blocking mouse 
cursor change call, or does too many of them? For now, I opened Bug 389411 
 about this.
  > - NEW issue since your latest updates:
  > - `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? 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:
  > 
  > 6. 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 
(https://phabricator.kde.org/F5), I've fixed that now.

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

> rkflx wrote in generator_pdf.cpp:1177
> `const`

Any reason you changed it everywhere but here?

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Henrik Fehlauer]

rkflx added a comment.


  Better, but not perfect yet ;)
  
  > There's no autotests in https://phabricator.kde.org/D8379, no?
  
  Seems I mixed it up with https://phabricator.kde.org/D8642, sorry.
  
  > 2. OOM killed when changing sidebar size.
  
  I suspect there are multiple issues triggered by sidebar resizing:
  
  - OOM, which seems to be solved with your latest updates.
  - Unresponsiveness, which has been there before. Observation: This is not 
restricted to Okular, but extends to X. I cannot switch windows (or even move 
window focus via focus-follows-mouse), which is odd because normally even with 
all CPU cores busy (here some are idle, still) that's not a problem. I can move 
the mouse, but the pointer does not change. Maybe Okular has a blocking mouse 
cursor change call, or does too many of them? For now, I opened Bug 389411 
 about this.
  - NEW issue since your latest updates:
  
  5. `ASSERT: "page()" in file okular/core/generator_p.cpp, line 129` (Happened 
two times already, but not everytime.)
  
  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:
  
  6. 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.
  
  I'll finalize testing today or tomorrow.

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a comment.


  @rkflx should be hopefully better now, can you have a look?

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid updated this revision to Diff 25886.
aacid added a comment.
This revision is now accepted and ready to land.


  Fixed issues with cancelling tile requests

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9328?vs=25868=25886

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/area.h
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page.cpp
  core/page_p.h
  core/rotationjob.cpp
  core/rotationjob_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  core/tilesmanager.cpp
  core/tilesmanager_p.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid planned changes to this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D9328#195391, @aacid wrote:
  
  > In https://phabricator.kde.org/D9328#193130, @rkflx wrote:
  >
  > > 4. 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.
  
  
  Ok, can reproduce relatively easily now, will try to fix it up.

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid updated this revision to Diff 25868.
aacid marked an inline comment as done.
aacid added a comment.


  Fix crash on resizing window and assert on triggering layer updates

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9328?vs=24109=25868

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid marked 9 inline comments as done.
aacid added a comment.


  In https://phabricator.kde.org/D9328#193130, @rkflx wrote:
  
  > 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 
https://phabricator.kde.org/D8379 added some autotests, but here we get nothing 
in that regard.
  
  
  There's no autotests in https://phabricator.kde.org/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
  
  > 2. 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
  
  > 3. ASSERT when deselecting "parking (level 7)" in Layers sidebar:
  
  Right, fixed.
  
  > 4. 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.

INLINE COMMENTS

> rkflx wrote in document.cpp:3327
> Add `{ }`.

Nope, not okular style (if it has any :D) see 5 lines below

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, aacid

D9328: Add support for cancellable image rendering and text extraction

[Henrik Fehlauer]

rkflx added a comment.


  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 
https://phabricator.kde.org/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:
  
  - 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.
  - 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.
  
  1. Segfault when resizing window. Backtrace:
  
#0  0x72306893 in _int_free (av=0x72639c20 , 
p=0x7affb0, have_lock=0) at malloc.c:4184
#1  0x7233bba5 in tzset_internal (always=1) at tzset.c:403
#2  0x7233bd68 in tzset_internal (always=1) at tzset.c:553
#3  __tzset () at tzset.c:555
#4  0x7233ac29 in __GI_mktime (tp=0x7f7ff0e0) at mktime.c:588
#5  0x73019291 in qt_mktime(QDate*, QTime*, 
QDateTimePrivate::DaylightStatus*, QString*, bool*) ()
   from /usr/lib64/libQt5Core.so.5
#6  0x7301a109 in refreshDateTime(QDateTime::Data&) () from 
/usr/lib64/libQt5Core.so.5
#7  0x7301a3b0 in setDateTime(QDateTime::Data&, QDate const&, QTime 
const&) () from /usr/lib64/libQt5Core.so.5
#8  0x7301cfb9 in QDateTime::setMSecsSinceEpoch(long long) () from 
/usr/lib64/libQt5Core.so.5
#9  0x7301f5a7 in QDateTime::fromMSecsSinceEpoch(long long, 
Qt::TimeSpec, int) () from /usr/lib64/libQt5Core.so.5
#10 0x7301f8c8 in QDateTime::currentDateTime() () from 
/usr/lib64/libQt5Core.so.5
#11 0x7301f933 in QTime::currentTime() () from 
/usr/lib64/libQt5Core.so.5
#12 0x7fffda79034b in Okular::DocumentPrivate::getFreeMemory 
(this=0x7c7d20, freeSwap=0x0)
at okular/core/document.cpp:430
#13 0x7fffda78f671 in Okular::DocumentPrivate::calculateMemoryToFree 
(this=0x7c7d20)
at okular/core/document.cpp:229
#14 0x7fffda7952ae in 
Okular::DocumentPrivate::sendGeneratorPixmapRequest (this=0x7c7d20)
at okular/core/document.cpp:1248
#15 0x7fffda7a0bc2 in Okular::Document::requestPixmaps (this=0x74ec70, 
requests=..., reqOptions=...)
at okular/core/document.cpp:3325
#16 0x7fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, 
requests=...)
at okular/core/document.cpp:3177
#17 0x7fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps 
(this=0x9d98d0)
at okular/ui/thumbnaillist.cpp:634
#18 0x7fffdac02e40 in ThumbnailList::notifyContentsCleared 
(this=0x96a7a0, changedFlags=1)
at okular/ui/thumbnaillist.cpp:361
#19 0x7fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, 
requests=..., reqOptions=...)
at okular/core/document.cpp:3328
#20 0x7fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, 
requests=...)
at okular/core/document.cpp:3177
#21 0x7fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps 
(this=0x9d98d0)
at okular/ui/thumbnaillist.cpp:634
#22 0x7fffdac02e40 in ThumbnailList::notifyContentsCleared 
(this=0x96a7a0, changedFlags=1)  
at okular/ui/thumbnaillist.cpp:361  
 
#23 0x7fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, 
requests=..., reqOptions=...)
at okular/core/document.cpp:3328
 
#24 0x7fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, 
requests=...)
at okular/core/document.cpp:3177
 
#25 0x7fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps 
(this=0x9d98d0)   
at okular/ui/thumbnaillist.cpp:634  
 
#26 0x7fffdac02e40 in ThumbnailList::notifyContentsCleared 
(this=0x96a7a0, changedFlags=1)  

...

#495 0x7fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, 
requests=..., reqOptions=...)
at 

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a subscriber: rkflx.
aacid added a comment.


  @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

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: ervin, michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a comment.


  Added the qAsConst

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: ervin, michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid updated this revision to Diff 24109.
aacid marked 2 inline comments as done.
aacid added a comment.


  Add qAsConst

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9328?vs=23970=24109

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid, ervin
Cc: ervin, michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  Couple of smaller issues. Otherwise I didn't spot anything which looked very 
wrong to me, admittedly I don't know much about okular though.

INLINE COMMENTS

> document.cpp:3269
> +{
> +for ( PixmapRequest *executingRequest : d->m_executingPixmapRequests 
> )
> +{

Shouldn't that be using qAsConst?

> document.cpp:3327
> +
> +for ( DocumentObserver *o : observersPixmapCleared )
> +o->notifyContentsCleared( Okular::DocumentObserver::Pixmap );

qAsConst missing

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid, ervin
Cc: ervin, michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Michael Weghorn]

michaelweghorn added a comment.


  In https://phabricator.kde.org/D9328#179915, @aacid wrote:
  
  > @michaelweghorn can you give it a try with the new version of the patch? 
This should make it better.
  
  
  It does, thanks! I can no longer reproduce with the new version of the patch.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid
Cc: michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a comment.


  @michaelweghorn can you give it a try with the new version of the patch? This 
should make it better.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid
Cc: michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid updated this revision to Diff 23970.
aacid added a comment.


  Better way to ensure the text generation thread always start after the pixmap 
one

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9328?vs=23951=23970

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid
Cc: michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D9328#179878, @michaelweghorn wrote:
  
  > 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:
  >
  > - open a file that takes long to render
  > - zoom "one step" in and out every few seconds (using the mouse wheel)
  >
  >   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
  
  
  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid
Cc: michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Michael Weghorn]

michaelweghorn added a comment.


  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:
  
  - open a file that takes long to render
  - zoom "one step" in and out every few seconds (using the mouse wheel)
  
  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).

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D9328

To: aacid
Cc: michaelweghorn, ngraham, #okular, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid updated this revision to Diff 23951.
aacid added a comment.


  Made a mistake while cleaning up and the previous code didn't really work ^_^

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9328?vs=23910=23951

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid
Cc: ngraham, #okular, michaelweghorn, gassaf, aacid

D9328: Add support for cancellable image rendering and text extraction

[Albert Astals Cid]

aacid created this revision.
Restricted Application added a subscriber: Okular.
Restricted Application added a project: Okular.

REVISION SUMMARY
  Only supported by the pdf backend if using poppler >= 0.63
  
  Sadly had to change the generator API

TEST PLAN
  Needs https://bugs.freedesktop.org/show_bug.cgi?id=104263

REPOSITORY
  R223 Okular

BRANCH
  cancellable (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D9328

AFFECTED FILES
  core/document.cpp
  core/document_p.h
  core/generator.cpp
  core/generator.h
  core/generator_p.cpp
  core/generator_p.h
  core/page_p.h
  core/textdocumentgenerator.cpp
  core/textdocumentgenerator.h
  generators/chm/generator_chm.cpp
  generators/chm/generator_chm.h
  generators/djvu/generator_djvu.cpp
  generators/djvu/generator_djvu.h
  generators/dvi/generator_dvi.cpp
  generators/dvi/generator_dvi.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp
  generators/poppler/generator_pdf.h
  generators/xps/generator_xps.cpp
  generators/xps/generator_xps.h

To: aacid
Cc: #okular, michaelweghorn, gassaf, ngraham, aacid