Last Comment Bug 647560 - Incorrect font rendering with 2 sec delay greyscale->subpixel on scrollable div
: Incorrect font rendering with 2 sec delay greyscale->subpixel on scrollable div
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 3 votes (vote)
: mozilla7
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
Mentors:
Depends on: 665133
Blocks: 579276 602757
  Show dependency treegraph
 
Reported: 2011-04-03 06:44 PDT by ripper5555
Modified: 2012-02-23 10:15 PST (History)
21 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tetcase (130.12 KB, application/zip)
2011-04-03 06:45 PDT, ripper5555
no flags Details
Testcase (130.15 KB, application/zip)
2011-04-03 07:07 PDT, ripper5555
no flags Details
modified reporter's testcase (20.62 KB, text/html)
2011-04-03 08:42 PDT, Alice0775 White
no flags Details
another testcase (31.51 KB, text/html)
2011-04-03 08:43 PDT, Alice0775 White
no flags Details
Simplified testcase (7.18 KB, text/html)
2011-04-09 18:24 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Clean up MarkLeafLayersHidden and make it set the hidden state on container layers (5.69 KB, patch)
2011-04-18 20:16 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Add support for compositing BasicLayers with OPERATOR_SOURCE (18.22 KB, patch)
2011-04-18 20:19 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Create ApplyDoubleBuffering to recursively walk layer tree and implement double-buffering by setting mUseIntermediateSurface on ContainerLayers where necessary (12.77 KB, patch)
2011-04-18 21:08 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Cache temporary backbuffer surfaces (19.06 KB, patch)
2011-04-18 21:08 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
karlt: review+
Details | Diff | Splinter Review
unfixed testcase (7.24 KB, text/html)
2011-04-18 21:10 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Use passed-in aContext instead of getting it from the layer manager (1.13 KB, patch)
2011-05-29 20:01 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 6: If Cleartype is disabled, just use default rendering parameters and ignore GDI_CLASSIC overrides (1012 bytes, patch)
2011-06-05 05:14 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
performance regression fix (3.64 KB, patch)
2011-06-08 22:37 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
performance regression fix v2 (3.86 KB, patch)
2011-06-09 15:03 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
performance regression fix v3 (3.87 KB, patch)
2011-06-09 15:04 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
jmuizelaar: review+
Details | Diff | Splinter Review

Description ripper5555 2011-04-03 06:44:51 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0

With included testcase, when scrolling to the very beginning or very ending, the font is rendered grayscale.
After a 2-3 second delay renders with subpixel.
Halfway it's rendered ok.

This is different from bug no. 606075 and others and is reproducable in official 4.0 release and the latest nightly build.
The 3.6 branch is unaffected.

The weird font rendering can be resolved by either removing the "background-image" css tags OR the "border-radius" ones.
Either one will "fix" the incorrect rendering.
Setting the "border-radius" bigger then 20px also seems to "fix" it.
So there isn't a distinct CSS tag responsible.

"hardware acceleration" enabled/disabled doesn't fix it.
"Anti Aliasing Tuner" Doesn't fix anything, even if set to DISABLE subpixel rendering. (it still renders)

Reproducible: Always

Steps to Reproduce:
1. Just load the testcase.
Actual Results:  
Scroll all the way up or all the way down.

Expected Results:  
Incorrect font rendering: greyscale instead of subpixel.

Subpixel rendering...
Comment 1 ripper5555 2011-04-03 06:45:23 PDT
Created attachment 523872 [details]
Tetcase
Comment 2 ripper5555 2011-04-03 07:07:48 PDT
Created attachment 523875 [details]
Testcase
Comment 3 Alice0775 White 2011-04-03 07:42:19 PDT
Regression window:
works:
http://hg.mozilla.org/mozilla-central/rev/75b99ffe98d7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101231 Firefox/4.0b9pre ID:20110102142430
Fails:
http://hg.mozilla.org/mozilla-central/rev/836e01a2a6dc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20110102 Firefox/4.0b9pre ID:20110102175218
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75b99ffe98d7&tochange=836e01a2a6dc
Comment 4 Alice0775 White 2011-04-03 07:47:24 PDT
Confirmed on
http://hg.mozilla.org/mozilla-central/rev/4e4c7457e8f7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110403 Firefox/4.2a1pre ID:20110403030449

