Bug 144335 – Applying a filter on an SVG element, which is larger than 4096 pixels, causes this element to be rendered shifted to the left

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Created attachment 251853 [details]
test case

Open the attached test case. Maximize the screen resolution and the window as well.

Result: The two rectangles are shifted to the left.
Expected: The two rectangles should have the same size and the gap between them should be centered in the window.

Notes:
1. If the filter is removed from the group, the bug does not happen.
2. If the left rectangle starts at 0 instead of -1000 and its with becomes 900 instead of 1900, the bug does not happen.

[Said Abou-Hallawa]

<rdar://problem/19849167>

[Said Abou-Hallawa]

Created attachment 251860 [details]
test case

[Said Abou-Hallawa]

Created attachment 252210 [details]
another test case

This test case is even worse, nothing is displayed in the window unless, the window width is really small.

[Said Abou-Hallawa]

When applying a filter on an SVG element and one of the input effects of this element is the SourceGraphic, this is what happens to render the element and the applied filter:

1- An ImageBuffer is created with the size of this element. This ImageBuffer is called the sourceGraphicBuffer and it's created by calling SVGRenderingContext::createImageBuffer().
2- The GraphicContext of this image is transformed such that the render object can be drawn on the image without modifying its coordinates.
3- The GraphicContext of the paintInfo is switched to the created image.
4- The SVG render element is called to paint itself.
5- After finishing the drawing of the element, the FilterEffects are applied in RenderSVGResourceFilter::postApplyResource(). The FilterEffect inputs are passed as ImageBuffers and its output is also an ImageBuffer.
6- As an example, SourceGraphic::platformApplySoftware() has the sourceGraphicBuffer as the only input. All what it does is creating the result ImageBuffer and drawing the sourceGraphicBuffer to it.
7- The ImageBuffer of the lastEffect is drawImageBuffer on the painting GraphicContext in the same rectangle as the original SVG element.
           
The steps 1-4 are done almost the same way by the clipper and the masker. They both call SVGRenderingContext::clipToImageBuffer() to transfer the clipped sourceGraphicBuffer to the painting context.
           
For some reason, clamping code was added to SVGRenderingContext::createImageBuffer() such that the size of the image can't exceed 4096x4096 pixels. Because of this clamping we had to transform the GraphicContext so the drawing will be scaled down to the clamped ImageBuffer. The implementation was not complete for the following reasons:

-- The order of the transformation in SVGRenderingContext::createImageBuffer() is incorrect. Every time a transformation is applied, the following transformation has to use units in the current coordinates system. The order should be the following:
  a) Change the origin of the coordinate system of the ImageBuffer to match the painting GraphicContext. This is done by translate(-paintRect.x(), -paintRect.x()) since paintRect is in absolute coordinates.
  b) Change the coordinates system from absolute to local by concatCTM(absoluteTransform).
  c) Apply the clamping scaling.
So scaling down the coordinates system of the ImageBuffer was wrong because the paintRect is not in the clamped coordinates system.
        
-- The clamping transform was not propagated correctly to do the mapping from the clamped ImageBuffer to the painting GraphicContext. As an example look at RenderSVGResourceClipper::applyClippingToContext(). This function does the steps 1-4 above and then calls SVGRenderingContext::clipToImageBuffer(). The later function has no idea whether the clipperMaskImage is clamped or not. If it is clamped then it should used the clamping scaling in translating the ImageBufferRect. And also the painting GraphicContext has to scale up so the ImageBuffer is enlarged to fill the element painting rectangle. And this was not happening.
   
-- Although we clamp the ImageBuffer sourceGraphicBuffer, we do not take that into consideration when applying the FilterEffect. SourceGraphic::platformApplySoftware() just copy the clamped sourceGraphicBuffer without scaling it up so we end up having part of the SourceGraphic FilterEffect not drawn

-- We do not clamp any of the FilterEffect. See FilterEffect::createImageBufferResult(). In this function we call ImageBuffer::create() and pass the original size. So the clamping was done partially for the SourceGraphic but it was not done for any of the other FilterEffects.  
           
So it seems, this clamping code should be deleted since it has many defects and it's not complete. It has also the drawback of losing the SVG drawing quality because of scaling  the GraphicContext down and then scaling it up.

[Said Abou-Hallawa]

Created attachment 252213 [details]
Patch

[Said Abou-Hallawa]

Created attachment 252214 [details]
Patch

[Simon Fraser (smfr)]

I think we should look at the history of that clamping code, to understand why it was added.

