935353 - Gallery: consider removing useless divisions in fragment shader

Status: RESOLVED FIXED

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

Description

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

The Gallery's fragment shader currently computes this for every pixel:

  (clamped_color.xyz - minValues) / (maxValues - minValues)

Here maxValues and minValues are uniforms (constants). I'm not sure that all of the shader compilers in the world know how to optimize that.

Jeff, do you think that this is consistently optimized away by all shader compilers?

Comment 1

[Kelsey Gilbert [:jgilbert]]

It's not safe to pretend uniforms are constants, so it's not safe to assume they'll be folded together.
Do this in the vertex shader, or outside the shader program altogether.

Comment 2

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

Attachment: avoid useless divisions in fragment shader by storing quotients in unused matrix column

We were already using a 3x3 matrix to pass the min and max clamp values for RGB colors; these were respectively the first and second columns of the matrix. The third column of the matrix was unused in the fragment shader. So, while I don't love the idea of packing random stuff in a matrix, since we were already doing that and had just enough unused space in that matrix to store the quotients that I needed to store, this seemed like the path of least resistance.

r? jgilbert for WebGL details, djf for whether it's an OK code change to make to the Gallery code.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 832493 [details] [diff] [review]
avoid useless divisions in fragment shader by storing quotients in unused matrix column

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

I hate sneaking new variables into existing 'unused' slots. Is there a pressing reason we can't add a variable specifically for this, or a reason we don't wan to do this? I don't even like how min and max are stuffed into a matrix in the existing code.

Comment 4

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

Yeah, I agree that's bad. I'll whip up patches fixing the existing code to stop using a uniform matrix as a block device.

Comment 5

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

Attachment: avoid useless divisions in fragment shader and stop using a mat3 as a block device to store vec3's

Comment 6

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

Attachment: avoid useless divisions in fragment shader and stop using a mat3 as a block device to store vec3's

rebased (had bitrotted)

Comment 7

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

Attachment: avoid useless divisions in fragment shader and stop using a mat3 as a block device to store vec3's

Properly rebased

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8344837 [details] [diff] [review]
avoid useless divisions in fragment shader and stop using a mat3 as a block device to store vec3's

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

r+ if you fix the two nits.

::: apps/gallery/js/ImageEditor.js
@@ +1466,5 @@
>  
>    // set rgb max/min values for auto Enhancing
> +  var minMaxValuesMatrix = options.rgbMinMaxValues ||
> +                           ImageProcessor.default_enhancement;
> +  gl.uniformMatrix3fv(this.rgbMinMaxValuesAddress, false, minMaxValuesMatrix);

I thought you got rid of this variable.  Don't you need to delete this line?

@@ +1523,5 @@
>    // Otherwise take the image color, apply color and gamma correction and
>    // the color manipulation matrix.
>    '  vec4 original_color = texture2D(image, src_position);\n' +
> +  '  vec3 clamped_color = clamp(original_color.xyz, rgb_min, rgb_max);\n' +
> +  '  vec4 corrected_color = vec4((clamped_color.xyz - rgb_min) * rgb_one_over_max_minus_min, original_color.a);\n' +

This line runs over 80 characters, and our gjslint linter will fail on it.

Comment 9

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

Attachment: (Fixed review comments) avoid useless divisions in fragment shader and stop using a mat3 as a block device to store vec3's

Comment 10

[David Flanagan [:djf]]

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