Closed Bug 647560 Opened 13 years ago Closed 13 years ago

Incorrect font rendering with 2 sec delay greyscale->subpixel on scrollable div

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla7

People

(Reporter: ripper5555, Assigned: roc)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(11 files, 4 obsolete files)

Testcase
130.15 KB, application/zip
Details
modified reporter's testcase
20.62 KB, text/html
Details
another testcase
31.51 KB, text/html
Details
Simplified testcase
7.18 KB, text/html
Details
Clean up MarkLeafLayersHidden and make it set the hidden state on container layers
5.69 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
Add support for compositing BasicLayers with OPERATOR_SOURCE
18.22 KB, patch
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
tnikkel
: review+
Details | Diff | Splinter Review
Cache temporary backbuffer surfaces
19.06 KB, patch
karlt
: review+
Details | Diff | Splinter Review
unfixed testcase
7.24 KB, text/html
Details
Use passed-in aContext instead of getting it from the layer manager
1.13 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
performance regression fix v3
3.87 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review 
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...
Attached file Tetcase (obsolete) — 

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase
        — 
      

    


  

        Attachment #523872 -
        Attachment is obsolete: true

    
    

    
  
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
Blocks: 602757
Status: UNCONFIRMED → NEW
Component: General → Graphics
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: general → thebes
Version: unspecified → 2.0 Branch

    
    

    
  
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 .

    
    

    
  

      
      
      
      

        Attached file
          
        another testcase
        — 
      

    


  
Sorry, Regression range in comment#3 is wrong.

    
  
No longer blocks: 602757

    
    

    
  
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
Blocks: 363861, 602757

    
  
Blocks: 579276
No longer blocks: 363861

    
    

    
  
(In reply to comment #8)
> Related to bug #626293 maybe ?

I do not think so, because regression range is different.

    
    

    
  
The reports of "bug 618211" and "bug 630879" seem to be explaining the same symptoms.
One is assigned.
Link those bugs with this one?

    
    

    
  
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.

        Attachment #524901 -
        Attachment is patch: false

        Attachment #524901 -
        Attachment mime type: text/plain → text/html

    
    

    
  

      
      
      
      

        Attached file
          
        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.

    
    

    
  
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.

    
    

    
  
... 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 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.

        Attachment #526903 -
        Flags: review?(tnikkel) → review+

    
    

    
  
Thank you!
Whiteboard: [needs landing]

    
    

    
  
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 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.)

        Attachment #526923 -
        Flags: review?(karlt) → review+

    
    

    
  
(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 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 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?

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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!

    
    

    
  
(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.

    
    

    
  
(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 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.

        Attachment #526922 -
        Flags: review?(tnikkel) → review+

    
    

    
  
Comment on attachment 526904 [details] [diff] [review]
Add support for compositing BasicLayers with OPERATOR_SOURCE

r+ with the changes mentioned.

        Attachment #526904 -
        Flags: review?(tnikkel) → review+

    
    

    
  
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?

    
    

    
  
No. Assuming you're talking about BasicLayerManager::PushGroupForLayer, PushGroupAndCopyBackground with CONTENT_COLOR is always just the same as a regular PushGroup.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
Those patches don't apply on m-c - are there any prerequisites?

    
    

    
  
Part 2 and/or 3 from bug 629866.

    
    

    
  
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

    
    

    
  
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).

    
    

    
  
The orange is still there so this patch should be safe to be pushed again in cedar later.
Target Milestone: --- → mozilla6

    
    

    
  
I did backout the code because it broke mobile on desktop (bug 658332)

http://hg.mozilla.org/mozilla-central/rev/f3b87db5dc4e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  


  
This patch fixes the regression with Fennec.

        Attachment #535977 -
        Flags: review?(tnikkel)

        Attachment #535977 -
        Flags: review?(tnikkel) → review+

    
    

    
  
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?

    
    

    
  
I'll do some profiling. Maybe we need some patches from bug 629866 here. In the meantime we should probably back out.

    
    

    
  
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...

    
    

    
  
Armen, is it possible to get the graphics section from about:support from a machine that runs Txul on Windows 7 64-bit?

    
    

    
  
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?

    
    

    
  
Yes, 64-bit build running on a 64-bit OS.

    
    

    
  
OK, let me try to figure this out.

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
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

    
    

    
  
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.

    
    

    
  


  
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?

        Attachment #537449 -
        Flags: review?

    
    

    
  
(In reply to comment #60)
> Created attachment 537449 [details] [diff] [review] [review]

Roc, did you meant to assign this review to no one?

    
    

    
  
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 :-)

        Attachment #537449 -
        Flags: review? → review?(jfkthame)

    
    

    
  
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.

        Attachment #537449 -
        Attachment is obsolete: true

        Attachment #537449 -
        Flags: review?(jfkthame)

    
    

    
  
I think I can reproduce the Twinopen regression in a 32-bit build.

    
    

    
  
Or rather, on a 32-bit build I see BasicLayers having a significantly higher twinopen than D3D10 layers.

    
    

    
  
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.

    
    

    
  
I can definitely reproduce the regression. It's the ApplyDoubleBuffering patch, unsurprisingly.

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        performance regression fix (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #538174 -
        Flags: review?(jmuizelaar)

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        performance regression fix v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #538174 -
        Attachment is obsolete: true

        Attachment #538174 -
        Flags: review?(jmuizelaar)

        Attachment #538362 -
        Flags: review?(jmuizelaar)

    
    

    
  


  
Oops, fix print_gdi_error string

        Attachment #538362 -
        Attachment is obsolete: true

        Attachment #538362 -
        Flags: review?(jmuizelaar)

    
    

    
  
Comment on attachment 538363 [details] [diff] [review]
performance regression fix v3

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

        Attachment #538363 -
        Flags: review?(jmuizelaar)

        Attachment #538363 -
        Flags: review?(jmuizelaar) → review+
Whiteboard: [needs landing]

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
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 )

    
    

    
  
Firefox/7.0 (beta6) also still fails.
Is the fix just a late beta entry or is something else wrong?

    
    

    
  
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?

    
    

    
  
See Bug 664966, I had already filed.

    
    

    
  
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?

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
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 )

    
    

    
  
Patches landed for this bug. Please file a new bug for any remaining issues.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED

    
    

    
  
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.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: