771406 - Graphics driver crash on Intel+Mac for glsl faceforward w/T = float

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

See bug 735560 for origin. Switching on ANGLE's GLSL function emulation fixes the crashes, but ANGLE doesn't emulate faceforward. This bug should add ANGLE GLSL emulation support for faceforward(float, float, float). (T=float seems to be the only problem. T=vec2,vec3,vec4 should all be fine)

Known affected machines are MacBook Airs, possibly others.

Testcase is linked by URL.

Comment 1

[daniel-bzmz]

Please file an ANGLE bug and CC Ken and Mo.

Comment 2

[daniel-bzmz]

Oh and an Apple Radar too, of course :-)

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to daniel-bzmz from comment #2)
> Oh and an Apple Radar too, of course :-)

It should already have a Radar, since it's just a continuation of the workarounding for the previous bug.

Comment 4

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

For the Intel driver this is Radar 10670574.

Filed bug 786318 about this crashing our OSX 10.8 slaves --- strange, I didn't expect them to use the Intel driver, let's check.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

This builds locally, and thrown it at try at:
https://tbpl.mozilla.org/?tree=Try&rev=7595ef831376

I'll file this with ANGLE if it's not there already.

Comment 6

[Kelsey Gilbert [:jgilbert]]

This is now ANGLE bug 364.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Nevermind, this was ANGLE bug 277, marked invalid:
http://code.google.com/p/angleproject/issues/detail?id=277

We should probably use this anyways, since it doesn't seem like we're going to be getting fixed drivers for this issue, especially for older versions of Mac.

Comment 8

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

Comment on attachment 656693 [details] [diff] [review]
patch

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

Looks good on the surface... but I don't know this code. r+ if this is an emergency. Need proper review from ANGLE team.

::: gfx/angle/src/compiler/BuiltInFunctionEmulator.cpp
@@ +273,5 @@
> +            bool needToEmulate = false;
> +
> +            if (sequence.size() == 2) {
> +                TIntermTyped* param1 = sequence[0]->getAsTyped();
> +                TIntermTyped* param2 = sequence[1]->getAsTyped();

I'm really not competent to review this code. I'm just checking that this seems consistent with surrounding code. OK to land this if this is an emergency, otherwise it wouldn't hurt to get it reviewed upstream first.

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> Comment on attachment 656693 [details] [diff] [review]
> patch
> 
> Review of attachment 656693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good on the surface... but I don't know this code. r+ if this is an
> emergency. Need proper review from ANGLE team.
> 
> ::: gfx/angle/src/compiler/BuiltInFunctionEmulator.cpp
> @@ +273,5 @@
> > +            bool needToEmulate = false;
> > +
> > +            if (sequence.size() == 2) {
> > +                TIntermTyped* param1 = sequence[0]->getAsTyped();
> > +                TIntermTyped* param2 = sequence[1]->getAsTyped();
> 
> I'm really not competent to review this code. I'm just checking that this
> seems consistent with surrounding code. OK to land this if this is an
> emergency, otherwise it wouldn't hurt to get it reviewed upstream first.

Unfortunately, ANGLE is uninterested in fixing this issue, so I doubt they'd be interested in reviewing this. If we don't adopt this fix, we should blocklist Mac+Intel, which I don't think we want to do. I do not think it's viable to wait for Apple to release new drivers, especially for those people who are not on 10.8.

I am fairly confident in this code. I mimed the surrounding code, which already had examples for both 1-arg and 2-arg versions, so the new 3-arg versions should be fine.

I think we should wrap this up and land it. Since this is a crash bug that is hit simply by running the conformance suite, I don't feel like we have much choice.

Comment 10

[Kelsey Gilbert [:jgilbert]]

It seems like this hasn't quite fixed this:
https://tbpl.mozilla.org/?tree=Try&rev=7595ef831376&noignore=1

It could very well be because I'm using dot(float,float) in the faceforward(float,float,float) macro, and it's using the broken version of dot() instead of our macro. I'll try replacing it with just the multiplication.

Comment 11

[Kelsey Gilbert [:jgilbert]]

New version Trying at:
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=ea972e28222b

Comment 12

[Kelsey Gilbert [:jgilbert]]

And:
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=3d70795bf457

Looks like I was shadowing a variable after moving some code around.

Comment 13

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

Yup, OK, go ahead and land it when it's ready.

We have put so much effort into working around driver bugs on Mac+Intel already, and people do buy MacBook Air's.

Comment 14

[Kelsey Gilbert [:jgilbert]]

More try:
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=57753d120ed5

I forgot GLSL doesn't coerce |0| to |0.0|.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Try run looks clean this time.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: passing patch

Carrying forward r+.

Changes since r+:
Use multiplication instead of dot() builtin, since dot() also needs emulation.
Use |< 0.0| instead of |< 0|.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: Add to ANGLE patch list

Comment 18

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/482fce902f2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/70ced7d14f3c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/482fce902f2c
https://hg.mozilla.org/mozilla-central/rev/70ced7d14f3c

Comment 20

[Phil Ringnalda (:philor)]

I can't evaluate the risk, and I'm fine with either answer, but so I know whether or not to push my other fixes for test failures in mochitest-1 down the line, can this be pushed down to aurora and beta?