User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101007 Firefox/4.0b8pre xp x86 sp3, dx9 nvidia graphics chip, new profile Eval the following (as one line) in the error console to add text to the add-on bar: _______________________________________________________ var doc = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("navigator:browser").document; var addon_bar = doc.getElementById("addon-bar"); var tbitem = doc.createElement("toolbaritem"); var tblabel = doc.createElement("label"); tbitem.appendChild(tblabel); addon_bar.appendChild(tbitem); tblabel.value = "Done"; _________________________________________________________ When acceleration is disabled (gfx.direct2d.disabled == true) the text is rendered using native XP sub-pixel anti-aliasing (ClearType) When acceleration is enabled, some other, inferior, non sub-pixel anti-aliasing is used (not ClearType), resulting in blurrier text. Zoomed screen-shots attached. First noticed in the Status-4-Evar extension: https://addons.mozilla.org/en-US/firefox/addon/235283/ Reproducible: Always

Should this perhaps depend on Bug 593604 (DirectX) rather than Bug 593733 (OpenGL)?

Why is this still unconfirmed?

I have patches that will fix this.

(In reply to comment #6) > I have patches that will fix this. Will that also take care of Bug 611918, or is it different?

I'm not sure.

Actually, I'm going to put my patches here.

The basic approach here is to find any text that's over a transparent part of the window and disable subpixel antialiasing for it (this will require a bit of new font API). Then we can safely render the chrome ThebesLayer as an RGBA surface, with Cleartype turned on for the text that's over opaque content in the layer. Due to the fancy effects used in the Firefox chrome --- in particular, CSS gradient backgrounds, multiple non-repeating backgrounds, and text-shadows with large blurs --- we need to extend display list analysis significantly to prove that the text of the active tab title is entirely over opaque stuff.

See previous comments. We need to be able to disable subpixel AA for text we know is over transparent parts of the window.

Part 2 fails to disable component alpha for some cases --- list bullets, MathML. They might render slightly oddly if drawn over the transparent parts of a window. I could make a followup patch for those display items if someone convinces me it's worth it.

It turns out that most of the opaque areas in the Firefox chrome are actually using border-radius. To get useful information about opaque areas, we have to change IsOpaque to GetOpaqueRegion, so we can get opaque regions for items that aren't actually opaque everywhere in their bounds. The FrameLayerBuilder changes here let us capture opaque regions for display items that are clipped by rounded rects. This patch alone doesn't really buy us all that much since nsDisplayBackground::GetOpaqueRegion still bails when there's a border-radius. I'll fix that in the next patch.

This patch makes nsDisplayBackground::GetOpaqueRegion return something useful whenever any background image is opaque. It refactors the background-drawing logic in nsCSSRendering so we can reuse all the code that handles background positioning, sizing, repeating, etc --- except for background clipping, which is handled explicitly. The background clipping handling handles border-radius, so we can discover opaque regions for the rounded elements in the Firefox chrome.

I have one more patch to attach, but I just horked my hg repo, so I'll return to this tomorrow. With the final patch, plus the patches in dependent bugs, I got Cleartype enabled for everything in the standard Firefox chrome except for the titles of inactive tabs and the Firefox button. The titles of inactive tabs are a lost cause since they're clearly over a translucent background. I thought the Firefox button's background was also slightly translucent, but I was wrong, so I need to look into that case further.

Oh, here it is. The current tab titles are using a text-shadow with 7.5px blur, which makes the nsTextBoxFrame display item's painted area much larger than expected. Some of that blur gets clipped out, but maybe not all. But we can take advantage of the fact that blurred text-shadows don't use subpixel AA at all. So generalize HasText to a new method GetComponentAlphaBounds, which returns the bounds of the area of a display item which needs component alpha (an empty rect if there's none). For nsTextBoxFrame, only the area of the text and any non-blurred shadows is required to be in that rectangle. We could do the same for nsTextFrame but we haven't needed it yet.

To make sure we capture all the detail needed for the browser chrome, bump the rect limit from 4 to 10.

There was a small bug in part 5 --- nsTextBoxFrame::GetComponentAlphaBounds should only return the frame border-box when mComponentAlphaBounds is null, not the visual overflow area. Fixing this bug gives us back subpixel AA on the "Firefox" button, so we're done here!

Comment on attachment 491144 [details] [diff] [review] Part 1: Add nsFont/gfxFontStyle API to enable/disable subpixel AA > PRBool Equals(const nsFont& aOther) const ; >- // Compare ignoring differences in 'variant' and 'decoration' >+ // Compare ignoring differences in 'variant', 'decoration', and 'enableSubpixelAA'. > PRBool BaseEquals(const nsFont& aOther) const; While you're here, could we please have a blank line after Equals(), before the comment that applies to BaseEquals(). Just for clarity. > PLDHashNumber Hash() const { > return ((style + (systemFont << 7) + (familyNameQuirks << 8) + >- (weight << 9)) + PRUint32(size*1000) + PRUint32(sizeAdjust*1000)) ^ >+ (weight << 9)) + (enableSubpixelAA << 10) + >+ PRUint32(size*1000) + PRUint32(sizeAdjust*1000)) ^ > nsISupportsHashKey::HashKey(language); > } Seems like it'd be more logical to have (enableSubpixelAA << 9) and then (weight << 10), given that weight is not a single bit. Also, how would you feel about renaming the variable/field "enableSubpixelAA"? My first thought was that rather than "enable...", I'd prefer "allow...". To me, "enable" could easily be interpreted as specifically turning on the feature, which isn't the case here: if this is TRUE then we're going to respect the default setting (or other prefs that override it), while if it's FALSE we will not allow subpixel to be used. But on second thoughts, it looks like a more accurate name would be forceGrayscaleAA (and reverse the sense of the flag, defaulting to FALSE), because that's what you're doing with it in CreateFontInstance: in the case where this flag says we should do something other than our default behavior, its effect is to force the use of gfxFont::kAntialiasGrayscale. And that seems like a bit of a problem, because if I'm understanding it correctly, this means that using this option will FORCE the use of grayscale AA, even if the system (user's) preference is for no antialiasing, because we'll be instantiating fonts with CAIRO_ANTIALIAS_GRAY instead of CAIRO_ANTIALIAS_DEFAULT. Some users - particularly, I think, some people who are still running XP - intensely dislike "font smoothing" of any kind, and choose to use fonts that are carefully hinted for crisp, pixel-precise accuracy withOUT antialiasing. So I think the API that we really need on nsFont/gfxFontStyle is something that allows us to *disable* subpixel AA without thereby *forcing* the use of grayscale. Maybe a bit-mask of "permitted rendering modes" with separate flags for subpixel AA and grayscale AA; normally, both would be set (so we'd get whatever the back-end decides is its default), but we'd clear the subpixel bit here, and clear both when we want no AA at all. Minusing for now, therefore, pending consideration of the above. On a separate issue, are there no situations where we'd need to respect such a flag in other font back-ends? It seems a bit odd to support it only for GDI.

I'm planning to add support on other font backends as needed. I think you're right about wanting to disable subpixel AA without forcing grayscale. Unfortunately, you can't do that with the cairo font options API. In bug 363861 I added surface-level API to enable and disable Cleartype on transparent surfaces. I can probably use that here instead of the font options API, although it seems like a bit of a hack to work around the font options API.

Maybe a better way to do this would be to add a flag to _cairo_surface to indicate whether subpixel AA is permitted or not. This flag would be initially true for CONTENT_COLOR surfaces and false for all other surfaces. We would provide a getter and setter for this flag. In _cairo_gstate_ensure_scaled_font we would check the flag and if subpixel AA is not permitted, but the merged font options say CAIRO_ANTIALIAS_SUBPIXEL, we change it to CAIRO_CAIRO_ANTIALIAS_GRAY. We should also check this flag in cairo-win32-font and cairo-dwrite-font when we decide how to interpret CAIRO_ANTIALIAS_DEFAULT. This would replace the allow_cleartype API added in bug 363861.

Jeff, Jonathan, John, please let me know what you think of comment #22.

(In reply to comment #23) > Jeff, Jonathan, John, please let me know what you think of comment #22. I don't understand the differences between your new suggestion and the api added in bug 363861.

The changes in bug 363861 only affect Windows. Also, that allow_cleartype flag only affects transparent surfaces; comment #22 would apply to all surfaces.

Ok, so I attached new patches in bug 363861 that allow disabling of subpixel-AA on a per-surface basis, avoiding the problems Jonathan mentioned. This patch simply adds a small helper object to make that API more convenient for the patches in this bug.

Oops, I had an extra change in the wrong patch.

This is a lot simpler now. Instead of having to mess with fonts, we can just disable subpixel AA on the surface while we draw.

The previous patch wasn't quite accurate enough when computing GetComponentAlphaBounds for nsTextBoxFrames in some situations. We need to use the actual rectangle containing the text, not just use the frame border-box. We can do this without any potential performance regressions by computing and storing the text rectangle in DoLayout. (Well, there could still be a performance regression if we reflow the frame many times in between paints, but hopefully that won't be an issue.)

Just bumping the limit to 10 doesn't work when you have a lot of tab titles. Instead, just allow an arbitrarily complex region if we're dealing with chrome content in a transparent window.

(In reply to comment #10) > The basic approach here is to find any text that's over a transparent part of > the window and disable subpixel antialiasing for it (this will require a bit of > new font API). Then we can safely render the chrome ThebesLayer as an RGBA > surface, with Cleartype turned on for the text that's over opaque content in > the layer. Just trying to understand the idea behind this. We divide the text into two categories: text that is over transparent parts of the window, and text thats not. For text that is not we either draw it into an opaque part of a layer, pull up a background color into the layer so that we draw it onto something opaque, or store it temporarily with component alpha so that is composites perfectly onto something opaque in other layers? For text that is over a transparent part of the window we draw with grayscale AA (aka standard antialiasing?) or no AA to a RGBA surface because we can't pass something with component alpha to the windows desktop compositing engine?

And we get worse results if we initially render the text with subpixel AA and thenn convert to grayscale AA?

Comment on attachment 493170 [details] [diff] [review] Part 2: Detect display items over the transparent part of a window, and disable usage of component alpha (i.e., subpixel antialiasing) for those items >+SuppressComponentAlpha(nsDisplayListBuilder* aBuilder, >+ for (nsIFrame* t = f; t; t = t->GetParent()) { >+ if (t->IsTransformed()) >+ return PR_FALSE; >+ } What is the reason for wanting component alpha for transform frames?

(In reply to comment #31) > Just trying to understand the idea behind this. We divide the text into two > categories: text that is over transparent parts of the window, and text thats > not. For text that is not we either draw it into an opaque part of a layer, > pull up a background color into the layer so that we draw it onto something > opaque, or store it temporarily with component alpha so that is composites > perfectly onto something opaque in other layers? For text that is over a > transparent part of the window we draw with grayscale AA (aka standard > antialiasing?) or no AA to a RGBA surface because we can't pass something with > component alpha to the windows desktop compositing engine? That's right. (In reply to comment #32) > And we get worse results if we initially render the text with subpixel AA and > thenn convert to grayscale AA? Also right. (In reply to comment #33) > >+SuppressComponentAlpha(nsDisplayListBuilder* aBuilder, > >+ for (nsIFrame* t = f; t; t = t->GetParent()) { > >+ if (t->IsTransformed()) > >+ return PR_FALSE; > >+ } > > What is the reason for wanting component alpha for transform frames? Actually I'm just terminating the analysis here because I don't want to deal with transformed frames. I should add a comment to indicate that. We need a separate patch, which I haven't written, to disable subpixel AA in every retained ThebesLayer that has a transform that's not just an integer translation.

Comment on attachment 491151 [details] [diff] [review] Part 3: Change IsOpaque to GetOpaqueRegion FrameLayerBuilder::Clip::ClipToOpaque could perhaps use a comment in the .h saying what it does. It's a little unfortunate that RoundedRectIntersectRect is named so similarly to RoundedRectIntersectsRect (an existing function in nsCSSRendering, which is different). I guess it's ok, though. sr=dbaron

Comment on attachment 491156 [details] [diff] [review] Part 4: generalize nsDisplayBackground::GetOpaqueRegion BackgroundLayerState is a pretty large object to return by value. I think it would be better to pass it in to PrepareBackgroundLayer (and, if needed, give it an Init method instead of a constructor). In nsDisplayBackground::GetOpaqueRegion, the GetPrevInFlow/GetNextInFlow checks should be GetPrevContinuation / GetNextContinuation. nsDisplayBackground::GetOpaqueRegion should just return if it hits the color clause; there's no way an image can cover more than the color does. Also, in that clause: >+ result = GetInsideClipRegion(bg->BottomLayer().mClip, borderBox); you already have bg->BottomLayer() in the bottomLayer variable. Either use it here or remove it. nsDisplayBackground::GetInsideClipRegion should probably have an NS_NOTREACHED default case. sr=dbaron (or r=dbaron; I reviewed all of this one)

Comment on attachment 496290 [details] [diff] [review] Part 5 v3: Change HasText to GetComponentAlphaBounds sr=dbaron

(In reply to comment #35) > FrameLayerBuilder::Clip::ClipToOpaque could perhaps use a comment in the > .h saying what it does. Done.

(In reply to comment #36) > BackgroundLayerState is a pretty large object to return by value. I > think it would be better to pass it in to PrepareBackgroundLayer (and, > if needed, give it an Init method instead of a constructor). Returning a struct by value produces the same code as passing in a pointer to a struct to be initialized, if the named-return-value optimization is used, which it is in this case. > In nsDisplayBackground::GetOpaqueRegion, the GetPrevInFlow/GetNextInFlow > checks should be GetPrevContinuation / GetNextContinuation. Right thanks. Done. > nsDisplayBackground::GetOpaqueRegion should just return if it hits the > color clause; there's no way an image can cover more than the color > does. Couldn't the bottom-layer's clip be smaller than the clip on another layer? > Also, in that clause: > >+ result = GetInsideClipRegion(bg->BottomLayer().mClip, borderBox); > you already have bg->BottomLayer() in the bottomLayer variable. Either > use it here or remove it. Fixed. > nsDisplayBackground::GetInsideClipRegion should probably have an > NS_NOTREACHED default case. Done.

Comment on attachment 496316 [details] [diff] [review] Part 6 v2 >Bug 602757. Part 6: Bump maximum rects in opaque region to 10. r=tnikkel Need to update the commit message.

I've updated it to "Part 6: Don't limit complexity of opaque region when adding opaque chrome display items".

Comment on attachment 496290 [details] [diff] [review] Part 5 v3: Change HasText to GetComponentAlphaBounds NS_DISPLAY_DECL_NAME("nsDisplayTransform", TYPE_TRANSFORM); + virtual nsRect GetComponentAlphaBounds(nsDisplayListBuilder* aBuilder) + { + if (mStoredList.GetComponentAlphaBounds(aBuilder).IsEmpty()) + return nsRect(); + return GetBounds(aBuilder); + } Given your earlier comment about wanting to disable component alpha in layers with non-integer translations, this seems very sub-optimal. But I guess it's what we were doing before. diff --git a/layout/mathml/nsMathMLChar.cpp b/layout/mathml/nsMathMLChar.cpp - virtual nsRect GetBounds(nsDisplayListBuilder* aBuilder) { + nsRect GetBounds() { nsRect rect; mChar->GetRect(rect); nsPoint offset = ToReferenceFrame() + rect.TopLeft(); nsBoundingMetrics bm; mChar->GetBoundingMetrics(bm); return nsRect(offset.x + bm.leftBearing, offset.y, bm.rightBearing - bm.leftBearing, bm.ascent + bm.descent); } + virtual nsRect GetBounds(nsDisplayListBuilder* aBuilder) { return GetBounds(); } I don't understand, why make this change?

Error, will fix.

Comment on attachment 491151 [details] [diff] [review] Part 3: Change IsOpaque to GetOpaqueRegion - * of items that return true from IsOpaque(), and automatically + * of items that return true from GetOpaqueRegion(), and automatically This search/replace result doesn't quite make sense. +nsRegion +nsDisplayWrapList::GetOpaqueRegion(nsDisplayListBuilder* aBuilder, + PRBool* aForceTransparentSurface) { - return mList.IsOpaque(); + nsRegion result; + if (mList.IsOpaque()) { + result = GetBounds(aBuilder); + } + return result; } Hmm, aren't we going to be throwing away a lot of the potential gain from using opaque regions if we only pass an opaque flag through wrapped lists? +nsRegion nsDisplayTransform::GetOpaqueRegion(nsDisplayListBuilder *aBuilder, + nsRegion result; + if (disp->mTransform.GetMainMatrixEntry(1) == 0.0f && + disp->mTransform.GetMainMatrixEntry(2) == 0.0f) { + result = mStoredList.GetOpaqueRegion(aBuilder); + } + return result; } I think this code isn't quite right, but I looked at the updated patch in your queue and I think that is fine. + nsRect ClipToOpaque(const nsRect& aRect) const; Can we call this something else? It has nothing to do with opaque. Maybe ApproxClipRect?

Comment on attachment 491156 [details] [diff] [review] Part 4: generalize nsDisplayBackground::GetOpaqueRegion + nsRect GetBackgroundLayerRect(const nsStyleBackground::Layer& aLayer); Looks to be unimplemented and unused. +struct BackgroundLayerState { + BackgroundLayerState(nsIFrame* aForFrame, const nsStyleImage* aImage, PRUint32 aFlags) + : mImageRenderer(aForFrame, aImage, aFlags) {} + + ImageRenderer mImageRenderer; + nsRect mDestArea; + nsRect mFillArea; + nsPoint mAnchor; +}; A comment on each item would be nice, especially mDestArea and mFillArea.

(In reply to comment #44) > +nsRegion > +nsDisplayWrapList::GetOpaqueRegion(nsDisplayListBuilder* aBuilder, > + PRBool* aForceTransparentSurface) { > - return mList.IsOpaque(); > + nsRegion result; > + if (mList.IsOpaque()) { > + result = GetBounds(aBuilder); > + } > + return result; > } > > Hmm, aren't we going to be throwing away a lot of the potential gain from using > opaque regions if we only pass an opaque flag through wrapped lists? This is certainly good enough for chrome. Maybe later it'll be worth making it more complicated. > + nsRect ClipToOpaque(const nsRect& aRect) const; > > Can we call this something else? It has nothing to do with opaque. Maybe > ApproxClipRect? Renamed to ApproximateIntersect.

(In reply to comment #45) > Comment on attachment 491156 [details] [diff] [review] > Part 4: generalize nsDisplayBackground::GetOpaqueRegion > > + nsRect GetBackgroundLayerRect(const nsStyleBackground::Layer& aLayer); > > Looks to be unimplemented and unused. Removed. > +struct BackgroundLayerState { > + BackgroundLayerState(nsIFrame* aForFrame, const nsStyleImage* aImage, > PRUint32 aFlags) > + : mImageRenderer(aForFrame, aImage, aFlags) {} > + > + ImageRenderer mImageRenderer; > + nsRect mDestArea; > + nsRect mFillArea; > + nsPoint mAnchor; > +}; > > A comment on each item would be nice, especially mDestArea and mFillArea. Done.

http://hg.mozilla.org/mozilla-central/rev/5b0756d80fe8 http://hg.mozilla.org/mozilla-central/rev/841881532933 http://hg.mozilla.org/mozilla-central/rev/d6696d3206f8 http://hg.mozilla.org/mozilla-central/rev/475fe8dd48a3 http://hg.mozilla.org/mozilla-central/rev/b804550e79d7 http://hg.mozilla.org/mozilla-central/rev/836e01a2a6dc Note: this is only fixed when DirectWrite is disabled. Bug 622482 needs to be fixed to get subpixel AA in chrome with DirectWrite.