Last Comment Bug 697057 - Scaled patterns are blurred
: Scaled patterns are blurred
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Robert Longson
: Jet Villegas (:jet)
Depends on: 691646
Blocks: 535185
  Show dependency treegraph
Reported: 2011-10-25 04:44 PDT by Robert Longson
Modified: 2011-10-28 04:43 PDT (History)
2 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (1.10 KB, patch)
2011-10-25 04:47 PDT, Robert Longson
no flags Details | Diff | Splinter Review
patch with reftest (3.10 KB, patch)
2011-10-25 05:29 PDT, Robert Longson
no flags Details | Diff | Splinter Review
updated patch to address efficiency concerns (3.10 KB, patch)
2011-10-26 04:01 PDT, Robert Longson
no flags Details | Diff | Splinter Review
right patch (6.44 KB, patch)
2011-10-26 04:16 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description Robert Longson 2011-10-25 04:44:14 PDT
bug 691646 fixed one way that patterns become blurred but broke another - bug 535185.

In bug 535185 the pattern is small and is scaled when it's used. In bug 691646 the pattern has a patternTransform which scales it. We need to account for both scalings when determining the size of the pattern to create.
Comment 1 Robert Longson 2011-10-25 04:47:26 PDT
Created attachment 569330 [details] [diff] [review]

I'll see if I can do a reftest for this.
Comment 2 Robert Longson 2011-10-25 05:29:16 PDT
Created attachment 569335 [details] [diff] [review]
patch with reftest
Comment 3 Daniel Holbert [:dholbert] 2011-10-25 13:09:21 PDT
Looks like pattern-scale-01a.svg and pattern-scale-01-ref.svg are missing from the patch.
Comment 4 Daniel Holbert [:dholbert] 2011-10-25 13:26:26 PDT
oh, sorry -- the ref already exists, and "a" is renamed from an existing file.  Cool.  Darn bugzilla's diff viewer.
Comment 5 Daniel Holbert [:dholbert] 2011-10-25 13:28:25 PDT
Comment on attachment 569335 [details] [diff] [review]
patch with reftest

>@@ -254,21 +254,23 @@ nsSVGPatternFrame::PaintPattern(gfxASurf
>   // routine.
>   *patternMatrix = GetPatternMatrix(bbox, callerBBox, callerCTM);
>+  float scale = nsSVGUtils::MaxExpansion(callerCTM);
>   bool resultOverflows;
>   gfxIntSize surfaceSize =
>     nsSVGUtils::ConvertToSurfaceSize(
>-      gfxSize(patternWidth * fabs(patternMatrix->xx),
>-              patternHeight * fabs(patternMatrix->yy)),
>+      gfxSize(patternWidth * fabs(patternMatrix->xx) * scale,
>+              patternHeight * fabs(patternMatrix->yy) * scale),
>       &resultOverflows);

Why do we need this extra "scale" factor?  Shouldn't we already be getting all the scaling information that we need from |callerCTM| when we create patternMatrix, in the "GetPatternMatrix(..., callerCTM)" call?

See also bug 691646 comment 13... Perhaps that helps with this?
Comment 6 Daniel Holbert [:dholbert] 2011-10-25 14:57:55 PDT
Actually I'm less sure about bug 691646 comment 13 now, as noted there, so disregard that for now.

However, I'm still a little iffy on the patch here...  Note that GetPatternMatrix() already involves a scale by 1/MaxExpansion(callerCTM), so your attached patch here is basically reversing that scale, for the purposes of the surface-size computation.  That seems like it could be extra work.

Perhaps we really want to remove that scale from GetPatternMatrix completely? (possibly lifting it up to this method, and applying it to patternMatrix after we've computed the surfaceSize?)

I'm not sure -- I don't totally grok what's going on here.  I'd like to get a better idea of why the patch here (or its successor) is correct, before granting r+.
Comment 7 Robert Longson 2011-10-26 03:40:03 PDT
We only want to apply the patternTransform scaling (the scaling in the pattern) without the callerCTM (i.e. the scaling on to the object). You are right in that we're multiplying and dividing in by MaxExpansion order to get the number we'd get from GetPatternTransform directly.
Comment 8 Robert Longson 2011-10-26 04:01:56 PDT
Created attachment 569636 [details] [diff] [review]
updated patch to address efficiency concerns
Comment 9 Robert Longson 2011-10-26 04:16:37 PDT
Created attachment 569638 [details] [diff] [review]
right patch
Comment 10 Daniel Holbert [:dholbert] 2011-10-26 10:29:36 PDT
Comment on attachment 569638 [details] [diff] [review]
right patch

Could you put a short chunk of documentation over nsSVGPatternFrame::GetPatternMatrix() (either the decl or the impl), to reflect its new usage?

r=me with that.
Comment 12 Ed Morley [:emorley] 2011-10-27 10:24:04 PDT
Do you think this could have caused the following regressions?

Talos Regression :( V8 increase 2.78% on MacOSX 10.6.2 (rev3) Mozilla-Inbound
Talos Regression :( Dromaeo (String/Array/Eval/Regex) decrease 4.17% on MacOSX 10.6.2 (rev3) Mozilla-Inbound!topic/
Comment 13 Daniel Holbert [:dholbert] 2011-10-27 10:38:56 PDT
(In reply to Ed Morley [:edmorley] from comment #12)
> Do you think this could have caused the following regressions?

No. The patch *only* touched code for the <pattern> element in SVG, so it has no effect on non-SVG content. (I'd be very surprised if either of those test suites included SVG at all, let alone SVG <pattern> elements.)

I'm guessing that the treewatcher bot might have mis-attributed blame here, and the real guilty party might be immediately before/after (or maybe an infra change?)
Comment 14 Ed Morley [:emorley] 2011-10-27 10:41:48 PDT
(In reply to Daniel Holbert [:dholbert] from comment #13)
> No. The patch *only* touched code for the <pattern> element in SVG

Ah yes. Sorry was in a rush so forgot to actually check the diff :$
Comment 15 Daniel Holbert [:dholbert] 2011-10-27 11:07:42 PDT
No problem; thanks for your vigilance!  This does look like a real regression (with unknown cause so far) -- let's follow up in m.d.tree-management.
Comment 16 Ed Morley [:emorley] 2011-10-28 04:43:13 PDT

Note You need to log in before you can comment on or make changes to this bug.