599507 - [OpenGL] Pass down opacity/transform from single-child ContainerLayerOGL

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

glDeleteTextures shows up significantly in this testcase: https://bug584494.bugzilla.mozilla.org/attachment.cgi?id=463418

By hanging onto the texture and attempting to reuse it wherever possible, we get a decent performance gain.

Profiling also shows a lot of time spent in glCheckFrameBufferStatus (it calls glFinish), but this should only happen in debug mode.

The current patch gets an error occasionally because of deleting a texture twice. Doesn't appear to affect rendering at all, will look into it.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Attachment attempt 2

Comment 2

[Bas Schouten (:bas.schouten)]

An easier improvement for that specific testcase would be something like:

https://bugzilla.mozilla.org/attachment.cgi?id=477167

Comment 3

[Bas Schouten (:bas.schouten)]

Also keep in mind this can create significant VRAM requirements, for example in the case of the transform stresstest, there's 25 containers layers. If each of them were 1000x1000, that's 1000000x4x25 100MB of VRAM just for those containers.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Pass transforms/opacity down where we have a single child

This takes fps for the above testcase from 18->55 for me.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Pass transforms/opacity down where we have a single child v2

Fixed clipping issues.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We should hook an expiration timer into these long-lived textures.

Also, I like the collapsing of single-child containers, but two questions...
1 - why isn't that happening in layout?
2 - assuming #1 can't happen, then I fear the explosion of args (especially the need to pass 1.0/identity matrices); I wonder if a state struct wouldn't be a better thing to hand down that has all this stuff + any potential future bits.

Comment 7

[Matt Woodrow (:mattwoodrow)]

I'm now less convinced caching the container layer textures is a good idea. It's only an issue on benchmarks where we really stress things, and the collapsing patch completely removes it from the only benchmark it showed it up in. I doubt its worth landing due to the extra memory pressure for the time being.

Both BasicLayers and D3D9 layers already implement this sort of thing in the same way. Maybe a followup bug to combine all three in layout at some point would be a good idea?

Comment 8

[Joe Drew (not getting mail)]

So, to be clear, we only want to look into passing transforms and opacity down, right?

Comment 9

[Matt Woodrow (:mattwoodrow)]

Correct

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 478495 [details] [diff] [review]
Pass transforms/opacity down where we have a single child v2

Can you separate the gfx3DMatrix constness patch out into its own separate patch?

Looks great.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Make gfxMatrix::operator* const

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Pass transforms/opacity down where we have a single child v3

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Pass transforms/opacity down where we have a single child v4

Fixed a few bugs I found while testing bug 586683.

Allow transform only containers with multiple (unclipped) children to still skip the framebuffer.

Skip clipping to the visible rect of child layers when not using a frame buffer - Is this correct? The only other option is to transform the clip which could end up as something other than an aligned rectangle. Clipping using glScissor won't work in this case and we'd have to fallback to a framebuffer.

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 482803 [details] [diff] [review]
Pass transforms/opacity down where we have a single child v4


>@@ -199,26 +237,44 @@ ContainerLayerOGL::RenderLayer(int aPrev
> 
>     const nsIntRect *clipRect = layerToRender->GetLayer()->GetClipRect();
>     if (clipRect) {
>       scissorRect = *clipRect;
>     }
> 
>     if (needsFramebuffer) {
>       scissorRect.MoveBy(- visibleRect.TopLeft());
>+    } 
>+
>+    if (!needsFramebuffer && aPreviousFrameBuffer) {
>+      scissorRect.IntersectRect(scissorRect, cachedScissor);
>+    }
>+    if (!needsFramebuffer && aPreviousFrameBuffer == 0) {
>+      gl()->FixWindowCoordinateRect(scissorRect, mOGLManager->GetWigetSize().height);
>+    }
>+
>+    if (!needsFramebuffer && !aPreviousFrameBuffer) {
>+      scissorRect.IntersectRect(scissorRect, cachedScissor);
>+    }

Can you collapse these three if clauses to 

if (!needsFramebuffer && aPreviousFrameBuffer) {
  scissorRect.IntersectRect(scissorRect, cachedScissor);
} else if (!needsFramebuffer)
  gl()->FixWindowCoordinateRect(scissorRect, mOGLManager->GetWigetSize().height);
  scissorRect.IntersectRect(scissorRect, cachedScissor);
}

