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

Add an option to process huge images #431

Merged
merged 3 commits into from
Jun 16, 2021
Merged

Add an option to process huge images #431

merged 3 commits into from
Jun 16, 2021

Conversation

dsschult
Copy link
Contributor

This PR adds an option to set PIL.Image.MAX_IMAGE_PIXELS, to enable huge image processing.

Fixes #332.

@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #431 (8537afe) into main (7ba5f7b) will increase coverage by 0.03%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
+ Coverage   86.48%   86.52%   +0.03%     
==========================================
  Files          22       22              
  Lines        1828     1833       +5     
==========================================
+ Hits         1581     1586       +5     
  Misses        247      247              
Impacted Files Coverage Δ
sigal/settings.py 97.87% <ø> (ø)
sigal/gallery.py 89.44% <87.50%> (-0.10%) ⬇️
sigal/image.py 91.66% <0.00%> (+0.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ba5f7b...8537afe. Read the comment docs.

Copy link
Owner

@saimn saimn left a comment

Choose a reason for hiding this comment

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

Thanks @dsschult, just a few comments.

@@ -768,6 +773,9 @@ def log_func(x):
"defined in the settings file, which can't be serialized.",
exc_info=True)
sys.exit('Abort')
finally:
self.pool.close()
self.pool.join()
Copy link
Owner

Choose a reason for hiding this comment

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

Why moving this here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If an exception gets raised (in debug mode), the pool still needs to be closed. Otherwise you can sometimes get errors like

Exception ignored in: <function Pool.__del__ at 0x7f3744d57670>
Traceback (most recent call last):
  File "/usr/lib/python3.9/multiprocessing/pool.py", line 268, in __del__
    self._change_notifier.put(None)
  File "/usr/lib/python3.9/multiprocessing/queues.py", line 378, in put
    self._writer.send_bytes(obj)
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 205, in send_bytes
    self._send_bytes(m[offset:offset + size])
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 416, in _send_bytes
    self._send(header + buf)
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 373, in _send
    n = write(self._handle, buf)
OSError: [Errno 9] Bad file descriptor

Copy link
Owner

Choose a reason for hiding this comment

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

Right indeed, I was focused on moving this after terminate but it seems fine so 👍

gal = Gallery(settings, ncpu=1)
gal.build()

settings['max_img_pixels'] = 100000000
Copy link
Owner

Choose a reason for hiding this comment

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

monkeypatch sets MAX_IMAGE_PIXELS to the same value above, why ?

Copy link
Owner

Choose a reason for hiding this comment

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

Can you also use 100_000_000 instead for better readability ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

monkeypatch sets MAX_IMAGE_PIXELS to the same value above, why ?

I'm using monkeypatch to make sure the value goes "back to default" after the test. The value here doesn't really matter, since I am setting it explicitly in the test.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I missed the 5000 one above. Can you add a comment explaining why monkeypatch is used ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done.

@saimn saimn merged commit fee8a3c into saimn:main Jun 16, 2021
@saimn
Copy link
Owner

saimn commented Jun 16, 2021

Thanks!

@saimn saimn modified the milestones: 2.2, 2.3 Jun 16, 2021
@dsschult dsschult deleted the huge_image branch June 16, 2021 21:20
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.

Huge images cause PIL.Image.DecompressionBombError exception
2 participants