475968 - Windows (ClearType): make all area painted by text be part of text frame's ink overflow area (Rendering errors with font Tahoma in 11px and ClearType)

Status: RESOLVED FIXED

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

Description

[Emil Ivanov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5
Build Identifier: 

There are rendering errors with jQuery during slide.
ClearType is on.

I will attach testcase and screenshot.
You can select between the two stylesheets from View > Page Style, the first is without outlines on links, the second is with default outlines.

Reproducible: Always

Steps to Reproduce:
1. Click on the first link
2. Look between the links
Actual Results:  
Vertical lines are drawing during slide.

Comment 1

[Emil Ivanov]

Attachment: testcase

Comment 2

[Emil Ivanov]

Attachment: screenshot

Comment 3

[Emil Ivanov]

Just to note that this is present too in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

if you add 'text-rendering: optimizeLegibility' to the CSS styles for body, does the problem go away?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I'm asking because I think that causes us to compute more accurate overflow areas for text frames.)

Comment 6

[Emil Ivanov]

I tried with body {text-rendering: optimizeLegibility} but i cant see any difference with it. Still the same rendering errors.

Comment 7

[Emil Ivanov]

BTW if you click after the slide with the CSS with outline, the problem will gone, while in the other CSS the vertical lines will stay there. Thats the difference between both styles.

Comment 8

[Jonathan Kew [:jfkthame]]

I don't see this on Vista, with either 3.0.5 or 3.2a1pre. Will try to reproduce on XP shortly.

Comment 9

[Jonathan Kew [:jfkthame]]

OK, I can confirm that this occurs on XP when ClearType is enabled.

I believe the underlying issue is the same as that for bug 439831, which I have seen previously but cannot currently reproduce; I suspect another change to how the location bar is drawn may have masked that manifestation rather than resolved the real cause.

Comment 10

[Emil Ivanov]

I have seen that not only in the Location bar but in the Status bar too and now in HTML pages, the last one worry me most, because its most visible and hell i cant create a site with such thing on it...

In all places the font is Tahoma 11px and there is some text replacement or deleting. That causes the bugs.

The Location bar is with OS settings Tahoma 8px, but the titles in the autocomplete are with Tahoma 11px and i believe that reflects on the Location bar.
To reproduce it press the Location bar dropdown and while the location bar text is still selected use the mouse to hover on the first autocomplete result and hit escape key. For me it always leaves something like yellow line from the blinking cursor.

On the statusbar so far i have no idea how to reproduce it.

Comment 11

[Emil Ivanov]

Hm i tried on local file and the url was with file://etc... no problem there, it wasnt from the blinking curosr. So this happens when the address starts with http. Maybe this is related to specific letters which pixels are touching the edges because of the ClearType.

Comment 12

[Jonathan Kew [:jfkthame]]

Yes, I'm sure (well, virtually sure!) that's the key factor. ClearType sometimes produces colored antialiasing pixels that "bleed" outside the nominal bounding box of the glyph, as reported by Windows GDI functions, and this means that they get painted on areas that we don't necessarily invalidate and repaint when the text moves.

I've just provided a patch for a related issue within the Cairo graphics library (bug 445087), but in that case the symptom was clipped glyphs because the "bleeding" pixels were not being drawn at all in certain circumstances. I think this problem will require a similar patch but in a different place.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Not a blocker (pretty sure it happens in 3.0) but should be fixed as per previous comment by another bug. Please check back shortly to see if that does it, renom if it turns out that this is a regression.

Comment 14

[Jonathan Kew [:jfkthame]]

Correct, this is not a regression; I have just tried the test case with FF 3.0.5 and seen the same rendering glitches. (Note however that it does seem to be intermittent; with recent trunk builds of Minefield on XP, I was able to reproduce on one machine but not on another.)

The patch for bug  445087 does *not* resolve this issue; the underlying cause is related but the fix will be elsewhere. Although it's clear that the problem arises because of the "bleeding" of antialiasing pixels outside the glyph bounding box when ClearType is used, I'm not sure exactly where in the layout/thebes/cairo world this needs to be handled, so no patch is available yet for this bug.

Comment 15

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

OK Jonathan, since you can reproduce this, it's all yours :-).

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: test file to reliably demonstrate the ClearType rendering glitch

This is a modified version of the original testcase, and makes it possible to reliably reproduce the bug on any XP or Vista machine with ClearType enabled (I believe!), using either Firefox 3.0.x or a current trunk build of 3.1.

It turns out that the effect is critically dependent on the exact (fractional-pixel) horizontal position of the text within the window. The original testcase had "auto" margins, and therefore the result was dependent on the width of the window when testing; that's why it appeared to be intermittent, or would appear on one machine but not another.

This new test file offers three stylesheet settings, with left margins of 260, 260.5, and 261 px. To reproduce the problem:

* Open the tahoma-test.htm file with Firefox. The initial style will have a 260px margin. Ensure the zoom factor is reset to the default; this issue is sensitive to page zoom.

* Click the "Click Here" link; a hidden <div> will be revealed, sliding the text "Enter" downwards, and leaving ClearType artifacts on the screen. Click again to slide it back up again.

* Now use View / Page Style to select the stylesheet with 260.5px margin. Note that the text shifts one pixel to the right.

* Now click the "Click Here" link again. Note that this time the "Enter" text does not leave any ClearType artifacts. (There will be a residual artifact beside both lines of text, actually, from the rightwards shift when the style was changed. That's another manifestation of the same problem. It can be removed by opening and closing the Tools menu, which covers this area of the window, causing it to be repainted. After this, the "Enter" text can be toggled up and down cleanly with no visual artifacts.)

* Now select the stylesheet with 261px margin. Note that this time, the text does not shift rightwards when changing the style; 260.5px and 261px result in the exact same text position on screen.

* However, using "Click Here" to move the "Enter" text downwards, the artifacts will reappear.

So to summarize, the problem appears when the left margin of the text is an integral number of pixels; it does not appear when the left margin falls on a half-pixel value, although it seems that the actual rendering of the text then gets rounded to the next whole-pixel position.

I guess an additional factor might be the Windows font "dpi" setting; I haven't tried altering this. All testing was carried out with the default (96dpi) setting.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: ensure ClearType pixels will be erased when text changes

The problem arises because during reflow, when a text frame has moved or changed its contents, we use a bounding box based on metrics from gfxFont::Measure to determine the area that needs to be invalidated and repainted. However, because of ClearType "pixel bleed" (see also bug 445087), there may be pixels outside the nominal "ink box" of the glyphs in the run.

To solve this, the patch overrides Measure() in gfxWindowsFont to add "padding" to each side of the bounding box, so that the ClearType pixels will be included in the resulting dirtyRect.

I'll run some more tests with this before requesting review, but so far it's looking good. (BTW, note that these stray ClearType pixels can be left at the right-hand edge of the text as well as the left, especially with right-aligned text, but they're usually harder to see as they tend to be very pale blue pixels.)

Comment 18

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

If this patch causes nsTextFrame::Reflow to add an overflow area for many frames (round about here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#6068
) that extends outside the normal width/height, that would be very bad.

Comment 19

[Jonathan Kew [:jfkthame]]

It does indeed. I'm not sure how that can be avoided, as the issue here is that ClearType causes glyphs to touch pixels that fall outside of their normal bounding box, either to the left of the glyph origin or beyond the advance (or both).

I've just pushed this patch to the tryserver, to see what impact it has there. I know you are concerned about the possible performance issues, but I'm curious to see what actually happens. Note that in most cases we must be repainting the relevant pixels anyway, as the artifacts don't show up. The question is whether this will cause an additional, redundant repaint of adjacent frames. If we're handling dirty regions well, collecting them all before repainting, it may not be the disaster you're expecting.

Comment 20

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

I'm partly worried about the memory impact and performance overhead of managing lots of overflow-area properties. Not only will text frames get them, but probably also inline frames and block frames up the frame tree. There's a hash table that maps from the frames to heap-allocated rectangles.

Comment 21

[Jonathan Kew [:jfkthame]]

Tryserver WINNT 5.1 talos log at

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1233836465.1233840691.21258.gz

I don't have the experience to know how to interpret the figures here, though.... obviously there's some "random" variation to be expected, but what constitutes a significant problem?

Comment 22

[Karl Tomlinson (:karlt)]

Tryserver wasn't able to detect any statistically significant change in Tp:

http://graphs-stage.mozilla.org/graph.html#show=117696&sel=1233653471,1233865911

or Mem-WS:

http://graphs-stage.mozilla.org/graph.html#show=117702&sel=1233653471,1233865911

The index of 475968-cleartype looks like 1233830040.

Comment 23

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

That's good, but I'm reluctant to take it just because it doesn't cause a statistically significant change in those tests.

One thing that would be useful is to instrument the code to measure the number of textframes that overflow before and after this patch, say running the Tp3 pageset. We could also measure the same thing for all frames via FinishAndStoreOverflow. If that increase is small, then I'm not worried.

Comment 24

[Jonathan Kew [:jfkthame]]

It definitely causes "many" more textframes to overflow, as (virtually) every textframe will now have a negative left sidebearing, whereas very few did previously. It's hard to see how to avoid this with ClearType enabled, as it does frequently color "overflow" pixels, and that's what leads to the visual artifacts we're trying to eliminate here. But I'll do some measurements to see what the real impact is.

One thing we can do if it seems worthwhile is to test whether ClearType is currently enabled, and only add the extra pixels in that case, so that we don't take the hit when it's turned off.

Comment 25

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

It would be interesting to see the effect on loading a big document like the HTML5 spec, especially in memory use.

Comment 26

[Jonathan Kew [:jfkthame]]

Some figures from loading the HTML 5 spec (the all-in-one version).... the exact figures will vary a bit depending on things like window size, but these should give an idea.

Current trunk code, without anti-ClearType-clipping patch:
  calls to FinishAndStoreOverflow:      142174
  # needing to set an overflow area:     27511  (19%)
  total # of overflow rect allocations:   9989  (7%)
(I presume that in some cases the frame already has an overflow-rect property, so no new allocation is needed.)

Interestingly, applying the Cairo patch from bug 445087 actually reduces the total number of calls to FinishAndStoreOverflow (I'm not sure why), although it does not affect the overall number of overflow-rect properties that get allocated:
  calls to FinishAndStoreOverflow:      137374
  # needing to set an overflow area:     22701  (17%)
  total # of overflow rect allocations:   9989  (7%)

Adding the patch here, as expected, causes a big jump in the number of overflow rects:
  calls to FinishAndStoreOverflow:      141877
  # needing to set an overflow area:    124300  (88%)
  total # of overflow rect allocations: 106372  (75%)

Although this didn't seem to make a significant performance difference on the try server, I wondered about the whole principle of storing the overflow rect as a property, involving a hash-table and a separate allocation. Suppose we made it a direct member of nsIFrame instead?

To see what might happen if we moved the overflow area into the frame itself, I pushed a patch to the try server that implements this change; see the builds labelled "frame-overflow". Overall, it doesn't look as though there is a significant impact on either performance or memory usage from doing this; but the advantage it gives us is that changing from 10-20% of frames having overflow areas to 80-90%, as a result of ClearType "pixel bleed", will not be a problem.

Comment 27

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

Thanks!!!

Certainly, if most frames are going to have overflow areas, we should put the overflow area inline into the frame. But I will note that any way we add these overflow areas would mean adding at least 1.6MB of footprint for loading the HTML5 spec testcase, which is something I'd like to avoid.

> Interestingly, applying the Cairo patch from bug 445087 actually reduces the
> total number of calls to FinishAndStoreOverflow

Possibly just noise.

Here's an idea: wherever we retrieve an overflow rect for dirty-rect testing or invalidation, if it's not a frame that clips its children, add one pixel in each direction around that overflow rect before we use it. In nsTextFrame, if the glyph extents extend no more than one pixel outside the frame bounds, we can then avoid storing the overflow rect (and its ancestors will avoid having to have an overflow rect too).

Comment 28

[Jonathan Kew [:jfkthame]]

(In reply to comment #27)

> Here's an idea: wherever we retrieve an overflow rect for dirty-rect testing or
> invalidation, if it's not a frame that clips its children, add one pixel in
> each direction around that overflow rect before we use it. In nsTextFrame, if
> the glyph extents extend no more than one pixel outside the frame bounds, we
> can then avoid storing the overflow rect (and its ancestors will avoid having
> to have an overflow rect too).

Yes, I've been wondering about possible ways to optimize the common cases. This sounds like it could work (actually, we need two pixels, at least on the RHS, but that's a minor detail).

Another approach I wondered about, which ought to avoid rect allocation in even more cases, would be to add a single word to the frame itself, and use this to store small overflow values for the 4 edges of the box (one PRUint8 each). Then we'd only have to do an allocation and use the property hash in the (probably extremely rare) case of an overflow that exceeds the one-byte deltas we can store directly.

For the HTML5 example, adding 4 bytes to every frame will use 0.4MB of memory; but on the other hand, avoiding the need to allocate full-sized rects on the heap, as well as the hash-table overhead, will win back a good chunk of that, and we'll be doing fewer allocations overall which is usually a good thing.

I can try instrumenting the code to see what overflow amounts are actually occurring (besides the 1- and 2-pixel ones that we have to add at some level for ClearType), to get a better idea of how this would really work out. (A variant on this, if it turns out that virtually all overflows are horizontal but some of them are large, would be to store 16-bit overflow deltas for left and right, and use the actual allocated rect whenever there is vertical overflow.)

Comment 29

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

That sounds like a good plan. Run with it!

Comment 30

[Jonathan Kew [:jfkthame]]

I've done some browsing with an instrumented build to see how big overflow areas really are, including loading the HTML5 spec as well as some general web use (mozilla.org, bbc.co.uk, wikipedia, etc) and the big page_load_test set. This is *without* the additional small overflows that we'll need (at least conceptually) for the ClearType issue.

I also instrumented the frames to see how many were actually in existence at any one time. The big single-page HTML5 spec is atypical, in that it creates around 100,000 frames; during the entire page_load_test suite, the maximum number of frames never reached as high as 12,000.

Classifying overflows in FinishAndStoreOverflow into "small" and "large", where "small" overflows do not exceed 255 app units in any direction and therefore could be stored directly in the frame for a cost of only one 32-bit word, I'm finding that somewhere between 50% and 85% of frames with overflow fit within the "small" category, depending on the site.

Lots of these "small" overflows also occur on-the-fly during user interaction, e.g, as the mouse drifts over links or other significant regions of the page.

(BTW, if we were to give 8 bytes in the frame to overflow deltas, so that "small" overflows could be up to 64K app units in each direction, then only a residue of less than 1% of overflows would have to be heap-allocated. But overall I think there's less value in that than just handling the one-byte values directly in the frame, at a cost of only 4 bytes per frame altogether. Yet another option would be to store these values in device pixels (rounded up) instead of app units, which would make many more of them fit, but that would cost us extra calculation whenever we need to access them, so might hurt performance marginally.)

I have a patch for this mostly ready, but currently it's causing a couple of odd reftest failures that I need to resolve.

Comment 31

[Jonathan Kew [:jfkthame]]

Attachment: patch to store small overflow areas as deltas within the frame

This is not quite as simple as I would have liked, because of the possibility that the frame's bounding rect is changed after the overflow area has been stored. In this case, if the overflow area was stored as small deltas from the frame, it would be incorrectly changed. So we have to catch this situation and update the deltas (or switch to a full-size allocated rect property).

Just pushed this to the tryserver to check for any performance impact.

Assuming this patch looks ok, we should be able to add the ClearType padding pixels with minimal impact on memory use, as they will normally fit within the "small overflow" range.

Comment 32

[Jonathan Kew [:jfkthame]]

Comment on attachment 362436 [details] [diff] [review]
patch to store small overflow areas as deltas within the frame

requesting review on these two patches, as this seems to resolve the issue and avoids a heavy memory cost; however, before landing we also need to deal with the effect on SVG and MathML tests, so there will be additional patches needed

Comment 33

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

+   * When we change the frame's bounding rect, we may need to fix up the overflow rect

border-box rect, not bounding rect

It seems like with this approach we can stop using the NS_FRAME_OUTSIDE_CHILDREN state bit, just check for 0,0,0,0 in mOverflowDelta. That would free up that state bit, which is pretty useful, so we should do it.

CCing dbaron for comments since this is quite a core change.

Some of your lines are beyond 80 chars.

+    else
+      mRect = aRect;

These should have {}

+  void SetPosition(const nsPoint& aPt) {

I don't think this needs to change, since the overflow areas are always relative to the position.

+nsIFrame::SetOverflowRect(const nsRect aRect)

Should be const nsRect&

+  PRUint32 l = mRect.x - aRect.x,
+           t = mRect.y - aRect.y,
+           r = aRect.XMost() - mRect.XMost(),
+           b = aRect.YMost() - mRect.YMost();

Hmm, we really shouldn't be subtracting aRect.x/y here...

Comment 34

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

Comment on attachment 360624 [details] [diff] [review]
ensure ClearType pixels will be erased when text changes

I think I prefer to do this in cairo's computation of the ink rect; i.e. the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=445087#c69

Does that patch fix this bug?

Comment 35

[Jonathan Kew [:jfkthame]]

No, it doesn't (see comments #12 and #14). Which is a little odd; it suggests that we may not be propagating the extents properly in all cases.

Aha! I think that the glyph-extents patch from bug 445087 *in combination with* turning on the "quality" rendering path (by setting browser.display.auto_quality_min_font_size to 1 instead of the default 20) may be the answer. Works for me so far, anyhow. The trouble is that otherwise, with small text sizes (which is exactly where the ClearType "bleed" is most likely to be a problem), we ignore the padded extents from Cairo and so that patch doesn't help us here.

So perhaps what we could do (besides fixing bug 445087 by padding the glyph extents in Cairo -- needed for its own internal drawing anyway) is to detect when ClearType is enabled, and always use the "quality" rendering path in this case even for small font sizes. Any idea what the performance cost of that is like? I could do some tests but it'll take a while.

Comment 36

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

Actually something I've been meaning to do but haven't gotten around to is to push a patch to the try-servers to see what the perf impact of setting that pref to 0 is. So I just pushed that patch and we should get some results here:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

I think if we were going to use the quality rendering path for Cleartype, always, then we might as well use it on all platforms always. But if the perf isn't good we still have a couple of options:
a) Get glyph extents always, but don't shape always
b) take a version of your current patch that pads the extents in our code if we're not calling cairo to get the real extents.

Comment 37

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

Interesting results.

On Windows, Ts/Twinopen/Tp all look unchanged ... which I find somewhat suspicious. But there's a clear jump in tp_memset: 65-66MB to 71MB.

On Mac, it looks like a clear Tp hit, from around 300 to 317. Mac is already doing shaping so that hit must be due to the work to get glyph extents. No significant memory change.

On Linux, Tp has risen from the mid-230s to just over 240. Probably a small Tp_RSS increase, from around 83MB to 85MB.

Probably the easiest way to go here is option b) in comment #36.

Comment 38

[Jonathan Kew [:jfkthame]]

(In reply to comment #33)

> +  void SetPosition(const nsPoint& aPt) {
> 
> I don't think this needs to change, since the overflow areas are always
> relative to the position.

They're closely associated, of course. But if our code were ever to update the overflow area *before* updating mRect via this method, we'd lose, as we might store the overflow as delta values from the old mRect. Then we call SetPosition to move mRect to its proper place, and this changes the overflow area from the value we just stored to a shifted version of it.

We may not have any code path in layout that would currently do this, but allowing SetPosition to change mRect without preserving the effective overflow area would mean that the behavior depends on whether the overflow area happened to be stored as deltas (so that SetPosition moves it along with mRect) or as a separate allocated rect (in which case SetPosition doesn't touch it). This sounds to me like a latent bug waiting to happen.

Actually,  I think that in practice we almost always set the overflow area right after updating mRect. In an early version of the patch I didn't touch any of the SetRect/SetSize/SetPosition methods, and there were only a handful of resulting failures in the whole reftest suite. But there *were* those few failures, and it's clearly fragile to make assumptions the order of setting those rects, so I made all three setters safe.

> +  PRUint32 l = mRect.x - aRect.x,
> +           t = mRect.y - aRect.y,
> +           r = aRect.XMost() - mRect.XMost(),
> +           b = aRect.YMost() - mRect.YMost();
> 
> Hmm, we really shouldn't be subtracting aRect.x/y here...

Why not? If we're going to store aRect as a set of deltas (offsets) from the edges of mRect, we need to find the difference between them.... that's the delta for that edge. Or am I completely missing something?

Comment 39

[Jonathan Kew [:jfkthame]]

(In reply to comment #38)
> > +  PRUint32 l = mRect.x - aRect.x,
> > +           t = mRect.y - aRect.y,
> > +           r = aRect.XMost() - mRect.XMost(),
> > +           b = aRect.YMost() - mRect.YMost();
> > 
> > Hmm, we really shouldn't be subtracting aRect.x/y here...
> 
> Why not? If we're going to store aRect as a set of deltas (offsets) from the
> edges of mRect, we need to find the difference between them.... that's the
> delta for that edge. Or am I completely missing something?

Or did you mean that the sign seems inverted, it should be aRect.x - mRect.x, etc? That should be commented, I guess. Because the overflow area is always bigger than mRect (if they're not identical), the idea is to store *unsigned* "outward deltas", not signed differences, so that more of the small overflow areas will fit within the single-byte values. So the left/top delta values are positive to the left and upwards respectively, while the right/bottom ones are positive to the right and downwards.

If we do ever try to store an overflow area that's smaller than mRect, so that any of these deltas would be negative, then because of the use of unsigned variables, they'll appear as very large positive values, they won't fit within the "small" range that we store as deltas, and we'll simply store the overflow area as a separate rect, so it's all still OK.

Comment 40

[Jonathan Kew [:jfkthame]]

Attachment: updated patch eliminating NS_FRAME_OUTSIDE_CHILDREN flag

This version of the patch frees up the NS_FRAME_OUTSIDE_CHILDREN flag bit, as suggested, replacing it with an accessor HasOverflowRect() that the layout code can use whenever it wants to know; this hides the implementation of overflow areas as either small deltas within the frame or a separate allocated rectangle.

See comments #38 and #39 regarding other review notes.

Comment 41

[Jonathan Kew [:jfkthame]]

Uploaded this patch to the try server today, and results look fine, no sign of an impact on either performance or memory use. (See builds labelled "overflow-deltas".)

Once we're happy with this change, we'll be able to fix the original ClearType drawing glitch without a significant memory cost for the extra overflow areas that will cause.

Comment 42

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

(In reply to comment #38)
> They're closely associated, of course. But if our code were ever to update the
> overflow area *before* updating mRect via this method, we'd lose, as we might
> store the overflow as delta values from the old mRect. Then we call
> SetPosition to move mRect to its proper place, and this changes the overflow
> area from the value we just stored to a shifted version of it.

But the overflow rect is *already* stored as a rectangle relative to the frame position, so that code would already be wrong.

> We may not have any code path in layout that would currently do this, but
> allowing SetPosition to change mRect without preserving the effective overflow
> area would mean that the behavior depends on whether the overflow area
> happened to be stored as deltas (so that SetPosition moves it along with
> mRect) or as a separate allocated rect (in which case SetPosition doesn't
> touch it). This sounds to me like a latent bug waiting to happen.

The separately allocated rect is always already relative to the frame position, so there is no latent bug here regarding SetPosition.

> > +  PRUint32 l = mRect.x - aRect.x,
> > +           t = mRect.y - aRect.y,
> > +           r = aRect.XMost() - mRect.XMost(),
> > +           b = aRect.YMost() - mRect.YMost();
> > 
> > Hmm, we really shouldn't be subtracting aRect.x/y here...
> 
> Why not? If we're going to store aRect as a set of deltas (offsets) from the
> edges of mRect, we need to find the difference between them.... that's the
> delta for that edge. Or am I completely missing something?

I think you're missing that aRect is already relative to mRect's top-left. This should just be l = -aRect.x, t = -aRect.y, r = aRect.XMost() - mRect.width, b = aRect.YMost() - mRect.height.

For example, look in nsIFrame::FinishAndStoreOverflow, where we compare *aOverflowArea to nsRect(nsPoint(0, 0), aNewSize)).

Although I'm not sure how tests are passing with the code the way it is.

Comment 43

[Jonathan Kew [:jfkthame]]

Ohhhh..... ok, so the overflow rect is stored in frame-based coordinates, not in the coordinate system of whatever contains the frame. Yes, that should have been obvious now that I look again!

That's fine, I'm just building and testing an updated patch. With this correction in SetOverflowRect, we need a corresponding change in GetOverflowRect also. It was passing tests before because this Set/Get pair were both using the same mistaken understanding, so (except perhaps in strange corner cases where the frame is changing?) they would cancel out each other's misunderstanding. :)

Comment 44

[Jonathan Kew [:jfkthame]]

Attachment: corrected coordinate system of the overflow rect

Would it make sense to try and land this "foundational" patch first, to make sure it sticks, before adding the patch to fix the original ClearType drawing glitch?

Comment 45

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

Comment on attachment 364050 [details] [diff] [review]
corrected coordinate system of the overflow rect

+  }
+  else {

one line

+  struct {
+    PRUint8 left;
+    PRUint8 top;
+    PRUint8 right;
+    PRUint8 bottom;
+  }                mOverflowDelta;

These should be mLeft etc ... sorry

Comment 46

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

(In reply to comment #44)
> Would it make sense to try and land this "foundational" patch first, to make
> sure it sticks, before adding the patch to fix the original ClearType drawing
> glitch?

Sure.

David, are you happy with this change? It's small but feels major :-).

Comment 47

[Jonathan Kew [:jfkthame]]

Attachment: fixed nits from review (comment #45)

Comment 48

[Emil Ivanov]

Does this fix the issues at the location bar and status bar too?

BTW STR for status bar issue (just in case):
1. After a page is loaded and when status bar is with text Done open new tab

Result:
There is vertical line on the new tab's status bar

Comment 49

[Jonathan Kew [:jfkthame]]

Attachment: updated to pad bounds only if antialiasing is enabled

Updated for current trunk; also, this version of the patch will add "padding" pixels to the LOOSE_INK_EXTENTS bounds only if antialiasing is enabled.

This is the "conservative" approach of comment #36 (b), not turning on quality rendering globally but just padding the extents if we're not getting real ones from Cairo.

Note that these patches need to land together, or the frame-overflow one first, otherwise we're likely to get some talos regression from adding overflow areas all over the place.

Comment 50

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

We need to add the padding for TIGHT_INK_EXTENTS as well, right?

Comment 51

[Jonathan Kew [:jfkthame]]

No, because in that case we're getting the "real" extents from Cairo, which it will have already padded (bug 445087).

Comment 52

[Jonathan Kew [:jfkthame]]

Remember that an alternative fix for this patch would be to simply use TIGHT_INK_EXTENTS in place of LOOSE, always. That also resolves the drawing problems, but it has a (slight) measurable performance impact (around 1-2% on Tp, AFAICT), as it's more expensive to get the real extents.

Comment 53

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

Right OK.

+        metrics.mBoundingBox.size.width += 2 * appUnitsPerDevUnit;

Shouldn't this be 3*appUnitsPerDevUnit? I thought we could have one pixel on the left and two pixels on the right?

Comment 54

[Jonathan Kew [:jfkthame]]

Oops, yes... I'll fix that.

BTW, there are very occasional instances where antialiasing also touches a pixel outside the *vertical* bounds of the ink rect Windows gives us, so in theory we should also pad up and down. However, these are so rare (as far as I've seen yet, anyhow) that I really don't want to do that. We may need to work around this in a few reftests, though; I have seen a couple that can fail on Windows because of such a pixel. Any opinion on this - pad the bounds to ensure we'll never clip these, even though in 99%+ of cases our bounds will then be excessive, or ignore the tiny glitches and tweak the tests where necessary?

Comment 55

[Jonathan Kew [:jfkthame]]

Attachment: updated to add 2 pixels on RHS

Comment 56

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

Adding vertical padding doesn't cost us anything really, does it? Why not do it?

Comment 57

[Jonathan Kew [:jfkthame]]

The main place it's relevant is actually in bug 445087, not here, as the loose extent usually has whitespace top and bottom anyway. So if we want to consider it, we'll need to revisit the cairo patch; that's the one that would actually affect the reftests I've seen.

I'm a bit hesitant to do that, though, as my gut feeling is that it'll be a lot harder to convince cairo upstream to accept the addition of pixels above/below when the cases where this actually occurs are *extremely* rare, afaict. I haven't yet seen an instance with more than one very faint pixel, and nothing that's actually visible to the casual viewer. Given that they're using these extents to determine the area they need to paint (e.g. when compositing), they may well be reluctant to include unnecessary pixel rows in almost all cases.

Comment 58

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

OK, then we should discuss that upstream.

Comment 59

[Jonathan Kew [:jfkthame]]

I'm currently getting two reftest failures on Linux, apparently as a result of the frame-overflow-storage patch; trying to figure out why, as we really need to get this landed (but it's no use if it breaks tests!).

The failures are
    layout/reftests/mathml/non-spacing-accent-1.xhtml
    layout/reftests/bugs/412352-1.html 
The first of these is definitely a problem; the accents simply don't appear. (I wonder if this is related to bug 479276 in some way, although that patch by itself didn't cause the failure.) Not sure what to make of the second; reading bug 412352, it sounds like something related to overflow areas that was not clearly understood at the time... :(

Comment 60

[Jonathan Kew [:jfkthame]]

The first reftest failure mentioned is due to an error in the patch (GetOverflowRect() doesn't actually compute the rect properly); I will update the patch after running full reftests with the corrected version.

Haven't got to the root of the second test failure yet.... it doesn't appear to be related to the use of the deltas to store the overflow. Probably something to do with the elimination of the NS_FRAME_OUTSIDE_CHILDREN flag; I'll look through all those changes again.

Comment 61

[Jonathan Kew [:jfkthame]]

Attachment: fix the calculation in GetOverflowRect, resolves mathml reftest failure on linux

Comment 62

[Jonathan Kew [:jfkthame]]

Attachment: updated overflow-rect storage patch resolving the last Linux reftest failure

Solved the last reftest failure on Linux from the overflow-rect-as-deltas patch.

It turns out that there is a situation involving abs-positioned elements where we explicitly store an overflow rect that is identical to the current border rect of the frame, and need to be able to retrieve this overflow rect later after the frame's own rect (mRect) has been resized to zero width/height. So we have to be able to distinguish the case where there is no stored overflow rect at all (represented by all delta values of zero, as that's the initial creation state of frames) from the case where there's an "overflow" rect that happens to match mRect at the time it is stored. And we have to make sure we do actually store such a rect, and not optimize away that operation on the basis that the rect we're considering for storage matches the current return value of GetOverflowRect() - which might not actually be a stored rect at all.

Marking this for re-review as the use of the mOverflow fields in the frame has been revised significantly in order to support this, although the basic idea remains the same. The important change is that it now knows the difference between the absence of an overflow rect (GetOverflowRect() will return a rect the size of mRect, but HasOverflowRect() will return false) and an explicitly-stored overflow area of that same size (HasOverflowRect() will return true, and therefore the overflow area will be maintained properly if the frame itself is subsequently resized).

Comment 63

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

Can't we handle this abs-pos quirk just by providing a way to force the creation of a rect property that happens to denote the border-box? Then when the size changes to zero via SetRect or SetSize, GetOverflowRect will still return the old border-box.

Comment 64

[Jonathan Kew [:jfkthame]]

Yes, that would also work - in fact, that was the way I first did it, once I figured out what was going on. But as we were already using a special "flag" value in the deltas (to indicate when the overflow is stored as a separate rect property), this version seemed tidier to me (as well as perhaps slightly more efficient, though I'm sure that wouldn't be significant).

No matter exactly how we implement the details, there's a certain amount of "trickiness" here because we're squeezing the functionality of the old NS_FRAME_OUTSIDE_CHILDREN flag, as well as a new flag telling us whether the overflow is actually stored as compact deltas or a full rect property, into the same storage space in the frame as the delta values themselves. This is desirable to minimize the RAM footprint of thousands of frames with overflow areas. I've tried to be pretty explicit with comments, so that it shouldn't be too hard to understand what's being done.

Comment 65

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

The problem is that the distinction between EXACT and NONE is quite unclear. I'd prefer to avoid that problem by just not having EXACT. It's also less code AFAICT.

Comment 66

[Jonathan Kew [:jfkthame]]

The only code saving I can see would be the elimination of one test in GetOverflowRect:

+  if (mOverflow.mType == NS_FRAME_OVERFLOW_EXACT) {
+    // the overflow rect exactly matches the frame's border rect
+    // (but was explicitly set, not entirely absent as would be
+    // implied by all-zero deltas)
+    return nsRect(0, 0, mRect.width, mRect.height);
+  }

In SetOverflowRect, we'd still have to check for the all-zeros case, it would just lead to a different result (allocating a rect property instead of storing the EXACT value). This could be combined with the existing test for values small enough to store as deltas, but it's not really a significant code savings, it just moves the complexity by a few lines.

The other side of the equation is that if there are circumstances where we store a substantial number of these "overflow" rects that are the same as the mRect, we'll be paying for the extra properties. (I don't know if there are other situations where this would happen, besides the abs-positioned one that led to the reftest failure. They wouldn't necessarily show up in most tests.) But it's probably not a big deal; I'll remove EXACT if you prefer it that way.

(Would renaming it to something like ....MATCHES_FRAME_RECT be less unclear?)

Comment 67

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

(In reply to comment #66)
> The other side of the equation is that if there are circumstances where we
> store a substantial number of these "overflow" rects that are the same as the
> mRect, we'll be paying for the extra properties. (I don't know if there are
> other situations where this would happen, besides the abs-positioned one that
> led to the reftest failure. They wouldn't necessarily show up in most tests.)

I'm not too worried about that. At least it's not a regression.

> (Would renaming it to something like ....MATCHES_FRAME_RECT be less unclear?)

Not really. The problem is having two cases that both mean "the overflow rect equals the border-box".

It's actually a fairly nasty (existing) quirk of the overflow rect system that overflow-rects-as-properties don't change when the frame size changes, but when there's no property the overflow rect changes when the frame size changes. Something to think about. We might want to push towards a model where they are always set simultaneously. Not in this bug though :-).

Comment 68

[Jonathan Kew [:jfkthame]]

Attachment: eliminated NS_FRAME_OVERFLOW_EXACT setting as requested in roc comments

OK, updated the patch as suggested. I'll send this version back to the try-server; not expecting any problems but it doesn't hurt to check.

Comment 69

[Jonathan Kew [:jfkthame]]

Hmmm. Try-server unit test reported a small amount of leakage. However, I have not been able to reproduce this locally, and I see a lot of other unit-test orange on the try-server around the same time, so I'm suspicious that this was really a problem on mozilla-central trunk at the time, not due to the actual patch being tested. I'm waiting for a fresh try-server run to see if it's repeatable.

Comment 70

[Jonathan Kew [:jfkthame]]

Second tryserver run failed one crashtest (but no leak reported by a11y this time). I don't think either of these problems are actually caused by this patch, it looks to me like it just depends where the tryserver build happens to catch the trunk.

Do you want to re-review this, or should we mark it as ready for checkin and try landing it when there's an opportunity?

Comment 71

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

Comment on attachment 366956 [details] [diff] [review]
eliminated NS_FRAME_OVERFLOW_EXACT setting as requested in roc comments

Lets land this.

Comment 72

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

Pushed attachment 366956 [details] [diff] [review]: http://hg.mozilla.org/mozilla-central/rev/9ff3a933d89f

I guess attachment 365540 [details] [diff] [review] still needs to land.

Comment 73

[Jonathan Kew [:jfkthame]]

(In reply to comment #72)

> I guess attachment 365540 [details] [diff] [review] still needs to land.

Right; the original plan was to land the overflow-rect patch first and make sure it sticks, before the glyph-extents patch that will cause lots of additional overflow rects to be created. Once 366956 has baked for a few days, we should land the other as well.

BTW, bug 476927 shows a similar problem with Quartz antialiasing on OS X, though the visual problems are much less glaring. But the fundamental issue of needing to allow for "overflow" of antialiasing pixels that go beyond the glyph's bounds (as reported by the host font system) is the same, and the fix will lead to creation of lots more overflow areas on OS X as well.

Comment 74

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

It seems to have stuck, but I'll give it a few more days.

Comment 75

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

http://hg.mozilla.org/mozilla-central/rev/994ecd62b880

Can we have a reftest here that fails on at least some Windows systems?

Comment 76

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

This seemed to cause test failures on Tinderbox:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239442982.1239454469.23733.gz#err2

So I backed it out:
http://hg.mozilla.org/mozilla-central/rev/abad3a76f2d4

Comment 77

[Jonathan Kew [:jfkthame]]

Attachment: variant of reftest 308406-1 to demonstrate the failure on OS X also, independent of this bug

The reftest failure on bugs/308406 isn't really a new issue with this patch, it is just bringing a latent issue to the surface. (Wonder why it didn't show up for me locally, though? Font differences?)

The failure arises with "overflow: auto" when layout decides the table content doesn't fit within its frame, and adds a horizontal scrollbar. Enlarging the glyph extents to allow for ClearType triggers this, but it can happen elsewhere too. This attachment is a variant of the reference file from the reftest, illustrating the issue on Mac OS X as well. View the attachment in Minefield on OS X, and try zooming in; note how the scroll bar appears and disappears at different zoom levels. (Also happens with Firefox 3.0.5; this is not a new behavior.)

For the sake of re-landing the ClearType fix, we can probably revise the bug-308406 reftests (perhaps by selecting sans-serif text, and/or modifying the actual text content) to make them less fragile. But in general, is this behavior a bug that we should track and try to fix? I'm inclined to think that it is; are we maybe a bit confused about whether to include overflow areas or not in the dimensions we're using?

Comment 78

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

I've actually encountered this problem before with other tests. The immediate fix is to add some 'padding' to the overflow:auto element and the reference.

The ultimate fix is to split "overflow areas" into two different areas: the "visual overflow area" containing all the pixels changed by the frame subtree, and the "scrollable overflow area" which is the union of the border-boxes of the frames in the frame subtree. Karl is signed up for that.

Comment 79

[Jonathan Kew [:jfkthame]]

Attachment: added padding to <td> in reftests for bug 308406, to avoid spurious failures

AFAICT, this should fix the reftest orange that happened when this patch landed previously.

Comment 80

[Jonathan Kew [:jfkthame]]

(In reply to comment #78)
> The ultimate fix is to split "overflow areas" into two different areas: the
> "visual overflow area" containing all the pixels changed by the frame subtree,
> and the "scrollable overflow area" which is the union of the border-boxes of
> the frames in the frame subtree. Karl is signed up for that.

Great; that definitely sounds like it needs doing. It may also help us get a better handle on things like bug 480255, as that arises when we start confusing the visual area touched during painting with the structural area needed for layout purposes.

Comment 81

[Jonathan Kew [:jfkthame]]

Attachment: possible reftest for the cleartype rendering issue

This is a reftest based on the already-posted test case. Note that it relies on a script from http://ajax.googleapis.com/ to do the animation that leaves visual artifacts, so it's not properly standalone. I'm sure someone familiar with JS and dynamically manipulating the document could remove that dependency and just use a small in-document script instead....

This test consistently fails on Vista (and probably XP) with ClearType enabled unless the glyph-extent-padding patch (i.e., the patch in this bug) is applied.

Comment 82

[Jonathan Kew [:jfkthame]]

Attachment: update the reftest to eliminate dependency on google ajax script

Comment 83

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

Jonathan, would it be possible to rewrite this reftest to use just one step and MozReftestInvalidate?

http://weblogs.mozillazine.org/roc/archives/2009/01/invalidation_re.html

That would probably make it more reliable, and also faster.

Comment 84

[Karl Tomlinson (:karlt)]

rob checked this in
http://hg.mozilla.org/mozilla-central/rev/06a7d2def034

but it looked like it may have been causing

44188 ERROR TEST-UNEXPECTED-FAIL |
/tests/layout/base/tests/test_bug450930.xhtml | scrolled-out invalidation
should notify in subdoc

This may have been bug 483218, but it happened on the first run on NT and on OS X (re bug 476927), which seemed too coincidental given the frequency of occurrences of that bug, and we weren't able to be around to watch the tree any longer, sorry, so it got backed out:

http://hg.mozilla.org/mozilla-central/rev/be99fcdb6add

Comment 85

[Jonathan Kew [:jfkthame]]

Attachment 383263 [details] [diff] (see bug 476927, which is essentially the same issue on Mac OS X) should resolve the issue with test_bug450930.

Comment 86

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

http://hg.mozilla.org/mozilla-central/rev/b6d407af386f

I forgot to check in the reftest. Maybe you can do that when you get checkin access :-).

Comment 87

[Sylvain Pasche]

This caused bug 503718

Comment 88

[Jonathan Kew [:jfkthame]]

Backed out due to bug 503718:
http://hg.mozilla.org/mozilla-central/rev/17a10b614f99

Comment 89

[Jonathan Kew [:jfkthame]]

Sorry, wrong changeset in the previous comment. The backout is actually
http://hg.mozilla.org/mozilla-central/rev/980e61b2e20b

Comment 91

[Jonathan Kew [:jfkthame]]

Attachment: add padding pixels to loose ink bounds on Windows because of ClearType pixel bleed

Now that bug 542595 has landed, we should be able to fix this (like bug 476927, the Mac equivalent). Updated patch to apply to current trunk.

Note that this patch is only for the GDI font back-end; the problem does not seem to occur with DirectWrite, or at least not as severely as with GDI/ClearType rasterization.

Comment 92

[Emil Ivanov]

(In reply to comment #91)

> Note that this patch is only for the GDI font back-end; the problem does not
> seem to occur with DirectWrite, or at least not as severely as with
> GDI/ClearType rasterization.

Yes i can confirm that there is no problem with DirectWrite, which is great.

Comment 93

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

Jonathan, can we land this now?

Comment 94

[Jonathan Kew [:jfkthame]]

Attachment: patch, add padding pixels to loose ink bounds on Windows because of ClearType pixel bleed

Rebased to current trunk; also added a reftest manifest update because this resolves bug 623403 and thus leads to 4 "unexpected pass" messages on WinXP.

I propose to land this (and its MacOSX equivalent bug 476927) as soon as possible after the next m-c -> aurora merge, so that it gets a good amount of exposure on Nightly builds before moving down the channels towards release.

Comment 95

[Jonathan Kew [:jfkthame]]

I'd misunderstood about m-c -> m-a merge dates (thought it was happening at the same time as the Fx5 release). It turns out we still have a couple of weeks before the merge, so I went ahead and landed this on trunk so we can see how it fares:

http://hg.mozilla.org/mozilla-central/rev/2c2b3ebca177

Comment 96

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Artifacts with DirectWrite

Running the 2011-06-23 nightly on Windows 7 x64 with Direct2D disabled, I can confirm the problem is fixed. However, with Direct2D (and by extension DirectWrite) enabled, I get the same result as the screenshot from comment #2. I see the patch is only for GDI, but DirectWrite seems affected too, at least for me.

Additional Information:
Switching focus to another application or the desktop makes the artifacts go away. Switching to a different tab and back makes them go away as well, and finally closing/opening the search bar makes them disappear too.

OS: Windows 7 x64 w/ SP1 and the latest patches
GPU: NVIDIA GeForce GTS 250, driver version 275.27.

about:support shows Direct2D Enabled = true and DirectWrite Enabled = true.

Comment 97

[Emanuel Hoogeveen [:ehoogeveen]]

Ah, it appears that bug 661471 is responsible. Setting gfx.font_rendering.cleartype_params.force_gdi_classic_max_size = 0 makes the problem go away for DirectWrite, so it's a problem with GDI-style text rendering, not just GDI itself.

Comment 98

[Jonathan Kew [:jfkthame]]

(In reply to comment #97)
> Ah, it appears that bug 661471 is responsible. Setting
> gfx.font_rendering.cleartype_params.force_gdi_classic_max_size = 0 makes the
> problem go away for DirectWrite, so it's a problem with GDI-style text
> rendering, not just GDI itself.

I suspected that might prove to be an issue, but wanted to get the original GDI fix landed anyway. I'd suggest filing a followup bug about GDI-mode rendering with DW/D2D; we should be able to do a similar patch for that backend.

Comment 99

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to comment #98)
> I'd suggest filing a followup bug about GDI-mode rendering with DW/D2D; we 
> should be able to do a similar patch for that backend.

Filed bug 666893.