Bug 182367 - Rendering SVG images with same size as WebGL texture doesn't work correctly
Summary: Rendering SVG images with same size as WebGL texture doesn't work correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: Safari 11
Hardware: Macintosh macOS 10.13
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-31 17:50 PST by yifso
Modified: 2018-02-07 18:34 PST (History)
14 users (show)

See Also:


Attachments
Patch (8.52 KB, patch)
2018-02-06 16:42 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (7.68 KB, patch)
2018-02-06 18:09 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (7.68 KB, patch)
2018-02-06 18:15 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yifso 2018-01-31 17:50:42 PST
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.
Comment 1 yifso 2018-01-31 17:56:37 PST
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
Comment 2 Jon Lee 2018-01-31 20:07:24 PST
rdar://problem/37090895
Comment 3 Myles C. Maxfield 2018-02-05 10:36:08 PST
Wow, we draw totally different results from Firefox/Chrome
Comment 4 Said Abou-Hallawa 2018-02-06 16:42:44 PST
Created attachment 333236 [details]
Patch
Comment 5 Dean Jackson 2018-02-06 17:10:49 PST
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.
Comment 6 Said Abou-Hallawa 2018-02-06 18:09:56 PST
Created attachment 333242 [details]
Patch
Comment 7 Said Abou-Hallawa 2018-02-06 18:14:18 PST
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).
Comment 8 Said Abou-Hallawa 2018-02-06 18:15:30 PST
Created attachment 333246 [details]
Patch
Comment 9 WebKit Commit Bot 2018-02-06 18:51:06 PST
Comment on attachment 333246 [details]
Patch

Clearing flags on attachment: 333246

Committed r228213: <https://trac.webkit.org/changeset/228213>
Comment 10 WebKit Commit Bot 2018-02-06 18:51:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Ryan Haddad 2018-02-07 17:51:16 PST
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
Comment 12 Said Abou-Hallawa 2018-02-07 18:34:41 PST
(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.