Bug 48336 - Scaling of feTurbulence is miscalculated
Summary: Scaling of feTurbulence is miscalculated
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-26 09:35 PDT by Renata Hodovan
Modified: 2013-01-08 02:36 PST (History)
3 users (show)

See Also:


Attachments
Fix the calcluation of feTurbulence if it's scaled (2.89 KB, patch)
2010-10-26 10:27 PDT, Renata Hodovan
zimmermann: review-
Details | Formatted Diff | Diff
Example (707 bytes, image/svg+xml)
2010-10-27 11:39 PDT, Renata Hodovan
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Renata Hodovan 2010-10-26 09:35:17 PDT
Scaling of feTurbulence is miscalculated
Comment 1 Dirk Schulze 2010-10-26 09:39:39 PDT
Do you have an example that shows the problem? I couldn't see a problem right now.
Comment 2 Renata Hodovan 2010-10-26 10:27:42 PDT
Created attachment 71906 [details]
Fix the calcluation of feTurbulence if it's scaled
Comment 3 Renata Hodovan 2010-10-26 10:29:49 PDT
(In reply to comment #1)
> Do you have an example that shows the problem? I couldn't see a problem right now.
If you get any test of feTurbulence and resize the image, the turbulence filter doesn't scale but seems like you see the pic through a growing window.
Comment 4 Nikolas Zimmermann 2010-10-26 10:32:47 PDT
Comment on attachment 71906 [details]
Fix the calcluation of feTurbulence if it's scaled

Hey Reni,

this needs a test. You can copy any existing feTurbulence patch to svg/zoom/page, add the "var zoomPageCount..." etc. logic. That should be sufficient.
Going to leave the real review of the code changes to Dirk.

Cheers,
Niko
Comment 5 Dirk Schulze 2010-10-26 23:24:00 PDT
Comment on attachment 71906 [details]
Fix the calcluation of feTurbulence if it's scaled

View in context: https://bugs.webkit.org/attachment.cgi?id=71906&action=review

In general I don't see the great difference to the current implementation. It's definitely better not to map the point on every loop call, thats why I'm fine with the changes anyway (but please take a look at the subregion first), but you mentioned that you saw failures. I didn't so far. Can you please add a test that fails at the moment but doesn't with your patch? Please attache it on this br as well.

> WebCore/platform/graphics/filters/FETurbulence.cpp:31
> +#include "SVGFilter.h"

Why did you inclue SVGFilter? It's definitely wrong to use SVG code in WebCore/platform. This will also break some builds.

> WebCore/platform/graphics/filters/FETurbulence.cpp:332
> +    FloatRect subregion = filterPrimitiveSubregion();
> +    subregion.intersect(filter->filterRegionInUserSpace());

I'm not sure if this is correct, since I understand the algorihtm that it needs the subregion, unclipped. Can you please add more tests? I'd like to see a subregion that is bigger than filterRegion and x,y should positioned outside of the filterRegion (top left).

> WebCore/platform/graphics/filters/FETurbulence.cpp:338
> +    FloatRect absoluteScale = reinterpret_cast<SVGFilter*>(filter)->mapLocalRectToAbsoluteRect(subregion);

I'd use FloatSize for scales, you could just add a size() at the end.

> WebCore/platform/graphics/filters/FETurbulence.cpp:340
> +    float stepY = subregion.height() / absoluteScale.height();
> +    float stepX = subregion.width() / absoluteScale.width();

Please use FloatSize again here.
Comment 6 Renata Hodovan 2010-10-27 11:39:03 PDT
Created attachment 72062 [details]
Example

The two images should be equivalent except of their size
Comment 7 Dirk Schulze 2010-10-27 11:51:18 PDT
(In reply to comment #6)
> Created an attachment (id=72062) [details]
> Example
> 
> The two images should be equivalent except of their size

Checked it with a 4 days old build. The result looks to 100% identical to the results of Opera and Firefox.
Comment 8 Dirk Schulze 2011-09-07 01:33:08 PDT
Reni, could you check it again please? Still don't see any problems.
Comment 9 Renata Hodovan 2013-01-08 02:36:34 PST
It looks good to me now. Closed.