Gallery: consider removing useless divisions in fragment shader

RESOLVED FIXED

Status

Firefox OS
Gaia::Gallery
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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?
(Assignee)

Updated

5 years ago
Flags: needinfo?(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.
Flags: needinfo?(jgilbert)
(Assignee)

Comment 2

5 years ago
Created attachment 832493 [details] [diff] [review]
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.
Attachment #832493 - Flags: review?(jgilbert)
Attachment #832493 - Flags: review?(dflanagan)
(Assignee)

Updated

5 years ago
Attachment #832493 - Attachment description: avoid-div-fs-gallery → avoid useless divisions in fragment shader by storing quotients in unused matrix column
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.
Attachment #832493 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 8337959 [details] [diff] [review]
avoid useless divisions in fragment shader and stop using a mat3 as a block device to store vec3's
Attachment #832493 - Attachment is obsolete: true
Attachment #832493 - Flags: review?(dflanagan)
Attachment #8337959 - Flags: review?(jgilbert)
Attachment #8337959 - Flags: review?(dflanagan)
Attachment #8337959 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 8344409 [details] [diff] [review]
avoid useless divisions in fragment shader and stop using a mat3 as a block device to store vec3's

rebased (had bitrotted)
Attachment #8337959 - Attachment is obsolete: true
Attachment #8337959 - Flags: review?(dflanagan)
Attachment #8344409 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #8344409 - Flags: review?(dflanagan)
(Assignee)

Comment 7

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

Properly rebased
Attachment #8344409 - Attachment is obsolete: true
Attachment #8344409 - Flags: review?(dflanagan)
Attachment #8344837 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #8344837 - Flags: review?(dflanagan)
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.
Attachment #8344837 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 8344862 [details] [diff] [review]
(Fixed review comments) avoid useless divisions in fragment shader and stop using a mat3 as a block device to store vec3's
Attachment #8344862 - Flags: review+
Rolled this patch up with others and resolved it in the parent bug.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Assignee: nobody → bjacob
You need to log in before you can comment on or make changes to this bug.