Preventing division by 0 in non-separable blend mode shaders.

Preventing division by 0 in non-separable blend mode shaders.

In the software path, the same issue has been fixed some time ago:
https://codereview.chromium.org/114173002

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/ace7f4276997235abe559b70620d9f89737d2518

[rosca, 2014-10-21 11:32:35]

rosca@adobe.com changed reviewers:
+ reed@google.com

[reed1, 2014-10-21 13:23:21]

reed@google.com changed reviewers:
+ egdaniel@google.com

[egdaniel, 2014-10-21 14:24:39]

The divide by zero check looks good. Do you know of any tests or gm's that we
have that hit this case? If not can you add one so that we can make sure this is
covered in the future?

Also do you have any insight into the note I added in the code. I'm not fully
aware of how these calculations should be done and what is the correct formula.

https://codereview.chromium.org/666043003/diff/1/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/666043003/diff/1/src/core/SkXfermode.cpp#newc...
src/core/SkXfermode.cpp:1098: setLumBody.printf("\tfloat diff = %s(lumColor -
hueSat);\n", getFunction.c_str());
Side note: This line looks like it does diff = Lum(lumColor - hueSat). The
software does this same calculation at line 490. However the software seems to
be doing diff = lumColor - Lum(hueSat). I haven't looked back at all the callers
to see what form all the inputs are in, but seems like this may produce
different things (but our gm's show no difference so maybe it's right). Do you
have any idea if these are doing what we expect?

[egdaniel, 2014-10-21 14:36:26]

https://codereview.chromium.org/666043003/diff/1/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/666043003/diff/1/src/core/SkXfermode.cpp#newc...
src/core/SkXfermode.cpp:1098: setLumBody.printf("\tfloat diff = %s(lumColor -
hueSat);\n", getFunction.c_str());
On 2014/10/21 14:24:39, egdaniel wrote:
> Side note: This line looks like it does diff = Lum(lumColor - hueSat). The
> software does this same calculation at line 490. However the software seems to
> be doing diff = lumColor - Lum(hueSat). I haven't looked back at all the
callers
> to see what form all the inputs are in, but seems like this may produce
> different things (but our gm's show no difference so maybe it's right). Do you
> have any idea if these are doing what we expect?

Disregard this. It seems like the software/gpu just do all the various
multiplies at different stages in the call stack, but all end up equivalent in
the end.

[rosca, 2014-10-21 14:42:48]

On 2014/10/21 14:24:39, egdaniel wrote:
> The divide by zero check looks good. Do you know of any tests or gm's that we
> have that hit this case? If not can you add one so that we can make sure this
is
> covered in the future?

Thanks for the review.
I've reproduced this by using the program's code outside skia and it happens on
anti-aliased pixels getting various alpha values.
I will try to reproduce it within skia and add a test to gm.

[rosca, 2014-11-05 12:16:02]

On 2014/10/21 14:42:48, rosca wrote:
> On 2014/10/21 14:24:39, egdaniel wrote:
> > The divide by zero check looks good. Do you know of any tests or gm's that
we
> > have that hit this case? If not can you add one so that we can make sure
this
> is
> > covered in the future?
> 
> Thanks for the review.
> I've reproduced this by using the program's code outside skia and it happens
on
> anti-aliased pixels getting various alpha values.
> I will try to reproduce it within skia and add a test to gm.

Please review this patch. I found one combination of colors the issue can be
reproduced reproduced with and I added a gm test.

[reed1, 2014-11-05 19:30:35]

can this be tested via a unittest instead of a GM? I ask because it seems to be
a functional fix, and not a visual one.

[egdaniel, 2014-11-07 19:01:40]

On 2014/11/05 19:30:35, reed1 wrote:
> can this be tested via a unittest instead of a GM? I ask because it seems to
be
> a functional fix, and not a visual one.

So I don't think we can do this as a unittest since we currently have no real
way to unittest our shader code (without actually drawing something, aka gm).

