Bug 46918 – ContextShadow should not use the blur radius as kernel size of the box blurs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alejandro G. Castro]

Currently we are using the blur radius in ContextShadow directly as the kernel size when blurring, this is causing a bigger blurring effect than expected. We should calculate the kernel size required to get a blur effect which size is like the blur radius. We were using before 2/3 of the blur radius, because we are adding kernel_size/2 pixels with each box blur, therefore with 3 box blurs we get 3*kernel_size/2 more pixels, which should be the blur radius. Firefox is applying a similar solution.

[Alejandro G. Castro]

Created attachment 69347 [details]
Proposed patch

This is an option without supporting fractional kernel sizes which would be probably the one with the best results, mostly for the small radii. It is basically what we are currently doing in FEGaussianBlur.

[Martin Robinson]

Comment on attachment 69347 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69347&action=review

> WebCore/platform/graphics/ContextShadow.cpp:158
> +    // We expand the area by the blur radius to give extra space for the blur transition.
> +    m_layerRect.inflate((m_type == BlurShadow) ? ceil(m_blurRadius) : 0);

We probably want to change the name m_blurRadius to m_blurWidth to clear up any confusion about what this value means.

[Alejandro G. Castro]

(In reply to comment #2)
>
> [...]
> 
> We probably want to change the name m_blurRadius to m_blurWidth to clear up any confusion about what this value means.

I agree, I've checked the CSS spec and it is call blur distance we can use that:

http://www.w3.org/TR/css3-background/#box-shadow

[Jarred Nicholls]

Alejandro, I would update this patch to conform to the landed patch in r68824 (https://bugs.webkit.org/show_bug.cgi?id=46931), which removes the unnecessary calls to ceil (which happen to break MSVC builds since 9/23 when the Qt blur was forked for Cairo).  Just FYI.

[Alejandro G. Castro]

(In reply to comment #4)
> Alejandro, I would update this patch to conform to the landed patch in r68824 (https://bugs.webkit.org/show_bug.cgi?id=46931), which removes the unnecessary calls to ceil (which happen to break MSVC builds since 9/23 when the Qt blur was forked for Cairo).  Just FYI.

Thanks for information, I'll do it.

[Alejandro G. Castro]

Created attachment 69450 [details]
Proposed patch

Addressed the recomendations, I've used blurDistance (from the css spec) and d (from the svg spec). Rebased over Jarred patch.

[Early Warning System Bot]

Attachment 69450 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4212035

[Jarred Nicholls]

See r68145, Mark changed several files in Cairo and Qt to use m_blurRadius that will also need refactoring for the name change to m_blurDistance, e.g. qt/FontQt.cpp.

[Jarred Nicholls]

(In reply to comment #8)
> See r68145, Mark changed several files in Cairo and Qt to use m_blurRadius that will also need refactoring for the name change to m_blurDistance, e.g. qt/FontQt.cpp.

First off, I meant to type Martin and for some reason typed Mark.  My apologies Martin!

Second, it's actually just the qt/FontQt.cpp after doing a search.  Hope that saves you some time.

[Alejandro G. Castro]

(In reply to comment #9)
> (In reply to comment #8)
> > See r68145, Mark changed several files in Cairo and Qt to use m_blurRadius that will also need refactoring for the name change to m_blurDistance, e.g. qt/FontQt.cpp.
> 
> First off, I meant to type Martin and for some reason typed Mark.  My apologies Martin!
> 
> Second, it's actually just the qt/FontQt.cpp after doing a search.  Hope that saves you some time.

Yeah, thanks very much Jarred, preparing the patch.

[Alejandro G. Castro]

Created attachment 69615 [details]
Proposed patch

I've modified the m_blurRadius in FontQt, is there any reason not to declare those members as private and add a funtion to get them? In that case we could open other bug to do that.

[Early Warning System Bot]

Attachment 69615 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4166069

[Alejandro G. Castro]

Created attachment 69617 [details]
Proposed patch

[Dirk Schulze]

Comment on attachment 69617 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69617&action=review

Please fix the issues and I'm fine with it. r- because of the issues.

> WebCore/platform/graphics/ContextShadow.cpp:88
> +    unsigned d = max(2.f, (2 / 3.f) * m_blurDistance);

d is unsigned, you can't use unsigned d = max(2.f
use
unsigned d = max<unsigned>(2, floorf(2 / 3.f * m_blurDistance)); instead.

> WebCore/platform/graphics/ContextShadow.cpp:158
> +    m_layerRect.inflate((m_type == BlurShadow) ? m_blurDistance : 0);

remove the braces arround m_type == BlurShadow

> WebCore/platform/graphics/ContextShadow.cpp:171
> +            m_layerRect.inflate((m_type == BlurShadow) ? m_blurDistance : 0);

Ditto.

[Dirk Schulze]

(In reply to comment #14)
> (From update of attachment 69617 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69617&action=review
> d is unsigned, you can't use unsigned d = max(2.f
> use
> unsigned d = max<unsigned>(2, floorf(2 / 3.f * m_blurDistance)); instead.
> 
you still need a static_cast<unsigned> for floorf, sorry.

[Alejandro G. Castro]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 69617 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=69617&action=review
> > d is unsigned, you can't use unsigned d = max(2.f
> > use
> > unsigned d = max<unsigned>(2, floorf(2 / 3.f * m_blurDistance)); instead.
> > 
> you still need a static_cast<unsigned> for floorf, sorry.

Thanks for the review, I'm going to use int the distance is limited to 128 so it is not a problem, something like:

+    int d = max(2, static_cast<int>(floorf((2 / 3.f) * m_blurDistance)));

[Alejandro G. Castro]

Created attachment 69942 [details]
Proposed patch

Fixed the issues pointed out in the comments.

[Dirk Schulze]

Comment on attachment 69942 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69942&action=review

LGTM. Please use a better phrasing in the ChangeLog.

> WebCore/ChangeLog:10
> +        Calculate the size of the kernel in the blur algorihtm using the
> +        radius instead of using directly. Change the name of the variables

using directly 
something is missing here. What don't you want to use directly?

[Alejandro G. Castro]

(In reply to comment #18)
> (From update of attachment 69942 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69942&action=review
> 
> LGTM. Please use a better phrasing in the ChangeLog.
> 
> > WebCore/ChangeLog:10
> > +        Calculate the size of the kernel in the blur algorihtm using the
> > +        radius instead of using directly. Change the name of the variables
> 
> using directly 
> something is missing here. What don't you want to use directly?

Yep, ... instead of using directly the blur distance as the kernel size. Thanks, I'm going to commit it.

[Martin Robinson]

Committed r69223: <http://trac.webkit.org/changeset/69223>