Bug 63771

What's the change you need in Pango?

Improvements / bug fix to the Pango CoreText shape module to properly handle UCS4. I have finished & working patches sitting on the disk of my other laptop, I simply want to give it another round of testing and then commit it.

Comment on attachment 78291 [details] [review]
Use CTFontDrawGlyphsPtr() when available, so that colored bitmap glyphs are rendered correctly. Based on original patch authored by Jonathan Kew.

Review of attachment 78291 [details] [review]:
-----------------------------------------------------------------

The patch does not regress the results of the testsuite (tested on MacOS X 10.7).

::: src/cairo-quartz-private.h
@@ +100,5 @@
>  cairo_private CGFontRef
>  _cairo_quartz_scaled_font_get_cg_font_ref (cairo_scaled_font_t *sfont);
>  
> +CTFontRef
> +_cairo_quartz_scaled_font_get_ct_font_ref (cairo_scaled_font_t *sfont);

This function should probably be declared cairo_private.

Created attachment 78520 [details] [review]
Add cairo_private qualifier

FWIW, this has been shipped to a large userbase (Mono / Xamarin Studio) for several months without any reported issue.

Created attachment 118089 [details] [review]
Updated patch against master as of September 5, 2015

Updated the patch against today's master.

Compared to the previous version, added a compile time assert to ensure CGPoint <= CGSize. It is perhaps not pretty to share the memory between cg_advances and cg_positions, but the alternative is either to allocate twice as much memory or to move the declaration into if branches (in which case perhaps the CAIRO_STACK_ARRAY foo cannot be used?). Please let me know what is preferred within Cairo and I can adapt the code if necessary.


Is there anything else I can do to get this patch accepted? We have a depending bug in Pango Bugzilla (https://bugzilla.gnome.org/show_bug.cgi?id=753312), so I would love to get it committed :) Thanks!

Created attachment 118099 [details] [review]
Minimized patch

I can confirm that the patch does not regress the test-suite.

I tried to further simplify the patch and integrate it with the changes that already affected the Quartz backend (specifically, the bump of the minimum version to 10.5, which removes the need to define CTFont and to perform the manual lookup for some symbols).
Can you confirm that the reduced patch works for you?

The 1.14.4 release should be coming soon, so I'm unsure if it is a good idea to merge it right now. Merging it immediately after the release would allow some time to catch regressions, but I'm afraid that most cairo-quartz users depend on releases, so bugs might go undetected until it is included in a release.
Bryce, what do you think is the best route here?

How hard would it be to add a test for this?
I'll try to write one starting from the code example in the Pango bug report.

(In reply to Andrea Canciani from comment #8)
> How hard would it be to add a test for this?
> I'll try to write one starting from the code example in the Pango bug report.

Should be easy, simply render a character string containing Unicode characters which are rendered from the Apple Color Emoji font. For example 0x1f600, 0x1f601, 0x1f602, 0x1f630.


Will test your minimized patch tonight or tomorrow.

Just tested: your minimized patch still works.

I was wondering how this interacts with the fallback infrastructure...

We might need to perform the corresponding change (CGContextShowGlyphsWithAdvances/CGContextShowGlyphsAtPoint -> CTFontDrawGlyphs) in _cairo_quartz_init_glyph_surface(), but it currently returns A8 surfaces.
I do not think that the changes needed to support colors in _cairo_quartz_init_glyph_path() would be trivial.

AFAICT the current infrastructure does not really expect multi-color glyphs.
Behdad might be able to provide some more insight and directions about what is the best way forward here.

Ideally this should be merged with Matthias Clasen's color-fonts branch [0], so we get color even on other surfaces.

[0] https://github.com/matthiasclasen/cairo/commits/matthiasc/color-emoji-5

This bug has been discussed in an issue in the node-canvas project: https://github.com/Automattic/node-canvas/issues/760.

We merged Kristian's updated patch into https://github.com/matthiasclasen/cairo/tree/matthiasc/emoji-5 and building cairo (which is used by node-canvas). pango@1.40.3 was also installed. Unfortunately, this build wasn't able to render emoji's.

We would appreciate any advice on properly using this patch. Thanks!

A problem in the handling of UCS4 characters in the Quartz font backend has recently been reported on the mailing list: https://lists.cairographics.org/archives/cairo/2017-March/027903.html

The patch attached in this report did not make it possible to show emoji because of that issue, but the combined patches correctly render emojis on my system.

This makes me much more confident about this patch, although a little surprised that it did work for somebody without the non-BMP fix.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/54.