[Webkit-unassigned] [Bug 41605] Fixes to the gaussian blur algorithm

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 7 04:44:20 PDT 2010


https://bugs.webkit.org/show_bug.cgi?id=41605





--- Comment #6 from Alejandro G. Castro <alex at igalia.com>  2010-07-07 04:44:20 PST ---
(In reply to comment #5)
> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:150
>  +      unsigned kernelSizeX = static_cast<unsigned>(floor(max(m_stdX, static_cast<float>(2.0)) * filter->filterResolution().width() * gGaussianKernelFactor + 0.5));
> Isn't it enough to write 2.f instead of static_cast<float>(2.0)? Please use 0.5f. Only if you have a whole-number, you can omit .0 or .f (not sure about the denominator on a division).
> 

2.f was the original code :), I modified it this way because it was part of your suggestions for the last patch.

> 
> To the LayoutTests.. You changes behavior, so you need some more layoutTests to cover the changes. I suggest a test, with a stdDerivation of "0", "0 3" and "3 0". According to the new spec, we should see results in all three cases (not the case at the moment).

I understand, are you talking about pixel tests or dump texts? I guess pixel tests are the best option in this case, in that case we can upload them without baseline so they appear as new to the ports doing pixel tests. BTW, Martin is trying to get the patch adding gtk pixel tests, hopefully we could have them soon :).

> You changed the result of one of the tests, so that other platforms don't need new test results? That's not the regular case to do so :-)

The test that I added to gtk was skipped, it had no result, it was a dump test not pixel test, and the expected result does not change before or after the patch, so it could be the same for the other port. The other option is to skip those tests and upload a bug so the people from the other ports have the opportunity to check it and we do not risk to break the bots.

> If you don't have a Mac, or no access to a Mac, I can create the results for you.

If you could do that for me I'd owe you a beer :). Honestly I'm not sure the best option in these cases, I see a lot of people skipping tests from other ports and I seems kind of sane to me, the other option of having all the platforms each developer looks like not very efficient to me, but I'm no

> The patch misses a ChangeLog entry for LayoutTests.
> 

Oops :).

> Great patch! Hope to see an update soon. r- for the few style issues and the missing LayoutTests/Changelog-entry.

Thanks for the prompt and helpful reply. I'll review and upload a new revision.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list