Last Comment Bug 596907 - Fix glScissor calls in ContainerLayerOGL
: Fix glScissor calls in ContainerLayerOGL
Status: RESOLVED FIXED
[approved-patches-landed]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
: Milan Sreckovic [:milan]
Mentors:
: 597127 (view as bug list)
Depends on:
Blocks: 586462
  Show dependency treegraph
 
Reported: 2010-09-16 00:57 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2011-11-17 20:45 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix glScissor calls in ContainerLayerOGL (3.24 KB, patch)
2010-09-16 00:57 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
Fix glScissor calls in ContainerLayerOGL (2.82 KB, patch)
2010-09-16 14:34 PDT, Benoit Jacob [:bjacob] (mostly away)
vladimir: review+
joe: approval2.0+
Details | Diff | Splinter Review
use XP_MACOSX (753 bytes, patch)
2010-09-18 13:40 PDT, Benoit Jacob [:bjacob] (mostly away)
shaver: review+
shaver: approval2.0+
Details | Diff | Splinter Review
Reverse Y axis when double buffering is disabled (drawing in a FBO) (5.15 KB, patch)
2010-09-19 17:14 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2010-09-16 00:57:27 PDT
Created attachment 475782 [details] [diff] [review]
Fix glScissor calls in ContainerLayerOGL

Account for the fact that glScissor counts y coordinates starting from the bottom.

This fixes the bug making a black rectangle being overlaid over the bottom of the firefox main window, the same size as the GUI stuff at the top.

Introduced GLIntRect to make that clean; could have used nsIntRect but didn't necessarily want to implicitly rely on its internal data layout never changing, and on PRInt32==GLint (althought the latter point is really unlikely to ever change).
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2010-09-16 01:01:07 PDT
Needs to be tested on Mac as this could easily break stuff there without being caught by reftests...
Comment 2 Matt Woodrow (:mattwoodrow) 2010-09-16 01:38:10 PDT
This patch actually creates the mentioned bug on mac. Is it possible that the y coordinates are platform specific somehow?
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2010-09-16 01:40:09 PDT
Heh, that's what it's sounding like!
On one end, glScissor is always expecting coords starting from the bottom.
On the other end, what is the y-axis for the clipRect and visibleRect?
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2010-09-16 09:46:06 PDT
On Mac, window coords match OpenGL coords -- Y increases up from the bottom.  Everywhere else, Y increases down.  So we need to add something to nsIWidget that will tell us what direction the coordinate space goes (or, actually, in the interim we can just do some #ifdef XP_MAC ery).

Also, please don't introduce GLIntRect; a one-off type just complicates matters here, because you have to go back and look at its trivial definition while reading the code.
Comment 5 Joe Drew (not getting mail) 2010-09-16 13:10:18 PDT
*** Bug 597127 has been marked as a duplicate of this bug. ***
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2010-09-16 14:34:24 PDT
Created attachment 476022 [details] [diff] [review]
Fix glScissor calls in ContainerLayerOGL

Here you go.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2010-09-16 17:04:08 PDT
http://hg.mozilla.org/mozilla-central/rev/3e57d3b0e8f9
Comment 8 Matt Woodrow (:mattwoodrow) 2010-09-17 20:57:40 PDT
This has regressed behaviour on both mac and ubuntu for me. We're also failing a lot more tests in the RGL suite on tinderbox because of it.

It appears XP_MAC isn't defined in ContainerLayerOGL.cpp even on mac.

What OS (and probably more importantly, which widget implementation) did you originally observe this on? It appears ubuntu has the same behaviour as mac (changing the #ifdef to #if 1 fixes things).
Comment 9 Joe Drew (not getting mail) 2010-09-18 09:33:38 PDT
The Mac #define is XP_MACOSX, not XP_MAC.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2010-09-18 13:32:17 PDT
(In reply to comment #8)
> This has regressed behaviour on both mac and ubuntu for me. We're also failing
> a lot more tests in the RGL suite on tinderbox because of it.

Oh. Can you explain me where you see them? I wasn't aware of that.

> 
> It appears XP_MAC isn't defined in ContainerLayerOGL.cpp even on mac.
> 
> What OS (and probably more importantly, which widget implementation) did you
> originally observe this on? It appears ubuntu has the same behaviour as mac
> (changing the #ifdef to #if 1 fixes things).

I observed this on Fedora 13 / x86-64. The widgets are the default i.e. GTK. This patch did fix it for me here.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2010-09-18 13:40:18 PDT
Created attachment 476571 [details] [diff] [review]
use XP_MACOSX

(In reply to comment #9)
> The Mac #define is XP_MACOSX, not XP_MAC.

Here's a patch fixing this.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2010-09-18 13:59:05 PDT
(In reply to comment #8)
> What OS (and probably more importantly, which widget implementation) did you
> originally observe this on? It appears ubuntu has the same behaviour as mac
> (changing the #ifdef to #if 1 fixes things).

What graphics driver are you using? If it's Mesa-based (i.e. if it's a free driver) are you using Mesa 7.8 ? I'm asking because I got this answer on #kwin:

[09:53] <fredrikh> bjacob: there's a bug in mesa 7.8 where glCopySubImage() inverts the y axis when it's used with a bound framebuffer object
[09:53] <fredrikh> it's fixed in 7.9
[09:54] <fredrikh> i'm not aware of anything like that with glScissor(), but it could be a similar issue

So it would be worth trying with Mesa 7.9. On my side, I'm using the NVIDIA driver which bypasses Mesa altogether.
Comment 13 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-09-18 15:55:11 PDT
Comment on attachment 476571 [details] [diff] [review]
use XP_MACOSX

r+a=shaver for define fix, may the gods of module purity have mercy on my soul
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2010-09-18 16:03:37 PDT
http://hg.mozilla.org/mozilla-central/rev/cf2461dc5055

Keeping this bug open until things work for Matt on Ubuntu.
Comment 15 Matt Woodrow (:mattwoodrow) 2010-09-19 17:14:19 PDT
Created attachment 476686 [details] [diff] [review]
Reverse Y axis when double buffering is disabled (drawing in a FBO)

Note You need to log in before you can comment on or make changes to this bug.