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

Fix: regression failure with Pillow 8.1.0 #182

Merged
merged 1 commit into from Jan 21, 2021
Merged

Conversation

@glx22
Copy link
Contributor

@glx22 glx22 commented Jan 6, 2021

A change in Pillow 8.1.0 (release 4 days ago) triggers OSError: buffer overrun when reading image file for opengfx_generic_trams1.pcx (371x150) and OSError: image file is truncated (0 bytes not processed) for opengfx_trains_start.pcx (409x210)

I don't really know if it's a bug in Pillow, but I fixed the issue by croping first file to 370x150 and second file to 408x210.

@LordAro
Copy link
Member

@LordAro LordAro commented Jan 6, 2021

I'm not sure I like changing the tests to fix a bug :)
At the very least, shouldn't nml handle files of "broken" sizes better?

@glx22
Copy link
Contributor Author

@glx22 glx22 commented Jan 6, 2021

I'm not sure the sizes are "broken" as it worked perfectly before Pillow update.

@LordAro
Copy link
Member

@LordAro LordAro commented Jan 7, 2021

No, as far as NML is concerned they are not. But the images (assuming they're not actually invalid) should not have to be changed to work around Pillow bugs/changed behaviour

And also, NML should handle such "broken" images better than outputting a python stacktrace

@glx22 glx22 force-pushed the glx22:regression branch from a76091c to e63e3e3 Jan 7, 2021
@glx22
Copy link
Contributor Author

@glx22 glx22 commented Jan 7, 2021

I think the best for now is to not use Pillow >=8.1.0
Then the best move will be to try to write a minimal test case and submit a bug report to Pillow.

@glx22
Copy link
Contributor Author

@glx22 glx22 commented Jan 11, 2021

Opened an issue on pillow

@glx22
Copy link
Contributor Author

@glx22 glx22 commented Jan 20, 2021

Got a reply. The failing PCX are indeed malformed (according to spec linked in the issue comment). But they opened a PR that may allow loading, as some image editors (like gimp) can create malformed PCX.
So still waiting.

@LordAro
Copy link
Member

@LordAro LordAro commented Jan 21, 2021

Given that it's out of spec, it's probably fine to change the images, but fixing the CI is more important right now, imo

@glx22 glx22 merged commit 267b8d2 into OpenTTD:master Jan 21, 2021
21 checks passed
21 checks passed
Commit checker
Details
Python 3.5 on ubuntu-latest
Details
Security and Quality Security and Quality
Details
Python 3.6 on ubuntu-latest
Details
Python 3.7 on ubuntu-latest
Details
Python 3.8 on ubuntu-latest
Details
Python pypy3 on ubuntu-latest
Details
Python 3.5 on macOS-latest
Details
Python 3.6 on macOS-latest
Details
Python 3.7 on macOS-latest
Details
Python 3.8 on macOS-latest
Details
Python 3.5 on windows-2016
Details
Python 3.6 on windows-2016
Details
Python 3.7 on windows-2016
Details
Python 3.8 on windows-2016
Details
Python 3.x on ubuntu-latest
Details
Python 3.x on macOS-latest
Details
Python 3.x on windows-2016
Details
Flake8
Details
Black
Details
CodeQL No new alerts
Details
@glx22 glx22 deleted the glx22:regression branch Jan 21, 2021
@matthijskooijman
Copy link
Contributor

@matthijskooijman matthijskooijman commented Feb 4, 2021

This fix is a bit problematic for the Debian package: We don't have the luxury of using an older pillow version, so this bug now breaks building in unstable, which is a release-critical bug.

Any plans to also fix the PCX images? Would that be a matter of re-saving them with a program that saves them properly?

@glx22
Copy link
Contributor Author

@glx22 glx22 commented Feb 4, 2021

As said in PR on pillow,

Unfortunately, both Gimp and ImageMagick
both write images with odd strides

So the solution would be my first idea, ie resize the images.

@FLHerne
Copy link
Contributor

@FLHerne FLHerne commented Feb 5, 2021

"Fixing" the tests isn't really a full solution -- many grfs are made using GIMP, so this issue is likely to hurt actual users. The test images are typical of grf source files in the wild and indeed taken from real projects.

@Wormnest
Copy link

@Wormnest Wormnest commented Feb 5, 2021

Can you open an issue on GIMP's issue tracker linking this then I'll try to look at what we can do on our end.

@glx22
Copy link
Contributor Author

@glx22 glx22 commented Feb 5, 2021

In the issue I opened on pillow there's a link to pcx spec, and it seems many tools don't follow BytesPerLine description.

@frosch123
Copy link
Member

@frosch123 frosch123 commented Feb 5, 2021

"Fixing" the tests isn't really a full solution -- many grfs are made using GIMP, so this issue is likely to hurt actual users. The test images are typical of grf source files in the wild and indeed taken from real projects.

Do NewGRF people still use PCX? I would think everyone learnt about PNG by now.

@Wormnest
Copy link

@Wormnest Wormnest commented Feb 5, 2021

Actually this was already fixed in GIMP 2 years ago, so if you have a recent enough GIMP it should not save with odd values of BytesPerLine anymore. Meaning you can just re export the images, resizing is not necessary.

See: https://gitlab.gnome.org/GNOME/gimp/-/issues/2160

@glx22
Copy link
Contributor Author

@glx22 glx22 commented Feb 5, 2021

Indeed re exporting files with GIMP seem to be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants