Fix quickReject computation for blurs

Without this CL the quickReject code can sometimes incorrectly eliminate the edge of some blurs.

One downside is that the gpu library now needs access to a src\effects header.

[bsalomon, 2013-06-14 16:10:22]

On 2013/06/14 16:02:43, robertphillips wrote:

Egads.

We need SkMaskFilter::filterMaskGPU(GrContext*, ...) so much.

[bsalomon, 2013-06-14 16:15:31]

How important is it to fix the GPU aspect of this right now? Can it wait until
we fix the architectural issue and have a cleaner way for SkMaskFilters to play
nice with GrContext?

[bsalomon, 2013-06-14 16:15:31]

How important is it to fix the GPU aspect of this right now? Can it wait until
we fix the architectural issue and have a cleaner way for SkMaskFilters to play
nice with GrContext?

[bsalomon, 2013-06-14 18:23:54]

https://codereview.chromium.org/17035007/diff/18/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/17035007/diff/18/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:803: float sigma = SkScalarToFloat(radius) *
SkBlurMaskFilter::kBLUR_SIGMA_SCALE;
Could the radius returned in info already account for this?

[robertphillips, 2013-06-14 19:15:15]

PTAL. This decouples the gpu and effect libs (via the SkMaskFilter::BlurInfo
struct) and consolidates all the constants in SkBlurMask.

[bsalomon, 2013-06-14 19:31:44]

On 2013/06/14 19:15:15, robertphillips wrote:
> PTAL. This decouples the gpu and effect libs (via the SkMaskFilter::BlurInfo
> struct) and consolidates all the constants in SkBlurMask.

I'll defer to others, but I don't get why the Info can't just return the final
sigma value rather than three params to compute it in SkGpuDevice.cpp. I don't
see any other code that uses the fRadius from blur info. XPS calls asABlur() but
doesn't use the radius.

[Stephen White, 2013-06-14 19:37:38]

https://codereview.chromium.org/17035007/diff/15001/src/effects/SkBlurMaskFil...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/17035007/diff/15001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:298: // GPU path
I guess there's no way to pass the device down here, so it can do a
device-specific thing?

[robertphillips, 2013-06-14 19:58:44]

> I'll defer to others, but I don't get why the Info can't just return the final
> sigma value rather than three params to compute it in SkGpuDevice.cpp. I don't
> see any other code that uses the fRadius from blur info. XPS calls asABlur()
but
> doesn't use the radius.

In drawWithGPUMaskFilter the radius is passed through the CTM before being
rescaled by the fudge factor but we probably know enough about the CTM (e.g.,
translation components ignored, not perspective) that the uniform scale (in the
rescaling) could be safely pulled before the application of the CTM and into the
asABlur method. The shouldDrawBlurWithCPU method would also have to be altered
to operate on sigma rather than the original radius. The max radius clamp needs
to be performed after the application of the CTM however. We could pass the CTM
into the asABlur method and then just move the drawWithGPUMaskFilter preamble
code there (and still update shouldDrawBlurWithCPU to operate on sigma).

One pro of doing this is that I think Skia's blur interface should just take the
sigma value (a la SVG) so this would be a move in that direction.

[robertphillips, 2013-07-29 19:33:55]

Back from the dead. PTAL.

I have verified that without this patch the new GM differs (in the low bits)
between with and without quick reject enabled. With this patch the with and
without quick reject images are the same.

[reed1, 2013-07-29 19:37:09]

Do we have a demo-CL ready on the Chrome side, document what the next step(s) is
to finally eliminate this Fudge from Skia?

lgtm

[robertphillips, 2013-07-30 12:22:29]

committed as r10428.

I have a TODO to convert Skia's API to take 'sigma' rather than radius but do
not yet have a CL for that. I think the full solution will require 3 more CLs:

1) alter the raster & GPU pipelines to use sigma & push the computation of sigma
from radius up the call stack & unify it.
2) Alter the external API to take sigma
3) Push the radius mashing up into Blink

With 2&3 happening in the same DEPS roll or separated by using a new API (e.g.,
deprecate the current Create method and add a new one).

[reed1, 2013-07-30 13:37:38]

On 2013/07/30 12:22:29, robertphillips wrote:
> committed as r10428.
> 
> I have a TODO to convert Skia's API to take 'sigma' rather than radius but do
> not yet have a CL for that. I think the full solution will require 3 more CLs:
> 
> 1) alter the raster & GPU pipelines to use sigma & push the computation of
sigma
> from radius up the call stack & unify it.
> 2) Alter the external API to take sigma
> 3) Push the radius mashing up into Blink
> 
> With 2&3 happening in the same DEPS roll or separated by using a new API
(e.g.,
> deprecate the current Create method and add a new one).

SGTM -- lets make 1-3 happen soon.