Bug 84324

Looks good to me.  Anyone can commit this please?

(In reply to Behdad Esfahbod from comment #1)
> Looks good to me.  Anyone can commit this please?

I raised some concerns about the patch in the discussion on the mailing list.
Are we ok with dropping support for 10.4? (This would also enable some more cleanup in quartz, but at the very least we should mention it)

Should we go for a CoreText font backend?

For a handle to the thread in the mailing list, see http://lists.cairographics.org/archives/cairo/2014-July/025348.html

(In reply to Andrea Canciani from comment #2)
> (In reply to Behdad Esfahbod from comment #1)
> > Looks good to me.  Anyone can commit this please?
> 
> I raised some concerns about the patch in the discussion on the mailing list.
> Are we ok with dropping support for 10.4? (This would also enable some more
> cleanup in quartz, but at the very least we should mention it)

I wouldn't mind if we don't support 10.5 or 10.6 either.  10.4 is definitely a no-brainer to me.


> Should we go for a CoreText font backend?

Not quite sure.  Only if we have to.  Remove the old ATSU stuff.  Start adding CoreText when it makes sense.  Right now the backend does without CoreText because what we need is very simple to do with CGFont (scale by em-size).  This works because OS X almost fully supports subpixel-positioned text, so there's little hinting to worry about.

> For a handle to the thread in the mailing list, see
> http://lists.cairographics.org/archives/cairo/2014-July/025348.html

Hi, are there any conclusions regarding the introduction of the patch to remove the call to CGFontGetGlyphPath?

We currently have a problem to run pspp on yosemite because of that problem:

https://savannah.gnu.org/bugs/?43469

Here is the bug tracker for macports that discusses the problem with 
CGFontGetGlyphPath and MacOS yosemite. 

https://trac.macports.org/ticket/45599

Here is a bug entry in macports regarding MacOS 10.10 yosemite and the deprecated CGFontGetGlyphPath function.

https://trac.macports.org/ticket/45599

Andrea wants to provide an updated patch which removes 10.4 specific code. 10.4 will not be supported in the future. See cairo mailing list:

http://lists.cairographics.org/archives/cairo/2014-November/025792.html

Created attachment 110157 [details]
iOS project which works on ipad 3 and crashes on ipad 4, both on latest iOS

There seems to be a problem with this patch (at least as far as iOS goes).  It works on ipad 3 but crashes on ipad 4 and later.

I am investigating further, but see attached iOS test project PDFCrashTest.zip if interested (includes cairo and pixman and renders and displays a PDF).  It works on ipad 3 and crashes on ipad 4 (both running ios 8.1.1).

It crashes here (only after drawing text and calling cairo_show_page):

ctFont = CTFontCreateWithGraphicsFontPtr (font_face->cgFont);

with EXC_BAD_ACCESS.  The call stack looks like:

#0	0x234bc750 in CTFontDescriptorCopyAttribute ()
#1	0x234af26c in TFont::SetExtras(__CTFontDescriptor const*) ()
#2	0x234aed12 in TFont::TFont(CGFont*, float, CGAffineTransform const*, __CTFontDescriptor const*) ()
#3	0x234aec06 in CTFontCreateWithGraphicsFont ()
#4	0x000f2c40 in _cairo_quartz_init_glyph_path at /Users/rkdgu/PDFCrashTest/PDFCrashTest/cairo/cairo-quartz-font.c:575
#5	0x000f2736 in _cairo_quartz_scaled_glyph_init at /Users/rkdgu/PDFCrashTest/PDFCrashTest/cairo/cairo-quartz-font.c:748
#6	0x00128044 in _cairo_scaled_glyph_lookup at /Users/rkdgu/PDFCrashTest/PDFCrashTest/cairo/cairo-scaled-font.c:3004
...
#20	0x0023cd28 in cairo_show_page at /Users/rkdgu/PDFCrashTest/PDFCrashTest/cairo/cairo.c:2284

Created attachment 110168 [details]
patch with minor fix

I fixed the crash by calling CTFontCreateWithGraphicsFont with all of its documented parameters.  Strange that it crashes on ipad 4 and not ipad 3.  See attached updated patch.

Created attachment 110634 [details] [review]
Wannabe fix

This could be a fix.
I tried it on Yosemite, but I don't have iOS devices.
Could somebody check if it works?

The main differences from the original and the previously attached patches are:
 - CFRelease() on the CTFont instance (it should be needed to avoid memleaks)
 - no explicit linking of CoreText (AFAICT it might be already pulled in indirectly by CoreGraphics)

Should be fixed by:

commit 70cc8f250b5669e757b4f044571ba0f71e3dea9e
Author: Andrea Canciani <ranma42@gmail.com>
Date:   Tue Dec 9 16:13:00 2014 +0100

    quartz: Remove call to obsolete CGFontGetGlyphPath
    
    CGFontGetGlyphPath was not public and is not available anymore on
    modern OSX/iOS systems. The same functionality is available through
    the CoreText API since OSX 10.5.
    
    Based on a patch by Simon Cozens.
    
    Fixes https://bugs.freedesktop.org/show_bug.cgi?id=84324

This commit breaks canvas text (SPCanvasText) in Inkscape (used e.g. to display snap indicators and measurements (new measure tool) on-canvas) if compiled with the Quartz backend. The messages display as expected if running the same Inkscape builds with a recompiled cairo 1.14.2 which has this commit reverted. Is there anything that cairo clients need to watch out for to adapt to this commit?

Build env:
Mac OS X 10.7.5, dependencies installed via MacPorts (local port tree), tested with Inkscape 0.48.5, 0.91 and recent trunk (r14007).

I can't offer a reduced test case (code), but do have two osx application bundles of Inkscape available - the older one includes cairo 1.14.0, the newer one 1.14.2 - if this would be of any help [1].

Please let me know if you prefer to have a separate report filed for this regression. I'd also be glad for any info I could forward to inkscape devs if canvas text needs to be changed when compiled with GTK+/Quartz [2].

-- 
[1] download links for bundled apps:
with cairo 1.14.0: https://inkscape.org/en/gallery/6542/
with cairo 1.14.2: https://inkscape.org/en/gallery/6796/

Steps to reproduce:
1) draw a rectangle
2) switch to the measure tool, and measure (click+drag) across the rectangle

[2] AFAICT relevant source in inkscape (trunk):
https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/view/head:/src/display/drawing-text.cpp

Created attachment 114401 [details] [review]
Fix font size handling

(In reply to ~suv from comment #13)
> This commit breaks canvas text (SPCanvasText) in Inkscape (used e.g. to
> display snap indicators and measurements (new measure tool) on-canvas) if
> compiled with the Quartz backend. The messages display as expected if
> running the same Inkscape builds with a recompiled cairo 1.14.2 which has
> this commit reverted. Is there anything that cairo clients need to watch out
> for to adapt to this commit?

No, I'm afraid I'm at fault here; I investigated it a bit and it looks like the issue comes from missing handling of the font size. I created the patch I just attached, which in my env it seems to work and fix Inkscape.
Can you confirm if it fixes the issue in your env (when applied on top of 1.14.2)?

Sorry for the breakage :(

> 
> Build env:
> Mac OS X 10.7.5, dependencies installed via MacPorts (local port tree),
> tested with Inkscape 0.48.5, 0.91 and recent trunk (r14007).
> 
> I can't offer a reduced test case (code), but do have two osx application
> bundles of Inkscape available - the older one includes cairo 1.14.0, the
> newer one 1.14.2 - if this would be of any help [1].
> 
> Please let me know if you prefer to have a separate report filed for this
> regression. I'd also be glad for any info I could forward to inkscape devs
> if canvas text needs to be changed when compiled with GTK+/Quartz [2].
> 
> -- 
> [1] download links for bundled apps:
> with cairo 1.14.0: https://inkscape.org/en/gallery/6542/
> with cairo 1.14.2: https://inkscape.org/en/gallery/6796/
> 
> Steps to reproduce:
> 1) draw a rectangle
> 2) switch to the measure tool, and measure (click+drag) across the rectangle
> 
> [2] AFAICT relevant source in inkscape (trunk):
> https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/view/head:/src/
> display/drawing-text.cpp

(In reply to Andrea Canciani from comment #15)
> (...) it looks like the issue comes from missing handling of the font
> size. I created the patch I just attached, which in my env it seems
> to work and fix Inkscape. Can you confirm if it fixes the issue in
> your env (when applied on top of 1.14.2)?

Confirmed - patch from comment #15 applied to cairo 1.14.2 does fix the canvas text rendering in local Inkscape builds too (tested with Inkscape 0.91 and latest trunk r14010).

> Sorry for the breakage :(

No problem - many thanks for looking into this, and providing a fix so promptly!

Thanks, patch applied.

   d5ffe67..8b798c3  master -> master

Created attachment 115677 [details]
Screenshots comparing canvas text in Inkscape

Attaching missing screenshots [1] to illustrate the regression as observed with Inkscape's canvas text and latest stable cairo 1.14.2:
- left: Inkscape 0.91+devel, cairo 1.14.2
- right: Inkscape 0.91+devel, cairo 1.14.2 + patch from comment #14

-- 
[1] http://lists.freedesktop.org/archives/cairo/2015-May/026263.html