-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix creation of AGG images bigger than 1024**3 pixels #19210
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a test to show that it works? Even just a test that it doesn't error out (versus an image comparison)....
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.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned that this is only the tip of the iceberg of things that will need to be converted to long as I think that AGG internally uses 32bit ints in the rasterization logic.
Yeah fair enough. I didn't test if it could actually draw anything ;-). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to dismiss my previous review, still concerned that this is the tip of an iceberg, but no longer sure that it will be a big iceberg.
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
pytest
passes).flake8
on changed files to check).flake8-docstrings
and 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).