Closed Bug 606730 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

ARM
Maemo
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: romaxa, Assigned: tatiana)

References

Details

Attachments

(3 files, 12 obsolete files)

4.05 KB, text/plain
Details
10.40 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.40 KB, patch
roc
: review+
Details | Diff | Splinter Review
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).
OS: Linux → Maemo
Hardware: x86 → ARM
Attached file Render transaction (obsolete) —
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
Attached file Render transaction
Right file
Attachment #485554 - Attachment is obsolete: true
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
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?
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.
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
Attached patch patch (obsolete) — Splinter Review
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
Attachment #487245 - Flags: feedback?(roc)
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.
Assignee: nobody → tanya.meshkova
Attached patch patch v2 (obsolete) — Splinter Review
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
Attachment #487245 - Attachment is obsolete: true
Attachment #487245 - Flags: feedback?(roc)
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #487359 - Attachment is obsolete: true
Attachment #487876 - Flags: review?(roc)
+  }
+  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).
(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.
we are using patch from bug 602200, and we testing this fix with that patch
Attached patch patch v4 (obsolete) — Splinter Review
fixed rotation and reftests failures
added mLastChild
moved all to BasicLayers
Attachment #487876 - Attachment is obsolete: true
Attachment #488230 - Flags: review?(roc)
Attachment #487876 - Flags: review?(roc)
+  }
+  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.
Attached patch patch v5 (obsolete) — Splinter Review
(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.
Attachment #488230 - Attachment is obsolete: true
Attachment #488467 - Flags: review?(roc)
Attachment #488230 - Flags: review?(roc)
Depends on: 602200
Attachment #488467 - Attachment is obsolete: true
Attachment #488646 - Flags: review?(roc)
Attachment #488467 - Flags: review?(roc)
Attachment #488646 - Flags: review?(roc)
Attachment #488646 - Attachment is obsolete: true
Attachment #488661 - Flags: review?(roc)
+// 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"?
Attachment #488661 - Attachment is obsolete: true
Attachment #488661 - Flags: review?(roc)
Attachment #489504 - Flags: review?(roc)
+  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.
(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 ?
Attachment #489504 - Attachment is obsolete: true
Attachment #489838 - Flags: feedback?(roc)
Attachment #489504 - Flags: review?(roc)
(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).
Attachment #489838 - Attachment is obsolete: true
Attachment #489838 - Flags: feedback?(roc)
Nominating for blocking-fennec because this may fix the panning regression from the checkerboard background (bug 593310).
tracking-fennec: --- → ?
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?
tracking-fennec: ? → 2.0-
Attachment #490112 - Flags: review?(roc)
tracking-fennec: 2.0- → 2.0b3+
cjones corrected the blocking issue
Duplicate of this bug: 611790
+  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!
clipping logging was there just temporary
Attachment #490112 - Attachment is obsolete: true
Attachment #490357 - Flags: review?(roc)
Attachment #490112 - Flags: review?(roc)
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.
> 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.)
taking into account containers opacity

see comment above about usefulness.
Attachment #490357 - Attachment is obsolete: true
Attachment #490563 - Flags: review?(roc)
Attachment #490357 - Flags: review?(roc)
Attachment #488645 - Flags: approval2.0?
Attachment #490563 - Flags: approval2.0?
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.