Closed Bug 1368284 Opened 6 years ago Closed 6 years ago

webgl loop using less than only iterates a couple times, period

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Version:
53 Branch
Platform:
Unspecified
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: robert, Assigned: daoshengmu)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

Bug 1368284 - Add SH_ADD_AND_TRUE_TO_LOOP_CONDITION shader compile work around for Intel drivers on MacOSX;
59 bytes, text/x-review-board-request
jgilbert
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170518000419

Steps to reproduce:

I used the following glsl code:
```glsl

highp float kernelResult = 0.0;
void kernel() {
float user_x=0.0;

for (float user_i=0.0;(user_i < 10.0);user_i++){
user_x=(user_x+1.0);
}

kernelResult = user_x / 10.0;
}

void main( void ) {
	kernel();
	gl_FragColor.r = kernelResult;
	gl_FragColor.g = kernelResult;
	gl_FragColor.b = kernelResult;

}
```


Actual results:

The loop never gets passed once or twice.  However, when you use less than or equal to (<=) it works as expected.


Expected results:

The following loops should do the same exact thing:
```glsl
for (float user_i=0.0;(user_i < 10.0);user_i++){
```

```glsl
for (float user_i=0.0;(user_i <= 9.0);user_i++){
```
To reproduce: http://glslsandbox.com/e#40647.0
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
Flags: needinfo?(dmu)
Whiteboard: [gfx-noted]
This looks like a bug for shader compiler. I can reproduce it locally on my Mac. However, Chrome works well on this sample code.
Flags: needinfo?(dmu)
On Windows 10, it is good.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Mac OS X
On Mac with AMD or NV GPUs are good. It only happens on Mac using Intel GPU.
Assignee: nobody → dmu
After enable 'WEBGL_debug_shaders' extension, we can get some difference between Firefox and Chrome at the fragment shader.

--------------------------
In Firefox

#extension GL_ARB_gpu_shader5 : enable
varying vec4 webgl_b4d819df8386ec1;
varying vec3 webgl_93c00d372b1b4a9;
varying vec2 webgl_32f30d2968d2f3cf;
uniform vec4 webgl_eb58f14022fea5de;
uniform vec4 webgl_7509db683ee04a04;
uniform vec4 webgl_7569cd5cadf121c5;
float webgl_32e3b78af43c9131(){
float webgl_23194e0f4458bf51 = 0.0;
for (float webgl_ecabb75d5535b019 = 0.0; (webgl_ecabb75d5535b019 < 10.0); (webgl_ecabb75d5535b019++))
{
(webgl_23194e0f4458bf51 = (webgl_23194e0f4458bf51 + 1.0));
}
return (webgl_23194e0f4458bf51 / 10.0);
}
void main(){
(gl_FragColor = vec4(0.0, 0.0, 0.0, 0.0));
vec4 webgl_993afeba60642268;
(webgl_993afeba60642268.w = (webgl_eb58f14022fea5de.w * webgl_b4d819df8386ec1.w));
vec3 webgl_d63a0821229be6c6 = normalize(webgl_93c00d372b1b4a9);
float webgl_36cf6ca598d33be9 = dot(webgl_d63a0821229be6c6, vec3(webgl_7509db683ee04a04));
float webgl_a716eff6ab1886a1 = webgl_32e3b78af43c9131();
(webgl_993afeba60642268.xyz = vec3(webgl_a716eff6ab1886a1, webgl_a716eff6ab1886a1, webgl_a716eff6ab1886a1));
(gl_FragColor = webgl_993afeba60642268);
}

-------------------------------
In Chrome

#version 410
out vec4 webgl_FragColor;
in vec4 webgl_169a7a6aa3c9636e;
in vec3 webgl_e2f0046712458cac;
in vec2 webgl_958fb98f72119846;
uniform vec4 webgl_ed453eaed6b35a1a;
uniform vec4 webgl_9eb4d92dc1c1ecba;
uniform vec4 webgl_ea2703688485f4da;
float webgl_63096b2f10557d80(){
float webgl_442bf1bb4d091fc3 = 0.0;
for (float webgl_d1e902eb38d9db10 = 0.0; (((webgl_d1e902eb38d9db10 < 10.0)) ? (true) : (false)); (webgl_d1e902eb38d9db10++))
{
(webgl_442bf1bb4d091fc3 = (webgl_442bf1bb4d091fc3 + 1.0));
}
return (webgl_442bf1bb4d091fc3 / 10.0);
}
void main(){
(webgl_FragColor = vec4(0.0, 0.0, 0.0, 0.0));
vec4 webgl_83732344298d9ffe;
(webgl_83732344298d9ffe.w = (webgl_ed453eaed6b35a1a.w * webgl_169a7a6aa3c9636e.w));
vec3 webgl_30668dba91e90790 = normalize(webgl_e2f0046712458cac);
float webgl_f75713992ec6dbe4 = dot(webgl_30668dba91e90790, vec3(webgl_9eb4d92dc1c1ecba));
float webgl_dd53f4a42b14b185 = webgl_63096b2f10557d80();
(webgl_83732344298d9ffe.xyz = vec3(webgl_dd53f4a42b14b185, webgl_dd53f4a42b14b185, webgl_dd53f4a42b14b185));
(webgl_FragColor = webgl_83732344298d9ffe);
}


