Closed Bug 907048 Opened 6 years ago Closed 6 years ago

Skip colorlayer if other opacity layer just covers the colorlayer region

Categories

(Core :: Graphics: Layers, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: pchang, Assigned: pchang)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

The following are layer tree dump from home screen.

We could skip colorlayer because of thebesLayer 0x441d9000 covers on it.
I will check the correctness of opacity property from my layer tree dump.

I/Gecko   (20583): aLayer 0x441d7c00 name ContainerLayer fmt 0 vis(0 0 320 480) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):   aLayer 0x441d8800 name ThebesLayerComposite fmt 0 vis(0 0 0 0) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):   aLayer 0x441d9400 name ColorLayer fmt 0 vis(0 0 0 0) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):   aLayer 0x441d8c00 name ContainerLayer fmt 0 vis(0 0 320 480) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):     aLayer 0x441d9000 name ThebesLayerComposite fmt 4 vis(0 0 320 480) sf(320 480) visReg xxx offset1 (0 0) opacity 1.00
I/Gecko   (20583):     aLayer 0x4b8b0c00 name RefLayer fmt 0 vis(0 0 320 460) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):       aLayer 0x4b8e8000 name ContainerLayer fmt 0 vis(0 0 320 460) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):         aLayer 0x451b5000 name ThebesLayerComposite fmt 1 vis(0 0 320 460) sf(320 460) visReg xxx offset1 (0 0) opacity 1.00
I/Gecko   (20583):         aLayer 0x469bec00 name ContainerLayer fmt 0 vis(0 0 320 461) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):           aLayer 0x46c16800 name ThebesLayerComposite fmt 1 vis(0 0 320 461) sf(320 461) visReg xxx offset1 (0 0) opacity 1.00
I/Gecko   (20583):         aLayer 0x46dd6000 name ContainerLayer fmt 0 vis(0 0 64 3) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):           aLayer 0x472eb400 name ThebesLayerComposite fmt 0 vis(0 0 0 0) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):           aLayer 0x472ec000 name ColorLayer fmt 0 vis(0 0 64 3) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
I/Gecko   (20583):         aLayer 0x46c18c00 name ThebesLayerComposite fmt 1 vis(0 385 320 75) sf(320 460) visReg xxx offset1 (0 0) opacity 1.00
I think we can do this more generally.  When painting a container layer, we should traverse the child layers in reverse painting order first and for each layer determine the guaranteed covered region (covered with opaque pixels). We can keep that region and subtract it from the visible region as we paint.

For example, the 5th layer is a 320x480 thebes layer, and its fully visible (320x480). However, we should easily be able to see that the content layer (RefLayer here) covers with opaque pixels 320x460 of that, so we actually should have shrunk the visible area of the 5th layer to 320x20.

This approach would avoid a bunch of overpainting in the compositor.

There is also a clear bug here with the 460x461 layers here. Probably the 1-pixel delta is caused by gaia. We should check that. And of course we should reduce the overpaint here with the above scheme (ideally to 0 instead of to a 320x1 visible layer).
I create another bug 907109 for detail home screen layer analysis.
We could discuss overpaint issue there.

For the following (3rd) colorlayer, we could discard because no visible region.

I/Gecko   (20583):   aLayer 0x441d9400 name ColorLayer fmt 0 vis(0 0 0 0) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00

But we may need to handle color layer for camera/video. I will paste layer tree dump for camera/video later.
Depends on: 907109
Update layer tree when playing video.

