Closed Bug 1400917 Opened 2 years ago Closed 2 years ago

wr-text: implement tofu glyphs

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox59 --- fixed

People

(Reporter: calixte, Assigned: lsalzman)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [wr-reserve] [gfx-noted])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-84ec09a4-b7b1-42e8-ac89-470180170918.
=============================================================

There are 5 crashes (from 3 installs) in nightly 57 with buildid 20170918100059. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1400411.

[1] https://hg.mozilla.org/mozilla-central/rev?node=8a0b8dfa2d7df3e13bea7f81ed7a17c360eef5d7
Flags: needinfo?(a.beingessner)
Priority: -- → P2
Whiteboard: [clouseau] → [wr-mvp] [clouseau]
Yes, this is my fault. Tofu glyphs take the StrokeRect path; should be easy to fix.
Flags: needinfo?(a.beingessner)
Reverted the problem patch; hijacking this for "implement this properly".
Blocks: 1392723
Severity: critical → normal
Summary: Crash in mozilla::layout::TextDrawTarget::StrokeRect → text-layers: implement tofu glyphs
:Gankro, there are 8 crashes from the same installation with signature "mozilla::layout::TextDrawTarget::CreatePathBuilder" (see [1]).  

[1] https://crash-stats.mozilla.com/report/index/1edee317-2ac4-46e8-bed2-535280170919
Removing the regression/crash keywords since this was repurposed, and setting 57:unaffected because webrender won't ship in 57.
Priority: P2 → P3
Whiteboard: [wr-mvp] [clouseau] → [gfx-noted]
Blocks: 1407627
Summary: text-layers: implement tofu glyphs → wr-text: implement tofu glyphs
Whiteboard: [gfx-noted] → [wr-reserve] [gfx-noted]
Assignee: nobody → a.beingessner
deprioritizing; mostly just a (minor) performance thing. Tofu just triggers fallback on the text-run, which does lose subpixel-AA, but is otherwise fine.
Assignee: a.beingessner → nobody
Priority: P1 → P3
On TextDrawTarget, I implemented StrokeRect for dealing with the tofu border, and FillRect for dealing with the mini font rects.

Since TextDrawTarget doesn't really deal with transforms, and we still need to support writing modes with tofu, I pass in a matrix representing the state of the current glyph transform flags to the render code. The border rect then gets transformed by this like a normal glyph would. It then uses the basis vectors of the matrix to step along the mini font grid, rendering via FillRect.

I tried as much as possible to avoid forking/copy-pasta-ing the missing glyph drawing code, so the final code change is quite small and still easy enough to work with, considering.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8943686 - Flags: review?(a.beingessner)
Priority: P3 → P1
OS: Linux → All
Hardware: Unspecified → All
Comment on attachment 8943686 [details] [diff] [review]
render missing glyphs with WebRender

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

Sick!

::: gfx/thebes/gfxFont.cpp
@@ +2014,5 @@
>          Float advanceDevUnits =
>              Float(ToDeviceUnits(advance, aRunParams.devPerApp));
>          Float height = GetMetrics(eHorizontal).maxAscent;
> +        // Horizontally center if drawing vertically upright with no sideways transform.
> +        Rect glyphRect = aFontParams.isVerticalFont && !mat.HasNonAxisAlignedTransform() ?

When would a NonAxisAligned transform happen? It seems like WR would mishandle it..?
Attachment #8943686 - Flags: review?(a.beingessner) → review+
(In reply to Alexis Beingessner [:Gankro] from comment #8)
> Comment on attachment 8943686 [details] [diff] [review]
> render missing glyphs with WebRender
> 
> Review of attachment 8943686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sick!
> 
> ::: gfx/thebes/gfxFont.cpp
> @@ +2014,5 @@
> >          Float advanceDevUnits =
> >              Float(ToDeviceUnits(advance, aRunParams.devPerApp));
> >          Float height = GetMetrics(eHorizontal).maxAscent;
> > +        // Horizontally center if drawing vertically upright with no sideways transform.
> > +        Rect glyphRect = aFontParams.isVerticalFont && !mat.HasNonAxisAlignedTransform() ?
> 
> When would a NonAxisAligned transform happen? It seems like WR would
> mishandle it..?

Here I am just checking if we generated a TRANSPOSE matrix as the comment indicates. The function name is a bit confusing.
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36bc055f7ad0
render missing glyphs with WebRender. r=gankro
https://hg.mozilla.org/mozilla-central/rev/36bc055f7ad0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1479196
You need to log in before you can comment on or make changes to this bug.