Bug 73643 – SVG filters incorrectly move elements

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Branimir Lambov]

Created attachment 117603 [details]
Sample

Chrome Version       : 16.0.912.36
OS Version: Goobuntu
URLs (if applicable) :
Other browsers tested:
  Add OK or FAIL after other browsers where you have tested this issue:
 Chrome MacOS: FAIL
     Safari 5: SVG filters not available
    Firefox 8: OK

What steps will reproduce the problem?
Create an animated SVG element and a filter over it.

What is the expected result?
The element's movement and size should not be affected by the filter.

What happens instead?
The element annoyingly jumps around and stretches by a small amount.

The attached file has some text under animated filters (in red and green), where the x, y, width or height attributes of the filter change. None of these should have any effect on the positioning of the text, but the text jumps about quite a lot. The last column of the example has only the text moving, and the filter still causes it to jump badly. 

The top line on the right shows the text moving without a filter, where the problem is not present.


UserAgentString: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.36 Safari/535.7

[Tim Horton]

Also occurs on WebKit nightlies with Safari.

[Branimir Lambov]

Created attachment 118020 [details]
Static test demonstrating the problem

[Branimir Lambov]

Created attachment 118061 [details]
Proposed fix

[Branimir Lambov]

Created attachment 118064 [details]
Proposed fix

[Branimir Lambov]

Created attachment 118071 [details]
Proposed fix

[WebKit Review Bot]

Attachment 118071 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1

Source/WebCore/platform/graphics/filters/FETile.cpp:75:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 118071 [details]
Proposed fix

Attachment 118071 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10745016

[Branimir Lambov]

Created attachment 118088 [details]
Proposed fix

[Branimir Lambov]

Created attachment 118208 [details]
SVG test expectations update

[Dirk Schulze]

Comment on attachment 118088 [details]
Proposed fix

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

This is just a code review. I have to check the functionality again in a second step.

> LayoutTests/ChangeLog:11
> +        * svg/filters/filter-placement-issue.svg: Added.

Is this a manual test? You miss the results for this test otherwise (I'd like to have an automated test with pixel result). Also, do you have examples for clipping and masking at well? Won't this code influence existing results? Don't you need to update pixel results?

> Source/WebCore/platform/graphics/filters/FETile.cpp:58
> +    FloatRect tileRect = enclosingIntRect(in->maxEffectRect());

Why don't you make it an IntRect?

> Source/WebCore/platform/graphics/filters/FETile.cpp:67
> +    // TODO: Clamp image size?

This should be a statement and use FIXME. Also, is it correct to remove the SVGImageBufferTools::createImageBuffer call? Is the following code a complete replacement of the functionality?

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

the (int)'s would be unnecessary if you use IntRect, same for IntSize.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-189
> -        // The save/restore pair is needed for clipToImageBuffer - it doesn't work without it on non-Cg platforms.
> -        GraphicsContextStateSaver stateSaver(*maskContext);

You seem to ignore this comment. This is not addressed somewhere later. This code was added for Cairo and Skia ports.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:231
>      absoluteDrawingRegion.scale(scale.width(), scale.height());

Do we still need this rect?

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:251
> +    // Context is now set up to draw in local coordinates. This shouldn't work.

You should check this first. I don't think that such a comment should be in trunk :)

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:252
> +    // sourceGraphicContext->clearRect(FloatRect(FloatPoint(), absoluteDrawingRegion.size()));

Don't use commented out code. Please be sure that this works correctly and why it works.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:72
> +    // This is done in absolute coordinates

Please use sentences and a dot at the end of a phrase.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:77
> +    // This happens in local coordinates

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:78
> +    imageContext->scale(FloatSize(((float)clampedSize.width()) / paintRect.width(), ((float)clampedSize.height()) / paintRect.height()));

you don't check if paintRect can be empty (width or height is zero).

> 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;
> +}

I'm sure that we have this code somewhere else. Did you copy this? We shouldn't have duplicated code.

> Source/WebCore/rendering/svg/SVGImageBufferTools.h:48
> +    static IntRect calcImageBufferRect(const FloatRect& targetRect, const AffineTransform& absoluteTransform)
> +    { return enclosingIntRect(absoluteTransform.mapRect(targetRect)); };

