956154 - Remove GLContext's viewport stack

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Remove GLContext's viewport stack

GLContext has the ability to push and pop (save and restore) the viewport rect. There was no need to implement such a stack, as there is already a stack in the c++ language and if we need to, we can achieve the same with it (RAII helper to save and restore the viewport rect).

Comment 1

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

Er no, these are not equivalent.  The previous push/pop always kept track of the current state of the viewport, and compared to that.  Your ScopedViewport RAII class calls GetIntegerv first, which is bad -- no getters in normal code paths please!  You can still keep the RAII class (and it's definitely nicer to have it), but please have GLContext or something keep track of the viewport.  We should be tracking *more* GL state this way, not less.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Good point about the performance impact of relying on GL getters and the need to do our own tracking; but we can make both of us happy by tracking only the current viewport rect, so that glGetIntegerv(VIEWPORT_RECT) is fast regardless of the underlying GL, without implementing any stack on our side. I'll write that patch, if you don't disagree.

Comment 3

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

Yup, that sounds great.  Having to pull the numbers out via the getter is a little unfortunate, but it will all likely get inlined and optimized away anyway.

Comment 4

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

(The call to glViewport should also do the comparison and not actually call the GL function if it doesn't need to; that's the potentially costly call, even with the same values.)

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Optimization: cache the viewport rect

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8355342 [details] [diff] [review]
Remove GLContext's viewport stack

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

::: gfx/gl/ScopedGLHelpers.h
@@ +208,5 @@
>  protected:
>      void UnwrapImpl();
>  };
>  
> +struct ScopedViewportRect

Great!

Comment 7

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

Comment on attachment 8355562 [details] [diff] [review]
Optimization: cache the viewport rect

r- -- you're missing a "break;" in the case statement.  Technically you can fix that and land with r+, but I'm wondering if that will change behaviour noticably?

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Ouch! The reason why this was not caught by any testing is that it was falling through to the generic path calling glGetIntegerv, so it was correct but slow.

Comment 9

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

r+ with the fix.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Optimization: cache the viewport rect

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dad6e5e57b1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/886b3ccaaa3c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dad6e5e57b1e
https://hg.mozilla.org/mozilla-central/rev/886b3ccaaa3c