Bug 40793 - [Cairo] The size of the shadow image uses the standard deviation size instead of the blur radius
: [Cairo] The size of the shadow image uses the standard deviation size instead...
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
: 41605
: 39582 42081 43962
  Show dependency treegraph
 
Reported: 2010-06-17 11:53 PST by
Modified: 2010-08-19 04:01 PST (History)


Attachments
Test example (406 bytes, text/html)
2010-06-17 11:53 PST, Alejandro G. Castro
no flags Details
Initial patch proposal (9.98 KB, patch)
2010-06-17 11:56 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (19.68 KB, patch)
2010-07-01 13:23 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (19.66 KB, patch)
2010-07-01 15:31 PST, Alejandro G. Castro
krit: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch (16.53 KB, patch)
2010-07-14 10:51 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (15.77 KB, patch)
2010-08-11 02:19 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (15.75 KB, patch)
2010-08-11 04:19 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (15.75 KB, patch)
2010-08-11 05:05 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (15.72 KB, patch)
2010-08-18 12:11 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (15.43 KB, patch)
2010-08-19 01:39 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (15.42 KB, patch)
2010-08-19 02:35 PST, Alejandro G. Castro
krit: review+
krit: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-06-17 11:53:06 PST
Created an attachment (id=59024) [details]
Test example

I've attached an html file where it is easy to check the problem that it causes, it clips the shadows. It is also a problem for the bug where we are trying to do tiling in order to create the shadow because we need the size of the kernel in order to get the minimum size of a rectangle (bug 39582)
39582
After talking to krit an hixie I have also uploaded a bug to the HTML5 standard to avoid misinterpretations of the sentence describing the shadowBlur: http://www.w3.org/Bugs/Public/show_bug.cgi?id=9942
------- Comment #1 From 2010-06-17 11:56:27 PST -------
Created an attachment (id=59026) [details]
Initial patch proposal

This is an initial example of what we can do, the other option would be to create the filters in before calling the GraphicContext functions. Open to comments :).
------- Comment #2 From 2010-07-01 13:23:41 PST -------
Created an attachment (id=60279) [details]
Proposed patch

This is a first final proposal, I checked the visual result of other browsers.


            The kernelSize variable was renamed to radius and recalculated
            considering the CSS3 specification
            http://www.w3.org/TR/css3-background/#the-box-shadow, and the
            visual result of other browsers. The HTML5 canvas shadow standard
            deviation calculation that was used, was not appropiate for the
            blur distance specified in the CSS3.

            Added also the recomendations of SVG regarding the kernel size
            calculation for the approximation of the gaussian
            blur. http://www.w3.org/TR/SVG/filters.html#feGaussianBlurElement
------- Comment #3 From 2010-07-01 13:25:25 PST -------
Attachment 60279 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:101:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:104:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:113:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:119:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:121:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:126:  GAUSSIAN_KERNEL_FACTOR is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:182:  Missing spaces around /  [whitespace/operators] [3]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:183:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #4 From 2010-07-01 15:31:06 PST -------
Created an attachment (id=60298) [details]
Proposed patch

Fixed style issues
------- Comment #5 From 2010-07-01 23:33:09 PST -------
(From update of attachment 60298 [details])
WebCore/platform/graphics/cairo/FontCairo.cpp:97
 +          float radius = 0.f;
Please use jut 0 without the .f at the end (according to the change in the style guide)

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:180
 +      float radius = 0.f;
dito

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:580
 +      float radius = 0.f;
dito

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:871
 +      radius = std::min(1000.f, radius);
dito

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:873
 +      FloatSize resolution = FloatSize(1.0f, 1.0f);
...(1, 1) Also, the resolution doesn't change here, add it to the funtion call directly

WebCore/platform/graphics/cairo/ImageCairo.cpp:143
 +          float radius = 0.f;
no .f

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:99
 +  static void kernelPosition(int boxBlur, unsigned std, int& dLeft, int& dRight)