This is the wrong style, please use multiple lines for a function, even on headers.

[Dirk Schulze]

Comment on attachment 118088 [details]
Proposed fix

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

This is just a code review. I have to check the functionality again in a second step.

> LayoutTests/ChangeLog:11
> +        * svg/filters/filter-placement-issue.svg: Added.

Is this a manual test? You miss the results for this test otherwise (I'd like to have an automated test with pixel result). Also, do you have examples for clipping and masking at well? Won't this code influence existing results? Don't you need to update pixel results?

> Source/WebCore/platform/graphics/filters/FETile.cpp:58
> +    FloatRect tileRect = enclosingIntRect(in->maxEffectRect());

Why don't you make it an IntRect?

> Source/WebCore/platform/graphics/filters/FETile.cpp:67
> +    // TODO: Clamp image size?

This should be a statement and use FIXME. Also, is it correct to remove the SVGImageBufferTools::createImageBuffer call? Is the following code a complete replacement of the functionality?

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

the (int)'s would be unnecessary if you use IntRect, same for IntSize.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-189
> -        // The save/restore pair is needed for clipToImageBuffer - it doesn't work without it on non-Cg platforms.
> -        GraphicsContextStateSaver stateSaver(*maskContext);

You seem to ignore this comment. This is not addressed somewhere later. This code was added for Cairo and Skia ports.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:231
>      absoluteDrawingRegion.scale(scale.width(), scale.height());

Do we still need this rect?

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:251
> +    // Context is now set up to draw in local coordinates. This shouldn't work.

You should check this first. I don't think that such a comment should be in trunk :)

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:252
> +    // sourceGraphicContext->clearRect(FloatRect(FloatPoint(), absoluteDrawingRegion.size()));

Don't use commented out code. Please be sure that this works correctly and why it works.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:72
> +    // This is done in absolute coordinates

Please use sentences and a dot at the end of a phrase.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:77
> +    // This happens in local coordinates

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:78
> +    imageContext->scale(FloatSize(((float)clampedSize.width()) / paintRect.width(), ((float)clampedSize.height()) / paintRect.height()));

you don't check if paintRect can be empty (width or height is zero).

> 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;
> +}

I'm sure that we have this code somewhere else. Did you copy this? We shouldn't have duplicated code.

> Source/WebCore/rendering/svg/SVGImageBufferTools.h:48
> +    static IntRect calcImageBufferRect(const FloatRect& targetRect, const AffineTransform& absoluteTransform)
> +    { return enclosingIntRect(absoluteTransform.mapRect(targetRect)); };

This is the wrong style, please use multiple lines for a function, even on headers.

[Branimir Lambov]

Created attachment 118397 [details]
Patch

[Branimir Lambov]

Thank you for your review. The patch is updated, please see comments below.

