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

VERIFIED FIXED in mozilla6

Status

()

Core
Graphics
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: lorife, Assigned: roc)

Tracking

({regression})

Trunk
mozilla6
x86
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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.
Status: UNCONFIRMED → NEW
Component: Preferences → Graphics
Ever confirmed: true
Product: Firefox → Core
QA Contact: preferences → thebes

Comment 2

6 years ago
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
Keywords: regression
Summary: when using a transparent DIV, and acceleration enabled google maps marker are invisible → [D3D9/10] when using a transparent DIV, and acceleration enabled google maps marker are invisible
Version: unspecified → Trunk
roc, any ideas?  Your stuff seems to be most of that first regression range.

Comment 4

6 years ago
#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
Blocks: 602200, 607653
The link is dead.
(Reporter)

Updated

6 years ago
(Reporter)

Comment 6

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

sorry i released it, i changed the link:

www.rtshow.it
(Reporter)

Comment 7

6 years ago
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
It works for me, but I have a large set of patches applied...
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...
Oops. My configuration was wrong, and I still see this bug with all those patches.
Assignee: nobody → roc
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? :-)
(Reporter)

Comment 12

6 years ago
Created attachment 533235 [details]
simulate error in local
(Reporter)

Comment 13

6 years ago
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

6 years ago
Created attachment 533236 [details]
reproduceble sample zip

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

Updated

6 years ago
Attachment #533236 - Attachment is obsolete: true

Comment 15

6 years ago
Created attachment 533255 [details]
Almost same of lorife's

Comment 16

6 years ago
Created attachment 533269 [details]
Semi-reduced html

Red and blue rectangles should appear at the left-top corner.
Attachment #533255 - Attachment is obsolete: true
This occurs on GL layers too.
Summary: [D3D9/10] when using a transparent DIV, and acceleration enabled google maps marker are invisible → [HW-Layers] when using a transparent DIV, and acceleration enabled google maps marker are invisible
(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!
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.
Created attachment 533529 [details] [diff] [review]
fix
Attachment #533529 - Flags: review?
Attachment #533529 - Flags: review? → review?(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.
Attachment #533529 - Flags: review?(bas.schouten) → review+
(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
Whiteboard: [needs landing]
(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.
(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.
(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'?
Because we use it in both the UseIntermediateSurface() and !UseIntermediateSurface() cases.
(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?
I've lost track of what the code would look like. Let's not worry about it.
http://hg.mozilla.org/projects/cedar/rev/ed0850a1bbb7
Flags: in-testsuite+
Whiteboard: [needs landing] → [fixed-in-cedar]
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
http://hg.mozilla.org/mozilla-central/rev/ed0850a1bbb7
http://hg.mozilla.org/mozilla-central/rev/54134ae42f5d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla6
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!
Depends on: 660565

Comment 33

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.