ContextShadow uses inaccurate kernel size computation

• Component: Platform
• Status: RESOLVED INVALID

[Simon Fraser (smfr)]

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.

[Helder Correia]

(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.

[Helder Correia]

Created attachment 77197 [details]
Untested with Cairo

[Alejandro G. Castro]

(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.

[Simon Fraser (smfr)]

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.

[Alejandro G. Castro]

(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.

[Dirk Schulze]

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?

[Simon Fraser (smfr)]

Fine, I don't care about ContextShadow any more :)

[Martin Robinson]

ContextShadow is long gone now. :)