934813 - Gallery: ImageProcessor.prototype.draw should not call bufferData everytime

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This is quite similar to bug 934790. Less critical, because a small WebGL buffer is cheaper to allocate than a larger WebGL texture, but still. There is no reason to reallocate the WebGL buffer storing the drawing geometry everytime. It also shouldn't depend on the parameters passed to draw(). Instead, just create once and for all a dummy buffer with the geometry of a dummy rectangle, and use uniforms to control its actual position and dimensions.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Also, only one buffer is needed instead of two, and it only needs 4 vertices with a TRIANGLE_STRIP, not 6.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Don't upload buffers on every draw

Jeff: please review from WebGL correctness perspective. This patch should not make any rendering difference; if it somehow can, that's a bug.

David: please review from Gallery perspective: this patch should not change anything in Gallery app besides how it uses WebGL.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 827786 [details] [diff] [review]
Don't upload buffers on every draw

Review of attachment 827786 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/gallery/js/ImageEditor.js
@@ +1347,5 @@
>  
>    // Create buffers to hold the input and output rectangles
> +  this.rectangleBuffer = gl.createBuffer();
> +  gl.bindBuffer(gl.ARRAY_BUFFER, this.rectangleBuffer);
> +  gl.disable(gl.CULL_FACE);

Why do you disable CULL_FACE here in the middle of buffer stuff?

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Oops, that was of course a remnant from debugging, sorry.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Don't upload buffers on every draw v2 (ready for landing)

Removed the unwanted gl.disable(gl.CULL_FACE);

Comment 6

[David Flanagan [:djf]]

Rolled this patch up with two others and resolved it in the parent bug.