Either you use inline, or you add the function to the header. I bet that is the reason why Mac-build fails.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:125
 +  static const float gaussianKernelFactor = (3.f * sqrt(2 * M_PI) / 4.f);
Please add this line right after the includes. Name it gGaussianKernelFactor to make sure that this is a constant. No .f. I think M_PI is wrong here (problems on Mac builds). please use piDouble again.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:141
 +      unsigned sdx = static_cast<unsigned>(floor(std::max(m_x, 2.f) * filter->filterResolution().width() * gaussianKernelFactor + 0.5f));
no .f

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:142
 +      unsigned sdy = static_cast<unsigned>(floor(std::max(m_y, 2.f) * filter->filterResolution().height() * gaussianKernelFactor + 0.5f));
dito

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:181
 +      sdx = std::max((radius.x() * 2 / 3 - 0.5f) / (resolution.width() * gaussianKernelFactor), 0.f);
Please use: using namespace std; Saw this multiple times before, please fix it there as well.
no .f

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:182
 +      sdy = std::max((radius.y() * 2 / 3 - 0.5f) / (resolution.height() * gaussianKernelFactor), 0.f);
dito

I'm quite sure, that this need updates of pixel tests. Can you run layout tests with a tolerance of zero please? I'm also not sure about kernelPosition. The spec for feGaussianBlur changed recently and a blurring radius of 0 is allowed now. If a stdDeviation for any direction is zero, the image is just not blurred for this direction. At the moment we break and don't draw the blurring at all. Now that you try to fix the SVG filter as well, and I'm realy glad about this :-), can you take about this fact as well? It might also be possible to move the feGaussianBlur fixes to another patch,if you think this is not the right place.

