Bug 37174

Summary: Must resolve multisampled back buffer during copyTexImage2D and copyTexSubImage2D
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, dglazkov, fishd, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
revised patch: responding to Ken Russell's review none

Kenneth Russell
Reported 2010-04-06 15:39:47 PDT
If the WebGL back buffer is multisampled (the antialias parameter is set to true during context creation), then during both copyTexImage2D and copyTexSubImage2D operations, it is necessary to resolve the framebuffer, bind the resolved version of the framebuffer, and then perform the operation. Afterward, the multisampled framebuffer should be re-bound. Currently these operations are raising an INVALID_OPERATION error because of the attempt to copy directly from the multisampled framebuffer to the texture.
Attachments
patch (14.70 KB, patch)
2010-04-06 18:12 PDT, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
revised patch: responding to Ken Russell's review (15.16 KB, patch)
2010-04-07 14:31 PDT, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Zhenyao Mo
Comment 1 2010-04-06 18:12:51 PDT
Created attachment 52690 [details] patch
Kenneth Russell
Comment 2 2010-04-07 14:21:01 PDT
Comment on attachment 52690 [details] patch The code looks good. One comment on the test case. > Index: LayoutTests/fast/canvas/webgl/copy-tex-image-and-sub-image-2d.html > =================================================================== > --- LayoutTests/fast/canvas/webgl/copy-tex-image-and-sub-image-2d.html (revision 0) > +++ LayoutTests/fast/canvas/webgl/copy-tex-image-and-sub-image-2d.html (revision 0) > @@ -0,0 +1,185 @@ > +<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 g_Position; > +attribute vec2 g_TexCoord0; > + > +varying vec2 texCoord; > + > +void main() > +{ > + gl_Position = vec4(g_Position.x, g_Position.y, g_Position.z, 1.0); > + texCoord = g_TexCoord0; > +} > +</script> > + > +<script id="fshader" type="x-shader/x-fragment"> > +uniform sampler2D tex; > +varying vec2 texCoord; > + > +void main() > +{ > + gl_FragColor = texture2D(tex, texCoord); > +} > +</script> > + > +<script> > +var successfullyParsed = false; > + > +function init() > +{ > + if (window.layoutTestController) { > + layoutTestController.overridePreference("WebKitWebGLEnabled", "1"); > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); > + } > + > + description('Verify copyTexImage2D and copyTexSubImage2D'); > + > + runTest(); > +} > + > +// These two declarations need to be global for "shouldBe" to see them > +var pixel = [0, 0, 0]; > +var correctColor = null; > + > +function runTestIteration(antialias) > +{ > + var gl = null; > + if (antialias) > + gl = initWebGL("antialiasOn", "vshader", "fshader", [ "g_Position", "g_TexCoord0" ], [ 0, 0, 0, 1 ], 1); > + else > + gl = initWebGL("antialiasOff", "vshader", "fshader", [ "g_Position", "g_TexCoord0" ], [ 0, 0, 0, 1 ], 1, { antialias: false }); > + > + var textureLoc = gl.getUniformLocation(gl.program, "tex"); > + > + var vertices = new WebGLFloatArray([ > + 1.0, 1.0, 0.0, > + -1.0, 1.0, 0.0, > + -1.0, -1.0, 0.0, > + 1.0, 1.0, 0.0, > + -1.0, -1.0, 0.0, > + 1.0, -1.0, 0.0]); > + var texCoords = new WebGLFloatArray([ > + 1.0, 1.0, > + 0.0, 1.0, > + 0.0, 0.0, > + 1.0, 1.0, > + 0.0, 0.0, > + 1.0, 0.0]); > + var texCoordOffset = vertices.byteLength; > + > + var vbo = gl.createBuffer(); > + gl.bindBuffer(gl.ARRAY_BUFFER, vbo); > + gl.bufferData(gl.ARRAY_BUFFER, > + texCoordOffset + texCoords.byteLength, > + gl.STATIC_DRAW); > + gl.bufferSubData(gl.ARRAY_BUFFER, 0, vertices); > + gl.bufferSubData(gl.ARRAY_BUFFER, texCoordOffset, texCoords); > + > + gl.enableVertexAttribArray(0); > + gl.vertexAttribPointer(0, 3, gl.FLOAT, false, 0, 0); > + gl.enableVertexAttribArray(1); > + gl.vertexAttribPointer(1, 2, gl.FLOAT, gl.FALSE, 0, texCoordOffset); > + > + gl.colorMask(1, 1, 1, 0); > + gl.disable(gl.BLEND); > + debug('Testing copyTexImage2D'); > + > + // Red canvas > + gl.clearColor(1, 0, 0, 1); > + gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT); > + > + var texture = gl.createTexture(); > + // Bind the texture to texture unit 0 > + gl.bindTexture(gl.TEXTURE_2D, texture); > + // Set up texture > + gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, 2, 2, 0, gl.RGBA, gl.UNSIGNED_BYTE, null); > + gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.NEAREST); > + gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.NEAREST); I think there should be a shouldBe("gl.getError()", "gl.NO_ERROR") here. > + gl.copyTexImage2D(gl.TEXTURE_2D, 0, gl.RGBA, 0, 0, 2, 2, 0); There should be a shouldBe("gl.getError()", "gl.NO_ERROR") here. Previously this was generating INVALID_OPERATION. > + > + // Green canvas > + gl.clearColor(0, 1, 0, 1); > + gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT); > + > + // Point the uniform sampler to texture unit 0 > + gl.uniform1i(textureLoc, 0); > + // Draw the triangles > + gl.drawArrays(gl.TRIANGLES, 0, 6); > + > + // Read back the rendering results, should be red > + var buf = gl.readPixels(0, 0, 2, 2, gl.RGBA, gl.UNSIGNED_BYTE); > + var idx = 0; > + correctColor = [255, 0, 0]; > + for (var y = 0; y < 2; y++) { > + for (var x = 0; x < 2; x++) { > + idx = (y * 2 + x) * 4; > + pixel[0] = buf[idx]; > + pixel[1] = buf[idx + 1]; > + pixel[2] = buf[idx + 2]; > + shouldBe("pixel", "correctColor"); > + } > + } > + > + debug('Testing copyTexSubImage2D'); > + > + // Green canvas > + gl.clearColor(0, 1, 0, 1); > + gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT); I think there should be a shouldBe("gl.getError()", "gl.NO_ERROR") here. > + > + gl.copyTexSubImage2D(gl.TEXTURE_2D, 0, 0, 0, 0, 0, 2, 2); There should be a shouldBe("gl.getError()", "gl.NO_ERROR") here. Previously this was generating INVALID_OPERATION. > + > + // Blue canvas > + gl.clearColor(0, 0, 1, 1); > + gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT); > + > + // Draw the triangles > + gl.drawArrays(gl.TRIANGLES, 0, 6); > + > + // Read back the rendering results, should be green > + buf = gl.readPixels(0, 0, 2, 2, gl.RGBA, gl.UNSIGNED_BYTE); > + correctColor = [0, 255, 0]; > + for (var y = 0; y < 2; y++) { > + for (var x = 0; x < 2; x++) { > + idx = (y * 2 + x) * 4; > + pixel[0] = buf[idx]; > + pixel[1] = buf[idx + 1]; > + pixel[2] = buf[idx + 2]; > + shouldBe("pixel", "correctColor"); > + } > + } > +} > + > +function runTest(antialias) > +{ > + debug("Testing with antialias on"); > + runTestIteration(true); > + debug("Testing with antialias off"); > + runTestIteration(false); > + > + 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="antialiasOn" width="2px" height="2px"></canvas> > +<canvas id="antialiasOff" width="2px" height="2px"></canvas> > +<div id="description"></div> > +<div id="console"></div> > +</body> > +</html> > Index: LayoutTests/fast/canvas/webgl/resources/utils3d.js > =================================================================== > --- LayoutTests/fast/canvas/webgl/resources/utils3d.js (revision 57160) > +++ LayoutTests/fast/canvas/webgl/resources/utils3d.js (working copy) > @@ -17,10 +17,10 @@ > // by the caller. By default, it maps to the window.console() function on WebKit and to > // an empty function on other browsers. > // > -function initWebGL(canvasName, vshader, fshader, attribs, clearColor, clearDepth) > +function initWebGL(canvasName, vshader, fshader, attribs, clearColor, clearDepth, contextAttribs) > { > var canvas = document.getElementById(canvasName); > - var gl = canvas.getContext("experimental-webgl"); > + var gl = canvas.getContext("experimental-webgl", contextAttribs); > if (!gl) { > alert("No WebGL context found"); > return null;
Zhenyao Mo
Comment 3 2010-04-07 14:31:17 PDT
Created attachment 52782 [details] revised patch: responding to Ken Russell's review
Kenneth Russell
Comment 4 2010-04-07 14:52:46 PDT
Comment on attachment 52782 [details] revised patch: responding to Ken Russell's review Looks good to me.
Dimitri Glazkov (Google)
Comment 5 2010-04-08 16:00:36 PDT
Comment on attachment 52782 [details] revised patch: responding to Ken Russell's review kbr's word is good as gold.
WebKit Commit Bot
Comment 6 2010-04-09 03:07:03 PDT
Comment on attachment 52782 [details] revised patch: responding to Ken Russell's review Clearing flags on attachment: 52782 Committed r57322: <http://trac.webkit.org/changeset/57322>
WebKit Commit Bot
Comment 7 2010-04-09 03:07:09 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.