Bug 38704 – SVG pattern size changed when resizing browser

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: SVG

[Steven Lai]

* SUMMARY

The pattern size changed when resizing browser, which the pattern size should be independent of the browser's size.
The pattern even failed to render repeatedly if we resize the browser to smaller than the element which applied the pattern and then scroll down.

The problem doesn't exist if we don't use viewBox, but it is needed.

* STEPS TO REPRODUCE  (see test-case in attachment)
1. Define a pattern with {x=0, y=0, width=700, height=700}
2. Set the pattern with viewBox {0 0 200 200}
3. Apply the pattern on a rect with {x=0, y=0, width=800 height=800}
4. Resize browser to smaller than the pattern size
5. Scroll down

* EXPECTED RESULTS
The pattern size doesn't change when resizing and scrolling

* ACTUAL RESULTS
pattern size changed in step 4
pattern failed to render repeatedly in step 5

* REGRESSION
Safari Version 4.0.4 (6531.21.10, r53178) / Mac

It seems that the size of SVG pattern is upper-bounded by the size of the browser window.

[Steven Lai]

Created attachment 55321 [details]
Test case

[Steven Lai]

<rdar://problem/7541466>

[Dirk Schulze]

So the problem is, that the pattern is cut to the actual viewport, but doesn't rebuild if the size of the windows changes or if we scroll a bit.

I looked at an older revision, and even the size of the pattern changes with resizing the window. That is wrong to.

My proposal is to not clip to viewport anymore on creating a pattern in RenderSVGResourcePattern.cpp.

[Dirk Schulze]

(In reply to comment #3)
> So the problem is, that the pattern is cut to the actual viewport, but doesn't
> rebuild if the size of the windows changes or if we scroll a bit.
> 
> I looked at an older revision, and even the size of the pattern changes with
> resizing the window. That is wrong to.
> 
> My proposal is to not clip to viewport anymore on creating a pattern in
> RenderSVGResourcePattern.cpp.

Clipping itself is realy dangerous. I thougt about at least clipping to the size of the target element, but we have to respect pattern transformations. While scale and translation isn't a problem, determin the visible size of a pattern on skewing or rotation is realy difficult.

[Steven Lai]

Yep. Just tried removing the call to clampImageBufferSizeToViewport(object->document()->view(), imageSize)
it works :-)

Although the repaint issue (if the browser client area size is smaller than the pattern filled rectangle and you scroll it) is still there.

[Steven Lai]

> While scale and translation isn't a problem, determine the visible size of a
> pattern on skewing or rotation is realy difficult.

We should be able to deal with the easy case first: check if the affine transform matrix to see if is in the form:
[ a 0 e ]
[ 0 d f ]
[ 0 0 1 ]
So that we know if there's no rotation and skewing. The decomposition of this particular case is trivial.

[Steven Lai]

> Clipping itself is realy dangerous. I thougt about at least clipping to the
> size of the target element, but we have to respect pattern transformations.
> While scale and translation isn't a problem, determin the visible size of a
> pattern on skewing or rotation is realy difficult.

By the way, do you think it's possible to simply use GraphicsContext::setFillPattern() / GraphicsContext::applyFillPattern() instead of implementing it ourselves??

[Dirk Schulze]

(In reply to comment #7)
> > Clipping itself is realy dangerous. I thougt about at least clipping to the
> > size of the target element, but we have to respect pattern transformations.
> > While scale and translation isn't a problem, determin the visible size of a
> > pattern on skewing or rotation is realy difficult.
> 
> By the way, do you think it's possible to simply use
> GraphicsContext::setFillPattern() / GraphicsContext::applyFillPattern() instead
> of implementing it ourselves??

Not sure what you mean? We use  context->setFillPattern(patternData->pattern); and context->fillPath(). Simiular to the HTML Canvas code.

[Steven Lai]

> Not sure what you mean? We use  context->setFillPattern(patternData->pattern);
> and context->fillPath(). Simiular to the HTML Canvas code.

oops ... please ignore that :)
I'm looking at RenderSVGResourcePattern::buildPattern

    newTileImageContext->save();
    newTileImageContext->translate(-patternBoundaries.width() * numX, -patternBoundaries.height() * numY);
    for (int i = numY; i > 0; --i) {
        newTileImageContext->translate(0, patternBoundaries.height());
        for (int j = numX; j > 0; --j) {
            newTileImageContext->translate(patternBoundaries.width(), 0);
            newTileImageContext->drawImage(tileImage->image(), style()->colorSpace(), tileRect, tileRect);
        }
        newTileImageContext->translate(-patternBoundaries.width() * numX, 0);
    }
    newTileImageContext->restore();


