594489 - webgl shader validator truncates floating point literals?

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Sami Mäkelä]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.127 Safari/533.4
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5) Gecko/20100101 Firefox/4.0b5

There is a problem with floating point literals used in the shaders. It seems as if I use a literal such as 0.5, it is translated into 0.0 at some point.

Since I before had problems with my samples, where reloading fixed them, it is probably a problem in the shader validator, which was supposedly only enabled for the first WebGL context. One thing that the validator does seems to be checking that literals such as 1 or 0 are not used as floating point literals.

Reproducible: Always

Steps to Reproduce:
1. For example change the colors of first lesson from Learning WebGL to 0.99, 0.99, 0.99 instead of 1.0, 1.0, 1.0.
Actual Results:  
Nothing is seen (because the background is black), and 0.99 is truncated into 0.0.

Expected Results:  
Shows white shapes.

Comment 1

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

I can't reproduce this issue, either with or without the shader validator (pref webgl.shader_validator). I'm on linux x86-64 / nvidia 195.36).

Does the result change for you if you set the preference webgl.shader_validator to false?

How did you obtain Firefox and if built from sources, what revision is that? I know you pasted your UA above but the date Gecko/20100101 looks very strange.

Comment 2

[Markus Stange [:mstange]]

(In reply to comment #1)
> but the date Gecko/20100101 looks very strange.

The build date in the UA string has been frozen, see http://hacks.mozilla.org/2010/09/final-user-agent-string-for-firefox-4/

Comment 3

[Sami Mäkelä]

(In reply to comment #1)
> I can't reproduce this issue, either with or without the shader validator (pref
> webgl.shader_validator). I'm on linux x86-64 / nvidia 195.36).
> 
> Does the result change for you if you set the preference webgl.shader_validator
> to false?

This is just the 4.0 beta 5 for Windows that was released recently. It was a good idea to test webgl.shader_validator parameter, the results indeed change when it is switched confirming that the problem is in the validator. To test the issue easily, I've put a modified version of WebGL Lesson 1 to http://sami-code.110mb.com/gl/lesson1.html.

I also tested on Linux (32-bit) with beta 5, and it has the same behaviour, at least when using Mesa (on Windows I have ATI Opengl driver version 6.14.10.9655). Perhaps you have some older version, where the shader validator is only applied to the first context?

Comment 4

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

The fact that enabling/disabling the shader validator has an effect, means that:
 - either your shader is not GLSL ES compliant. You will then see messages in the error console (Ctrl+Shift+J) with the validator enabled.
 - or you hit a bug in the validator/translator. Indeed, this is translating the shader that you write (GLSL ES) into something that can be passed to the driver (GLSL). So if the error console doesn't say anything, you hit a translator bug and we should file a bug.

Comment 5

[Sami Mäkelä]

Ok now I've located the problem. ANGLE uses atof to convert floating point literals, but in fi_FI.utf8 locale, the decimal point is "," and therefore the 
literals are truncated.

Comment 6

[Ilmari Heikkinen]

I'm seeing this as well on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre.

On locales with decimal comma the ANGLE lexer fails on float literals. A workaround is to use fractions instead of decimals, e.g. 0.4 -> 4.0/10.0, or E-notation: 4e-1.

Filed a bug on the ANGLE issue tracker: http://code.google.com/p/angleproject/issues/detail?id=32

Comment 7

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

Thanks a lot for identifying the cause of the problem and filing upstream. This is a very american bug indeed: the C standard library's number parsing functions look fine until you realize that C locales are irremediably broken (since they are shared among all threads). The solution is to use C++ streams and imbue()... will tell that upstream.

Comment 8

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

I have submitted a patch upstream:
  http://code.google.com/p/angleproject/issues/detail?id=32#c5

Comment 9

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

Attachment: ANGLE: fix atof bug, upstreamed

Here's a version of the patch that applies to our copy in mozilla-central.

I couldn't try: make didn't pick it up. Could it be that that's still disabled on linux x86-64 ?

Vlad, do you think we should apply this until ANGLE applies it?

Comment 10

[Sami Mäkelä]

Just a quick note: make doesn't recompile the parser or lexer automatically, I think you currently have to manually regenerate the cpp-files and put them to angle/generated.

Comment 11

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

Ah! Thanks. Will try ASAP.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

it is also still disabled on x86-64.  Taking this in the interim sounds fine -- just make sure you add a .patch file to the gfx/angle directory and add it to the list in gfx/angle/README.mozilla.

Comment 13

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

Attachment: Fix angle's use of atof

OK, patching this locally for now. Sending the patch upstream.

Comment 14

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

Attachment: Fix angle's use of atof

forgot hg add...

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

needs a patch file in gfx/angle, and a line describing the patch in gfx/angle/README.mozilla... also, I believe you need to regenerate the parser from glslang.l (it's not automatic).

Comment 16

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

Attachment: Fix angle's use of atof

Here you go: added patch, regenerated files, edited readme.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 481055 [details] [diff] [review]
Fix angle's use of atof

Great, thanks!

Comment 18

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

Attachment: updated patch (using c++ stream)

ok, so, nonstandard c functions are a crossplatform nightmare. Can't get this stuff to build on linux tryserver even though it's building here on fedora

 ---> this new patch uses c++ streams instead. Will ask for review once i've tested on windows.

Comment 19

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

Comment on attachment 481932 [details] [diff] [review]
updated patch (using c++ stream)

Seems to build fine with msvc 2010. Now running this through the tryserver...

Comment 20

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

Attachment: updated patch

Previous patch didnt have the right buildsystem changes, retrying

Comment 21

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

Attachment: part 1: ANGLE fixes

Let's do this in 2 parts: first, the ANGLE fixes....

Comment 22

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

Attachment: part 2: mozilla integration

... and part 2: Mozilla integration, i.e. generated files, Makefile.in, and copy of part 1 patch under gfx/angle/

Comment 23

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

Bug 578880 is now preventing me from landing this. Argh!

Comment 24

[Scoobidiver (away)]

Is is fixed now? It seems to be fixed in angle r470.

Comment 25

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

It is fixed in ANGLE upstream, but bug 578880 is preventing us from syncing our copy of ANGLE.

Comment 26

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

Should have been fixed by recent update of our ANGLE copy.