Bug 38704 - SVG pattern size changed when resizing browser
Summary: SVG pattern size changed when resizing browser
Status: RESOLVED DUPLICATE of bug 41396
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2010-05-06 16:51 PDT by Steven Lai
Modified: 2010-08-19 04:55 PDT (History)
4 users (show)

See Also:


Attachments
Test case (3.74 KB, application/zip)
2010-05-06 16:52 PDT, Steven Lai
no flags Details
Patch (without optimization) (1.25 KB, patch)
2010-05-07 00:13 PDT, Steven Lai
krit: review-
Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2010-05-11 14:35 PDT, Steven Lai
abarth: review-
Details | Formatted Diff | Diff
Test case (2.01 KB, image/svg+xml)
2010-05-11 14:48 PDT, Dirk Schulze
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Lai 2010-05-06 16:51:30 PDT
* 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.
Comment 1 Steven Lai 2010-05-06 16:52:17 PDT
Created attachment 55321 [details]
Test case
Comment 2 Steven Lai 2010-05-06 16:52:59 PDT
<rdar://problem/7541466>
Comment 3 Dirk Schulze 2010-05-06 22:19:33 PDT
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.
Comment 4 Dirk Schulze 2010-05-06 22:31:25 PDT
(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.
Comment 5 Steven Lai 2010-05-06 22:34:45 PDT
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.
Comment 6 Steven Lai 2010-05-06 22:48:57 PDT
> 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.
Comment 7 Steven Lai 2010-05-06 22:56:13 PDT
> 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??
Comment 8 Dirk Schulze 2010-05-06 23:02:59 PDT
(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.
Comment 9 Steven Lai 2010-05-06 23:07:18 PDT
> 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
Comment 10 Dirk Schulze 2010-05-06 23:10:27 PDT
(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.
Comment 11 Dirk Schulze 2010-05-06 23:14:28 PDT
(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 ...
Comment 12 Dirk Schulze 2010-05-06 23:17:44 PDT
(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
Comment 13 Steven Lai 2010-05-07 00:13:40 PDT
Created attachment 55345 [details]
Patch (without optimization)
Comment 14 Steven Lai 2010-05-07 00:35:36 PDT
 
> 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
Comment 15 Nikolas Zimmermann 2010-05-07 04:47:36 PDT
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)
Comment 16 Dirk Schulze 2010-05-07 12:57:36 PDT
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.
Comment 17 Dirk Schulze 2010-05-07 13:02:18 PDT
(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.
Comment 18 Steven Lai 2010-05-11 14:35:55 PDT
Created attachment 55758 [details]
Patch
Comment 19 Dirk Schulze 2010-05-11 14:48:50 PDT
Created attachment 55760 [details]
Test case

SVG Test case.
Comment 20 Dirk Schulze 2010-05-11 15:10:04 PDT
(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.
Comment 21 Nikolas Zimmermann 2010-05-12 00:10:41 PDT
(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?
Comment 22 Dirk Schulze 2010-05-12 00:21:25 PDT
(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.
Comment 23 Adam Barth 2010-06-21 19:30:14 PDT
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.
Comment 24 Nikolas Zimmermann 2010-08-19 04:55:43 PDT

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