Bug 95914 – [CSS Shaders] u_textureSize uniform should be set to the size of the texture.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

There are some uniforms that are specified in the spec, but are not set in the implementation.

[Michelangelo De Simone]

Created attachment 163717 [details]
Patch

[Alexandru Chiculita]

View in context: https://bugs.webkit.org/attachment.cgi?id=163717&action=review

Looks good!

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:357
> +        m_context->uniform2f(m_compiledProgram->tileSizeLocation(), 1.0 / m_meshColumns, 1.0 / m_meshRows);

I know that m_meshColumns > 0 and m_meshRows > 0 (that check is in CSSParser.cpp), but maybe we could assert about it here.

> LayoutTests/css3/filters/custom/custom-filter-u-mesh-box.html:32
> +            As of now u_meshBox always returns (-0.5, -0.5, 1, 1); this shall change

Do you want to reference to that bug?

> LayoutTests/css3/filters/resources/u-mesh-box.fs:1
> +precision mediump float;

Add a comment at the top saying what this shader does.

> LayoutTests/css3/filters/resources/u-mesh-box.fs:7
> +    return a > b - epsilon && a < b + epsilon;

Add some ( ) to make it easier to read.

The same for the other shaders.

[Max Vujovic]

Comment on attachment 163717 [details]
Patch

Nice! I had one comment in addition to Alex's:

View in context: https://bugs.webkit.org/attachment.cgi?id=163717&action=review

> LayoutTests/css3/filters/resources/u-mesh-box.fs:12
> +    gl_FragColor = (areFloatsEqual(u_meshBox.x, -0.5) && areFloatsEqual(u_meshBox.y, -0.5) && areFloatsEqual(u_meshBox.z, 1.0) && areFloatsEqual(u_meshBox.w, 1.0)) ? vec4(0.0, 1.0, 0.0, 1.0) : vec4(1.0, 0.0, 0.0, 1.0);

Adding another function areVectorsEqual(vec4 a, vec4 b) that calls areFloatEqual could be more readable, like so:

gl_FragColor = areVectorsEqual(u_meshBox, vec4(-0.5, -0.5, 1.0, 1.0)) ? vec4(0.0, 1.0, 0.0, 1.0) : vec4(1.0, 0.0, 0.0, 1.0);

> LayoutTests/css3/filters/resources/u-mesh-size.fs:12
> +    gl_FragColor = (areFloatsEqual(u_meshSize.x, 20.0) && areFloatsEqual(u_meshSize.y, 10.0)) ? vec4(0.0, 1.0, 0.0, 1.0) : vec4(1.0, 0.0, 0.0, 1.0);

Ditto, but with vec2 instead of vec4.

> LayoutTests/css3/filters/resources/u-texture-size.fs:12
> +    gl_FragColor = (areFloatsEqual(u_textureSize.x, 100.0) && areFloatsEqual(u_textureSize.y, 100.0)) ? vec4(0.0, 1.0, 0.0, 1.0) : vec4(1.0, 0.0, 0.0, 1.0);

Ditto.

> LayoutTests/css3/filters/resources/u-tile-size.fs:12
> +    gl_FragColor = (areFloatsEqual(u_tileSize.x, 0.05) && areFloatsEqual(u_tileSize.y, 0.10)) ? vec4(0.0, 1.0, 0.0, 1.0) : vec4(1.0, 0.0, 0.0, 1.0);

Ditto.

[Michelangelo De Simone]

Created attachment 163733 [details]
Patch

[Dean Jackson]

Comment on attachment 163733 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163733&action=review

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:357
> +    if (m_compiledProgram->meshSizeLocation() != -1)
> +        m_context->uniform2f(m_compiledProgram->meshSizeLocation(), m_meshColumns, m_meshRows);
> +
> +    ASSERT(m_meshColumns);
> +    ASSERT(m_meshRows);

Might as well put the ASSERT before this first binding.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:365
> +    if (m_compiledProgram->meshBoxLocation() != -1)
> +        // FIXME: This will change when filter margins will be implemented,
> +        // see https://bugs.webkit.org/show_bug.cgi?id=71400
> +        m_context->uniform4f(m_compiledProgram->meshBoxLocation(), -0.5, -0.5, 1.0, 1.0);

Please enclose in {} if you're putting comments before the statement, or put the comment before the if.

> LayoutTests/css3/filters/resources/u-mesh-box.fs:1
> +// Test whether u_meshBox is correctly set; change fragment color to green if yes.

Could you rename these files (u-mesh-box.fs u-mesh-size, u-texture-size, u-tile-size) to reflect the parameters they expect? I think people often look through layout tests to find examples of code, and might interpret this to be such. In reality it is u-mesh-box-is-unit-square or something. Do you agree?

[Michelangelo De Simone]

Created attachment 164237 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164237 [details]
Patch for landing

Clearing flags on attachment: 164237

Committed r128666: <http://trac.webkit.org/changeset/128666>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

Comment on attachment 164237 [details]
Patch for landing

I wonder why you did rename u-mesh-box-is-unit-square.fs but not LayoutTests/css3/filters/resources/u-tile-size.fs and the other files as suggested. As far as I understand your code, you normalize tileSizeLocation. Shouldn't it be renamed?

[Michelangelo De Simone]

(In reply to comment #9)
> I wonder why you did rename u-mesh-box-is-unit-square.fs but not LayoutTests/css3/filters/resources/u-tile-size.fs and the other files as suggested. As far as I understand your code, you normalize tileSizeLocation. Shouldn't it be renamed?

Dean and I agreed on IRC that u-tile-size, u-texture-size and u-mesh-size were self explanatory enough to be left as they are.