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

RESOLVED FIXED in mozilla29

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(10 attachments, 1 obsolete attachment)

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)

Updated

4 years ago
Assignee: nobody → bjacob
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 2

4 years ago
Created attachment 8355908 [details] [diff] [review]
1/9 - CompositorOGL::SetRenderTarget doesn't need to touch the scissor rect

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)
(Assignee)

Comment 3

4 years ago
Created attachment 8355910 [details] [diff] [review]
2/9 - Add a ScopedScissorRect RAII helper

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)
(Assignee)

Comment 4

4 years ago
Created attachment 8355912 [details] [diff] [review]
3/9 - Let DrawQuadInternal use a ScopedScissorRect

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 5

4 years ago
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+
(Assignee)

Comment 6

4 years ago
Created attachment 8355913 [details] [diff] [review]
4/9 - Add ScopedGLState ctor without value parameter, leaving state unchanged

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)

Updated

4 years ago
Attachment #8355908 - Flags: review?(ncameron) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 8355915 [details] [diff] [review]
5/9 - Let DrawWindow{Under,Over}lay take care of saving and restoring GL state
Attachment #8355915 - Flags: review?(ncameron)
(Assignee)

Comment 8

4 years ago
Created attachment 8355916 [details] [diff] [review]
6/9 - Remove Compositor::SaveState and Compositor::RestoreState

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)
(Assignee)

Comment 9

4 years ago
Created attachment 8355917 [details] [diff] [review]
7/9 - CompositorOGL::{Begin,End}Frame do not need to touch the scissor rect

Same story as part 1/9: these scissor calls just weren't making any difference, because DrawQuadInternal overrode them.
Attachment #8355917 - Flags: review?(ncameron)
Created attachment 8355918 [details] [diff] [review]
8/9 - Remove GLContext's scissor stack

Thus the hard workers reap their just reward.
Attachment #8355918 - Flags: review?(jgilbert)
Created attachment 8355919 [details] [diff] [review]
9/9 - Optimization: cache the scissor rect in GLContext

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)
Created attachment 8355920 [details] [diff] [review]
Combined diff

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
(Assignee)

Updated

4 years ago
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 13

4 years ago
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 14

4 years ago
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+

Updated

4 years ago
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.
Created attachment 8356651 [details] [diff] [review]
9/9 - Optimization: cache the scissor rect in GLContext
Attachment #8355919 - Attachment is obsolete: true
Attachment #8355919 - Flags: review?(vladimir)
Attachment #8356651 - Flags: review?(vladimir)
Depends on: 958025

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.