Bug 51446 - ContextShadow uses inaccurate kernel size computation
: ContextShadow uses inaccurate kernel size computation
Status: RESOLVED INVALID
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 68469 26389
  Show dependency treegraph
 
Reported: 2010-12-21 21:22 PST by Simon Fraser (smfr)
Modified: 2014-05-12 05:54 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 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 Helder Correia 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 Helder Correia 2010-12-22 01:56:34 PST
Created attachment 77197 [details]
Untested with Cairo
Comment 3 Alejandro G. Castro 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 Simon Fraser (smfr) 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 Alejandro G. Castro 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 Dirk Schulze 2011-02-09 02:06:00 PST
Comment on attachment 77197 [details]
Untested with Cairo

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 Simon Fraser (smfr) 2011-02-09 07:48:17 PST
Fine, I don't care about ContextShadow any more :)
Comment 8 Martin Robinson 2012-06-13 11:40:52 PDT
ContextShadow is long gone now. :)