Possible bug in OpenGL ES main fragment shader

[angushewlett]

Dear Micah & team,

Congratulations on an excellent & useful library - but I think I've found a bug in your main fragment shader...

In main.frag:-

// receives the color passed in via glColor4f emulation uniform vec4 u_color; ... color = u_color; ///v_frontColor; // copy the uniform color value to the current color ...

if TEXTURE0_ENABLED != 0 || TEXTURE1_ENABLED != 0 || TEXTURE2_ENABLED != 0

gl_FragColor = color;

else

gl_FragColor = color * u_color; // <== BUG - color is multiplied by itself.

endif

The bug causes colors with values < 255i or 1.0f to appear too dark.

I'm a newbie when it comes to OpenGL shaders so I've not been able to recompile the shader program with a fix. However, I hacked the call to glColor4f to pass the square root of r,g,b,a, and drawing is now correct:

GL->glColor4f(sqrt(r),sqrt(g),sqrt(b),sqrt(a));

Clearly this is an awful hack and the right thing to do is recompile the shader without the unwanted multiply. Drop me a line if you like, angus@fxpansion.com.

Best regards, Angus.

[micahpearlman]

That is odd as you are the first person to mention this and I haven't noticed at all. Though, I guess the GLES 1.1 pipeline is used more then the 2.0 pipeline. MonkVG uses the project gles2-bc (https://code.google.com/p/gles2-bc/) for rendering. The shader code is "compiled" using the "update" script in the shader directory "MonkVG/thirdparty/gles2-bc/Sources/OpenGLES/OpenGLES20/shaders". It is not really compiling but converting the shader text files into C arrays. This is what the update script looks like:

for i in .frag .vert *.glsl; do xxd -i $i > $i.h; done

[angushewlett]

I see, thanks!

On 12/03/2014 17:35, Micah Pearlman wrote:

That is odd as you are the first person to mention this and I haven't noticed at all. Though, I guess the GLES 1.1 pipeline is used more then the 2.0 pipeline. MonkVG uses the project gles2-bc (https://code.google.com/p/gles2-bc/) for rendering. The shader code is "compiled" using the "update" script in the shader directory "MonkVG/thirdparty/gles2-bc/Sources/OpenGLES/OpenGLES20/shaders". It is not really compiling but converting the shader text files into C arrays. This is what the update script looks like:

for i in .frag .vert *.glsl; do xxd -i $i > $i.h; done

— Reply to this email directly or view it on GitHub https://github.com/micahpearlman/MonkVG/issues/29#issuecomment-37438834.

Angus F. Hewlett, Managing Director (CEO) FXpansion Audio UK Ltd. - http://www.fxpansion.com Registered in the UK: 4455834 | VAT GB798778233

[micahpearlman]

Angus,

Apologies for taking so long. I've checked in a fix. Can you verify that it works?

[angushewlett]

Hi Micah,

Sorry for slow reply - am on vacation the next week or so but will check it out when I get back.

Best, Angus.

On 07/05/2014 00:40, Micah Pearlman wrote:

Angus,

Apologies for taking so long. I've checked in a fix. Can you verify that it works?

— Reply to this email directly or view it on GitHub https://github.com/micahpearlman/MonkVG/issues/29#issuecomment-42373352.

Angus F. Hewlett, Managing Director (CEO) FXpansion Audio UK Ltd. - http://www.fxpansion.com Registered in the UK: 4455834 | VAT GB798778233

[archagon]

I'm trying to get batching working and I've noticed a related issue. It seems that in the normal flow — when batching isn't enabled — the color of each curve is simply set by whatever the glColor4f is at the time, which in turn is set by the paint's setGLState call. However, when batching is turned on, the vert colors for each curve in the batch get moved into the VBO instead. In main.vert, these colors are correctly copied to v_frontColor. However, in main.frag, the final color is oddly set to gl_FragColor = color * u_color. Not only does this not make any sense, since color = u_color, but v_frontColor is nowhere to be found! Furthermore, even if the offending line is changed to gl_FragColor = v_frontColor, the v_frontColor is still typed as an unsigned byte (0-255). It has to be multiplied by vec4(1.0/256.0, 1.0/256.0, 1.0/256.0, 1.0/256.0) to get it to display correctly.

Oddly enough, v_frontColor is mentioned in the fragment shader, but it's commented out!

(I could be off in some of the technical details/terminology, as I'm a complete OpenGL amateur.)

This is running on an iPad via the OpenGLES 2.0 pipeline.

[micahpearlman]

Good find. User bug fixes are always welcome. Just send a pull request.

On Feb 2, 2015, at 9:13 AM, Alexei Baboulevitch notifications@github.com wrote:

I'm trying to get batching working and I've noticed a related issue. It seems that in the normal flow — when batching isn't enabled — the color of each vector path is simply set by whatever the glColor4f is at the time, which in turn is set by the paint's setGLState call. However, when batching is turned on, the vert colors for each path in the batch get moved into the VBO instead. In main.vert, these colors are correctly copied to v_frontColor. However, in main.frag, the final color is oddly set to gl_FragColor = color * u_color. Not only does this not make any sense, since color = u_color, but v_frontColor is nowhere to be found! Furthermore, even if the offending line is changed to gl_FragColor = v_frontColor, the v_frontColor is still typed as an unsigned byte (0-255). It has to be multiplied by vec4(1.0/256.0, 1.0/256.0, 1.0/256.0, 1.0/256.0) to get it to display correctly.

Oddly enough, v_frontColor is mentioned in the fragment shader, but it's commented out!

— Reply to this email directly or view it on GitHub.

[archagon]

Sure, I'll do that once I figure out a good solution. Is there a reason for the v_frontColors in the comments? (Or is that not your code?)

[micahpearlman]

It is a third party library. MonkVG was originally implemented in OpenGL ES 1.1. This library allows 1.1 emulation in opengl es 2.0.

On Feb 2, 2015, at 9:47 AM, Alexei Baboulevitch notifications@github.com wrote:

Sure, I'll do that once I figure out a good solution. Is there a reason for the v_frontColors in the comments? (Or is that not your code?)

— Reply to this email directly or view it on GitHub.

[archagon]

I'm looking at the original shader code side by side with the code in your project, and it looks like you commented out the original (correct) v_frontColor in both cases to replace it with color. Any idea why you made that change? It looks like it's related to the u_color uniform you added, but I'm not sure what it's doing. I don't want to break something by accident...

[micahpearlman]

I seem to remember it was a submission from another user to fix a bug where the wrong color was being generated. I had originally thought it was suspicious but didn’t seem to break anything obvious. I would say go back to the original.

~~ Micah Pearlman www.zero-engineering.net micahpearlman@gmail.com 415-637-6986 ~~

On Feb 2, 2015, at 6:55 PM, Alexei Baboulevitch notifications@github.com wrote:

I looked at the original code side by side with the code in your project, and it looks like you commented out the original (correct) v_frontColor in both cases to replace it with color. Any idea why you made that change? I don't want to break something by accident...

— Reply to this email directly or view it on GitHub.

[archagon]

OK, I think I get it. The u_color uniform makes glColor4f colors work correctly (which is what setting the paint before making a non-batched draw call does), but breaks GL_COLOR_ARRAY VBOs with interleaved colors (which is what batched draw calls use). I think I can probably fix this by putting u_color into the vertex shader.