606730 - Remote thebesLayer BG color rendering always, even if it's not visible

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Analyzing fennec basic scrolling (bug 606672) case I found that we rendering BasicColorLayer all the time (pixman_composite_src_n_0565_asm_neon in profile which is coming from BasicColorLayer::PaintColorTo)

658       6.8706  libxul.so                fennec                   pixman_composite_src_0565_0565_asm_neon
653       6.8184  libc-2.5.so              Xorg                     memcpy
433       4.5212  libxul.so                fennec                   pixman_composite_src_n_0565_asm_neon
176       1.8377  oprofiled                oprofiled                /usr/bin/oprofiled

I've disabled BasicColorLayer::PaintColorTo and found that this layer responsible for painting background color in area where thebes layer not yet rendered.

we rendering gray color even when it is not visible, and that is wasting about 10% CPU in full speed rendering testcase (scrolling 56FPS on N900).

Comment 2

[Oleg Romashin (:romaxa)]

ok, seems I've found where it is created:
http://mxr.mozilla.org/mozilla-central/search?string=CreateOrRecycleColorLayer

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Render transaction

Sounds like we should clip or at least disable bottom layer rendering if it is covered by opaque layer, in current case we have ColorLayer covered by Opaque ThebesLayer

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: Render transaction

Right file

Comment 5

[Oleg Romashin (:romaxa)]

Btw, the same problem reproducible for desktop firefox (fennec non-remote)... for example while playing youtube video or any other animation we always painting ColorLayer

Comment 6

[Oleg Romashin (:romaxa)]

This bug very similar to bug 485252...
I guess we should add one cycle from top layer to bottom, and assign clip region for each layer according to transparency and overlapping... and drop layers from list which are has 0x0 clip rect

roc what do you think about it?

Comment 7

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

Culling layers that are covered by other layers in the invalid area is certainly possible. I would try only culling layers that are completely covered.

Comment 8

[Oleg Romashin (:romaxa)]

Ok, IIUC small loop should be added somewhere here, and filter layers list according to visibility state:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicLayers.cpp#1168

Comment 9

[Tatiana Meshkova (:tatiana)]

Attachment: patch

this disables BasicColorLayer when it's not visible
it requires 2f456d0310fa + bug #599507 and bug #602200 patches applied

still need to check if it actually helps in maemo6

Comment 10

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

Something like this could work but I wouldn't write it this way.

It would be more efficient to do a single pass over the whole layer tree from top to bottom in z-order, maintaining a region of "pixels known to be covered by opaque layers", and marking layers that are entirely covered by that region.

Also, you'll need to check the opaqueness of layers via GetContentFlags() & CONTENT_OPAQUE.

Comment 11

[Tatiana Meshkova (:tatiana)]

Attachment: patch v2

ok, tested in maemo6 now, pixman_composite_src_n_0565_asm_neon is gone

this v2 doesn't implement single pass yet, but it
- checks for opaque content now and
- remembers about root transform

Comment 12

[Tatiana Meshkova (:tatiana)]

Attachment: patch v3

Comment 13

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

+  }
+  else {

} else {

+    nsIntRect r = GetEffectiveVisibleRegion().GetBounds();
+    gfxRect gr = transform.Transform(gfxRect(r.x, r.y, r.width, r.height));
+    aRegion.Or(aRegion, nsIntRect(gr.X(), gr.Y(), gr.Width(), gr.Height()));

This is incorrect. For transforms that include rotation, the transformed rect contains areas that aren't actually covered by the layer. Also, you need to round the rect inward instead of just casting from floats to integers.

The recursive calls in MarkLayersCoveredByOpaqueForChildren might become a problem. Instead, since we have mPrevSibling, let's add an mLastChild to ContainerLayer so you can easily traverse from last to first.

There's a coordinate system problem here: what coordinate system is aRegion in? In general, GetEffectiveTransform doesn't always transform all layers to the same coordinate system. With the patches in bug 602200 it does for BasicLayers, but not for the accelerated layer managers. So you may wish to move all this code to BasicLayers for now and add a comment explaining why it can't easily be used for accelerated layer managers (but we probably don't need it for accelerated layer managers anyway).

