Support getting Glyph Extents from Moz2D ScaledFonts

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla51
x86_64
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(5 attachments, 7 obsolete attachments)

2.83 KB, patch
Details | Diff | Splinter Review
6.04 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
5.58 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
7.37 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.81 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
In order to remove a dependency on cairo we want to get glyph extents out of Moz2D ScaledFont objects.
(Assignee)

Comment 1

5 years ago
Created attachment 8338860 [details] [diff] [review]
Add GetGlyhMetric API to ScaledFont objects
Attachment #8338860 - Flags: review?(jdaggett)
(Assignee)

Comment 2

5 years ago
Created attachment 8338868 [details] [diff] [review]
Add GetGlyhMetric API to ScaledFont objects v2

Updated to avoid wasteful glyph position info.
Attachment #8338860 - Attachment is obsolete: true
Attachment #8338860 - Flags: review?(jdaggett)
Attachment #8338868 - Flags: review?(jdaggett)

Updated

5 years ago
Attachment #8338868 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 8338976 [details] [diff] [review]
Part 2: Implement GetGlyphMetrics API for DirectWrite

Haven't tested this yet as I have yet to do the integration code, but it works in theory :-).
Attachment #8338976 - Flags: review?(jdaggett)
(Assignee)

Comment 4

5 years ago
Created attachment 8338978 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo
(Assignee)

Updated

5 years ago
Attachment #8338978 - Attachment is patch: true
Attachment #8338978 - Flags: review?(jdaggett)
(Assignee)

Comment 5

5 years ago
Created attachment 8338980 [details] [diff] [review]
Part 1: Add GetGlyhMetric API to ScaledFont objects v3

Removed the bogus hunks, carrying r+.
Attachment #8338868 - Attachment is obsolete: true
Attachment #8338980 - Flags: review+
Comment on attachment 8338976 [details] [diff] [review]
Part 2: Implement GetGlyphMetrics API for DirectWrite

Review of attachment 8338976 [details] [diff] [review]:
-----------------------------------------------------------------

::: ScaledFontDWrite.cpp
@@ +355,5 @@
> +                              designUnitCorrection * mSize;
> +    aGlyphMetrics[i].mHeight = (metrics[i].topSideBearing - metrics[i].verticalOriginY) * designUnitCorrection * mSize;
> +
> +    // GetDesignGlyphMetrics returns 'ideal' glyph metrics, we need to pad to
> +    // account for antialiasing.

Whether we need such "padding" depends on the purpose for which we're requesting the metrics: for painting/invalidation, yes, but for layout measurements, no. How's that distinction going to be handled?
Comment on attachment 8338978 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo

Review of attachment 8338978 [details] [diff] [review]:
-----------------------------------------------------------------

::: ScaledFontBase.cpp
@@ +164,5 @@
> +      aGlyphMetrics[i].mYAdvance = extents.y_advance;
> +      aGlyphMetrics[i].mWidth = extents.width;
> +      aGlyphMetrics[i].mHeight = extents.height;
> +    }
> +

You'd better do a return here, otherwise you'll hit the MOZ_ASSERT below!
(Assignee)

Comment 8

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Comment on attachment 8338976 [details] [diff] [review]
> Part 2: Implement GetGlyphMetrics API for DirectWrite
> 
> Review of attachment 8338976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ScaledFontDWrite.cpp
> @@ +355,5 @@
> > +                              designUnitCorrection * mSize;
> > +    aGlyphMetrics[i].mHeight = (metrics[i].topSideBearing - metrics[i].verticalOriginY) * designUnitCorrection * mSize;
> > +
> > +    // GetDesignGlyphMetrics returns 'ideal' glyph metrics, we need to pad to
> > +    // account for antialiasing.
> 
> Whether we need such "padding" depends on the purpose for which we're
> requesting the metrics: for painting/invalidation, yes, but for layout
> measurements, no. How's that distinction going to be handled?

We currently do this padding unconditionally in cairo for DirectWrite and GDI, I simply wanted to mimic that behavior :).
(In reply to Bas Schouten (:bas.schouten) from comment #8)
> We currently do this padding unconditionally in cairo for DirectWrite and
> GDI, I simply wanted to mimic that behavior :).

