Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve the image quality of thumbnails rendered by PDFThumbnailView.draw (issue 8233) #13197

Merged
merged 3 commits into from
Apr 9, 2021
Merged

Improve the image quality of thumbnails rendered by PDFThumbnailView.draw (issue 8233) #13197

merged 3 commits into from
Apr 9, 2021

Commits on Apr 8, 2021

  1. Convert some properties, on PDFThumbnailView-instances, to local va… 

    …riables

These properties are always updated/used together, and there's no other methods which depend on just one of them, hence they're changed into local variables instead.
Looking through the history of this code, it seems they were converted *from* local variables and to properties all the way back in PR 2914; however as far as I can tell from that diff it doesn't seem to have been necessary even back then!?
    @Snuffleupagus
    Snuffleupagus committed Apr 8, 2021
    Configuration menu
    Copy the full SHA
    8ea83f7 View commit details
    Browse the repository at this point in the history

  2. Stop looping over childNodes, in PDFThumbnailView.reset, when rem… 

    …oving the thumbnail

A loop is less efficient than just overwriting the content, which is what we've generally been using (for years) in other parts of the code-base (see e.g. `BaseViewer` and `PDFThumbnailViewer`).
    @Snuffleupagus
    Snuffleupagus committed Apr 8, 2021
    Configuration menu
    Copy the full SHA
    32a00b9 View commit details
    Browse the repository at this point in the history

  3. Improve the image quality of thumbnails rendered by `PDFThumbnailView… 

    ….draw` (issue 8233)

The reason for the fairly large discrepancy, in the thumbnail quality, between the `draw`/`setImage`-methods is that in the former case we *directly* render the thumbnails at the final size that they'll appear at in the sidebar. In the latter case, we instead downsize the (generally) much larger "regular" pages.

To address this, I'm thus proposing that we let `PDFThumbnailView.draw` render thumbnails at *twice* their intended size and then downsize them to the final size.
Obviously this will increase *peak* memory usage during thumbnail rendering in `PDFThumbnailView.draw`, since doubling the width/height of a `canvas` will lead to its pixel-count increasing by a factor of `4`. Furthermore, since you need four components per pixel (given that it's RGBA-data), this will thus lead to the *temporary* thumbnail `canvas`-sizes increasing by a factor of `16` during rendering. Hence why rendering thumbnails at their "original" scale, i.e. using something like `PDFPageProxy.getViewport({ scale: 1 });`, would be an absolutely terrible idea!

To reduce the size and scope of these changes, I've tried to re-factor and re-use as much of the existing downsizing-implementation already present in `PDFThumbnailView` as possible.

While this will generally *not* make thumbnails rendered by `PDFThumbnailView.draw` look *identical* to those based on the rendered pages (via `PDFThumbnailView.setImage`), it's a considerable improvement as far as I'm concerned and enough to call the issue fixed.

*Please note:* This patch will not lead to *any* additional overhead, in either memory usage or parsing, for thumbnails which are based on the rendered pages.
    @Snuffleupagus
    Snuffleupagus committed Apr 8, 2021
    Configuration menu
    Copy the full SHA
    d8e0794 View commit details
    Browse the repository at this point in the history