1099977 - Markup indentation whitespace causes text display item, a new thebes layer, component alpha, intermediate surface copy

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benoit Girard (:BenWa)]

I was writing a testcase for bug 1098495 but noticed that I was hitting all the above slow path. Basically the chain of slow path happens because we allocate a display item for the markup indentation (I know it add spacing):

  Text p=0x10e3bedb0 f=0x109cfa618(Text(2)"\n    ") bounds(6450,5730,300,1020) visible(0,0,53580,62100) componentAlpha(6450,5730,300,1020) clip()  layer=0x1174ecc00

When I change my testcase to remove the whitespace the thebes layer goes away, the component alpha flag goes away and my intermediate surface no longer requires a copy.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: retain_buffer_whitespace.html

Comment 2

[Benoit Girard (:BenWa)]

Attachment: display list

Comment 3

[Benoit Girard (:BenWa)]

Attachment: retain_buffer_nowhitespace.html

Comment 4

[Benoit Girard (:BenWa)]

ni?roc since mstange told me you worked on avoiding whitespace text display item?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's a good idea to skip creating display items for text whose textrun contains only spaces (which would cover most forms of whitespace since CSS normally converts them to spaces). The difficulty is that certain very rare fonts have a space glyph that actually draws something. We've discussed this problem before but I don't remember the outcome. Jonathan, do you?

It seems likely to me we could analyze the font data to determine whether the glyph returned by gfxFont::GetSpaceGlyph draws anything. Not sure what the overhead of that would be since we might have to load additional tables.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually we can probably just extend gfxFont::CheckForFeaturesInvolvingSpace to request the exact ink bounds of the space glyph. If they're empty, we know nothing is drawn.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Make gfxDwriteFont cache GetSpaceGlyph

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Make gfxFont::Measure return an empty bounding-box when all glyphs are invisible

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Skip creating nsDisplayText when all glyphs are invisible

Comment 10

[Jonathan Kew [:jfkthame]]

I haven't looked closely at the patches here yet, but one quick question -- maybe the answer is obvious -- will this have any impact on our ability to highlight or hit-test such text?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, part 3 takes care of that by always creating the display item if the text is selected or we're building the display list for some reason other than painting.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8523616 [details] [diff] [review]
Part 1: Make gfxDwriteFont cache GetSpaceGlyph

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

Yes, we should cache it; but I think you can do this a bit differently. Note that gfxDWriteFont::ComputeMetrics() always looks up the space glyph anyway, in order to set mMetrics->spaceWidth. Change that to use mFontFace->GetGlyphIndices directly, rather than calling GetSpaceGlyph(), and store the resulting glyphID. Then GetSpaceGlyph() won't need to check anything, it can just return the saved value.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1 v2

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8524512 [details] [diff] [review]
part 1 v2

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

::: gfx/thebes/gfxDWriteFonts.cpp
@@ +77,5 @@
>                               AntialiasOption anAAOption)
>      : gfxFont(aFontEntry, aFontStyle, anAAOption)
>      , mCairoFontFace(nullptr)
>      , mMetrics(nullptr)
> +    , mSpaceGlyph(-1)

Let's make this a uint32_t (for consistency with other places) and initialize it to zero, now that you don't need -1 as a "not yet initialized" indicator.

@@ +231,5 @@
>  
>      mMetrics->internalLeading = std::max(mMetrics->maxHeight - mMetrics->emHeight, 0.0);
>      mMetrics->externalLeading = ceil(fontMetrics.lineGap * mFUnitsConvFactor);
>  
> +    UINT32 ucs = L' ';

You'd better remove the (re)declaration of ucs a bit later in this method.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1.5: Fix gfxFT2FontBase::GetSpaceGlyph to work for zero-sized fonts

There's also a weird Mac reftest failure that I need to figure out:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-731125b76d43/try-macosx64-debug/try_snowleopard-debug_test-reftest-bm108-tests1-macosx-build3034.txt.gz

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Still getting weird failures on Mac:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c5160f64ef5&filter-searchStr=Rev4%20MacOSX%20Snow%20Leopard%2010.6%20try%20debug%20test%20reftest
and Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c5160f64ef5&filter-searchStr=Windows%207%2032-bit%20try%20opt%20test%20reftest

We're failing to draw text decorations on some space-only textruns. I can't get this to reproduce locally on Mac or Windows, despite the fact it shows up 100% reliably on try on both platforms.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jonathan, could you try reproducing these failures on Mac and/or Windows?

Comment 18

[Jonathan Kew [:jfkthame]]

The patches here didn't apply cleanly on my tree; do they need some rebasing/updating? I might be able to do that locally if necessary, but maybe you have newer ones on hand already...

Looking at the logs, I notice that the bits of text-decoration/complex-decoration-style-*.html that fail are the lines that use text-shadow. Is this because nsContextBoxBlur::Init doesn't set up a gfxContext at all if it's passed an empty rect (which the patches here will give for space-only textruns)? That would cause nsTextFrame::PaintOneShadow to take an early return, too, and not paint anything.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I bet you're right!!! Thanks!!!

Actually it looks like a more general problem that shadowGfxRect/shadowRect doesn't include the area occupied by text decorations, if they extend out of the LOOSE_INK_EXTENTS text area...

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c5f7321e2da4

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixing the shadowRect didn't work :-(

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1.6: Make gfxFT2FontBase set mAdjustedSize

This is why I was not seeing failures on Linux.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 v2

Updated with some refactoring of nsTextFrame to share more shadow-drawing code, and adding the shadowRect extension in there.

