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

Dynamically link FriBiDi instead of Raqm #5062

Merged
merged 22 commits into from Mar 27, 2021
Merged

Conversation

@nulano
Copy link
Contributor

@nulano nulano commented Nov 25, 2020

This is likely to fix issues #3066 and #4225.

Includes Raqm and a FriBiDi shim in src/thirdparty. Raqm is modified to use the shim and for compatibility with C89 (edit: reverted because Raqm is no longer built by default).

I've checked that Raqm/FriBiDi/HarfBuzz are being loaded correctly in all of the Windows, Ubuntu, and Docker GHA jobs.
I'm not sure what is missing to get HarfBuzz detected on macOS edit: merging master fixed this somehow. Travis is failing to compile Raqm because it has HarfBuzz 1.0.1 which is too old edit: disabled dynamic loading by default.

Is this the way forward to resolving the Raqm/FreeType crashes? I haven't tried building wheels to see if the issues are actually resolved.

This effectively removes dynamic loading as the default option. It now needs to be explicitly enabled with --vendor-raqm --vendor-fribidi, e.g. when building wheels. A side effect of these changes is that Windows users now need fribidi.dll instead of libraqm.dll.

TODO:

  • I intended to add a detailed message for which Raqm version is selected in setup.py.
  • PR in pillow-wheels adding HarfBuzz and enabling dynamic linking. python-pillow/pillow-wheels#181
@wiredfool
Copy link
Member

@wiredfool wiredfool commented Jan 2, 2021

@nulano is there a way that this can go into the current release?

setup.py Outdated Show resolved Hide resolved
setup.py Outdated
_dbg("--enable-raqm implies --enable-freetype")
self.feature.required.add("freetype")
for x in ("raqm", "fribidi"):
if getattr(self, f"system_{x}"):
Copy link
Contributor Author

@nulano nulano Jan 2, 2021

This PR is currently set up to default to building vendored libraries. Since this is not fully functional yet (e.g. macOS), I think the best way to get this into the release would be to invert this for now (perhaps using --_vendored-...) and re-evaluate later. Thoughts?

@nulano
Copy link
Contributor Author

@nulano nulano commented Jan 2, 2021

I've triggered a run of pillow-wheels on this branch to make sure this actually fixes the issue: https://github.com/nulano/pillow-wheels/actions/runs/457534488

@wiredfool
Copy link
Member

@wiredfool wiredfool commented Jan 2, 2021

We're getting seemingly unrelated errors with a bunch of the runs on that pillow-wheels push. It looks like that might be a tiff 4.2.0 issue?

@radarhere
Copy link
Member

@radarhere radarhere commented Jan 2, 2021

If you're talking about 'RuntimeError: Error setting from dictionary', then that should have been fixed by 6b21a96

@nulano
Copy link
Contributor Author

@nulano nulano commented Jan 2, 2021

We're getting seemingly unrelated errors with a bunch of the runs on that pillow-wheels push. It looks like that might be a tiff 4.2.0 issue?

If you're talking about 'RuntimeError: Error setting from dictionary', then that should have been fixed by 6b21a96

I changed the repo url to my fork which has a slightly outdated master branch.

@wiredfool
Copy link
Member

@wiredfool wiredfool commented Jan 2, 2021

Ah, ok.

@nulano
Copy link
Contributor Author

@nulano nulano commented Jan 2, 2021

I think this is (mostly) ready for review, but it will require a second PR in pillow-wheels, and I'm having trouble getting HarfBuzz to compile there.

@wiredfool
Copy link
Member

@wiredfool wiredfool commented Jan 2, 2021

These wheels are looking good against my earlier repro. (3.8-manylinux1 x86_64) Valgrind is giving a clean report to 10k iterations, where it used to fail quickly.

@nulano
Copy link
Contributor Author

@nulano nulano commented Jan 2, 2021

These wheels are looking good against my earlier repro. (3.8-manylinux1 x86_64) Valgrind is giving a clean report to 10k iterations, where it used to fail quickly.

If you used the wheels from my link above, I'm pretty sure they are missing HarfBuzz and hence also Raqm:

... will require a second PR in pillow-wheels, and I'm having trouble getting HarfBuzz to compile there.

@wiredfool
Copy link
Member

@wiredfool wiredfool commented Jan 2, 2021

ah, not a good test then.

@nulano

This comment has been hidden.

@nulano
Copy link
Contributor Author

@nulano nulano commented Jan 2, 2021

I finally got it to compile! (tests failed because it detected Raqm which was not on the expected feature list)

@nulano
Copy link
Contributor Author

@nulano nulano commented Jan 2, 2021

I've tested the wheels and they do appear to have Raqm support, and I can no longer reproduce a crash with 50k iterations.

I'll open a PR in pillow-wheels shortly. Edit: python-pillow/pillow-wheels#181

@nulano nulano marked this pull request as ready for review Jan 2, 2021
@wiredfool
Copy link
Member

@wiredfool wiredfool commented Jan 3, 2021

Looks good to me:

--------------------------------------------------------------------
Pillow 8.1.0.dev0
Python 3.8.5 (default, Jul 28 2020, 12:59:40)
       [GCC 9.3.0]
--------------------------------------------------------------------
Python modules loaded from /home/ubuntu/vpy38-dbg/lib/python3.8/site-packages/PIL
Binary modules loaded from /home/ubuntu/vpy38-dbg/lib/python3.8/site-packages/PIL
--------------------------------------------------------------------
--- PIL CORE support ok, compiled for 8.1.0.dev0
--- TKINTER support ok
--- FREETYPE2 support ok, loaded 2.10.4
--- LITTLECMS2 support ok, loaded 2.10
--- WEBP support ok, loaded 1.1.0
--- WEBP Transparency support ok
--- WEBPMUX support ok
--- WEBP Animation support ok
--- JPEG support ok, compiled for 9.0
--- OPENJPEG (JPEG2000) support ok, loaded 2.4.0
--- ZLIB (PNG/ZIP) support ok, loaded 1.2.11
--- LIBTIFF support ok, loaded 4.2.0
--- RAQM (Bidirectional Text) support ok, loaded 0.7.1, fribidi 1.0.8, harfbuzz 2.7.4
*** LIBIMAGEQUANT (Quantization method) support not installed
--- XCB (X protocol) support ok
--------------------------------------------------------------------
Running selftest:
--- 58 tests passed.

All ordinary tests pass, as well as the valgrind run against the test case in #4225. As a comparison, the valgrind case still fails against the shipped 8.1.0 binaries.

setup.py Outdated Show resolved Hide resolved
@wiredfool
Copy link
Member

@wiredfool wiredfool commented Mar 15, 2021

Does this need to merge before the wheels PR goes in? Is there anything that needs to happen on this?

@nulano
Copy link
Contributor Author

@nulano nulano commented Mar 25, 2021

Does this need to merge before the wheels PR goes in? Is there anything that needs to happen on this?

Missed the GitHub notification for this...

This PR needs to be merged before the wheels PR to make that one pass.

Looking over the changes again I noticed and fixed a few small issues, but I don't think there is anything else needed (other than release notes).

@wiredfool
Copy link
Member

@wiredfool wiredfool commented Mar 27, 2021

OK, Merging.

Still need release notes: #5287

@wiredfool wiredfool merged commit 3addd7d into python-pillow:master Mar 27, 2021
51 of 53 checks passed
@nulano
Copy link
Contributor Author

@nulano nulano commented Mar 29, 2021

OK, Merging.

Still need release notes: #5287

In #5365.

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

3 participants