8000 Fix creation of AGG images bigger than 1024**3 pixels by pkgw · Pull Request #19210 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 3 commits into from
Jan 5, 2021

Conversation

pkgw
Copy link
Contributor
@pkgw pkgw commented Dec 31, 2020

PR Summary

Various buffer size calculations were overflowing, because C is awesome. Simple reproduction case:

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.)

Closes #19209.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [N/A] New features are documented, with examples if plot related.
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

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.)
Copy link
@github-actions github-actions bot left a 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.

Copy link
Member
@jklymak jklymak left a 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)....

@WeatherGod
Copy link
Member
WeatherGod commented Dec 31, 2020 via email

@jklymak
Copy link
Member
jklymak commented Dec 31, 2020

No probably not - it took about 20 s on my machine....

Copy link
Member
@jklymak jklymak left a 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.

@pkgw
Copy link
Contributor Author
pkgw commented Dec 31, 2020

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.

@pkgw pkgw marked this pull request as draft December 31, 2020 19:17
Copy link
Member
@tacaswell tacaswell left a 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.

@jklymak
Copy link
Member
jklymak commented Dec 31, 2020

Yeah fair enough. I didn't test if it could actually draw anything ;-).

Copy link
Member
@tacaswell tacaswell left a 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.

@tacaswell tacaswell self-requested a review December 31, 2020 20:17
@tacaswell
Copy link
Member

I suspect you also need to get

agg::rendering_buffer buffer;
buffer.attach(
image.data(), (unsigned)image.dim(1), (unsigned)image.dim(0), -(int)image.dim(1) * 4);
pixfmt pixf(buffer);
as I am going to guess you are trying to render a very big image?

@pkgw
Copy link
Contributor Author
pkgw commented Dec 31, 2020

@tacaswell I don't think that bit should be an issue. The individual dimensions are well within 32 bits; it's unguarded multiplications of width * height that are causing problems.

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.
@pkgw pkgw marked this pull request as ready for review January 1, 2021 17:27
@pkgw
Copy link
Contributor Author
pkgw commented Jan 1, 2021

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>
@tacaswell tacaswell dismissed their stale review January 5, 2021 22:36

outdated, but I could not sort out how to clear reviews on drafts!

@tacaswell tacaswell merged commit 68ef27d into matplotlib:master Jan 5, 2021
@tacaswell
Copy link
Member

Awesome work @pkgw !

Congrats on your first PR to Matplotlib 🎉 and hopefully we will hear from you again!

@QuLogic QuLogic added this to the v3.4.0 milestone Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault when trying to create gigapixel image with agg backend
6 participants
0