composition start
 aLayer 0x4851ec00 in   1 name ContainerLayer fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
 aLayer 0x4851f400 in   2 name ThebesLayerComposite fmt 0 cntflag 0x0 vis(0 0 0 0) sf(0 0) opacity 1.00
 aLayer 0x4851f400 empty visibleRegion
 aLayer 0x48520000 in   3 name ColorLayer fmt 0 cntflag 0x1 vis(0 0 0 0) sf(0 0) opacity 1.00
 aLayer 0x48520000 empty visibleRegion
   aLayer 0x4851f800 in 100 name ContainerLayer fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
   aLayer 0x4851fc00 in 101 name ThebesLayerComposite fmt 4 cntflag 0x1 vis(0 0 0 0) sf(320 480) opacity 1.00
   aLayer 0x4851fc00 empty visibleRegion
   aLayer 0x4774b800 in 102 name ColorLayer fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
   aLayer 0x4774b800 scissorRect(0 0 320 480)
//caused by background image
     aLayer 0x48749800 in 200 name RefLayer fmt 0 cntflag 0x0 vis(0 0 320 480) sf(0 0) opacity 1.00
       aLayer 0x4865ac00 in 300 name ContainerLayer fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
       aLayer 0x48660800 in 301 name ThebesLayerComposite fmt 4 cntflag 0x1 vis(0 0 320 480) sf(320 480) opacity 1.00
       aLayer 0x48660800 scissorRect(0 0 320 480)
       aLayer 0x486c8000 in 302 name ImageLayer fmt 266 cntflag 0x1 vis(0 0 320 240) sf(320 240) opacity 1.00
       aLayer 0x486c8000 scissorRect(0 0 320 480)


This color layer(0x4774b800) was generated from background wallpaper layer. And we should skip this color layer because it was covered by the next ThebesLayerComposite layer (0x48660800).

aLayer 0x4851fc00 in 101 name ThebesLayerComposite fmt 4 cntflag 0x1 vis(0 0 0 0) sf(320 480) opacity 1.00
   aLayer 0x4851fc00 empty visibleRegion
   aLayer 0x4774b800 in 102 name ColorLayer fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
   aLayer 0x4774b800 scissorRect(0 0 320 480)
