updated version of Add complex text support #1682 #2284

Fahad-Alsaidi · 2016-12-11T10:11:05Z

This pull request adds support for languages that require complex text layout.

We are using the Raqm library, that wraps FriBidi (for bidirectional
text support) and HarfBuzz (for text shaping), and does proper BiDi and script
itemization:
https://github.com/HOST-Oman/libraqm

This should fix #1089, #2255

@andreymal

khaledhosny · 2016-12-11T12:03:20Z

.travis.yml

  - 3.4
  - nightly

+sudo: required # needed for trusty beta


I don’t think this is needed anymore, see https://docs.travis-ci.com/user/trusty-ci-environment/.

Fahad-Alsaidi · 2016-12-13T09:58:54Z

Finally, passed all tests. I welcome any feedback.

hugovk

Thanks for getting this all passing!

It might be good to squash some of these later commits.

I don't think it's a problem, especially as 12.04 is at end-of-life next April, but it's worth noting this swaps Precise (12.04.5 LTS) for Trusty (14.04.5 LTS).

Do we need to mention the Noto fonts' licence if we include NotoNastaliqUrdu-Regular.ttf in the test suite? Details on the SIL Open Font License (OFL) v1 here: https://github.com/googlei18n/noto-fonts

hugovk · 2016-12-13T11:28:44Z

.travis.yml

 # Then run the remainder.
 python:
-  - "pypy"
+  - "pypy-5.3.1"


Please could you include a comment to say why PyPy is pinned to 5.3.1?

