[webkit-reviews] review denied: [Bug 73643] SVG filters incorrectly move elements : [Attachment 118397] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Branimir Lambov
<blambov at google.com>'s request for review:
Bug 73643: SVG filters incorrectly move elements
https://bugs.webkit.org/show_bug.cgi?id=73643

Attachment 118397: Patch
https://bugs.webkit.org/attachment.cgi?id=118397&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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;backgroun
d: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;backgroun
d: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).