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

Don't downscale by steps images which will end up with small dimensions (bug 1445735) #16482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

calixteman
Copy link
Contributor

No description provided.

@Snuffleupagus
Copy link
Collaborator

Maybe add https://bugzilla.mozilla.org/attachment.cgi?id=8958936 as a linked eq-test?

@Snuffleupagus
Copy link
Collaborator

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/8d8638c98cec67e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 27.90 mins

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

Image differences available at: http://54.241.84.105:8877/5a6c47b79ac7b9a/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/8d8638c98cec67e/output.txt

Total script time: 35.48 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 74
  different first/second rendering: 1

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

@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/9e028dbde979716/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/042adaa0245dd21/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 27.70 mins

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

Image differences available at: http://54.241.84.105:8877/9e028dbde979716/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/042adaa0245dd21/output.txt

Total script time: 33.87 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 43
  different first/second rendering: 1

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

@calixteman
Copy link
Contributor Author

I'd say that the differences are acceptable. @Snuffleupagus, wdyt ?

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.

The following look like, somewhat clear, regressions:

  • fit11-talk
  • pr8808-page1, which is now too "dark" when compared with e.g. Adobe Reader
  • issue1658-page10

@@ -1188,6 +1188,16 @@ class CanvasGraphics {
1
);

if (width / widthScale <= 5 || height / heightScale <= 5) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we perhaps limit this to cases where both dimensions are small, hence replace the OR with AND above?

@calixteman
Copy link
Contributor Author

The following look like, somewhat clear, regressions:

* `fit11-talk`

The "missing" tip of the curly braces on the images are only one pixel which was gray and is now white.
On a normal screen it's almost invisible and if the user needs to really read the content they should zoom and the tip will be there.

* `pr8808-page1`, which is now too "dark" when compared with e.g. Adobe Reader

I opened the pdf in nightly and in Acrobat on mac at 100% and I can't see any differences (that said I'm not that young... I'm likely not a reference).

* `issue1658-page10`

At normal scales there is no problem but I agree at small scales (when the text is really unreadable) some lines become invisible and it's the same in Acrobat.
That said without the patch, the rendering at 22% is better in Firefox when compared to Acrobat, but who cares about the rendering at 22% ?

My feeling is that the images we've in tests aren't representative of what the users have in real life... (on linux, for example, we use a headless mode). It's just a feeling, hence I can be completely wrong here.
That said, I trust your judgement here: so if you prefer having a AND, I'm fine with that (I began with a AND and I hesitated and finally I chose the OR).

@Snuffleupagus
Copy link
Collaborator

That said, I trust your judgement here: so if you prefer having a AND, I'm fine with that (I began with a AND and I hesitated and finally I chose the OR).

I think that it'd be interesting to see what using AND does to the test-results, since then we'd only consider "truly" small images. Otherwise narrow images are affected as well, which might not actually be what you want here?

@Snuffleupagus
Copy link
Collaborator

My feeling is that the images we've in tests aren't representative of what the users have in real life... (on linux, for example, we use a headless mode). It's just a feeling, hence I can be completely wrong here.

It could perhaps be connected to the window.devicePixelRatio value, since I believe that the bots run with window.devicePixelRatio === 1 set?
Hence I suppose that it's possible that things may look slightly "worse" on such systems, but good/fine when testing locally on modern hardware where window.devicePixelRatio > 1.

@calixteman
Copy link
Contributor Author

I'll test tomorrow with one of my old screen with a devicePixelRatio equal to 1 to see if there's something noticeable.

@calixteman calixteman force-pushed the dont_always_rescale_images branch 2 times, most recently from 0b33db6 to 500a08e Compare May 29, 2023 07:30
@calixteman
Copy link
Contributor Author

/botio browsertest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/64fc9166533d5fc/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/d5d30514e668dcd/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/64fc9166533d5fc/output.txt

Total script time: 23.09 mins

  • Regression tests: FAILED
  different ref/snapshot: 38
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/64fc9166533d5fc/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/d5d30514e668dcd/output.txt

Total script time: 23.42 mins

  • Regression tests: FAILED
  different ref/snapshot: 36
  different first/second rendering: 1

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

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jun 2, 2023

I'll test tomorrow with one of my old screen with a devicePixelRatio equal to 1 to see if there's something noticeable.

What became of this testing, since I'm still not sure that this patch is necessarily always better (e.g. in the mentioned case)?

@calixteman
Copy link
Contributor Author

I'll test tomorrow with one of my old screen with a devicePixelRatio equal to 1 to see if there's something noticeable.

What became of this testing, since I'm still not sure that this patch is necessarily always better (e.g. in the mentioned case)?

I wrote something but I obviously forgot to click on the "Comment" button...

Anyway on some pdfs the lines weren't correctly rendered when compared with Acrobat rendering (the black was too gray).
So normally the last browsertests were with a AND instead of a OR (but I'm not sure I had to fix that when I rebased my patch).

@calixteman
Copy link
Contributor Author

/botio browsertest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/68cdedbe61a9838/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/14edc2a90279730/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/68cdedbe61a9838/output.txt

Total script time: 20.16 mins

  • Regression tests: FAILED
  different ref/snapshot: 39
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/68cdedbe61a9838/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/14edc2a90279730/output.txt

Total script time: 23.70 mins

  • Regression tests: FAILED
  different ref/snapshot: 42

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

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.

None yet

3 participants