1049887 - gl::ScopedScissorRect doesn't properly restore scissor rect after Fennec Java code modifies it

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The code at [1] is intended to save the current scissor rect and restore it at the end of the function, so as to guard against changes in the java code at [2].

However, this doesn't work. When the ScopedScissorRect is created, it reads the current scissor rect, which GLContext.h actually provides from the internally-cached mScissorRect [3]. That's fine. When the ScopedScissorRect is destroyed though, it calls fScissor which then compares the value to the cached value and does an early return at [4].

So even though the Java code modifies the scissor rect directly in the GL context, the ScopedScissorRect and GLContext don't know about this, and so don't think it needs to be restored.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=8581ea023552#2267
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/LayerRenderer.java?rev=0a020e033338#598
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContext.h?rev=ff07dcb918fd#1157
[4] http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContext.h?rev=ff07dcb918fd#1494

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Tag-along doc update

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Try push at https://tbpl.mozilla.org/?tree=Try&rev=c090fcbdd201 includes the fix patch.

Comment 4

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

I find it very scary to have to add this bool flag here, because if we can't assume that we're the only ones making GL calls on this GL context, then many other assumptions made in GLContext are wrong.

Why does the GL work in [2] have to be made in java? Can't it be made in C++ code going through GLContext instead?

Comment 5

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

Or if GL state changes have to be made direcly in Java, at least restore state locally in that Java code before reverting back to C++ code.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8468773 [details] [diff] [review]
Patch

OK, I'll look at other options here.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Reset GL state in java before returning to c++

How about this?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(Feel free to bounce the review to snorp if you think he's a more appropriate reviewer)

Comment 9

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

Comment on attachment 8475165 [details] [diff] [review]
Reset GL state in java before returning to c++

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

Yes, snorp may be a more appropriate reviewer for this, since this turned out to be a patch in Android-specific code. Also, I'm totally swamped (just moved to a new house, on top of lots of work).

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8475165 [details] [diff] [review]
Reset GL state in java before returning to c++

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

Ugh. I really wish we could ditch all the Java GL code.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Yeah it is kind of annoying.

https://hg.mozilla.org/integration/fx-team/rev/d60356c72b78

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d60356c72b78