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

Always enable smoothing when rendering downscaled image #17868

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented Apr 2, 2024

and rely on the Interpolate flag only when the image is upscaled.
Fixes #16273.

@Aditi-1400
Copy link
Contributor

Thanks for this, looks like it improves the PDF quality in Firefox as well :)
Before:
image

After:
image

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Is there perhaps a reference to an issue (either on GitHub or Bugzilla), or does it not exist (yet)? If this improves a particular PDF file, would it be possible to add a reference test for this improvement so we avoid regressing it in the future or is it already covered by the existing reference tests?

src/display/canvas.js Outdated Show resolved Hide resolved
@timvandermeij
Copy link
Contributor

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/5f73ea89661b0f7/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/7f9e2fca1a5f7af/output.txt

@Aditi-1400
Copy link
Contributor

Looks good! Is there perhaps a reference to an issue (either on GitHub or Bugzilla), or does it not exist (yet)? If this improves a particular PDF file, would it be possible to add a reference test for this improvement so we avoid regressing it in the future or is it already covered by the existing reference tests?

There's an issue here #16273. Also see, #14641

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/7f9e2fca1a5f7af/output.txt

Total script time: 26.23 mins

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/5f73ea89661b0f7/output.txt

Total script time: 42.92 mins

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

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

@Snuffleupagus
Copy link
Collaborator

/botio browsertest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_browsertest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/78e3c81b81dcbab/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_browsertest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/d434381c52606ed/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/d434381c52606ed/output.txt

Total script time: 18.25 mins

  • Regression tests: FAILED
  different ref/snapshot: 43
  different first/second rendering: 2

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/78e3c81b81dcbab/output.txt

Total script time: 27.10 mins

  • Regression tests: FAILED
  different ref/snapshot: 13

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

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is correct, with the two questions answered/addressed, and overall this seems like a good rendering improvement to me. It would be good though if @Snuffleupagus also signed off on this before merging, to make sure I didn't miss anything since this logic is important for rendering and it seems a bit tricky looking at how the spec describes it and how other viewers implement it.

src/display/canvas.js Show resolved Hide resolved
src/display/canvas.js Show resolved Hide resolved
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, with the remaining comment above addressed (and the tests re-run if the change is made) and a check from @Snuffleupagus. Thank you for improving this!

@calixteman
Copy link
Contributor Author

@Snuffleupagus, gentle ping ?

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Apr 24, 2024

@Snuffleupagus, gentle ping ?

Sorry, last I looked at this there were still "open" review questions and then it slipped my mind (since I'm still short on time for open source stuff).

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try this and see how it works out, but we should obviously be prepared to (if necessary) tweak the logic and/or back-out this if there's any "serious" regressions in Firefox.

and rely on the Interpolate flag only when the image is upscaled.
Fixes mozilla#16273.
@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/1b259618dc2ab93/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/1ce3caf39edd715/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 27.18 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 44
  different first/second rendering: 2

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/1ce3caf39edd715/output.txt

Total script time: 43.04 mins

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

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

@calixteman calixteman merged commit 90d4b9c into mozilla:master Apr 24, 2024
9 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/b13a432d5316a70/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/4233a562b108213/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/b13a432d5316a70/output.txt

Total script time: 19.41 mins

  • Lint: Passed
  • 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/4233a562b108213/output.txt

Total script time: 24.95 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pdf document with image displayed in low quality
6 participants