748228 - nsTextFrameThebes doesn't account for all subpixel AA pixels

Status: NEW

(Core :: Layout: Text and Fonts, defect)

Description

[Matt Woodrow (:mattwoodrow)]

With DLBI (Bug 539356), we are getting some failures where nsDisplayText is drawing pixels outside of it's reported bounds.

I haven't been able to reproduce this locally, so not sure exactly what is going wrong here. It definitely looks like nsTextFrameThebes is using the LOOSE_INK_EXTENTS on the text, which should be correct.

This at least causing layout/reftests/scrolling/fixed-text-1.html and layout/reftests/tab-size/tab-size-change-1b.html to fail on the win7 test boxes, there may be others.

As a workaround I'm expanding the size of nsDisplayText::GetBounds by 1 pixel in all directions.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Any theories on this bug? With WR enabled on Windows I'm able to consistently reproduce a reftest failure in layout/reftests/line-breaking/non-breakable-1.html which seems to be because of this problem.

In particular, the display item for the last line (and probably other lines) isn't wide enough for the antialiased text it contains. The visible region for the painted layer gets simplified differently on the "test" and "reference" pages - as a result the "reference" page ends up with a visible region that's a few pixels too narrow and clips the "f" on the last line.

Here's a reftest analyzer link [1] that shows the problem (the test is the seventh one down). It's from the try push at [2]. Not sure how long these logs will stick around, but I can reproduce the problem locally.

[1] https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-d5693edbf46ddc14597435ce0b7105352cbeb640/try-win64-debug/try_win8_64-debug_test-reftest-e10s-2-bm126-tests1-windows-build102.txt.gz&only_show_unexpected=1
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5693edbf46ddc14597435ce0b7105352cbeb640&selectedJob=109786402

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Expanding the inflation of nsDisplayText::GetBounds from 1 pixel to 2 pixels fixes the reftest, but doesn't seem like a great fix.

Comment 3

[Matt Woodrow (:mattwoodrow)]

My understanding was that the text measurement code is just wrong for this, and is returning a rectangle that doesn't actually cover all filled pixels.

That causes the overflow rect for the frame to be wrong too, and then we invalidate incorrectly.

Padding when we go from extents -> overflow might be a less sad fix, and fixing the underlying text measurement would be ideal.

Comment 4

[Jonathan Kew [:jfkthame]]

I wonder if this is related to the "fast path" for glyph measurement, which doesn't try to return actual ink bounds. If you add "text-rendering: optimizeLegibility" to the relevant CSS in the testcase and reference, does this affect the result? Or alternatively, if you set browser.display.auto_quality_min_font_size to zero, does that help?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> If you add "text-rendering:
> optimizeLegibility" to the relevant CSS in the testcase and reference, does
> this affect the result? Or alternatively, if you set
> browser.display.auto_quality_min_font_size to zero, does that help?

Both of these (I tested them separately) make the reftest pass.

Comment 6

[Jonathan Kew [:jfkthame]]

OK. This isn't really about subpixel-AA, then; it's about the fact that in optimizeSpeed rendering mode (as opposed to optimizeLegibility mode) we don't attempt to get the actual ink bounds of the glyphs; we use the typographic glyph rectangle (i.e. font ascent+descent in the block direction, and glyph origin/advance in the inline dir) as an approximation of the area that may need to be invalidated/painted, which is sometimes not quite sufficient.

Lowercase "f" (in some fonts) is that rare case of a glyph where the image projects beyond the glyph's advance width.

So yes, this is true:

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> My understanding was that the text measurement code is just wrong for this,
> and is returning a rectangle that doesn't actually cover all filled pixels.
> 
> That causes the overflow rect for the frame to be wrong too, and then we
> invalidate incorrectly.
> 
> Padding when we go from extents -> overflow might be a less sad fix, and
> fixing the underlying text measurement would be ideal.

The measurement code is wrong. It's known to be wrong, because it's using a fast path that relies on a not-entirely-reliable approximation.

We could fix this behavior (at the cost of some perf) by using the accurate path always; or we could fudge it by adding a couple of pixels of padding to the approximated glyph rect, but that would still have some perf cost because it will result in the creation of overflow areas on lots of frames that otherwise wouldn't need them; or we could leave the behavior unchanged (accepting the slightly inferior rendering in the optimizeSpeed case) and just fix the reftest by adding optimizeLegibility to its CSS.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Thanks for the info and analysis! I'll try and figure out how many reftests are affected by this issue. I suspect not very many, in which case just adding optimizeLegibility should be fine for my purposes.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> I'll try and figure out how many reftests
> are affected by this issue.

For the record, I tried to do this but didn't get very reliable results. The problem is that running reftests with WR enabled on Windows right now is nondeterministic and produces a lot of failures (see [1] for example). I thought running them locally would be more deterministic but it turns out even in local runs there is some element of randomness, so I'm getting ~315 failures per run, but they vary slightly from one run to another. There's enough variance that it's hard to tell which failures are fixed by inflating the invalidation rect by another pixel.

Anyway, I'll leave this for now. If and when I hammer out the nondeterministic behaviour I'll revisit this and see how many tests are affected by this particular problem.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=63c6ccd0fa337f1c3d43cc8e325dc7ee13be9047