Issue 6554064: Vertical metrics for FreeType. (Closed)
|
Created:
12 years, 2 months ago by bungeman Modified:
12 years, 1 month ago CC:
skia-review_googlegroups.com, raph, violets_google.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionVertical metrics for FreeType.
Patch Set 1 #
Total comments: 5
Patch Set 2 : Moar. #Patch Set 3 : Always use FreeType's vertical metrics. #Patch Set 4 : SkDot6ToScalar cleanup. #
Total comments: 6
Patch Set 5 : Something which can be checked in. #
Total comments: 2
MessagesTotal messages: 18
First pass at using vertical metrics in FreeType when they are available.
Sign in to reply to this message.
Looks good, though I vote for more comments and/or more descriptive names: Using freetype is tricky as is. What we've having to do to use real or invented vertical metrics, and having to offset the bbox accordingly, is very non-obvious. https://codereview.appspot.com/6554064/diff/1/src/ports/SkFontHost_FreeType.cpp File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.appspot.com/6554064/diff/1/src/ports/SkFontHost_FreeType.c... src/ports/SkFontHost_FreeType.cpp:1124: // fUseVertMetrics means we can use the metric data directly from FreeType. If this is false, but fRec.fFlags is set for vertical, then we must "cons-up" vertical positioning data. https://codereview.appspot.com/6554064/diff/1/src/ports/SkFontHost_FreeType.c... src/ports/SkFontHost_FreeType.cpp:1242: // Vertical meeds the matrix so it can offset the bbox by the difference between the two side-bearings (X and Y). This is because FreeType always positions the bbox relative to the horizontal-origin, even when we've asked for vertical metrics. https://codereview.appspot.com/6554064/diff/1/src/ports/SkFontHost_FreeType_c... File src/ports/SkFontHost_FreeType_common.cpp (right): https://codereview.appspot.com/6554064/diff/1/src/ports/SkFontHost_FreeType_c... src/ports/SkFontHost_FreeType_common.cpp:324: Rename matrix22 to matrixNeededForVertical ? Comment why we do this (i.e. FreeType always places the bbox/outline relative to its horizontal origin, even when we've asked for vertical metrics... https://codereview.appspot.com/6554064/diff/1/src/ports/SkFontHost_FreeType_c... File src/ports/SkFontHost_FreeType_common.h (right): https://codereview.appspot.com/6554064/diff/1/src/ports/SkFontHost_FreeType_c... src/ports/SkFontHost_FreeType_common.h:39: void generateGlyphImage(FT_Face face, const SkGlyph& glyph, SkMaskGamma::PreBlend* maskPreBlend); Rename matrix22 -> matrixForVerticalMetrics ?
Sign in to reply to this message.
LGTM as well.
Sign in to reply to this message.
https://codereview.appspot.com/6554064/diff/1/gm/verttext.cpp File gm/verttext.cpp (right): https://codereview.appspot.com/6554064/diff/1/gm/verttext.cpp#newcode29 gm/verttext.cpp:29: VertTextGM() : face(SkTypeface::CreateFromFile("MotoyaL04Mincho_3.ttf")) { you'll want to remove this before committing.
Sign in to reply to this message.
This removes our old code and just uses FreeType. Tested to work back to 2.4.2 (the oldest I have installed on my test machine). https://codereview.appspot.com/6554064/diff/13001/src/ports/SkFontHost_FreeTy... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.appspot.com/6554064/diff/13001/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:1039: err = FT_Load_Glyph( fFace, glyph->getGlyphID(fBaseGlyphCount), fLoadGlyphFlags); If I add '& ~FT_LOAD_VERTICAL_LAYOUT' here as Cairo appears to be doing, then exact 90 degree rotations are wrong (non-90 degree rotations look ok). https://codereview.appspot.com/6554064/diff/13001/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:1188: path->offset(SkFDot6ToScalar(vector.x), -SkFDot6ToScalar(vector.y)); Needed SkFDot6ToScalar here, so added it to SkFDot6 and then used that to clean up _common.cpp. https://codereview.appspot.com/6554064/diff/13001/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:1188: path->offset(SkFDot6ToScalar(vector.x), -SkFDot6ToScalar(vector.y)); I pulled the translate here instead of having FreeType doing it in _common.cpp since we're doing the bbox offset in this file as well.
Sign in to reply to this message.
lgtm modulo the rebaselining cost (if any) https://codereview.appspot.com/6554064/diff/13001/src/core/SkFDot6.h File src/core/SkFDot6.h (right): https://codereview.appspot.com/6554064/diff/13001/src/core/SkFDot6.h#newcode42 src/core/SkFDot6.h:42: #define SkScalarToFDot6(x) (SkFDot6)((x) * 64) needf anf eff
Sign in to reply to this message.
https://codereview.appspot.com/6554064/diff/13001/gm/verttext.cpp File gm/verttext.cpp (right): https://codereview.appspot.com/6554064/diff/13001/gm/verttext.cpp#newcode16 gm/verttext.cpp:16: //static const char gText[] = "Hello"; I think we still want this to be the default so we still draw something on systems that don't have the CJK characters available. https://codereview.appspot.com/6554064/diff/13001/gm/verttext.cpp#newcode33 gm/verttext.cpp:33: SkAutoTUnref<SkTypeface> face; if we aren't going to revert the changes in this file it should be fFace.
Sign in to reply to this message.
Patch Set 4 is something which can be checked in and hopefully addresses all comments. It has been tested with a number of fonts and settings and seems to hold up.
Sign in to reply to this message.
https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:997: vector.y = -fFace->glyph->metrics.vertBearingY - fFace->glyph->metrics.horiBearingY; Here are some comments from the android team... Victoria Lease: I see a lot of this construction in this change and am trying to wrap my head around what it's doing and why. It looks like it's backing out a horizontal bearing transform and applying a vertical bearing transform in its place. Does Freetype cook horizontal bearing into the glyph metrics it hands us, leaving it up to us to back them out if we're operating in vertical mode? Raph Levien: And if this is the case, the code is probably wrong in the case where the horiBearingY is nonzero, as the unary minus flipping the coordinate seems to be applied to the vertBearingY but not the horiBearingY. That said, I think in practice the horiBearingY will almost always be zero, which would explain why it would work in testing.
Sign in to reply to this message.
On 2012/09/26 12:55:41, DerekS wrote: > https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... > File src/ports/SkFontHost_FreeType.cpp (right): > > https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... > src/ports/SkFontHost_FreeType.cpp:997: vector.y = > -fFace->glyph->metrics.vertBearingY - fFace->glyph->metrics.horiBearingY; > Here are some comments from the android team... > > Victoria Lease: > I see a lot of this construction in this change and am trying to wrap my head > around what it's doing and why. > It looks like it's backing out a horizontal bearing transform and applying a > vertical bearing transform in its place. Does Freetype cook horizontal bearing > into the glyph metrics it hands us, leaving it up to us to back them out if > we're operating in vertical mode? > > Raph Levien: > And if this is the case, the code is probably wrong in the case where the > horiBearingY is nonzero, as the unary minus flipping the coordinate seems to be > applied to the vertBearingY but not the horiBearingY. That said, I think in > practice the horiBearingY will almost always be zero, which would explain why it > would work in testing. I'm not entirely sure about it myself, but it is what Cairo has been doing for some time, was recommended by Behdad, hasn't been corrected by anyone affiliated with FreeType, and seems to be working. If you have an example font to the contrary, please let me know.
Sign in to reply to this message.
I think I'm in best position to answer these. See blow: On Wed, Sep 26, 2012 at 8:55 AM, <djsollen@google.com> wrote: > > https://codereview.appspot.**com/6554064/diff/16001/src/** > ports/SkFontHost_FreeType.cpp<https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeType.cpp> > File src/ports/SkFontHost_FreeType.**cpp (right): > > https://codereview.appspot.**com/6554064/diff/16001/src/** > ports/SkFontHost_FreeType.cpp#**newcode997<https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeType.cpp#newcode997> > src/ports/SkFontHost_FreeType.**cpp:997: vector.y = > -fFace->glyph->metrics.**vertBearingY - > fFace->glyph->metrics.**horiBearingY; > Here are some comments from the android team... > > Victoria Lease: > I see a lot of this construction in this change and am trying to wrap my > head around what it's doing and why. > It looks like it's backing out a horizontal bearing transform and > applying a vertical bearing transform in its place. Does Freetype cook > horizontal bearing into the glyph metrics it hands us, leaving it up to > us to back them out if we're operating in vertical mode? > Glyphs can have two different "origins". The horizontal origin is positioned at baseline-left of the glyph, whereas vertical origin is at top-center. See the images in this page: http://www.freetype.org/freetype2/docs/tutorial/step2.html Now, each font API has its own way to choose between the two origins. In FreeType, the APIs to switch to the vertical origin are broken at best. The documentation speaks for itself: * FT_LOAD_VERTICAL_LAYOUT :: * Load the glyph for vertical text layout. _Don't_ use it as it is * problematic currently. Which, umm, I think is what the patch under review does. So that needs to be backed out. Alternatively, the way to do it with FreeType is to do the translation on the user side. Here we would need to know the vector difference between the horizontal origin and vertical origin. Call those points O_h and O_v. Now, the "bearings" point to the top-left of the glyph. Call that point B. We have the values: horiBearings = B - O_h vertBearings = B - O_v From which we can deduce: O_v - O_h = vertBearings - horiBearings And vice versa. > Raph Levien: > And if this is the case, the code is probably wrong in the case where > the horiBearingY is nonzero, as the unary minus flipping the coordinate > seems to be applied to the vertBearingY but not the horiBearingY. That > said, I think in practice the horiBearingY will almost always be zero, > which would explain why it would work in testing. > Again, those images should help. Everywhere in FreeType, Y grows up. However, in vertical metrics, Y bearings grow down. I reported this a while back and I think docs got updated to reflect it: http://lists.nongnu.org/archive/html/freetype-devel/2011-05/msg00044.html Also note that almost never horiBearingY is zero. It points to the top of the glyph from the baseline. I worked extensively to get vertical text right in cairo, so I'm confident that what's there is good enough for us. behdad
Sign in to reply to this message.
I think I'm in best position to answer these. See blow: On Wed, Sep 26, 2012 at 8:55 AM, <djsollen@google.com> wrote: https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:997: vector.y = -fFace->glyph->metrics.vertBearingY - fFace->glyph->metrics.horiBearingY; Here are some comments from the android team... Victoria Lease: I see a lot of this construction in this change and am trying to wrap my head around what it's doing and why. It looks like it's backing out a horizontal bearing transform and applying a vertical bearing transform in its place. Does Freetype cook horizontal bearing into the glyph metrics it hands us, leaving it up to us to back them out if we're operating in vertical mode? Glyphs can have two different "origins". The horizontal origin is positioned at baseline-left of the glyph, whereas vertical origin is at top-center. See the images in this page: http://www.freetype.org/freetype2/docs/tutorial/step2.html Now, each font API has its own way to choose between the two origins. In FreeType, the APIs to switch to the vertical origin are broken at best. The documentation speaks for itself: * FT_LOAD_VERTICAL_LAYOUT :: * Load the glyph for vertical text layout. _Don't_ use it as it is * problematic currently. Which, umm, I think is what the patch under review does. So that needs to be backed out. Alternatively, the way to do it with FreeType is to do the translation on the user side. Here we would need to know the vector difference between the horizontal origin and vertical origin. Call those points O_h and O_v. Now, the "bearings" point to the top-left of the glyph. Call that point B. We have the values: horiBearings = B - O_h vertBearings = B - O_v From which we can deduce: O_v - O_h = vertBearings - horiBearings And vice versa. Raph Levien: And if this is the case, the code is probably wrong in the case where the horiBearingY is nonzero, as the unary minus flipping the coordinate seems to be applied to the vertBearingY but not the horiBearingY. That said, I think in practice the horiBearingY will almost always be zero, which would explain why it would work in testing. Again, those images should help. Everywhere in FreeType, Y grows up. However, in vertical metrics, Y bearings grow down. I reported this a while back and I think docs got updated to reflect it: http://lists.nongnu.org/archive/html/freetype-devel/2011-05/msg00044.html Also note that almost never horiBearingY is zero. It points to the top of the glyph from the baseline. I worked extensively to get vertical text right in cairo, so I'm confident that what's there is good enough for us.
Sign in to reply to this message.
https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:835: loadFlags |= FT_LOAD_VERTICAL_LAYOUT; As per recent discussion I think this needs to be backed out. If you read cairo-ft-font.c, it turns this flag on, but turns it off later: /* * Don't pass FT_LOAD_VERTICAL_LAYOUT to FT_Load_Glyph here as * suggested by freetype people. */ if (load_flags & FT_LOAD_VERTICAL_LAYOUT) { load_flags &= ~FT_LOAD_VERTICAL_LAYOUT; vertical_layout = TRUE; }
Sign in to reply to this message.
On 2012/09/26 15:41:48, behdad wrote: > https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... > File src/ports/SkFontHost_FreeType.cpp (right): > > https://codereview.appspot.com/6554064/diff/16001/src/ports/SkFontHost_FreeTy... > src/ports/SkFontHost_FreeType.cpp:835: loadFlags |= FT_LOAD_VERTICAL_LAYOUT; > As per recent discussion I think this needs to be backed out. If you read > cairo-ft-font.c, it turns this flag on, but turns it off later: > > /* > * Don't pass FT_LOAD_VERTICAL_LAYOUT to FT_Load_Glyph here as > * suggested by freetype people. > */ > if (load_flags & FT_LOAD_VERTICAL_LAYOUT) { > load_flags &= ~FT_LOAD_VERTICAL_LAYOUT; > vertical_layout = TRUE; > } Cannot back this out. Tried that, but then 0, 90, 180, and 270 degree rotations don't work with fonts without vertical metrics. I realize Cairo does this, but they also use different metrics.
Sign in to reply to this message.
Lets construct some tests that... - show that the VERTICAL flag in FT is needed? - show that the VERTICAL flag in FT is not needed? - show that the VERTICAL flag in FT is buggy?
Sign in to reply to this message.
Okay, I'm satisfied about the sign on vertBearingY - Behdad's explanation is good, and the message that observes that those coordinates (and the vertical advance) grow down (as opposed to nearly all the other Y coordinates in FreeType) cleared up my concern. I also checked it against the Cairo source to be sure. No comments so far on the FT_LOAD_VERTICAL_LAYOUT question - that looks tricky. Raph On Wed, Sep 26, 2012 at 8:44 AM, <reed@google.com> wrote: > Lets construct some tests that... > > - show that the VERTICAL flag in FT is needed? > - show that the VERTICAL flag in FT is not needed? > - show that the VERTICAL flag in FT is buggy? > > > https://codereview.appspot.**com/6554064/<https://codereview.appspot.com/6554... >
Sign in to reply to this message.
On 2012/09/26 15:44:41, bungeman wrote:
> > */
> > if (load_flags & FT_LOAD_VERTICAL_LAYOUT) {
> > load_flags &= ~FT_LOAD_VERTICAL_LAYOUT;
> > vertical_layout = TRUE;
> > }
>
> Cannot back this out. Tried that, but then 0, 90, 180, and 270 degree
rotations
> don't work with fonts without vertical metrics. I realize Cairo does this, but
> they also use different metrics.
Then I think we should figure out why they are not working.
What were you observing?
Sign in to reply to this message.
I *think* what cairo does is to use LOAD_VERTICAL_LAYOUT to get the bearings / metrics, but not use it when rendering the glyph or loading the path. My guess is that certain FreeType drivers ignore the LOAD_VERTICAL_LAYOUT.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
