RESOLVED FIXED 37178
Must enable GL_VERTEX_PROGRAM_POINT_SIZE during initialization 
https://bugs.webkit.org/show_bug.cgi?id=37178
Summary Must enable GL_VERTEX_PROGRAM_POINT_SIZE during initialization
Kenneth Russell
Reported 2010-04-06 16:40:45 PDT
In OpenGL ES 2.0, the semantic is that the desktop GL capability GL_VERTEX_PROGRAM_SIZE is enabled all of the time; this enum is not exposed in the OpenGL ES 2.0 headers. In order to comply with this, in the desktop GL implementations of WebGL, this capability must be enabled at initialization time.
Attachments
patch (7.11 KB, patch)
2010-04-09 14:09 PDT, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
revised patch : responding to Ken Russell's review (7.72 KB, patch)
2010-04-09 17:27 PDT, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
revised patch: update the expected test output (7.69 KB, patch)
2010-04-09 17:34 PDT, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
revised patch: modified a few comments in the test (7.91 KB, patch)
2010-04-09 17:38 PDT, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Zhenyao Mo
Comment 1 2010-04-09 14:09:40 PDT
Created attachment 52988 [details] patch
Kenneth Russell
Comment 2 2010-04-09 14:56:46 PDT
Comment on attachment 52988 [details] patch > Index: LayoutTests/fast/canvas/webgl/point-size.html > =================================================================== > --- LayoutTests/fast/canvas/webgl/point-size.html (revision 0) > +++ LayoutTests/fast/canvas/webgl/point-size.html (revision 0) > @@ -0,0 +1,113 @@ > +<html> > +<head> > +<link rel="stylesheet" href="../../js/resources/js-test-style.css"/> > +<script src="../../js/resources/js-test-pre.js"></script> > +<script src="resources/webgl-test.js"></script> > +<script src="resources/utils3d.js"></script> > +<script id="vshader" type="x-shader/x-vertex"> > +attribute vec3 pos; > +attribute vec4 colorIn; > +uniform float pointSize; > +varying vec4 color; > + > +void main() > +{ > + gl_PointSize = pointSize; > + color = colorIn; > + gl_Position = vec4(pos.xyz, 1.0); > +} > +</script> > + > +<script id="fshader" type="x-shader/x-fragment"> > +varying vec4 color; > + > +void main() > +{ > + gl_FragColor = color; > +} > +</script> > + > +<script> > +var successfullyParsed = false; > + > +// These two declarations need to be global for "shouldBe" to see them > +var gl = null; > +var pixel = [0, 255, 0]; > + > +function init() > +{ > + if (window.layoutTestController) { > + layoutTestController.overridePreference("WebKitWebGLEnabled", "1"); > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); > + } This more complex framework (using layoutTestController.waitUntilDone()) isn't needed for this test. It is only needed when resources, like textures, are being downloaded in the background and the test needs to wait for them to download in order to run to completion. You can use tex-sub-image-2d.html as the basis for this test. We should use the simpler framework so it will be easier to port these to the Khronos site later. > + > + description('Verify GL_VERTEX_PROGRAM_POINT_SIZE is enabled in WebGL'); > + > + runTest(); > +} > + > +function runTest() > +{ > + shouldBeNonNull("gl = initWebGL('testbed', 'vshader', 'fshader', ['pos', 'colorIn'], [0, 0, 0, 1], 1, 0)"); > + gl.disable(gl.BLEND); > + gl.clear(gl.COLOR_BUFFER_BIT); > + > + var vertices = new WebGLFloatArray([ > + 0.5, 0.5, 0.0]); > + var colors = new WebGLUnsignedByteArray([ > + 255, 0, 0, 255]); > + > + var colorOffset = vertices.byteLength; This test should query gl.getParameter(gl.ALIASED_POINT_SIZE_RANGE) and see whether the range covers [1.0, 2.0]. If it turns out the implementation doesn't support a size greater than 1.0, the test should report that it passes. While this behavior won't work with the -expected.txt file, it will be semantically correct and will work within the Khronos conformance test harness. > + var vbo = gl.createBuffer(); > + gl.bindBuffer(gl.ARRAY_BUFFER, vbo); > + gl.bufferData(gl.ARRAY_BUFFER, colorOffset + colors.byteLength, gl.STATIC_DRAW); > + gl.bufferSubData(gl.ARRAY_BUFFER, 0, vertices); > + gl.bufferSubData(gl.ARRAY_BUFFER, colorOffset, colors); > + > + gl.vertexAttribPointer(0, 3, gl.FLOAT, false, 0, 0); > + gl.enableVertexAttribArray(0); > + gl.vertexAttribPointer(1, 4, gl.UNSIGNED_BYTE, true, 0, colorOffset); > + gl.enableVertexAttribArray(1); > + > + var locPointSize = gl.getUniformLocation(gl.program, 'pointSize'); A comment (or debug statement) would be helpful here, like "Draw a point of size 1 and verify it doesn't touch any other pixels." > + gl.uniform1f(locPointSize, 1.0); > + gl.drawArrays(gl.POINTS, 0, vertices.length / 3); > + var buf = gl.readPixels(0, 0, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE); > + pixel[0] = buf[0]; > + pixel[1] = buf[1]; > + pixel[2] = buf[2]; > + shouldBeTrue("pixel[0] == 0 && pixel[1] == 0 && pixel[2] == 0"); > + > + gl.clear(gl.COLOR_BUFFER_BIT); A comment (or debug statement) would be helpful here, like "Draw a point of size 2 and verify it fills the appropriate region." > + gl.uniform1f(locPointSize, 2.0); > + gl.drawArrays(gl.POINTS, 0, vertices.length / 3); > + buf = gl.readPixels(0, 0, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE); I think you should read the pixel at (0, 1) (or (1, 0)) rather than (0, 0). I believe that one should be guaranteed to be filled with exactly the intended color. > + pixel[0] = buf[0]; > + pixel[1] = buf[1]; > + pixel[2] = buf[2]; > + shouldBeTrue("pixel[0] > 0 && pixel[0] <= 255 && pixel[1] == 0 && pixel[2] == 0"); > + > + successfullyParsed = true; > + var epilogue = document.createElement("script"); > + epilogue.onload = finish; > + epilogue.src = "../../js/resources/js-test-post.js"; > + document.body.appendChild(epilogue); > +} > + > +function finish() { > + if (window.layoutTestController) { > + layoutTestController.notifyDone(); > + } > +} > +</script> > +</head> > +<body onload="init()"> > +<canvas id="testbed" width="2px" height="2px"></canvas> > +<div id="description"></div> > +<div id="console"></div> > +</body> > +</html>
Zhenyao Mo
Comment 3 2010-04-09 17:27:48 PDT
Created attachment 53019 [details] revised patch : responding to Ken Russell's review
Zhenyao Mo
Comment 4 2010-04-09 17:34:27 PDT
Created attachment 53021 [details] revised patch: update the expected test output
Zhenyao Mo
Comment 5 2010-04-09 17:38:12 PDT
Created attachment 53022 [details] revised patch: modified a few comments in the test
Kenneth Russell
Comment 6 2010-04-09 17:57:46 PDT
Comment on attachment 53022 [details] revised patch: modified a few comments in the test Looks great. Excellent work on the test case for this.
Eric Seidel (no email)
Comment 7 2010-04-10 15:49:15 PDT
Comment on attachment 53022 [details] revised patch: modified a few comments in the test I don't really understand what this does, or "why" we need to enable this. I believe you both, would just be nice to document "why" in the ChangeLog.
Kenneth Russell
Comment 8 2010-04-10 16:34:08 PDT
(In reply to comment #7) > (From update of attachment 53022 [details]) > I don't really understand what this does, or "why" we need to enable this. I > believe you both, would just be nice to document "why" in the ChangeLog. The description in the bug report concisely describes why it is needed. Is that sufficient?
Dimitri Glazkov (Google)
Comment 9 2010-04-15 12:33:01 PDT
Comment on attachment 53022 [details] revised patch: modified a few comments in the test rs=me given kbr approves.
WebKit Commit Bot
Comment 10 2010-04-15 17:43:57 PDT
Comment on attachment 53022 [details] revised patch: modified a few comments in the test Clearing flags on attachment: 53022 Committed r57694: <http://trac.webkit.org/changeset/57694>
WebKit Commit Bot
Comment 11 2010-04-15 17:44:04 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.