search

mozilla / pdf.js

PDF Reader in JavaScript
https://mozilla.github.io/pdf.js/
Apache License 2.0
47.17k stars 9.82k forks source link

Respect `maxCanvasPixels` when computing canvas dimensions #18218

Closed nicolo-ribaudo closed 2 weeks ago

nicolo-ribaudo commented 4 weeks ago

Closes https://github.com/mozilla/pdf.js/issues/18105

Review this PR disabling whitespace changes, since one of the existing tests is indented and it makes the diff quite confusing.

timvandermeij commented 3 weeks ago

/botio-linux preview

moz-tools-bot commented 3 weeks ago

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/504be7417e49d80/output.txt

moz-tools-bot commented 3 weeks ago

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/504be7417e49d80/output.txt

Total script time: 1.08 mins

Published

timvandermeij commented 3 weeks ago

/botio integrationtest

moz-tools-bot commented 3 weeks ago

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/44aeb66a25858b3/output.txt

moz-tools-bot commented 3 weeks ago

From: Bot.io (Windows)

Received

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

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

moz-tools-bot commented 3 weeks ago

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/44aeb66a25858b3/output.txt

Total script time: 7.77 mins

nicolo-ribaudo commented 3 weeks ago

The "PDF viewer CSS-only zoom triggers when going bigger than maxCanvasPixels test correctly configured" failure means that the numbers I hard-coded don't work for these tests on the CI machine.

I picked them based on how big the Firefox/Chrome windows were on my machine when running the tests. Do you know:

moz-tools-bot commented 3 weeks ago

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/dcdf1f861b1fcba/output.txt

Total script time: 19.96 mins

timvandermeij commented 3 weeks ago

I'm not sure how big the viewport is on the bots, but in general I think we should probably set a fixed viewport size for the integration tests to eliminate any possible differences between configurations (since we also want everything to work correctly locally where window sizes might differ). Fortunately Puppeteer already seems to support this in the launch() call, refer to https://pptr.dev/api/puppeteer.browserconnectoptions#properties, and this should not cause other tests to fail (and if they do, we found an implicit dependency on the viewport size which we should then fix).

timvandermeij commented 3 weeks ago

The default viewport property for the launch() call turns out to already be set at https://github.com/mozilla/pdf.js/blob/bb73d2a92240a5177268a652f400a98805f83682/test/test.mjs#L888 but it's set to null instead of a fixed value. That dates back to my initial commit that introduced Puppeteer, but I don't really know anymore why I apparently explicitly set it to that :sweat_smile:

I would say that we can pick a standard value (maybe 1024x768 or 1280x1024) here which should be fine for any modern configuration.

Snuffleupagus commented 3 weeks ago

That dates back to my initial commit that introduced Puppeteer, but I don't really know anymore why I apparently explicitly set it to that 😅

That's done to address https://github.com/mozilla/pdf.js/pull/11807#pullrequestreview-400514439, so please don't change that unconditionally since it'd make working with browsertest results more "annoying" locally.

I would say that we can pick a standard value (maybe 1024x768 or 1280x1024) here which should be fine for any modern configuration.

If so, please limit that to only the integration tests.

timvandermeij commented 3 weeks ago

Good point. Alternatively, instead of setting a fixed viewport, it might also be possible to dynamically determine the values in this PR based on the current viewport size (perhaps a certain fraction/percentage of it) which can be fetched using e.g. the approach from https://stackoverflow.com/questions/1248081/how-to-get-the-browser-viewport-dimensions? That avoids changing the existing viewport behavior while also having a way to deal with different viewport sizes, and it avoids having to change the test if we would later decide to increase the fixed viewport size if we went with that approach.

nicolo-ribaudo commented 2 weeks ago

It turns out that my test was not depending on the viewport dimensions :) The PDF size is guaranteed to be consistent, given that it's always reset to "50%" in beforeEach. However, given a fixed PDF size, the canvas size depends on the pixel ratio.

I changed the options to pass a maxCanvasPixels based on the pixel ratio, which solves this problem.

timvandermeij commented 2 weeks ago

/botio integrationtest

moz-tools-bot commented 2 weeks ago

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/971890288bbb4c2/output.txt

moz-tools-bot commented 2 weeks ago

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/91894c56e5e5e04/output.txt

moz-tools-bot commented 2 weeks ago

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/971890288bbb4c2/output.txt

Total script time: 7.86 mins

moz-tools-bot commented 2 weeks ago

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/91894c56e5e5e04/output.txt

Total script time: 18.51 mins

timvandermeij commented 2 weeks ago

/botio-linux preview

moz-tools-bot commented 2 weeks ago

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/76d3b7a02ce2390/output.txt

moz-tools-bot commented 2 weeks ago

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/76d3b7a02ce2390/output.txt

Total script time: 1.04 mins

Published

timvandermeij commented 2 weeks ago

/botio test

moz-tools-bot commented 2 weeks ago

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

moz-tools-bot commented 2 weeks ago

From: Bot.io (Windows)

Received

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

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

moz-tools-bot commented 2 weeks ago

From: Bot.io (Linux m4)

Failed

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

Total script time: 28.74 mins

  different ref/snapshot: 16

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

moz-tools-bot commented 2 weeks ago

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/b65656c11c97246/output.txt

Total script time: 44.07 mins

  different ref/snapshot: 5

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

timvandermeij commented 2 weeks ago

Thank you for improving this!