On Thu, Dec 8, 2011 at 7:48 AM, <[e-mail]> wrote:
>
> https://bugs.webkit.org/show_bug.cgi?id=73643
>
>
> Dirk Schulze <[e-mail]> changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>  Attachment #118088 [details]|review?                     |review-
>               Flag|                            |
>
>
>
>
> --- Comment #10 from Dirk Schulze <[e-mail]>  2011-12-07 23:48:05 PST ---
> (From update of attachment 118088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118088&action=review
>
> This is just a code review. I have to check the functionality again in a second step.
>
> > LayoutTests/ChangeLog:11
> > +        * svg/filters/filter-placement-issue.svg: Added.
>
> Is this a manual test? You miss the results for this test otherwise (I'd like to have an automated test with pixel result). Also, do you have examples for clipping and masking at well? Won't this code influence existing results? Don't you need to update pixel results?

I was having a bit of trouble the patch upload system and sent the expectations update as a separate upload. It is now included in the patch. 

A clipping and masking test is added. I will rebaseline the Chrome versions of these tests after the patch is landed.
>
>
> > Source/WebCore/platform/graphics/filters/FETile.cpp:58
> > +    FloatRect tileRect = enclosingIntRect(in->maxEffectRect());
>
> Why don't you make it an IntRect?

Changed. 
>
>
> > Source/WebCore/platform/graphics/filters/FETile.cpp:67
> > +    // TODO: Clamp image size?
>
> This should be a statement and use FIXME. Also, is it correct to remove the SVGImageBufferTools::createImageBuffer call? Is the following code a complete replacement of the functionality?

Yes, it is a complete replacement as used by FETile (i.e. without clamping). Removing this reference is required to make sure non-SVG targets work properly.

Comment changed.
>
>
> > Source/WebCore/platform/graphics/filters/FETile.cpp:68
> > +    OwnPtr<ImageBuffer> tileImage = ImageBuffer::create(IntSize((int)tileRect.width(), (int)tileRect.height()), ColorSpaceDeviceRGB, filter()->renderingMode());
>
> the (int)'s would be unnecessary if you use IntRect, same for IntSize.

Changed. 
>
>
> > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-189
> > -        // The save/restore pair is needed for clipToImageBuffer - it doesn't work without it on non-Cg platforms.
> > -        GraphicsContextStateSaver stateSaver(*maskContext);
>
> You seem to ignore this comment. This is not addressed somewhere later. This code was added for Cairo and Skia ports.

The saving of the context here does not affect the placement of the clipping/masking rectangle (it applies to the mask context, while it should be done on the parent one).

Clipping and masking do not currently work on Skia, due to incorrect treatment of transformations (see https://bugs.webkit.org/show_bug.cgi?id=53378). That problem is properly solved by the patch attached to that bug.
>
>
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:231
> >      absoluteDrawingRegion.scale(scale.width(), scale.height());
>
> Do we still need this rect?

Removed. 
>
>
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:251
> > +    // Context is now set up to draw in local coordinates. This shouldn't work.
>
> You should check this first. I don't think that such a comment should be in trunk :)

Yes, I have tested that it does work correctly (on Mac and Chrome Linux) without the call. As it was set up, the clearRect call had no effect.
>
>
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:252
> > +    // sourceGraphicContext->clearRect(FloatRect(FloatPoint(), absoluteDrawingRegion.size()));
>
> Don't use commented out code. Please be sure that this works correctly and why it works.

Removed. 
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:72
> > +    // This is done in absolute coordinates
>
> Please use sentences and a dot at the end of a phrase.

Done. 
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:77
> > +    // This happens in local coordinates
>
> Ditto.

Done. 
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:78
> > +    imageContext->scale(FloatSize(((float)clampedSize.width()) / paintRect.width(), ((float)clampedSize.height()) / paintRect.height()));
>
> you don't check if paintRect can be empty (width or height is zero).

This was done by the clampedSize.isEmpty() call above, but I agree checking for the original paintRect being empty makes more sense. Changed.
>
>
> > 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;
> > +}
>
> I'm sure that we have this code somewhere else. Did you copy this? We shouldn't have duplicated code.

This is an integer version of clampedAbsoluteTargetRect above, applied to IntSize. It wouldn't be efficient or entirely correct to force one to call the other (unless you prefer to have clampedAbsoluteSize as a template function in the header; please let me know if that's the case and I'll be happy to change it).
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.h:48
> > +    static IntRect calcImageBufferRect(const FloatRect& targetRect, const AffineTransform& absoluteTransform)
> > +    { return enclosingIntRect(absoluteTransform.mapRect(targetRect)); };
>
> This is the wrong style, please use multiple lines for a function, even on headers.

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




--

Regards,
Branimir

[Dirk Schulze]

Comment on attachment 118397 [details]
Patch

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).

[Branimir Lambov]

Comment on attachment 118397 [details]
Patch

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.

[Branimir Lambov]

Created attachment 119866 [details]
Proposed fix

[WebKit Review Bot]

Comment on attachment 119866 [details]
Proposed fix

Attachment 119866 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10915033

New failing tests:
http/tests/inspector-enabled/dedicated-workers-list.html
svg/filters/filter-placement-issue.svg
svg/clip-path/clipper-placement-issue.svg
media/video-poster-blocked-by-willsendrequest.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
svg/filters/big-sized-filter.svg
css2.1/20110323/abspos-containing-block-initial-004d.htm
http/tests/inspector/resource-tree/resource-tree-document-url.html
svg/filters/filterRes2.svg

[Branimir Lambov]

Created attachment 119995 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 119995 [details]
Patch

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

Some comments regarding the newest patch:

> LayoutTests/svg/clip-path/clipper-placement-issue.svg:21
> +    <!-- clipPath doesn't like use, so we copy the paths here. -->

Hm, this should work just fine, if not you've found a bug. Can you elaborate on this?

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

change with zooming? You're not testing zooming here?

> LayoutTests/svg/filters/filter-placement-issue.svg:58
> +  <rect x="135.9" y="55.9" width="8" height="30" />
> +  <rect x="151" y="56" width="8" height="30" />

... these all have the same 8x30 size, I guess you meant "changes with the position of the target element"?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:190
> +        if (resources && (clipper = resources->clipper())) {

This looks awkward.
I'd propose:
if (RenderSVGResourceClipper* clipper = resources ? resources->clipper() : 0) {
    ...
}

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

A comment doesn't hurt here :-)

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:63
> +

This newline can go away.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:69
> +

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:74
> +

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:78
> +    imageContext->scale(FloatSize(((float)clampedSize.width()) / paintRect.width(), ((float)clampedSize.height()) / paintRect.height()));

Don't use c-style casts, use an explicit static_cast<float>(..) if you need it.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:130
> +    FloatRect absoluteTargetRect = calcImageBufferRect(targetRect, absoluteTransform);

Please don't use abbreviations, "calculateImageBufferRect". (Note that 'Rect' is okay, you don't need to expand it to 'Rectangle', after all FloatRect isn't named FloatRectangle...) It's inconsistent, but we have to live with it.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:146
> +    const FloatSize maxImageBufferSize(kMaxImageBufferSize, kMaxImageBufferSize);

I don't think adding const gives you a benefit here, I'd rather make it static.
DEFINE_STATIC_LOCAL(FloatSize, maxImageBufferSize, (kMaxImageBuferSize, ..));

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:147
> +    return FloatRect(absoluteTargetRect.location(), absoluteTargetRect.size().shrunkTo(maxImageBufferSize));

hm, can't you reuse clampedAbsoluteSize?
IMHO clampedAbsoluteTargetRect should use clampedAbsoluteSize with FloatSizes, there could be another variant of clampedAbsoluteSize taking IntSizes, but using the FloatSize code-path, with a "enclosingIntSize".

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:152
> +    const IntSize maxImageBufferSize(kMaxImageBufferSize, kMaxImageBufferSize);

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.h:47
> +     static IntSize clampedAbsoluteSize(const IntSize& absoluteSize);

One leading space too much, and you don't need the "absoluteSize" parameter name here.

[Branimir Lambov]

Created attachment 120013 [details]
Patch

[Branimir Lambov]

 Thank you for the review. The patch is updated, see comments below.

> --- Comment #19 from Nikolas Zimmermann <[e-mail]>  2011-12-20 02:39:12 PST ---
> (From update of attachment 119995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119995&action=review
>
> Some comments regarding the newest patch:
>
> > LayoutTests/svg/clip-path/clipper-placement-issue.svg:21
> > +    <!-- clipPath doesn't like use, so we copy the paths here. -->
>
> Hm, this should work just fine, if not you've found a bug. Can you elaborate on this?

 The behaviour is correct. I needed to reference a <g> element, which is not allowed according to the spec:
 "If a ‘use’ element is a child of a ‘clipPath’ element, it must directly reference ‘path’, ‘text’ or basic shape elements."

>
> > LayoutTests/svg/filters/filter-placement-issue.svg:6
> > +change with zooming. -->
>
> change with zooming? You're not testing zooming here?

 No, I am not. The error will be caught by the pixel test in any zoom level. A different zoom level would still fail, but differently. This comment describes what the failing output is, and is meant to help anyone that is trying to understand whether or not a failure is a genuine reoccurrence of the problem or something else.

 Successful output has no red lines whatsoever.
>
>
> > LayoutTests/svg/filters/filter-placement-issue.svg:58
> > +  <rect x="135.9" y="55.9" width="8" height="30" />
> > +  <rect x="151" y="56" width="8" height="30" />
>
> ... these all have the same 8x30 size, I guess you meant "changes with the position of the target element"?

 Well, when they are rendered on screen they are not the same size. Because pixels are discrete, most of these rects are not 8x30 on screen (most grow to 9x31). What they actually end up as depends on the integer value of the absolute coordinates with respect to the buffer the object is drawn in.

 This is the problem this patch solves: if the filter/clip-path/mask buffer is not aligned on a pixel boundary, the positions and sizes of the elements drawn in that buffer changes by 1-2 pixels compared to the same elements drawn without an intermediate buffer, because they round differently. The effect was made even worse by the scaling that the existing code applied to try to compensate for the rounding.

>
> > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:190
> > +        if (resources && (clipper = resources->clipper())) {
>
> This looks awkward.
> I'd propose:
> if (RenderSVGResourceClipper* clipper = resources ? resources->clipper() : 0) {
>    ...
> }

 Done. 

>
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:233
> > +    AffineTransform effectiveTransform;
> > +    effectiveTransform.scale(scale.width(), scale.height());
> > +    effectiveTransform.multiply(filterData->shearFreeAbsoluteTransform);
>
> A comment doesn't hurt here :-)

 Done.
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:63
> > +
>
> This newline can go away.

 Done.
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:69
> > +
>
> Ditto.

 Done.
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:74
> > +
>
> Ditto.

 Leaving this, as it indicates the line that the comment above refers to.
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:78
> > +    imageContext->scale(FloatSize(((float)clampedSize.width()) / paintRect.width(), ((float)clampedSize.height()) / paintRect.height()));
>
> Don't use c-style casts, use an explicit static_cast<float>(..) if you need it.

 Done.
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:130
> > +    FloatRect absoluteTargetRect = calcImageBufferRect(targetRect, absoluteTransform);
>
> Please don't use abbreviations, "calculateImageBufferRect". (Note that 'Rect' is okay, you don't need to expand it to 'Rectangle', after all FloatRect isn't named FloatRectangle...) It's inconsistent, but we have to live with it.

 Done.
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:146
> > +    const FloatSize maxImageBufferSize(kMaxImageBufferSize, kMaxImageBufferSize);
>
> I don't think adding const gives you a benefit here, I'd rather make it static.
> DEFINE_STATIC_LOCAL(FloatSize, maxImageBufferSize, (kMaxImageBuferSize, ..));

 You are right that 'const' does not have any effect here, but I would strongly prefer not to declare a static local. As the FloatSize constructor is trivial there is a very high chance the use of the local will be optimized away by the compiler, while a static would almost certainly incur a per-call overhead.
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:147
> > +    return FloatRect(absoluteTargetRect.location(), absoluteTargetRect.size().shrunkTo(maxImageBufferSize));
>
> hm, can't you reuse clampedAbsoluteSize?
> IMHO clampedAbsoluteTargetRect should use clampedAbsoluteSize with FloatSizes, there could be another variant of clampedAbsoluteSize taking IntSizes, but using the FloatSize code-path, with a "enclosingIntSize".

 The behaviour is quite different between enclosing clamped float rects and clamping enclosing int rects. The only way I could safely reuse the code is by using a template.

 The clampedAbsoluteRect method is only required for pattern buffers. I am hoping to be able to get rid of it in a separate patch that should improve pattern placement (to address https://bugs.webkit.org/show_bug.cgi?id=74851).
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:152
> > +    const IntSize maxImageBufferSize(kMaxImageBufferSize, kMaxImageBufferSize);
>
> Ditto.

 See above.
  
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.h:47
> > +     static IntSize clampedAbsoluteSize(const IntSize& absoluteSize);
>
> One leading space too much, and you don't need the "absoluteSize" parameter name here.

 Done.
  

--

Regards,
Branimir

[Nikolas Zimmermann]

Comment on attachment 120013 [details]
Patch

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

Looks great, r=me!

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:232
> +    // Change the coordinate transformation applied to the filtered element to reflect the
> +    // resolution of the filter.

This should be turned into one line.

[WebKit Review Bot]

Comment on attachment 120013 [details]
Patch

Rejecting attachment 120013 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
m 'LayoutTests/platform/chromium-linux/svg/filters/filterRes-expected.txt'
patching file LayoutTests/platform/chromium/test_expectations.txt
Hunk #1 succeeded at 3952 (offset -36 lines).
Original content of LayoutTests/platform/mac-snowleopard/svg/W3C-SVG-1.1/filters-morph-01-f-expected.png mismatches at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 261.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Nikolas Zimmermann', u..." exit_code: 9

Full output: http://queues.webkit.org/results/10996301

[Branimir Lambov]

Created attachment 120336 [details]
Resynced patch

[Nikolas Zimmermann]

Comment on attachment 120336 [details]
Resynced patch

Ouch, this is still not in! I fear it won't apply anymore - but we can try. Can you update the patch if possible?

[WebKit Review Bot]

Comment on attachment 120336 [details]
Resynced patch

Rejecting attachment 120336 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
s/Scripts/svn-apply', u'--reviewer', u'Nikolas Zimmermann', u..." exit_code: 9

Parsed 76 diffs from patch file(s).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
Original content of LayoutTests/platform/chromium-linux/svg/W3C-SVG-1.1/filters-color-01-b-expected.png mismatches at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 261.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Nikolas Zimmermann', u..." exit_code: 9

Full output: http://queues.webkit.org/results/11297134

[Branimir Lambov]

Created attachment 123734 [details]
Resynced patch

[Nikolas Zimmermann]

Comment on attachment 123734 [details]
Resynced patch

r=me!

[WebKit Review Bot]

Comment on attachment 123734 [details]
Resynced patch

Rejecting attachment 123734 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ED at 105.
Hunk #3 succeeded at 156 (offset 5 lines).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp.rej
patching file Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp
patching file Source/WebCore/rendering/svg/SVGImageBufferTools.cpp
patching file Source/WebCore/rendering/svg/SVGImageBufferTools.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Nikolas Zi..." exit_code: 1

Full output: http://queues.webkit.org/results/11297562

[Branimir Lambov]

Created attachment 124123 [details]
Resynced patch

Resynced again. 

There are some small code changes in ResourceSVGClipper and ResourceSVGMasker to accommodate the patch for bug #69714.

[Nikolas Zimmermann]

Comment on attachment 124123 [details]
Resynced patch

Next try... I'll go and watch the bots.

[WebKit Review Bot]

Comment on attachment 124123 [details]
Resynced patch

Rejecting attachment 124123 [details] from commit-queue.

New failing tests:
svg/filters/big-sized-filter.svg
svg/wicd/test-scalable-background-image1.xhtml
svg/as-background-image/svg-as-background-2.html
Full output: http://queues.webkit.org/results/11314968

[Nikolas Zimmermann]

(In reply to comment #32)
> (From update of attachment 124123 [details])
> Rejecting attachment 124123 [details] from commit-queue.
> 
> New failing tests:
> svg/filters/big-sized-filter.svg
> svg/wicd/test-scalable-background-image1.xhtml
> svg/as-background-image/svg-as-background-2.html
> Full output: http://queues.webkit.org/results/11314968

I'm testing this patch locally now, and look at the problem.

[Nikolas Zimmermann]

Okay, I had to generate several results with Lion that were missed before and updated the LayoutTests/ChangeLog which was not up2date. I hope the chromium expectation changes you generated are correct, I didn't touch those, except for two obvious errors, of files that you rebaselined, where you shouldn't generating two of the cr-linux ews failures. (svg/wicd/test-scalable-background-image1.xhtml and svg/as-background-image/svg-as-background-2.html). The big-sized-filter.svg may still fail.

I've landed this manually in r106108. It will need rebaselines on all platforms (except mac, and hopefully cr), I hope someone can help me as well :-)

[Nikolas Zimmermann]

Comment on attachment 124123 [details]
Resynced patch

Gtk/Qt/Mac are all fine with this patch, I'm closing this bug.

[Nikolas Zimmermann]

Nice job Branimir, thanks for your patience with this!

[Philip Rogers]

Created attachment 135689 [details]
Rebaseline some tests

[Philip Rogers]

Created attachment 135699 [details]
Rebaseline some tests

[Philip Rogers]

Created attachment 135830 [details]
Another rebaseline update since the bots are back online now.

[Stephen Chenney]

Comment on attachment 135830 [details]
Another rebaseline update since the bots are back online now.

Clearing flags on attachment: 135830

Committed r113322: <http://trac.webkit.org/changeset/113322>

[Stephen Chenney]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

Created attachment 135838 [details]
Last expectations update

[Stephen Chenney]

Committed r113463: <http://trac.webkit.org/changeset/113463>