Great patch! Please fix the style problems and think about the last comment about kernelPosition. r- for now, also for breaking Mac.
------- Comment #6 From 2010-07-02 00:35:01 PST -------
(In reply to comment #5)
>
> [...]
>
> I'm quite sure, that this need updates of pixel tests. Can you run layout tests with a tolerance of zero please?

Yep, the visual result is different, I was wondering if other platforms with pixel tests for this filter could have problems because I could not test it in webkitgtk+. I'll review what we need.

> I'm also not sure about kernelPosition. The spec for feGaussianBlur changed recently and a blurring radius of 0 is allowed now. If a stdDeviation for any direction is zero, the image is just not blurred for this direction. At the moment we break and don't draw the blurring at all.

I understand, we can change also this condition that controls the standard deviation parameter:

    if (m_x == 0 || m_y == 0)
        return;

with

    if (m_x == 0 && m_y == 0)
        return;

And add ifs to the calculations to avoid calculating sdx or sdy and avoid the blox blur in that direction.

Probably the name of the parameters/variables are still confusing, I could change m_x and m_y with mStdx and mStdy, and sdx/sdy with kernelSizex/kernelSizey.

> Now that you try to fix the SVG filter as well, and I'm realy glad about this :-), can you take about this fact as well? It might also be possible to move the feGaussianBlur fixes to another patch,if you think this is not the right place.

Yep, actually it was my my initial intention to do it separately, but finally checking the visual result of reviewing the kernel_size I've checked the position of the shadow was not correct without all the modifications, and I was not sure if it was due to the change I did, my fault, I'll check it and try to separate both patches adding the kernelPosition. In case we have pixel tests we will have to do two modifications also.

> Great patch! Please fix the style problems and think about the last comment about kernelPosition. r- for now, also for breaking Mac.

I'll do, thanks very much for the review Dirk :).
------- Comment #7 From 2010-07-03 05:47:44 PST -------
(In reply to comment #6)
> (In reply to comment #5)
> >
> > [...]
> >
> > I'm quite sure, that this need updates of pixel tests. Can you run layout tests with a tolerance of zero please?
> 
> Yep, the visual result is different, I was wondering if other platforms with pixel tests for this filter could have problems because I could not test it in webkitgtk+. I'll review what we need.
> 
> > I'm also not sure about kernelPosition. The spec for feGaussianBlur changed recently and a blurring radius of 0 is allowed now. If a stdDeviation for any direction is zero, the image is just not blurred for this direction. At the moment we break and don't draw the blurring at all.
> 
> I understand, we can change also this condition that controls the standard deviation parameter:
> 
>     if (m_x == 0 || m_y == 0)
>         return;
> 
> with
> 
>     if (m_x == 0 && m_y == 0)
>         return;
The result of the filter, if stdDeviation is 0 for all sizes, is as if no filter was applied at all. that means input = output. With if (m_x == 0 && m_y == 0) the output is transparent black, independent of the input. The spec changed lately and noone removed this check up to now.

> 
> And add ifs to the calculations to avoid calculating sdx or sdy and avoid the blox blur in that direction.
> 
> Probably the name of the parameters/variables are still confusing, I could change m_x and m_y with mStdx and mStdy, and sdx/sdy with kernelSizex/kernelSizey.
For private variables of a class we use m_... as prefix. But of course you can rename it to something like m_stdX and m_stdY.

> I'll do, thanks very much for the review Dirk :).
Thank you for working on this problem :-)
------- Comment #8 From 2010-07-13 12:39:49 PST -------
Hi,

I was wondering why are we limiting the radius to 1000? It is true that CG is doing this. However, skia in *http://code.google.com/p/skia/source/browse/trunk/src/effects/SkBlurMaskFilter.cpp* (MAX_RADIUS) is limiting it to 128? For performance reasons, wouldn't it be better to follow skia model here and limit the radius values even more? It seems to me that values greater than 128 are both computational intensive and unlikely to be used in practice.

Regards,
Mihnea Ovidenie
------- Comment #9 From 2010-07-14 02:27:10 PST -------
(In reply to comment #8)
> Hi,
> 
> I was wondering why are we limiting the radius to 1000? It is true that CG is doing this. However, skia in *http://code.google.com/p/skia/source/browse/trunk/src/effects/SkBlurMaskFilter.cpp* (MAX_RADIUS) is limiting it to 128? For performance reasons, wouldn't it be better to follow skia model here and limit the radius values even more? It seems to me that values greater than 128 are both computational intensive and unlikely to be used in practice.
>

I agree, I think we can use deviation instead of radius and do it inside the FEGaussianBlur object that way, we can use 50 for instance which is a radius of 94.5 aprox. I think mozilla is using 25, at leas in the canvas.
------- Comment #10 From 2010-07-14 10:51:32 PST -------
Created an attachment (id=61538) [details]
Proposed patch

After trying to add all the comments I've disagreed with myself and agreed with Mihnea about the limit :), adding it to the deviation was not as natural as using the radius.

This patch depends on the patch in the bug 41605. I'm not adding it for review until we land the other patch.
------- Comment #11 From 2010-08-11 02:19:19 PST -------
Created an attachment (id=64088) [details]
Proposed patch

The patch that blocked this one has landed so we can add this one for review, not sure if any of the pixel tests have to be modified.
------- Comment #12 From 2010-08-11 04:19:30 PST -------
Created an attachment (id=64093) [details]
Proposed patch

Not much information about the fail, added the .f in the max function parameter.
------- Comment #13 From 2010-08-11 05:05:11 PST -------
Created an attachment (id=64099) [details]
Proposed patch

And a new one trying to make both parameters float.
------- Comment #14 From 2010-08-11 05:31:16 PST -------
Attachment 64093 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3759061
------- Comment #15 From 2010-08-11 05:36:01 PST -------
Attachment 64093 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3754044
------- Comment #16 From 2010-08-13 08:38:21 PST -------
This bug affects all Cairo variants, not just GTK.
------- Comment #17 From 2010-08-18 12:11:21 PST -------
Created an attachment (id=64751) [details]
Proposed patch

Rebased patch to current trunk.
------- Comment #18 From 2010-08-18 12:53:37 PST -------
(From update of attachment 64751 [details])
WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:933
 +      FEGaussianBlur::calculateStdDeviation(blurRadius, FloatSize(1, 1), sdx, sdy);
See next comment.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:203
 +  void FEGaussianBlur::calculateStdDeviation(const FloatPoint& radius, const FloatSize& resolution, float& sdx, float& sdy)
Why did you add this? IIRC we just need the code for CSS shadows. We don't have different radii for width or height. The resolution is always FloatSize(1,1). So why not just add the following line once directly to the Shadow code in GC?

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:207
 +      sdy = max((radius.y() * 2 / 3 - 0.5) / (resolution.height() * gGaussianKernelFactor), 0.0);
should be
2 / 3.f
and
, 0.f);
------- Comment #19 From 2010-08-19 00:41:47 PST -------
(In reply to comment #18)
ยก> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:203
>  +  void FEGaussianBlur::calculateStdDeviation(const FloatPoint& radius, const FloatSize& resolution, float& sdx, float& sdy)
> Why did you add this? IIRC we just need the code for CSS shadows. We don't have different radii for width or height. The resolution is always FloatSize(1,1). So why not just add the following line once directly to the Shadow code in GC?
> 

The main point to add the function is keep all the gaussian calculation in the same file, it allow us to reuse calculations like the gGaussianKernelFactor for this and to get the kernel size in the apply method.

The reason to leave all the width, height even though they are the same for css is to keep the API like a general way to get the standard deviation from the radius. I could make the function more specific for this case if that fits better.
------- Comment #20 From 2010-08-19 01:39:16 PST -------
Created an attachment (id=64819) [details]
Proposed patch
------- Comment #21 From 2010-08-19 02:01:03 PST -------
Hm, FEGaussianBlur::calculateStdDeviation is a void but it's only task is to calculate a float, can't you just give back a float?
float sd = FEGaussianBlur::calculateStdDeviation(...)
Also please use: float FEGaussianBlur::calculateStdDeviation(float radius) const
------- Comment #22 From 2010-08-19 02:15:24 PST -------
(In reply to comment #21)
> Hm, FEGaussianBlur::calculateStdDeviation is a void but it's only task is to calculate a float, can't you just give back a float?
> float sd = FEGaussianBlur::calculateStdDeviation(...)
> Also please use: float FEGaussianBlur::calculateStdDeviation(float radius) const

Yep, I agree, makes more sense now.
------- Comment #23 From 2010-08-19 02:35:27 PST -------
Created an attachment (id=64823) [details]
Proposed patch

Modified the API of the calculateStdDeviation function.
------- Comment #24 From 2010-08-19 03:15:17 PST -------
(From update of attachment 64823 [details])
WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:931
 +      sd = FEGaussianBlur::calculateStdDeviation(radius);
avoid the new line, just:
float sd = FEGaussianBlur::calculateStdDeviation(radius);

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:934
 +      if (!radius || (!sd)) {
avoid the braces arround !sd

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:203
 +  float FEGaussianBlur::calculateStdDeviation(const float& radius)
No need for const float& . We usally just use float for normal floats.

WebCore/platform/graphics/filters/FEGaussianBlur.h:46
 +      static float calculateStdDeviation(const float& radius);
make it const please.

Small changes. But please fix them before landing. r=me
------- Comment #25 From 2010-08-19 03:47:46 PST -------
(In reply to comment #24)
> (From update of attachment 64823 [details] [details])
> WebCore/platform/graphics/filters/FEGaussianBlur.h:46
>  +      static float calculateStdDeviation(const float& radius);
> make it const please.
> 

I've just checked this function is already static so it can no be const.
------- Comment #26 From 2010-08-19 04:01:00 PST -------
landed r65661