Update feSpotLight to match spec

Update feSpotLight to match spec

This change updates feSpotLight to match the spec via two changes:

1) specularExponent is ignored if the spotlight has no coneAngle (GPU
   bug only). This change updates the GPU path so that it matches the
   CPU path and the spec in this regard.

2) specularExponent is clamped to the 1-128 range. The spec does not
   specify a clamp for the specularExponent attribute of feSpotLight.
   Note that the spec *does* specify this clamp for the
   specularExponent attribute of feSpecularLighting. It looks like we
   incorrectly applied this to both specularExponent attributes.

   This change (along with a parallel change in Blink) allows us to pass
   the SVG filter effects conformance test here:
   http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObject/filters-light-01-f.html

   Additionally, this brings our behavior in line with Safari and Edge’s
   behavior on this filter.

Two new cases were added to gm/lighting.cpp to catch these issues:
- The existing spotlight case exercised the path where our specular
  exponent was between 1-128 and had a limiting cone angle.
- The first new spotlight case exercises the path where our specular
  exponent is between 1-128 and we do not have a limiting cone angle.
- The second new spotlight case exercises the path where the specular
  exponent is not within the 1-128 range, to ensure that we don’t
  incorrectly clip to this range.

BUG=472849
Committed: https://skia.googlesource.com/skia/+/c84ccb070258db2803a9e8f532bfe7239a737063

[ericrk, 2015-10-15 22:59:12]

ericrk@chromium.org changed reviewers:
+ senorblanco@google.com

[ericrk, 2015-10-15 22:59:12]

This one was more interesting :D. PTAL

[Stephen White, 2015-10-16 15:03:10]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2015-10-16 15:03:10]

Good catches!

https://codereview.chromium.org/1403403003/diff/1/src/effects/SkLightingImage...
File src/effects/SkLightingImageFilter.cpp (left):

https://codereview.chromium.org/1403403003/diff/1/src/effects/SkLightingImage...
src/effects/SkLightingImageFilter.cpp:858:
fSpecularExponent(SkScalarPin(specularExponent, kSpecularExponentMin,
kSpecularExponentMax))
Although it may not be spec-correct, this was added by sugoi@ back in
https://chromiumcodereview.appspot.com/23533042/ in order to fix a fuzzer issue.
According to that change, large negative values were a problem, presumably
generating near-zero scale when passed to pow(). That said, I tried the unit
test in debug with your change, and it didn't assert, so maybe we're ok.

(By way of explanation, the fuzzer is there to test Skia when run cross-process,
which we do for accelerated filters. Even if Blink applies the correct limits,
we need to protect Skia against a compromised renderer process giving arbitrary
values.)

Also, this seems to be the last use of these min/max constants. We should either
apply them to "shininess" as the spec suggests, or remove them.

[Stephen White, 2015-10-16 18:21:13]

The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 18:21:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403403003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403403003/1

[ericrk, 2015-10-16 18:26:09]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-10-16 18:26:13]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-10-16 18:26:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403403003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403403003/1

[commit-bot: I haz the power, 2015-10-16 18:26:18]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-10-17 00:26 UTC

[ericrk, 2015-10-16 18:29:41]

The CQ bit was unchecked by ericrk@chromium.org

[Stephen White, 2015-10-16 18:35:25]

LGTM

[ericrk, 2015-10-16 18:36:22]

ericrk@chromium.org changed reviewers:
+ bsalomon@google.com

[ericrk, 2015-10-16 18:36:23]

https://codereview.chromium.org/1403403003/diff/1/src/effects/SkLightingImage...
File src/effects/SkLightingImageFilter.cpp (left):

https://codereview.chromium.org/1403403003/diff/1/src/effects/SkLightingImage...
src/effects/SkLightingImageFilter.cpp:858:
fSpecularExponent(SkScalarPin(specularExponent, kSpecularExponentMin,
kSpecularExponentMax))
On 2015/10/16 15:03:10, Stephen White wrote:
> Although it may not be spec-correct, this was added by sugoi@ back in
> https://chromiumcodereview.appspot.com/23533042/ in order to fix a fuzzer
issue.
> According to that change, large negative values were a problem, presumably
> generating near-zero scale when passed to pow(). That said, I tried the unit
> test in debug with your change, and it didn't assert, so maybe we're ok.
> 
> (By way of explanation, the fuzzer is there to test Skia when run
cross-process,
> which we do for accelerated filters. Even if Blink applies the correct limits,
> we need to protect Skia against a compromised renderer process giving
arbitrary
> values.)
> 
> Also, this seems to be the last use of these min/max constants. We should
either
> apply them to "shininess" as the spec suggests, or remove them.

Per offline discussion - going to remove and see if fuzz gets mad - will add a
less restrictive limit back if it does. removing constant for now.

[ericrk, 2015-10-19 21:24:16]

Friendly ping for owners review. Thanks!

[Stephen White, 2015-10-19 21:32:17]

On 2015/10/19 21:24:16, ericrk wrote:
> Friendly ping for owners review. Thanks!

This should be able to land with just my LG.

[Stephen White, 2015-10-19 21:32:24]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2015-10-19 21:32:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403403003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403403003/20001

[commit-bot: I haz the power, 2015-10-19 21:44:58]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/c84ccb070258db2803a9e8f532bfe7239a737063

[caryclark, 2015-10-20 17:03:47]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1417463006/ by caryclark@google.com.

The reason for reverting is: re-land once layout test have been disabled (so
they can be rebased).