JSFiddler: http://jsfiddle.net/t5jy8ufm/ Repro steps: 1. Load multiple SVG images. All these SVG images should have the same size (size means Height x Width. This is the key to repro). 2. Convert each of them to texture. Then, render the texture on canvas. Expected (behavior in Edge/Chrome/FF): These textures are rendered one by one. There is no overlap between them. Actual (behavior in Safari): They are rendered one by one, somehow the latter images will “reuse” the rendering of a previous image, causing incorrect results.
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/604.5.6 (KHTML, like Gecko) Version/11.0.3 Safari/604.5.6
rdar://problem/37090895
Wow, we draw totally different results from Firefox/Chrome
Created attachment 333236 [details] Patch
Comment on attachment 333236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333236&action=review No need to make the changes I suggest. > LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:12 > + (function() { I wonder if this test would have been easier if it was drawing a single texture into the full canvas. ie. gl.clear set up image 1 gl.draw gl.clear set up image 2 gl.draw that way you should only end up with image 2 in the canvas. If you see parts of image 1, then the SVG buffer was not correctly cleared. This would avoid a bit of math in the test, such as the need for x,y,width,height in the texture quads. > LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:15 > + uniform vec2 u_resolution; ...and this wouldn't be necessary, since you're always drawing the full texture. In fact, I'm not quite sure why it is needed even now. The buffer for a_postion could be [-1, -1, 1, -1, -1, 1, 1, -1, 1, 1, -1, 1] - which is two triangles that cover the entire canvas. Then you could either provide [0, 0, 1, 0, 0, 1, 1, 0, 1, 1, 0, 1] for the texture coordinates, or simply calculate them in the shader from the vertex position. > LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:21 > + gl_Position = vec4((a_position / u_resolution) * 2.0 - 1.0, 0, 1); > + v_texCoord = a_texCoord; So this would become: gl_Position = a_position; // you'd have to change attribute vec2 a_position to attribute vec4 a_position; v_texCoord = (a_position + 1.0) / 2.0; > LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:63 > + promises.push(drawTexture(program, gl, textureTargets[index].url, textureTargets[index].x, textureTargets[index].y, textureTargets[index].width, textureTargets[index].height)); I think you could have just sent the object/dictionary in as a single argument, rather than expanding it. > LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:106 > + gl.generateMipmap(gl.TEXTURE_2D); You don't need this line. However, you should probably put this: gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S, gl.CLAMP_TO_EDGE); gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_T, gl.CLAMP_TO_EDGE); gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR); gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.LINEAR); You've managed to not need this because you picked a texture size that is a power of 2.
Created attachment 333242 [details] Patch
Comment on attachment 333236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333236&action=review >> LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:12 >> + (function() { > > I wonder if this test would have been easier if it was drawing a single texture into the full canvas. ie. > > gl.clear > set up image 1 > gl.draw > gl.clear > set up image 2 > gl.draw > > that way you should only end up with image 2 in the canvas. If you see parts of image 1, then the SVG buffer was not correctly cleared. This would avoid a bit of math in the test, such as the need for x,y,width,height in the texture quads. Done. This is better. >> LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:15 >> + uniform vec2 u_resolution; > > ...and this wouldn't be necessary, since you're always drawing the full texture. In fact, I'm not quite sure why it is needed even now. > > The buffer for a_postion could be [-1, -1, 1, -1, -1, 1, 1, -1, 1, 1, -1, 1] - which is two triangles that cover the entire canvas. > Then you could either provide [0, 0, 1, 0, 0, 1, 1, 0, 1, 1, 0, 1] for the texture coordinates, or simply calculate them in the shader from the vertex position. Done. All the calculations were removed. >> LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:21 >> + v_texCoord = a_texCoord; > > So this would become: > > gl_Position = a_position; // you'd have to change attribute vec2 a_position to attribute vec4 a_position; > v_texCoord = (a_position + 1.0) / 2.0; Done. >> LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:63 >> + promises.push(drawTexture(program, gl, textureTargets[index].url, textureTargets[index].x, textureTargets[index].y, textureTargets[index].width, textureTargets[index].height)); > > I think you could have just sent the object/dictionary in as a single argument, rather than expanding it. No need for passing the drawing rectangle anymore. Only the URL is passed. >> LayoutTests/webgl/webgl-texture-image-buffer-reuse.html:106 >> + gl.generateMipmap(gl.TEXTURE_2D); > > You don't need this line. > > However, you should probably put this: > > gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S, gl.CLAMP_TO_EDGE); > gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_T, gl.CLAMP_TO_EDGE); > gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR); > gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.LINEAR); > > You've managed to not need this because you picked a texture size that is a power of 2. Done. And the canvas size was changed to (100x100).
Created attachment 333246 [details] Patch
Comment on attachment 333246 [details] Patch Clearing flags on attachment: 333246 Committed r228213: <https://trac.webkit.org/changeset/228213>
All reviewed patches have been landed. Closing bug.
The LayoutTest added with this change is consistently failing on iOS Simulator: https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r228249%20(3075)/results.html
(In reply to Ryan Haddad from comment #11) > The LayoutTest added with this change is consistently failing on iOS > Simulator: > https://build.webkit.org/results/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r228249%20(3075)/ > results.html The test was skipped in <https://trac.webkit.org/changeset/228256>. The bug https://bugs.webkit.org/show_bug.cgi?id=182592 was filed to track fixing the test for 2x display.