Peter, can you make me a test case for this? A page as simple as possible showing this. I have a patch for this and I want to test it.
(In reply to Andreas Gal :gal from comment #4)
> Peter, can you make me a test case for this? A page as simple as possible
> showing this. I have a patch for this and I want to test it.

Above layer tree was dumped from video app (fullscreen app) from my leo device.
I tried to use browser to launch a simple video test web page but the layer tree was not the same as I dump above. Therefore, it is better to use b2g video app to reproduce this issue.

If needs, I can help to check your patch.
Peter, can you please try to re-create a simple static HTML5 page that causes a similar layer tree that shows the same problem. That will greatly help automated testing and investigation. Do we need a cross process layer tree for example? Or can we show the same problem just same-process?
(In reply to Andreas Gal :gal from comment #6)
> Peter, can you please try to re-create a simple static HTML5 page that
> causes a similar layer tree that shows the same problem. That will greatly
> help automated testing and investigation. Do we need a cross process layer
> tree for example? Or can we show the same problem just same-process?

I think it only happened for cross process, because layout should be able to optimize it if under same process. For cross-process case, only the compositor on chrome process notices this overpainting issue.

I will try to create simple test case and patch to fix this issue.
Attached patch overcomposite-patch (obsolete) — Splinter Review
WIP (doesn't work, ran out of time on the plane)
I think bug 911471 will solve this problem. Please dup once confirmed.
Depends on: 911471
Attachment #797065 - Attachment is obsolete: true
We need a patch here to use the newly added GetScissorRectsForSortedChildren from bug 911471 in the HWC to reduce the clipping rect of layers (or eliminate them entirely if the shrunk clipping rect is empty).
From my testing, it is not enough to check only scissorRect. For example, the following three thebeslayers all have the same scissor rect but they are displayed at different screen region based on different transform.

By the way, I had a local patch to save current traversed layerToRender to deferredCompositreLayer inside compositeManager and then continue to traverse the next layer.

It will check the displayed region between deferredCompositeLayer and layerToRender. If equals, it skip the deferredCompostieLayer. If not, it renders the deferredCompositeLayer during that run. Therefore, my path doesn't have to add new traverse for next composite layer.

Right now I'm checking the validation of the displayed region of every compositelayers.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp#148

aLayer 0x462e3800 in 300 name ContainerLayerComposite fmt 0 cntflag 0x2 vis(0 0 320 460) sf(0 0) opacity 1.00
aLayer 0x4650b400 in 301 name ThebesLayerComposite fmt 1 cntflag 0x0 vis(0 0 320 460) sf(320 460) opacity 1.00
aLayer 0x4650b400 scissorRect(0 20 320 460) tranRect(0 40 320 460) t[ 1 0; 0 1; 0 20; ]
  aLayer 0x46512400 in 400 name ContainerLayerComposite fmt 0 cntflag 0xa vis(0 0 320 385) sf(0 0) opacity 1.00
  aLayer 0x46512800 in 401 name ThebesLayerComposite fmt 1 cntflag 0x2 vis(0 0 320 385) sf(320 385) opacity 1.00
  aLayer 0x46512800 scissorRect(0 20 320 460) tranRect(-40 40 320 460) t[ 1 0; 0 1; -40 20; ]
  aLayer 0x46d5d400 in 425 name ContainerLayerComposite fmt 0 cntflag 0xa vis(0 0 320 385) sf(0 0) opacity 1.00
  aLayer 0x46d60400 in 426 name ThebesLayerComposite fmt 1 cntflag 0x2 vis(0 0 320 385) sf(320 385) opacity 1.00
  aLayer 0x46d60400 scissorRect(0 20 320 460) tranRect(280 40 320 460) t[ 1 0; 0 1; 280 20; ]


(In reply to Andreas Gal :gal from comment #10)
> We need a patch here to use the newly added GetScissorRectsForSortedChildren
> from bug 911471 in the HWC to reduce the clipping rect of layers (or
> eliminate them entirely if the shrunk clipping rect is empty).
Assignee: nobody → pchang
(In reply to peter chang[:pchang] from comment #11)
> By the way, I had a local patch to save current traversed layerToRender to
> deferredCompositreLayer inside compositeManager and then continue to
> traverse the next layer.
Could you share your local patch here? It does not need to work. With you patch, we may know how many factors we need to fix for this feature.
Flags: needinfo?(pchang)
Can you dump a full layer tree and display list here? I'd like to understand why layer visible regions haven't been restricted enough to avoid overdrawing.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Can you dump a full layer tree and display list here? I'd like to understand
> why layer visible regions haven't been restricted enough to avoid
> overdrawing.

#1 b2g background       ColorLayerComposite (0x4567f000) (x=0, y=0, w=320, h=480) opaqueContent
#2 video app background ColorLayerComposite (0x44c58400) (x=0, y=0, w=320, h=480) opaqueContent
#3 video                ImageLayerComposite (0x44c58c00) (x=0, y=0, w=320, h=480) [shadow-transform=[ 1 0; 0 1; 0 120; ]

For video app, it has three layers for composition.
But #1 is covered by #2. I could detect this kind of condition during GPU composition.

  LayerManager (0x44c937b0)
    ContainerLayerComposite (0x47c04c00) [shadow-visible=< (x=0, y=0, w=320, h=480); >] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=320.000000, h=480.000000) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollId=0 }]
      ThebesLayerComposite (0x47c05c00) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible] [isFixedPosition anchor=0.000000,0.000000]

      ColorLayerComposite (0x47c06c00) [not visible] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000] [color=rgba(0, 0, 0, 1)]
      ContainerLayerComposite (0x47c06000) [shadow-clip=(x=0, y=0, w=320, h=480)] [shadow-visible=< (x=0, y=0, w=320, h=480); >] [clip=(x=0, y=0, w=320, h=480)] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000]
        ThebesLayerComposite (0x47c06400) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000]
          ContentHostDoubleBuffered (0x45b70ca0) [buffer-rect=(x=0, y=0, w=320, h=480)] [buffer-rotation=(x=0, y=0)]
            GrallocDeprecatedTextureHostOGL (0x43fdf7c0) [size=(width=320, height=480)] [format=FORMAT_R5G6B5] [flags=TEXTURE_USE_NEAREST_FILTER|TEXTURE_DEALLOCATE_HOST]
            GrallocDeprecatedTextureHostOGL (0x43fdf790) [size=(width=320, height=480)] [format=FORMAT_R5G6B5] [flags=TEXTURE_USE_NEAREST_FILTER|TEXTURE_DEALLOCATE_HOST]
        ColorLayerComposite (0x4567f000) [shadow-visible=< (x=0, y=0, w=320, h=480); >] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000] [color=rgba(255, 255, 255, 1)]
        RefLayerComposite (0x485e4800) [shadow-clip=(x=0, y=0, w=320, h=480)] [shadow-visible=< (x=0, y=0, w=320, h=480); >] [clip=(x=0, y=0, w=320, h=480)] [visible=< (x=0, y=0, w=320, h=480); >] [isFixedPosition anchor=0.000000,0.000000] [id=4]
          ContainerLayerComposite (0x4653a400) [shadow-visible=< (x=0, y=0, w=320, h=480); >] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=320.000000, h=480.000000) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollId=1 }]
            ThebesLayerComposite (0x46a32000) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000]
              ContentHostDoubleBuffered (0x44c09350) [buffer-rect=(x=0, y=0, w=320, h=480)] [buffer-rotation=(x=0, y=0)]
                GrallocDeprecatedTextureHostOGL (0x46e6ba90) [size=(width=320, height=480)] [format=FORMAT_R5G6B5] [flags=TEXTURE_USE_NEAREST_FILTER|TEXTURE_DEALLOCATE_HOST]
                GrallocDeprecatedTextureHostOGL (0x46e6bc70) [size=(width=320, height=480)] [format=FORMAT_R5G6B5] [flags=TEXTURE_USE_NEAREST_FILTER|TEXTURE_DEALLOCATE_HOST]
            ColorLayerComposite (0x44c58400) [shadow-visible=< (x=0, y=0, w=320, h=480); >] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000] [color=rgba(0, 0, 0, 1)]
            ImageLayerComposite (0x44c58c00) [shadow-clip=(x=0, y=0, w=320, h=480)] [shadow-transform=[ 1 0; 0 1; 0 120; ]] [shadow-visible=< (x=0, y=0, w=320, h=240); >] [clip=(x=0, y=0, w=320, h=480)] [transform=[ 1 0; 0 1; 0 120; ]] [visible=< (x=0, y=0, w=320, h=240); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000]
              DeprecatedImageHostSingle (0x46e29dc0) [picture-rect=(x=0, y=0, w=320, h=240)]
                GrallocDeprecatedTextureHostOGL (0x45928f70) [size=(width=320, height=240)] [format=FORMAT_R8G8B8A8] [flags=NoFlags]