When I run this gm locally only 1 of the 4 xfermodes seems to draw something. I
believe it is color that draws something and hue, saturation, and luminosity
draw nothing. Is this the expected behavior? Would it be sufficient to just test
one of the modes as long as it uses the AddLumFunction? If this is the case and
we are only drawing one thing, which may not even show any pixels, maybe it
makes sense to add this one test to a previous xfermode gm? Also regardless if
there are draws that are expected to not display anything on the screen this
should be commented in the gm. Also does this gm also trigger the software check
that you linked to in your description?

Mike thoughts?

[rosca, 2014-11-07 19:19:23]

On 2014/11/07 19:01:40, egdaniel wrote:
> On 2014/11/05 19:30:35, reed1 wrote:
> > can this be tested via a unittest instead of a GM? I ask because it seems to
> be
> > a functional fix, and not a visual one.
> 
> So I don't think we can do this as a unittest since we currently have no real
> way to unittest our shader code (without actually drawing something, aka gm).
> 
> When I run this gm locally only 1 of the 4 xfermodes seems to draw something.
I
> believe it is color that draws something and hue, saturation, and luminosity
> draw nothing. Is this the expected behavior? Would it be sufficient to just
test
> one of the modes as long as it uses the AddLumFunction? If this is the case
and
> we are only drawing one thing, which may not even show any pixels, maybe it
> makes sense to add this one test to a previous xfermode gm? Also regardless if
> there are draws that are expected to not display anything on the screen this
> should be commented in the gm. Also does this gm also trigger the software
check
> that you linked to in your description?
> 
> Mike thoughts?
Thanks for your comment.
Yes, this is the expected behavior, this is the result they give for blending
the transparent gray with a white background.
I couldn't find a better combination of colors that crashes.
I think it's better to add this as a special case to one of the existing
xfermode gm tests.
I will do this and I will make sure that the test would touch the software issue
I referred.

[rosca, 2014-11-10 15:41:37]

