RESOLVED FIXED 51161
[Qt] Blur distance should not be affected by transformations 
https://bugs.webkit.org/show_bug.cgi?id=51161
Summary [Qt] Blur distance should not be affected by transformations
Helder Correia
Reported 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.
Attachments
Patch (14.34 KB, patch)
2010-12-15 22:10 PST, Helder Correia 		no flags
DetailsFormatted DiffDiff
Untested with GTK (16.77 KB, patch)
2010-12-18 20:00 PST, Helder Correia 		no flags
DetailsFormatted DiffDiff
Suggestions in comment #5 (16.62 KB, patch)
2010-12-18 20:23 PST, Helder Correia 		no flags
DetailsFormatted DiffDiff
Correction from comment #7 (16.64 KB, patch)
2010-12-18 20:32 PST, Helder Correia 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Helder Correia
Comment 1 2010-12-15 22:10:02 PST
Created attachment 76734 [details] Patch
Ariya Hidayat
Comment 2 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.
Helder Correia
Comment 3 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.
Helder Correia
Comment 4 2010-12-18 20:00:41 PST
Created attachment 76950 [details] Untested with GTK
Martin Robinson
Comment 5 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.
Ariya Hidayat
Comment 6 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
Helder Correia
Comment 7 2010-12-18 20:23:33 PST
Created attachment 76951 [details] Suggestions in comment #5
Helder Correia
Comment 8 2010-12-18 20:32:33 PST
Created attachment 76952 [details] Correction from comment #7
Ariya Hidayat
Comment 9 2010-12-19 15:17:34 PST
Comment on attachment 76952 [details] Correction from comment #7 Looks good. re=me
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2010-12-19 17:02:38 PST
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 12 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.
Note You need to log in before you can comment on or make changes to this bug.