Closed Bug 599507 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

(Whiteboard: [fennec-checkin-postb2][has-patch])

Attachments

(2 files, 6 obsolete files)

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.
Attached patch Attachment attempt 2 (obsolete) — Splinter Review
Attachment #478416 - Flags: feedback?(joe)
An easier improvement for that specific testcase would be something like:

https://bugzilla.mozilla.org/attachment.cgi?id=477167
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.
This takes fps for the above testcase from 18->55 for me.
Attachment #478491 - Flags: review?(joe)
Fixed clipping issues.
Attachment #478491 - Attachment is obsolete: true
Attachment #478495 - Flags: review?(joe)
Attachment #478491 - Flags: review?(joe)
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.
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?
So, to be clear, we only want to look into passing transforms and opacity down, right?
Correct
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.
Attachment #478495 - Flags: review?(joe) → review+
Summary: [OpenGL] Cache intermediate textures used for FBOs in ContainerLayerOGL → [OpenGL] Pass down opacity/transform from single-child ContainerLayerOGL
Attachment #478416 - Flags: feedback?(joe)
Attachment #478416 - Attachment is obsolete: true
Attachment #478495 - Attachment is obsolete: true
Attachment #481049 - Flags: review+
Blocks: 602435
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b2+
Blocks: 586683
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.
Attachment #481050 - Attachment is obsolete: true
Attachment #482803 - Flags: review?(joe)
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?
Attachment #482803 - Flags: review?(joe) → review-
> 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.
tracking-fennec: 2.0b2+ → 2.0b3+
Massive rebase against trunk (ShadowLayer changes).
Attachment #482803 - Attachment is obsolete: true
Attachment #483689 - Flags: review?(joe)
> >+    /**
> >+     *  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 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? :)
Attachment #483689 - Flags: review?(joe) → review+
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.
Fixed review comments.

Changed the clipRect condition to transform.IsIdentity() since it better reflects the actual logic we're trying to enforce.
Attachment #483689 - Attachment is obsolete: true
Attachment #483879 - Flags: review+
Whiteboard: [needs landing] → [fennec-checkin-postb2][has-patch][needs landing]
Whiteboard: [fennec-checkin-postb2][has-patch][needs landing] → [fennec-checkin-postb2][has-patch]
You need to log in before you can comment on or make changes to this bug.