Issue 64508 - honour fontconfig autohint and hinting attributes
Summary: honour fontconfig autohint and hinting attributes
Status: CLOSED FIXED
Alias: None
Product: gsl
Classification: Code
Component: code (show other issues)
Version: OOo 2.0.2
Hardware: All Unix, all
: P3 Trivial with 8 votes (vote)
Target Milestone: OOo 3.3
Assignee: stefan.baltzer
QA Contact: issues@gsl
URL:
Keywords:
: 93565 (view as issue list)
Depends on:
Blocks: 104239
  Show dependency tree
 
Reported: 2006-04-19 13:16 UTC by caolanm
Modified: 2010-09-17 12:45 UTC (History)
11 users (show)

See Also:
Issue Type: ENHANCEMENT
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
sample document (7.76 KB, application/vnd.oasis.opendocument.text)
2006-04-19 13:16 UTC, caolanm
no flags Details
how it looks in vanilla OOo, with sample text in gedit for reference (53.12 KB, image/png)
2006-04-19 13:17 UTC, caolanm
no flags Details
patch to implement (21.86 KB, patch)
2006-04-20 11:54 UTC, caolanm
no flags Details | Diff
after the patch is applied (50.65 KB, patch)
2006-04-20 11:54 UTC, caolanm
no flags Details | Diff
better after-patch screenshot (50.65 KB, image/png)
2006-04-20 11:59 UTC, caolanm
no flags Details
new defer hints until we need them patch (39.95 KB, patch)
2006-05-05 11:08 UTC, caolanm
no flags Details | Diff
updated patch (36.91 KB, patch)
2006-05-05 12:58 UTC, caolanm
no flags Details | Diff
update patch for 2.0.4 (36.52 KB, patch)
2006-08-15 12:11 UTC, caolanm
no flags Details | Diff
patch resynced to OOG680 (1.71 KB, patch)
2007-08-21 14:03 UTC, caolanm
no flags Details | Diff
indeed, it was the wrong one, here's the updated one for this issue. (50.36 KB, patch)
2007-08-21 16:40 UTC, caolanm
no flags Details | Diff
updated patch for a post-cairo render world (53.13 KB, patch)
2008-04-21 13:19 UTC, caolanm
no flags Details | Diff
we don't need the subpixel stuff now either (49.70 KB, patch)
2008-05-30 10:39 UTC, caolanm
no flags Details | Diff
just an update to use FcConfigGetCurrent instead of getDefConfig (49.52 KB, patch)
2008-09-19 12:49 UTC, caolanm
no flags Details | Diff
update patch to apply against DEV300_m61 (51.08 KB, patch)
2009-10-09 15:00 UTC, caolanm
no flags Details | Diff
Check more thoroughly before using ptr (918 bytes, patch)
2009-11-26 20:59 UTC, thb
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description caolanm 2006-04-19 13:16:04 UTC
A patch to honour fontconfig autohint and hinting attributes, similiar to the
previous antialiasing and embedded bitmap directives
Comment 1 caolanm 2006-04-19 13:16:47 UTC
Created attachment 35807 [details]
sample document
Comment 2 caolanm 2006-04-19 13:17:55 UTC
Created attachment 35808 [details]
how it looks in vanilla OOo, with sample text in gedit for reference
Comment 3 caolanm 2006-04-20 11:54:01 UTC
Created attachment 35841 [details]
patch to implement
Comment 4 caolanm 2006-04-20 11:54:32 UTC
Created attachment 35842 [details]
after the patch is applied
Comment 5 caolanm 2006-04-20 11:58:20 UTC
1. Check for autohint/hinting/hintstyle from fontconfig and use them if possible
2. see issue 64109, FC_EMBEDDED_BITMAP etc don't seem to be filled in by default
by FcObjectSetBuild, hence the call to FcConfigSubstitute to achieve this
3. mbUseGamma is set to true for Asian fonts, but the comment talks about being
good for making artificial bold fonts. I suspect (hope) that the intention is to
enable this Gammaing feature only for Artificial Bold creation, and not for all
asian fonts.
Comment 6 caolanm 2006-04-20 11:59:02 UTC
Created attachment 35843 [details]
better after-patch screenshot
Comment 7 caolanm 2006-04-20 12:00:32 UTC
cmc->pl: For your consideration
Comment 8 philipp.lohmann 2006-04-24 08:34:32 UTC
I'll check if this default substitution also behaves quadratic; one could hope
that it runs on a smaller set and will not cost that dearly
Comment 9 philipp.lohmann 2006-05-02 13:43:00 UTC
it does not cost as much, but still it costs 1 second on every startup (on my
2000 font system). However this information (hint styling, embedded bitmaps,
etc) is only needed on demand for specific fonts anyway since it does not
influence font selection AFAIK. So perhaps we should not store this information
for every font on startup, but instead find a way to get this information when
needed.

