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

RESOLVED FIXED in mozilla38

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: BenWa, Assigned: roc)

Tracking

unspecified
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

450 bytes, text/html
Details
5.85 KB, text/plain
Details
434 bytes, text/html
Details
5.92 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
3.19 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
1.30 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
17.38 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8523437 [details]
retain_buffer_whitespace.html
(Reporter)

Comment 2

4 years ago
Created attachment 8523438 [details]
display list
(Reporter)

Comment 3

4 years ago
Created attachment 8523439 [details]
retain_buffer_nowhitespace.html
(Reporter)

Comment 4

4 years ago
ni?roc since mstange told me you worked on avoiding whitespace text display item?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc) → needinfo?(jfkthame)
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.
Created attachment 8523616 [details] [diff] [review]
Part 1: Make gfxDwriteFont cache GetSpaceGlyph
Assignee: nobody → roc
Attachment #8523616 - Flags: review?(jfkthame)
Created attachment 8523617 [details] [diff] [review]
Make gfxFont::Measure return an empty bounding-box when all glyphs are invisible
Attachment #8523617 - Flags: review?(jfkthame)
Created attachment 8523618 [details] [diff] [review]
Part 3: Skip creating nsDisplayText when all glyphs are invisible
Attachment #8523618 - Flags: review?(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?
Flags: needinfo?(jfkthame) → needinfo?(roc)
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.
Flags: needinfo?(roc)
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.
Attachment #8523617 - Flags: review?(jfkthame) → review+
Attachment #8523618 - Flags: review?(jfkthame) → review+
Attachment #8523616 - Attachment is obsolete: true
Attachment #8523616 - Flags: review?(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.
Attachment #8524512 - Flags: review?(jfkthame) → review+
Created attachment 8525205 [details] [diff] [review]
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/rocallahan@mozilla.com-731125b76d43/try-macosx64-debug/try_snowleopard-debug_test-reftest-bm108-tests1-macosx-build3034.txt.gz
Attachment #8525205 - Flags: review?(jfkthame)
Attachment #8525205 - Flags: review?(jfkthame) → review+
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.
Jonathan, could you try reproducing these failures on Mac and/or Windows?
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
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...
Fixing the shadowRect didn't work :-(
Created attachment 8547328 [details] [diff] [review]
Part 1.6: Make gfxFT2FontBase set mAdjustedSize

This is why I was not seeing failures on Linux.
Attachment #8547328 - Flags: review?(jfkthame)
Created attachment 8547330 [details] [diff] [review]
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.
Attachment #8547330 - Flags: review?(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.)
(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.
(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.
Flags: needinfo?(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.)
Flags: needinfo?(jfkthame)
Created attachment 8547532 [details] [diff] [review]
part 2 v3

Thanks, this sounds good.
Attachment #8547328 - Attachment is obsolete: true
Attachment #8547330 - Attachment is obsolete: true
Attachment #8547328 - Flags: review?(jfkthame)
Attachment #8547330 - Flags: review?(jfkthame)
Attachment #8547532 - Flags: review?(jfkthame)
Attachment #8523617 - Attachment is obsolete: true
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. :)
Attachment #8547532 - Flags: review?(jfkthame) → review+
Depends on: 1149519
You need to log in before you can comment on or make changes to this bug.