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

[bugzilla-daemon at webkit.org]

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


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #118397|review?                     |review-
               Flag|                            |




--- Comment #14 from Dirk Schulze <krit at webkit.org>  2011-12-10 01:22:30 PST ---
(From update of attachment 118397)
View in context: https://bugs.webkit.org/attachment.cgi?id=118397&action=review

Still some changes needed and I also have some questions.

> LayoutTests/ChangeLog:9
> +        [SVG] Fixed SVG image buffer creation to use the enclosing integer rectangle
> +        instead of unstable rounded coordinates with scaling which was causing
> +        animated images to jump around under filters. The change improves the
> +        positioning of clip paths, masks, patterns and filters.
> +        https://bugs.webkit.org/show_bug.cgi?id=73643
> +
> +        Reviewed by NOBODY (OOPS!).

This should be

bug title
link

Reviewed by

Description

> LayoutTests/svg/clip-path/clipper-placement-issue.svg:2
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
> +  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">

> LayoutTests/svg/clip-path/clipper-placement-issue.svg:7
> +<!-- This tests some filter placement oddities caused by rounding
> +(https://bugs.webkit.org/show_bug.cgi?id=73643).
> +When opened, the test should not show any thin red lines that
> +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.

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

Ditto.

> LayoutTests/svg/filters/filter-placement-issue.svg:7
> +<!-- This tests some filter placement oddities caused by rounding
> +(https://bugs.webkit.org/show_bug.cgi?id=73643).
> +When opened, the test should not show any thin red lines that
> +change with zooming. -->

Ditto.

> Source/WebCore/ChangeLog:9
> +        [SVG] Fixed SVG image buffer creation to use the enclosing integer rectangle
> +        instead of unstable rounded coordinates with scaling which was causing
> +        animated images to jump around under filters. The change improves the
> +        positioning of clip paths, masks, patterns and filters.
> +        https://bugs.webkit.org/show_bug.cgi?id=73643
> +
> +        Reviewed by NOBODY (OOPS!).

See comment on LayoutTest change log.

> Source/WebCore/platform/graphics/filters/FETile.cpp:68
> +    OwnPtr<ImageBuffer> tileImage = ImageBuffer::create(tileRect.size(), ColorSpaceDeviceRGB, filter()->renderingMode());

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

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

Remove the extra line.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-189
> -        GraphicsContextStateSaver stateSaver(*maskContext);

I wrote a line about the save/restore pair later...

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

Remove this line

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:233
> +    effectiveTransform.multiply(filterData->shearFreeAbsoluteTransform);

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

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:117
> +    targetRect = textRootBlock->repaintRectInLocalCoordinates();
> +    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?

> Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp:-116
> -        // The save/restore pair is needed for clipToImageBuffer - it doesn't work without it on non-Cg platforms.
> -        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?

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:253
> +    IntSize bufferSize((int) clampedAbsoluteTileBoundaries.width(), (int) clampedAbsoluteTileBoundaries.height());

Don't use (int) but rounding/floor/ceil.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:145
> +IntSize SVGImageBufferTools::clampedAbsoluteSize(const IntSize& absoluteSize)
> +{
> +    IntSize clampedAbsoluteSize = absoluteSize;
> +
> +    if (clampedAbsoluteSize.width() > kMaxImageBufferSize)
> +        clampedAbsoluteSize.setWidth(kMaxImageBufferSize);
> +
> +    if (clampedAbsoluteSize.height() > kMaxImageBufferSize)
> +        clampedAbsoluteSize.setHeight(kMaxImageBufferSize);
> +
> +    return clampedAbsoluteSize;
> +}

We have a function shrunkTo in IntSize. Please use that one and add a static IntSize(kMaxImageBufferSize, kMaxImageBufferSize).

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