[Darin Adler]

Comment on attachment 252214 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FETile.cpp:66
> +    std::unique_ptr<ImageBuffer> tileImage = SVGRenderingContext::createImageBuffer(tileRect.size(), ColorSpaceDeviceRGB, filter().renderingMode());

I think auto is better for the return value here.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:206
> +    sourceGraphic = SVGRenderingContext::createImageBuffer(filterData->drawingRegion, effectiveTransform, ColorSpaceLinearRGB, renderingMode);

Should just use auto here instead of defining sourceGraphics on a separate line above.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:65
> +    std::unique_ptr<ImageBuffer> maskImage = SVGRenderingContext::createImageBuffer(repaintRect, absoluteTransform, ColorSpaceDeviceRGB, Unaccelerated);

I think auto is better for the return value here.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:97
> +    std::unique_ptr<ImageBuffer> tileImage = createTileImage(m_attributes, tileBoundaries, absoluteTileBoundaries, tileImageTransform);

I think auto is better for the return value here.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:235
> +    std::unique_ptr<ImageBuffer> tileImage = SVGRenderingContext::createImageBuffer(absoluteTileBoundaries.size(), ColorSpaceDeviceRGB, Unaccelerated);

I think auto is better for the return value here.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:253
> +    std::unique_ptr<ImageBuffer> imageBuffer = SVGRenderingContext::createImageBuffer(paintRect.size(), colorSpace, renderingMode);

I think auto is better for the return value here.

[Darin Adler]

