Use integer coordinates when drawing images (bug 1264608, issue #3351) #14853

calixteman · 2022-04-28T14:55:39Z

it aims to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1264608;
it's only a partial fix for White lines though images on certain pdfs #3351;
some tiled images have some spurious white lines between the tiles.
When the current transform is applied the corners of an image can have
some non-integer coordinates leading to some extra transparency added
to handle that. So with this patch the current transform is applied on the
point and on the dimensions in order to have at the end only integer values.

Snuffleupagus

This looks like the sort of thing that we have to do regardless, but does it affect performance in any noticeable way?

Snuffleupagus · 2022-04-28T15:34:48Z

src/display/canvas.js

+  // bottom-right one is at (X + width, Y + height).
+
+  // If leftX is 4.321 then it's rounded to 4.
+  // If width is 10.432 then it's rounded to 11! because


I don't think you meant to use 11!, since that's actually 39916800 :-)
Please remove the exclamation mark here.

Snuffleupagus · 2022-04-28T15:37:25Z

test/test_manifest.json

+   },
+   {  "id": "issue3351.1",
+      "file": "pdfs/issue3351.1.pdf",
+      "md5": "4216245a5f18bb3eac80575ccf0b272d",
+      "rounds": 1,
+      "link": true,
+      "lastPage": 1,
+      "type": "eq"
+   },
+   {  "id": "issue3351.2",
+      "file": "pdfs/issue3351.2.pdf",
+      "md5": "fa3fd1659c409c7824ef8838c3071efc",
+      "rounds": 1,
+      "link": true,
+      "lastPage": 1,
+      "type": "eq"
+   },
+   {  "id": "issue3351.3",
+      "file": "pdfs/issue3351.3.pdf",
+      "md5": "60e2f2c480b6bc0e7f726743c6896520",
+      "rounds": 1,
+      "link": true,
+      "lastPage": 1,
+      "type": "eq"


Are all of the different code-paths that are calling drawImageAtIntegerCoords covered by these tests?

I firstly fixed the 2nd but without the hack for diag/anti-diag matrices and then in order to fix the 1st I had to add this hack.
And the 3rd is using a mask which was not positioned "correctly".

calixteman · 2022-04-28T16:29:06Z

This looks like the sort of thing that we have to do regardless, but does it affect performance in any noticeable way?

Do you have in mind a pdf with a lot of images ?
I profiled Doc2.pdf in beta with local server and there are small variations around 1/10 ms so as far as I can tell it's only noise.
MDN docs claim that it should be faster with integer coordinates so I guess the small overhead added because of rounding is almost offset by the improvement from the render. And it probably depends on the backend too, the graphic card, the machine itself...
My feeling is that in general there are not enough images on a page to see a noticeable difference (from user pov or from profiling pov).

calixteman · 2022-04-28T18:56:27Z

/botio test

pdfjsbot · 2022-04-28T18:56:29Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/565a7d6541ffb86/output.txt

pdfjsbot · 2022-04-28T18:56:29Z

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/42c39c2338044a9/output.txt

pdfjsbot · 2022-04-28T19:21:01Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/42c39c2338044a9/output.txt

Total script time: 24.52 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  errors: 12
  different ref/snapshot: 969
  different first/second rendering: 1

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

pdfjsbot · 2022-04-28T19:23:16Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/565a7d6541ffb86/output.txt

Total script time: 26.78 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: Passed
Regression tests: FAILED

  errors: 6
  different ref/snapshot: 486

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

Snuffleupagus · 2022-04-28T21:16:38Z

Given the huge number of changes it's possible that I've missed many issues, however the following test-cases stand out:

~~bug946506-page1~~
~~issue9462-page3~~
~~issue7303-page1~~
~~issue11230-page100~~
bug956965-page33, since it looks worse in some spots (e.g. the big "text" on the door)
~~issue8380-page1~~
~~issue8330-page1~~
~~issue2139-page1~~
~~issue12752-page1~~
~~issue2642-page1~~
~~issue1015~~
~~S2-eq-page1~~
Maybe images_1bit_grayscale-page1, since compared to Adobe Reader there appears to be a mix of improvements and regressions
~~issue5747-page1~~
~~issue12294-print-page1~~
~~issue13971-page1~~
~~issue2006-page1~~
~~High-Pressure-Measurement-WP-001287-page3~~
~~bug887152-page1~~
~~bug1077808-page1~~
decodeACSuccessive-page1

Please go through all of the test results carefully yourself, since there's a few too many clear regressions.

calixteman · 2022-04-29T13:08:17Z

/botio test

pdfjsbot · 2022-04-29T13:08:18Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-04-29T13:08:18Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/83ec4279cbfcd05/output.txt

pdfjsbot · 2022-04-29T13:38:58Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 24.47 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  errors: 12
  different ref/snapshot: 967

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

pdfjsbot · 2022-04-29T13:44:23Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/83ec4279cbfcd05/output.txt

Total script time: 26.98 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  errors: 6
  different ref/snapshot: 483

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

Snuffleupagus · 2022-04-29T13:52:57Z

Looking at the logs there's now a number of errors like ... | render : Error: Invalid canvas size[1], which suggest that the canvas.js-code is now somehow attempting to creating empty canvases.

[1] Those errors originate in

pdf.js/src/display/base_factory.js

Lines 18 to 57 in 24d5d5d

    
           class BaseCanvasFactory { 
        
             constructor() { 
        
               if (this.constructor === BaseCanvasFactory) { 
        
                 unreachable("Cannot initialize BaseCanvasFactory."); 
        
               } 
        
             } 
        
             create(width, height) { 
        
               if (width <= 0 || height <= 0) { 
        
                 throw new Error("Invalid canvas size"); 
        
               } 
        
               const canvas = this._createCanvas(width, height); 
        
               return { 
        
                 canvas, 
        
                 context: canvas.getContext("2d"), 
        
               }; 
        
             } 
        
             reset(canvasAndContext, width, height) { 
        
               if (!canvasAndContext.canvas) { 
        
                 throw new Error("Canvas is not specified"); 
        
               } 
        
               if (width <= 0 || height <= 0) { 
        
                 throw new Error("Invalid canvas size"); 
        
               } 
        
               canvasAndContext.canvas.width = width; 
        
               canvasAndContext.canvas.height = height; 
        
             } 
        
             destroy(canvasAndContext) { 
        
               if (!canvasAndContext.canvas) { 
        
                 throw new Error("Canvas is not specified"); 
        
               } 
        
               // Zeroing the width and height cause Firefox to release graphics 
        
               // resources immediately, which can greatly reduce memory consumption. 
        
               canvasAndContext.canvas.width = 0; 
        
               canvasAndContext.canvas.height = 0; 
        
               canvasAndContext.canvas = null; 
        
               canvasAndContext.context = null; 
        
             }

…la#3351) - it aims to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1264608; - it's only a partial fix for mozilla#3351; - some tiled images have some spurious white lines between the tiles. When the current transform is applyed the corners of an image can have some non-integer coordinates leading to some extra transparency added to handle that. So with this patch the current transform is applied on the point and on the dimensions in order to have at the end only integer values.

calixteman · 2022-04-29T14:02:07Z

/botio test

pdfjsbot · 2022-04-29T14:02:09Z

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/594bcade3639c71/output.txt

pdfjsbot · 2022-04-29T14:02:09Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/7931134130ba41f/output.txt

pdfjsbot · 2022-04-29T14:11:27Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/7931134130ba41f/output.txt

Total script time: 9.29 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: Passed

calixteman · 2022-04-29T14:21:30Z

/botio-windows test

pdfjsbot · 2022-04-29T14:21:31Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/72844dcc75baac3/output.txt

pdfjsbot · 2022-04-29T14:21:58Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/594bcade3639c71/output.txt

Total script time: 19.82 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  errors: 665
  different ref/snapshot: 566

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

Snuffleupagus · 2022-04-29T14:44:07Z

/botio browsertest

pdfjsbot · 2022-04-29T14:44:09Z

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/62538ed5ef8b498/output.txt

pdfjsbot · 2022-04-29T14:44:09Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/4313779aab6ca3d/output.txt

pdfjsbot · 2022-04-29T14:48:10Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/72844dcc75baac3/output.txt

Total script time: 26.63 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 490

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

pdfjsbot · 2022-04-29T15:05:02Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/62538ed5ef8b498/output.txt

Total script time: 20.88 mins

Regression tests: FAILED

  different ref/snapshot: 980

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

pdfjsbot · 2022-04-29T15:08:42Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/4313779aab6ca3d/output.txt

Total script time: 20.53 mins

Regression tests: FAILED

  different ref/snapshot: 490

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

Snuffleupagus · 2022-04-29T15:29:36Z

We obviously need to carefully go through all test-results again, to check that nothing else regressed.

In the meantime, I've only re-checked the list in #14853 (comment) and found a couple of remaining issues.

calixteman · 2022-04-29T15:38:06Z

I went through all the list and there is nothing obviously wrong.
In few of them there very small differences obviously visible when I keep the "t" pressed, but when I compare the output with acrobat at 100% (and with my eyes) I don't see anything obviously wrong (e.g. in images_1bit_grayscale-page1 or bug956965-page33).

Snuffleupagus · 2022-04-29T15:45:36Z

I went through all the list and there is nothing obviously wrong.

Do you mean that you've gone through all of #14853 (comment) and #14853 (comment), since we need to do that before moving forward with (and landing) this PR?

bug956965-page33

So that one looks OK in the viewer, when comparing with Adobe Reader? Because it looks like a pretty obvious regression in the reftest-analyzer.

calixteman · 2022-04-29T15:51:16Z

It's what I see:

calixteman · 2022-04-29T15:53:17Z

And on an other screen with a lower resolution:

calixteman · 2022-04-29T15:59:21Z

And yes I went through all the 490 tests, it's a bit long... and I did it this morning with the tests from yesterday and thx to that I fixed all the bugs I guess.

Snuffleupagus

r=me, let's do this :-)

calixteman · 2022-04-29T16:15:11Z

/botio makeref

pdfjsbot · 2022-04-29T16:15:12Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/4013a619e718697/output.txt

pdfjsbot · 2022-04-29T16:15:12Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-04-29T16:38:37Z

From: Bot.io (Linux m4)

Success

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

Total script time: 21.57 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-04-29T16:39:47Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/4013a619e718697/output.txt

Total script time: 21.26 mins

Lint: Passed
Make references: Passed
Check references: Passed

calixteman requested a review from Snuffleupagus April 28, 2022 14:55

Snuffleupagus reviewed Apr 28, 2022

View reviewed changes

Snuffleupagus added core image-quality labels Apr 28, 2022

calixteman force-pushed the white_lines branch from f663540 to 858409f Compare April 28, 2022 16:43

calixteman force-pushed the white_lines branch from 858409f to a08f0cd Compare April 29, 2022 12:37

calixteman force-pushed the white_lines branch from a08f0cd to 624d8a8 Compare April 29, 2022 14:01

Snuffleupagus approved these changes Apr 29, 2022

View reviewed changes

calixteman merged commit b10b8da into mozilla:master Apr 29, 2022

Snuffleupagus mentioned this pull request May 8, 2022

Time for a new release? #14892

Closed

calixteman mentioned this pull request Dec 6, 2022

Set the dimensions of the various layers at their creation #15770

Merged

Use integer coordinates when drawing images (bug 1264608, issue #3351) #14853

Use integer coordinates when drawing images (bug 1264608, issue #3351) #14853

Conversation

calixteman commented Apr 28, 2022

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Apr 28, 2022

Choose a reason for hiding this comment

Snuffleupagus Apr 28, 2022

Choose a reason for hiding this comment

calixteman Apr 28, 2022

Choose a reason for hiding this comment

calixteman commented Apr 28, 2022

calixteman commented Apr 28, 2022

pdfjsbot commented Apr 28, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 28, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 28, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Apr 28, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus commented Apr 28, 2022 • edited Loading

calixteman commented Apr 29, 2022

pdfjsbot commented Apr 29, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 29, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 29, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Apr 29, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus commented Apr 29, 2022 • edited Loading

calixteman commented Apr 29, 2022

pdfjsbot commented Apr 29, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 29, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 29, 2022

From: Bot.io (Windows)

Failed

calixteman commented Apr 29, 2022

pdfjsbot commented Apr 29, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 29, 2022

From: Bot.io (Linux m4)

Failed

Snuffleupagus commented Apr 29, 2022

pdfjsbot commented Apr 29, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 29, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 29, 2022

From: Bot.io (Windows)

Failed

pdfjsbot commented Apr 29, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Apr 29, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus commented Apr 29, 2022 • edited Loading

calixteman commented Apr 29, 2022

Snuffleupagus commented Apr 29, 2022

calixteman commented Apr 29, 2022

calixteman commented Apr 29, 2022

calixteman commented Apr 29, 2022

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus commented Apr 28, 2022 •

edited

Loading

Snuffleupagus commented Apr 29, 2022 •

edited

Loading

Snuffleupagus commented Apr 29, 2022 •

edited

Loading