Graphics
  Adapter Description: ATI Radeon HD 4300/4500 Series
  Vendor ID: 1002
  Device ID: 954f
  Adapter RAM: 512
  Adapter Drivers: aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
  Driver Version: 8.831.2.0
  Driver Date: 3-8-2011
  Direct2D Enabled: true
  DirectWrite Enabled: true (6.1.7601.17563, font cache n/a)
  WebGL Renderer: Google Inc. -- ANGLE -- OpenGL ES 2.0 (ANGLE 0.0.0.541)
  GPU Accelerated Windows: 1/1 Direct3D 10

And It happens regardless of Hardware Acceleration .
Comment 5 Alice0775 White 2011-04-03 08:42:04 PDT
Created attachment 523881 [details]
modified reporter's testcase
Comment 6 Alice0775 White 2011-04-03 08:43:40 PDT
Created attachment 523882 [details]
another testcase

Sorry, Regression range in comment#3 is wrong.
Comment 7 Alice0775 White 2011-04-03 09:36:40 PDT
There are 2 regression window(layers.accelerate enable/disable  respectively)

Regression window1:
set user_pref("layers.accelerate-all", true);
Works(After scroll. it is repainted immediately):
http://hg.mozilla.org/mozilla-central/rev/7aaf30721c48
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100902 Firefox/4.0b6pre ID:20100902204148
Fails(After scroll. It is repainted after the approximately 2 seconds):
http://hg.mozilla.org/mozilla-central/rev/4b879b793eb6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100902 Firefox/4.0b6pre ID:20100902234753
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7aaf30721c48&tochange=4b879b793eb6
Triggered by:
Bug 579276 - White text on a fixed position background triggers Bug 363861 (bad ClearType) with GDI and retained layers