No; we don't add padding if antialiasing is disabled in the font options:

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-win32-font.c#1002
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-dwrite-font.cpp#747
(Assignee)

Comment 10

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > We currently do this padding unconditionally in cairo for DirectWrite and
> > GDI, I simply wanted to mimic that behavior :).
> 
> No; we don't add padding if antialiasing is disabled in the font options:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> win32-font.c#1002
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> dwrite-font.cpp#747

Yes, but we practically never disable that as far as I know. I was referring to not based on whether the font is used for measuring or drawing. It could be that we disable anti-aliasing explicitly for those cases, but I didn't see that anywhere (I might've missed it though). As far as I could tell unless the user explicitly switched off antialiasing on the system, we'll always get a default scaled font option, which is not NONE.
We -do- explicitly disable antialiasing (so as to get the unpadded glyph extents) if gfxFont::Measure is called with aBoundingBoxType = TIGHT_HINTED_OUTLINE_EXTENTS:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#2923

(I think the comment "only used by MathML layout at present" there may not be accurate; a search for TIGHT_HINTED_OUTLINE_EXTENTS shows other use cases, too.)
(Assignee)

Comment 12

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #11)
> We -do- explicitly disable antialiasing (so as to get the unpadded glyph
> extents) if gfxFont::Measure is called with aBoundingBoxType =
> TIGHT_HINTED_OUTLINE_EXTENTS:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#2923
> 
> (I think the comment "only used by MathML layout at present" there may not
> be accurate; a search for TIGHT_HINTED_OUTLINE_EXTENTS shows other use
> cases, too.)

Ah, interesting :) That's quite well hidden! So.. I'd been thinking about this more tonight anyway, and my suggestion was going to be as follows:

We remove any padding from the existing code (this means making sure anti-alias is set to none for cairo :( I'll look into this). In other words we return the ideal glyph metrics. Then when the caller knows he's doing anti-aliased drawing it can add padding itself, it seems like this would make a lot more sense than the current, fairly obscure, system.

Comment 13

5 years ago
Comment on attachment 8338978 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo

Probably better if Jonathan reviews these.
Attachment #8338978 - Flags: review?(jdaggett) → review?(jfkthame)

Updated

5 years ago
Attachment #8338976 - Flags: review?(jdaggett) → review?(jfkthame)
(In reply to Bas Schouten (:bas.schouten) from comment #12)
> (In reply to Jonathan Kew (:jfkthame) from comment #11)
> > We -do- explicitly disable antialiasing (so as to get the unpadded glyph
> > extents) if gfxFont::Measure is called with aBoundingBoxType =
> > TIGHT_HINTED_OUTLINE_EXTENTS:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#2923
> > 
> > (I think the comment "only used by MathML layout at present" there may not
> > be accurate; a search for TIGHT_HINTED_OUTLINE_EXTENTS shows other use
> > cases, too.)
> 
> Ah, interesting :) That's quite well hidden! So.. I'd been thinking about
> this more tonight anyway, and my suggestion was going to be as follows:
> 
> We remove any padding from the existing code (this means making sure
> anti-alias is set to none for cairo :( I'll look into this).

That would presumably mean the cairo backend will end up keeping two versions of every scaled_font, one with AA for drawing, and one without for extents. :( Currently, we only create such a "shadow" font without AA for the rare cases where we really need the unpadded extents.

> In other words
> we return the ideal glyph metrics. Then when the caller knows he's doing
> anti-aliased drawing it can add padding itself, it seems like this would
> make a lot more sense than the current, fairly obscure, system.

That's a possible approach, though it might mean a bunch of code in layout needs to become aware of font/antialiasing settings that it currently doesn't have to consider. I'm not sure whether the added complexity there would be a win...

On looking back at some of the bugs (e.g. bug 445087, bug 475968, bug 476927) where padding was added, I notice that we actually address this at two levels - there's the code in cairo-win32-font and cairo-dwrite-font, but there is ALSO padding code in gfxGDIFont::Measure and gfxDWriteFont::Measure. (And similarly in gfxMacFont.) I don't remember the details of all this now, but it's an area we'll need to examine and test pretty carefully if we're going to change how this is handled.

Some things to test would include the precise glyph positioning in MathML layout and first-letter styling, to ensure that unwanted  glyph-extents padding doesn't loosen up the spacing; and painting and invalidation in examples like the testcases on those earlier bugs, where lack of the appropriate padding can lead to rendering artifacts.
(Assignee)

Comment 15

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #14)
> (In reply to Bas Schouten (:bas.schouten) from comment #12)
> > (In reply to Jonathan Kew (:jfkthame) from comment #11)
> > > We -do- explicitly disable antialiasing (so as to get the unpadded glyph
> > > extents) if gfxFont::Measure is called with aBoundingBoxType =
> > > TIGHT_HINTED_OUTLINE_EXTENTS:
> > > 
> > > http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#2923
> > > 
> > > (I think the comment "only used by MathML layout at present" there may not
> > > be accurate; a search for TIGHT_HINTED_OUTLINE_EXTENTS shows other use
> > > cases, too.)
> > 
> > Ah, interesting :) That's quite well hidden! So.. I'd been thinking about
> > this more tonight anyway, and my suggestion was going to be as follows:
> > 
> > We remove any padding from the existing code (this means making sure
> > anti-alias is set to none for cairo :( I'll look into this).
> 
> That would presumably mean the cairo backend will end up keeping two
> versions of every scaled_font, one with AA for drawing, and one without for
> extents. :( Currently, we only create such a "shadow" font without AA for
> the rare cases where we really need the unpadded extents.

I think we can just check whether our scaled font is antialiased, and if it is during the conversion remove the padding again :). The overhead should be negligible.

> 
> > In other words
> > we return the ideal glyph metrics. Then when the caller knows he's doing
> > anti-aliased drawing it can add padding itself, it seems like this would
> > make a lot more sense than the current, fairly obscure, system.
> 
> That's a possible approach, though it might mean a bunch of code in layout
> needs to become aware of font/antialiasing settings that it currently
> doesn't have to consider. I'm not sure whether the added complexity there
> would be a win...

We can just set a flag and pad whereever we call getextents now. Right?
(In reply to Bas Schouten (:bas.schouten) from comment #15)

> I think we can just check whether our scaled font is antialiased, and if it
> is during the conversion remove the padding again :). The overhead should be
> negligible.

Not sure exactly what you mean here: remove the padding at what level - somewhere within the (platform-specific) font objects, or at a higher (and more generic) level? Different backends pad by different amounts (or not at all), which means any code wanting to remove the padding needs to know what backend is in use and what its magic numbers are.

Given that the discrepancy between glyph extents for "layout" purposes (ignoring antialiasing "bleed") and for "painting" (which needs to include all pixels that may be touched) is highly dependent on the particular platform/font rasterizer in use, I'm inclined to think we should handle this as close to the font rasterizer level as possible - e.g. by having a pair of APIs such as GetGlyphLayoutExtents and GetGlyphPaintExtents. A typical implementation for GetGlyphPaintExtents would call GetGlyphLayoutExtents, and then check the font's antialiasing mode and add any appropriate padding to the result.

Does that sound at all sensible to you?
(Assignee)

Comment 17

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #16)
> (In reply to Bas Schouten (:bas.schouten) from comment #15)
> 
> > I think we can just check whether our scaled font is antialiased, and if it
> > is during the conversion remove the padding again :). The overhead should be
> > negligible.
> 
> Not sure exactly what you mean here: remove the padding at what level -
> somewhere within the (platform-specific) font objects, or at a higher (and
> more generic) level? Different backends pad by different amounts (or not at
> all), which means any code wanting to remove the padding needs to know what
> backend is in use and what its magic numbers are.

We'd do the 'unpadding' at the ScaledFontCairo level inside Moz2D.

> Given that the discrepancy between glyph extents for "layout" purposes
> (ignoring antialiasing "bleed") and for "painting" (which needs to include
> all pixels that may be touched) is highly dependent on the particular
> platform/font rasterizer in use, I'm inclined to think we should handle this
> as close to the font rasterizer level as possible - e.g. by having a pair of
> APIs such as GetGlyphLayoutExtents and GetGlyphPaintExtents. A typical
> implementation for GetGlyphPaintExtents would call GetGlyphLayoutExtents,
> and then check the font's antialiasing mode and add any appropriate padding
> to the result.
> 
> Does that sound at all sensible to you?

The problem is that a ScaledFont is not really rasterizer dependent, so it's a bit of a vague thing to do. Although I don't have strong objections either.
(In reply to Bas Schouten (:bas.schouten) from comment #17)
> > Given that the discrepancy between glyph extents for "layout" purposes
> > (ignoring antialiasing "bleed") and for "painting" (which needs to include
> > all pixels that may be touched) is highly dependent on the particular
> > platform/font rasterizer in use, I'm inclined to think we should handle this
> > as close to the font rasterizer level as possible - e.g. by having a pair of
> > APIs such as GetGlyphLayoutExtents and GetGlyphPaintExtents. A typical
> > implementation for GetGlyphPaintExtents would call GetGlyphLayoutExtents,
> > and then check the font's antialiasing mode and add any appropriate padding
> > to the result.
> > 
> > Does that sound at all sensible to you?
> 
> The problem is that a ScaledFont is not really rasterizer dependent, so it's
> a bit of a vague thing to do. Although I don't have strong objections either.

It's not really clear to me where this should happen, then. ISTM that the ScaledFont (subclass) is closest to the rasterization and so it's best placed to understand what antialiasing/padding may be going on, but maybe that's not correct.

I think I'd be happiest with one of two options for Moz2D:

(a) a GetGlyphExtents API that does *not* attempt any antialias-padding, but returns the "true" outline extents as accurately as it can, leaving the client code to add padding if needed to allow for "bleeding" during rasterization - though this means the higher-level client code needs to be aware of what backend it's using and how much "bleeding" is liable to occur;

or

(b) a pair of APIs (or an API with an options parameter, or something) so that client code can explicitly request *either* the true outline extents or the potential ink extents that include all antialiasing-affected pixels.

There's also (c), where the ScaledFont returns padded extents if its rendering mode is antialiased, and unpadded if it isn't. That's what the cairo fonts currently do; but it seems a bit awkward, as client code that's using antialiased fonts but wants unpadded outline extents for layout purposes ends up having to instantiate a second font with a different AA option just to get those extents. It'd be nice if we could get away from that.

I'm least comfortable with the version in the current ScaledFontDWrite patch here (let's call it (d)), where the GetGlyphExtents API *always* pads, so that client code needing unpadded outline extents needs to know how much padding was done in order to undo it. That seems the least intuitive and most error-prone option, IMO.

I think my inclination is to favor (b), but I'm open to persuasion if you think an alternative is more practical. If we're going to go with (a) for the Moz2D API, though, I'd like to see the corresponding patches for Gecko code that needs glyph extents (and relies on passing a BoundingBoxType parameter to determine which kind of extents it gets), to see what the implications will be once we take cairo out of the picture and just use the Moz2D fonts.

(IIRC, the reason this padding is originally implemented down in the cairo scaled_fonts is that cairo depends on the glyph extents to determine a clip rect for the glyphs when painting; and therefore if its values are unpadded, it ends up incorrectly clipping the glyph bitmaps it paints in some cases. But if Moz2D doesn't depend on this glyph extents API for painting purposes, I think it's more natural for it to return unpadded extents - or to allow the caller to specify which.)
(In reply to Jonathan Kew (:jfkthame) from comment #18)
> 
> I think my inclination is to favor (b), but I'm open to persuasion if you
> think an alternative is more practical. If we're going to go with (a) for
> the Moz2D API, though, I'd like to see the corresponding patches for Gecko
> code that needs glyph extents (and relies on passing a BoundingBoxType
> parameter to determine which kind of extents it gets), to see what the
> implications will be once we take cairo out of the picture and just use the
> Moz2D fonts.

I too also favor (b). How about:
// returns unpadded metrics
ScaledFont::GetGlyphMetrics(const uint16_t* aGlyphs, uint32_t aNumGlyphs, GlyphMetrics* aGlyphMetrics)

// returns padded metrics
DrawTarget::GetGlyphRasterizationMetrics(ScaledFont *font, const uint16_t* aGlyphs, uint32_t aNumGlyphs, GlyphMetrics* aGlyphMetrics)
Comment on attachment 8338978 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo

Review of attachment 8338978 [details] [diff] [review]:
-----------------------------------------------------------------

::: ScaledFontBase.cpp
@@ +155,5 @@
> +      cairo_text_extents_t extents;
> +      glyph.index = aGlyphs[i];
> +      glyph.x = 0;
> +      glyph.y = 0;
> +      cairo_glyph_extents(ctx, &glyph, 1, &extents);

You should use cairo_scaled_font_glyph_extents. That will avoid needing the draw target and will actually use the mScaledFont.
Attachment #8338978 - Flags: review-
Blocks: 948503
(Assignee)

Comment 22

5 years ago
Created attachment 8359225 [details] [diff] [review]
Part 1: Add GetGlyhDesign/RasterizationMetric API to Moz2D
Attachment #8338980 - Attachment is obsolete: true
Attachment #8359225 - Flags: review?(jmuizelaar)
(Assignee)

Comment 23

5 years ago
Created attachment 8359226 [details] [diff] [review]
Part 2: Implement GetGlyphMetrics API for DirectWrite v2
Attachment #8338976 - Attachment is obsolete: true
Attachment #8338976 - Flags: review?(jfkthame)
Attachment #8359226 - Flags: review?(jmuizelaar)
(Assignee)

Comment 24

5 years ago
Created attachment 8359229 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo v2
Attachment #8338978 - Attachment is obsolete: true
Attachment #8338978 - Flags: review?(jfkthame)
Attachment #8359229 - Flags: review?(jmuizelaar)
Comment on attachment 8359229 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo v2

Review of attachment 8359229 [details] [diff] [review]:
-----------------------------------------------------------------

::: ScaledFontBase.cpp
@@ +179,5 @@
> +      cairo_text_extents_t extents;
> +      glyph.index = aGlyphs[i];
> +      glyph.x = 0;
> +      glyph.y = 0;
> +      cairo_glyph_extents(ctx, &glyph, 1, &extents);

we should be able to use cairo_scaled_font_glyph_extents to avoid the dummy ctx.
Attachment #8359229 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 26

5 years ago
Created attachment 8359823 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo v3

Performance was actually unacceptable with this many context creations anyway (ignore the fact I was leaking them too).
Attachment #8359229 - Attachment is obsolete: true
Attachment #8359823 - Flags: review?(jmuizelaar)
(Assignee)

Comment 27

5 years ago
Created attachment 8359833 [details] [diff] [review]
Part 2: Implement GetGlyphMetrics API for DirectWrite v3

This fixes a small problem that prevented this from linking.
Attachment #8359226 - Attachment is obsolete: true
Attachment #8359226 - Flags: review?(jmuizelaar)
Attachment #8359833 - Flags: review?(jmuizelaar)
Attachment #8359833 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8359823 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo v3

Review of attachment 8359823 [details] [diff] [review]:
-----------------------------------------------------------------

::: ScaledFontBase.cpp
@@ +189,5 @@
> +      aGlyphMetrics[i].mHeight = extents.height;
> +
> +      cairo_font_options_t *options;
> +      cairo_scaled_font_get_font_options(mScaledFont, options);
> +      

Might as well add a comment saying that this is undoing the padding the cairo adds.

@@ +212,5 @@
> +  }
> +#endif
> +
> +  // Don't know how to get the glyph metrics...
> +  MOZ_CRASH("The specific backend type is not supported for GetGlyphDesignMetrics.");

It looks like this will be always be hit right now.
Attachment #8359823 - Flags: review?(jmuizelaar) → review+
Attachment #8359225 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8359823 [details] [diff] [review]
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo v3

Review of attachment 8359823 [details] [diff] [review]:
-----------------------------------------------------------------

::: ScaledFontBase.h
@@ +30,5 @@
>    virtual TemporaryRef<Path> GetPathForGlyphs(const GlyphBuffer &aBuffer, const DrawTarget *aTarget);
>  
>    virtual void CopyGlyphsToBuilder(const GlyphBuffer &aBuffer, PathBuilder *aBuilder, BackendType aBackendType, const Matrix *aTransformHint);
>  
> +  virtual void GetGlyphDesignMetrics(const uint16_t* aGlyphIndices, uint32_t aNumGlyphs, GlyphMetrics* aGlyphMetrics);

I think we need a comment explaining exactly what "design metrics" this returns. (I say this partly because I'm not sure it's the right term to be using - but that depends what it's supposed to mean here.)

My natural understanding would be that "design metrics" would be the "idealized" metrics of the glyph outline, assuming infinite resolution and no rounding, antialiasing, etc. As such, they'd be expected to scale linearly with font size. But the implementation in ScaledFontBase, based on the cairo_scaled_font, will (I think) be affected by hinting and grid-fitting of the glyph at the particular size.

If that's true, then I don't think it they should be referred to as "design metrics"; maybe something like "scaled metrics" would be better. But in any case, we need documentation of this API so we know exactly what it's intended to return.
(Assignee)

Comment 30

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #29)
> Comment on attachment 8359823 [details] [diff] [review]
> Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo v3
> 
> Review of attachment 8359823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ScaledFontBase.h
> @@ +30,5 @@
> >    virtual TemporaryRef<Path> GetPathForGlyphs(const GlyphBuffer &aBuffer, const DrawTarget *aTarget);
> >  
> >    virtual void CopyGlyphsToBuilder(const GlyphBuffer &aBuffer, PathBuilder *aBuilder, BackendType aBackendType, const Matrix *aTransformHint);
> >  
> > +  virtual void GetGlyphDesignMetrics(const uint16_t* aGlyphIndices, uint32_t aNumGlyphs, GlyphMetrics* aGlyphMetrics);
> 
> I think we need a comment explaining exactly what "design metrics" this
> returns. (I say this partly because I'm not sure it's the right term to be
> using - but that depends what it's supposed to mean here.)
> 
> My natural understanding would be that "design metrics" would be the
> "idealized" metrics of the glyph outline, assuming infinite resolution and
> no rounding, antialiasing, etc. As such, they'd be expected to scale
> linearly with font size. But the implementation in ScaledFontBase, based on
> the cairo_scaled_font, will (I think) be affected by hinting and
> grid-fitting of the glyph at the particular size.
> 
> If that's true, then I don't think it they should be referred to as "design
> metrics"; maybe something like "scaled metrics" would be better. But in any
> case, we need documentation of this API so we know exactly what it's
> intended to return.

In the case of Cairo-dwrite at least this will cause the actual design metrics to be returned. For other Cairo backends I'm not sure. In any case the -intention- is that these are the idealized metrics.
(Assignee)

Updated

5 years ago
Summary: Get Glyph Extents from Moz2D ScaledFonts → Support getting Glyph Extents from Moz2D ScaledFonts
(Assignee)

Updated

5 years ago
Blocks: 962832

Comment 31

2 years ago
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d0b7f7d78a
Part 1: Add GetGlyphDesign/RasterizationMetric API to Moz2D r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/96594b9a6eb4
Part 2: Implement GetGlyphMetrics API for DirectWrite r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aae091cd8e1
Part 3: Implement GetGlyphMetrics API for ScaledFontBase with Cairo r=jrmuizel
Created attachment 8780276 [details] [diff] [review]
0001-Bug-943626-Implement-GetGlyphMetrics-API-for-Skia.patch

Don't really have a way of testing this yet, but probably worth reviewing.
Attachment #8780276 - Flags: review?(lsalzman)

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8d0b7f7d78a
https://hg.mozilla.org/mozilla-central/rev/96594b9a6eb4
https://hg.mozilla.org/mozilla-central/rev/1aae091cd8e1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Depends on: 1294923
Depends on: 1296258
Comment on attachment 8780276 [details] [diff] [review]
0001-Bug-943626-Implement-GetGlyphMetrics-API-for-Skia.patch

Canceling the review on this for now until we decide how/if to deal with vertical text layout issues.
Attachment #8780276 - Flags: review?(lsalzman)
You need to log in before you can comment on or make changes to this bug.