Bug 40793 – [Cairo] The size of the shadow image uses the standard deviation size instead of the blur radius

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

[Alejandro G. Castro]

Created attachment 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

[Alejandro G. Castro]

Created attachment 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 :).

[Alejandro G. Castro]

Created attachment 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

[WebKit Review Bot]

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.

[Alejandro G. Castro]

Created attachment 60298 [details]
Proposed patch

Fixed style issues

[Dirk Schulze]

Comment on attachment 60298 [details]
Proposed patch

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.

[Alejandro G. Castro]

(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 :).

[Dirk Schulze]

(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 :-)

[Mihnea Ovidenie]

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

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

Created attachment 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.

[Alejandro G. Castro]

Created attachment 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.

[Alejandro G. Castro]

Created attachment 64093 [details]
Proposed patch

Not much information about the fail, added the .f in the max function parameter.

[Alejandro G. Castro]

Created attachment 64099 [details]
Proposed patch

And a new one trying to make both parameters float.

[WebKit Review Bot]

Attachment 64093 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3759061

[WebKit Review Bot]

Attachment 64093 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3754044

[Brent Fulgham]

This bug affects all Cairo variants, not just GTK.

[Alejandro G. Castro]

Created attachment 64751 [details]
Proposed patch

Rebased patch to current trunk.

[Dirk Schulze]

Comment on attachment 64751 [details]
Proposed patch

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);

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

Created attachment 64819 [details]
Proposed patch

[Dirk Schulze]

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

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

Created attachment 64823 [details]
Proposed patch

Modified the API of the calculateStdDeviation function.

[Dirk Schulze]

Comment on attachment 64823 [details]
Proposed patch

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

[Alejandro G. Castro]

(In reply to comment #24)
> (From update of attachment 64823 [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.

[Alejandro G. Castro]

landed r65661