Regression window2:
set user_pref("layers.accelerate-none", true);
Works:
http://hg.mozilla.org/mozilla-central/rev/75b99ffe98d7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101231 Firefox/4.0b9pre ID:20110102142430
Fails(After scroll. It is repainted after the approximately 2 seconds):
http://hg.mozilla.org/mozilla-central/rev/836e01a2a6dc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20110102 Firefox/4.0b9pre ID:20110102175218
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75b99ffe98d7&tochange=836e01a2a6dc
Triggered by:
Bug 602757 - Hardware acceleration disables ClearType on the add-on bar
Comment 8 Virtual_ManPL [:Virtual] - (ni? me) 2011-04-03 11:04:49 PDT
Related to bug #626293 maybe ?
Comment 9 Alice0775 White 2011-04-03 12:04:15 PDT
(In reply to comment #8)
> Related to bug #626293 maybe ?

I do not think so, because regression range is different.
Comment 10 ripper5555 2011-04-08 11:07:48 PDT
The reports of "bug 618211" and "bug 630879" seem to be explaining the same symptoms.
One is assigned.
Link those bugs with this one?
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-09 18:24:39 PDT
Created attachment 524901 [details]
Simplified testcase
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-10 02:11:16 PDT
When we scroll that testcase with acceleration disabled, the text is put in a layer with no background. Since the text is over transparent pixels we request component alpha for the layer. BasicLayers decides to paint the text directly to the RGBA backbuffer. The text is over an opaque region of the surface so we enable subpixel antialiasing. Because the surface is RGBA we take the "fallback path" in _cairo_win32_scaled_font_show_glyphs that draws to a temporary surface and then copies the results to the destination. That path is designed to assume dark text on a white background:
	/* Otherwise, we need to draw using software fallbacks. We create a mask
	 * surface by drawing the the glyphs onto a DIB, black-on-white then
	 * inverting. GDI outputs gamma-corrected images so inverted black-on-white
	 * is very different from white-on-black. We favor the more common
	 * case where the final output is dark-on-light.
This testcase uses light text on a dark background so that assumption is all wrong.

With acceleration enabled the component-alpha layer is rendered using similar techniques which also get the gamma correction wrong.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 20:16:26 PDT
Created attachment 526903 [details] [diff] [review]
Clean up MarkLeafLayersHidden and make it set the hidden state on container layers

This patch is on top of bug 629866 part 2.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 20:19:44 PDT
Created attachment 526904 [details] [diff] [review]
Add support for compositing BasicLayers with OPERATOR_SOURCE
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 21:08:06 PDT
Created attachment 526922 [details] [diff] [review]
Create ApplyDoubleBuffering to recursively walk layer tree and implement double-buffering by setting mUseIntermediateSurface on ContainerLayers where necessary
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 21:08:47 PDT
Created attachment 526923 [details] [diff] [review]
Cache temporary backbuffer surfaces
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 21:10:47 PDT
Created attachment 526924 [details]
unfixed testcase

These patches make things a lot better for the non-accelerated case. It's still possible to get into situations like in comment #12. In particular, if chrome overlaps content then we create a backbuffer for the whole window, it's transparent, and the same bug occurs. You can see this in this testcase.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 21:12:44 PDT
We could make things better still (for the non-accelerated case) by setting mUseIntermediateSurface on the opaque ContainerLayer for the Web content when it has COMPONENT_ALPHA descendants.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 21:15:39 PDT
... but I'm not sure it's worth it.

The above patches should be a clear win; they make double-buffering strictly more efficient --- we need a temporary double-buffering surface less often, and when we do need it it's likely to be smaller than what we use now.
Comment 20 Timothy Nikkel (:tnikkel) 2011-04-19 20:52:18 PDT
Comment on attachment 526903 [details] [diff] [review]
Clean up MarkLeafLayersHidden and make it set the hidden state on container layers

This patch was reviewed at the Grand Canyon.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-19 20:55:56 PDT
Thank you!
Comment 22 Karl Tomlinson (:karlt) 2011-04-20 02:43:55 PDT
Comment on attachment 526923 [details] [diff] [review]
Cache temporary backbuffer surfaces

>-      if (opacity != 1.0) {

>+      if (needsGroup) {

Fixing a bug in attachment 526904 [details] [diff] [review].
Comment 23 Karl Tomlinson (:karlt) 2011-04-24 23:01:26 PDT
Comment on attachment 526923 [details] [diff] [review]
Cache temporary backbuffer surfaces

>-                                   const gfxIntSize& aSize,
>+                                   const gfxRect& aRect,

Can you either add an assertion that the size is integer or round up, please?
(I'm not sure about the top-left.  Asserting that is integer might make sense to prevent the client making mistakes, but there isn't anything intrinsically bad about integer offsets within gfxCachedTempSurface.)
Comment 24 Karl Tomlinson (:karlt) 2011-04-24 23:03:45 PDT
(In reply to comment #23)
> but there isn't anything intrinsically
> bad about integer offsets within gfxCachedTempSurface.)

I mean ... bad about non-integer offsets ...
Comment 25 Timothy Nikkel (:tnikkel) 2011-05-11 13:47:19 PDT
Comment on attachment 526904 [details] [diff] [review]
Add support for compositing BasicLayers with OPERATOR_SOURCE

@@ -542,36 +575,40 @@ BasicThebesLayer::PaintThebes(gfxContext
       PRBool needsClipToVisibleRegion = PR_FALSE;
-      if (opacity != 1.0) {
-        needsClipToVisibleRegion = PushGroupForLayer(aContext, this, toDraw);
+      PRBool needsGroup =
+          opacity != 1.0 || GetOperator() != gfxContext::OPERATOR_OVER;
+      if (needsGroup) {
+        needsClipToVisibleRegion = PushGroupForLayer(aContext, this, toDraw) ||
+                GetOperator() != gfxContext::OPERATOR_OVER;
       }
       SetAntialiasingFlags(this, aContext);
       aCallback(this, aContext, toDraw, nsIntRegion(), aCallbackData);
       if (opacity != 1.0) {
         aContext->PopGroupToSource();
         if (needsClipToVisibleRegion) {
           gfxUtils::ClipToRegion(aContext, toDraw);
         }
+        AutoSetOperator setOperator(aContext, GetOperator());
         aContext->Paint(opacity);
       }

Shouldn't the second "if (opacity != 1.0)" be changed to "if (needsGroup)" too?

@@ -1476,18 +1518,23 @@ BasicLayerManager::SetRoot(Layer* aLayer
+  BasicContainerLayer* container = static_cast<BasicContainerLayer*>(aLayer);
+  PRBool needsGroup = container->GetFirstChild() &&
+    container->UseIntermediateSurface();
+  NS_ASSERTION(needsGroup || !container->GetFirstChild() ||
+               container->GetOperator() == gfxContext::OPERATOR_OVER,
+               "non-OVER operator should have forced UseIntermediateSurface");
+

Doesn't the container->GetFirstChild() call assume that aLayer is a container layer? Can we assume that?
Comment 26 Timothy Nikkel (:tnikkel) 2011-05-11 16:27:55 PDT
Comment on attachment 526922 [details] [diff] [review]
Create ApplyDoubleBuffering to recursively walk layer tree and implement double-buffering by setting mUseIntermediateSurface on ContainerLayers where necessary

So with this kind of double buffering it would be possible to see an intermediate result where we have blitted some, but not all, of the child layers to the screen?

+  /**
+   * Returns true when:
+   * a) no (non-hidden) childrens' visible areas overlap in
+   * (aInRect intersected with this layer's visible region).
+   * b) the (non-hidden) childrens' visible areas cover
+   * (aInRect intersected with this layer's visible region).
+   * c) this layer and all (non-hidden) children have transforms that are translations
+   * by integers.
+   * aInRect is in the root coordinate system.
+   * Child layers with opacity do not contribute to the covered area in check b).
+   * This method can be conservative; it's OK to return false under any
+   * circumstances.
+   */

Why not child layers with opacity? We're allowing non-opaque layers.

@@ -1418,61 +1533,33 @@ BasicLayerManager::EndTransactionInterna
-    PRBool useDoubleBuffering = mUsingDefaultTarget &&
-      mDoubleBuffering != BUFFER_NONE &&
-      MayHaveOverlappingOrTransparentLayers(mRoot, deviceSpaceClipExtents, &rootRegion);

So is MayHaveOverlappingOrTransparentLayers unused after this patch then?
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 16:44:09 PDT
(In reply to comment #25)
> Shouldn't the second "if (opacity != 1.0)" be changed to "if (needsGroup)"
> too?

Yes, the "Cache temporary backbuffer surfaces" patch actually contains that fix. I'll move it around.

> @@ -1476,18 +1518,23 @@ BasicLayerManager::SetRoot(Layer* aLayer
> +  BasicContainerLayer* container =
> static_cast<BasicContainerLayer*>(aLayer);
> +  PRBool needsGroup = container->GetFirstChild() &&
> +    container->UseIntermediateSurface();
> +  NS_ASSERTION(needsGroup || !container->GetFirstChild() ||
> +               container->GetOperator() == gfxContext::OPERATOR_OVER,
> +               "non-OVER operator should have forced
> UseIntermediateSurface");
> +
> 
> Doesn't the container->GetFirstChild() call assume that aLayer is a
> container layer? Can we assume that?

It doesn't assume that. GetFirstChild is supported on all layers. I'll change the call to be aLayer->GetFirstChild() to make that clearer.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 16:49:17 PDT
(In reply to comment #26)
> Comment on attachment 526922 [details] [diff] [review] [review]
> Create ApplyDoubleBuffering to recursively walk layer tree and implement
> double-buffering by setting mUseIntermediateSurface on ContainerLayers where
> necessary
> 
> So with this kind of double buffering it would be possible to see an
> intermediate result where we have blitted some, but not all, of the child
> layers to the screen?

Yes. However, those child layers can't overlap, so you can't get any kind of flickering. And trunk actually already does this, since currently if no layers overlap we disable double-buffering.

> +  /**
> +   * Returns true when:
> +   * a) no (non-hidden) childrens' visible areas overlap in
> +   * (aInRect intersected with this layer's visible region).
> +   * b) the (non-hidden) childrens' visible areas cover
> +   * (aInRect intersected with this layer's visible region).
> +   * c) this layer and all (non-hidden) children have transforms that are
> translations
> +   * by integers.
> +   * aInRect is in the root coordinate system.
> +   * Child layers with opacity do not contribute to the covered area in
> check b).
> +   * This method can be conservative; it's OK to return false under any
> +   * circumstances.
> +   */
> 
> Why not child layers with opacity? We're allowing non-opaque layers.

Because when we draw OPERATOR_SOURCE with opacity < 1 the background is not cleared.

> @@ -1418,61 +1533,33 @@ BasicLayerManager::EndTransactionInterna
> -    PRBool useDoubleBuffering = mUsingDefaultTarget &&
> -      mDoubleBuffering != BUFFER_NONE &&
> -      MayHaveOverlappingOrTransparentLayers(mRoot, deviceSpaceClipExtents,
> &rootRegion);
> 
> So is MayHaveOverlappingOrTransparentLayers unused after this patch then?

Yes, good point. I'll post an additional patch to remove that.
Comment 29 Timothy Nikkel (:tnikkel) 2011-05-11 16:51:52 PDT
(In reply to comment #27)
> > Doesn't the container->GetFirstChild() call assume that aLayer is a
> > container layer? Can we assume that?
> 
> It doesn't assume that. GetFirstChild is supported on all layers. I'll
> change the call to be aLayer->GetFirstChild() to make that clearer.

Calling a virtual function from a pointer type that is not compatible with the object's actual type sure feels wrong!
Comment 30 Timothy Nikkel (:tnikkel) 2011-05-11 17:13:34 PDT
(In reply to comment #28)
> > So with this kind of double buffering it would be possible to see an
> > intermediate result where we have blitted some, but not all, of the child
> > layers to the screen?
> 
> Yes. However, those child layers can't overlap, so you can't get any kind of
> flickering. And trunk actually already does this, since currently if no
> layers overlap we disable double-buffering.

You could get an effect where the user expect the contents of two layers to be in sync (ie not think of them as being different 'parts' but see them as part of the same thing), but one gets updated and not the other. I mostly asked to make sure I was understanding this correctly.
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 17:28:19 PDT
(In reply to comment #30)
> (In reply to comment #28)
> > > So with this kind of double buffering it would be possible to see an
> > > intermediate result where we have blitted some, but not all, of the child
> > > layers to the screen?
> > 
> > Yes. However, those child layers can't overlap, so you can't get any kind of
> > flickering. And trunk actually already does this, since currently if no
> > layers overlap we disable double-buffering.
> 
> You could get an effect where the user expect the contents of two layers to
> be in sync (ie not think of them as being different 'parts' but see them as
> part of the same thing), but one gets updated and not the other.

You could, but the difference would be very short-lived and I doubt this would be a problem in practice. For someone to notice there would probably have to be an object moving parallel to the layer boundary that crosses the boundary. And because the pieces of the objects would have to be in separate layers, you'd have to construct something very contrived.
Comment 32 Timothy Nikkel (:tnikkel) 2011-05-11 20:07:32 PDT
Comment on attachment 526922 [details] [diff] [review]
Create ApplyDoubleBuffering to recursively walk layer tree and implement double-buffering by setting mUseIntermediateSurface on ContainerLayers where necessary

r+ with the changes mentioned.
Comment 33 Timothy Nikkel (:tnikkel) 2011-05-11 20:07:36 PDT
Comment on attachment 526904 [details] [diff] [review]
Add support for compositing BasicLayers with OPERATOR_SOURCE

r+ with the changes mentioned.
Comment 34 Timothy Nikkel (:tnikkel) 2011-05-11 20:54:55 PDT
Comment on attachment 526923 [details] [diff] [review]
Cache temporary backbuffer surfaces

So this means we are going to lose the benefits of the "Copy Background" part of PushGroupAndCopyBackground?
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 23:30:38 PDT
No. Assuming you're talking about BasicLayerManager::PushGroupForLayer, PushGroupAndCopyBackground with CONTENT_COLOR is always just the same as a regular PushGroup.
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-12 08:26:36 PDT
(In reply to comment #23)
> Comment on attachment 526923 [details] [diff] [review] [review]
> Cache temporary backbuffer surfaces
> 
> >-                                   const gfxIntSize& aSize,
> >+                                   const gfxRect& aRect,
> 
> Can you either add an assertion that the size is integer or round up, please?
> (I'm not sure about the top-left.  Asserting that is integer might make
> sense to prevent the client making mistakes, but there isn't anything
> intrinsically bad about integer offsets within gfxCachedTempSurface.)

Rounding up.
Comment 37 Timothy Nikkel (:tnikkel) 2011-05-12 09:31:17 PDT
(In reply to comment #35)
> No. Assuming you're talking about BasicLayerManager::PushGroupForLayer,
> PushGroupAndCopyBackground with CONTENT_COLOR is always just the same as a
> regular PushGroup.

Oops, I misread the patch. Ignore my comment.
Comment 38 Florian Hänel [:heeen] 2011-05-13 01:48:02 PDT
Those patches don't apply on m-c - are there any prerequisites?
Comment 39 Timothy Nikkel (:tnikkel) 2011-05-13 08:52:37 PDT
Part 2 and/or 3 from bug 629866.
Comment 40 Mounir Lamouri (:mounir) 2011-05-17 02:40:51 PDT
Roc pushed these patches in cedar but I did backout it because of an orange on Windows Mochitest-4:
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1305623848.1305624333.18347.gz&fulltext=1

There are two candidates, this bug and bug 629866
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-17 04:11:19 PDT
I backed it out as well :-) but that was not the cause of the orange, the orange persisted after backout (and the patch passed those tests on try multiple times).
Comment 42 Mounir Lamouri (:mounir) 2011-05-17 04:46:05 PDT
The orange is still there so this patch should be safe to be pushed again in cedar later.
Comment 45 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-20 08:49:57 PDT
I did backout the code because it broke mobile on desktop (bug 658332)

http://hg.mozilla.org/mozilla-central/rev/f3b87db5dc4e
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-29 20:01:33 PDT
Created attachment 535977 [details] [diff] [review]
Use passed-in aContext instead of getting it from the layer manager

This patch fixes the regression with Fennec.
Comment 49 Timothy Nikkel (:tnikkel) 2011-06-02 13:57:14 PDT
I asked Armen to run the changesets before and after this bug again to see if this bug was responsible for a 60% regression in Txul on Windows 7 64-bit and it seems that it was this bug. Any idea what might be the cause?
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 17:59:52 PDT
I'll do some profiling. Maybe we need some patches from bug 629866 here. In the meantime we should probably back out.
Comment 51 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 18:16:22 PDT
Er ... do you know what layers backend was in use? Normally we test D2D/D3D10 on Windows 7, and that shouldn't have been affected by this bug...
Comment 52 Timothy Nikkel (:tnikkel) 2011-06-02 20:04:47 PDT
Armen, is it possible to get the graphics section from about:support from a machine that runs Txul on Windows 7 64-bit?
Comment 53 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 20:06:28 PDT
The other thing is ... "Windows 7 64-bit" means a 64-bit Firefox build running on Windows 7, right? Not a 32-bit Firefox build running on 64-bit Windows 7?
Comment 54 Timothy Nikkel (:tnikkel) 2011-06-02 20:07:47 PDT
Yes, 64-bit build running on a 64-bit OS.
Comment 55 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 20:37:25 PDT
OK, let me try to figure this out.
Comment 56 Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-06-03 13:46:23 PDT
Here you go (from t-r3-w7-035):

  Application Basics

        Name
        Firefox

        Version
        7.0a1

        User Agent
        Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:7.0a1) Gecko/20110603 Firefox/7.0a1

        Profile Directory

          Open Containing Folder

        Enabled Plugins

          about:plugins

        Build Configuration

          about:buildconfig

  Extensions

        Name

        Version

        Enabled

        ID

  Modified Preferences

      Name

      Value

        browser.places.smartBookmarksVersion
        2

        browser.startup.homepage_override.buildID
        20110603030224

        browser.startup.homepage_override.mstone
        rv:7.0a1

        extensions.lastAppVersion
        7.0a1

        network.cookie.prefsMigrated
        true

        places.history.expiration.transient_current_max_pages
        55550

        privacy.sanitize.migrateFx3Prefs
        true

  Graphics

        Adapter Description
        RDPDD Chained DD

        Adapter RAM
        Unknown

        Adapter Drivers
        RDPDD

        Direct2D Enabled
        Blocked for your graphics card because of unresolved driver issues.

        DirectWrite Enabled
        false (6.1.7600.16385)

        ClearType Parameters
        ClearType parameters not found

        WebGL Renderer
        Blocked for your graphics card because of unresolved driver issues.

        GPU Accelerated Windows
        0/1. Blocked for your graphics card because of unresolved driver issues.
Comment 57 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-03 13:50:23 PDT
OK, that makes sense. I'll do some profiling of my build with acceleration off.

Armen, you might want to investigate why acceleration is disabled on that machine. Do 32-bit builds on the same system get acceleration enabled?
Comment 58 Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-06-03 13:53:09 PDT
We never updated the graphic card drivers on these slaves like for Win7 32-bit slaves:
> https://wiki.mozilla.org/ReferencePlatforms/Test/Win7#NVidia_drivers_update_.28Version:_260.99.3B_Date:_2010.10.25.29
Comment 59 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-03 13:55:50 PDT
OK. I expect that the regression here was BasicLayers + Aero and it just so happens that the 64-bit build was the only configuration when we're still doing perf tests for that.
Comment 60 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-05 05:14:25 PDT
Created attachment 537449 [details] [diff] [review]
Part 6: If Cleartype is disabled, just use default rendering parameters and ignore GDI_CLASSIC overrides

This mostly fixes it. It appears using CLEARTYPE_GDI_CLASSIC rendering when Cleartype is disabled completely fails. This doesn't completely handle the case where the Cleartype setting changes dynamically without restarting the browser; do we care?
Comment 61 Mounir Lamouri (:mounir) 2011-06-05 07:07:27 PDT
(In reply to comment #60)
> Created attachment 537449 [details] [diff] [review] [review]

Roc, did you meant to assign this review to no one?
Comment 62 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-05 13:33:02 PDT
Comment on attachment 537449 [details] [diff] [review]
Part 6: If Cleartype is disabled, just use default rendering parameters and ignore GDI_CLASSIC overrides

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

of course not :-)
Comment 63 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-05 14:25:54 PDT
Comment on attachment 537449 [details] [diff] [review]
Part 6: If Cleartype is disabled, just use default rendering parameters and ignore GDI_CLASSIC overrides

Wrong bug.
Comment 64 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-06 21:25:11 PDT
I think I can reproduce the Twinopen regression in a 32-bit build.
Comment 65 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-06 21:36:27 PDT
Or rather, on a 32-bit build I see BasicLayers having a significantly higher twinopen than D3D10 layers.
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-07 04:16:39 PDT
tpaint is also higher with BasicLayers than D3D10 layers (about 180ms instead of 160ms on my laptop). But I profiled tpaint with xperf and BasicLayers spends less CPU time in firefox.exe per window open than D3D10 layers. I don't understand it.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-07 04:50:40 PDT
I can definitely reproduce the regression. It's the ApplyDoubleBuffering patch, unsurprisingly.
Comment 68 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-07 23:03:20 PDT
At least part of the problem seems to be direct drawing of ColorLayers to the window HDC. With these patches, we draw the entire default browser window without double-buffering because none of the leaf layers overlap. When ColorLayers paint to a non-DIB Win32 surface, we get a call chain like this:

xul.dll!mozilla::layers::BasicColorLayer::PaintColorTo
xul.dll!gfxContext::Paint
xul.dll!_moz_cairo_paint_with_alpha
xul.dll!_cairo_gstate_paint
xul.dll!_cairo_surface_paint
xul.dll!_cairo_surface_fallback_paint
xul.dll!_clip_and_composite_trapezoids
xul.dll!_clip_and_composite_region
xul.dll!_cairo_surface_fill_region
xul.dll!_cairo_surface_fill_rectangles
xul.dll!_cairo_surface_fallback_fill_rectangles
xul.dll!_cairo_surface_acquire_dest_image

... and then goes on to fill the temporary image surface and write it back. Ouch.
Comment 69 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-08 22:37:32 PDT
Created attachment 538174 [details] [diff] [review]
performance regression fix

This fixes the performance regression for me locally by making win32 fill_rectangles work on ARGB surfaces without fallback, if you're using OPERATOR_SOURCE or OVER with an opaque color. I considered various alternative implementations but settled on using StretchDIBits to fill the destination rectangle(s) with a 1x1 pixel bitmap. I carefully tested the translucent borders of the Firefox window and it seems to be correctly copying the alpha value into the destination window surface.
Comment 70 Jeff Muizelaar [:jrmuizel] 2011-06-09 07:47:13 PDT
(In reply to comment #69)
> Created attachment 538174 [details] [diff] [review] [review]
> performance regression fix
> 
> This fixes the performance regression for me locally by making win32
> fill_rectangles work on ARGB surfaces without fallback, if you're using
> OPERATOR_SOURCE or OVER with an opaque color. I considered various
> alternative implementations but settled on using StretchDIBits to fill the
> destination rectangle(s) with a 1x1 pixel bitmap. I carefully tested the
> translucent borders of the Firefox window and it seems to be correctly
> copying the alpha value into the destination window surface.

Why did you choose StretchDIBits over FillRect? It's worth documenting this in the patch.
Comment 71 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-09 14:50:37 PDT
I tested FillRect and it doesn't work. I'm not sure exactly what it does, but under some conditions it seems to just not propagate the alpha value to the destination at all. I can add a comment for that.
Comment 72 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-09 15:03:34 PDT
Created attachment 538362 [details] [diff] [review]
performance regression fix v2
Comment 73 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-09 15:04:23 PDT
Created attachment 538363 [details] [diff] [review]
performance regression fix v3

Oops, fix print_gdi_error string
Comment 74 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-13 02:36:32 PDT
Comment on attachment 538363 [details] [diff] [review]
performance regression fix v3

Review of attachment 538363 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 75 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-16 20:55:52 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/1631f3557eae
Comment 76 Jonathan Kew (:jfkthame) 2011-06-17 01:25:16 PDT
Comment on attachment 538363 [details] [diff] [review]
performance regression fix v3

>+    int pixel = ((color->alpha_short >> 8) << 24) |
>+                ((color->red_short >> 8) << 16) |
>+                ((color->blue_short >> 8) << 8) |
>+                (color->green_short >> 8);

The order of the color components here looks unusual - is it really ARBG?! Mozilla-inbound WinXP had a slew of test failures with incorrect colors after this landed, so I suspect it's wrong. I pushed a followup exchanging the blue and green components here:

http://hg.mozilla.org/integration/mozilla-inbound/rev/cdf17433d1f0

If that doesn't fix the XP oranges, we'll back all this out from m-i.
Comment 77 Philipp von Weitershausen [:philikon] 2011-06-17 07:33:21 PDT
Merged follow-up: http://hg.mozilla.org/mozilla-central/rev/cdf17433d1f0
Comment 78 Philipp von Weitershausen [:philikon] 2011-06-17 07:36:40 PDT
Also: http://hg.mozilla.org/mozilla-central/rev/1631f3557eae
Comment 79 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-17 16:07:33 PDT
Thanks
Comment 80 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-22 13:22:42 PDT
This patch did fix the Twinopen regression. However, I need to check that a modified version of Twinopen with an initial window that's not completely blank (i.e. requires a ThebesLayer) did not regress.
Comment 81 ripper5555 2011-08-31 08:33:35 PDT
Just downloaded the v6.01 version of Mozilla Firefox.
In the release notes of 6.0 it was mentioned that this bug was resolved.
http://www.mozilla.org/en-US/firefox/6.0/releasenotes/buglist.html

However the bug is still there.
See the "modified reporter's testcase" and "another testcase".
Scrolling still gives the weird 2 second font rendering problem.
On my site it is also still a problem.

So...did the patches make it into 6.0 or is the release note wrong?
Comment 82 Boris Zbarsky [:bz] 2011-08-31 08:41:05 PDT
The release note is wrong; these patches landed for 6.0 initially, then were backed out, and relanded and stuck for 7.0.

I'll see if I can get the release note (and process for generating them) fixed.
Comment 83 Mihaela Velimiroviciu (:mihaelav) 2011-09-08 05:50:34 PDT
Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0 (beta4)

The font rendering issue is still reproducible on Firefox 7 beta 4 (using test cases from comment 2 , comment 5  and comment 6 )
Comment 84 ripper5555 2011-09-22 05:56:58 PDT
Firefox/7.0 (beta6) also still fails.
Is the fix just a late beta entry or is something else wrong?
Comment 85 Boris Zbarsky [:bz] 2011-09-22 06:34:12 PDT
It sounds like something else is wrong, at least on some configurations.... Could you please file a new bug and put your about:support graphics info in an attachment?
Comment 86 Alice0775 White 2011-09-22 06:40:41 PDT
See Bug 664966, I had already filed.
Comment 87 ripper5555 2011-09-22 07:05:06 PDT
Just added the about:support from 2 different machines and vendors in the new bugreport from Alice0775 White.
Same problems.

Maybe Robert O'Callahan (:roc) can shed some light on this matter?
Comment 88 ripper5555 2011-10-30 05:29:41 PDT
Whatever the reason may be, the bug doesn't look fixed.
Difference in Linux/Windows that triggers it?
The problem still exists within different versions of windows with different GPU vendors (ATI, NVIDIA and INTEL).
All with or without support for Cleartype and/or DirectWrite enabled.
( Bug 664966 look at the Intel part. Chipset hardly supports any hardware acceleration )
Comment 89 Timothy Nikkel (:tnikkel) 2011-10-30 14:25:34 PDT
Patches landed for this bug. Please file a new bug for any remaining issues.
Comment 90 Virtual_ManPL [:Virtual] - (ni? me) 2012-02-23 10:15:33 PST
This patch causes a site tearing while scrolling it, when HW Acc is disabled/not available.
It's fully described in bug #700867
Please look there, because it's marked as UNCONFIRMED for about 4 months, thanks.

Note You need to log in before you can comment on or make changes to this bug.