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.

Sign up for GitHub

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

fixed memory leak when layer offset lower than mask offset #15102

Closed
wants to merge 1 commit into from
Closed

fixed memory leak when layer offset lower than mask offset #15102

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/display/canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,12 +1127,9 @@ function genericComposeSMask(
const chunkSize = Math.min(height, Math.ceil(PIXELS_TO_PROCESS / width));
for (let row = 0; row < height; row += chunkSize) {
const chunkHeight = Math.min(chunkSize, height - row);
const maskData = maskCtx.getImageData(
layerOffsetX - maskOffsetX,
row + (layerOffsetY - maskOffsetY),
width,
chunkHeight
);
const maskX = Math.max(0, layerOffsetX - maskOffsetX);
Copy link
Contributor

@timvandermeij timvandermeij Jun 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This PR should ideally get a test case (PDF file as mentioned above), but if that's hard at the very least a comment here on why this is necessary to prevent it from being removed accidentally during e.g., refactoring. I think the line from your PR description, In some PDF files the mask offset is larger than the layer offset. In this case first two parameters of getImageData become negative numbers and produce a memory leak., is already quite a good and descriptive one.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus Jun 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that this is Node.js-specific, which probably makes the need for a test-case slightly less important?
Also, the example provided below is 14.5 MB in size (for a one-page document) so it's probably not a great reduced test-case unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep according to the getImageDatadoc:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/getImageData
it's ok to have negative sx and sy and the corresponding pixels in the extracted data should be black transparent.

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 view this as a bug in the node-canvas package then, or do we want to consider accepting this work-around here?

const maskY = Math.max(0, row + (layerOffsetY - maskOffsetY));
const maskData = maskCtx.getImageData(maskX, maskY, width, chunkHeight);
const layerData = layerCtx.getImageData(
layerOffsetX,
row + layerOffsetY,
Expand Down