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.

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

Fix: bring the image size limit of PIL in line #373

Closed
wants to merge 1 commit into from

Conversation

ahyangyi
Copy link

The intention of the current code is to limit heightmap to at most 16384 * 16384. However, the actual limit is smaller due to PIL having a similar protection mechanism.

This patch sets the PIL limit to match our intention.

Locally tested.

TrueBrain
TrueBrain previously approved these changes May 27, 2023
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Tnx for the fix!

It will take a bit of time before this rolls out, as we are making some changes to the infra .. but it will get there :)

PS: don't worry about the commit checker failing, I will handle that when merging :)

TrueBrain
TrueBrain previously approved these changes May 27, 2023
@TrueBrain
Copy link
Member

Turns out, this need a bit more thinking.

That is to say, this PR is better than the code was, but we hit another problem: a 16kx16k map consumes so much memory, that the backend is killed for overusing its memory allocation. Of course we can increase that allocation, but it requires 1+GB of RAM to process a single map. That is ... not within the realm of possibilities :D

An additional issue is that processing even a 8kx8k map takes over 30 seconds ..

So I need to give this a better look; mostly, I am wondering if we should actually use PIL, or if we can somehow stream the image, and not have everything in memory :) I will give this some more thought in the next few weeks .. basically: it is not your PR, but it does make it more visible :)

@@ -47,14 +51,11 @@ def read(self, fp):

try:
im = Image.open(fp)
except Image.DecompressionBombError:
raise ValidationException("Image is too large.")
Copy link
Member

Choose a reason for hiding this comment

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

If the number of pixels is greater than twice [MAX_IMAGE_PIXELS](https://pillow.readthedocs.io/en/stable/reference/Image.html#PIL.Image.MAX_IMAGE_PIXELS), then a DecompressionBombError will be raised instead.

I was wondering why my 20kx20k map wasn't triggering this, but I have to go slightly bigger before it does so :) Seems we need both statements or something to make this work properly :)

@TrueBrain
Copy link
Member

After a lot of fiddling, decided to go the evil way, and came up with #391.

Basically, this drops Pillow, and resolves all other issues we have with heightmaps (slow, memory hungry, etc).

Nevertheless, tnx for this work, and I hope you feel up for picking up a few more BaNaNaS tickets :) This only was one .. that had more issues than was visible on the surface :) Nothing I even knew when we were talking about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants