Last Comment Bug 654950 - [HW-Layers] when using a transparent DIV, and acceleration enabled google maps marker are invisible
: [HW-Layers] when using a transparent DIV, and acceleration enabled google map...
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla6
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
http://www.rtshow.it/
Depends on: 660565
Blocks: 602200 607653
  Show dependency treegraph
 
Reported: 2011-05-05 02:15 PDT by lorife
Modified: 2011-07-28 07:31 PDT (History)
9 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simulate error in local (1.82 KB, application/octet-stream)
2011-05-18 04:20 PDT, lorife
no flags Details
reproduceble sample zip (322.64 KB, application/x-zip-compressed)
2011-05-18 04:40 PDT, Alice0775 White
no flags Details
Almost same of lorife's (320.94 KB, application/x-zip-compressed)
2011-05-18 05:26 PDT, Alice0775 White
no flags Details
Semi-reduced html (1.30 KB, text/html)
2011-05-18 06:25 PDT, Alice0775 White
no flags Details
fix (17.45 KB, patch)
2011-05-18 21:17 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bas: review+
Details | Diff | Splinter Review

Description lorife 2011-05-05 02:15:43 PDT
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

If you click on the link and then on "dove siamo" you'll see that if acceleration hardware is enabled and the div is transparent the marker in google maps is invisible. 

If i disable the acceleration i can see the marker again

Reproducible: Always

Steps to Reproduce:
1.enable acceleration
2.create a transparent div
3.put a google map and a marker in it
4.marker is invisible

Actual Results:  
i can't see the marker in google maps

Expected Results:  
i should have seen the marker

graphic card: nvidia geforce 6600
Comment 1 Alice0775 White 2011-05-05 02:38:45 PDT
Confirmed on
http://hg.mozilla.org/mozilla-central/rev/c3c4c902e9cd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110504 Firefox/6.0a1 ID:20110504030604

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.841.0.0
  Driver Date: 4-5-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.611)
  GPU Accelerated Windows: 1/1 Direct3D 10

It works if set  layers.acceleration.disabled t o true.
Comment 2 Alice0775 White 2011-05-05 03:12:19 PDT
There are 2 regression with enabled layers.acceleration.

#1 Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/af89c96d0939
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101107 Firefox/4.0b8pre ID:20101108010423
Fails but appears after drag scroll:
http://hg.mozilla.org/mozilla-central/rev/f35c89eac392
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101108 Firefox/4.0b8pre ID:20101108023254
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=af89c96d0939&tochange=f35c89eac392


#2 Regression windows:
Fails but it appears after drag scroll:
http://hg.mozilla.org/mozilla-central/rev/d538a677628e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221094054
Fails:
http://hg.mozilla.org/mozilla-central/rev/9b02aa0de19c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221105418
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d538a677628e&tochange=9b02aa0de19c
Comment 3 Boris Zbarsky [:bz] 2011-05-05 06:20:41 PDT
roc, any ideas?  Your stuff seems to be most of that first regression range.
Comment 4 Alice0775 White 2011-05-05 10:31:29 PDT
#1 Regression
build from f35c89eac392 : fails but appears after drag scroll
build from 362567eee982 : works
build from 5d82a6476d53 : works
build from d01932e9114e : works
build from 33652dc3d274 : works
build from 75ca0ab361b8 : works
Triggered by:
f35c89eac392	Robert O'Callahan — Bug 602200. Share code to compute effective transforms and opacity, and snap effective transforms. r=bas,sr=vlad,a=blocker

#2 Regression
build from ab173acf8a1f : fails
build from d538a677628e : fails but appears after drag scroll
Triggered by:
ab173acf8a1f	Oleg Romashin — Bug 607653 - avoid temporary fbos/textures on transformed layers, when possible. r=roc a=approval2.0
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-09 22:19:34 PDT
The link is dead.
Comment 6 lorife 2011-05-10 00:00:21 PDT
(In reply to comment #5)
> The link is dead.

sorry i released it, i changed the link:

www.rtshow.it
Comment 7 lorife 2011-05-10 00:01:16 PDT
by the way, i have this problem also on another pc with an ati hd 4850, if acceleration is enabled in mozilla firefox 4.0.1
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-10 21:34:44 PDT
It works for me, but I have a large set of patches applied...
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-10 21:35:51 PDT
In particular I have patches for bus 648483, 647560, 650228, 637852 and 648277 in my tree. So let's retest once some of those are fixed...
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-10 21:51:07 PDT
Oops. My configuration was wrong, and I still see this bug with all those patches.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-18 03:36:20 PDT
When I save the testcase locally, the marker doesn't work at all. So making a reduced testcase seems hard :-(.

lorife, do you have a testcase other than your entire site? :-)
Comment 12 lorife 2011-05-18 04:20:48 PDT
Created attachment 533235 [details]
simulate error in local
Comment 13 lorife 2011-05-18 04:22:06 PDT
I attached a little test. 

I don't know why now it's half-marker..but still there's some problem.

Please forgive me if it's indented wrong..i copy-pasted the important parts.
Comment 14 Alice0775 White 2011-05-18 04:40:51 PDT
Created attachment 533236 [details]
reproduceble sample zip

Extract to local HDD.
Open "RT Show.htm".
Comment 15 Alice0775 White 2011-05-18 05:26:29 PDT
Created attachment 533255 [details]
Almost same of lorife's
Comment 16 Alice0775 White 2011-05-18 06:25:06 PDT
Created attachment 533269 [details]
Semi-reduced html

Red and blue rectangles should appear at the left-top corner.
Comment 17 Bas Schouten (:bas.schouten) 2011-05-18 09:14:42 PDT
This occurs on GL layers too.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-18 16:39:29 PDT
(In reply to comment #16)
> Created attachment 533269 [details]
> Semi-reduced html
> 
> Red and blue rectangles should appear at the left-top corner.

Amazing work Alice!
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-18 19:46:11 PDT
The problem here is in Layer::CalculateScissorRect:

  if (aIntermediate) {
    scissorRect.MoveBy(- aVisibleRect.TopLeft());
  } else if (clipRect) {
    scissorRect.IntersectRect(scissorRect, aParentScissor);
  }

When a container with an intermediate surface (aIntermediate true) has an immediate child with a cliprect, we correctly adjust the scissorRect to be relative to its intermediate surface (whose top-left is aVisibleRect.TopLeft()). But when that container has a grandchild with a cliprect, the parent container layer's call to CalculateScissorRect does not adjust for the grandparent's intermediate surface top-left.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-18 21:17:21 PDT
Created attachment 533529 [details] [diff] [review]
fix
Comment 21 Bas Schouten (:bas.schouten) 2011-05-18 22:48:44 PDT
Comment on attachment 533529 [details] [diff] [review]
fix

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

::: gfx/layers/Layers.cpp
@@ +309,5 @@
>  }
>  
>  nsIntRect 
> +Layer::CalculateScissorRect(const nsIntRect& aCurrentScissorRect,
> +                            const gfxMatrix* aWorldTransform)

Any reason we shouldn't do const gfxMatrix &aWorldTransform? I hate the name aWorldTransform actually, would've preferred something like GlobalTransform but oh well, this patch isn't the place to change it.

@@ +316,5 @@
> +  NS_ASSERTION(container, "This can't be called on the root!");
> +
> +  // Establish initial clip rect: it's either the one passed in, or
> +  // if the parent has an intermediate surface, it's the extents of that surface.
> +  nsIntRect result;

I think we should rename 'result' to something like 'existingClip' or something like that to make it more obvious throughout the rest of this function what it does.

@@ +347,5 @@
> +    // Find the nearest ancestor with an intermediate surface
> +    do {
> +      container = container->GetParent();
> +    } while (container && !container->UseIntermediateSurface());
> +  }

How about we move this code into the 'else' clause of the previous if statement (i.e. container->UseIntermediateSurface()). We declare clipRect before the first if statement and move if (clipRect->IsEmpty()) etc. into the else clause as well.

In the container->UseIntermediateSurface we'll move it, but that's fine (it'll still be empty).

The only downside would be you'd need to duplicate the if (!clipRect) statement into both branches but I personally think it'll increase the readability a bit.

I'll leave it up to you whether to do this or not.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-19 02:00:54 PDT
(In reply to comment #21)
> ::: gfx/layers/Layers.cpp
> @@ +309,5 @@
> >  }
> >  
> >  nsIntRect 
> > +Layer::CalculateScissorRect(const nsIntRect& aCurrentScissorRect,
> > +                            const gfxMatrix* aWorldTransform)
> 
> Any reason we shouldn't do const gfxMatrix &aWorldTransform?

Yes, then we'd have to construct an identity matrix and pass it in from the D3D9 callers. This is slightly more concise and efficient.

> I hate the name
> aWorldTransform actually, would've preferred something like GlobalTransform
> but oh well, this patch isn't the place to change it.

Yeah, this is consistent with the GL code that's the only caller.

> @@ +316,5 @@
> > +  NS_ASSERTION(container, "This can't be called on the root!");
> > +
> > +  // Establish initial clip rect: it's either the one passed in, or
> > +  // if the parent has an intermediate surface, it's the extents of that surface.
> > +  nsIntRect result;
> 
> I think we should rename 'result' to something like 'existingClip' or
> something like that to make it more obvious throughout the rest of this
> function what it does.

OK.

> @@ +347,5 @@
> > +    // Find the nearest ancestor with an intermediate surface
> > +    do {
> > +      container = container->GetParent();
> > +    } while (container && !container->UseIntermediateSurface());
> > +  }
> 
> How about we move this code into the 'else' clause of the previous if
> statement (i.e. container->UseIntermediateSurface()). We declare clipRect
> before the first if statement and move if (clipRect->IsEmpty()) etc. into
> the else clause as well.

