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.

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

Avoid to have a white line around the canvas #18698

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

calixteman
Copy link
Contributor

No description provided.

@calixteman
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/88217b30ad42bab/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/88217b30ad42bab/output.txt

Total script time: 1.01 mins

Published

web/pdf_page_view.js Outdated Show resolved Hide resolved
web/pdf_page_view.js Outdated Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Contributor

This looks good to me, does it also still fix the "small movement when the canvas is redrawing" problem you mentioned?

@calixteman
Copy link
Contributor Author

This looks good to me, does it also still fix the "small movement when the canvas is redrawing" problem you mentioned?

Yes it does for me, doesn't it for you ?

@Snuffleupagus
Copy link
Collaborator

Comparing https://mozilla.github.io/pdf.js/web/viewer.html and #18698 (comment) side-by-side, it does appear that this patch makes the canvas slightly blurry at some zoom levels (e.g. auto).

@calixteman
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @calixteman received. Current queue size: 1

Live output at: http://54.241.84.105:8877/9d05db1bb764359/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/9d05db1bb764359/output.txt

Total script time: 1.01 mins

Published

@Snuffleupagus
Copy link
Collaborator

The new preview still suffers from blurriness issues unfortunately; there doesn't seem to be any improvement there.

Are the src/display/display_utils.js changes perhaps enough to fix the regression as filed, because if so I believe that we should do that first and worry about the other parts of this PR later!?

@calixteman
Copy link
Contributor Author

The new preview still suffers from blurriness issues unfortunately; there doesn't seem to be any improvement there.

Are the src/display/display_utils.js changes perhaps enough to fix the regression as filed, because if so I believe that we should do that first and worry about the other parts of this PR later!?

Unfortunately it isn't enough.
At 70%, the page dims are 555x785 when the canvas css dims are 554x784.

I can see some blurriness on a screen with window.devicePixelRatio === 1.5 too but only around a zoom level equals to 60-70% when the text becomes barely readable (at least for me). In changing in the css width & height to calc(100% - 1px), I can see an improvement but then we see the page background.
I tried the same with a screen where the ratio is 1 and there aren't that much differences.
I've the impress that the problem mainly something about font hinting and anti-aliasing.
Out of curiosity, I added image-rendering: pixelated; to the canvas style and it fixes the blurriness for me.

That said my feeling is that the patch by itself isn't really the problem but there's maybe something else on the gfx side.
I'll check how it behaves in Chrome later.

@Snuffleupagus
Copy link
Collaborator

I just realized that I wasn't clear before: The blurriness I'm talking about with this patch is with the default tracemonkey.pdf file and window.devicePixelRatio === 2, at e.g. the "auto" zoom level.
Hence this patch really doesn't seem acceptable as-is, most unfortunately.

@calixteman
Copy link
Contributor Author

I think it's because the ratio between the canvas width/height and the canvas css width/height is no more exactly equal to 1.5 (my devicePixelRatio).
So I think, we should change the 3rd parameter of the round function in the css to have something which uses the denominator found by Farey approximation.
That said maybe having the right ratio helped to just use the right algorithm to downscale the canvas, so maybe (too much maybe in this sentence) using pixelated could be the right way to do it.

@calixteman
Copy link
Contributor Author

@Snuffleupagus, what's the real zoom level, as a number, you use ?

@Snuffleupagus
Copy link
Collaborator

what's the real zoom level, as a number, you use ?

125 percent; as defined in

const MAX_AUTO_SCALE = 1.25;

@calixteman calixteman marked this pull request as ready for review September 6, 2024 13:56
@calixteman
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/1a16f65dd7ba6b1/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/1a16f65dd7ba6b1/output.txt

Total script time: 1.01 mins

Published

@Snuffleupagus
Copy link
Collaborator

Testing with the tracemonkey.pdf file, this patch unfortunately still looks worse (for me) compared to master.

@calixteman
Copy link
Contributor Author

What do you mean exactly by worse ?
Could you share a screenshot showing both rendering ?

@Snuffleupagus
Copy link
Collaborator

Here's two screen-shots, from master respectively the patch, where the letters are noticeably more blurry with this patch. (Comparing the images at high magnification might be helpful to see the differences more clearly.)

master

patch

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Sep 6, 2024

The new preview still suffers from blurriness issues unfortunately; there doesn't seem to be any improvement there.
Are the src/display/display_utils.js changes perhaps enough to fix the regression as filed, because if so I believe that we should do that first and worry about the other parts of this PR later!?

Unfortunately it isn't enough. At 70%, the page dims are 555x785 when the canvas css dims are 554x784.

Is this with the test PDF? I'm seeing both as 555x785 🤔 (by using inspect element and changing those round() CSS functions at https://mozilla.github.io/pdf.js/web/viewer.html, with Firefox 130.0)

@calixteman
Copy link
Contributor Author

FYI, the added integration test is correctly failing on master.

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/6593269a41bf21a/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/2d76cd3c497b2ab/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/6593269a41bf21a/output.txt

Total script time: 30.89 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 163

Image differences available at: http://54.241.84.105:8877/6593269a41bf21a/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

The small movements we've are very likely due to the change in the text (or annotation) layer dimensions where we now round differently (in passing --scale-round-x/y as third parameter).

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/2d76cd3c497b2ab/output.txt

Total script time: 46.01 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 77

Image differences available at: http://54.193.163.58:8877/2d76cd3c497b2ab/reftest-analyzer.html#web=eq.log

@calixteman calixteman merged commit 5369a24 into mozilla:master Sep 7, 2024
8 checks passed
@calixteman
Copy link
Contributor Author

/botio makeref

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/5d01090de0eacc4/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/1788070cff34fc7/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/5d01090de0eacc4/output.txt

Total script time: 20.28 mins

  • Make references: Passed
  • Check references: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/1788070cff34fc7/output.txt

Total script time: 26.54 mins

  • Make references: Passed
  • Check references: Passed

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 25, 2024
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 25, 2024
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 26, 2024
Required after mozilla/pdf.js#18698

Differential Revision: https://phabricator.services.mozilla.com/D223272

UltraBlame original commit: b32cd1d903a953ee18f2c81bf07fbe0a4b272a2a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 26, 2024
Required after mozilla/pdf.js#18698

Differential Revision: https://phabricator.services.mozilla.com/D223272

UltraBlame original commit: 4afa8cfd1623b9bb830e25ad9a2db3e301f5ee9c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 26, 2024
Required after mozilla/pdf.js#18698

Differential Revision: https://phabricator.services.mozilla.com/D223272

UltraBlame original commit: b32cd1d903a953ee18f2c81bf07fbe0a4b272a2a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 26, 2024
Required after mozilla/pdf.js#18698

Differential Revision: https://phabricator.services.mozilla.com/D223272

UltraBlame original commit: 4afa8cfd1623b9bb830e25ad9a2db3e301f5ee9c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 26, 2024
Required after mozilla/pdf.js#18698

Differential Revision: https://phabricator.services.mozilla.com/D223272

UltraBlame original commit: b32cd1d903a953ee18f2c81bf07fbe0a4b272a2a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 26, 2024
Required after mozilla/pdf.js#18698

Differential Revision: https://phabricator.services.mozilla.com/D223272

UltraBlame original commit: 4afa8cfd1623b9bb830e25ad9a2db3e301f5ee9c
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Sep 26, 2024
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Sep 26, 2024
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Sep 27, 2024
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: White border in viewer for PDFs with background color
5 participants