Bug 98396

Created attachment 167071 [details] Patch

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.

Created attachment 167179 [details] Patch

(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.

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

Created attachment 167281 [details] Patch

(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.

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 :)

Created attachment 167420 [details] Patch

(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.

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.

Created attachment 167956 [details] Patch

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

Comment on attachment 167956 [details] Patch Clearing flags on attachment: 167956 Committed r131103: <http://trac.webkit.org/changeset/131103>

All reviewed patches have been landed. Closing bug.