I think the root cause is the function for translating for-loop. Please take a look the code at below, in Chrome, they have an additional conditional operator for checking the loop needs to be break but Firefox doesn't have.

float webgl_63096b2f10557d80(){
float webgl_442bf1bb4d091fc3 = 0.0;
for (float webgl_d1e902eb38d9db10 = 0.0; (((webgl_d1e902eb38d9db10 < 10.0)) ? (true) : (false)); (webgl_d1e902eb38d9db10++))
{
(webgl_442bf1bb4d091fc3 = (webgl_442bf1bb4d091fc3 + 1.0));
}
return (webgl_442bf1bb4d091fc3 / 10.0);
}
I have checked neither Windows nor Linux with Intel GPU needs this workaround. It only happens on MacOSX.
Comment on attachment 8879406 [details]
Bug 1368284 - Add SH_ADD_AND_TRUE_TO_LOOP_CONDITION shader compile work around for Intel drivers on MacOSX;

https://reviewboard.mozilla.org/r/150728/#review155928

::: dom/canvas/WebGLShaderValidator.cpp:59
(Diff revision 1)
>          options |= SH_REGENERATE_STRUCT_NAMES;
>          options |= SH_INIT_OUTPUT_VARIABLES;
> +
> +        // Work around that Intel drivers on Mac OSX handle for-loop incorrectly.
> +        if (gl->Vendor() == gl::GLVendor::Intel) {
> +            options ^= SH_ADD_AND_TRUE_TO_LOOP_CONDITION;

Why ^= not |=?
Comment on attachment 8879406 [details]
Bug 1368284 - Add SH_ADD_AND_TRUE_TO_LOOP_CONDITION shader compile work around for Intel drivers on MacOSX;

https://reviewboard.mozilla.org/r/150728/#review155928

> Why ^= not |=?

Well, that is my mistake. I have fixed it. Thanks!
Comment on attachment 8879406 [details]
Bug 1368284 - Add SH_ADD_AND_TRUE_TO_LOOP_CONDITION shader compile work around for Intel drivers on MacOSX;

https://reviewboard.mozilla.org/r/150728/#review156976
Attachment #8879406 - Flags: review?(jgilbert) → review+
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfdccf4dc6a1
Add SH_ADD_AND_TRUE_TO_LOOP_CONDITION shader compile work around for Intel drivers on MacOSX; r=jgilbert
https://hg.mozilla.org/mozilla-central/rev/dfdccf4dc6a1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/2f10d697c307
Backed out changeset dfdccf4dc6a1 to fix bustage a=me
Hi, had to back this out since this doesn't played well with a other change comming from inbound and so caused https://treeherder.mozilla.org/logviewer.html#?job_id=109510911&repo=mozilla-inbound
Status: RESOLVED → REOPENED
Flags: needinfo?(dmu)
Resolution: FIXED → ---
(In reply to Daosheng Mu[:daoshengmu] from comment #18)
> Comment on attachment 8879406 [details]
> Bug 1368284 - Add SH_ADD_AND_TRUE_TO_LOOP_CONDITION shader compile work
> around for Intel drivers on MacOSX;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/150728/diff/2-3/

Solved the conflict and update it.
Flags: needinfo?(dmu)
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05d88e32a4db
Add SH_ADD_AND_TRUE_TO_LOOP_CONDITION shader compile work around for Intel drivers on MacOSX; r=jgilbert
You guys rock!
https://hg.mozilla.org/mozilla-central/rev/05d88e32a4db
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
(In reply to Robert Plummer from comment #21)
> You guys rock!

Thanks for your report. Your help makes Firefox be better!
You need to log in before you can comment on or make changes to this bug.