934790 - Gallery: ImageProcessor.prototype.draw should not call texImage2D everytime

Status: RESOLVED FIXED

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

Description

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

ImageProcessor.prototype.draw is currently calling texImage2D everytime.

Since that is the only place in the gallery/ directory that any WebGL draw operation is made, that has to mean that as I drag sliders in the gallery's preview UI, the texture is re-uploaded everytime a new frame is about to be rendered.

You want to avoid doing this. There are various reasons why uploading textures on every WebGL draw operation prevents high performance. These reasons include: 1) preventing multiple WebGL draw operations from running in parallel; 2) forcing WebGL draw operations to wait for texture uploads to complete; 3) using more memory bandwidth than needed.

Instead, when I'm using the gallery editor, the texture should only be uploaded with texImage2D once, when I'm entering the editor.

For example, when I drag e.g. the contrast slider, that should only result in some WebGL uniform setter call (e.g. gl.uniform1f) changing the corresponding uniform value, and then calling WebGL drawArrays again to update the drawing. WebGL uniform setters are cheap, but texture uploads (texImage2D) are slow.

Btw this and some more high-level GL tips are discussed in this post,
http://schoolofweb.org/blog/the-concepts-of-webgl/

Comment 1

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

Oops, awesomebar fail, I meant this link:
https://hacks.mozilla.org/2013/04/the-concepts-of-webgl/

Comment 2

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

Attachment: Don't upload the texture on every draw

Jeff: please review this from the perspective of WebGL; what this intends to do is avoid calling texImage2D over and over again with an image that we already uploaded to this texture. Note that a constraint here was that Gallery code is calling this draw() function already before the image is actually loaded (in the preview case).

David: please review this from the perspective of Gallery logic. Does this patch risk skipping a texture upload when it shouldn't, i.e. when the image actually changed.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 827785 [details] [diff] [review]
Don't upload the texture on every draw

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

::: apps/gallery/js/ImageEditor.js
@@ +1434,5 @@
>    gl.enableVertexAttribArray(this.destPixelAddress);
>    gl.vertexAttribPointer(this.destPixelAddress, 2, gl.FLOAT, false, 0, 0);
> +  // Load the image into the texture
> +  if (image != this.lastImage && image.loaded) {
> +    gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, image);

Doesn't this just skip the upload if it's not loaded yet?

Comment 4

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

It does; all what that did was throw an exception anyway. This is getting continuously called over and over again; eventually the image is loaded. What I really wanted to avoid was setting this.lastImage = image; before it actually got uploaded to the texture.

Comment 5

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

Note that what I would really like to do would be completely take out the texImage2D call from the draw() function, but as it currently is, this draw() function takes an image argument. Ideally, we'd change draw() to not take an image argument and require the texture to have been set beforehand. Or, if really there is a usage pattern here (I don't think that there is) that requires switching images frequently, draw() could take a WebGL texture object that would have been previously uploaded, and just re-bind it.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 827785 [details] [diff] [review]
Don't upload the texture on every draw

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

::: apps/gallery/js/ImageEditor.js
@@ +1434,5 @@
>    gl.enableVertexAttribArray(this.destPixelAddress);
>    gl.vertexAttribPointer(this.destPixelAddress, 2, gl.FLOAT, false, 0, 0);
> +  // Load the image into the texture
> +  if (image != this.lastImage && image.loaded) {
> +    gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, image);

Please leave rationale for skipping upload and using the incomplete texture.
(Really just say 'just use the incomplete texture until we can upload it properly')

Comment 7

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

Attachment: Don't upload the texture on every draw (carries r=jgilbert)

Rebased (had bitrotted).

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8344408 [details] [diff] [review]
Don't upload the texture on every draw (carries r=jgilbert)

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

r- because I think the .loaded tests breaks the code that saves the edited image.

Also I think the the image !== this.lastImage test doesn't help much because the other code in this file keeps a single preview image and just reloads its content.

Maybe change ImageProcessor.prototype.draw() to add a new second argument named uploadNeeded or something.  Then, the tile-based file saving code can pass true always.

And the onscreen preview editing code would have its own boolean for upload needed that gets set in resetPreview().

::: apps/gallery/js/ImageEditor.js
@@ +531,4 @@
>  
>    function thumbnailReady(thumbnail) {
>      self.preview.src = URL.createObjectURL(thumbnail);
> +    self.preview.loaded = false;

If we're going to do this, I think this line should be in resetPreview() rather than here.

@@ +535,2 @@
>      self.preview.onload = function() {
> +      self.preview.loaded = true;

Is this even necessary?  Don't we call draw() from the callback below?  Is it ever actually possible for draw() to be called without a loaded image?

@@ +1471,5 @@
>    makeRectangle(this.destinationRectangle, dx, dy, dw, dh);
>    gl.enableVertexAttribArray(this.destPixelAddress);
>    gl.vertexAttribPointer(this.destPixelAddress, 2, gl.FLOAT, false, 0, 0);
> +  // Load the image into the texture
> +  if (image != this.lastImage && image.loaded) {

The test for image.loaded breaks the call to draw() in getFullSizeBlob() (line 716). There, I pass a CanvasPixelArray instead of an image.

(I think I was working on that patch at the same time you created this one.)

And I think there is another problem: the image != this.lastImage test won't do much good since we always use a single preview image and just reload it all the time.  We need some other flag to indicate "this image needs to be uploaded again".

Comment 9

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

Ah, yes, I got conflicts when rebasing and didn't remember the details, so I rebased this wrong. Thanks for catching this.

Comment 10

[David Flanagan [:djf]]

Rolled this patch up with two related patches, and resolved in the parent bug.