Fix handling of attributes prior to compiling shader
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 May 2014 19:02:49 +0000 (19:02 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 May 2014 19:02:49 +0000 (19:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=132430

Reviewed by Dean Jackson.

Source/WebCore:
WebGL programs that called bindAttribLocations prior to compiling shader sources
would perform the bind using the non-hashed symbol name, but would later create
the attributes as hashed names. Consequently, the program would refer to
attributes that were never actually part of any shader, resulting in some amazing
display artifacts.

This patch adds a dictionary of hashed symbol names so that we can tell the WebGL
program the proper name that will be used when the shader is eventually compiled,
allowing the WebGL program to link against the proper symbol after compiling and
linking completes.

* platform/graphics/GraphicsContext3D.h:
* platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:
(WebCore::generateHashedName): Function uses the ANGLE hashing
function to generate correct symbol.
(WebCore::GraphicsContext3D::mappedSymbolName): If we haven't compiled shaders yet, look
in our set of potentially unused attributes.
(WebCore::GraphicsContext3D::originalSymbolName): Ditto, for reverse lookup.

Source/WTF:
WebGL programs that called bindAttribLocations prior to compiling shader sources
would perform the bind using the non-hashed symbol name, but would later create
the attributes as hashed names. Consequently, the program would refer to
attributes that were never actually part of any shader, resulting in some amazing
display artifacts.

This patch adds a dictionary of hashed symbol names so that we can tell the WebGL
program the proper name that will be used when the shader is eventually compiled,
allowing the WebGL program to link against the proper symbol after compiling and
linking completes.

* wtf/HexNumber.h:
(WTF::appendUnsigned64AsHex): Add uint64_t-compatible hex->string converter.

LayoutTests:
* fast/canvas/webgl/gl-bind-attrib-location-before-compile-test-expected.txt: Added.
* fast/canvas/webgl/gl-bind-attrib-location-before-compile-test.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@168112 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/canvas/webgl/gl-bind-attrib-location-before-compile-test-expected.txt [new file with mode: 0644]
LayoutTests/fast/canvas/webgl/gl-bind-attrib-location-before-compile-test.html [new file with mode: 0644]
Source/WTF/ChangeLog
Source/WTF/wtf/HexNumber.h
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/GraphicsContext3D.h
Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp

index e659baa..3387cef 100644 (file)
@@ -1,3 +1,13 @@
+2014-05-01  Brent Fulgham  <bfulgham@apple.com>
+
+        Fix handling of attributes prior to compiling shader
+        https://bugs.webkit.org/show_bug.cgi?id=132430
+
+        Reviewed by Dean Jackson.
+
+        * fast/canvas/webgl/gl-bind-attrib-location-before-compile-test-expected.txt: Added.
+        * fast/canvas/webgl/gl-bind-attrib-location-before-compile-test.html: Added.
+
 2014-05-01  Zalan Bujtas  <zalan@apple.com>
 
         Subpixel rendering: Make selection gaps painting subpixel aware.
 2014-05-01  Zalan Bujtas  <zalan@apple.com>
 
         Subpixel rendering: Make selection gaps painting subpixel aware.
diff --git a/LayoutTests/fast/canvas/webgl/gl-bind-attrib-location-before-compile-test-expected.txt b/LayoutTests/fast/canvas/webgl/gl-bind-attrib-location-before-compile-test-expected.txt
new file mode 100644 (file)
index 0000000..8bad1f9
--- /dev/null
@@ -0,0 +1,28 @@
+This test ensures WebGL implementations properly handle bindAttribLocation before compiling shaders.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Canvas.getContext
+PASS [object WebGLRenderingContext] is non-null.
+
+Checking gl.bindAttribLocation.
+PASS getError was expected value: NO_ERROR : bindAttribLocation should return NO_ERROR when called before compiling shader.
+PASS program linked successfully
+_glesVertex:5
+vColor   :2
+PASS location of _glesVertex should be 5
+PASS location of vColor should be 2
+PASS drawing is correct
+PASS program linked successfully
+_glesVertex:5
+vColor   :1
+PASS location of _glesVertex should be 5
+PASS location of vColor should be 1
+FAIL pixel at (20,15) is (0,255,0,255), should be (255,0,0,255)
+PASS getError was expected value: NO_ERROR : 
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/canvas/webgl/gl-bind-attrib-location-before-compile-test.html b/LayoutTests/fast/canvas/webgl/gl-bind-attrib-location-before-compile-test.html
new file mode 100644 (file)
index 0000000..b847a2b
--- /dev/null
@@ -0,0 +1,197 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
+  "http://www.w3.org/TR/html4/loose.dtd">
+<html>
+<head>
+<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+<title>WebGL BindAttribLocation (Before Compile) Conformance Tests</title>
+<script src="../../../resources/js-test-pre.js"></script>
+<script src="resources/webgl-test.js"></script>
+</head>
+<body>
+<div id="description"></div>
+<div id="console"></div>
+<canvas style="border: 1px solid black;" id="canvas" width="50" height="50"></canvas>
+<script id="vshader" type="text/something-not-javascript">
+attribute vec4 _glesVertex;
+attribute vec4 vColor;
+varying vec4 color;
+void main()
+{
+  gl_Position = _glesVertex;
+  color = vColor;
+}
+</script>
+<script id="fshader" type="text/something-not-javascript">
+#ifdef GL_ES
+precision highp float;
+#endif
+varying vec4 color;
+void main()
+{
+  gl_FragColor = color;
+}
+</script>
+<script>
+description("This test ensures WebGL implementations properly handle bindAttribLocation before compiling shaders.");
+
+if (window.internals)
+    window.internals.settings.setWebGLErrorsToConsoleEnabled(false);
+
+debug("");
+debug("Canvas.getContext");
+
+var gl = create3DContext(document.getElementById("canvas"));
+shouldBeNonNull(gl);
+
+function fail(x,y, buf, shouldBe)
+{
+  var i = (y*50+x) * 4;
+  var reason = "pixel at ("+x+","+y+") is ("+buf[i]+","+buf[i+1]+","+buf[i+2]+","+buf[i+3]+"), should be "+shouldBe;
+  testFailed(reason);
+}
+
+function pass()
+{
+  testPassed("drawing is correct");
+}
+
+function loadShader(shaderType, shaderId) {
+  // Get the shader source.
+  var shaderSource = document.getElementById(shaderId).text;
+
+  // Create the shader object
+  var shader = gl.createShader(shaderType);
+  if (shader == null) {
+    debug("*** Error: unable to create shader '"+shaderId+"'");
+    return null;
+  }
+
+  // Load the shader source
+  gl.shaderSource(shader, shaderSource);
+
+  // Compile the shader
+  gl.compileShader(shader);
+
+  // Check the compile status
+  var compiled = gl.getShaderParameter(shader, gl.COMPILE_STATUS);
+  if (!compiled) {
+    // Something went wrong during compilation; get the error
+    var error = gl.getShaderInfoLog(shader);
+    debug("*** Error compiling shader '"+shader+"':"+error);
+    gl.deleteShader(shader);
+    return null;
+  }
+  return shader;
+}
+
+debug("");
+debug("Checking gl.bindAttribLocation.");
+
+var program = gl.createProgram();
+
+// Bind _glesVertex BEFORE we comile the program.
+gl.bindAttribLocation(program, 5, "_glesVertex");
+glErrorShouldBe(gl, gl.NO_ERROR,
+    "bindAttribLocation should return NO_ERROR when called before compiling shader.");
+
+var vs = loadShader(gl.VERTEX_SHADER, "vshader");
+var fs = loadShader(gl.FRAGMENT_SHADER, "fshader");
+gl.attachShader(program, vs);
+gl.attachShader(program, fs);
+
+var positions = gl.createBuffer();
+gl.bindBuffer(gl.ARRAY_BUFFER, positions);
+gl.bufferData(gl.ARRAY_BUFFER, new Float32Array([ 0,0.5,0, -0.5,-0.5,0, 0.5,-0.5,0 ]), gl.STATIC_DRAW);
+
+var colors = gl.createBuffer();
+gl.bindBuffer(gl.ARRAY_BUFFER, colors);
+gl.bufferData(gl.ARRAY_BUFFER, new Float32Array([
+    0,1,0,1,
+    0,1,0,1,
+    0,1,0,1]), gl.STATIC_DRAW);
+
+function setBindLocations(colorLocation, positionLocation) {
+  gl.bindAttribLocation(program, colorLocation, "vColor");
+  gl.linkProgram(program);
+  gl.useProgram(program);
+  var linked = (gl.getProgramParameter(program, gl.LINK_STATUS) != 0);
+  assertMsg(linked, "program linked successfully");
+
+  debug("_glesVertex:" + gl.getAttribLocation(program, "_glesVertex"))
+  debug("vColor   :" + gl.getAttribLocation(program, "vColor"))
+  assertMsg(gl.getAttribLocation(program, "_glesVertex") == positionLocation,
+            "location of _glesVertex should be " + positionLocation);
+  assertMsg(gl.getAttribLocation(program, "vColor") == colorLocation,
+            "location of vColor should be " + colorLocation);
+
+  var ploc = gl.getAttribLocation(program, "_glesVertex");
+  var cloc = gl.getAttribLocation(program, "vColor");
+  gl.bindBuffer(gl.ARRAY_BUFFER, positions);
+  gl.enableVertexAttribArray(positionLocation);
+  gl.vertexAttribPointer(positionLocation, 3, gl.FLOAT, false, 0, 0);
+  gl.bindBuffer(gl.ARRAY_BUFFER, colors);
+  gl.enableVertexAttribArray(colorLocation);
+  gl.vertexAttribPointer(colorLocation, 4, gl.FLOAT, false, 0, 0);
+}
+
+function checkDraw(colorLocation, positionLocation, r, g, b, a) {
+  gl.clearColor(0, 0, 0, 1);
+  gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT);
+  gl.drawArrays(gl.TRIANGLES, 0, 3);
+
+  var width = 50;
+  var height = 50;
+  var buf = new Uint8Array(width * height * 4);
+  gl.readPixels(0, 0, width, height, gl.RGBA, gl.UNSIGNED_BYTE, buf);
+
+  function checkPixel(x, y, r, g, b, a) {
+    var offset = (y * width + x) * 4;
+    if (buf[offset + 0] != r ||
+        buf[offset + 1] != g ||
+        buf[offset + 2] != b ||
+        buf[offset + 3] != a) {
+        fail(x, y, buf, "(" + r + "," + g + "," + b + "," + a + ")");
+        return false;
+    }
+    return true;
+  }
+
+  // Test several locations
+  // First line should be all black
+  var success = true;
+  for (var i = 0; i < 50; ++i)
+    success = success && checkPixel(i, 0, 0, 0, 0, 255);
+
+  // Line 15 should be red for at least 10 rgba pixels starting 20 pixels in
+  var offset = (15 * 50 + 20) * 4;
+  for (var i = 0; i < 10; ++i)
+    success = success && checkPixel(20 + i, 15, r, g, b, a);
+
+  // Last line should be all black
+  for (var i = 0; i < 50; ++i)
+    success = success && checkPixel(i, 49, 0, 0, 0, 255);
+
+  if (success)
+    pass();
+
+  gl.disableVertexAttribArray(positionLocation);
+  gl.disableVertexAttribArray(colorLocation);
+}
+
+setBindLocations(2, 5);
+checkDraw(2, 5, 0, 255, 0, 255);
+
+setBindLocations(1, 5);
+gl.disableVertexAttribArray(0);
+gl.vertexAttrib4f(0, 1, 0, 0, 1);
+checkDraw(1, 5, 255, 0, 0, 255);
+
+glErrorShouldBe(gl, gl.NO_ERROR);
+
+debug("");
+
+</script>
+<script src="../../../resources/js-test-post.js"></script>
+
+</body>
+</html>
index 6fc7d84..720ac60 100644 (file)
@@ -1,3 +1,24 @@
+2014-05-01  Brent Fulgham  <bfulgham@apple.com>
+
+        Fix handling of attributes prior to compiling shader
+        https://bugs.webkit.org/show_bug.cgi?id=132430
+
+        Reviewed by Dean Jackson.
+
+        WebGL programs that called bindAttribLocations prior to compiling shader sources
+        would perform the bind using the non-hashed symbol name, but would later create
+        the attributes as hashed names. Consequently, the program would refer to
+        attributes that were never actually part of any shader, resulting in some amazing
+        display artifacts.
+
+        This patch adds a dictionary of hashed symbol names so that we can tell the WebGL
+        program the proper name that will be used when the shader is eventually compiled,
+        allowing the WebGL program to link against the proper symbol after compiling and
+        linking completes.
+
+        * wtf/HexNumber.h:
+        (WTF::appendUnsigned64AsHex): Add uint64_t-compatible hex->string converter.
+
 2014-04-30  Geoffrey Garen  <ggaren@apple.com>
 
         Link against bmalloc in production builds
 2014-04-30  Geoffrey Garen  <ggaren@apple.com>
 
         Link against bmalloc in production builds
index b698dd5..741a8c1 100644 (file)
@@ -78,6 +78,20 @@ inline void appendUnsignedAsHex(unsigned number, T& destination, HexConversionMo
     result.reverse();
     destination.append(result.data(), result.size());
 }
     result.reverse();
     destination.append(result.data(), result.size());
 }
