search

matthewwithanm / pilkit

Utilities and processors built for, and on top of PIL
BSD 3-Clause "New" or "Revised" License
196 stars 54 forks source link

Sets MAXBLOCK based on original image size #6

Closed ilhan closed 11 years ago

ilhan commented 11 years ago

I hope someone working with Pillow will be able to fix the problem but here is a pull request for a better(?) workaround which should fix https://github.com/matthewwithanm/django-imagekit/issues/231. My premiss is that utils.process_image always get called, so I set original_size = img.size and pass that to > img_to_fobj > save_image. Have I neglected something? Is it a style-wise way to pass that parameter?

ilhan commented 11 years ago

Hm, what if someone scales the image up:) This should handle that: ImageFile.MAXBLOCK = max(original_size + img.size) \ 2

matthewwithanm commented 11 years ago

Sorry, I'm not following the logic. Why would this be based on original_size? At the point where we're saving, the original image is gone and its size shouldn't be a factor.

ilhan commented 11 years ago

Yeah, I made a wrong conclusion but here was my thought process: Old solution: ImageFile.MAXBLOCK = max(img.size) \ 2 where img.size is the size of the newly generated image. So if I process an image in the size of 1920x1080 to 300x300 MAXBLOCK will be set to 3002, which might not be enough. So instead I do: ImageFile.MAXBLOCK = max(original_size + img.size) \ 2 and MAXBLOCK will be set to 19202, which is enough. And wont cause OverflowError: string is too large like sys.maxint gave me.

I thought that MAXBLOCK had some relation to the original image size but it doesn't seem to have from what I understand reading the Pillow issue discussion. So this is basically unnecessary code when we still are guessing.

Regarding why sys.maxint didnt work for me on production when it did work on my developer machine where sys.maxint is much higher is over my head right now. It might be better to try to understand that since this is a backup workaround if Pillow don't manage to fix the underlying issue. The best solution must to set a safe arbitrary value with least amount of code. Need more thought. I'll get back.

matthewwithanm commented 11 years ago

Alright, add7d2ca2a1f04c88a909da58c78945f3b0bda25 should fix this…hopefully once and for all! The MAXBLOCK thing really is a PIL/Pillow issue, and fixed in the latest release of Pillow (which the solution in add7d2ca2a1f04c88a909da58c78945f3b0bda25 is taken from) but of course we need to support PIL as well for the time being. My recommendation, though, is to use Pillow if it's an option.

ilhan commented 11 years ago

Just ran some scripts on the same server to recreate my old situation(scaling 6 large newly downloaded images per request) Worked without problems on master branch! Thanks a lot!

matthewwithanm commented 11 years ago

Great! But all thanks goes to @wiredfool on this one (:

ilhan commented 11 years ago

OK, did a test on an old app with an old version of Pillow==1.7.8: pilkit 1.1.2 gives me OverflowError. Update to pilkit==1.1.3 works great. Time to leave MAXBLOCK behind us:)

matthewwithanm commented 11 years ago

:thumbsup: