[Webkit-unassigned] [Bug 73643] SVG filters incorrectly move elements

Mon Dec 19 08:14:10 PST 2011

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

--- Comment #15 from Branimir Lambov <blambov at google.com>  2011-12-19 08:14:10 PST ---
(From update of attachment 118397)
View in context: https://bugs.webkit.org/attachment.cgi?id=118397&action=review

>> LayoutTests/ChangeLog:9
>> +        Reviewed by NOBODY (OOPS!).
> 
> This should be
> 
> bug title
> link
> 
> Reviewed by
> 
> Description

Done

>> LayoutTests/svg/clip-path/clipper-placement-issue.svg:2
>> +  style="fill-rule:evenodd;pointer-events:none;width:300px;height:400px;background:rgb(255,255,255)">
> 
> Remove unnecessary information. Just <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

Done.

>> LayoutTests/svg/clip-path/clipper-placement-issue.svg:7
>> +change with zooming. -->
> 
> Is it possible to simulate this with scaling? Otherwise you might take a look a the zooming and panning examples in the zoom folder.

Changed message.

>> LayoutTests/svg/filters/filter-placement-issue.svg:2
>> +  style="fill-rule:evenodd;pointer-events:none;width:300px;height:400px;background:rgb(255,255,255)">
> 
> Ditto.

Done.

>> LayoutTests/svg/filters/filter-placement-issue.svg:7
>> +change with zooming. -->
> 
> Ditto.

This comment is information for anyone that opens the file in the browser. The pixel test itself does not need to zoom to catch the problem-- before this patch the browser would show red lines at any zoom level.

>> Source/WebCore/ChangeLog:9
>> +        Reviewed by NOBODY (OOPS!).
> 
> See comment on LayoutTest change log.

Done

>> Source/WebCore/platform/graphics/filters/FETile.cpp:68

> 
> Can you add an early return for tileRect.isEmpty before creating the image please?

Removing all pattern-related changes from this CL as they require a different treatment of float-to-integer conversion (see bug 74851).

>> Source/WebCore/platform/graphics/filters/FETile.cpp:70
>> +    if (!tileImage)
> 
> Remove the extra line.

Removing all pattern-related changes from this CL as they require a different treatment of float-to-integer conversion (see bug 74851).

>> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-189
>> -        GraphicsContextStateSaver stateSaver(*maskContext);
> 
> I wrote a line about the save/restore pair later...

Unlike the one below, this pair is actually necessary for a clipPath applied to this clipPath to work. I restructured things a bit to make the bahaviour clearer.

This wasn't caught by the existing tests, so I added a new one.

>> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:201
>> +
> 
> Remove this line

Done.

>> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:233

> 
> I assume that you run all pixel tests? Have you checked this code with LayoutTests/svg/batik/filters/feTile.svg as well?

Yes, I do run all pixel tests. feTile is now better positioned but is otherwise identical (pattern changes are no longer included in this CL).

>> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:117
>> +    SVGImageBufferTools::clipToImageBuffer(context, absoluteTransform, targetRect, imageBuffer);
> 
> I wonder that we don't need the unclamped targetRect later? Can you describe this a bit more please?

The createImageBuffer/clipToImageBuffer pair handles all of the transformations required to paint correctly in the new buffer and to clip with the buffer at the correct position and scale. The coordinate mapping, snapping to the correct pixel boundary, and image size clamping are transparent to the user of the pair.

>> Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp:-116
>> -        maskImageContext->save();
> 
> I think I wrote it in the last review. This save/restore pair is needed for the clipToImageBuffer logic (masking in GraphicsContext). We mask everything with the mask image between this pair. Skia, Qt and Cairo relay on this pair. Are you sure that we can remove this now?

This would be necessary if clipping or masking is applied to the "mask" element, otherwise the rendering of the clipped/masked group will save/restore as necessary. However, the code currently completely ignores the "mask" and "clip-path" attributes of the "mask" element. The SVG spec is not very clear on that part, and this behaviour is consistent with what Opera and Firefox do.

The pair as it stands here is unnecessary and confusing; if masking a mask is fixed some time in the future, the code that applies the inner mask/clip-path/filter/etc. is the one that should apply the necessary context save and restore.

>> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:253
>> +    IntSize bufferSize((int) clampedAbsoluteTileBoundaries.width(), (int) clampedAbsoluteTileBoundaries.height());
> 
> Don't use (int) but rounding/floor/ceil.

Removing all pattern-related changes from this CL as they require a different treatment of float-to-integer conversion (see bug 74851).

>> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:145
>> +}
> 
> We have a function shrunkTo in IntSize. Please use that one and add a static IntSize(kMaxImageBufferSize, kMaxImageBufferSize).

Using shrunkTo a const non-static IntSize local, which should be optimized away by the compiler. Static IntSizes seem to require a static global constructor on MacOS and won't compile in release builds.

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