How about moving these enums from FastPrintFontInfo to PrintFontInfo and filling
it in on the fly for specific fonts ?

hdu: what is your opinion ?

cmc: would you change the patch that way ?
Comment 10 caolanm 2006-05-02 20:11:16 UTC
Let me fiddle it around a bit. In theory deferring is the way to go, waiting
until we know the actual desired font-size would allow the fontconfig "feature
when <= fontsize" stuff to kick in.
Comment 11 caolanm 2006-05-05 11:08:34 UTC
Created attachment 36280 [details]
new defer hints until we need them patch
Comment 12 caolanm 2006-05-05 11:11:25 UTC
cmc->pl: how about this ? (applied after vcl57)

Defer until we need to know and also don't put the info in the cache. 

The reason the details weren't filled by fontconfig "properly" for us is
apparently because these settings are overridable per-size so there isn't a
correct answer unless the size is known.
Comment 13 philipp.lohmann 2006-05-05 11:35:36 UTC
I think this is almost what we want; some small things:

- the getFontConfigHints method should be fed with a fontId instead of a
FastPrintFontInfo; you already know exactly which font is involved (even if
there is sadly no exact mapping to fontconfig at the moment); this way you can
remove the mapping stuff in X11SalGraphics::GetFontHints
- X11SalGraphics::GetFontHints does not handle the X font case (in which case it
should not do anything)
- Please add a stub for WinSalGraphics::GetFontHints

I think if you change these minor things, we have a very good solution.
Comment 14 caolanm 2006-05-05 12:58:00 UTC
Created attachment 36282 [details]
updated patch
Comment 15 caolanm 2006-05-05 12:58:26 UTC
how about that
Comment 16 philipp.lohmann 2006-05-05 13:17:47 UTC
pl->hdu: please consider this for integration into vcl59

there needs to be done:
- handle the X font case in X11SalGraphics::GetFontHints
- GetFontHints on SalGraphics should not be fed a fontId; it should be used with
an ImplFontSelectData IMHO
Comment 17 hdu@apache.org 2006-05-08 15:07:38 UTC
The GetFontHints() for WinSalGraphics and PspGraphics returns without having set
rFontHints. It is only implemented for X11SalGraphics::GetFontHints().

X11SalGraphics is the only one that in practice get a GetFontHints() request,
because only a VirtualDevice is ever asked to provide them. Constructing a whole
VirtualDevice and its graphics is expensive, especially when considering that
this vdev is supposed to get into the FreetypeServerFont constructor, which in
some use cases gets called very often.

I suggest to have ImplFontHints constructor call psp's getFontConfigHints()
directly if psp is available. And making sure that it is fast, eventually by
caching older FC results.

Another small question is, what to do with hints for the stretched or squeezed
fonts. In the latest patch the width is used. Is there are reason to use the
width over the height?
Comment 18 caolanm 2006-07-25 15:50:41 UTC
"Is there are reason to use the width over the height?"

