Closed Bug 771406 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

All
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jgilbert, Assigned: jgilbert)

References

()

Details

(Keywords: crash, testcase)

Attachments

(2 files, 1 obsolete file)

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.
Keywords: crash, testcase
Please file an ANGLE bug and CC Ken and Mo.
Oh and an Apple Radar too, of course :-)
(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.
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.
Blocks: 786318
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Attachment #656693 - Flags: review?(bjacob)
This is now ANGLE bug 364.
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 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.
Attachment #656693 - Flags: review?(bjacob) → review+
(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.
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.
And:
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=3d70795bf457

Looks like I was shadowing a variable after moving some code around.
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.
More try:
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=57753d120ed5

I forgot GLSL doesn't coerce |0| to |0.0|.
Try run looks clean this time.
Attached patch passing patchSplinter Review
Carrying forward r+.

Changes since r+:
Use multiplication instead of dot() builtin, since dot() also needs emulation.
Use |< 0.0| instead of |< 0|.
Attachment #656693 - Attachment is obsolete: true
Attachment #659887 - Flags: review+
Attachment #659888 - Flags: review?(bjacob)
Attachment #659888 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/mozilla-central/rev/482fce902f2c
https://hg.mozilla.org/mozilla-central/rev/70ced7d14f3c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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?
You need to log in before you can comment on or make changes to this bug.