654950 - [HW-Layers] when using a transparent DIV, and acceleration enabled google maps marker are invisible

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[lorife]

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]

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]

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 [:bzbarsky]]

roc, any ideas?  Your stuff seems to be most of that first regression range.

Comment 4

[Alice0775 White]

#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) (email my personal email if necessary)]

The link is dead.

Comment 6

[lorife]

(In reply to comment #5)
> The link is dead.

sorry i released it, i changed the link:

www.rtshow.it

Comment 7

[lorife]

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) (email my personal email if necessary)]

It works for me, but I have a large set of patches applied...

Comment 9

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

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) (email my personal email if necessary)]

Oops. My configuration was wrong, and I still see this bug with all those patches.

Comment 11

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

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]

Attachment: simulate error in local

Comment 13

[lorife]

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]

Attachment: reproduceble sample zip

Extract to local HDD.
Open "RT Show.htm".

Comment 15

[Alice0775 White]

Attachment: Almost same of lorife's

Comment 16

[Alice0775 White]

Attachment: Semi-reduced html

Red and blue rectangles should appear at the left-top corner.

Comment 17

[Bas Schouten (:bas.schouten)]

This occurs on GL layers too.

Comment 18

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

(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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

Attachment: fix

Comment 21

[Bas Schouten (:bas.schouten)]

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) (email my personal email if necessary)]

(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) (email my personal email if necessary)]

(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)]

(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)]

(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) (email my personal email if necessary)]

Because we use it in both the UseIntermediateSurface() and !UseIntermediateSurface() cases.

Comment 27

[Bas Schouten (:bas.schouten)]

(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) (email my personal email if necessary)]

I've lost track of what the code would look like. Let's not worry about it.

Comment 29

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

http://hg.mozilla.org/projects/cedar/rev/ed0850a1bbb7

Comment 30

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

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 31

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/ed0850a1bbb7
http://hg.mozilla.org/mozilla-central/rev/54134ae42f5d

Comment 32

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

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

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.