Add tiling support to the QtPDF backend#55
Closed
bmjcode wants to merge 7 commits into
Closed
Conversation
Previously the background was painted or erased several times in separate parts of the code when rendering a PDF page. This was partly my own fault, as I did not fully understand the rendering logic when I first wrote the QtPDF backend (the previous Poppler backend I used as a reference also overrode significant parts of it). This new code only paints the background once, and only when necessary to do so. For example, it's not necessary or desirable to paint the background when printing. Another consequence of this fix is that it's possible now to render an image with a transparent background, for example in Frescobaldi's copy-to-image dialog. I believe this was how it worked in older versions of qpageview, and was also the intended behavior here all along.
This is an optimization that eliminates the need to fill the background separately before calling drawImage(), which can cause flicker. We do this in job() rather than in render() so that it only affects the cached images used to paint the widget, not images rendered for other purposes such as printing or Frescobaldi's copy-to-image feature.
This greatly improves performance at the cost of momentarily higher memory usage when rendering the page. Because of QtPDF limitations, this is implemented by rendering the entire page at once, then splitting it into tiles that are cached and displayed individually. This is less efficient than rendering tile-by-tile, as qpageview was intended to do (but which QtPDF does not currently support). However, it still provides important performance advantages of using tiles like reduced flicker when repainting. In practice, this will have the most noticeable impact at higher zoom levels, as lower zoom levels render the full page at once anyway.
Member
Author
|
Note this builds on #54, but I'm intentionally keeping them separate so potential issues with this PR don't hold up that one. |
This makes it available to other backends if needed, and simplifies the code considerably besides.
This prevents flicker when scrolling over a page break at higher zoom levels. If two pages are displayed at once, but the cache only holds one at a time, then rendering each page purges the other's cached tiles, forcing both pages to be re-rendered each time the widget is repainted.
When two pages are visible, we don't necessarily need all the tiles from both pages -- just the bottom tiles of the first and the top tiles of the second. We can discard the others if they don't fit in memory. This is, in fact, the entire point of tile-based rendering.
This prevents flicker when both page and tile breaks are visible. Even with the limitations of full-page rendering, there are probably ways to get the performance we want with a smaller cache. For example, our algorithm might take into account which tiles are currently visible when deciding what to keep in memory. But are the potential savings really worth the effort? Frescobaldi doesn't use that much memory by current standards, even under extreme conditions. For example, 800% zoom on a high-DPI screen uses about 2GB on my system, which is comparable to a couple tabs in a modern browser. I probably will find an excuse to come back and fuss with this again anyway, but for now I think the improved performance with a larger cache outweighs any potential concerns about memory usage.
Member
Author
|
This is a silly PR. Testing on my old Core 2 Duo confirms smaller tiles provide no performance benefit when repainting. The only benefits are faster rendering and lower memory usage when the backend can avoid rendering the entire page at once -- both of which this PR utterly negates. That said, this did help me discover how to optimize the cache size to reduce flickering, which is better implemented in #56. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This greatly improves performance at the cost of momentarily higher memory usage when rendering the page.
Because of QtPDF limitations, this is implemented by rendering the entire page at once, then splitting it into tiles that are cached and displayed individually. This is less efficient than rendering tile-by-tile, as qpageview was intended to do (but which QtPDF does not currently support). However, it still provides important performance advantages of using tiles like reduced flicker when repainting.
In practice, this will have the most noticeable impact at higher zoom levels, as lower zoom levels render the full page at once anyway.