Bug 51161 – [Qt] Blur distance should not be affected by transformations

• Component: Canvas
• Status: RESOLVED FIXED

[Helder Correia]

From the spec at http://dev.w3.org/html5/2dcontext/#dom-context-2d-shadowblur :
"The shadowBlur attribute specifies the level of the blurring effect. (The units do not map to coordinate space units, and are not affected by the current transformation matrix.)"

Currently, blurring is applied in the untransformed layerImage. When layerImage is transformed and displayed on screen, the blur radius will also be affected by the scaling factor. The solution is dividing the blur radius by the balanced (X and Y) scaling factor.

[Helder Correia]

Created attachment 76734 [details]
Patch

[Ariya Hidayat]

Comment on attachment 76734 [details]
Patch

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

Minor issues, otherwise LGTM.

> WebCore/ChangeLog:5
> +        [Qt] shadowBlur should not be affected by scaling

Although shadowBlur is the term used by the spec, I guess it's clearer if here we put e.g. "Shadow distance" instead.

> WebCore/platform/graphics/qt/ContextShadowQt.cpp:119
> +    // Adjust blur if we're scaling, since the radius must not be affected by transformations.

Seems that it is possible to refactor this to be usable for Cairo as well.

> WebCore/platform/graphics/qt/ContextShadowQt.cpp:142
> +        // Calculate X axis scale factor.
> +        const float xAxisHorizontalChange = transformedXAxisUnit.x() - transformedOrigin.x();
> +        const float xAxisVerticalChange = transformedXAxisUnit.y() - transformedOrigin.y();
> +        const float xAxisScale = sqrtf(xAxisHorizontalChange * xAxisHorizontalChange
> +                                       + xAxisVerticalChange * xAxisVerticalChange);
> +
> +        // Calculate Y axis scale factor.
> +        const float yAxisHorizontalChange = transformedYAxisUnit.x() - transformedOrigin.x();
> +        const float yAxisVerticalChange = transformedYAxisUnit.y() - transformedOrigin.y();
> +        const float yAxisScale = sqrtf(yAxisHorizontalChange * yAxisHorizontalChange
> +                                       + yAxisVerticalChange * yAxisVerticalChange);

This could be simplified using QVector2D.

[Helder Correia]

(In reply to comment #2)
> Seems that it is possible to refactor this to be usable for Cairo as well.

I'll wait until bug 50422 is resolved for good.

[Helder Correia]

Created attachment 76950 [details]
Untested with GTK

[Martin Robinson]

Comment on attachment 76950 [details]
Untested with GTK

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

Very nice. A couple nits below.

> WebCore/platform/graphics/ContextShadow.cpp:177
> +    if (!transform.isIdentity()) {

I think this is a good place to use an early return.

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:97
>      double x1, x2, y1, y2;
>      cairo_clip_extents(context, &x1, &y1, &x2, &y2);
> +    adjustBlurDistance(context);

Maybe move the third line above the first two lines? The clip extents are only used for the call to calculateLayerBoundingRect, so this is in the middle of another "sentence" so to speak.

[Ariya Hidayat]

Comment on attachment 76950 [details]
Untested with GTK

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

> LayoutTests/ChangeLog:5
> +        [Qt] Blur distance should not be affected by scaling

s/scaling/transformation

[Helder Correia]

Created attachment 76951 [details]
Suggestions in comment #5

[Helder Correia]

Created attachment 76952 [details]
Correction from comment #7

[Ariya Hidayat]

Comment on attachment 76952 [details]
Correction from comment #7

Looks good. re=me

[WebKit Commit Bot]

Comment on attachment 76952 [details]
Correction from comment #7

Clearing flags on attachment: 76952

Committed r74328: <http://trac.webkit.org/changeset/74328>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 76952 [details]:

http/tests/navigation/ping-cross-origin-from-https.html bug 51314 (author: [e-mail])
The commit-queue is continuing to process your patch.