Closed Bug 956154 Opened 6 years ago Closed 6 years ago
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).
Attachment #8355342 - Flags: review?(jgilbert)
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.
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.
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.
(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 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!
Attachment #8355342 - Flags: review?(jgilbert) → review+
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?
Attachment #8355562 - Flags: review?(vladimir) → review-
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.
r+ with the fix.
Attachment #8356649 - Flags: review?(vladimir) → review+
Assignee: nobody → bjacob
Target Milestone: --- → mozilla29
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.