+    
+template<typename T>
+inline void appendUnsigned64AsHex(uint64_t number, T& destination, HexConversionMode mode = Uppercase)
+{
+    const LChar* hexDigits = Internal::hexDigitsForMode(mode);
+    Vector<LChar, 8> result;
+    do {
+        result.append(hexDigits[number % 16]);
+        number >>= 4;
+    } while (number > 0);
+    
+    result.reverse();
+    destination.append(result.data(), result.size());
+}
 
 // Same as appendUnsignedAsHex, but using exactly 'desiredDigits' for the conversion.
 template<typename T>
 
 // Same as appendUnsignedAsHex, but using exactly 'desiredDigits' for the conversion.
 template<typename T>
index 3edc28b..e9fa587 100644 (file)
@@ -1,3 +1,29 @@
+2014-05-01  Brent Fulgham  <bfulgham@apple.com>
+
+        Fix handling of attributes prior to compiling shader
+        https://bugs.webkit.org/show_bug.cgi?id=132430
+
+        Reviewed by Dean Jackson.
+
+        WebGL programs that called bindAttribLocations prior to compiling shader sources
+        would perform the bind using the non-hashed symbol name, but would later create
+        the attributes as hashed names. Consequently, the program would refer to
+        attributes that were never actually part of any shader, resulting in some amazing
+        display artifacts.
+
+        This patch adds a dictionary of hashed symbol names so that we can tell the WebGL
+        program the proper name that will be used when the shader is eventually compiled,
+        allowing the WebGL program to link against the proper symbol after compiling and
+        linking completes.
+
+        * platform/graphics/GraphicsContext3D.h:
+        * platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:
+        (WebCore::generateHashedName): Function uses the ANGLE hashing
+        function to generate correct symbol.
+        (WebCore::GraphicsContext3D::mappedSymbolName): If we haven't compiled shaders yet, look
+        in our set of potentially unused attributes.
+        (WebCore::GraphicsContext3D::originalSymbolName): Ditto, for reverse lookup.
+
 2014-05-01  Zalan Bujtas  <zalan@apple.com>
 
         Subpixel rendering: Make selection gaps painting subpixel aware.
 2014-05-01  Zalan Bujtas  <zalan@apple.com>
 
         Subpixel rendering: Make selection gaps painting subpixel aware.
