635383 - Rewrite flipping and clipping for OGL Layers.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

We've had way too many obscure bugs and regressions come out of this.

See https://bugzilla.mozilla.org/show_bug.cgi?id=635302#c4 for a write-up of the framebuffer->framebuffer blitting insanity.

We need to write standalone logic for handling the clipping of layers and share this between all layer managers.

Then we can rewrite the double buffering/flipping code in a way that the differences are abstracted within the LayerManager/GLContext and not dragged all the way through ContainerLayer in obscure ways.

A decent set of tests that actually force active layers and intermediate framebuffers wouldn't go astray either.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Let GLContext know about the flip, and let it handle things internally

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Share the scissor rect calculations between all 3 accelerated backends

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Don't special case reftests

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 516142 [details] [diff] [review]
Part 1: Let GLContext know about the flip, and let it handle things internally

Overall, do like. Lots of great stuff here, especially all the deleted code.

>diff --git a/gfx/layers/opengl/ContainerLayerOGL.cpp b/gfx/layers/opengl/ContainerLayerOGL.cpp

>@@ -293,26 +269,26 @@ ContainerRender(Container* aContainer,

>-  aContainer->gl()->PopScissorRect();

I'm not in love with duplicating the scissor rect popping in the if/else here. Can we avoid it, and just do it at the end of the function?

>   if (needsFramebuffer) {
>     // Unbind the current framebuffer and rebind the previous one.
>     
>     // Restore the viewport
>     aContainer->gl()->PopViewportRect();
>     nsIntRect viewport = aContainer->gl()->ViewportRect();
>     aManager->SetupPipeline(viewport.width, viewport.height,
>                             LayerManagerOGL::ApplyWorldTransform);
>+    aContainer->gl()->PopScissorRect();


>@@ -327,20 +303,25 @@ ContainerRender(Container* aContainer,

>-    aManager->BindAndDrawQuad(rgb, aManager->IsDrawingFlipped());
>+    // Drawing is always flipped, but when copying between surfaces we want to avoid

Are we always flipped? (See below too.)

>+    // this. Pass true for the flip parameter to introduce a second flip
>+    // that cancels the other one out.
>+    aManager->BindAndDrawQuad(rgb, true);
> 
>     // Clean up resources.  This also unbinds the texture.
>     aContainer->gl()->fDeleteTextures(1, &containerSurface);
>+  } else {
>+    aContainer->gl()->PopScissorRect();

>diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp

>@@ -174,16 +174,17 @@ LayerManagerOGL::Initialize(nsRefPtr<GLC

>+  mGLContext->SetFlipped(PR_TRUE);

Are we always flipped? Should this be based on the double-buffered status?

> LayerManagerOGL::SetupPipeline(int aWidth, int aHeight, WorldTransforPolicy aTransformPolicy)

>-  // When we are double buffering, we change the view matrix around so
>+  // We flip the view matrix around so
>   // that everything is right-side up; we're drawing directly into
>   // the window's back buffer, so this keeps things looking correct.

nit: reformat comment block (gq in vim)


>+  // skip this update if it hadn't since the last call. We will
>+  // need to track changes to aTransformPolicy and mWorldMatrix
>+  // though.

I don't think you actually did this skipping? Also, you accidentally a verb in the first sentence I think.

>+  gfxMatrix viewMatrix;
>+  viewMatrix.Translate(-gfxPoint(1.0, -1.0));
>+  viewMatrix.Scale(2.0f / float(aWidth), 2.0f / float(aHeight));
>+  viewMatrix.Scale(1.0f, -1.0f);

Can you add a comment explaining the derivation of this?

>@@ -858,17 +841,19 @@ LayerManagerOGL::CopyToTarget()
>       PRUint32 *row = (PRUint32*) (imageSurface->Data() + imageSurface->Stride() * j);
>       for (int i = 0; i < width; ++i) {
>         *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
>         row++;
>       }
>     }
>   }
> 
>-  mTarget->SetOperator(gfxContext::OPERATOR_OVER);
>+  mTarget->SetOperator(gfxContext::OPERATOR_SOURCE);

Was this the problem behind our regression test failures all along?

>diff --git a/gfx/thebes/GLContext.h b/gfx/thebes/GLContext.h

>     // only does the glScissor call, no ScissorRect business
>     void raw_fScissor(GLint x, GLint y, GLsizei width, GLsizei height) {
>         BEFORE_GL_CALL;
>-        mSymbols.fScissor(x, y, width, height);
>+        // GL's coordinate system is flipped compared to ours (in the Y axis),
>+        // so we need to flip our rectangle.
>+        mSymbols.fScissor(x, 
>+                          mFlipped ? ViewportRect().height - (height + y) : y,
>+                          width, 
>+                          height);

nit: you should probably make the comment say "we may need to flip our rectangle" or something along those lines.

Also, a macro or function that does the mFlipped ? blah : bloo dance would not go astray.

>     // only does the glViewport call, no ViewportRect business
>     void raw_fViewport(GLint x, GLint y, GLsizei width, GLsizei height) {
>         BEFORE_GL_CALL;
>+        NS_ASSERTION(x == 0 && y == 0, "TODO: Need to flip the viewport rect");
>         mSymbols.fViewport(x, y, width, height);
>         AFTER_GL_CALL;

I don't understand why you added this assertion.

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 516143 [details] [diff] [review]
Part 2: Share the scissor rect calculations between all 3 accelerated backends

Love it.

>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h

>+  /**
>+   * Calculate the scissor rect required when rendering this layer.
>+   *
>+   * @param aIntermediate true if the layer is being rendered to an
>+   * intermediate surface, false otherwise.
>+   * @param aVisibleRect The bounds of the parents visible region.
>+   * @param aParentScissor The existing scissor rect set for the parent.
>+   * @param aTransform The current 2d transform of the parent.
>+   */
>+  nsIntRect GetScissorRect(bool aIntermediate,
>+                           const nsIntRect& aVisibleRect,
>+                           const nsIntRect& aParentScissor,
>+                           const gfxMatrix& aTransform);

Maybe we should call it "CalculateScissorRect"? That's sort of bikesheddy though.

Nit: The bounds of the _parent's_ visible region (missing apostrophe)

>diff --git a/gfx/layers/d3d10/ContainerLayerD3D10.cpp b/gfx/layers/d3d10/ContainerLayerD3D10.cpp
>--- a/gfx/layers/d3d10/ContainerLayerD3D10.cpp
>+++ b/gfx/layers/d3d10/ContainerLayerD3D10.cpp
>@@ -252,96 +252,57 @@ ContainerLayerD3D10::RenderLayer()
>       SetRawValue(renderTargetOffset, 0, 8);
> 
>     previousViewportSize = mD3DManager->GetViewport();
>     mD3DManager->SetViewport(nsIntSize(visibleRect.Size()));
>   } else {
>     PRBool is2d = GetEffectiveTransform().Is2D(&contTransform);
>     NS_ASSERTION(is2d, "Transform must be 2D");
>   }
>+    
>+  D3D10_RECT oldD3D10Scissor;
>+  UINT numRects = 1;
>+  device()->RSGetScissorRects(&numRects, &oldD3D10Scissor);
>+  nsIntRect oldScissor(oldD3D10Scissor.left,
>+                       oldD3D10Scissor.top,
>+                       oldD3D10Scissor.right - oldD3D10Scissor.left,
>+                       oldD3D10Scissor.bottom - oldD3D10Scissor.top);

Holy crap. I did not realize that Windows RECTs are exclusive on the bottom and right. That is insane. INSANE.

(Ref: http://msdn.microsoft.com/en-us/library/dd162897.aspx)

I guess add a comment?

>diff --git a/gfx/layers/d3d9/ContainerLayerD3D9.cpp b/gfx/layers/d3d9/ContainerLayerD3D9.cpp

>+  RECT containerD3D9ClipRect;
>+  device()->GetScissorRect(&containerD3D9ClipRect);
>+  nsIntRect oldScissor(containerD3D9ClipRect.left,
>+                       containerD3D9ClipRect.top,
>+                       containerD3D9ClipRect.right - containerD3D9ClipRect.left,
>+                       containerD3D9ClipRect.bottom - containerD3D9ClipRect.top);

And another comment here too, same reason.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 516144 [details] [diff] [review]
Part 3: Don't special case reftests

let's do this thing!

Comment 7

[Matt Woodrow (:mattwoodrow)]

> I'm not in love with duplicating the scissor rect popping in the if/else here.
> Can we avoid it, and just do it at the end of the function?

Sadly not. The cached scissor rect is stored in our coordinate system and needs the correct viewport to be set for it to be applied to GL correctly. In the case of the if branch, we have a different viewport applied (for the temporary surface), so we have to reset this correctly first before restoring the old scissor.

> 
> Are we always flipped? (See below too.)

Yes. Trying to maintain two different paths was causing all sorts of weird bugs. I also don't believe we have any tinderbox machines with single buffering, so this case wasn't even being tested. I've changed the code to always flip for simplicity.

> nit: reformat comment block (gq in vim)
> 

Will do!


> 
> I don't think you actually did this skipping? Also, you accidentally a verb in
> the first sentence I think.

I think it was meant to refer to skipping from the first half of the sentence. The comment was also meant to imply that it was a TODO, not part of the current code. I'll rewrite this into slightly less terrible english.

> 
> >+  gfxMatrix viewMatrix;
> >+  viewMatrix.Translate(-gfxPoint(1.0, -1.0));
> >+  viewMatrix.Scale(2.0f / float(aWidth), 2.0f / float(aHeight));
> >+  viewMatrix.Scale(1.0f, -1.0f);
> 
> Can you add a comment explaining the derivation of this?

Sure can.

> > 
> >-  mTarget->SetOperator(gfxContext::OPERATOR_OVER);
> >+  mTarget->SetOperator(gfxContext::OPERATOR_SOURCE);
> 
> Was this the problem behind our regression test failures all along?

I don't believe so, this just seemed like the 'right' thing to do. The test failures were from not flipping the result back into our orientation I believe.

> 
> >diff --git a/gfx/thebes/GLContext.h b/gfx/thebes/GLContext.h
> 
> >     // only does the glScissor call, no ScissorRect business
> >     void raw_fScissor(GLint x, GLint y, GLsizei width, GLsizei height) {
> >         BEFORE_GL_CALL;
> >-        mSymbols.fScissor(x, y, width, height);
> >+        // GL's coordinate system is flipped compared to ours (in the Y axis),
> >+        // so we need to flip our rectangle.
> >+        mSymbols.fScissor(x, 
> >+                          mFlipped ? ViewportRect().height - (height + y) : y,
> >+                          width, 
> >+                          height);
> 
> nit: you should probably make the comment say "we may need to flip our
> rectangle" or something along those lines.
> 
> Also, a macro or function that does the mFlipped ? blah : bloo dance would not
> go astray.

Agreed.

> 
> >     // only does the glViewport call, no ViewportRect business
> >     void raw_fViewport(GLint x, GLint y, GLsizei width, GLsizei height) {
> >         BEFORE_GL_CALL;
> >+        NS_ASSERTION(x == 0 && y == 0, "TODO: Need to flip the viewport rect");
> >         mSymbols.fViewport(x, y, width, height);
> >         AFTER_GL_CALL;
> 
> I don't understand why you added this assertion.

glScissor, glViewport and others take a rectangle that is in window coordinates, and need to have this flipped to match our coordinate system. For our current code, we assume the viewport rect always matches the current window size, so that no flipping is required for glViewport and we use the viewport height to flip the others. If we ever set a viewport smaller than the window we will have problems. This assertion should probably only hit if mFlipped is true though, otherwise we could hit it with WebGL.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Let GLContext know about the flip, and let it handle things internally v2

Fixed review comments. Carrying forward r=joe

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Share the scissor rect calculations between all 3 accelerated backends v2

Fixed review comments. Carrying forward r=joe

Comment 10

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/mozilla-central/rev/7cc0aaab5b51
http://hg.mozilla.org/mozilla-central/rev/e99895e04814
http://hg.mozilla.org/mozilla-central/rev/62a554e8286a

Comment 11

[Joe Drew (not getting mail)]

Attachment: fix mixup causing black screens on D3D10

This is a followup patch from Matt that Boris and I reviewed on irc. It fixes black rendering on non-square browser windows. :)

Comment 12

[Joe Drew (not getting mail)]

Checked in the followup patch: http://hg.mozilla.org/mozilla-central/rev/9a1aa415c9a1

Comment 13

[Gary [:streetwolf52]]

I saw that and wondered about it.