Fix creation of AGG images bigger than 1024**3 pixels #19210
Conversation
Various buffer size calculations were overflowing, because C is awesome.
Simple reproduction case:
```python
import matplotlib
matplotlib.use('agg')
import matplotlib.pyplot as plt
fig, ax = plt.subplots(figsize=(463, 232), dpi=100)
fig.savefig('big.png')
```
(A size of 462x232 is just *under* the gigapixel threshold.)
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things to fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process please see the developer guide
We strive to be a welcoming and open project, please follow our Code of Conduct.
|
Do we really want to have a test that requires generating a gigapixel image
running in our CI? Maybe tag it such that it gets run only as part of a
release candidate or something?
…On Thu, Dec 31, 2020 at 1:40 PM Jody Klymak ***@***.***> wrote:
***@***.**** commented on this pull request.
Does this need a test to show that it works? Even just a test that it
doesn't error out (versus an image comparison)....
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#19210 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHF6BD3N6N35K65LPSMP3SXTAQZANCNFSM4VPROS3Q>
.
|
|
No probably not - it took about 20 s on my machine.... |
|
Yes, fortunately it seems that when the image data are all zeros, the test case isn't that demanding (given a modern machine with more than a few gigs of RAM). I'm going to mark this as a draft, though, because my final use case is still getting a segfault. It might end up being something that is best addressed in a separate PR, but maybe it will be something that I can reasonably tack on to this one. |
|
Yeah fair enough. I didn't test if it could actually draw anything ;-). |
|
I suspect you also need to get Lines 837 to 840 in a8f52d9 |
|
@tacaswell I don't think that bit should be an issue. The individual dimensions are well within 32 bits; it's unguarded multiplications of That being said, I'm having more trouble figuring out what's causing my current crash. It's deeper in AGG, in the rasterization code, and the C++ templates are not playing nicely with my debugger. |
When AGG rasterizes a horizontal line, it has code to check if the computations will overflow its fixed-precision coordinate math. If an hline is too wide, it will subdivide the line into two pieces until the overflow is avoided. Except a `return` statement was missing, so overflow-y code ran as well as the subdivided code! Leading to negative coordinates and a crash. This only happens for lines that span more than something like 32768 pixels in the X direction, so it's not something that happens in regular usage.
|
I believe that I've fixed the remaining issue in my use case. Another one-liner — see the commit message for an explanation. The fix is inside AGG, but if you read the code of the function in question I think it's pretty clear that there is a simple mistake in the function's overflow prevention. |
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
outdated, but I could not sort out how to clear reviews on drafts!
|
Awesome work @pkgw ! Congrats on your first PR to Matplotlib 🎉 and hopefully we will hear from you again! |
PR Summary
Various buffer size calculations were overflowing, because C is awesome. Simple reproduction case:
(A size of 462x232 is just under the gigapixel threshold.)
Closes #19209.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).