Closed Bug 956401 Opened 6 years ago Closed 6 years ago

Remove GLContext's scissor stack, untangling the spaghetti of callers of Push/PopScissorRect

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Whiteboard: [qa-])

Attachments

(10 files, 1 obsolete file)

1.04 KB, patch
nrc
: review+
Details | Diff | Splinter Review
2.09 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.71 KB, patch
nrc
: review+
Details | Diff | Splinter Review
1.54 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
3.83 KB, patch
nrc
: review+
Details | Diff | Splinter Review
2.86 KB, patch
nrc
: review+
Details | Diff | Splinter Review
2.06 KB, patch
nrc
: review+
Details | Diff | Splinter Review
4.01 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
18.36 KB, patch
Details | Diff | Splinter Review
3.11 KB, patch
vlad
: review+
Details | Diff | Splinter Review
This is similar to bug 956154, but for the scissor stack instead of the viewport stack. It should go away for the same reason: better stick to having only one stack, the C++ language's, and using RAII helpers to make it do what we need with GL scissors.

This is however going to be trickier than bug 956154, because usage of the scissor stack has grown in ways that aren't trivial to map back to the call stack. In other words, we have in CompositorOGL.cpp functions that do a PushScissorRect but leave it to other functions to do the matching PopScissorRect. To untangle this, we need to draw the graph of callers to PushScissorRect and PopScissorRect, and callers to these callers, until we find the "common denominators" responsible for both Push and Pop calls.

Here is this graph, where A->B means A calls B:




                _____________  ContainerRender  ______
               /                                      \
              /                                        \
             /                                          \
            V                                            V

   CompositorOGL::            CompositorOGL::            CompositorOGL::
   SetRenderTarget(1)         DrawQuadInternal           SetRenderTarget(2)

            |              /                   \             |
            |             /                     \            |
            |            /                       \           |
            V           V                         V          V
 
           GLContext::                             GLContext::
           PushScissorRect                         PopScissorRect

          ^        ^                                ^        ^
         /          \                              /          \
        /            \                            /            \
       /              \                          /              \

CompositorOGL::     CompositorOGL::       CompositorOGL::     CompositorOGL::
BeginFrame          SaveState             RestoreState        EndFrame

            ^             ^                  ^               ^
             \______       \                /      _________/
                    \       \              /      /

                      LayerManagerComposite::Render



On this graph we see that ultimately we have 3 functions making (possibly indirectly) calls to Push/PopScissorRect. By increasing order of difficulty:
  1. CompositorOGL::DrawQuadInternal;
  2. ContainerRender, via CompositorOGL::SetRenderTarget;
  3. LayerManagerComposite::Render, via BeginFrame/SaveState/RestoreState/EndFrame.

Let's handle these 3 places separately.

***************************************************

1. CompositorOGL::DrawQuadInternal is trivial: it does a PushScissorRect, followed by a matching PopScissorRect. Trivially replaceable by a RAII helper.

***************************************************

2. ContainerRender seems to be simply going through CompositorOGL::SetRenderTarget, but it's not so simple: let's take a look at CompositorOGL::SetRenderTarget:

void
CompositorOGL::SetRenderTarget(CompositingRenderTarget *aSurface)
{
  MOZ_ASSERT(aSurface);
  CompositingRenderTargetOGL* surface
    = static_cast<CompositingRenderTargetOGL*>(aSurface);
  if (mCurrentRenderTarget != surface) {
    // Restore the scissor rect that was active before we set the current
    // render target.
    mGLContext->PopScissorRect();

    // Save the current scissor rect so that we can pop back to it when
    // changing the render target again.
    mGLContext->PushScissorRect();

    surface->BindRenderTarget();
    mCurrentRenderTarget = surface;
  }
}

Thus, SetRenderTarget does a Pop followed by a new Push. These aren't simply a matching Push/Pop pair.

Fortunately, it is only called by one function: ContainerRender. It seems that we can use a RAII helper there.

***************************************************

3. LayerManagerComposite::Render goes through two distinct pairs of helper functions that each call Push/PopScissorRect: BeginFrame/EndFrame, and SaveState/RestoreState.

Fortunately, it seems to be the only caller of these helper functions, so we should be able to move the scissor work entirely to LayerManagerComposite::Render, and use RAII helpers there.
Assignee: nobody → bjacob
The other question is: since DrawQuadInternal does set the scissor rect, is it useful that ContainerRender sets it? It seems that whichever scissor rect ContainerRender sets, will be overridden by DrawQuadInternal.

The scissor set by LayerManagerComposite::Render seems more justified: it affects the drawing of window overlays/underlays which can be drawn by GL code outside of our compositor.
Summary: Remove GLContext's scissor stack, untangling the spaghetti of callers or Push/PopScissorRect → Remove GLContext's scissor stack, untangling the spaghetti of callers of Push/PopScissorRect
As explained above, the suspiscion here, which turned out to be true, was that most scissor changes made in CompositorOGL were without effect, because most drawing goes through DrawQuadInternal which pushes its own scissor rect anyway, overriding what other parts of CompositorOGL do.

