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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 20 05:57:43 PST 2011


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





--- Comment #21 from Branimir Lambov <blambov at google.com>  2011-12-20 05:57:43 PST ---
 Thank you for the review. The patch is updated, see comments below.

> --- Comment #19 from Nikolas Zimmermann <zimmermann at kde.org>  2011-12-20 02:39:12 PST ---
> (From update of attachment 119995)
> 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

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


More information about the webkit-unassigned mailing list