OK.

> In the container->UseIntermediateSurface we'll move it, but that's fine
> (it'll still be empty).
> 
> The only downside would be you'd need to duplicate the if (!clipRect)
> statement into both branches but I personally think it'll increase the
> readability a bit.

OK
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-20 05:30:06 PDT
(In reply to comment #21)
> How about we move this code into the 'else' clause of the previous if
> statement (i.e. container->UseIntermediateSurface()). We declare clipRect
> before the first if statement and move if (clipRect->IsEmpty()) etc. into
> the else clause as well.
> 
> In the container->UseIntermediateSurface we'll move it, but that's fine
> (it'll still be empty).
> 
> The only downside would be you'd need to duplicate the if (!clipRect)
> statement into both branches but I personally think it'll increase the
> readability a bit.
> 
> I'll leave it up to you whether to do this or not.

I tried this, and it got ugly. 'scissor' needs to be defined before the first 'if' too, and then set to *clipRect in both branches. There's too much duplicated code.
Comment 24 Bas Schouten (:bas.schouten) 2011-05-20 09:18:18 PDT
(In reply to comment #23)
> (In reply to comment #21)
> > How about we move this code into the 'else' clause of the previous if
> > statement (i.e. container->UseIntermediateSurface()). We declare clipRect
> > before the first if statement and move if (clipRect->IsEmpty()) etc. into
> > the else clause as well.
> > 
> > In the container->UseIntermediateSurface we'll move it, but that's fine
> > (it'll still be empty).
> > 
> > The only downside would be you'd need to duplicate the if (!clipRect)
> > statement into both branches but I personally think it'll increase the
> > readability a bit.
> > 
> > I'll leave it up to you whether to do this or not.
> 
> I tried this, and it got ugly. 'scissor' needs to be defined before the
> first 'if' too, and then set to *clipRect in both branches. There's too much
> duplicated code.

Oh, I thought scissor could just be set before the first 'if' too.

But it's not that important.
Comment 25 Bas Schouten (:bas.schouten) 2011-05-20 09:23:01 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #21)
> > I tried this, and it got ugly. 'scissor' needs to be defined before the
> > first 'if' too, and then set to *clipRect in both branches. There's too much
> > duplicated code.
> 
> Oh, I thought scissor could just be set before the first 'if' too.
> 
> But it's not that important.

Never mind, of course it can't. But why does it need to be defined before the first 'if'?
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-21 06:04:33 PDT
Because we use it in both the UseIntermediateSurface() and !UseIntermediateSurface() cases.
Comment 27 Bas Schouten (:bas.schouten) 2011-05-21 06:36:14 PDT
(In reply to comment #26)
> Because we use it in both the UseIntermediateSurface() and
> !UseIntermediateSurface() cases.

But only after the if (container) clause which could completely go after the first if/else statement right?
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-22 16:50:04 PDT
I've lost track of what the code would look like. Let's not worry about it.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-22 17:34:54 PDT
http://hg.mozilla.org/projects/cedar/rev/ed0850a1bbb7
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-22 19:46:21 PDT
The test fails on Mac with GL because GL and Quartz alpha computation differs slightly. So I disabled it there.
http://hg.mozilla.org/projects/cedar/rev/54134ae42f5d
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-23 18:31:14 PDT
I'm not sure whether we should try to get this fix into Firefox 5. We probably should at least give it a little bake time on trunk/Aurora. It doesn't have any known duplicates so maybe this case is rare enough fixing it in FF6 is good enough.

Thanks again Alice!
Comment 33 Vlad [QA] 2011-07-28 07:31:11 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0b3

Considering comment16, the two rectangles are present.

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