Bug 109464 – [CSS Shaders] Implement hue and saturation non-separable blend modes

• Component: CSS
• Status: RESOLVED FIXED

[Michelangelo De Simone]

Support for these non-separable blend modes is missing and shall be added.

[Rik Cabanier]

Is there a reason to make this a separate patch?

[Max Vujovic]

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

[Michelangelo De Simone]

Created attachment 190604 [details]
Patch

[Michelangelo De Simone]

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

[Rik Cabanier]

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

[Michelangelo De Simone]

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

[WebKit Review Bot]

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.

[Rik Cabanier]

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

[Michelangelo De Simone]

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

[Rik Cabanier]

(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

[Max Vujovic]

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

[Rik Cabanier]

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

[Michelangelo De Simone]

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

[Michelangelo De Simone]

Created attachment 191059 [details]
Patch

[WebKit Review Bot]

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.

[Dean Jackson]

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.

[Michelangelo De Simone]

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

[Michelangelo De Simone]

Comment on attachment 191059 [details]
Patch

Clearing flags on attachment: 191059

Committed r145114: <http://trac.webkit.org/changeset/145114>

[Michelangelo De Simone]

All reviewed patches have been landed.  Closing bug.