Flags: needinfo?(pchang)
Detect the over composition during layer tree traverse.

Not sure this problem was caused by layout engine or we have to fix inside layer composition.
Attachment #799298 - Flags: feedback?(roc)
OK, the issue here is that display list construction for the b2g process does not know that the video app is completely opaque, so it's unable to prove that the b2g background is completely hidden by the video app. RenderFrameParent::BuildDisplayList builds an nsDisplayRemote representing the layer tree of the app, but because layer tree updates are sent directly to the compositor and don't go through the b2g process's main thread, we don't know whether the layer tree is opaque enough until we're in the compositor and the layer trees are hooked up.

Occlusion culling in the compositor would fix this, but we don't have to try very hard since this situation is quite specialized. The whole situation is sort of annoying since apps would not normally be transparent in B2G. I guess some special ones like the keyboard are.
We can't transmit information from the compositor thread to the main thread indicating that the subprocess layer tree is opaque, since that could change at any time due to layer tree updates that don't go through the main thread.
I think we can write a simple patch to ContainerRender that just tests whether layerToRender's next sibling covers layerToRender, and if so, skip rendering layerToRender. We can customize that check to the specific situation we care about here.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> I think we can write a simple patch to ContainerRender that just tests
> whether layerToRender's next sibling covers layerToRender, and if so, skip
> rendering layerToRender. We can customize that check to the specific
> situation we care about here.

