762737 - [Azure] Refactor the gfxFT2* font classes to separate better between FreeType and Cairo

Status: RESOLVED INVALID

(Core :: Graphics, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Separate FreeType and Cairo classes

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Keep Cairo fonts hanging around so we don't need to keep recreating them

Comment 2

[George Wright (:gw280) (needinfo me!)]

Attachment: refactor fonts

Hopefully something close to what I can land.

Pushed a full try build with talos to https://tbpl.mozilla.org/?tree=Try&rev=d3d311ee87fe

Comment 3

[John Daggett (:jtd)]

Comment on attachment 637692 [details] [diff] [review]
refactor fonts

I think it might be better for Jonathan to review this.

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: Small patch in order to fix compilation with Qt port with pango-disabled

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 637692 [details] [diff] [review]
refactor fonts

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

Not a review yet (sorry), just some thoughts for possible consideration. I'm still struggling to get my head around the tangle of classes involved here (I don't mean just this patch, but our existing mess...)

It seems to me that gfxFT2Extents is mis-named, as it's not just an object that represents font (or glyph) extents, or even provides an interface (just) for extents - it's more like a gfxFT2FontAccessor, as it provides char/glyph mapping and font table access as well as extents. However, it's not obvious to me why we'd want to split this out from gfxFT2FontBase as a separate object at all.

I'm a bit confused too as to why gfxCairoFTFont is a separate subclass of gfxFont, rather than being a subclass of gfxFT2FontBase; ISTM that we should be able to have gfxFT2FontBase to handle everything that can be shared by all FreeType-based font backends, like getting tables and metrics from the FT_Face, and then gfxCairoFTFont would be a subclass of this, along with any other FT2-based implementations (do we need gfxSkiaFT2Font?).

Comment 6

[George Wright (:gw280) (needinfo me!)]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Comment on attachment 637692 [details] [diff] [review]
> refactor fonts
> 
> Review of attachment 637692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not a review yet (sorry), just some thoughts for possible consideration. I'm
> still struggling to get my head around the tangle of classes involved here
> (I don't mean just this patch, but our existing mess...)
> 
> It seems to me that gfxFT2Extents is mis-named, as it's not just an object
> that represents font (or glyph) extents, or even provides an interface
> (just) for extents - it's more like a gfxFT2FontAccessor, as it provides
> char/glyph mapping and font table access as well as extents. However, it's
> not obvious to me why we'd want to split this out from gfxFT2FontBase as a
> separate object at all.

I think ultimately I just want this class to allow us to use the current backend's metrics calculators. Ideally we want to push this functionality into Azure but it's a little too early for that, so this was the compromise I came up with. The reason we can't just calculate the metrics ourselves using FreeType directly is that we suffer a huge performance hit if we don't use our current rendering backend to calculate the metrics; this is because the backend will cache it internally and we need that. Even if we cache it ourselves, the backend is still going to calculate/cache it when we ask it to render, so it seems sensible to let it handle it all the time for us.

All of the non-extents methods in the extents class can/should be moved into gfxFT2Utils, and the callers appropriately modified to call it there instead.

> I'm a bit confused too as to why gfxCairoFTFont is a separate subclass of
> gfxFont, rather than being a subclass of gfxFT2FontBase; ISTM that we should
> be able to have gfxFT2FontBase to handle everything that can be shared by
> all FreeType-based font backends, like getting tables and metrics from the
> FT_Face, and then gfxCairoFTFont would be a subclass of this, along with any
> other FT2-based implementations (do we need gfxSkiaFT2Font?).

That's the way the class setup used to be in a previous iteration of the patch, but off-hand I can't remember why it switched to the way it is now. I suspect it was due to extents calculation before I had the Extents class. I think we should be able to make the hierarchy like that again, which would make far more sense.

Comment 8

[George Wright (:gw280) (needinfo me!)]

(In reply to George Wright (:gw280) from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #5)
> > Comment on attachment 637692 [details] [diff] [review]
> > refactor fonts
> > 
> > Review of attachment 637692 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Not a review yet (sorry), just some thoughts for possible consideration. I'm
> > still struggling to get my head around the tangle of classes involved here
> > (I don't mean just this patch, but our existing mess...)
> > 
> > It seems to me that gfxFT2Extents is mis-named, as it's not just an object
> > that represents font (or glyph) extents, or even provides an interface
> > (just) for extents - it's more like a gfxFT2FontAccessor, as it provides
> > char/glyph mapping and font table access as well as extents. However, it's
> > not obvious to me why we'd want to split this out from gfxFT2FontBase as a
> > separate object at all.
> 
> I think ultimately I just want this class to allow us to use the current
> backend's metrics calculators. Ideally we want to push this functionality
> into Azure but it's a little too early for that, so this was the compromise
> I came up with. The reason we can't just calculate the metrics ourselves
> using FreeType directly is that we suffer a huge performance hit if we don't
> use our current rendering backend to calculate the metrics; this is because
> the backend will cache it internally and we need that. Even if we cache it
> ourselves, the backend is still going to calculate/cache it when we ask it
> to render, so it seems sensible to let it handle it all the time for us.

Hrm, thinking about this more, I'm starting to think that your suggestion of just renaming the class would be a better idea. I feel this is mainly because all the operations require the FT_Face object, and the different backends will have different ways of accessing the FT_Face represented by the native object; specifically, cairo requires that we lock the scaled font, use the face, then unlock it, and discard our reference to that face. Also things such as GetGlyph we will likely want to use the backend to do, rather than freetype, for caching reasons.

Comment 9

[George Wright (:gw280) (needinfo me!)]

Closing as we decided against doing this.