[webkit-reviews] review granted: [Bug 182367] Rendering SVG images with same size as WebGL texture doesn't work correctly : [Attachment 333236] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 182367: Rendering SVG images with same size as WebGL texture doesn't work
correctly
https://bugs.webkit.org/show_bug.cgi?id=182367

Attachment 333236: Patch

https://bugs.webkit.org/attachment.cgi?id=333236&action=review




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 333236
  --> https://bugs.webkit.org/attachment.cgi?id=333236
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.