-
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
Enable/disable image smoothing based on image interpolate value. (bug 1722191) #13991
Conversation
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.241.84.105:8877/dcb67c58bc85309/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.193.163.58:8877/c842147ac86d5d6/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/dcb67c58bc85309/output.txt Total script time: 21.93 mins
Image differences available at: http://54.241.84.105:8877/dcb67c58bc85309/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/c842147ac86d5d6/output.txt Total script time: 39.97 mins
Image differences available at: http://54.193.163.58:8877/c842147ac86d5d6/reftest-analyzer.html#web=eq.log |
A lot more failures on the bots than locally. |
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.241.84.105:8877/4c17cc59c1ded86/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.193.163.58:8877/80201d8ee89a39a/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/4c17cc59c1ded86/output.txt Total script time: 22.06 mins
Image differences available at: http://54.241.84.105:8877/4c17cc59c1ded86/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/80201d8ee89a39a/output.txt Total script time: 40.16 mins
Image differences available at: http://54.193.163.58:8877/80201d8ee89a39a/reftest-analyzer.html#web=eq.log |
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.
While I completely agree that this looks good (or better) for most cases, even with the changes in the second commit there appear to be some regressions. (Also, please squash the commits before landing this.)
Looking through the test results, and focusing (only) on Firefox, there's a few cases that I do think look worse now when comparing against Adobe Reader:
- issue3903, page1
- freeculture, page2
- wdsg_fitc, page20
- cmykjpeg, page1
- issue5644, page1
- issue7200, page1; the "text" is a lot less readable
- issue5549, page1
- issue5475, page1
- issue5481, all pages
Is there anything that could possibly be done about some of these cases, or are we going to have to accept those changes?
I'm looking into some heuristics. I was able to fix a few by following this comment, but there are still a few where acrobat seems to be doing some image smoothing in cases I wouldn't expect. |
653fab7
to
1ebb595
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.241.84.105:8877/3d3dc844caf864e/output.txt |
This will still need some tweaks. On a high dpi monitor the output now looks very close to acrobat, but on a monitor with a device pixel ratio of 1, pdf.js looks much worse than acrobat. I thought it was maybe do to pdf.js trying to draw images at fractional pixels, but even when I align the image to whole pixel values it still looks worse. I wonder if they're using a different scaling algorithm. |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/3d3dc844caf864e/output.txt Total script time: 21.51 mins
Image differences available at: http://54.241.84.105:8877/3d3dc844caf864e/reftest-analyzer.html#web=eq.log |
1ebb595
to
017ab04
Compare
I think I may have figured it out, instead of comparing the scale to 1 we have to take into account that css pixels per inch are different than pdf pixels per inch. Using that value for deciding when to smooth seems to match acrobats behavior. |
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.241.84.105:8877/b3405493217d82f/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 1 Live output at: http://54.193.163.58:8877/7618d3c62bba454/output.txt |
src/display/canvas.js
Outdated
const CSS_PIXELS_PER_INCH = 96.0; | ||
const PDF_PIXELS_PER_INCH = 72.0; |
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.
Given how these values are being used, it appears (as far as I can tell) to be basically the following viewer constant:
Line 16 in 5678c75
const CSS_UNITS = 96.0 / 72.0; |
Can we perhaps move the existing CSS_UNITS
constant into display_utils.js
(and export it via pdf.js
) and then re-use here too, instead of essentially duplicating this?
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.
Looking at the final patch, I've got one small suggestion for simplifying the new exports a little bit.
However, in the interest of getting this landed sooner rather than later, I'll merge it as-is and file a follow-up PR instead.
src/display/canvas.js
Outdated
function getImageSmoothingEnabled(transform, interpolate) { | ||
const scale = Util.singularValueDecompose2dScale(transform); | ||
const actualScale = | ||
(window.devicePixelRatio * CSS_PIXELS_PER_INCH) / PDF_PIXELS_PER_INCH; |
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 is causing the unit-tests to fail, when run in Node.js, since we cannot assume that window
is defined.
How about simply doing (globalThis.devicePixelRatio || 1) * ...
here, since that ought to prevent any issues?
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/b3405493217d82f/output.txt Total script time: 25.15 mins
Image differences available at: http://54.241.84.105:8877/b3405493217d82f/reftest-analyzer.html#web=eq.log |
I need to look into why firefox-issue5549.pdf-page1 is blurry, locally (with lodpi and highdpi) it's getting smoothed in the viewer at 100%. I'm guessing something is different in test harness vs viewer. |
… 1722191) While some of the output looks worse to my eye, this behavior more closely matches what I see when I open the PDFs in Adobe acrobat. Fixes: mozilla#4706, mozilla#9713, mozilla#8245, mozilla#1344
017ab04
to
f38fb42
Compare
The issue with firefox-issue5549.pdf-page1 was a float rounding error, the scale's were both 1.333... but slightly different. |
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.193.163.58:8877/64e5c0427fdd0e9/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/64e5c0427fdd0e9/output.txt Total script time: 32.90 mins
Image differences available at: http://54.193.163.58:8877/64e5c0427fdd0e9/reftest-analyzer.html#web=eq.log |
/botio-linux test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.241.84.105:8877/04d1adaeccbeb8b/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/04d1adaeccbeb8b/output.txt Total script time: 22.22 mins
Image differences available at: http://54.241.84.105:8877/04d1adaeccbeb8b/reftest-analyzer.html#web=eq.log |
/botio-windows browsertest |
From: Bot.io (Windows)ReceivedCommand cmd_browsertest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/bd07d8d265ffcb5/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/bd07d8d265ffcb5/output.txt Total script time: 35.58 mins
Image differences available at: http://54.193.163.58:8877/bd07d8d265ffcb5/reftest-analyzer.html#web=eq.log |
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/1102bdb64794c67/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/1102bdb64794c67/output.txt Total script time: 4.35 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 looks much better, and fixes a bunch of old issues which is great; thank you!
/botio makeref |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 1 Live output at: http://54.193.163.58:8877/d9c648a3d312a27/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/ebd544998cbce6b/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/ebd544998cbce6b/output.txt Total script time: 18.35 mins
|
/botio-linux makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/6a4973df1f1f0bd/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/6a4973df1f1f0bd/output.txt Total script time: 18.47 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/d9c648a3d312a27/output.txt Total script time: 36.28 mins
|
/botio-linux makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/fd69c212627b74d/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/fd69c212627b74d/output.txt Total script time: 20.07 mins
|
While some of the output looks worse to my eye, this behavior more
closely matches what I see when I open the PDFs in Adobe acrobat.
Fixes #4706
Fixes #9713
Fixes #8245
Fixes #1344
Fixes #5757