I don't really know, but I think that when I looked at cairo, that was what it
seemed to do, I didn't pick width over height for any good reason.
Comment 19 ooo 2006-08-02 12:39:08 UTC
unfortunately, this task won't make it for OOo 2.0.4 due to some resource
constraints. Retargeting to OOo 2.x
Comment 20 caolanm 2006-08-15 12:11:43 UTC
Created attachment 38531 [details]
update patch for 2.0.4
Comment 21 hdu@apache.org 2006-08-22 11:15:14 UTC
Thanks for the patch. There are very many merge conflicts against SRC680m182 though:
  patching file source/glyphs/gcach_ftyp.cxx
  Hunk #1 succeeded at 102 with fuzz 2 (offset -5 lines).
  Hunk #2 succeeded at 834 with fuzz 2 (offset 29 lines).
  Hunk #3 succeeded at 1210 with fuzz 1 (offset 69 lines).
  Hunk #4 succeeded at 1221 (offset 69 lines).
  Hunk #5 FAILED at 1250.
  Hunk #6 FAILED at 1282.
  Hunk #7 FAILED at 1305.
  Hunk #8 succeeded at 1341 with fuzz 2 (offset 75 lines).
  Hunk #9 succeeded at 1391 with fuzz 2 (offset 86 lines).
  Hunk #10 FAILED at 1423.
  Hunk #11 FAILED at 1446.
  Hunk #12 succeeded at 1549 with fuzz 2 (offset 126 lines).
  Hunk #13 succeeded at 1601 with fuzz 2 (offset 137 lines).
  Hunk #14 FAILED at 1629.
  Hunk #15 succeeded at 1713 with fuzz 1 (offset 173 lines).
  Hunk #16 FAILED at 2291.
  7 out of 16 hunks FAILED -- saving rejects to file 
source/glyphs/gcach_ftyp.cxx.rej
  patching file source/glyphs/gcach_ftyp.hxx
  Hunk #1 succeeded at 227 (offset 9 lines).
  patching file source/glyphs/glyphcache.cxx
  Hunk #1 FAILED at 89.
1 out of 1 hunk FAILED -- saving rejects to file source/glyphs/glyphcache.cxx.rej
Comment 22 hdu@apache.org 2006-08-22 12:25:37 UTC
For the psprint part of the patch I'll ask PL to have a closer look. A change in
this area caused a bad showstopper (issue 65884) not long ago which was
triggered by issue 61841.
Comment 23 hdu@apache.org 2006-09-25 13:45:12 UTC
changing target...
Comment 24 hdu@apache.org 2006-10-27 16:02:30 UTC
We really should do this but not for this target. With code freeze for 2.1
coming soon and open issues of the patch like
- impact on platforms without or with old fontconfig libraries
- impact on systems with fonts without good fontconfig configurations
- performance impact of creating+destroying a VDev per font face
I'm not comfortable with just merging the patch in yet.
Comment 25 rabauke 2006-12-08 18:59:15 UTC
Regarding the problems that could arise when using the system's settings rather
than OO's: At the moment there is a setting in OO "use system fonts". Could this
not expanded to "use system settings for fonts"?

This would allow OO to respect the system's settings, if they work better than
OO's for the user.
Comment 26 pavel 2007-01-23 07:10:57 UTC
hdu: will you make it into 2.2 or should/could this wait for 2.3?
Comment 27 hdu@apache.org 2007-01-23 09:44:48 UTC
Sorry, not high enough priority to push it into 2.2
Comment 28 philipp.lohmann 2007-05-31 09:51:16 UTC
setting from patch to enhancement, since this seems to need additional work.
Comment 29 ht990332 2007-06-25 12:15:48 UTC
Anyone can be kind enough to attach a copy of the patch
openoffice.org-2.0.2.ooo64508.vcl.honourfontconfighinting.patch that applies to
SRC680_m217 ?
Thanks in advance.
Comment 30 rene 2007-08-10 12:51:33 UTC
Issue 80539 might be interesting in this context
Comment 31 caolanm 2007-08-21 14:03:07 UTC
Created attachment 47681 [details]
patch resynced to OOG680
Comment 32 hdu@apache.org 2007-08-21 16:09:36 UTC
@cmc: this latest patch looks like it belongs to issue 72129?

Anyway, I could only apply it if OOo's base platform would contain fontconfig, or if the problem mentioned 
in the latest comment of issue 72129 (we need to wrap a dynamically loaded fontconfig, export it, etc.) 
was solved.
Comment 33 caolanm 2007-08-21 16:40:56 UTC
Created attachment 47690 [details]
indeed, it was the wrong one, here's the updated one for this issue.
Comment 34 Martin Hollmichel 2008-01-28 02:33:44 UTC
set target 3.0
Comment 35 caolanm 2008-04-21 13:19:09 UTC
Created attachment 53086 [details]
updated patch for a post-cairo render world
Comment 36 caolanm 2008-04-21 13:20:39 UTC
After the cairo workspace in in place I don't think we need to use the cairo
stuff in psprint anymore as for the interesting case cairo should just OR the
cairo flags with the fontconfig flags itself (I think) so that can be removed
from this proposal

