-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Conversation
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/88217b30ad42bab/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/88217b30ad42bab/output.txt Total script time: 1.01 mins Published |
ec8cc8f
to
d48d0a2
Compare
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 ? |
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. |
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @calixteman received. Current queue size: 1 Live output at: http://54.241.84.105:8877/9d05db1bb764359/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/9d05db1bb764359/output.txt Total script time: 1.01 mins Published |
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. I can see some blurriness on a screen with 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 just realized that I wasn't clear before: The blurriness I'm talking about with this patch is with the default |
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). |
@Snuffleupagus, what's the real zoom level, as a number, you use ? |
125 percent; as defined in Line 22 in 77c7ec6
|
d48d0a2
to
d641dc5
Compare
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/1a16f65dd7ba6b1/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/1a16f65dd7ba6b1/output.txt Total script time: 1.01 mins Published |
Testing with the |
What do you mean exactly by worse ? |
Is this with the test PDF? I'm seeing both as 555x785 🤔 (by using inspect element and changing those |
d641dc5
to
0c5a919
Compare
@Snuffleupagus, it's because Firefox is using float32 numbers when evaluating a |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/ab979362296cb40/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/ab979362296cb40/output.txt Total script time: 1.01 mins Published |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work much better now.
Having a more or less thin white line on some page sides is a longstanding bug... Thanks to mozregression, I managed to get that the regressor is: |
9085d35
to
2d2570f
Compare
I added an integration test to avoid any future regression. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me, with a few more comments and passing tests; thank you!
The canvas must have the same dims as the page in order to avoid to see the page background.
2d2570f
to
68332ec
Compare
FYI, the added integration test is correctly failing on master. |
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/6593269a41bf21a/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/2d76cd3c497b2ab/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/6593269a41bf21a/output.txt Total script time: 30.89 mins
Image differences available at: http://54.241.84.105:8877/6593269a41bf21a/reftest-analyzer.html#web=eq.log |
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). |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/2d76cd3c497b2ab/output.txt Total script time: 46.01 mins
Image differences available at: http://54.193.163.58:8877/2d76cd3c497b2ab/reftest-analyzer.html#web=eq.log |
/botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/5d01090de0eacc4/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/1788070cff34fc7/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/5d01090de0eacc4/output.txt Total script time: 20.28 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/1788070cff34fc7/output.txt Total script time: 26.54 mins
|
No description provided.