Since switching to Trusty, the default version of PyPy in Trusty is PyPy 5.4.1. There is a known error in it: TypeError: PyPy does not yet implement the new buffer interface (issue #2163).

@Fahad-Alsaidi That looks good (I've edited it slightly). Please could you add it as a comment in .travis.yml, above - "pypy-5.3.1"?

If we've got one test that is failing on a specific version of pypy, we should document that at the test and skip it for just that version. We've done it before, and recently.

@wiredfool but the test not related to adding CTL support. Is it fine to edit that test or you may edit later?

That can be a separate PR, and note that it has to be merged prior to this one.

@wiredfool done

Fixed via PR #2294.

hugovk · 2016-12-13T11:34:30Z

Tests/test_imagefontctl.py

+except (KeyError, ImportError):
+    class TestImagecomplextext(PillowTestCase):
+        def test_skip(self):
+            self.skipTest("KeyError")


Let's have a more descriptive skip message (because it could be ImportError rather than KeyError).

hugovk · 2016-12-13T11:45:29Z

setup.py

 FREETYPE_ROOT = None
 LCMS_ROOT = None

+RAQM_ROOT = None


Can delete the empty newline above this to bring these all together.

hugovk · 2016-12-13T11:46:13Z

setup.py


+        if feature.want('raqm'):
+            if _find_include_file(self, "raqm.h"):
+                if _find_library_file(self, "raqm") and \


Can these two if be a single if with an extra and?

hugovk · 2016-12-13T11:54:18Z

_imagingft.c

            yoffset = face->glyph->metrics.horiBearingY;

-        last_index = index;
+        //last_index = index;


If this line isn't needed, delete it.

hugovk · 2016-12-13T12:00:48Z

_imagingft.c

+
+    count = 0;
+    while (font_getchar(string, count, &ch))
+       count++;


Add brackets for while.

hugovk · 2016-12-13T12:01:56Z

_imagingft.c

+
+    load_flags = FT_LOAD_RENDER|FT_LOAD_NO_BITMAP;
+    if (mask)
+        load_flags |= FT_LOAD_TARGET_MONO;


Add brackets for if.

hugovk · 2016-12-13T12:02:58Z

_imagingft.c


+static size_t
+text_layout(PyObject* string, FontObject* self, const char* dir,
+            PyObject *features ,GlyphInfo **glyph_info, int mask);


Space after comma, not before.

hugovk · 2016-12-13T12:13:18Z

_imagingft.c

+    glyph_info = NULL;
+    count = text_layout(string, self, dir, features, &glyph_info, mask);
+    if (count == 0)
        return NULL;


Add brackets for if.

hugovk · 2016-12-13T12:14:03Z

_imagingft.c

+    PyObject *features = NULL;
+
+    if (!PyArg_ParseTuple(args, "On|izO:render", &string,  &id, &mask, &dir, &features))
        return NULL;


Add brackets for if.

wiredfool · 2016-12-13T12:45:50Z

I've done a quick review of this PR. In general, this covers most of the issues that I had with the initial PR.

I'm still concerned with the dependency on the not well known library raqm, especially on platforms other than Ubuntu Trusty.

Documentation is still too sparse:
- There are three new libraries, only one is mentioned.
- The parameter values for direction and features are unspecified.
depends/* needs to be touched up and tested where possible.
I think that the text_layout function should be broken up into two implementations, text_layout_raqm and text_layout_[something else] and use a #ifdef in text_layout to delegate to one or the other. Furthermore, I think splitting out text_layout_raqm to a separate source file makes sense to minimize the implementation specific changes to _imagingft.
There are a lot of places where you've got PY_VERSION_HEX #ifdefs. Check to see if the definitions in py3.h cover the use cases. If they don't, maybe we should look at that.

khaledhosny · 2016-12-13T20:45:26Z

I'm still concerned with the dependency on the not well known library raqm, especially on platforms other than Ubuntu Trusty.

The alternative would be to roll your own solution (not essentially a bad thing, but more code for your to maintain) or depend on large package with even larger dependencies like Pango (again not essentially bad, if you can afford Pango then it is indeed the more mature and featureful choice).

Fahad-Alsaidi · 2016-12-14T08:03:23Z

@wiredfool

There are three new libraries, only one is mentioned.

what is the right place to mention them?

The parameter values for direction and features are unspecified.

I hope now they are clear, if not please give me a guideline.

depends/* needs to be touched up and tested where possible.

I update all files but need testing at least in FreeBSD & Fedora. BTW, Fedora have already a package for libraqm.

I think that the text_layout function should be broken up into two implementations, text_layout_raqm and text_layout_[something else] and use a #ifdef in text_layout to delegate to one or the other. Furthermore, I think splitting out text_layout_raqm to a separate source file makes sense to minimize the implementation specific changes to _imagingft.

I spite text_layout to two implementations as you request but I don't think it is needed to put it to a separate file.

There are a lot of places where you've got PY_VERSION_HEX #ifdefs. Check to see if the definitions in py3.h cover the use cases. If they don't, maybe we should look at that.

I really need help in this point.

@hugovk I did all modifications as requested. Please review.

wiredfool · 2016-12-14T16:06:26Z

.travis.yml

 # Then run the remainder.
 python:
-  - "pypy"
+  - "pypy-5.3.1"


That can be a separate PR, and note that it has to be merged prior to this one.

wiredfool · 2016-12-14T16:07:26Z

PIL/features.py

    "tkinter": "PIL._imagingtk",
    "freetype2": "PIL._imagingft",
    "littlecms2": "PIL._imagingcms",
+    "raqm": "PIL._imagingft",


This is going to trigger on raqm being available when it's not compiled in but freetype is.

when I delete it, all builds fails the error is

Traceback (most recent call last): File "selftest.py", line 184, in <module> supported = features.check_module(name) File "build\bdist.win-amd64\egg\PIL\features.py", line 15, in check_module ValueError: Unknown module raqm

wiredfool · 2016-12-14T16:08:27Z

Tests/test_imagefontctl.py

+    from PIL import ImageFont
+
+    # check if raqm is available
+    ttf = ImageFont.truetype(FONT_PATH, FONT_SIZE)


There should be a way to check if raqm is installed. As it stands, if there is an error that triggers an import error on ImageFont, then this whole set of tests are going to get skipped.

There are version flags for several of the libraries, that would be something to add to _imagingft.c, potentially for all three libraries, but at least raqm.

wiredfool · 2016-12-14T16:09:01Z

_imagingft.c

+  RAQM_DIRECTION_LTR,
+  RAQM_DIRECTION_TTB
+} raqm_direction_t;
+#endif


It doesn't look like this is ever used outside of the HAVE_RAQM #ifdef

wiredfool · 2016-12-14T16:10:25Z

_imagingft.c

-    if (!PyArg_ParseTuple(args, "O:getsize", &string))
-        return NULL;
-
+    if (features != Py_None || dir != NULL) {


This caveat needs to make it into the documentation

wiredfool · 2016-12-14T16:15:20Z

depends/ubuntu_14.04.sh

 #
-
+sudo add-apt-repository -y ppa:as-bahanta/raqm
+sudo apt-get update


This is extraneous with the ./install_raqm line below.

This is needed it to install a required harfbuzz version for raqm.

It doesn't work with the version in trusty?

wiredfool · 2016-12-14T16:16:00Z

depends/install_raqm.sh

+
+
+if [ ! -f raqm-0.2.0.tar.gz ]; then
+    wget -O 'raqm-0.2.0.tar.gz' 'https://github.com/HOST-Oman/libraqm/releases/download/v0.2.0/raqm-0.2.0.tar.gz?raw=true'


Should add a PR to add this to our depends repo.

PR created: python-pillow/pillow-depends#17

wiredfool · 2016-12-14T16:17:22Z

docs/installation.rst

    so it is unlikely to work with any Python prior to 3.5 on Windows.

+* **libraqm** provides complex text layout support.
+


Need to mention the other libraries. Also, below there are build flags that need to be updated.

wiredfool · 2016-12-14T16:18:13Z

docs/reference/ImageDraw.rst

                    the number of pixels between lines.
    :param align: If the text is passed on to multiline_text(),
                  "left", "center" or "right".
+    :param direction: Direction of the text. It can be  'rtl', 'ltr', 'ttb' or 'btt'.


Needs a Requires raqm note here, and all similar locations.

wiredfool · 2016-12-14T16:20:26Z

setup.py

 class pil_build_ext(build_ext):
    class feature:
-        features = ['zlib', 'jpeg', 'tiff', 'freetype', 'lcms', 'webp',
+        features = ['zlib', 'jpeg', 'tiff', 'freetype', 'raqm', 'lcms', 'webp',


need to filter and error if raqm is requested and freetype is either not found or disabled.

wiredfool · 2016-12-14T16:23:11Z

Libraries should be mentioned in docs/installation.rst. It should be clear from the discussion there what libraries we rely on, as well as filling in the examples that are there. There are also the build flags that need to be updated.

Do you know what the status of the dependencies are on Windows and OSX?

wiredfool · 2016-12-14T16:35:54Z

@hugovk We do need to mention the licence for the font somewhere.

homm · 2016-12-14T21:53:48Z

PIL/ImageDraw.py

-            return self.multiline_text(xy, text, fill, font, anchor,
-                                       *args, **kwargs)
-
+            return self.multiline_text(xy, text, fill, font, anchor, *args, **kwargs)


From https://github.com/HOST-Oman/libraqm/releases/download/v0.2.0/raqm-0.2.0.tar.gz?raw=true See python-pillow/Pillow#2284 / https://github.com/python-pillow/Pillow/pull/2284/files#r92428379

This pull request adds support for languages that require complex text layout. We are using the Raqm library, that wraps FriBidi (for bidirectional text support) and HarfBuzz (for text shaping), and does proper BiDi and script itemization: https://github.com/HOST-Oman/libraqm This should fix python-pillow#1089.

so the test not fail when there is no raqm lib.

wiredfool · 2017-07-01T09:57:58Z

Closed, merged with #2576

Thanks to everyone who worked on this, I think it's a valuable addition to the library.

khaledhosny reviewed Dec 11, 2016

View reviewed changes

Fahad-Alsaidi force-pushed the CTL branch 2 times, most recently from 38969d5 to d5b8434 Compare December 13, 2016 07:01

Fahad-Alsaidi force-pushed the CTL branch from 733b831 to 6a70181 Compare December 13, 2016 10:36

hugovk added the Needs Review label Dec 13, 2016

hugovk reviewed Dec 13, 2016

View reviewed changes

wiredfool reviewed Dec 14, 2016

View reviewed changes

homm reviewed Dec 14, 2016

View reviewed changes

Fahad-Alsaidi mentioned this pull request Dec 15, 2016

switch to ubuntu trusty 14.04 #2289

Closed

hugovk mentioned this pull request Dec 19, 2016

Dependency for complex text support python-pillow/pillow-depends#17

Merged

ShamsaHamed and others added 13 commits December 21, 2016 13:51

Fix failure in test_imagefont.py

ff5c2ca

Fix build with python 3.2

f013dd1

fallback code

8b3ed96

Document complex text layout features.

228af63

Testing complex text layout.

25b2a64

Solve merge conflict

95a2ac1

Fix gitsize function.

c6d5de6

fix testing

3130bce

fix windows build

1b7401a

fix windows build

af0df0c

fix setup.py

f2ba268

improve docs for CTL

37e2588

Fahad-Alsaidi added 9 commits December 21, 2016 13:54

break up text_layout into two implementations

ebdc297

update depends/* to install raqm

629522a

only catch ImportError same as test_imagefont.py

22d2a5e

declare text_layout_raqm only when we HAVE_RAQM

b18ead3

adding KeyError exception

5833779

so the test not fail when there is no raqm lib.

update

4b8d884

fix build

5891d23

fix ubuntu build

8739e53

fedora has raqm package

8144eb4

Fahad-Alsaidi force-pushed the CTL branch from 8ad1b3f to 8144eb4 Compare December 21, 2016 09:56

radarhere added the Needs Rebase label Jan 4, 2017

khaledhosny mentioned this pull request May 16, 2017

Devanagari font not working #2255

Closed

wiredfool mentioned this pull request May 27, 2017

Features detection #2520

Merged

This was referenced Jun 9, 2017

How to render stacked Tibetan letters #2572

Closed

Update of Pr #2284, Complex Text Support #2576

Merged

wiredfool closed this Jul 1, 2017

amitdo mentioned this pull request Jul 13, 2017

Advanced Font rendering for ocropus-linegen ocropus-archive/DUP-ocropy#198

Open

nulano mentioned this pull request Apr 21, 2020

Font spacing changes #3977

Closed



		if [ ! -f raqm-0.2.0.tar.gz ]; then
		wget -O 'raqm-0.2.0.tar.gz' 'https://github.com/HOST-Oman/libraqm/releases/download/v0.2.0/raqm-0.2.0.tar.gz?raw=true'

		so it is unlikely to work with any Python prior to 3.5 on Windows.

		* libraqm provides complex text layout support.

Uh oh!

updated version of Add complex text support #1682 #2284

updated version of Add complex text support #1682 #2284

Uh oh!

Conversation

Fahad-Alsaidi commented Dec 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fahad-Alsaidi commented Dec 13, 2016

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fahad-Alsaidi Dec 14, 2016 • edited by hugovk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiredfool commented Dec 13, 2016

Uh oh!

khaledhosny commented Dec 13, 2016

Uh oh!

Fahad-Alsaidi commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fahad-Alsaidi Dec 14, 2016 •

edited by hugovk

Loading