1394568 - wr-text: implement oblique text (synthetic italics)

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Aria Beingessner [:Gankra]]

Example: italic Lucida Grande on macos.

The font code tries to handle this by adding a skew to the text transform and then messing with the translation. We mishandle this and end up drawing the text offset from where it should be, and with no skew.

Talking with webrender devs, it seems like this is a good candidate for a "font instance option" https://github.com/servo/webrender/pull/1602

Comment 2

[Aria Beingessner [:Gankra]]

Blocked on: https://github.com/servo/webrender/issues/1654

Comment 4

[Lee Salzman [:lsalzman]]

This requires upstream WR PRs https://github.com/servo/webrender/pull/2245 and https://github.com/servo/webrender/pull/2262

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: plumb synthetic italics flag through thebes and Moz2D into WebRender

This doesn't do anything too controversial. It just takes the determination of whether a gfxFont needs synthetic italics and sends if off to the various ScaledFonts in Moz2D. This allows WebRender to get the synthetic italics flag it needs from them to activate its own synthetic italics support, which is handled via the upstream WR PRs 2245 and 2262.

Comment 6

[Aria Beingessner [:Gankra]]

Comment on attachment 8940306 [details] [diff] [review]
plumb synthetic italics flag through thebes and Moz2D into WebRender

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

Some minor nits/questions; could land as-is, but would appreciate looking into them.

::: gfx/2d/ScaledFontDWrite.cpp
@@ +431,5 @@
>    RefPtr<ScaledFontBase> scaledFont =
>      new ScaledFontDWrite(mFontFace, this, aGlyphSize,
>                           instanceData->mUseEmbeddedBitmap,
>                           instanceData->mForceGDIMode,
> +                         false,

Why is this unconditionally false? Handled by the native windows APIs?

::: gfx/thebes/gfxFont.cpp
@@ +1912,5 @@
>                        GlyphBufferAzure& aBuffer, bool *aEmittedGlyphs) const
>  {
>      const TextRunDrawParams& runParams(aBuffer.mRunParams);
>  
> +    auto* textDrawer = runParams.context->GetTextDrawer();

This is kinda expensive (a virtual call) for DrawOneGlyph, which has historically been a perf issue; is it plausible to hoist out?

@@ +2014,5 @@
>          // If there's a fake-italic skew in effect as part
>          // of the drawTarget's transform, we need to undo
>          // this before drawing the hexbox. (Bug 983985)
>          gfxContextMatrixAutoSaveRestore matrixRestore;
> +        if (aFontParams.needsOblique && !aFontParams.isVerticalFont && !textDrawer) {

This is confusing to me; !textDrawer is unconditionally false here right? If it was true, we would have returned. Just future-proofing?

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Alexis Beingessner [:Gankro] from comment #6)
> Comment on attachment 8940306 [details] [diff] [review]
> plumb synthetic italics flag through thebes and Moz2D into WebRender
> 
> Review of attachment 8940306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some minor nits/questions; could land as-is, but would appreciate looking
> into them.
> 
> ::: gfx/2d/ScaledFontDWrite.cpp
> @@ +431,5 @@
> >    RefPtr<ScaledFontBase> scaledFont =
> >      new ScaledFontDWrite(mFontFace, this, aGlyphSize,
> >                           instanceData->mUseEmbeddedBitmap,
> >                           instanceData->mForceGDIMode,
> > +                         false,
> 
> Why is this unconditionally false? Handled by the native windows APIs?

In the cases where ::CreateScaledFont() is used (i.e. printing), we are not going to need this flag since right now only WR actually uses it.

> ::: gfx/thebes/gfxFont.cpp
> @@ +1912,5 @@
> >                        GlyphBufferAzure& aBuffer, bool *aEmittedGlyphs) const
> >  {
> >      const TextRunDrawParams& runParams(aBuffer.mRunParams);
> >  
> > +    auto* textDrawer = runParams.context->GetTextDrawer();
> 
> This is kinda expensive (a virtual call) for DrawOneGlyph, which has
> historically been a perf issue; is it plausible to hoist out?

Can do.
 
> @@ +2014,5 @@
> >          // If there's a fake-italic skew in effect as part
> >          // of the drawTarget's transform, we need to undo
> >          // this before drawing the hexbox. (Bug 983985)
> >          gfxContextMatrixAutoSaveRestore matrixRestore;
> > +        if (aFontParams.needsOblique && !aFontParams.isVerticalFont && !textDrawer) {
> 
> This is confusing to me; !textDrawer is unconditionally false here right? If
> it was true, we would have returned. Just future-proofing?

Yes. Just for the day we eventually support that to prevent gotchas.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc271c111e33
plumb synthetic italics flag through thebes and Moz2D into WebRender. r=gankro

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/dc271c111e33