index c7ff35d..6232ae1 100644 (file)
@@ -1091,7 +1091,10 @@ private:
     };
     typedef HashMap<Platform3DObject, ActiveShaderSymbolCounts> ShaderProgramSymbolCountMap;
     ShaderProgramSymbolCountMap m_shaderProgramSymbolCountMap;
     };
     typedef HashMap<Platform3DObject, ActiveShaderSymbolCounts> ShaderProgramSymbolCountMap;
     ShaderProgramSymbolCountMap m_shaderProgramSymbolCountMap;
-    
+
+    typedef HashMap<String, String> HashedSymbolMap;
+    HashedSymbolMap m_possiblyUnusedAttributeMap;
+
     String mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
     String mappedSymbolName(Platform3DObject shaders[2], size_t count, const String& name);
     String originalSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
     String mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
     String mappedSymbolName(Platform3DObject shaders[2], size_t count, const String& name);
     String originalSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
index 38bcb43..000199c 100644 (file)
@@ -54,6 +54,7 @@
 #include <runtime/Float32Array.h>
 #include <runtime/Int32Array.h>
 #include <runtime/Uint8Array.h>
 #include <runtime/Float32Array.h>
 #include <runtime/Int32Array.h>
 #include <runtime/Uint8Array.h>
+#include <wtf/HexNumber.h>
 #include <wtf/MainThread.h>
 #include <wtf/text/CString.h>
 #include <wtf/text/StringBuilder.h>
 #include <wtf/MainThread.h>
 #include <wtf/text/CString.h>
 #include <wtf/text/StringBuilder.h>