Agree with your comment. 
I uploaded attachment 799298 [details] [diff] [review] for first draft patch. 
It could detect the redundant composition in compositor side.

Could you give me your comment about the implementation?
Comment on attachment 799298 [details] [diff] [review]
avoid redundant colorlayer composition

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +150,5 @@
> +      gfxMatrix transform;
> +      nsIntRect screenClip;
> +
> +      //check 2d transform are right???
> +      if (layerToRender->GetLayer()->GetEffectiveTransform().CanDraw2D(&transform)) {

We don't need to handle non-identity transforms here.

We do need to check for opacity and masking somewhere.

::: gfx/layers/composite/LayerManagerComposite.h
@@ +261,5 @@
>    bool PlatformDestroySharedSurface(SurfaceDescriptor* aSurface);
>    RefPtr<Compositor> mCompositor;
>  
> +  void SetDeferredCompositeLayerInfo(DeferredCompositeLayerInfo info) { mDeferredCompositeLayerInfo = info; }
> +  DeferredCompositeLayerInfo GetDeferredCompositeLayerInfo() { return mDeferredCompositeLayerInfo; }

It's not immediately obvious what this does.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Created attachment 799402 [details] [diff] [review]
> patch?
> 
> Does this work?

It crashed when I launch video app.

BTW, your patch only check the next sibling, but the problem happened at another Ref layer tree.
Will it work?
Comment on attachment 799298 [details] [diff] [review]
avoid redundant colorlayer composition

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +150,5 @@
> +      gfxMatrix transform;
> +      nsIntRect screenClip;
> +
> +      //check 2d transform are right???
> +      if (layerToRender->GetLayer()->GetEffectiveTransform().CanDraw2D(&transform)) {

Here is tried to calculate the screen clip of each layer and then uses to compare the screenclip with last deferred compositelayer.

Why don't we need to handle non-identity transforms.

::: gfx/layers/composite/LayerManagerComposite.h
@@ +261,5 @@
>    bool PlatformDestroySharedSurface(SurfaceDescriptor* aSurface);
>    RefPtr<Compositor> mCompositor;
>  
> +  void SetDeferredCompositeLayerInfo(DeferredCompositeLayerInfo info) { mDeferredCompositeLayerInfo = info; }
> +  DeferredCompositeLayerInfo GetDeferredCompositeLayerInfo() { return mDeferredCompositeLayerInfo; }

These function are trying to cache/query the data for opaque layercomposite and use it to detect the over composition problem.

How about the following version?

//Save the data for deferred opaque LayerComposite and then compare the screenclip with next opaque LayerComposite.
void DeferredOpaqueLayerComposite(LayerComposite* layer, nsIntRect screenRect, nsIntRect scissorClip, nsIntpoint childOffset)

//Get the data of last deferred LayerComposite
CompositeLayerInfo GetDeferredOpaqueLayerComposite() {return mDeferredCompositeLayerInfo;}
Attachment #799298 - Attachment is obsolete: true
Attachment #799298 - Flags: feedback?(roc)
Attachment #800577 - Flags: review?(roc)
(In reply to peter chang[:pchang] from comment #22)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> > Created attachment 799402 [details] [diff] [review]
> > patch?
> > 
> > Does this work?
> 
> It crashed when I launch video app.

I guess there's a bug, but it shouldn't be hard to figure out.

> BTW, your patch only check the next sibling, but the problem happened at
> another Ref layer tree.
> Will it work?

No, but in the layer tree you showed, the RefLayer is the next sibling of the ColorLayer. If there are important cases where we need to search later siblings, we can easily do that.

We shouldn't make this code more complex than it needs to be for the cases we care about.

(In reply to peter chang[:pchang] from comment #23)
> Comment on attachment 799298 [details] [diff] [review]
> ::: gfx/layers/composite/ContainerLayerComposite.cpp
> @@ +150,5 @@
> > +      gfxMatrix transform;
> > +      nsIntRect screenClip;
> > +
> > +      //check 2d transform are right???
> > +      if (layerToRender->GetLayer()->GetEffectiveTransform().CanDraw2D(&transform)) {
> 
> Here is tried to calculate the screen clip of each layer and then uses to
> compare the screenclip with last deferred compositelayer.

"screen clip" isn't the right phrase to use. You're computing the combination of the clipped visible rect.

> Why don't we need to handle non-identity transforms.

Because it seems unlikely we'll apply a CSS transform to the app's <browser> element or the background behind it.

> ::: gfx/layers/composite/LayerManagerComposite.h
> @@ +261,5 @@
> >    bool PlatformDestroySharedSurface(SurfaceDescriptor* aSurface);
> >    RefPtr<Compositor> mCompositor;
> >  
> > +  void SetDeferredCompositeLayerInfo(DeferredCompositeLayerInfo info) { mDeferredCompositeLayerInfo = info; }
> > +  DeferredCompositeLayerInfo GetDeferredCompositeLayerInfo() { return mDeferredCompositeLayerInfo; }
> 
> These function are trying to cache/query the data for opaque layercomposite
> and use it to detect the over composition problem.

We shouldn't need to add any extra state to the layer. We should be able to do this entirely inside the ContainerRender function.
Comment on attachment 800577 [details] [diff] [review]
Detect redundant composition when multiple opaque layers exist

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +55,5 @@
> +
> +  gfx3DMatrix transform = layer->GetEffectiveTransform();
> +  const nsIntRegion& visibleRegion = layer->GetEffectiveVisibleRegion();
> +  gfxRect rect(visibleRegion.GetBounds());
> +  rect = transform.TransformBounds(rect);

The CONTENT_OPAQUE flag indicates that the layer paints opaquely in its visible region. But here, rect includes areas outside the layer's (transformed) visible region --- because first you're taking the rectangular bounds of the region, and also because you transform that rectangle using a transform that might include rotation. So there is no guarantee that the layer will paint the contents of 'rect' opaquely.
Comment on attachment 800577 [details] [diff] [review]
Detect redundant composition when multiple opaque layers exist

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +55,5 @@
> +
> +  gfx3DMatrix transform = layer->GetEffectiveTransform();
> +  const nsIntRegion& visibleRegion = layer->GetEffectiveVisibleRegion();
> +  gfxRect rect(visibleRegion.GetBounds());
> +  rect = transform.TransformBounds(rect);

This function is used to calculate the screen clip for every layer no matter it is opaque or not and the result will be used to compare with deferred opaque layer. P.S. I should multiply the world transform too.

But actually I don't have to calculate the screen clip for next layer if it is not opaque.
I can just render the deferred layer directly.

So we couldn't guarantee the layer opaque region if it is CONTENT_OPAQUE and has the rotation transform. Am I right?
Update for calculation screen clip only for opaque layer
Attachment #800577 - Attachment is obsolete: true
Attachment #800577 - Flags: review?(roc)
Attachment #800725 - Flags: review?(roc)
Comment on attachment 800725 [details] [diff] [review]
Detect redundant composition when multiple opaque layers exist v2

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

::: gfx/layers/composite/LayerManagerComposite.h
@@ +324,5 @@
>    bool mInTransaction;
> +
> +  //LayerComposite Data for redundant compositio detection
> +  //when multiple opaque layers exist
> +  DeferredLayerCompositeData mDeferredLayerCompositeData;

You haven't explained why you need to add data to the layer.

Can we just try my approach?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 800725 [details] [diff] [review]
> Detect redundant composition when multiple opaque layers exist v2
> 
> Review of attachment 800725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.h
> @@ +324,5 @@
> >    bool mInTransaction;
> > +
> > +  //LayerComposite Data for redundant compositio detection
> > +  //when multiple opaque layers exist
> > +  DeferredLayerCompositeData mDeferredLayerCompositeData;
> 
> You haven't explained why you need to add data to the layer.
> 
> Can we just try my approach?

Ohoh...I just saw your feedback on comment 25.

I also had a little concern about my approach because it makes the code more complex.

Okay...I will try to debug your patch to see why it crash and update soon.
Comment on attachment 799402 [details] [diff] [review]
patch?

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +68,5 @@
> +    // Drill down into RefLayers because that's what we particularly care about;
> +    // layer construction for aLayer will not have known about the opaqueness
> +    // of any RefLayer subtrees.
> +    RefLayer* refLayer = aLayer->AsRefLayer();
> +    if (refLayer) {

change to if (refLayer && refLayer->GetFirstChild()) {

@@ +167,5 @@
>      LayerComposite* layerToRender = static_cast<LayerComposite*>(children.ElementAt(i)->ImplData());
>  
> +    if (i + 1 < children.Length() &&
> +        layerToRender->GetLayer()->GetEffectiveTransform().IsIdentity()) {
> +      LayerComposite* nextLayer = static_cast<LayerComposite*>(children.ElementAt(i + 1)->ImplData());

Add nextlayer checking here

if (nextLayer && nextLayer->GetLayer()) {


With these two changes, b2g won't crash
#1 GPU    aLayer 0x47333400 in 102 name ColorLayerComposite vis(0 0 0 0)
#2 GPU        aLayer 0x48bf7000 in 302 name ColorLayerComposite vis(0 0 320 480)
#3 GPU        aLayer 0x4735d400 in 303 name ImageLayerComposite vis(0 0 320 240)

Origin the video app will compose above three layers.

With your patch, the visible region of #1 colorlayer become empty.
As we expect, it tried to get the opaqueRect from RefLayer and compare the rect with #1 colorlayer.

GPU    aLayer 0x47333400 in 102 name ColorLayerComposite fmt 0 cntflag 0x1 vis(0 0 0 0) sf(0 0) opacity 1.00
GPU getoqaqueRect next 0x47f43e18 layer 0x47f43c00 (0 0 320 480)
GPU getoqaqueRect set empty visible to layer 0x47333400
GPU      aLayer 0x47f43c00 in 200 name RefLayerComposite fmt 0 cntflag 0x0 vis(0 0 320 480) sf(0 0) opacity 1.00

[Detail log]
GPU composition start
GPU  aLayer 0x48069000 in   1 name ContainerLayerComposite fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
GPU  aLayer 0x48069c00 in   2 name ThebesLayerComposite fmt 0 cntflag 0x0 vis(0 0 0 0) sf(0 0) opacity 1.00
GPU getoqaqueRect next 0x4806ada0 layer 0x4806ac00 (0 0 0 0)
GPU  aLayer 0x4806ac00 in   3 name ColorLayerComposite fmt 0 cntflag 0x1 vis(0 0 0 0) sf(0 0) opacity 1.00
GPU getoqaqueRect next 0x4806a208 layer 0x4806a000 (0 0 320 480)
GPU getoqaqueRect set empty visible to layer 0x4806ac00
GPU    aLayer 0x4806a000 in 100 name ContainerLayerComposite fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
GPU    aLayer 0x4806a800 in 101 name ThebesLayerComposite fmt 4 cntflag 0x1 vis(0 0 0 0) sf(320 480) opacity 1.00
GPU getoqaqueRect next 0x473335a0 layer 0x47333400 (0 0 0 0)
GPU    aLayer 0x47333400 in 102 name ColorLayerComposite fmt 0 cntflag 0x1 vis(0 0 0 0) sf(0 0) opacity 1.00
GPU GetopaqueRect aLayer 0x47f43c00 refLayer 0x47f43c00 first child 0x471c3000
GPU getoqaqueRect next 0x47f43e18 layer 0x47f43c00 (0 0 320 480)
GPU getoqaqueRect set empty visible to layer 0x47333400
GPU      aLayer 0x47f43c00 in 200 name RefLayerComposite fmt 0 cntflag 0x0 vis(0 0 320 480) sf(0 0) opacity 1.00
GPU        aLayer 0x471c3000 in 300 name ContainerLayerComposite fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
GPU        aLayer 0x483f2800 in 301 name ThebesLayerComposite fmt 4 cntflag 0x1 vis(0 0 0 0) sf(320 480) opacity 1.00
GPU getoqaqueRect next 0x48bf71a0 layer 0x48bf7000 (0 0 320 480)
GPU getoqaqueRect set empty visible to layer 0x483f2800
GPU        aLayer 0x48bf7000 in 302 name ColorLayerComposite fmt 0 cntflag 0x1 vis(0 0 320 480) sf(0 0) opacity 1.00
GPU getoqaqueRect next 0x4735d588 layer 0x4735d400 (0 0 0 0)
GPU        aLayer 0x4735d400 in 303 name ImageLayerComposite fmt 266 cntflag 0x1 vis(0 0 320 240) sf(320 240) opacity 1.00
Comment on attachment 799402 [details] [diff] [review]
patch?

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +180,4 @@
>      if (layerToRender->GetLayer()->GetEffectiveVisibleRegion().IsEmpty() &&
>          !layerToRender->GetLayer()->AsContainerLayer()) {
>        continue;
>      }

Base on my testing, we will run GetOpaqueRect func even the layer with empty visibleRegion.
I suggest move your change after visibleRegion checking.

And you can check visibleRegion again after layerToRender->SetShadowVisibleRegion(visibleRegion) (line 176)
(In reply to peter chang[:pchang] from comment #33)
> Base on my testing, we will run GetOpaqueRect func even the layer with empty
> visibleRegion.
> I suggest move your change after visibleRegion checking.

OK. If it's working for you, can you re-upload your fixed version of the patch?
Attached patch patch v2 (obsolete) — Splinter Review
a. Fix the crash
b. Move visible region check before redundant composition detection
Attachment #800725 - Attachment is obsolete: true
Attachment #800725 - Flags: review?(roc)
Attachment #801342 - Flags: review?(roc)
Backed out for marionette-webapi failures (note these were even visible in the Try run...), eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27626336&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0a4b4b9b1f
(In reply to Ed Morley [:edmorley UTC+1] from comment #38)
> Backed out for marionette-webapi failures (note these were even visible in
> the Try run...), eg:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27626336&tree=Mozilla-
> Inbound
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0a4b4b9b1f

This landed and was then backed out again, I picked the wrong one for the target of qbackout - which doesn't matter in terms of this backout diff; but just means the commit message references the older rev, when it should have been a2013b29212c. Adding [leave open] in case mcMerge closes the bug as a result.
Whiteboard: [leave open]
A crash in those tests maybe? Peter, can you try running them locally?
Whiteboard: [leave open]
Ran marionette testing with my local emulator and all passed.

SUMMARY
-------
passed: 181
failed: 0
todo: 0

Will send to try server again.
Attached patch patch v3Splinter Review
Add null ptr checking to pass marionette-webapi test
Attachment #801342 - Attachment is obsolete: true
Attachment #805124 - Flags: review+
Comment on attachment 805124 [details] [diff] [review]
patch v3

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +67,5 @@
> +    // Drill down into RefLayers because that's what we particularly care about;
> +    // layer construction for aLayer will not have known about the opaqueness
> +    // of any RefLayer subtrees.
> +    RefLayer* refLayer = aLayer->AsRefLayer();
> +    if (refLayer && refLayer->GetFirstChild()) {

Here is the new added ptr checking to pass marionette on try server
https://hg.mozilla.org/mozilla-central/rev/e56638da129b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.