Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Limit the size of canvases to 5MP (iOS restriction) #4834
Conversation
| ctx.scale(outputScale.sx, outputScale.sy); | ||
| ctx._scaleX = outputScale.sx * correctiveScale.sx; | ||
| ctx._scaleY = outputScale.sy * correctiveScale.sy; | ||
| if (outputScale.scaled | correctiveScale.scaled) { |
Snuffleupagus
May 24, 2014
Contributor
Shouldn't it be || instead of |?
Shouldn't it be || instead of |?
|
Limiting the canvas size should generally be a good thing, but isn't there a risk that hard-coding one value that will apply to all platforms/devices could cause unnecessary restrictions (e.g. blurry canvases at higher zoom levels on desktop)? Perhaps we could instead introduce e.g. |
|
Done. |
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/bcf0b4658e5764b/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/bcf0b4658e5764b/output.txt Total script time: 0.70 mins Published
|
| @@ -134,6 +134,14 @@ PDFJS.verbosity = (PDFJS.verbosity === undefined ? | |||
| PDFJS.VERBOSITY_LEVELS.warnings : PDFJS.verbosity); | |||
|
|
|||
| /** | |||
| * The maximum supported canvas size in total pixels e.g. width * height. | |||
| * Use -1 for no limit. | |||
Snuffleupagus
May 24, 2014
Contributor
Perhaps also add a comment about the default value, i.e. something like this:
* The default value is 8192 * 8192. Use -1 for no limit.
Perhaps also add a comment about the default value, i.e. something like this:
* The default value is 8192 * 8192. Use -1 for no limit.
| @@ -575,3 +575,11 @@ if (typeof PDFJS === 'undefined') { | |||
| window.setTimeout(callback, 20); | |||
| }); | |||
| })(); | |||
|
|
|||
| (function checkCanvasSizeLimitation() { | |||
| var isIOS = navigator.userAgent.match(/(iPad|iPhone|iPod)/g) ? true : false; | |||
Snuffleupagus
May 24, 2014
Contributor
Looks unnecessarily complicated, what about (similarily below):
/(iPad|iPhone|iPod)/g.test(navigator.userAgent)
Looks unnecessarily complicated, what about (similarily below):
/(iPad|iPhone|iPod)/g.test(navigator.userAgent)
|
Done. |
| canvas.width = (Math.floor(viewport.width) * outputScale.sx) | 0; | ||
| canvas.height = (Math.floor(viewport.height) * outputScale.sy) | 0; | ||
| canvas.width = ( | ||
| Math.floor(viewport.width) * outputScale.sx * correctiveScale.sy |
Snuffleupagus
May 24, 2014
Contributor
Typo at the end of the line, should be correctiveScale.sx.
Typo at the end of the line, should be correctiveScale.sx.
|
Oops. Sorry. |
|
Rebased. |
| correctiveScale.sy = maxScale / outputScale.sy; | ||
| correctiveScale.scaled = true; | ||
| } | ||
| } |
yurydelendik
May 29, 2014
Contributor
Looking at the entire diff, I have feeling that this code (above and below) can be simplified to something like:
var pixelsInViewport = viewport.width * viewport.height;
if (PDFJS.maxCanvasPixels > 0 && pixelsInViewport > PDFJS.maxCanvasPixels) {
var correction = Math.sqrt(PDFJS.maxCanvasPixels / pixelsInViewport);
outputScale.sx *= correction;
outputScale.sy *= correction;
outputScale.scaled = true;
}
Looking at the entire diff, I have feeling that this code (above and below) can be simplified to something like:
var pixelsInViewport = viewport.width * viewport.height;
if (PDFJS.maxCanvasPixels > 0 && pixelsInViewport > PDFJS.maxCanvasPixels) {
var correction = Math.sqrt(PDFJS.maxCanvasPixels / pixelsInViewport);
outputScale.sx *= correction;
outputScale.sy *= correction;
outputScale.scaled = true;
}
|
At https://github.com/dferer/pdf.js/blob/canvas-max-size/web/page_view.js#L122, can we check if we are already showing corrected css scale and canvas size if still at max viewport pixel to update only transform? |
|
Really nice. I think with this patch we can also increase MAX_SCALE value a little (e.g. up to 10?) |
That would mean that we'd also fix https://bugzilla.mozilla.org/show_bug.cgi?id=827496 at the same time. |
|
I think I addressed all your remarks. |
|
/botio-windows preview |
From: Bot.io (Windows)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/9a06581fa49e880/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/9a06581fa49e880/output.txt Total script time: 0.92 mins Published
|
Apart from huge memory consumption, I'm also seeing rendering issues similar to those of #605 at zoom levels around 800 % (on Windows). So we might indeed want to limit the maximum values somewhat. Edit: Just wanted to point out that the rendering issues mentioned above are in no way caused by the PR. |
|
Try 4096x4096 which will be max webgl texture size. (400dpi for 10x10inch printed material) |
|
Done |
|
let me paste this info here for completeness. I was wondering why I can see in chrome profilings the drawImage for thumbnails, but not the ones for the main content. The one intel guy says that it is not really a use case to copy from a large canvas to a small canvas. Maybe one of you guys want to comment there that pdfjs has this use case? |
|
@dferer this looks really good. Can we use internal |
|
@yurydelendik Something like this? |
|
yes, thank you /botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @yurydelendik received. Current queue size: 3 Live output at: http://107.21.233.14:8877/ed642fe4325a78a/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/ed642fe4325a78a/output.txt Total script time: 0.74 mins Published
|
Limit the size of canvases to 5MP (iOS restriction)
ghost
deleted the
This is an attempt to fix #2439 and maybe some other related issues.
It seems to work for me but more tests are definitely needed.