@@ -76,7 +77,7 @@
 
 namespace WebCore {
 
 
 namespace WebCore {
 
-static ShaderNameHash* currentNameHashMapForShader;
+static ShaderNameHash* currentNameHashMapForShader = nullptr;
 
 // Hash function used by the ANGLE translator/compiler to do
 // symbol name mangling. Since this is a static method, before
 
 // Hash function used by the ANGLE translator/compiler to do
 // symbol name mangling. Since this is a static method, before
@@ -789,10 +790,21 @@ void GraphicsContext3D::getAttachedShaders(Platform3DObject program, GC3Dsizei m
     ::glGetAttachedShaders(program, maxCount, count, shaders);
 }
 
     ::glGetAttachedShaders(program, maxCount, count, shaders);
 }
 
+static String generateHashedName(const String& name)
+{
+    if (name.isEmpty())
+        return name;
+    uint64_t number = nameHashForShader(name.utf8().data(), name.length());
+    StringBuilder builder;
+    builder.append("webgl_");
+    appendUnsigned64AsHex(number, builder, Lowercase);
+    return builder.toString();
+}
+
 String GraphicsContext3D::mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType symbolType, const String& name)
 {
     GC3Dsizei count;
 String GraphicsContext3D::mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType symbolType, const String& name)
 {
     GC3Dsizei count;
-    Platform3DObject shaders[2];
+    Platform3DObject shaders[2] = { };
     getAttachedShaders(program, 2, &count, shaders);
 
     for (GC3Dsizei i = 0; i < count; ++i) {
     getAttachedShaders(program, 2, &count, shaders);
 
     for (GC3Dsizei i = 0; i < count; ++i) {
@@ -805,6 +817,23 @@ String GraphicsContext3D::mappedSymbolName(Platform3DObject program, ANGLEShader
         if (symbolEntry != symbolMap.end())
             return symbolEntry->value.mappedName;
     }
         if (symbolEntry != symbolMap.end())
             return symbolEntry->value.mappedName;
     }
+
+    if (symbolType == SHADER_SYMBOL_TYPE_ATTRIBUTE && !name.isEmpty()) {
+        // Attributes are a special case: they may be requested before any shaders have been compiled,
+        // and aren't even required to be used in any shader program.
+        if (!nameHashMapForShaders)
+            nameHashMapForShaders = adoptPtr(new ShaderNameHash);
+        currentNameHashMapForShader = nameHashMapForShaders.get();
+
+        String generatedName = generateHashedName(name);
+
+        currentNameHashMapForShader = nullptr;
+
+        m_possiblyUnusedAttributeMap.set(generatedName, name);
+
+        return generatedName;
+    }
+
     return name;
 }
     
     return name;
 }
     
@@ -825,6 +854,16 @@ String GraphicsContext3D::originalSymbolName(Platform3DObject program, ANGLEShad
                 return symbolEntry.key;
         }
     }
                 return symbolEntry.key;
         }
     }
+
+    if (symbolType == SHADER_SYMBOL_TYPE_ATTRIBUTE && !name.isEmpty()) {
+        // Attributes are a special case: they may be requested before any shaders have been compiled,
+        // and aren't even required to be used in any shader program.
+
+        const auto& cached = m_possiblyUnusedAttributeMap.find(name);
+        if (cached != m_possiblyUnusedAttributeMap.end())
+            return cached->value;
+    }
+
     return name;
 }
 
     return name;
 }