Bug 182367 – Rendering SVG images with same size as WebGL texture doesn't work correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

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

[Jon Lee]

rdar://problem/37090895

[Myles C. Maxfield]

Wow, we draw totally different results from Firefox/Chrome

[Said Abou-Hallawa]

Created attachment 333236 [details]
Patch

[Dean Jackson]

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.

[Said Abou-Hallawa]

Created attachment 333242 [details]
Patch

[Said Abou-Hallawa]

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).

[Said Abou-Hallawa]

Created attachment 333246 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 333246 [details]
Patch

Clearing flags on attachment: 333246

Committed r228213: <https://trac.webkit.org/changeset/228213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Said Abou-Hallawa]

(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.