And the other addition is then from
https://bugzilla.redhat.com/show_bug.cgi?id=443356 where fontconfig's rules only
work on the "canonical" name so use that when fetching the fontconfig hints
Comment 37 caolanm 2008-05-30 10:39:06 UTC
Created attachment 54084 [details]
we don't need the subpixel stuff now either
Comment 38 caolanm 2008-05-30 10:39:38 UTC
So that shrinks this down further to the minimum required
Comment 39 hdu@apache.org 2008-05-30 12:00:27 UTC
Still quite big... I hope to have a detailed look + test on the important platforms in time for OOo 3.

Our asian friends really appreciated the unhinted look for CJK glyphs though. Enabling/disabling it for the 
whole font is probably seen as regression... What are the current opinions?
Comment 40 tml 2008-06-10 21:58:00 UTC
This patch has not been tested on Windows, I assume? At least the corresponding
patch in the "FedoraFixes" set in ooo-build,
ooo64508.vcl.honourfontconfighinting.diff, causes a build break on Windows.
There is at least one trivial problem (the GetFontHints added to WinSalGraphics
should be const), and at least one larger one (the patch removes
meEmbeddedBitmap from ImplDevFontAttributes, but doesn't do anything to the use
of that field in vcl/win/source/gdi/salgdi3.cxx). (That is from looking at the
compiler errors, not actually understanding the code and what the patch does.)
Comment 41 hdu@apache.org 2008-07-21 16:04:41 UTC
target
Comment 42 caolanm 2008-09-19 12:49:57 UTC
Created attachment 56635 [details]
just an update to use FcConfigGetCurrent instead of getDefConfig
Comment 43 hdu@apache.org 2009-01-12 11:30:17 UTC
Retargeting: Most changes in related areas caused bad regressions and OOo3.1 code freeze is way too close.
Comment 44 caolanm 2009-10-09 15:00:30 UTC
Created attachment 65272 [details]
update patch to apply against DEV300_m61
Comment 45 thb 2009-11-26 20:59:18 UTC
Created attachment 66365 [details]
Check more thoroughly before using ptr
Comment 46 thb 2009-11-26 21:00:41 UTC
Above patch fixes crash when using generic plugin instead (in the most
simplistic way).
Comment 47 hdu@apache.org 2009-12-23 10:51:12 UTC
@rt: if the baselines allow it the build environment should be updated fontconfig.h from 2.2.0 to at least 
2.2.91. Even if older baselines were required having a newer header was fine as its newer parts (e.g. 
"hintstyle") would just be ignored.

@cmc: the latest patch seems to miss some changes (to settings.hxx and settings.cxx?)
  salgdi3.cxx:1622: error: 'const class StyleSettings' has no member named 'GetCairoFontOptions'
Comment 48 caolanm 2010-01-04 10:15:58 UTC
cmc->hdu: The sequence of fontconfig-related patches I'm using are..

a) issue 88303 which introduces GetCairoFontOptions to get cairo's defaults for
hinting etc.
b) this issue 64508 to use autohint/hinting as mediated by fontconfig for
specific fonts and the global cairo options (needs thb's fix apparently for the
non-gtk case, something that never arises in our world)
c) issue 87970 to stop "same name" substitutions
d) issue 105784 to improve suggestions a bit by providing language hints when
   we don't otherwise have them (minor in comparison to a and b)

So my patch depends on the 88303 stuff. I could regenerate it without that, but
without access to the cairo default flags it wouldn't give the same rendering as
say gedit, though it'll still be a step closer.
Comment 49 hdu@apache.org 2010-01-08 16:07:10 UTC
Thanks for the info.
@hr: for the patches to work we'd need to update the baseline to gtk>=2.8.2 and cairo>=0.1.6
Comment 50 hdu@apache.org 2010-01-18 16:03:03 UTC
Applied in CWS fchints01 for release target 3.3. Thanks Caolan!
Comment 51 hdu@apache.org 2010-02-17 08:56:33 UTC
@sba: please verify in CWS fchints01
Only the X11-ports are affected.
Comment 52 stefan.baltzer 2010-03-02 09:19:53 UTC
SBA: Verified in CWS fchints01.
Comment 53 caolanm 2010-05-22 17:43:36 UTC
closing, integrated DEV300_m75
Comment 54 hdu@apache.org 2010-09-17 12:45:17 UTC
*** Issue 93565 has been marked as a duplicate of this issue. ***