We really should call something like nsTextFrame::UnionAdditionalOverflow here to handle cases where the decoration lines extend outside the font-box. But that's a separate bug.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29cfc297c304

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 8547328 [details] [diff] [review]
Part 1.6: Make gfxFT2FontBase set mAdjustedSize

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

::: gfx/thebes/gfxFT2FontBase.cpp
@@ +119,5 @@
>      } else {
>          gfxFT2LockedFace face(this);
>          mFUnitsConvFactor = face.XScale();
>          face.GetMetrics(&mMetrics, &mSpaceGlyph);
> +        mAdjustedSize = mStyle.size;

I'm not sure this is right, in the case where font-size-adjust is in effect and an adjustment has been applied to the font pattern by gfxPangoFonts code, but won't be reflected in mStyle.size. I will try to look into this a bit more .... unless you want to convince me it's legitimate in the meantime.

(It looks like the non-Pango FT2 backend, as used on mobile, doesn't implement font-size-adjust at all, so no discrepancy will show up in that case - but that's a separate bug we should also fix.)

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, we still have an SVG issue. Fix:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a24d78d40d3a

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #25)
> Comment on attachment 8547328 [details] [diff] [review]
> Part 1.6: Make gfxFT2FontBase set mAdjustedSize
> 
> Review of attachment 8547328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFT2FontBase.cpp
> @@ +119,5 @@
> >      } else {
> >          gfxFT2LockedFace face(this);
> >          mFUnitsConvFactor = face.XScale();
> >          face.GetMetrics(&mMetrics, &mSpaceGlyph);
> > +        mAdjustedSize = mStyle.size;
> 
> I'm not sure this is right, in the case where font-size-adjust is in effect
> and an adjustment has been applied to the font pattern by gfxPangoFonts
> code, but won't be reflected in mStyle.size. I will try to look into this a
> bit more .... unless you want to convince me it's legitimate in the meantime.
> 
> (It looks like the non-Pango FT2 backend, as used on mobile, doesn't
> implement font-size-adjust at all, so no discrepancy will show up in that
> case - but that's a separate bug we should also fix.)

I don't know how to proceed here. Does this mean we don't have anywhere in gfxFont a cross-platform way to check the size of the font after font-size-adjust?

Is there any way we can initialize mAdjustedSize so it's at least as valid for gfxFT2FontBase as the current value (0.0)?

Would it be OK for gfxFont::IsSpaceGlyphInvisible to check mStyle.size > 0 instead of mAdjustedSize >= 1.0? All I was trying to do there was avoid spurious "true" returns for zero-sized fonts.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Is there any way we can initialize mAdjustedSize so it's at least as valid
> for gfxFT2FontBase as the current value (0.0)?

I'd really like to do something about this here because I got burned in this bug by mAdjustedSize being valid on all platforms except those using gfxFT2FontBase.

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> (In reply to Jonathan Kew (:jfkthame) from comment #25)
> > Comment on attachment 8547328 [details] [diff] [review]
> > Part 1.6: Make gfxFT2FontBase set mAdjustedSize
> > 
> > Review of attachment 8547328 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/thebes/gfxFT2FontBase.cpp
> > @@ +119,5 @@
> > >      } else {
> > >          gfxFT2LockedFace face(this);
> > >          mFUnitsConvFactor = face.XScale();
> > >          face.GetMetrics(&mMetrics, &mSpaceGlyph);
> > > +        mAdjustedSize = mStyle.size;
> > 
> > I'm not sure this is right, in the case where font-size-adjust is in effect
> > and an adjustment has been applied to the font pattern by gfxPangoFonts
> > code, but won't be reflected in mStyle.size. I will try to look into this a
> > bit more .... unless you want to convince me it's legitimate in the meantime.
> > 
> > (It looks like the non-Pango FT2 backend, as used on mobile, doesn't
> > implement font-size-adjust at all, so no discrepancy will show up in that
> > case - but that's a separate bug we should also fix.)
> 
> I don't know how to proceed here. Does this mean we don't have anywhere in
> gfxFont a cross-platform way to check the size of the font after
> font-size-adjust?

In theory, that's what gfxFont::GetAdjustedSize() ought to be; in practice, I think it's probably wrong on Linux.

> Would it be OK for gfxFont::IsSpaceGlyphInvisible to check mStyle.size > 0
> instead of mAdjustedSize >= 1.0? All I was trying to do there was avoid
> spurious "true" returns for zero-sized fonts.

AFAICS, GetAdjustedSize() should work for that, even if the value it returns may not be accurate in the gfxPangoFonts case (as I suspect).

(I also think we should fix the FT2 backends -- both gfxPangoFonts, unless John gets rid of it first, and gfxFT2Fonts -- so that they *do* set mAdjustedSize correctly, and then GetAdjustedSize can be optimized to be a simple, non-virtual accessor for that field. But we should take that to a separate bug.)

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 2 v3

Thanks, this sounds good.

Comment 31

[Jonathan Kew [:jfkthame]]

Comment on attachment 8547532 [details] [diff] [review]
part 2 v3

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

LGTM - I didn't actually test it, but if you're happy and tryserver is happy, then so am I. :)

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02f84b2d07ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/953dbdb984be
https://hg.mozilla.org/integration/mozilla-inbound/rev/5af10836905a
https://hg.mozilla.org/integration/mozilla-inbound/rev/de30d7df906f

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02f84b2d07ba
https://hg.mozilla.org/mozilla-central/rev/953dbdb984be
https://hg.mozilla.org/mozilla-central/rev/5af10836905a
https://hg.mozilla.org/mozilla-central/rev/de30d7df906f