(In reply to comment #7)
> I think we should look at the history of that clamping code, to understand
> why it was added.

That’s a good point. Limits like this can sometimes be necessary to get acceptable performance and to prevent memory problems. Simply removing the limits might not be a good idea.

[Said Abou-Hallawa]

Created attachment 252317 [details]
95197-correct-fix

[Said Abou-Hallawa]

The revision http://trac.webkit.org/changeset/126993 is related to this bug. In this revision, the order of setting the ImageBuffer GraphicContext transformations was changed. I agree there was a bug about the clamping but I believe also this revision was wrong and incomplete. The original transformation order was right.

I am attaching a patch which I think can fix the issue of the bug https://bugs.webkit.org/show_bug.cgi?id=95197. Yet, the fix is still incomplete because, as I mentioned above, the SourceGraphic FilterEffect does not take the clamping into account when copying from the from the sourceGraphicBuffer to its result image buffer. This patch complies and passes all the SVG layout tests in particular the test: LayoutTests/svg/clip-path/mask-nested-clip-path-010.svg. This test was originally added with r126993 with the name svg/custom/clamped-masking-clipping.svg but later its name was changed to be LayoutTests/svg/clip-path/mask-nested-clip-path-010.svg. The idea of this patch is we need to propagate the clamping transformation when copying from the sourceGraphicBuffer.

[Said Abou-Hallawa]

http://trac.webkit.org/changeset/106108 is a related revision in particular the changes in http://trac.webkit.org/changeset/106108/trunk/Source/WebCore/rendering/svg/SVGImageBufferTools.cpp.

The ImageBuffer clamping did exit before this revision. What this revision did was correcting setting up the GraphicContext of the ImageBuffer by applying a set of transformation in the correct order.

[Said Abou-Hallawa]

http://trac.webkit.org/changeset/96155 is also related. In this revision, the clamping was changed from the viewport size to a fixed 4096x4096 image size.

In the https://bugs.webkit.org/show_bug.cgi?id=67700, there is a discussion about whether the clamping for tiling is really needed or not. The argument is we needed the clamping to avoid allocating huge image buffers. I do not know how can tile image buffer gets big. I still think we should leave the ImageBuffer size issue to the system limits. If the page has a huge SVG image and the system can't allocate memory for it, I think we should not do anything to avoid this from happening. And actually this is what we do in FilterEffect::createImageBufferResult(). We jus allocate the SVG with the given size without any clamping.

[Said Abou-Hallawa]

In http://trac.webkit.org/changeset/65880, setting the transformation of the GraphicContext was moved from the callers of createImageBuffer() to createImageBuffer() itself.

[Said Abou-Hallawa]

http://trac.webkit.org/changeset/65665 introduces the ImageBuffer clamping to the viewport. The goal was: "no more huge image buffer allocations, that could fail" as it is mentioned in the ChangeLog. No bugs, which may have been caused by "huge image buffer allocations", were mentioned.

[Said Abou-Hallawa]

I think I know now why SVG filters, clippers and maskers clamp the ImageBuffers. The FilterEffect class which is shared between the CSS filters and the SVG filters is preventing applying any filter if its size is greater than 4096x4096. In the function FilterEffect::apply() we make an early return if (!isFilterSizeValid(m_absolutePaintRect.size())). And isFilterSizeValid() returns true only if the size is less than 4096x4096. So it seems the SVG wanted to get around this limitation, which is applied anyway to the CSS filters, by clamping the ImageBuffer.

[Said Abou-Hallawa]

Created attachment 252548 [details]
Patch

[Said Abou-Hallawa]

Created attachment 252553 [details]
Patch

[Said Abou-Hallawa]

After further investigation, it turned out that some of the conclusions I stated above are incorrect. Here is my last beliefs:

-- The clamping of the ImageBuffer used by the filter can't be removed. The reason is the FilterEffect simply reject processing any filter if the ImageBuffer size is larger than 4096x4096. For CSS filters, this is not a problem since drawing large HTML elements is optimized by tiling and FilterEffect does not have to deal with large ImageBuffer no matter how big the filter element is. But for SVG drawing, no tiling is applied and we can hit the ImageBuffer size threshold regardless whether the tiling is applied or not.

-- The order of transformations in SVGRenderingContext::createImageBuffer() turns out to be correct. I added the following bugging code in this function:

    GraphicsContext* imageContext = imageBuffer->context();
    AffineTransform transform = imageContext->getCTM();
    imageContext->concatCTM(transform.inverse());

    AffineTransform t1 = AffineTransform().scale(0.5, 0.5);
    AffineTransform t2 = AffineTransform().translate(-100, -100);
    AffineTransform t3 = AffineTransform().scale(3, 3);

    imageContext->concatCTM(t1);
    imageContext->concatCTM(t2);
    imageContext->concatCTM(t3);

    ASSERT(imageContext->getCTM() == t1 * t2 * t3);

And the assertion did not fire. According to Apple developer documentations: https://developer.apple.com/library/mac/documentation/GraphicsImaging/Reference/CGContext/index.html#//apple_ref/c/func/CGContextConcatCTM

I think the assertion should fire since (imageContext->getCTM() == t3 * t2 * t1) unless I am really confused. But since the order of transformation in SVGRenderingContext::createImageBuffer() is reversed and since CGContextConcatCTM() does right multiplication, we get the desired order. So I am going to change this order.

[Darin Adler]

Comment on attachment 252553 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.cpp:37
> +static const float kMaxClampedLength = 4096;
> +static const float kMaxClampedArea = kMaxClampedLength * kMaxClampedLength;

The "k" here is not really the standard for constants in WebKit code; might be nice to fix to match the style guide.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:41
> +    return size.height() * size.width() <= kMaxClampedArea;

This assumes that the size is non-empty, neither width or height is negative. Is that assumption OK? Should we assert that? Maybe we should define what this returns for an empty size. Maybe it would be handy for it to be defined as false for such sizes; a definition that empty sizes are “not clamped” could help streamline other code.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:48
> +    float scaledArea = scaledSize.width() * scaledSize.height();

I don’t think we need a local variable for scaledArea since we use it only once.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:50
> +    if (ImageBuffer::isSizeClamped(scaledSize))

No need for ImageBuffer prefix here.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:54
> +    // If area of scaled size is bigger than the upper limit, adjust the scale
> +    // to fit.

Should make this comment one line.

Also, the “if” here does not match the code. The check if the scaled sizes is bigger than the limit is before this comment, so the comment is confusingly worded now.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:55
> +    scale.scale(sqrt(kMaxClampedArea / scaledArea));

Using sqrt translates the number from float to double and then back. Instead it should be std::sqrt or sqrtf, which will perform the operation on a float.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:56
> +    return false;

I think we should add an assertion here:

    ASSERT(isSizeClamped(size, scale));

Also, I would like to get clearer on whether this is truly guaranteed, even in the presence of rounding and limited precision.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:68
> +    FloatSize clampedSize = ImageBuffer::clampedSize(size);
> +    scale = FloatSize(clampedSize.width() / size.width(), clampedSize.height() / size.height());
> +    return clampedSize;

If scale is empty, then this may divide by zero. What guarantees the scale is not empty? Should we assert that?

> Source/WebCore/platform/graphics/ImageBuffer.cpp:73
> +    return FloatRect(rect.location(), ImageBuffer::clampedSize(rect.size()));

No need for ImageBuffer prefix here.

> Source/WebCore/platform/graphics/ImageBuffer.h:132
> +    static bool isSizeClamped(const FloatSize&);
> +    static bool isSizeClamped(const FloatSize&, FloatSize& scale);
> +    static FloatSize clampedSize(const FloatSize&);
> +    static FloatSize clampedSize(const FloatSize&, FloatSize& scale);
> +    static FloatRect clampedRect(const FloatRect&);

I think this needs a comment to explain when we would want to do this clamping. Presumably it’s for images that we plan to use as filters? I think we’ve confused things a bit by just calling this “size clamping” and not making the specific relationship to filters clearer. Perhaps these functions should go into a filter-related class, not the image buffer class.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:81
> +    ASSERT(hasResult());

I suggest a blank line after this for clearer paragraphing of the function.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:84
> +    FloatPoint location = srcRect.location();
> +    location.moveBy(-m_absolutePaintRect.location());
> +    FloatRect rect = FloatRect(location, srcRect.size());

This should be two lines instead of three:

    FloatRect rect = srcRect;
    rect.moveBy(-m_absolutePaintRect.location());

We should also consider adding an operator-(const FloatRect&, const FloatSize&) so we could write:

    FloatRect rect = srcRect - m_absolutePaintRect.location();

We could even put that equation into the call to mapRect. Another attractive alternative would be to make this translation part of the transform we create below instead of doing the math separately first.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:90
> +    AffineTransform transform = AffineTransform().scale(scale.width(), scale.height());
> +    return transform.mapRect(rect);

It’s frustrating that this will be converting everything to double and back to float, but I suppose that’s how we do things. It’s unpleasant that we have to split the FloatSize out into separate width and height to put it into the transform.

Not sure this is clearer with a local variable. I think this would read pretty well:

    return AffineTransform().scale(scale.width(), scale.height()).mapRect(rect);

And better if we added overloads to AffineTransform:

    return AffineTransform().scale(scale).mapRect(rect);

> Source/WebCore/rendering/FilterEffectRenderer.cpp:309
> +    if (filterRect.isZero() || !ImageBuffer::isSizeClamped(filterRect.size()))
> +        return false;

Why isZero here, but isEmpty everywhere else? I think we want isEmpty here.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:316
> +    FloatRect currentSourceRect = sourceImageRect();
> +    if (filterRect == currentSourceRect)
> +        return false;
> +
> +    setSourceImageRect(filterRect);
> +    return true;

This is not enhanced with the local variable. Should just be:

    if (filterRect == sourceImageRect())
        return false;
    setSourceImageRect(filterRect);
    return true;

In fact, paragraphed like that it makes clearer the relationship between the test and the set function.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:405
> +    if (!sourceGraphicsContext || filter->filterRegion().isZero() || !ImageBuffer::isSizeClamped(filter->filterRegion().size())) {

Why check only isZero here. What about other cases covered by isEmpty? One dimension 0 and the other not. Negative dimensions.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:256
> +    transform *= AffineTransform().scale(scale.width(), scale.height());
> +    transform *= AffineTransform().translate(-paintRect.x(), -paintRect.y());

We designed the scale and translate functions to modify transforms in place. Do we really have to make new transforms and multiply here? Can’t we just do:

    transform.scale(scale.width(), scale.height());
    transform.translate(-paintRect.x(), -paintRect.y());

Or does that result in incorrect behavior.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:269
> +    IntSize clampedSize = roundedIntSize(clampedRect.size());
> +    IntSize unclampedSize = roundedIntSize(targetRect.size());

The rounding here worries me a bit. Is rounding really what we want?

[Said Abou-Hallawa]

Created attachment 252624 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 252553 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:37
>> +static const float kMaxClampedArea = kMaxClampedLength * kMaxClampedLength;
> 
> The "k" here is not really the standard for constants in WebKit code; might be nice to fix to match the style guide.

I searched the code and I found different styles for global const variables. But I chose the most commonly used style, I saw, which is the UpperCamelCase.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:41
>> +    return size.height() * size.width() <= kMaxClampedArea;
> 
> This assumes that the size is non-empty, neither width or height is negative. Is that assumption OK? Should we assert that? Maybe we should define what this returns for an empty size. Maybe it would be handy for it to be defined as false for such sizes; a definition that empty sizes are “not clamped” could help streamline other code.

I added the following condition:

    if (size.width() < 0 && size.height() < 0)
        return false;

which was there in the original function FilterEffect::isFilterSizeValid().

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:48
>> +    float scaledArea = scaledSize.width() * scaledSize.height();
> 
> I don’t think we need a local variable for scaledArea since we use it only once.

Done.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:50
>> +    if (ImageBuffer::isSizeClamped(scaledSize))
> 
> No need for ImageBuffer prefix here.

Done.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:54
>> +    // to fit.
> 
> Should make this comment one line.
> 
> Also, the “if” here does not match the code. The check if the scaled sizes is bigger than the limit is before this comment, so the comment is confusingly worded now.

Removed the "if" from the comment and make it one line.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:55
>> +    scale.scale(sqrt(kMaxClampedArea / scaledArea));
> 
> Using sqrt translates the number from float to double and then back. Instead it should be std::sqrt or sqrtf, which will perform the operation on a float.

I replaced sqrt by sqrtf since I found all the other code is using it and no one is using std::sqrt.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:56
>> +    return false;
> 
> I think we should add an assertion here:
> 
>     ASSERT(isSizeClamped(size, scale));
> 
> Also, I would like to get clearer on whether this is truly guaranteed, even in the presence of rounding and limited precision.

Done.

You are right for your concern. This assertion fired in the test LayoutTests/svg/filters/feMorphology-crash.html because there is one pixel difference between the area of the clamped size and MaxClampedArea. So I fixed this by changing the way we calculate the area of the size in ImageBuffer::isSizeClamped(const FloatSize& size). I get the floor(width) *floorf(height) instead of width * height.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:68
>> +    return clampedSize;
> 
> If scale is empty, then this may divide by zero. What guarantees the scale is not empty? Should we assert that?

Done. I added the following condition at the beginning:

    if (size.isEmpty())
        return size;

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:73
>> +    return FloatRect(rect.location(), ImageBuffer::clampedSize(rect.size()));
> 
> No need for ImageBuffer prefix here.

Done.

>> Source/WebCore/platform/graphics/ImageBuffer.h:132
>> +    static FloatRect clampedRect(const FloatRect&);
> 
> I think this needs a comment to explain when we would want to do this clamping. Presumably it’s for images that we plan to use as filters? I think we’ve confused things a bit by just calling this “size clamping” and not making the specific relationship to filters clearer. Perhaps these functions should go into a filter-related class, not the image buffer class.

The ImageBuffer is created for the filters, maskers, and clippers. And this is why the original clamping calculation was split between the SVGRenderingContext and the FilterEffect. Since the clamping is applied to the created ImageBuffer in call cases, I could not think of any better place other than the ImageBuffer itself.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:81
>> +    ASSERT(hasResult());
> 
> I suggest a blank line after this for clearer paragraphing of the function.

Done.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:84
>> +    FloatRect rect = FloatRect(location, srcRect.size());
> 
> This should be two lines instead of three:
> 
>     FloatRect rect = srcRect;
>     rect.moveBy(-m_absolutePaintRect.location());
> 
> We should also consider adding an operator-(const FloatRect&, const FloatSize&) so we could write:
> 
>     FloatRect rect = srcRect - m_absolutePaintRect.location();
> 
> We could even put that equation into the call to mapRect. Another attractive alternative would be to make this translation part of the transform we create below instead of doing the math separately first.

I applied the origin translation to the transform as you suggested below.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:90
>> +    return transform.mapRect(rect);
> 
> It’s frustrating that this will be converting everything to double and back to float, but I suppose that’s how we do things. It’s unpleasant that we have to split the FloatSize out into separate width and height to put it into the transform.
> 
> Not sure this is clearer with a local variable. I think this would read pretty well:
> 
>     return AffineTransform().scale(scale.width(), scale.height()).mapRect(rect);
> 
> And better if we added overloads to AffineTransform:
> 
>     return AffineTransform().scale(scale).mapRect(rect);

Done. But since the transform includes translation and scaling, it looks like this:

    AffineTransform transform;
    transform.scale(scale).translate(-m_absolutePaintRect.location());
    return transform.mapRect(srcRect);

I could have written in one line but I felt one is too little for what this code is doing.

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:309
>> +        return false;
> 
> Why isZero here, but isEmpty everywhere else? I think we want isEmpty here.

Done.

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:316
>> +    return true;
> 
> This is not enhanced with the local variable. Should just be:
> 
>     if (filterRect == sourceImageRect())
>         return false;
>     setSourceImageRect(filterRect);
>     return true;
> 
> In fact, paragraphed like that it makes clearer the relationship between the test and the set function.

Done.

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:405
>> +    if (!sourceGraphicsContext || filter->filterRegion().isZero() || !ImageBuffer::isSizeClamped(filter->filterRegion().size())) {
> 
> Why check only isZero here. What about other cases covered by isEmpty? One dimension 0 and the other not. Negative dimensions.

Done. isEmpty() is used instead.

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:256
>> +    transform *= AffineTransform().translate(-paintRect.x(), -paintRect.y());
> 
> We designed the scale and translate functions to modify transforms in place. Do we really have to make new transforms and multiply here? Can’t we just do:
> 
>     transform.scale(scale.width(), scale.height());
>     transform.translate(-paintRect.x(), -paintRect.y());
> 
> Or does that result in incorrect behavior.

Done. The code now looks like this:

    AffineTransform transform;
    transform.scale(scale).translate(-paintRect.location()).multiply(absoluteTransform);

which is very concise and makes the order clearer.

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:269
>> +    IntSize unclampedSize = roundedIntSize(targetRect.size());
> 
> The rounding here worries me a bit. Is rounding really what we want?

I think you are right. I could not understand why we need to do this rounding here. It is even strange that the SVG pattern code needs this special function to create its ImageBuffer. I think I can delete this whole function and use the other one by the SVG pattern. I will file a bug and submit a follow up patch for this code.

[Said Abou-Hallawa]

Created attachment 252629 [details]
Patch

[Darin Adler]

Comment on attachment 252553 [details]
Patch

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

>>> Source/WebCore/platform/graphics/ImageBuffer.cpp:41
>>> +    return size.height() * size.width() <= kMaxClampedArea;
>> 
>> This assumes that the size is non-empty, neither width or height is negative. Is that assumption OK? Should we assert that? Maybe we should define what this returns for an empty size. Maybe it would be handy for it to be defined as false for such sizes; a definition that empty sizes are “not clamped” could help streamline other code.
> 
> I added the following condition:
> 
>     if (size.width() < 0 && size.height() < 0)
>         return false;
> 
> which was there in the original function FilterEffect::isFilterSizeValid().

That doesn’t seem quite right. For one thing it does && instead of || so you could still have either width or height as a negative number. For another, it doesn’t rule out the various types of empty sizes. I think there is still something a little messy here although there may be no practical problem because I think the callers generally call isEmpty before calling this.

I still think we should just come back here and define “empty” as “not clamped” even though that’s a little peculiar; I think it’s likely that with that definition all the callers could then remove their additional explicit checks for empty sizes. Or we could name this ImageBuffer::sizeNeedsClamping and then it would be more natural to return false for all the empty sizes.

[WebKit Commit Bot]

Comment on attachment 252629 [details]
Patch

Clearing flags on attachment: 252629

Committed r183956: <http://trac.webkit.org/changeset/183956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

(In reply to comment #24)
> Comment on attachment 252553 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252553&action=review
> 
> >>> Source/WebCore/platform/graphics/ImageBuffer.cpp:41
> >>> +    return size.height() * size.width() <= kMaxClampedArea;
> >> 
> >> This assumes that the size is non-empty, neither width or height is negative. Is that assumption OK? Should we assert that? Maybe we should define what this returns for an empty size. Maybe it would be handy for it to be defined as false for such sizes; a definition that empty sizes are “not clamped” could help streamline other code.
> > 
> > I added the following condition:
> > 
> >     if (size.width() < 0 && size.height() < 0)
> >         return false;
> > 
> > which was there in the original function FilterEffect::isFilterSizeValid().
> 
> That doesn’t seem quite right. For one thing it does && instead of || so you
> could still have either width or height as a negative number. For another,
> it doesn’t rule out the various types of empty sizes. I think there is still
> something a little messy here although there may be no practical problem
> because I think the callers generally call isEmpty before calling this.
> 
> I still think we should just come back here and define “empty” as “not
> clamped” even though that’s a little peculiar; I think it’s likely that with
> that definition all the callers could then remove their additional explicit
> checks for empty sizes. Or we could name this ImageBuffer::sizeNeedsClamping
> and then it would be more natural to return false for all the empty sizes.

Function was renamed to ImageBuffer::sizeNeedsClamping(). The condition for negative width/height was replaced by isEmpty(). I left the isEmpty() conditions in the callers side because if the size isEmpty(), sizeNeedsClamping() will return false but we still need to bail out and ignore the filter.

Committed r183960: <http://trac.webkit.org/changeset/183960>