Bug 51161

Summary: [Qt] Blur distance should not be affected by transformations
Product: WebKit Reporter: Helder Correia <helder>
Component: CanvasAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ariya.hidayat, commit-queue, mdelaney7, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 34479    
Attachments:
Description Flags
Patch
none
Untested with GTK
none
Suggestions in comment #5
none
Correction from comment #7 none

Description Helder Correia 2010-12-15 19:04:16 PST
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.
Comment 1 Helder Correia 2010-12-15 22:10:02 PST
Created attachment 76734 [details]
Patch
Comment 2 Ariya Hidayat 2010-12-16 09:23:23 PST
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.
Comment 3 Helder Correia 2010-12-16 13:05:47 PST
(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.
Comment 4 Helder Correia 2010-12-18 20:00:41 PST
Created attachment 76950 [details]
Untested with GTK
Comment 5 Martin Robinson 2010-12-18 20:12:34 PST
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.
Comment 6 Ariya Hidayat 2010-12-18 20:19:09 PST
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
Comment 7 Helder Correia 2010-12-18 20:23:33 PST
Created attachment 76951 [details]
Suggestions in comment #5
Comment 8 Helder Correia 2010-12-18 20:32:33 PST
Created attachment 76952 [details]
Correction from comment #7
Comment 9 Ariya Hidayat 2010-12-19 15:17:34 PST
Comment on attachment 76952 [details]
Correction from comment #7

Looks good. re=me
Comment 10 WebKit Commit Bot 2010-12-19 17:02:31 PST
Comment on attachment 76952 [details]
Correction from comment #7

Clearing flags on attachment: 76952

Committed r74328: <http://trac.webkit.org/changeset/74328>
Comment 11 WebKit Commit Bot 2010-12-19 17:02:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Commit Bot 2010-12-19 18:13:54 PST
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: japhet@chromium.org)
The commit-queue is continuing to process your patch.