Bug 98396 – [CSS Shaders] Make custom filters use a premultiplied buffer.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

There are three benefits when FECustomFilter uses a premultiplied buffer.
1. FilterEffect::asImageBuffer is faster.
2. The fragment shader is faster as reducing a division by alpha.
3. In the future when AC implementaions uses FECustomFilter, the implementations do not need to convert unmultiplied buffer to premultiplied buffer.

[Dongseong Hwang]

Created attachment 167071 [details]
Patch

[Max Vujovic]

Comment on attachment 167071 [details]
Patch

Nice patch!

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

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:227
> +    Uint8ClampedArray* dstPixelArray = createPremultipliedImageResult();

When an author is not using the CSS mix function in his or her shader, the author is writing directly to gl_FragColor. For example:

CSS:
-webkit-filter: custom(url(shader.vs) url(shader.fs));

shader.fs:
void main()
{
    gl_FragColor = vec4(0.5);
}

In this case, we can't guarantee the author has premultiplied the gl_FragColor output. Thus, we have to add a condition:

Uint8ClampedArray* dstPixelArray = (m_validatedProgram->programInfo().programType == PROGRAM_TYPE_NO_ELEMENT_TEXTURE ? createUnmultipliedImageResult() : createPremultipliedImageResult());

To be clear, PROGRAM_TYPE_NO_ELEMENT_TEXTURE means the author is not using the CSS mix function and is writing directly to gl_FragColor. PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE means the the author is using the CSS mix function and writing to css_MixColor. 

> LayoutTests/css3/filters/custom/custom-filter-composite-source-over-expected.html:4
> +    <title>Tests the source-over composite operator between source and destination color with alpha.</title>

Nice test! I think this should say:
"Tests the source-atop composite operator when source alpha is a fractional value."

I would also rename the test to something like:
custom-filter-composite-fractional-source-alpha.html

> LayoutTests/css3/filters/custom/custom-filter-composite-source-over.html:42
> +        -webkit-filter: custom(none mix(url('../resources/mix-color.fs') normal source-over), mix_color 0.0 0.8 0.5 0.6);

If you land this before bug 97859, you'll want to use source-atop, since we don't have source-over yet.

[Dongseong Hwang]

Created attachment 167179 [details]
Patch

[Dongseong Hwang]

(In reply to comment #2)
> In this case, we can't guarantee the author has premultiplied the gl_FragColor output. Thus, we have to add a condition:
> 
> Uint8ClampedArray* dstPixelArray = (m_validatedProgram->programInfo().programType == PROGRAM_TYPE_NO_ELEMENT_TEXTURE ? createUnmultipliedImageResult() : createPremultipliedImageResult());

Thanks for your instruction! I completely missed this. I used m_validatedProgram->programInfo().mixSettings().enabled like the constructor of CustomFilterValidatedProgram.

I hope dstPixelArray will be premultiplied even if an author is not using the CSS mix function in the future when AC implementations use FECustomFilter.

> Nice test! I think this should say:
> "Tests the source-atop composite operator when source alpha is a fractional value."
> 
> I would also rename the test to something like:
> custom-filter-composite-fractional-source-alpha.html

Thanks for proofreading. The changed words are so much natural!

> If you land this before bug 97859, you'll want to use source-atop, since we don't have source-over yet.

Sounds good! I'll do that.

[WebKit Review Bot]

Comment on attachment 167179 [details]
Patch

Attachment 167179 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14178274

New failing tests:
css3/filters/custom/custom-filter-composite-fractional-source-alpha.html

[Dongseong Hwang]

Created attachment 167281 [details]
Patch

[Dongseong Hwang]

(In reply to comment #6)
> Created an attachment (id=167281) [details]
> Patch

Make cr-linux pass a new test as fixing an opaque background color to offset the error in the color calculation with the 8-bit values.

[Max Vujovic]

Comment on attachment 167281 [details]
Patch

Thanks for the updates, Huang. This looks good to me- just minor ChangeLog nits if you want to fix them. You should probably ask Dean to review this formally now, since I'm only a committer :)

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

> Source/WebCore/ChangeLog:8
> +        Currently, a glsl css_Composite function returns a premultiplied color, so

Capitalization nit: GLSL

> Source/WebCore/ChangeLog:14
> +        2. In the future when AC implementaions use FECustomFilter, they do not need to

Spelling nit: implementations

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:227
> +    Uint8ClampedArray* dstPixelArray = m_validatedProgram->programInfo().mixSettings().enabled ? createPremultipliedImageResult() : createUnmultipliedImageResult();

In the near future, we will be removing mixSettings.enabled.

So instead of:
(1) m_validatedProgram->programInfo().mixSettings().enabled

We will write:
(2) m_validatedProgram->programInfo().programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE

You can write (2) right now because until we get rid of mixSettings.enabled, programType() does this:
CustomFilterProgramType programType() const { return m_mixSettings.enabled ? PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE : PROGRAM_TYPE_NO_ELEMENT_TEXTURE; }

Michelangelo is working on removing mixSettings.enabled in bug 96448, so it's not your responsibility to change (1) to (2) now, but you can if you want :)

[Dongseong Hwang]

Created attachment 167420 [details]
Patch

[Dongseong Hwang]

(In reply to comment #8)
> (From update of attachment 167281 [details])
> Thanks for the updates, Huang. This looks good to me- just minor ChangeLog nits if you want to fix them. You should probably ask Dean to review this formally now, since I'm only a committer :)

Thanks for the review. I'll ask Dean to review via IRC. :)

> Capitalization nit: GLSL
> Spelling nit: implementations

Done.

> We will write:
> (2) m_validatedProgram->programInfo().programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE

Done.

[Dean Jackson]

Comment on attachment 167420 [details]
Patch

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

Looks great. Just one minor change and then it can be committed.

> Source/WebCore/ChangeLog:14
> +        2. In the future when AC implementations use FECustomFilter, they do not need to

Nit: We should expand "AC" to Accelerated Compositing here.

[Dongseong Hwang]

Created attachment 167956 [details]
Patch

[Dongseong Hwang]

Thanks for your review.

Could you check r+ c+ one more time? I'm not commiter, so I always depend on commit-queue-bot.

[WebKit Review Bot]

Comment on attachment 167956 [details]
Patch

Clearing flags on attachment: 167956

Committed r131103: <http://trac.webkit.org/changeset/131103>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.