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) { |
There was a problem hiding this comment.
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. | |||
There was a problem hiding this comment.
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.
|
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 |
There was a problem hiding this comment.
Typo at the end of the line, should be correctiveScale.sx.
|
Oops. Sorry. |
|
Rebased. |
| correctiveScale.sy = maxScale / outputScale.sy; | ||
| correctiveScale.scaled = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.