Don't request pixmaps twice when opening okular
AbandonedPublic

Authored by aacid on Nov 16 2017, 10:54 AM.

Details

Reviewers
None
Summary

This is very visible if you open a file that will have partial updates
and you open it quite zoomed in (so that tiled rendering is in place).

There were two problems:

  • If we open the file from the command line, we get a few resize events,

in my case around four, so we where requesting different tiles for each of the
four resizes, which resulted in multiple re-renders, which with partial
updates looks bad. We fix that by also starting the delayResizeEventTimer
on resize events even if there's no items on the view yet, this has a
slight delay in rendering but i can't really see it

  • When doing tiled rendering we request a bit more than the immediate visible rect,

this is the expandedVisibleRect variable, but the problem is that if this is the
first request that will create the tiles, hasTileManager still returned false, so
we would request that tile, then go back and since we would have tiles, request
a somewhat bigger tile and that would also end up in a repaint. We fix that by
introducing the requestWillCreateTileManager function that lets the
pageview know that even if we have no tile manager yet this particular request
will create one and thus we should also expand the requested tile a bit to not
end up in the previous situation where we requested a similar area twice

Diff Detail

Repository
R223 Okular
Branch
do_not_request_twice
Lint
No Linters Available
Unit
No Unit Test Coverage
aacid created this revision.Nov 16 2017, 10:54 AM
Restricted Application added a project: Okular. · View Herald TranscriptNov 16 2017, 10:54 AM
Restricted Application added a subscriber: Okular. · View Herald Transcript
aacid added a subscriber: rkflx.Nov 16 2017, 10:56 AM

Hmmm, this has the side effect that now sometimes the thumbnailbar rendering starts first, need to investigate

aacid added a comment.Nov 16 2017, 1:38 PM

After having a second look it does indeed have a noticeable delay for pdf that render *very* fast, with this patch you can see the empty page while without it you always see the rendered page (it also renders twice but since it's so fast you don't really see it).

Ideally here the solution is page rendering cancellation when you see that the viewport area has changed, but that's not easily acomplishable either.

Or knowing when the "window is opening/maximizing" resizes have ended, but as far as i know that's not possible.

So as far as i see it, we have to decide whether we want to live with double repaints on opening a file at the same time than opening okular (files after okular is open don't suffer this) or with adding extra delay for all files. Double repaint only really looks bad with tiled rendering, otherwise it's "acceptable" (even if a waste or resources)

aacid added a comment.Nov 16 2017, 1:57 PM

Another option is just pushing the fix for the second problem and not for the first one (that introduces the delay) this makes it work "more often" since the delay is a race between how fast your window manager resizes the window and how fast okular opens the file

Sorry for the delay, finally could do some testing. First I was quite enthusiastic about what I saw, but then some oddities came up so I cannot draw a conclusion on what I would recommend to do here yet.

Any final patch (i.e. not fulfilled by current patch version) should ensure IMO:

  • Thumbnails are less important than the main view and therefore should not delay its rendering.
  • If we can currently render a fast document without any delay, we should not regress on that.

Here is what seems odd:

files after okular is open don't suffer this

IIUC, your explanation is about resize events when the window is opening. However, adding debug output to resizeEvent (for isEmpty() and for !isEmpty()) as well as slotRequestVisiblePixmaps shows that this is also happening for a simple Reload. This suggests those events are entirely controlled by Okular code and may be fixable there?

Next, I reverted both delayResizeEventTimer related changes to evaluate your fix to the second problem (tested with dublin-center-street.pdf @ 200% zoom, with T: thumbnails render, M: main view renders, D: weird delay of several seconds):

masterpatchpatch w/o timer
start normalM-D-M-T-MT-D-MM-D-T-M
reload normalM-D-M-T-MT-D-MM-D-T-M
start rotatedM-D-M-T-MT-D-MM-D-T-M
reload rotatedM-D-M-T-MT-D-MM-D-T-M

Note:

  • For normal documents the last M in -M-T-M would sometimes be omitted when choosing another initial window size.
  • For rotated documents the last M would sometimes affect only parts of the window.
  • T-D-M would sometimes turn into D-T-M when repeating the same test.
  • I still do not understand what D is about (in the other Diff you said "There's no text generation for main view and one for thumbnail, there is no reason to generate text twice", but this does not really answer the question I asked?).
  • Ideal log: M-T (or rather in parallel ;)

In conclusion, I'm not convinced that the standalone fix for the second problem is completely working either. This is all very strange, maybe you could have another look?

I suspect there is still something else going on, so perhaps we are not yet seeing the root cause? Especially for rotated pages in some cases reloading results in more redraws compared to normal pages and removing window focus can provoke blank pages.

Ideally here the solution is page rendering cancellation

Yeah, that would be great. Given the other solutions also have problems, this could be worth thinking about?


Refs: F5477455, D8379#164630

rkflx added a comment.Nov 27 2017, 5:07 PM

Another option is just pushing the fix for the second problem and not for the first one

After testing again, "patch w/o timer" now gives "M-D-T", i.e. the last M is not there anymore. I suspected I could find a window size or map area to break it again (as noted before), and indeed, I then got M-D-M-T, which is different to the one from above again…

For now I'd say your suggestion to only fix the second problem is probably the best we can do for the moment, it improves the situation in some cases at least.

The cancelling, resize events uncertainty, blank rotated pages and the seldom extra "M"s are most likely best handled in other patches.

core/generator.cpp
718

Interesting, how would I get this debug output? (modifying Okular's entries in kdebugsettings did not print this line).

Still, this looks a bit like stray debug output, or should this get its own .arg explanation?

aacid added a comment.Dec 14 2017, 2:13 PM

With https://phabricator.kde.org/D9328 we can probably just discard this one

rkflx added a comment.Dec 14 2017, 5:16 PM
In D8838#179480, @aacid wrote:

With https://phabricator.kde.org/D9328 we can probably just discard this one

Sounds lovely, cannot wait to test. I'm currently backlogged a bit on Okular related things, so it may take a while to get to this though (sorry).

aacid abandoned this revision.Jan 15 2018, 9:07 AM

Abandoning in favor of D9328