Bug 51446 - ContextShadow uses inaccurate kernel size computation
: ContextShadow uses inaccurate kernel size computation
Status: RESOLVED INVALID
: WebKit
Platform
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 26389
  Show dependency treegraph
 
Reported: 2010-12-21 21:22 PST by
Modified: 2012-06-13 11:40 PST (History)


Attachments
Untested with Cairo (7.39 KB, patch)
2010-12-22 01:56 PST, Helder Correia
krit: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-12-21 21:22:12 PST
The SVG spec, and FEGaussianBlur, compute the kernel size by multiplying the blur radius by
  gGaussianKernelFactor = 3 / 4.f * sqrtf(2 * piFloat)

which becomes, in blurLayerImage,
-    int d = max(2, static_cast<int>(floorf((2 / 3.f) * m_blurDistance)));
+    int d = max(2, static_cast<int>(floorf(m_blurDistance / 2 * GaussianKernelFactor + 0.5f)));

This gives a more accurate radius for small shadows. It's noticeable on a 5px shadow.
------- Comment #1 From 2010-12-21 22:59:01 PST -------
(In reply to comment #0)
> This gives a more accurate radius for small shadows. It's noticeable on a 5px shadow.

This might need changing a few tests. I'll take care of it.
------- Comment #2 From 2010-12-22 01:56:34 PST -------
Created an attachment (id=77197) [details]
Untested with Cairo
------- Comment #3 From 2010-12-22 02:08:40 PST -------
(In reply to comment #0)
> The SVG spec, and FEGaussianBlur, compute the kernel size by multiplying the blur radius by
>   gGaussianKernelFactor = 3 / 4.f * sqrtf(2 * piFloat)
> 
> which becomes, in blurLayerImage,
> -    int d = max(2, static_cast<int>(floorf((2 / 3.f) * m_blurDistance)));
> +    int d = max(2, static_cast<int>(floorf(m_blurDistance / 2 * GaussianKernelFactor + 0.5f)));
> 
> This gives a more accurate radius for small shadows. It's noticeable on a 5px shadow.

I'm not sure if this is correct, in case of the gaussian blur we use that factor because we have the standard deviation, but in case of the canvas we have the blur distance.

Basically 2/3 is way to say that we are doing a box blur that increases d/2 pixels each time we do a box blur, and we do 3 box blurs. Therefore to get a distance of m_blurDistance we multiply by 2/3. We could modify the calculation and create a heuristic for the small distances because the rounding could cause problems though.

Actually I pointed hixie about the problems of having different APIs for the same operation in specs that are going to be implemented in the browser because it causes this kind of confusions.

 http://www.w3.org/Bugs/Public/show_bug.cgi?id=9942#c2

Hope it helps.
------- Comment #4 From 2010-12-22 08:30:32 PST -------
ContextShadow is used for both CSS shadows and Canvas shadows. I have a patch that will pass this information down to GraphicsContext (a follow-on from bug 51448). Once we have that information, ContextShadow can support both behaviors.
------- Comment #5 From 2011-01-11 03:03:47 PST -------
(In reply to comment #4)
> ContextShadow is used for both CSS shadows and Canvas shadows. I have a patch that will pass this information down to GraphicsContext (a follow-on from bug 51448). Once we have that information, ContextShadow can support both behaviors.

That's nice but still SVG will be using standard deviation and CSS/canvas blur radius in the API for the same operation, which in my opinion could cause confusion. I'm completely clueless if this have any solution changing something in the standards at this point, did not think about it either, just pointed it out because we have found it confusing when implementing it more than once.
------- Comment #6 From 2011-02-09 02:06:00 PST -------
(From update of attachment 77197 [details])
The change seems to be in ShadowBlur.cpp. The other platforms should move to this code. I'd like to mark this bug as invalid. Any comments?
------- Comment #7 From 2011-02-09 07:48:17 PST -------
Fine, I don't care about ContextShadow any more :)
------- Comment #8 From 2012-06-13 11:40:52 PST -------
ContextShadow is long gone now. :)