D8379: PDF: Support the new poppler renderToImage with update callback

Thu Nov 9 07:50:42 UTC 2017

aacid added a comment.

  In https://phabricator.kde.org/D8379#164630, @rkflx wrote:

  > In https://phabricator.kde.org/D8379#157777, @ngraham wrote:
  >
  > > Can we add "BUG: 344081" to the Summary?
  >
  >
  > It's still not showing up for me in the summary on Phab. Note you'll have to use something like `arc diff --edit --verbatim` if you are not using Phab's web interface (with `arc amend` being the counterpart for bringing changes from Phab's summary to the local repo). Also, it would be great to mention the "incremental rendering" in the commit message, so we have a chance to git log grep it without knowing `renderToImage` and its meaning.

  It's on my local commit so it should be fine when i push it.

  > 
  > 
  > In https://phabricator.kde.org/D8379#162464, @aacid wrote:
  > 
  >> > - With Fit Page, first the main view renders and after finishing the thumbnail should be the only thing left to render. However, the thumbnail takes a much longer time to show an initial frame with the CPU being busy throughout doing something else (after this, the incremental rendering seems to need about the same time as the main view). It is likely this has been there all the time and the patch just makes this phenomenon more visible, nevertheless may be a candidate for huge CPU time savings in a later patch (freeing resources for subsequent pages).
  >>
  >> That's probably the text page being generated (also takes a long time), don't understand the mention to Fit Page, does it only happen in that case for you?
  > 
  > 
  > Are you saying text page generation is fast for the main view and slow for the thumbnail? Does not sound plausible to me. I gave Fit Page as a method to reproduce, because this way both main view and thumbnail view would show the complete page, but it also happens with other zoom levels, e.g. 100%.

  There's no text generation for main view and one for thumbnail, there is no reason to generate text twice

  > I expect that in both cases incremental rendering kicks in at the same 500ms after Okular starts the respective rendering job (I'm not talking about both jobs starting in parallel…), but currently the thumbnail lags badly. Have a look:
  > 
  > F5477430: okular-thumbnail-delay.webm <https://phabricator.kde.org/F5477430>
  > 
  >>> - When zooming, the initial timeout to start showing the incremental rendering seems much longer than 500ms and also much longer compared to when reloading the document at the same zoom level afterwards. Okular seems busy doing something else. Maybe the rendering of intermediate zoom levels is not cancelled correctly? This is also seen when closing Okular with rendering still in progress, where the shell prompt returns only much later while the window closes immediately.
  >> 
  >> Again take into account the text page generation, do you have https://phabricator.kde.org/D8378? Otherwise what you think is "initial timeout" is just the text page being created.
  > 
  > I did test with https://phabricator.kde.org/D8378. But as you said, cancelling is not supported currently. Do you prefer a bugzilla issue or a task on Okular's workboard to track this?

  I don't really mind :)

  >>> - In some situations, incremental rendering happens twice in a row, i.e. the first pass is thrown away when finished and a second pass starts. (Occurs now and then, unfortunately I cannot give you instructions on how to reproduce yet. Try moving around with the scroll wheel when zoomed in.)
  >> 
  >> We do not support canceling of rendering once rendering has started, so that is easily reproducible by zooming in/out at the right time if you are already in the tiled mode rendering. There's nothing we can really do about it other than implementing rendering cancellation in the future.
  > 
  > Your comment fits more to the previous issue, but not this one? I can now reproduce, no zooming involved:
  > 
  > F5477455: okular-double-rendering.webm <https://phabricator.kde.org/F5477455>
  > 
  > This double rendering (which I see no reason for why this would be useful) could also explain the first issue, although there the second pass (if indeed the same) does not cause a redraw.

  Right, i can reproduce now, this is an existing problem with the code, since if i add debug to PDFGenerator::image i will get

  PDFGenerator::image PixmapRequest(#94085861557552, async, 13307x11330, page 0, prio 1)
  PDFGenerator::image PixmapRequest(#94085861557552, async, 13307x11330, page 0, prio 1)
  PDFGenerator::image PixmapRequest(#94085860859632, async, 249x212, page 0, prio 2)

  So there's an existing problem with the tile manager somewhere that doesn't detect that the thing has already been rendered and asks for it to be rendered again. i will try to have a look at it but since it is an existing bug i'd prefer if we didn't block on this.

  >>> - Extensive flickering of some screen areas (switching between white and content rapidly for several seconds) when redrawing after panning a rotated page in some cases, even a segfault occasionally.
  >> 
  >> I've got a crash, will investigate, next time please post the backtraces of those segfaults somewhere. Otherwise i'll never know if what i fix is the same you had or not.
  > 
  > In general I try to, but note here "in some cases" in combination with "occasionally" meant "too seldom to reproduce again with gdb" in the time budget I had available for the review. Now I played around some more and the crashing seems to be gone, but this was not crashing very predictably in the first place.
  > 
  > I did notice some cases of bad flickering, though. The flickering is very camera shy, so the recording is not the best. The movement you see is just an initial flick (several ticks) of the scroll wheel:
  > 
  > F5477670: okular-weird-flickering.webm <https://phabricator.kde.org/F5477670>
  > 
  > Note that sometimes the viewport would change to white completely as soon as Okular lost focus.

  Weird, i can't really reproduce this :/
  Maybe it has to do with the previous issue about things being asked to be rendered twice?

  >>> I tried to get a feeling for the difference between updating immediately (which I presume would be more pleasant) and updating after a delay of much more than the typical 30ms human perception normally does not notice (which you claim to be preferable). However, there are so many timeouts/delays and intermediate images like the scaled up thumbnails happening, that to even compare both approaches a substantial code rework would be needed. Let's just keep it as proposed, then.
  >> 
  >> I don't understand this sentence :(
  > 
  > When I set the timeout to 0, (incremental) rendering still would not start immediately. That's probably because Okular first tries to show a scaled version of tiles from the same region if those have been rendered before (e.g. for the thumbnails or for a different zoom level), or there are other (intentional?) delays.

  Yes, we still favour showing the scaled version if it is there, which i think it makes sense, because even if it's of a bit worse quality it actually represents the full content.

  > Nothing for this patch, but it would be worth reworking this (low priority, though) to show as much content as early as possible. Maybe show the scaled version in the background, do the incremental rendering on top of that instead of on top of a white background, and finally fade out the scaled version?

  That looks hard to do, but sure would be nice to have if possible.

  > 
  > 
  >  ---
  > 
  > There is another problem I just found: With `QT_SCALE_FACTOR=1.1`, pressing [PgUp] and [PgDn] repeatedly does not cache the tiles but re-renders them everytime. I believe this regression was introduced in https://phabricator.kde.org/R223:ecc1141e0293e1a30e0f8787d86dcc6309ffb0c0, but without the incremental rendering this only results in wasted CPU time (compared to e.g. 17.08), while with incremental rendering it is practically unusable:
  > 
  > F5477598: okular-hidpi-cache-fail.webm <https://phabricator.kde.org/F5477598>

  Yes, this is a problem that already exists and not something this patch introduces, again i would prefer if we didn't block on this

  > 
  > 
  >  ---
  > 
  > To summarize my behavioural testing, all crashers I found are gone and incremental rendering happens everywhere I expect it to. Thanks for fixing all those problems.
  > 
  > In general the patch is now in a state we could ship it, the only thing I do not feel comfortable about is the HiDPI regression (but this one should be fixed separately anyway). I think the rest concerns mostly minor issues also affecting current master, where the render timing suggests the same is happening and we just don't see that explicitly in the UI.
  > 
  > Trusting Milian the code is fine, I'm accepting the Diff for now and leave it to your judgment whether the HiDPI regression is fixable before Poppler 0.62 actually ships.
  > 
  > @mlaurent As you "requested changes", are those solved now?

  Unless there's a disagreement i'll commit this later today since the KDE Applications 17.12 dependency freeze is tonight and i'd like to have this in for that release

REPOSITORY
  R223 Okular

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

To: aacid, #okular, mlaurent, mwolff, rkflx
Cc: mwolff, rkflx, ngraham, michaelweghorn, mlaurent, #okular, aacid
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20171109/5d9848ba/attachment-0001.html>