Bug 38704

Summary: SVG pattern size changed when resizing browser
Product: WebKit Reporter: Steven Lai <s3lance>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: bdakin, krit, pkchan+bugzilla=webkit, zimmermann
Priority: P2 Keywords: HasReduction, InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Test case
none
Patch (without optimization)
krit: review-
Patch
abarth: review-
Test case none

Steven Lai
Reported 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.
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-
DetailsFormatted DiffDiff
Patch (3.95 KB, patch)
2010-05-11 14:35 PDT, Steven Lai 		abarth: review-
DetailsFormatted DiffDiff
Test case (2.01 KB, image/svg+xml)
2010-05-11 14:48 PDT, Dirk Schulze 		no flags
Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Steven Lai
Comment 1 2010-05-06 16:52:17 PDT
Created attachment 55321 [details] Test case
Steven Lai
Comment 2 2010-05-06 16:52:59 PDT
<rdar://problem/7541466>
Dirk Schulze
Comment 3 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.
Dirk Schulze
Comment 4 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.
Steven Lai
Comment 5 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.
Steven Lai
Comment 6 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.
Steven Lai
Comment 7 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??
Dirk Schulze
Comment 8 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.
Steven Lai
Comment 9 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
Dirk Schulze
Comment 10 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.
Dirk Schulze
Comment 11 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 ...
Dirk Schulze
Comment 12 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
Steven Lai
Comment 13 2010-05-07 00:13:40 PDT
Created attachment 55345 [details] Patch (without optimization)
Steven Lai
Comment 14 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
Nikolas Zimmermann
Comment 15 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)
Dirk Schulze
Comment 16 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.
Dirk Schulze
Comment 17 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.
Steven Lai
Comment 18 2010-05-11 14:35:55 PDT
Created attachment 55758 [details] Patch
Dirk Schulze
Comment 19 2010-05-11 14:48:50 PDT
Created attachment 55760 [details] Test case SVG Test case.
Dirk Schulze
Comment 20 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.
Nikolas Zimmermann
Comment 21 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?
Dirk Schulze
Comment 22 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.
Adam Barth
Comment 23 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.
Nikolas Zimmermann
Comment 24 2010-08-19 04:55:43 PDT
*** This bug has been marked as a duplicate of bug 41396 ***
Note You need to log in before you can comment on or make changes to this bug.