and i saw a method called DrawTiledImage() in GraphicsContext

I just want to know if it could do the same thing

[Dirk Schulze]

(In reply to comment #6)
> > While scale and translation isn't a problem, determine the visible size of a
> > pattern on skewing or rotation is realy difficult.
> 
> We should be able to deal with the easy case first: check if the affine
> transform matrix to see if is in the form:
> [ a 0 e ]
> [ 0 d f ]
> [ 0 0 1 ]
> So that we know if there's no rotation and skewing. The decomposition of this
> particular case is trivial.

We have isIdentityOrTranslationOrFlipped() you can start with. If we want to clip to the target element boundaries, we need to be carefull, if we fil the target or stroke the target with the pattern. It's saver to take the strokeBoundaries of the object instead of the boundingBox.

[Dirk Schulze]

(In reply to comment #9)
> > Not sure what you mean? We use  context->setFillPattern(patternData->pattern);
> > and context->fillPath(). Simiular to the HTML Canvas code.
> 
> oops ... please ignore that :)
> I'm looking at RenderSVGResourcePattern::buildPattern
> 
>     newTileImageContext->save();
>     newTileImageContext->translate(-patternBoundaries.width() * numX,
> -patternBoundaries.height() * numY);
>     for (int i = numY; i > 0; --i) {
>         newTileImageContext->translate(0, patternBoundaries.height());
>         for (int j = numX; j > 0; --j) {
>             newTileImageContext->translate(patternBoundaries.width(), 0);
>             newTileImageContext->drawImage(tileImage->image(),
> style()->colorSpace(), tileRect, tileRect);
>         }
>         newTileImageContext->translate(-patternBoundaries.width() * numX, 0);
>     }
>     newTileImageContext->restore();
> 
> 
> and i saw a method called DrawTiledImage() in GraphicsContext
> 
> I just want to know if it could do the same thing

it's a rarely used case and was needed by css-property overflow. The old code had a switch and we didn't run throug that code the most of the time. I need to look at the history, why this switch was removed ...

[Dirk Schulze]

(In reply to comment #11)
> (In reply to comment #9)
> > > Not sure what you mean? We use  context->setFillPattern(patternData->pattern);
> > > and context->fillPath(). Simiular to the HTML Canvas code.
> > 
> > oops ... please ignore that :)
> > I'm looking at RenderSVGResourcePattern::buildPattern
> > 
> >     newTileImageContext->save();
> >     newTileImageContext->translate(-patternBoundaries.width() * numX,
> > -patternBoundaries.height() * numY);
> >     for (int i = numY; i > 0; --i) {
> >         newTileImageContext->translate(0, patternBoundaries.height());
> >         for (int j = numX; j > 0; --j) {
> >             newTileImageContext->translate(patternBoundaries.width(), 0);
> >             newTileImageContext->drawImage(tileImage->image(),
> > style()->colorSpace(), tileRect, tileRect);
> >         }
> >         newTileImageContext->translate(-patternBoundaries.width() * numX, 0);
> >     }
> >     newTileImageContext->restore();
> > 
> > 
> > and i saw a method called DrawTiledImage() in GraphicsContext
> > 
> > I just want to know if it could do the same thing
> 
> it's a rarely used case and was needed by css-property overflow. The old code
> had a switch and we didn't run throug that code the most of the time. I need to
> look at the history, why this switch was removed ...

Ah, here is it:
http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderSVGResourcePattern.cpp#L291

[Steven Lai]

Created attachment 55345 [details]
Patch (without optimization)

[Steven Lai]

 
> We have isIdentityOrTranslationOrFlipped() you can start with. If we want to
> clip to the target element boundaries, we need to be carefull, if we fil the
> target or stroke the target with the pattern. It's saver to take the
> strokeBoundaries of the object instead of the boundingBox.

We can also determine if the matrix represents any skewing too
Consider the 2x2 portion of the CTM

[ a b ]
[ c d ]

for rotation, scaling, and flipping, the row vectors [ a b ] and [ c d ] are orthogonal, thus we can check if the dot product is (close to) zero

then, we could decompose the matrix by normalizing the vector [ a b ] and [ c d ], determine flipping by inspecting the signs of the components, and use atan2() to find the rotation angle


perhaps skewing is hard because even if we know the skewed pattern tile rect fully encloses the clipping rect, we still have to think how we should clip it

[Nikolas Zimmermann]

I'm hestitated to r+ the patch, some background notes on the clipping. The intent was to avoid creating uber-large ImageBuffers that may consume all your memory. Can you investigate whether we can find a better way to handle this problem? What about LayoutTests/svg/custom/pattern-excessive-malloc.svg - doesn't it blow up with your patch? (aka. crash)

[Dirk Schulze]

Comment on attachment 55345 [details]
Patch (without optimization)

We should realy think about clipping here. I maybe mixed up this bug with another clipping bug we had with pattern. On the other bug, we also clipped the content of the pattern to viewPort, and that had the problems with determing the correct drawing are on skewing.
Maybe this is not quite relevant here. We know the strokeBoundaries of the target-element, and the tmpImageBuffer for the pattern don't need to be bigger (after all transformations) than the strokeBoundaries of the target.
We need some test cases, but maybe Niko is right and we don't care about the type of transformation, but just about the resulting size of the pattern.
And this is quite easy to check. We can transform the current boundaries of the Pattern with the transfromation matrix of the pattern and clip it to the strokeBoundaries of the target.

[Dirk Schulze]

(In reply to comment #16)
> (From update of attachment 55345 [details])
> We should realy think about clipping here. I maybe mixed up this bug with
> another clipping bug we had with pattern. On the other bug, we also clipped the
> content of the pattern to viewPort, and that had the problems with determing
> the correct drawing are on skewing.
> Maybe this is not quite relevant here. We know the strokeBoundaries of the
> target-element, and the tmpImageBuffer for the pattern don't need to be bigger
> (after all transformations) than the strokeBoundaries of the target.
> We need some test cases, but maybe Niko is right and we don't care about the
> type of transformation, but just about the resulting size of the pattern.
> And this is quite easy to check. We can transform the current boundaries of the
> Pattern with the transfromation matrix of the pattern and clip it to the
> strokeBoundaries of the target.

The other bug I mixed up this bug was bug 29911.

[Steven Lai]

Created attachment 55758 [details]
Patch

[Dirk Schulze]

Created attachment 55760 [details]
Test case

SVG Test case.

[Dirk Schulze]

(In reply to comment #19)
> Created an attachment (id=55760) [details]
> Test case
> 
> SVG Test case.

Note, that pattern were faster and had better results before the code move to the Renderer.

[Nikolas Zimmermann]

(In reply to comment #20)
> (In reply to comment #19)
> > Created an attachment (id=55760) [details] [details]
> > Test case
> > 
> > SVG Test case.
> 
> Note, that pattern were faster and had better results before the code move to the Renderer.
Huh? I almost can't believe you on that one, please elaborate. If you are aware of problems, why didn't you file a bug?

[Dirk Schulze]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Created an attachment (id=55760) [details] [details] [details]
> > > Test case
> > > 
> > > SVG Test case.
> > 
> > Note, that pattern were faster and had better results before the code move to the Renderer.
> Huh? I almost can't believe you on that one, please elaborate. If you are aware of problems, why didn't you file a bug?
I realised the perf problems with the test above and I created the test just for this bug.
I think that the patch (that still needs to be uploaded ;-)) will catch the issues on the test as well, since it looks like a to big imageBuffer is used.

But I'm not sure how we can cover this problem. The current pattern already has a size of 10000x10000. With the transformation of patternTransform (scale of 0.001) it gets 1000 times bigger. At the moment we clip this to the viewport size, so it is at least displayable.

[Adam Barth]

Comment on attachment 55758 [details]
Patch

This patch might well be a good code change, but it doesn't have any tests.  If this isn't possible to test in DRT (can we resize the viewport in DRT?), then please at least add a test case to manual-tests.

[Nikolas Zimmermann]

*** This bug has been marked as a duplicate of bug 41396 ***