Upstream poppler patch needed: https://bugs.freedesktop.org/show_bug.cgi?id=103372

It's not mandatory but used with https://phabricator.kde.org/D8378 gives better results since that way we ensure that the pixmap generation takes place before the text generation.

I'll find and post here (or in the poppler bug) a slow PDF that demonstrates the usefulness of this approach (Adobe Reader does the same) since the one i've been using is private.

One PDF that benefits from this is https://scalablemaps.com/download-request/dublin-center-street/pdf (even though this is not very slow it shows the difference)

You can see a video of the difference here https://www.youtube.com/watch?v=NaUbWL6800Y

That's a really great feature Okular's user will surely love. Does this solve https://bugs.kde.org/show_bug.cgi?id=344081?

When testing (with D8378 and D8379 both applied at the same time – sorry for that –, as well as the Poppler patch), there were still some issues for me:

• After zooming in and rendering finished (i.e. CPU usage went back to zero), the display would not update. Only after panning enough so a request for a new tile triggered rendering again the display would also update. (Note: I tested whether this is a regression with the HiDPI commit, but it is not.)
• Same thing for zooming out.
• The progressive rendering seems to only work with Fit Page on startup, but not with 100% Zoom on startup or when zooming in later. (My first thought was that there might be some connection to the scaled raster images shown as a temporary preview, but apparently that is not always the case.)

Also I am wondering if the partial updates might slow down the total rendering time, i.e. whether there should be some rate limiting to the partial updates (if there isn't already)?

This definitely looks like it implements that feature request. Can we add "BUG: 344081" to the Summary?

In D8379#157771, @rkflx wrote:

That's a really great feature Okular's user will surely love. Does this solve https://bugs.kde.org/show_bug.cgi?id=344081?

Yes

When testing (with D8378 and D8379 both applied at the same time – sorry for that –, as well as the Poppler patch), there were still some issues for me:

• After zooming in and rendering finished (i.e. CPU usage went back to zero), the display would not update. Only after panning enough so a request for a new tile triggered rendering again the display would also update. (Note: I tested whether this is a regression with the HiDPI commit, but it is not.)
• Same thing for zooming out.

Right, thanks for finding this, that happens because Page::hasPixmap is not really const when the page has a tile manager, i've added a workaround and FIXME and i'll see if i can find some time to fix that properly, but i don't think we should block this feature of landing because of old code being broken.

• The progressive rendering seems to only work with Fit Page on startup, but not with 100% Zoom on startup or when zooming in later. (My first thought was that there might be some connection to the scaled raster images shown as a temporary preview, but apparently that is not always the case.)

Nothing to do with Fit Page, basically if you got into tiled rendering the progressive rendering didn't happen, i've also fixed that now.

Also I am wondering if the partial updates might slow down the total rendering time, i.e. whether there should be some rate limiting to the partial updates (if there isn't already)?

It totally does since there's more "here take this pixmap and put it into screen", but since the user seems things earlier the end result for his mind is "things are faster". I don't see a need to limit it really (other than the initial 500ms barrier)

In D8379#158669, @aacid wrote:

Fix pixmaps not getting updated when the tile manager kicks in

Also make the tile request be partially updated if that's what the request wants

Thanks for updating, I'll try to re-test tomorrow or the day after.

The latest version is much better and I could not reproduce my issues from above anymore. Still some oddities (tested using dublin-center-street.pdf):

• 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).
• 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.
• 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.)
• Segfaults on entering Presentation mode (works fine without the patch to Okular but still with the newer Poppler).
• 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.

Not sure whether all of this should even be fixed in this patch, but I would appreciate a short comment from you regarding those points.

I don't see a need to limit it really (other than the initial 500ms barrier)

Just to put some numbers behind this: A quick measurement with a stopwatch and an artificially slowed down CPU consistently gives me total rendering times of ~7.5s vs. ~9.0s, i.e. the patch is ~15% slower.

lots of pages take more than 30ms and you don't really want to see them rendering just 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.

In D8379#160970, @rkflx wrote:

The latest version is much better and I could not reproduce my issues from above anymore. Still some oddities (tested using dublin-center-street.pdf):

• 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?

• 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 D8378? Otherwise what you think is "initial timeout" is just the text page being created.

• 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.

• Segfaults on entering Presentation mode (works fine without the patch to Okular but still with the newer Poppler).

Fixed

• 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.

Not sure whether all of this should even be fixed in this patch, but I would appreciate a short comment from you regarding those points.

I don't see a need to limit it really (other than the initial 500ms barrier)

Just to put some numbers behind this: A quick measurement with a stopwatch and an artificially slowed down CPU consistently gives me total rendering times of ~7.5s vs. ~9.0s, i.e. the patch is ~15% slower.

lots of pages take more than 30ms and you don't really want to see them rendering just 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 :(

@rkflx i rebased this patch on top of master that includes https://cgit.kde.org/okular.git/commit/?id=662fa69a2dcc0c1403b1773262368943d5cacd52

This fixes for me the crashes on rotation jobs, the bug wasn't on this branch, it's just that now that codepath was exercised more often and thus the crash happened more often too.

Could you try to see if the crashes are gone for you too?

@aacid In general I notice the changes you are making, sorry I'm not always able test immediately. I'll try to have another look in the next couple of days.

(That said, more people testing by actually running the code with the newest Poppler would be great.)

In D8379#163631, @rkflx wrote:

@aacid In general I notice the changes you are making, sorry I'm not always able test immediately. I'll try to have another look in the next couple of days.

(That said, more people testing by actually running the code with the newest Poppler would be great.)

FWIW: I ran it here, and it worked fine after the crash fix. Only tested it on the dublin map though.

In D8379#163666, @mwolff wrote:

FWIW: I ran it here, and it worked fine after the crash fix. Only tested it on the dublin map though.

Thanks, good to know :)

@rkflx void signalPartialPixmapRequest( Okular::PixmapRequest * request, const QImage &image ); it seems that it's not fixed no ? still space no ?
But after fixing it it's ok for me

In D8379#164630, @rkflx wrote:

In 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 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:

• 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 D8378? Otherwise what you think is "initial timeout" is just the text page being created.

I did test with 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:

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:

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 ecc1141e0293, 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:

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