Support for these non-separable blend modes is missing and shall be added.
Is there a reason to make this a separate patch?
(In reply to comment #1) > Is there a reason to make this a separate patch? I suggested splitting these up to make it easier to review. There's kind of a lot of math to verify on these :)
Created attachment 190604 [details] Patch
Please, disregard the following warning: Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:466: css_SetSatHelper is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] It is about css_SetSatHelper which is an inline GLSL function, not C++. I've already filed a bug for this: https://bugs.webkit.org/show_bug.cgi?id=111013
(In reply to comment #4) > Please, disregard the following warning: > > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:466: css_SetSatHelper is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] > > It is about css_SetSatHelper which is an inline GLSL function, not C++. I've already filed a bug for this: https://bugs.webkit.org/show_bug.cgi?id=111013 Why are you skipping the tests? You should build in a range of color values that is OK.
(In reply to comment #5) > Why are you skipping the tests? You should build in a range of color values that is OK. The reason is written in the comments. Anyhow, the results that Safari (WK1 and 2) returns are slightly off. Max filed this few weeks ago: https://bugs.webkit.org/show_bug.cgi?id=107487 Other than that, I guess we'll use this, if and when it'll be ready: https://bugs.webkit.org/show_bug.cgi?id=109356.
Attachment 190604 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-hue-expected.html', u'LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-hue.html', u'LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-saturation-expected.html', u'LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-saturation.html', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp']" exit_code: 1 Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:466: css_SetSatHelper is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > (In reply to comment #5) > > > Why are you skipping the tests? You should build in a range of color values that is OK. > > The reason is written in the comments. > Anyhow, the results that Safari (WK1 and 2) returns are slightly off. Max filed this few weeks ago: https://bugs.webkit.org/show_bug.cgi?id=107487 > > Other than that, I guess we'll use this, if and when it'll be ready: https://bugs.webkit.org/show_bug.cgi?id=109356. Can't you just build in a small range that is OK? I guess you would need to rewrite the test so it compares with the expected values.
(In reply to comment #8) > Can't you just build in a small range that is OK? I've indeed tried that before uploading the patch. This problem has occurred previously, see bug 104012 and bug 106226. The difference - though very slight (+/- 2/3 base 255 on one or two components) - is always beyond the acceptance threshold.
(In reply to comment #9) > (In reply to comment #8) > > > Can't you just build in a small range that is OK? > > I've indeed tried that before uploading the patch. > This problem has occurred previously, see bug 104012 and bug 106226. > > The difference - though very slight (+/- 2/3 base 255 on one or two components) - is always beyond the acceptance threshold. I think that is OK. they're probably happening because of integer math vs floating point
Comment on attachment 190604 [details] Patch Looks good! Just a couple of nits. View in context: https://bugs.webkit.org/attachment.cgi?id=190604&action=review > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:-420 > - notImplemented(); Nice. notImplemented() is gone, meaning we've implemented all the blend modes :) > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:475 > + mediump vec3 css_SetSat(mediump vec3 C, mediump float s) This looks reasonable. It's too bad we need need so many conditionals, but I can't think of a better way of sorting three values (and their references) that isn't more confusing. One approach I considered was computing the min, mid, and max indices with values 0, 1, or 2 corresponding to r, g, or b. Then the algorithm could look like: if (c[max] > c[min]) c[mid] = ... But you'd still need conditionals to figure out the indices, so not a huge improvement. Anyways, what you have is clear to me. > LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-hue.html:46 > + Backdrop with 0.5 blended with source with 0.5 alpha I think you missed the word "alpha" after "Backdrop with 0.5". > LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-hue.html:47 > + The underlaying proof of the above mentioned cases is the same: the colors are premultiplied underlaying -> underlying > LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-saturation.html:127 > + return (0.0.749, 0.549, 0.449) 0.0.749 -> 0.749
(In reply to comment #11) > (From update of attachment 190604 [details]) > > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:475 > > + mediump vec3 css_SetSat(mediump vec3 C, mediump float s) > > This looks reasonable. It's too bad we need need so many conditionals, but I can't think of a better way of sorting three values (and their references) that isn't more confusing. One approach I considered was computing the min, mid, and max indices with values 0, 1, or 2 corresponding to r, g, or b. Then the algorithm could look like: > > if (c[max] > c[min]) > c[mid] = ... > I've looked at 5 implementations of this, and they all have this mess of nested 'if' statements. It seems that's the most efficient way to do it.
(In reply to comment #11) > Nice. notImplemented() is gone, meaning we've implemented all the blend modes :) Awesoooome!:) > I think you missed the word "alpha" after "Backdrop with 0.5". Indeed, in both of them. > underlaying -> underlying Ditto. > 0.0.749 -> 0.749 Great catch. Thank you for that!
Created attachment 191059 [details] Patch
Attachment 191059 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-hue-expected.html', u'LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-hue.html', u'LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-saturation-expected.html', u'LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-saturation.html', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp']" exit_code: 1 Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:466: css_SetSatHelper is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 191059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191059&action=review > Source/WebCore/ChangeLog:10 > + - css_Sat(C): returns the saturation for the color C > + - css_SetSat(C, s): sets the saturation s on the color C > + - css_SetSatHelper(Cmin, Cmid, Cmax, s): helper function for css_SetSat Why are you using "Sat" rather than "Saturation"? I guess we already did "Lum" so it is consistent.
(In reply to comment #16) > > Source/WebCore/ChangeLog:10 > > + - css_Sat(C): returns the saturation for the color C > > + - css_SetSat(C, s): sets the saturation s on the color C > > + - css_SetSatHelper(Cmin, Cmid, Cmax, s): helper function for css_SetSat > Why are you using "Sat" rather than "Saturation"? I guess we already did "Lum" so it is consistent. I chose to use that naming because it's exactly the same currently used on the specs: https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#blendingnonseparable Thank you! I'll land this manually later given the check-webkit-style false positive.
Comment on attachment 191059 [details] Patch Clearing flags on attachment: 191059 Committed r145114: <http://trac.webkit.org/changeset/145114>
All reviewed patches have been landed. Closing bug.