Or even (if you like it more)

if (!needsFrameBuffer && aPreviousFrameBuffer == 0)
  gl()->FixWindowCoordinateRect(scissorRect, mOGLManager->GetWigetSize().height);

if (!needsFramebuffer)
  scissorRect.IntersectRect(scissorRect, cachedScissor);

>+    
>+    /**
>+     *  We can't clip to a visible region if theres no framebuffer since we might be transformed
>+     */
>+    if (needsFramebuffer || clipRect) {
>+      gl()->fScissor(scissorRect.x, scissorRect.y, scissorRect.width, scissorRect.height);

Is this right? We're going to scissor to the new scissor rect even when we don't have a framebuffer if we have a clip rect, if my reading of this code is correct.

To answer your question, it's probably fine to not scissor in general, considering it's only for drawing safety, right?

Comment 15

[Matt Woodrow (:mattwoodrow)]

> Can you collapse these three if clauses to 
> 
> if (!needsFramebuffer && aPreviousFrameBuffer) {
>   scissorRect.IntersectRect(scissorRect, cachedScissor);
> } else if (!needsFramebuffer)
>   gl()->FixWindowCoordinateRect(scissorRect,
> mOGLManager->GetWigetSize().height);
>   scissorRect.IntersectRect(scissorRect, cachedScissor);
> }
> 

Sure.


> 
> >+    
> >+    /**
> >+     *  We can't clip to a visible region if theres no framebuffer since we might be transformed
> >+     */
> >+    if (needsFramebuffer || clipRect) {
> >+      gl()->fScissor(scissorRect.x, scissorRect.y, scissorRect.width, scissorRect.height);
> 
> Is this right? We're going to scissor to the new scissor rect even when we
> don't have a framebuffer if we have a clip rect, if my reading of this code is
> correct.
> 
> To answer your question, it's probably fine to not scissor in general,
> considering it's only for drawing safety, right?

Hrm not entirely. When needsFramebuffer is false, clipRect will always be false too (or 0,0,0,0). This check probably should just be if (needsFramebuffer). I might add an explicit clipRect.IsEmpty() check as well.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Pass transforms/opacity down where we have a single child v5

Massive rebase against trunk (ShadowLayer changes).

Comment 17

[Matt Woodrow (:mattwoodrow)]

> >+    /**
> >+     *  We can't clip to a visible region if theres no framebuffer since we might be transformed
> >+     */
> >+    if (needsFramebuffer || clipRect) {
> >+      gl()->fScissor(scissorRect.x, scissorRect.y, scissorRect.width, scissorRect.height);
> 
> Is this right? We're going to scissor to the new scissor rect even when we
> don't have a framebuffer if we have a clip rect, if my reading of this code is
> correct.
> 

This should be correct actually. If we have a clip rect, then there's either no transform (ShouldUseIntermediate would have returned true) or we are already using an intermediate.

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 483689 [details] [diff] [review]
Pass transforms/opacity down where we have a single child v5

Can you add a comment and/or assertion saying that we're guaranteed there's no transform because of ShouldUseIntermediate?

Also, can you re-split-out the gfx3DMatrix operator* const again? :)

Comment 19

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

Flattening the structure of the layer tree is really difficult to do in layout, because it becomes very difficult to recycle the old layer tree as we build the new one (layers that we "temporarily" need wouldn't be present in the old layer tree). We flattened the layer tree in layout before retained layers, but for retained layers I had to take out the flattening to make things sane.

After constructing the layer tree, layout could do a pass to move various properties of a single-child container down into the child. But we could just as easily do that in layers code that's shared between layer managers --- perhaps in the same pass as the pass to compute effective transforms that I'm adding in bug 602200. Doing it there makes more sense to me because it can happen in the compositor process, and if any of the normalization we want to do depends on "effective" properties known only to the shadow compositor, or depends on compositor-based animation, that's where it needs to be.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Attachment: Pass transforms/opacity down where we have a single child v6

Fixed review comments.

Changed the clipRect condition to transform.IsIdentity() since it better reflects the actual logic we're trying to enforce.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/5d82a6476d53
http://hg.mozilla.org/mozilla-central/rev/362567eee982