D9328: Add support for cancellable image rendering and text extraction

Wed Jan 24 10:59:31 UTC 2018

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20180124/0f87a704/attachment.html>