885939 - SVG-as-image sometimes fails to stretch when it should (affects Australis theme mockup)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Seth Fowler [:seth] [:s2h]]

After bug 600207, which involved some changes to the matrices used for drawing SVG images, there are some cases in which SVG images fail to tile. I don't yet know specifically why the failure only occurs in some cases and not others.

I first noticed the problem when looking at this mockup of the Australis theme in the current Nightly:

http://people.mozilla.com/~shorlander/files/australis-linux-svg-test/australis-liveDemo-linux.html

Right now the background of the selected tab fails to tile. The same mockup looks fine in the Nightly build before the one which included bug 600207, so I strongly suspect that bug 600207 is the cause.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Bug 885919 is related; an appropriate reftest would have caught this bug.

Comment 2

[Daniel Holbert [:dholbert]]

(Note that we do still successfully render attachment 761196 [details] , which has a tiled svg image as a css background, FWIW.)

Comment 3

[Daniel Holbert [:dholbert]]

Actually, it looks like the CSS for that tab-title is:

.tab.active {
  position: relative;
  background-image: url(../images-linux/tabActiveStart.svg),
                    url(../images-linux/tabActiveMiddle.svg),
                    url(../images-linux/tabActiveEnd.svg),
                    url(../images-linux/tabActiveStart.png),
                    url(../images-linux/tabActiveMiddle.png),
                    url(../images-linux/tabActiveEnd.png);
  background-repeat: no-repeat,
                     no-repeat,
                     no-repeat,
                     no-repeat,
                     no-repeat,
                     no-repeat;
  background-position: left,
                       30px,
                       right,
                       left,
                       30px,
                       right;
  background-size: 30px 31px,
                   calc(100% - 60px) 31px,
                   30px 31px,
                   30px 31px,
                   calc(100% - 60px) 31px,
                   30px 31px;

which looks like it's *not* tiling-related (given the "no-repeat").

Source: http://people.mozilla.com/~shorlander/files/australis-linux-svg-test/css/styles-linux.css

So still a background-image issue, but not necessarily a tiling issue.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Yep, looks like it's being stretched, not tiled.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Make SVG images stretch correctly.

Alrighty, so the problem is that we can't use RenderDocument to directly render an SVG document with nonuniform scale factors (that is, where x and y are scaled differently). The solution is to only factor a uniform scale factor out of the transform. I chose to factor out the largest scale factor, so after the transformation we always render larger and then shrink down in a nonuniform fashion as appropriate. This should provide the highest image quality.

This resolves the Australis tab testcase and some other ones. (It does NOT address all known problems with scaling; I'll address the remainder in a separate bug.)

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add reftest for SVG image stretching.

This adds a reftest that tests the ability of SVG images to stretch (which is the clearest English way I can think of describing scaling in a nonuniform fashion). If we had this reftest, we would have caught this bug before it hit m-c; the test fails before Part 1 is applied and passes with it applied. It's based on the Australis tab test case.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 773475 [details] [diff] [review]
(Part 2) - Add reftest for SVG image stretching.

A few nits on the SVG file in the reftest:

>+++ b/layout/reftests/svg/as-image/white-rect.svg
>@@ -0,0 +1,16 @@
>+<?xml version="1.0" encoding="utf-8"?>
>+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

Drop the <!DOCTYPE line -- it's unnecessary, and our view-source UI actually warns about it (highlights it in red) as a "stray doctype".

>+<svg version="1.1"
>+     xmlns="http://www.w3.org/2000/svg"
>+     xmlns:xlink="http://www.w3.org/1999/xlink"
>+     x="0px"
>+     y="0px"

Nit: drop x/y here. They're meaningless on outermost SVG elements.
 http://www.w3.org/TR/SVG11/struct.html#SVGElementXAttribute

>+     width="6px"
>+     height="30px"
>+     viewBox="0 0 6 30"
>+     xml:space="preserve">

Drop this xml:space thing, too; it adds nothing.

>+  <rect width="6" height="30" style="fill:white; fill-opacity:1;" />

Drop "fill-opacity" -- it defaults to 1, so there's no reason to bother setting it.
 http://www.w3.org/TR/SVG11/painting.html#FillOpacityProperty

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 773475 [details] [diff] [review]
(Part 2) - Add reftest for SVG image stretching.

A few more reftest nits:

>+++ b/layout/reftests/svg/as-image/background-stretch-1-ref.html
>@@ -0,0 +1,39 @@
>+<!DOCTYPE html>
>+<head>
>+  <style>
>+    div {
>+      background-color: rgb(100, 100, 0);
>+      color: rgb(51, 51, 51);

This "color" has no effect, right? It controls text-color, but there's no text here.

>+      margin: 0px;
>+      padding: 0px;

These are the initial values of margin/padding; no need to set them explicitly, right?
(The same goes for the testcase)

>+      height: 30px;
>+    }
>+    .container {
>+      width: 100px;
>+    }
>+    .stretch {
>+      width: 40px;
>+      background-color: white;
[...]
>+    body {
>+      background-color: rgb(0, 0, 0);

I'd prefer that we keep this consistent on color names vs. rgb() values.  Names are more human readable, so maybe let's use "black" instead of rgb(0, 0, 0), and maybe "yellow" instead of rgb(100, 100, 0)?
(The same goes for the testcase)

>+++ b/layout/reftests/svg/as-image/reftest.list
>+# Test for stretching background images
>+== background-stretch-1.html background-stretch-1-ref.html

Clarify slightly, to e.g.:
# Test for stretching background images by different amounts in each dimension

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #5)
> Alrighty, so the problem is that we can't use RenderDocument to directly
> render an SVG document with nonuniform scale factors (that is, where x and y
> are scaled differently).

I knew there was a better way to express this and after posting the patch I realized it. RenderDocument _preserves the aspect ratio_ of the underlying SVG document.

Comment 10

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #8)

All good nits, thanks! I'll get a new version cooked up shortly.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #9)
> RenderDocument _preserves the aspect ratio_ of the underlying
> SVG document.

Hm... I'm not sure I'm convinced of this. I'd expect it to render the viewport-region that it knows about, to the gfxContext that it's passed.

So what I *suspect* is happening is something like this:
We're probably adjusting the viewport and matrices in parallel, such that the "stretched" viewport region ends up mapping back to "un-stretched" SVG coordinates, on the actual underlying render-target...  Or something like that.

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Hm... I'm not sure I'm convinced of this.

After some deep investigation of the other scaling issues I mentioned I agree; that this was happening may just have been the result of another bug. There was a more fundamental issue here - scaling the viewport size is a bad idea and we end up having to reproduce several calculations in VectorImage.cpp that the SVG document should be doing internally. If we just don't scale the viewport, everything becomes a lot simpler!

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Make SVG images scale correctly.

This new patch _does_ fix all known SVG-as-image scaling issues introduced by bug 600207. We don't scale the viewport at all; instead, we pass an additional matrix to SVGDrawingCallback which we apply just before drawing. Because we're still scaling the texture into which we're drawing, we still get the benefits of bug 600207, but the scaling issues are fixed.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 773697 [details] [diff] [review]
(Part 1) - Make SVG images scale correctly.

Thanks! I think this makes a lot more sense.

One thing I noticed that can be improved:

> // Helper-class: SVGDrawingCallback
> class SVGDrawingCallback : public gfxDrawingCallback {
> public:
>   SVGDrawingCallback(SVGDocumentWrapper* aSVGDocumentWrapper,
>                      const nsIntRect& aViewport,
>+                     const gfxMatrix& aMatrix,
[...]
> private:
>   nsRefPtr<SVGDocumentWrapper> mSVGDocumentWrapper;
>   const nsIntRect mViewport;
>+  const gfxMatrix mMatrix;

"mMatrix" is kind of a generic name, given the number of matrices involved here. :)  This could probably use a comment, EXCEPT: it actually looks like this will always just be a scaling matrix -- so, instead of storing a matrix here, how about we just pass in a gfxSize for the scale factors?  (We actually already have exactly the gfxSize to pass in -- "scale", in VectorImage::Draw.)

>@@ -266,16 +269,17 @@ SVGDrawingCallback::operator()(gfxContex
[...]
>   gfxContextMatrixAutoSaveRestore contextMatrixRestorer(aContext);
>   aContext->Multiply(gfxMatrix(aTransform).Invert());
>+  aContext->Multiply(gfxMatrix(mMatrix).Invert());

Then this can be:
    aContext->Scale(1.0 / mScale.width,
                    1.0 / mScale.height)
right?

Comment 15

[Daniel Holbert [:dholbert]]

So I'm also a little uncomfortable with the "drawableSize" vs. "viewportSize" distinction.

It's partly a naming thing -- we have aViewportSize, which we rescale and call drawableSize, which we then pass in as the "aViewportSize" argument to SVGDrawingCallback (and store it as "mViewportSize"), and then we treat it as if it is, actually, the viewport size (even though it's not?)  (That last part is the non-naming part of my confusion about this.)

Comment 16

[Daniel Holbert [:dholbert]]

er, I guess that was "mViewport", not "mViewportSize", in the DrawingCallback, but the naming point stands.

>  we treat it as if it is, actually, the viewport size
To clarify, I'm talking about this (in the existing code):
> 278   nsRect svgRect(presContext->DevPixelsToAppUnits(mViewport.x),
> 279                  presContext->DevPixelsToAppUnits(mViewport.y),
> 280                  presContext->DevPixelsToAppUnits(mViewport.width),
> 281                  presContext->DevPixelsToAppUnits(mViewport.height));
[...]
> 288   presShell->RenderDocument(svgRect, renderDocFlags,
> 289                             NS_RGBA(0, 0, 0, 0), // transparent
> 290                             aContext);

The point is to render the whole viewport there -- the same viewport we told the presShell about, up a view levels, via our call to mSVGDocumentWrapper->UpdateViewportBounds(aViewportSize).

If we're using a scaled size there, won't we be rendering something that's the wrong size? (too large or too small)  It may not matter, visibly, if we're rendering something that's large, since we're clipping, but it's still potentially wasteful...

Comment 17

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> So I'm also a little uncomfortable with the "drawableSize" vs.
> "viewportSize" distinction.
> 
> It's partly a naming thing -- we have aViewportSize, which we rescale and
> call drawableSize, which we then pass in as the "aViewportSize" argument to
> SVGDrawingCallback (and store it as "mViewportSize"), and then we treat it
> as if it is, actually, the viewport size (even though it's not?)  (That last
> part is the non-naming part of my confusion about this.)

Heh, actually I changed the name to _emphasize_ this distinction. Note that we do not pass drawableSize to SVGDrawingCallback - for SVGDrawingCallback's aViewport parameter, we (essentially) pass in aViewportSize directly. We pass drawableSize to gfxCallbackDrawable because that's the size we're ultimately going to draw at.

Does that make sense?

Comment 18

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> To clarify, I'm talking about this (in the existing code):

I think this is actually not happening - we do indeed pass aViewportSize through to SVGDrawingCallback in the way you want. I've been staring at this code for hours so I may be misreading it, but I'm pretty sure that's true. =)

Comment 19

[Daniel Holbert [:dholbert]]

Oh, sorry - I misread. I saw drawableTransform being passed to SVGDrawingCallback, and misread it as drawableSize (which I thought was being passed as the viewport size).

So: never mind about that, thanks for clearing that up. :) Re-reading with that in mind...

(Disregard comment 15 and 16; sorry for the confusion)

(In reply to Seth Fowler [:seth] from comment #17)
> Heh, actually I changed the name to _emphasize_ this distinction.

(Yup, that makes sense - I was just concerned that we were losing the rename, one level of abstraction down. But again, I was misreading. :))

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add reftests for SVG image stretching and scaling.

I've addressed all the review comments on the original version of part 2. I've also added a couple of new tests, so I'm rerequesting review. Only the "no-viewbox" version fails without the patch in part 1, but I included both just to be thorough, since this stuff didn't have good test coverage up to now.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 773697 [details] [diff] [review]
(Part 1) - Make SVG images scale correctly.

OK, one other optional nit:

>   gfxRect sourceRect = unscaledTransform.Transform(aFill);
[...]
>+  gfxRect imageRect(0, 0, drawableSize.width, drawableSize.height);
>+  gfxRect subimage(aSubimage.x, aSubimage.y,
>+                   aSubimage.width, aSubimage.height);
>+  subimage.ScaleRoundOut(1.0 / scale.width, 1.0 / scale.height);

It's a non-obvious why we're creating a variable 'subimage' here, as a rescaled version of |aSubimage|. If we're scaling it, we probably should give it a more clearly-distinct name.

(Looks like it *really* already is a rescaled version of |aSubimage|, in m-c, going at least as far back as bug 600207's patch, but it's more obviously-confusing now that it's getting its value *directly* from aSubimage. :))

SO: Maybe rename these 3 variables (sourceRect, imageRect, and subimage) to have a "drawable" prefix, to distinguish them (particularly 'subimage') from any imagelib analogs? (These ones are in terms of our drawable's coordinate-space, IIUC, hence the 'drawable' prefix -- to match 'drawableSize')

Aside: That also makes me wonder whether we should be scaling 'aFill' as well... looks like that's the only sizing arg that we pass *directly* through to DrawPixelSnapped, without any scaling...  That's probably a separate issue, though.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 773697 [details] [diff] [review]
(Part 1) - Make SVG images scale correctly.

>Bug 885939 (Part 1) - Make SVG images scale correctly. r=dholbert

This commit message is also a bit vague. (And honestly, probably misleading, since there are very likely still a some other svg image-scaling bugs that we haven't yet fixed :))

How about:
 "For scaled SVG images, resize the gfxDrawable instead of the viewport"
or something like that?

r=me on part 1, with a clearer commit message and comment 14 and comment 21 addressed / responded-to.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 773715 [details] [diff] [review]
(Part 2) - Add reftests for SVG image stretching and scaling.

Nit: Looks like all of the -ref files, along with the testcase background-stretch-1.html, are missing <html></html> wrappers.

Probably worth adding those, for consistency.

>+++ b/layout/reftests/svg/as-image/background-stretch-1-ref.html
>+<body>
>+  <div class="container">
>+    <div class="left-spacer"></div>
>+    <div class="stretch"></div>
>+    <div class="right-spacer"></div>
>+  </div>
>+</body>
>+

This reference case has a straggling newline at the end -- remove it (or populate it with </html>) :)

>+++ b/layout/reftests/svg/as-image/background-stretch-1.html
[...]
>+<body>
>+  <div></div>
>+</body>
>+

Same with this testcase.

r=me with that

Comment 24

[Seth Fowler [:seth] [:s2h]]

Thanks for the reviews!

(In reply to Daniel Holbert [:dholbert] from comment #14)
> so, instead of storing a matrix
> here, how about we just pass in a gfxSize for the scale factors?

That sounds good; I'll make that change.

Comment 25

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> SO: Maybe rename these 3 variables (sourceRect, imageRect, and subimage) to
> have a "drawable" prefix, to distinguish them (particularly 'subimage') from
> any imagelib analogs?

Sure, I'll make that change. I agree that that seems more consistent, now that I've introduced the 'drawable' concept.

> Aside: That also makes me wonder whether we should be scaling 'aFill' as
> well... looks like that's the only sizing arg that we pass *directly*
> through to DrawPixelSnapped, without any scaling...  That's probably a
> separate issue, though.

I wondered that when I was originally making the patch for bug 600207, but after some thought (and experimentation to verify my reasoning) it became clear that aFill doesn't need to be scaled. The reason is that aFill is the rect that we're drawing _into_, and that doesn't change. All that we're really changing here is the way that we express the other drawing parameters to map the image into that rect. The intention is that the final image will be the same size as it would've been before.

Comment 26

[Daniel Holbert [:dholbert]]

Ah, makes sense. Thanks for the explanation!

Comment 27

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Scale SVG images using the graphics context matrix instead of the viewport.

Addressed review comments.

Comment 28

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add reftests for SVG image stretching and scaling.

Addressed review comments.

Comment 29

[Seth Fowler [:seth] [:s2h]]

All that's left is a try push, but I'm not having much luck pushing to try. Will give it another shot later today.

Comment 30

[Seth Fowler [:seth] [:s2h]]

https://tbpl.mozilla.org/?tree=Try&rev=7ca818b56ec0

Comment 32

[Daniel Holbert [:dholbert]]

As noted in (duplicate) bug 890009 comment 1, this bug causes an icon in the apple.com header to render at the wrong size, during full-page zoom.

This bug is a regression introduced by bug 600207's patch, which landed in the Firefox 24 timeframe. Once we've fixed this on trunk, we should probably backport the fix to Firefox 24 (currently Aurora) to be sure we don't ship this regression when 24 hits release.

Comment 33

[Daniel Holbert [:dholbert]]

So IIUC, this patch scales our viewport to be closer to the size of the background-region that's being drawn.  In this case, that means we scale down from a huge number, and we end up getting 0 instead of something smallish, due to rounding error.  I think.

(In particular, if I put a breakpoint in nsSVGOuterSVGFrame::BuildDisplayList() and print out the frame tree, I get a SVG rect with nonzero width in current mozilla-central, but with zero width with the patch applied.  (using one of the failing testcases))

This is technically a bug (though it may not be one we care about very much, since it's so edge-casey).  I'd suggest filing a followup on this, and then annotate the failing tests as "fails" with a mention of that bug.

Comment 34

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Disable SVG reftests involving extreme viewboxes that fail due to integer overflow.

This patch disables the failing reftests, per discussion between dholbert and me. I have not been able to resolve the issue so far. Since this is a corner case that only occurs when extreme coordinates are involved, while the patch resolves problems that occur frequently on the web, I think it's worth it to go ahead and push this in. I'll create a new bug to try to get this issue solved at some point in the future.

Comment 35

[Seth Fowler [:seth] [:s2h]]

I should provide more info to clear up some things in comment 33:

- This patch does not scale the viewport at all. It removes viewport scaling, returning us (in that respect) to the status quo before bug 600207. There definitely is an issue with overflow somewhere, but I strongly suspect it is occurring in the graphics subsystem rather than anywhere to do with the viewport.

- I am not able to reproduce dholbert's report above of the SVG rect getting zero width with this patch applied. Not sure what's going on there, but at this point I think that's a red herring. Really, the thing that has changed here is the transformation matrix on the Thebes context when we call RenderDocument. Something, somewhere, is not handling that well.

Comment 36

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Disable SVG reftests involving extreme viewboxes that fail due to integer overflow.

Update the bug reference to refer to the new bug.

Comment 37

[Seth Fowler [:seth] [:s2h]]

Try is not currently accessible but as far as I know those tests were the only issues with this patch. I've gone ahead and pushed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c49f7bbd129
https://hg.mozilla.org/integration/mozilla-inbound/rev/de46c9ce71ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2f1cc4b2a41

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c49f7bbd129
https://hg.mozilla.org/mozilla-central/rev/de46c9ce71ef
https://hg.mozilla.org/mozilla-central/rev/c2f1cc4b2a41

Comment 39

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 774862 [details] [diff] [review]
(Part 1) - Scale SVG images using the graphics context matrix instead of the viewport.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 600207
User impact if declined: Rendering issues on popular websites that use SVG images (e.g. apple.com).
Testing completed (on m-c, etc.): The patch has been on m-c for 5 days with no known issues.
Risk to taking this patch (and alternatives if risky): This patch involves substantial changes to the way SVG images are rendered, which does have some risk, but since the current implementation is broken in a way that is likely to have a serious negative impact on user experience, I think the risk is justified.
String or IDL/UUID changes made by this patch: None.

Comment 40

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 774864 [details] [diff] [review]
(Part 2) - Add reftests for SVG image stretching and scaling.

[Approval Request Comment]

See above.

Comment 41

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 776614 [details] [diff] [review]
(Part 3) - Disable SVG reftests involving extreme viewboxes that fail due to integer overflow.

[Approval Request Comment]

See above.

Comment 42

[Daniel Holbert [:dholbert]]

For approval purposes: note also that this bug is a regression, which currently only affects Aurora (since it regressed when current-aurora was on trunk).

If we backport the fix to Aurora, then that will prevent us from ever shipping this regression to release/beta users, which would be great.

Comment 43

[bhavana bajaj [:bajaj]]

Comment on attachment 774862 [details] [diff] [review]
(Part 1) - Scale SVG images using the graphics context matrix instead of the viewport.

Given this is a regression and that it impacts high-profile web-sites, looks fine to land.

Can you please recommend applications/websites that may be SVG intensive so QA can help verify those for sure along with the regular Aurora/Beta testing that we'd get.that way we'd be proactive on our side and in a way mitigate the medium risk that was pointed here.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc9b10335396
https://hg.mozilla.org/releases/mozilla-aurora/rev/44c000c77e9d
https://hg.mozilla.org/releases/mozilla-aurora/rev/5facfc2bf316

Comment 45

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 25 beta 4: - the Australis theme mockup and the SVG images from apple.com are properly displayed.

Verified on Windows 7, Ubuntu 12.10 and Mac OS X 10.8:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20131001024718

I'm waiting for the additional suggestions requested in Comment 43, before setting the status for Firefox 25 to verified.

Comment 46

[Simona Badau, Desktop QA]

Since there are no additional suggestions, setting the status flag for Firefox 25 to verified.