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 libraqm #2753

Merged
merged 10 commits into from
Dec 28, 2017
Merged

Conversation

wiredfool
Copy link
Member

@wiredfool wiredfool commented Sep 22, 2017

We can't currently distribute a binary of Pillow with support for raqm due to license incompatibility, because while libraqm is MIT licensed, it links to GPL libraries. Unfortunately, this means that nearly no one gets support for complex text, as we're providing binaries on the big three platforms now.

This PR replaces the build dependency on libraqm with an attempt to dynamically load the dll, should it be installed by the user. To do this, we're shipping the header from the libraqm project (which is MIT licensed), using this for types, and then dynamically resolving the function addresses at runtime.

To Do:

  • fix fedora
  • fix osx
  • test on windows
  • check freebsd
  • works on my machine
  • release notes,
  • instructions

_imagingft.c Outdated
@@ -75,6 +78,32 @@ typedef struct {

static PyTypeObject Font_Type;

typedef struct {
void* raqm;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tabs to spaces

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'm on a new vm and the emacs isn't setup right yet.

p_raqm.raqm = NULL;

/* Microsoft needs a totally different system */
#if !defined(_MSC_VER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of:
#if !something
a
#else
b
#endif

How about this?
#if something
b
#else
a
#endif

Copy link
Member Author

@wiredfool wiredfool Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows is the weird one. Everyone else follows the posix path, even people on msys2.

(and frankly, unless I can actually test with windows, I'm potentially going to drop the second clause. I was ok with shipping code if it was compiled and staticly linked, but doing it at runtime has a much larger chance of doing something bad)

((and to test with windows, I'm going to have to find precompiled raqm, harfbuzz and fribidi, which is reported to be tricky))

return 1;
}

#if !defined(_MSC_VER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here

@wiredfool
Copy link
Member Author

wiredfool commented Sep 23, 2017

Windows: Can't just assign the pointers, need to cast them from FARPROC to a typedef'd version of the function pointer. Annoying but fixable.

_imagingft.c(170) : error C2440: '=' : cannot convert from 'FARPROC' to 'raqm_t *(__cdecl *)(void)'
_imagingft.c(171) : error C2440: '=' : cannot convert from 'FARPROC' to 'int (__cdecl *)(raqm_t *,const uint32_t *,size_t)'
...

Fedora is interesting, it's running but I'm getting corruption from one of the function calls. I'm getting what look like totally reasonable pointers to the functions, but I'm getting some garbage back from raqm_get_glpyhs.

Breakpoint 3, text_layout_raqm (string=0x7f706ceaeb70, self=0x7f7066736f80, dir=0x0, features=0x7f706dff05e0 <_Py_NoneStruct>, 
    glyph_info=0x7ffd534ab140, mask=0) at _imagingft.c:445
445	        (*glyph_info)[i].x_advance = glyphs[i].x_advance;
(gdb) l 
440	    }
441	
442	    for (i = 0; i < count; i++) {
443	        (*glyph_info)[i].index = glyphs[i].index;
444	        (*glyph_info)[i].x_offset = glyphs[i].x_offset;
445	        (*glyph_info)[i].x_advance = glyphs[i].x_advance;
446	        (*glyph_info)[i].y_offset = glyphs[i].y_offset;
447	        (*glyph_info)[i].cluster = glyphs[i].cluster;
448	    }
449	
(gdb) p glyphs[i]
$48 = {index = 570483616, x_advance = -632536784, y_advance = 1766808632, x_offset = 32624, y_offset = 1718968400, cluster = 32624, 
  ftface = 0xfb0ebe7441c223a1}

@wiredfool
Copy link
Member Author

Ok, so Fedora's (old) version had a different definition of the raqm_glyph_t structure, which messed up the function returning an array of them. Windows is compiling now, but it's untested as to if it works or not when raqm is actually installed.

Leaving aside the (current) messy code duplication and lack of having run on Windows, This is now in the state where it's been shown that it's possible to do this. We now need to ask the question: Is this a good idea?

On the plus side, it means that we can distribute support for raqm in the binaries, and people will (hopefully) be able to add libraqm and the dependencies and get text rendering that works for complex languages. On the downside, we're more sensitive to releases of raqm changing the interface on us, and we're opening up a new can of support worms about finding the libraries at runtime. We've not done a dlopen/LoadLibrary before, and I'm guessing that there's going to be at least a few support requests here.

@wiredfool wiredfool added this to the 4.4.0 milestone Sep 29, 2017
@khaledhosny
Copy link

The GPL issue is with FriBiDi, right? Would it be any help if we used a bidi implementation with a non-copyleft license?

@wiredfool
Copy link
Member Author

Maybe, but we've tested this set of libraries. We're not going to do it in this release, so I'll revisit it in a few days. I'm thinking that some sort of versioning symbol would be useful for making sure that we're not interpreting the return value incorrectly.

@wiredfool
Copy link
Member Author

We've now got equivalent support for libraqm on windows as before -- unsupported but may work. I've tested on freebsd, and we're good there.

I think this is a good approach to getting this code distributed in a manner that is license compatible, and should be included in the next release. @homm Comments?

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.

None yet

3 participants