This is fortunate, because, as explained above, these scissor changes made by SetRenderTarget were difficult to reason about: rather than pushing and then popping a scissor rect, it was doing the opposite: first popping whichever scissor rect had been pushed before, then pushing a new one.
Attachment #8355908 - Flags: review?(ncameron)
This will be used by the next patch (3/9). At the end of this patch queue, the only scissor calls made by CompositorOGL will be one usage of ScopedScissorRect in DrawQuadInternal.
Attachment #8355910 - Flags: review?(jgilbert)
This one change is trivial: replacing a (PushScissorRect, PopScissorRect) pair by a RAII ScopedScissorRect helper. In other words, using the C++ language's stack to do the same thing as was implemented by GLContext's scissor stack.
Attachment #8355912 - Flags: review?(ncameron)
Comment on attachment 8355912 [details] [diff] [review]
3/9 - Let DrawQuadInternal use a ScopedScissorRect

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1098,5 @@
>    }
>    IntRect intClipRect;
>    clipRect.ToIntRect(&intClipRect);
> +  ScopedScissorRect autoScissor(
> +                      mGLContext,

nit: put on prev line and adjust below indents accordingly
Attachment #8355912 - Flags: review?(ncameron) → review+
This is going to be used in the next patch (5/9) where we have code that saves GL state without changing it, then calls opaque code that might change GL state, then restores state.
Attachment #8355913 - Flags: review?(jgilbert)
Attachment #8355908 - Flags: review?(ncameron) → review+
Since patch 5/9, it's just not used anymore. The rationale for this change is that it seems better to have any specific DrawWindow{Under,Over}lay code take care of restoring its own GL state, rather than trying to abstract it in some way that tries to be platform-generic, at this point, as this only ever had one use case (better wait until we have two different use cases, before making a cross-platform abstraction).
Attachment #8355916 - Flags: review?(ncameron)
Same story as part 1/9: these scissor calls just weren't making any difference, because DrawQuadInternal overrode them.
Attachment #8355917 - Flags: review?(ncameron)
Thus the hard workers reap their just reward.
Attachment #8355918 - Flags: review?(jgilbert)
This one is specially for Vlad, who on the Viewport bug (bug 956154) expressed concern that this might make a noticeable performance difference. This should be similar.
Attachment #8355919 - Flags: review?(vladimir)
Attached patch Combined diffSplinter Review
In case it helps with reviewing.

Also, here's the diff --stat:

 gfx/gl/GLContext.cpp                           |   5 +-
 gfx/gl/GLContext.h                             |  57 +++++++---------------
 gfx/gl/ScopedGLHelpers.cpp                     |  32 ++++++++++++
 gfx/gl/ScopedGLHelpers.h                       |  19 +++++++
 gfx/layers/Compositor.h                        |   6 --
 gfx/layers/composite/LayerManagerComposite.cpp |   4 -
 gfx/layers/opengl/CompositorOGL.cpp            |  50 ++-----------------
 gfx/layers/opengl/CompositorOGL.h              |   3 -
 widget/android/nsWindow.cpp                    |  10 ++++
 9 files changed, 87 insertions(+), 99 deletions(-)

Here's a green try run:
https://tbpl.mozilla.org/?tree=Try&rev=3c9c4a27c843
Attachment #8355917 - Attachment description: 7/9 - CompositorOGL::{Begin,End}Frame do not need to touch the scissor rectbeginframe-endframe → 7/9 - CompositorOGL::{Begin,End}Frame do not need to touch the scissor rect
Comment on attachment 8355915 [details] [diff] [review]
5/9 - Let DrawWindow{Under,Over}lay take care of saving and restoring GL state

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

Do you need to also take care of the GL state in widget/cocoa/nsChildView::DrawWindowOverlay?
Attachment #8355915 - Flags: review?(ncameron) → review+
Comment on attachment 8355916 [details] [diff] [review]
6/9 - Remove Compositor::SaveState and Compositor::RestoreState

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

things tend to be hard to test and sometimes get missed by Try (because ). It is probably worth trying to test this manually.
Attachment #8355916 - Flags: review?(ncameron) → review+
Attachment #8355917 - Flags: review?(ncameron) → review+
Attachment #8355910 - Flags: review?(jgilbert) → review+
Attachment #8355913 - Flags: review?(jgilbert) → review+
Attachment #8355918 - Flags: review?(jgilbert) → review+
(In reply to Nick Cameron [:nrc] from comment #13)
> Do you need to also take care of the GL state in
> widget/cocoa/nsChildView::DrawWindowOverlay?

It seems so... at least this omissions was unintentional. Lesson learned: don't rely on DXR to be exhaustive in the face of OS-specific code paths.

(In reply to Nick Cameron [:nrc] from comment #14)
> things tend to be hard to test and sometimes get missed by Try (because ).
> It is probably worth trying to test this manually.

You're right. Here's a new try push,
https://tbpl.mozilla.org/?tree=Try&rev=ca29b2c49cc1

I'll make sure to give this some manual testing on each OS.
Green again; completed manual testing on Mac, Android and B2G.
Attachment #8355919 - Attachment is obsolete: true
Attachment #8355919 - Flags: review?(vladimir)
Attachment #8356651 - Flags: review?(vladimir)
Depends on: 958025
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.