Bug 37178

Summary:

Must enable GL_VERTEX_PROGRAM_POINT_SIZE during initialization

Product:

WebKit

Reporter:

Kenneth Russell <kbr>

Component:

WebGL

Assignee:

Zhenyao Mo <zmo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarrin, commit-queue, dglazkov, fishd, kbr, oliver, zmo

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	none
revised patch : responding to Ken Russell's review	none
revised patch: update the expected test output	none
revised patch: modified a few comments in the test	none

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	Details Formatted Diff Diff
revised patch : responding to Ken Russell's review (7.72 KB, patch) 2010-04-09 17:27 PDT, Zhenyao Mo	no flags	Details Formatted Diff Diff
revised patch: update the expected test output (7.69 KB, patch) 2010-04-09 17:34 PDT, Zhenyao Mo	no flags	Details Formatted Diff Diff
revised patch: modified a few comments in the test (7.91 KB, patch) 2010-04-09 17:38 PDT, Zhenyao Mo	no flags	Details Formatted Diff Diff
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.