Comment 14

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

(In reply to comment #13)
> This is incorrect. For transforms that include rotation, the transformed rect
> contains areas that aren't actually covered by the layer. Also, you need to
> round the rect inward instead of just casting from floats to integers.

This could have been caught by a reftest. If all reftests pass with the patch you add, please add a new reftest that fails with your patch.

Comment 15

[Oleg Romashin (:romaxa)]

we are using patch from bug 602200, and we testing this fix with that patch

Comment 16

[Tatiana Meshkova (:tatiana)]

Attachment: patch v4

fixed rotation and reftests failures
added mLastChild
moved all to BasicLayers

Comment 17

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

+  }
+  else {

} else {

I think you need to update the other ContainerLayer implementations to set mLastChild.

MarkLayersCoveredByOpaque should set mCoveredByOpaque for layers that have children (to false, I guess).

+    if (aLayer->GetOpacity() != 1.0f)
+      return;
+
+    if (!(aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE))
+      return;

Shouldn't these checks come after we set mCoveredByOpaque but before we do aRegion.Or?

+    nsIntRect r = aLayer->GetEffectiveVisibleRegion().GetBounds();

If the visible region is not a rectangle, then you'll incorrectly add r to aRegion even though not all of r is opaque.

+    gfxPoint c = gr.TopLeft() + gfxPoint(gr.Width()*0.5f, gr.Height()*0.5f);

I have no idea why you're doing this.

Comment 18

[Tatiana Meshkova (:tatiana)]

Attachment: patch v5

(In reply to comment #17)

> I think you need to update the other ContainerLayer implementations to set
> mLastChild.
I'll make a separate patch

> MarkLayersCoveredByOpaque should set mCoveredByOpaque for layers that have
> children (to false, I guess).
I guess, we don't need to care about Basic containers here at all. I've renamed MarkLayersCoveredByOpaque -> MarkLeafsCoveredByOpaque and added a comment about mCoveredByOpaque.

> +    if (aLayer->GetOpacity() != 1.0f)
> +      return;
> +
> +    if (!(aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE))
> +      return;
> 
> Shouldn't these checks come after we set mCoveredByOpaque but before we do
> aRegion.Or?
yes, thanks. It seems, I've ruined logic while rewriting.

The rest is fixed.

Comment 19

[Tatiana Meshkova (:tatiana)]

Attachment: Part 1: Add mLastChild to all ContainerLayer implementations

Comment 20

[Tatiana Meshkova (:tatiana)]

Attachment: Part 2: MarkLeafsCoveredByOpaque() for BasicLayers

Comment 21

[Tatiana Meshkova (:tatiana)]

Attachment: Part 2: MarkLeafsCoveredByOpaque() for BasicLayers (v6)

Comment 22

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

+// This implementation assumes, that GetEffectiveTransform transforms

Remove comma

+MarkLeafsCoveredByOpaque(Layer* aLayer, nsIntRegion& aRegion)

"Leafs" isn't really right, how about calling this MarkLeafLayersCoveredByOpaque?

You're still being erratic and marking containers with no children with mCoveredByOpaque = PR_FALSE. I would prefer you to set mCoveredByOpaque = PR_FALSE for all containers.

+    TransformIntRect(r, transform);

Here you need to round the resulting rectangle *out*. We need to make sure that aRegion contains everything in the transformed rect, including partial pixels.

+    if (r.IsEmpty()) {
+      return;
+    }

Why bother checking this?

+    // Allow aLayer to cover smth. only if it's completely opaque

"smth"?

Comment 23

[Tatiana Meshkova (:tatiana)]

Attachment: Part 2: MarkLeafsCoveredByOpaque() for BasicLayers (v7)

Comment 24

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

+  void SetCoveredByOpaque(PRBool aCovered) { mCoveredByOpaque = aCovered; }
+  PRBool IsCoveredByOpaque() const { return mCoveredByOpaque; }

These should be in BasicImplData now

MarkLeafLayersCoveredByOpaque needs to check for layer cliprects somehow. A layer with opaque content might not be fully visible because of clipping.

Comment 25

[Tatiana Meshkova (:tatiana)]

Attachment: Part 2: MarkLeafLayersCoveredByOpaque() for BasicLayers (v8)

(In reply to comment #24)
> MarkLeafLayersCoveredByOpaque needs to check for layer cliprects somehow. A
> layer with opaque content might not be fully visible because of clipping.
didn't get it. how bad is it? could you provide some testcase?

anyhow
+    const nsIntRect* clipRect = aLayer->GetEffectiveClipRect();
+    if (clipRect) {
+      region.And(region, *clipRect);
+    }

should I actually apply closest ancestor's clipping if !clipRect ?

Comment 26

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

(In reply to comment #25)
> (In reply to comment #24)
> > MarkLeafLayersCoveredByOpaque needs to check for layer cliprects somehow. A
> > layer with opaque content might not be fully visible because of clipping.
> didn't get it. how bad is it? could you provide some testcase?

I don't have a testcase handy, but the problem should be obvious. A layer that's opaque but clipped (with its own cliprect, or an ancestor with a cliprect), does not get to paint over its entire visible region, so you can't add the whole visible region to the opaque region.

> anyhow
> +    const nsIntRect* clipRect = aLayer->GetEffectiveClipRect();
> +    if (clipRect) {
> +      region.And(region, *clipRect);
> +    }
> 
> should I actually apply closest ancestor's clipping if !clipRect ?

You actually need to combine the clipping from *all* ancestors. Also, the clip rect for each layer is relative to the transform of the parent layer (see the documentation in Layers.h).

Comment 27

[Tatiana Meshkova (:tatiana)]

Attachment: Part 2: MarkLeafLayersCoveredByOpaque() for BasicLayers (v9)

Comment 28

[Matt Brubeck (:mbrubeck)]

Nominating for blocking-fennec because this may fix the panning regression from the checkerboard background (bug 593310).

Comment 29

[Doug Turner (:dougt)]

tatianna -

we are happy to take this when part 2 is reviewed.  When reviewed, just request approval 2.0 on the patch.  Should someone be looking at part 2?

Comment 30

[Doug Turner (:dougt)]

cjones corrected the blocking issue

Comment 32

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

+  if (!aClipRect.IsEmpty() && clipRect) {

You don't need to check aClipRect.IsEmpty() here.

+      aClipRect.IntersectRect(aClipRect, cr);
+    } else {
+      aClipRect.SetRect(0, 0, 0, 0);

Make aClipRect a const nsIntRect& and set a local rect here. Overwriting parameters is bad style.

Can you move the new logging code into a helper function or something? It makes the code harder to read where it is now.

The rest looks good!

Comment 33

[Tatiana Meshkova (:tatiana)]

Attachment: Part 2: MarkLeafLayersCoveredByOpaque() for BasicLayers (v10)

clipping logging was there just temporary

Comment 34

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

Actually I think this code has one more bug. When a container has opacity < 1.0, you're ignoring that, but it actually means its descendants should not add to the opaque region.

Also, can you explain to me why this code is useful? We shouldn't create layers that are completely covered by opaque content, so how are we building a layer tree that contains layers that are covered by opaque content? I thought you had a situation where a color layer extends outside some opaque content, but in the area that's being repainted it is completely covered by the opaque content. But your patch does not look at the area being repainted.

Comment 35

[Oleg Romashin (:romaxa)]

> Also, can you explain to me why this code is useful? We shouldn't create layers
> that are completely covered by opaque content, so how are we building a layer
> tree that contains layers that are covered by opaque content? I thought you had

Color layer created for rendering dirty area of shadowThebesLayer. and it is created in advance, in order to be painted when scrollViewPortBy moving thebesLayer inside screen view (checker board.)

Comment 36

[Tatiana Meshkova (:tatiana)]

Attachment: Part 2: MarkLeafLayersCoveredByOpaque() for BasicLayers (v11)

taking into account containers opacity

see comment above about usefulness.

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/2646f83e3d80 and http://hg.mozilla.org/mozilla-central/rev/133ad6c08288