On 2014/11/07 19:01:40, egdaniel wrote:
> On 2014/11/05 19:30:35, reed1 wrote:
> > can this be tested via a unittest instead of a GM? I ask because it seems to
> be
> > a functional fix, and not a visual one.
> 
> So I don't think we can do this as a unittest since we currently have no real
> way to unittest our shader code (without actually drawing something, aka gm).
> 
> When I run this gm locally only 1 of the 4 xfermodes seems to draw something.
I
> believe it is color that draws something and hue, saturation, and luminosity
> draw nothing. Is this the expected behavior? Would it be sufficient to just
test
> one of the modes as long as it uses the AddLumFunction? If this is the case
and
> we are only drawing one thing, which may not even show any pixels, maybe it
> makes sense to add this one test to a previous xfermode gm? Also regardless if
> there are draws that are expected to not display anything on the screen this
> should be commented in the gm. Also does this gm also trigger the software
check
> that you linked to in your description?
> 
I added a special case to the existing mixed_xfermodes test.
Unfortunately I couldn't get the similar issue in software mode crashing,
(https://codereview.chromium.org/114173002), neither with this test, nor with
lots
of randomly generated colors and shapes.

[egdaniel, 2014-11-14 16:08:21]

https://codereview.chromium.org/666043003/diff/40001/gm/mixedxfermodes.cpp
File gm/mixedxfermodes.cpp (right):

https://codereview.chromium.org/666043003/diff/40001/gm/mixedxfermodes.cpp#ne...
gm/mixedxfermodes.cpp:47: switch (type) {
You need to have a default case or else there will be errors for kNumShapeTypes
not being handled by the switch in certain compilers

https://codereview.chromium.org/666043003/diff/40001/gm/mixedxfermodes.cpp#ne...
gm/mixedxfermodes.cpp:121: if (i == kNumShapes - 1) {
Since what you want to test ends up drawing nothing (or I believe that's what it
does from your previous patch), it would probably be best to not change any of
the current draws in this loop, and make a function like
draw_lum_zero_whatever_mode(canvas) that you call after the for loop (obviously
you need a better name than that). Give a good comment on the function as to why
it is there, its function, and that it should not actually do any draws.

The main advantage of this is that we can easily document the specific test case
we are looking at, and it shouldn't alter the gm (assuming nothing draws) so you
will not need to rebase afterwards.

[rosca, 2014-11-17 16:09:31]

Thanks. I uploaded a new patch.

https://codereview.chromium.org/666043003/diff/40001/gm/mixedxfermodes.cpp
File gm/mixedxfermodes.cpp (right):

https://codereview.chromium.org/666043003/diff/40001/gm/mixedxfermodes.cpp#ne...
gm/mixedxfermodes.cpp:47: switch (type) {
On 2014/11/14 16:08:21, egdaniel wrote:
> You need to have a default case or else there will be errors for
kNumShapeTypes
> not being handled by the switch in certain compilers

Done.

https://codereview.chromium.org/666043003/diff/40001/gm/mixedxfermodes.cpp#ne...
gm/mixedxfermodes.cpp:121: if (i == kNumShapes - 1) {
On 2014/11/14 16:08:21, egdaniel wrote:
> Since what you want to test ends up drawing nothing (or I believe that's what
it
> does from your previous patch), it would probably be best to not change any of
> the current draws in this loop, and make a function like
> draw_lum_zero_whatever_mode(canvas) that you call after the for loop
(obviously
> you need a better name than that). Give a good comment on the function as to
why
> it is there, its function, and that it should not actually do any draws.
> 
> The main advantage of this is that we can easily document the specific test
case
> we are looking at, and it shouldn't alter the gm (assuming nothing draws) so
you
> will not need to rebase afterwards.

Done.

[egdaniel, 2014-11-18 14:57:16]

Overall this is looks much better. One issue was that when I try to test your cl
with and without the divide by zero fix, I end up getting no differences in the
image created. Can you confirm that no including you fix causes the gm to
actually draw differently?

It could just be an issue of different video cards/drivers. What platform and
card are you using?

https://codereview.chromium.org/666043003/diff/60001/gm/mixedxfermodes.cpp
File gm/mixedxfermodes.cpp (right):

https://codereview.chromium.org/666043003/diff/60001/gm/mixedxfermodes.cpp#ne...
gm/mixedxfermodes.cpp:142: * being fixed with
https://codereview.chromium.org/666043003/.
No need to put link to cl in the comment. The comment itself should describe the
bug enough and if need be the cl can always be found through history/blame.

[rosca, 2014-11-18 15:09:53]

On 2014/11/18 14:57:16, egdaniel wrote:
> Overall this is looks much better. One issue was that when I try to test your
cl
> with and without the divide by zero fix, I end up getting no differences in
the
> image created. Can you confirm that no including you fix causes the gm to
> actually draw differently?
> 
> It could just be an issue of different video cards/drivers. What platform and
> card are you using?

I have a mac pro with Radeon HD 5700 running windows. I ran it without the fix
and the test fails.
Basically, I ran 'out\Release\gm -w gm/my_dir --match mixed_xfermodes' with the
fix, this generates
a set of pngs, remove the fix, build and run 'out\Release\gm -r gm/my_dir
--match mixed_xfermodes',
and the test fails:
---- mixed_xfermodes_gpu.png: 143 (of 505600) differing pixels, max per-channel
mismatch R=255 G=255 B=255 A=0
Ran 4 tests: NoGpuContext=0 IntentionallySkipped=1 RenderModeMismatch=0
GeneratePdfFailed=0 ExpectationsMismatch=1 MissingExpectations=0
WritingReferenceImage=0
[*] 0 NoGpuContext:
[ ] 1 IntentionallySkipped
[*] 0 RenderModeMismatch:
[*] 0 GeneratePdfFailed:
[*] 1 ExpectationsMismatch: mixed_xfermodes_gpu
[ ] 0 MissingExpectations
[*] 0 WritingReferenceImage:

Am I missing something?

[egdaniel, 2014-11-20 14:34:15]

Sounds good, yeah that should be a valid way to test the code. Just fix up the
nits and it should be set to go.

https://codereview.chromium.org/666043003/diff/60001/gm/mixedxfermodes.cpp
File gm/mixedxfermodes.cpp (right):

https://codereview.chromium.org/666043003/diff/60001/gm/mixedxfermodes.cpp#ne...
gm/mixedxfermodes.cpp:48: case kShapeTypeCircle:
we tend to indent the "case" inside the switch statements.

[rosca, 2014-11-20 14:52:10]

Thanks!

https://codereview.chromium.org/666043003/diff/60001/gm/mixedxfermodes.cpp
File gm/mixedxfermodes.cpp (right):

https://codereview.chromium.org/666043003/diff/60001/gm/mixedxfermodes.cpp#ne...
gm/mixedxfermodes.cpp:48: case kShapeTypeCircle:
On 2014/11/20 14:34:15, egdaniel wrote:
> we tend to indent the "case" inside the switch statements.

Done.

https://codereview.chromium.org/666043003/diff/60001/gm/mixedxfermodes.cpp#ne...
gm/mixedxfermodes.cpp:142: * being fixed with
https://codereview.chromium.org/666043003/.
On 2014/11/18 14:57:16, egdaniel wrote:
> No need to put link to cl in the comment. The comment itself should describe
the
> bug enough and if need be the cl can always be found through history/blame.

Done.

[egdaniel, 2014-11-20 14:54:32]

lgtm

[rosca, 2014-11-20 14:55:02]

The CQ bit was checked by rosca@adobe.com

[commit-bot: I haz the power, 2014-11-20 14:55:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666043003/80001

[commit-bot: I haz the power, 2014-11-20 14:56:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 14:56:01]

Presubmit check for 666043003-80001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
The email rosca@adobe.com is not in Skia's AUTHORS file.
Issue owner, this CL must include an addition to the Skia AUTHORS file.
Googler reviewers, please check that the AUTHORS entry corresponds to an email
address in http://goto/cla-signers. If it does not then ask the issue owner to
sign the CLA at https://developers.google.com/open-source/cla/individual
(individual) or https://developers.google.com/open-source/cla/corporate
(corporate).

Presubmit checks took 1.0s to calculate.

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.

[rosca, 2014-11-20 15:09:48]

On 2014/11/20 14:56:01, I haz the power (commit-bot) wrote:
> Presubmit check for 666043003-80001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> The email mailto:rosca@adobe.com is not in Skia's AUTHORS file.
> Issue owner, this CL must include an addition to the Skia AUTHORS file.
> Googler reviewers, please check that the AUTHORS entry corresponds to an email
> address in http://goto/cla-signers. If it does not then ask the issue owner to
> sign the CLA at https://developers.google.com/open-source/cla/individual
> (individual) or https://developers.google.com/open-source/cla/corporate
> (corporate).
> 
> Presubmit checks took 1.0s to calculate.
> 
> Was the presubmit check useful? If not, run "git cl presubmit -v"
> to figure out which PRESUBMIT.py was run, then run git blame
> on the file to figure out who to ask for help.

Should I add adobe or myself as a contributor? Or maybe you could land it for
me?
Thanks.

[egdaniel, 2014-11-20 15:13:25]

On 2014/11/20 15:09:48, rosca wrote:
> On 2014/11/20 14:56:01, I haz the power (commit-bot) wrote:
> > Presubmit check for 666043003-80001 failed and returned exit status 1.
> > 
> > Running presubmit commit checks ...
> > 
> > ** Presubmit ERRORS **
> > The email mailto:rosca@adobe.com is not in Skia's AUTHORS file.
> > Issue owner, this CL must include an addition to the Skia AUTHORS file.
> > Googler reviewers, please check that the AUTHORS entry corresponds to an
email
> > address in http://goto/cla-signers. If it does not then ask the issue owner
to
> > sign the CLA at https://developers.google.com/open-source/cla/individual
> > (individual) or https://developers.google.com/open-source/cla/corporate
> > (corporate).
> > 
> > Presubmit checks took 1.0s to calculate.
> > 
> > Was the presubmit check useful? If not, run "git cl presubmit -v"
> > to figure out which PRESUBMIT.py was run, then run git blame
> > on the file to figure out who to ask for help.
> 
> Should I add adobe or myself as a contributor? Or maybe you could land it for
> me?
> Thanks.

You are already on the cla, so all you need to do is add to the AUTHORS file
which you can just add to this cl.

[rosca, 2014-11-20 15:15:38]

The CQ bit was checked by rosca@adobe.com

[commit-bot: I haz the power, 2014-11-20 15:16:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666043003/100001

[commit-bot: I haz the power, 2014-11-20 15:24:37]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/ace7f4276997235abe559b70620d9f89737d2518