Closed Bug 1106602 Opened 5 years ago Closed 5 years ago

SVG Image with nonuniform scale in CSS transform (e.g. to simulate rotation about vertical axis) gets rendered with uniform scale -- shrinking instead of rotating

Categories

(Core :: ImageLib, defect, blocker)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- wontfix
firefox35 + fixed
firefox36 + fixed
firefox37 + fixed

People

(Reporter: Harald, Assigned: seth)

References

Details

(Keywords: regression)

Attachments

(4 files, 4 obsolete files)

Image rotation ends up playing with the drawn z-depth or scale.  This used to be a Nightly issue but was introduced in Firefox 34.

http://michaelobriena.github.io/firefox/imageRotation/
Reported in bug 1084021.
Flags: needinfo?(bgirard)
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3488976ecf0f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822131153
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d51132a0099
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822131352
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3488976ecf0f&tochange=3d51132a0099

Regressed by
3d51132a0099	Seth Fowler — Bug 1043560 - Refactor the imgIContainer::Draw API. r=tn,dholbert,jwatt,mwu,mattwoodrow,roc sr=jrmuize
Keywords: regression
This only happens with .svg images.  Added more to the URL specified above to show that it is specific to that format.
Seth, can you help here? Thanks
Flags: needinfo?(seth)
I'm investigating this now.
Flags: needinfo?(seth)
Flags: needinfo?(bgirard)
Here's a standalone, non-animated testcase that demonstrates the problem. This example produces a circle on Firefox 37 and an ellipse on Chrome 39.
So the problem is that transformations on the gfxContext (ones produced by CSS transforms and the like) should not be applied to the viewport. That results in the SVGDrawingCallback code drawing as if there were no transformation in effect, since the image size and the viewport size cancel out. We don't want them to cancel out, so that the SVGDrawingCallback code will apply the scale factors that we want.

This patches makes us compute a pre-transform viewport in ComputeSnappedImageDrawingParameters. It's treated as a default that gets used if the caller of DrawImageInternal doesn't pass a different viewport in.
Attachment #8533526 - Flags: review?(dholbert)
Assignee: nobody → seth
Status: NEW → ASSIGNED
Here are tests that nonuniform scaling works for SVG-as-image. They compare a set of nonuniform scales using both 2d and 3d transforms and make sure that the result differs from the related uniform scales.
Attachment #8533527 - Flags: review?(dholbert)
(Here's a simplified testcase, with no 3d transforms and no need for an explicit matrix -- just a scalex(0.5).)
Comment on attachment 8533527 [details] [diff] [review]
(Part 2) - Add tests for nonuniform scaling of SVG-as-image

>diff --git a/layout/reftests/svg/as-image/blue-circle.svg b/layout/reftests/svg/as-image/blue-circle.svg
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/svg/as-image/blue-circle.svg
>@@ -0,0 +1,6 @@
>+<?xml version="1.0"?>
>+<!-- Any copyright is dedicated to the Public Domain.
>+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
>+<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">

No need for an additional SVG resource here -- just use the existing files squaredCircle-viewBox-100x100.svg or squaredCircle-viewBox-noSize.svg.

>+++ b/layout/reftests/svg/as-image/nonuniform-scale-2d.html
>+  <script>
>+    // Apply the style to the document.
>+    var sheet = document.createElement('style');
>+    sheet.innerHTML = style;
>+    document.body.appendChild(sheet);

Seems like it'd be simpler to just set the <img> element's style attribute, instead of creating & appending an entire <style> element. 
 e.g.:
   var img = document.getElementsByTagName("img")[0];
   img.style = style;

Not a big deal, though.

(Applies to both test files, if you do make this tweak.)

>diff --git a/layout/reftests/svg/as-image/reftest.list b/layout/reftests/svg/as-image/reftest.list
>+# Tests that nonuniform scales work with SVG-as-image.
>+!= nonuniform-scale-2d.html?0.3&1.0  nonuniform-scale-2d.html?0.3&0.3
>+!= nonuniform-scale-2d.html?0.3&1.0  nonuniform-scale-2d.html?1.0&1.0
[etc]

We should have at least one "==" test for this bug, instead of relying entirely on "!=" tests... (Otherwise, we don't really know that we're rendering it correctly.)

I'd expect that you should be able to make a true reference case for nonuniform-scale-2d.html that just sets the "width" & "height" properties to fixed pixel values (e.g. 60px & 200px), instead of using transforms.  You'd also need a separate (hg cp'd) SVG file for use in the reference case, with preserveAspectRatio="none" added.  (Might conceivably need a 'fuzzy' annotation, too -- looks visually like a perfect match in a quick local test that I did, but Try will tell us for sure.)

You could use the same URL parameters that you're using on the testcase (e.g. ?0.3&1.0) and multiply them by 200px to get the correct width & height properties.  So, it should look very similar to your existing testcase, code-wise.

Could you add that reference case & some tests with it? (at least the "2d" version, to get some coverage)
(In reply to Daniel Holbert [:dholbert] from comment #12)
> Could you add that reference case & some tests with it?

(by "some tests with it" I meant "some lines to reftest.list that use it" - not new testcase files.)
Comment on attachment 8533526 [details] [diff] [review]
(Part 1) - Use pretransform dest rect as default SVG-as-image viewport

>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -5498,30 +5498,35 @@ struct SnappedImageDrawingParameters {
>   nsIntSize size;
>   // The region in tiled image space which will be drawn, with an associated
>   // region to which sampling should be restricted.
>   ImageRegion region;
>+  // The default viewport size for SVG images, which we use unless a different
>+  // one has been explicitly specified.
>+  nsIntSize viewportSize;

Two things:
 - Let's rename viewportSize to "svgViewportSize" (or similar), to clearly mark it as a SVG-specific thing. (That makes it easier to conceptually separate from other member-vars whose names sound vaguely viewport-flavored, like e.g. "size" and "region")
 - Usually, viewportSize (svgViewportSize) should be the same as 'size'. Let's add a parenthetical to clarify that, & explain why it might be different -- e.g.
   (This should be the same as 'size', unless we're being rendered to a
   gfxContext that's scaled by different amounts in each dimension.)

>@@ -5703,17 +5708,22 @@ ComputeSnappedImageDrawingParameters(gfx
>+  nsIntSize viewport(NSAppUnitsToIntPixels(dest.width, aAppUnitsPerDevPixel),
>+                     NSAppUnitsToIntPixels(dest.height, aAppUnitsPerDevPixel));
>+
>+  return SnappedImageDrawingParameters(transform, intImageSize,
>+                                       region, viewport);

Here as well, let's call this svgViewportSize to be a bit more explicit about its purpose.

Also: can we assert that viewportSize is the same as intImageSize in the usual case here? e.g. I'd expect we could assert the following:

   !currentMatrix.IsIdentity() ||
   aImage->GetType() != imgIContainer::TYPE_VECTOR ||
   viewportSize == intImageSize

In other words: with a identity transform (really, any uniform transform), a SVG image should end up with the same image-size and viewport-size.  This would ensure that this patch doesn't change our behavior for the no-CSS-transform case, and help document our expectations for the viewport size. (enforcing the parenthetical that I suggested adding higher up)

r=me with something along those lines. Thanks!
Attachment #8533526 - Flags: review?(dholbert) → review+
Component: Graphics → ImageLib
Hardware: x86 → All
Version: unspecified → Trunk
Summary: Image rotation ends up playing with the drawn z-depth or scale → SVG Image with nonuniform scale in CSS transform (e.g. to simulate rotation about vertical axis) gets rendered with uniform scale -- shrinking instead of rotating
Here's a new version that should address the comments above.

I didn't add an explicit assertion like the one recommended in comment 14, because the code now pretty much implements what that assertion would have asserted - in other words, it's correct by construction.
Attachment #8534736 - Flags: review?(dholbert)
Attachment #8533526 - Attachment is obsolete: true
Comment on attachment 8534736 [details] [diff] [review]
(Part 1) - Use pretransform dest rect as default SVG-as-image viewport

>@@ -5619,30 +5626,39 @@ ComputeSnappedImageDrawingParameters(gfx
>+  gfxSize snappedUnscaledDest =
>+    gfxSize(NSAppUnitsToIntPixels(dest.width, aAppUnitsPerDevPixel),
>+            NSAppUnitsToIntPixels(dest.height, aAppUnitsPerDevPixel));
>   gfxSize snappedScaledDest =
[...]
>+  nsIntSize svgViewportSize = currentMatrix.IsIdentity()
>+      ? intImageSize
>+      : aImage->OptimalImageSizeForDest(snappedUnscaledDest,
>+                                        imgIContainer::FRAME_CURRENT,
>+                                        aGraphicsFilter, aImageFlags);

Feels a bit wasteful to compute snappedUnscaledDest here, when in the common case (no CSS transform), we don't actually use it.

Maybe better to split the ternary condition into an if/else, and move "snappedUnscaledDest" down into the "else"?
e.g.:
 nsIntSize svgViewportSize;
 if (currentMatrix.IsIdentity()) {
   svgViewportSize = intImageSize;
 } else {
   gfxSize snappedUnscaledDest = [...]
   svgViewportSize = [ ... ]
 }

I'm also not sure we need to bother calling OptimalImageSizeForDest() (and taking the hit of a virtual function-call), since svgViewportSize only matters for SVG images, and OptimalImageSizeForDest is basically a no-op in that case.  So unless you see that potentially changing soon, I'd lean towards avoiding the OptimalImageSizeForDest call, perhaps.

r=me with the above addressed in whatever way you see fit.
Attachment #8534736 - Flags: review?(dholbert) → review+
Thanks for the review!

(In reply to Daniel Holbert [:dholbert] from comment #16)
> I'm also not sure we need to bother calling OptimalImageSizeForDest() (and
> taking the hit of a virtual function-call), since svgViewportSize only
> matters for SVG images, and OptimalImageSizeForDest is basically a no-op in
> that case.  So unless you see that potentially changing soon, I'd lean
> towards avoiding the OptimalImageSizeForDest call, perhaps.

Hmmm.. calling OptimalImageSizeForDest was actually the entire reason I moved all this code around, but actually now that I think about it, I'm not sure that it helps. Not so much because it's a no-op (that could well change) but because anything that it does do is going to be done thinking that we're going to be drawing at that size, which isn't actually true here.

I'll replace OptimalImageSizeForDest with the manual calculation that I had before. That also eliminates the need for snappedUnscaledDest totally.
This version goes back to the previous manual calculation instead of using OptimalImageSizeForDest.
Attachment #8534736 - Attachment is obsolete: true
This updated version of part 2 adds equality reftests. I switched to a square with a stroke instead of a circle, to reduce the risk of fuzz. It also makes other small tweaks suggested above.
Attachment #8533527 - Attachment is obsolete: true
Attachment #8533527 - Flags: review?(dholbert)
OK, I think we're good to go now if try looks good. Here's a try run with reftests on all platforms. Hopefully we don't need fuzz:

https://tbpl.mozilla.org/?tree=Try&rev=1ae5b303c044
Comment on attachment 8534764 [details] [diff] [review]
(Part 2) - Add tests for nonuniform scaling of SVG-as-image

>diff --git a/layout/reftests/svg/as-image/blue-square-par-none.svg b/layout/reftests/svg/as-image/blue-square-par-none.svg
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/svg/as-image/blue-square-par-none.svg
>@@ -0,0 +1,6 @@
>+<?xml version="1.0"?>
>+<!-- Any copyright is dedicated to the Public Domain.
>+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
>+<svg preserveAspectRatio="none" viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
>+<rect x="1"  y="1" width="100" height="100" stroke-width="10" stroke="lightblue" fill="blue"/>

Nit: the rect here is the same size as its viewport (100 by 100), but it's offset (x=1 y=1), so its normal rendering looks off-by-one (which could be confusing in cases of failures).

Before landing, could you fix that by either dropping the x="1" y="1" (so that the rect exactly fills the viewBox), or make it height="98" width="98" (so that it's inset by 1px on all sides)?

(observation: The 10px stroke-width bleeds outside of the viewBox, too, but that shouldn't cause problems given how the file's being used, so not a big deal.)
Attachment #8534764 - Flags: review+
(The off-by-one-looking oddness can be seen if you view "nonuniform-scale-2d.html" directly -- the light blue stroke is smaller on the bottom & right edges than on the top & left edges, due to x=1 y=1 shifting the rect in that direction.)
OK, here are some final tweaks to the tests. The SVG files now draw the rects at (0, 0) - and it is rects now, because I've switched to drawing two rects instead of a rect with a stroke.

In the interest of detecting fuzz, I think this needs one more try job once try reopens.
Attachment #8534764 - Attachment is obsolete: true
Seth, is this good to land? Looks like the only issues on the Try run are infra-related.
Flags: needinfo?(seth)
I was just waiting on inbound to open up. Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a321e217332d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d56375d76f
Flags: needinfo?(seth)
https://hg.mozilla.org/mozilla-central/rev/a321e217332d
https://hg.mozilla.org/mozilla-central/rev/f9d56375d76f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8534763 [details] [diff] [review]
(Part 1) - Use pretransform dest rect as default SVG-as-image viewport

Approval Request Comment
[Feature/regressing bug #]: 1043560
[User impact if declined]: Incorrect rendering of transformed SVG images.
[Describe test coverage new/current, TBPL]: New tests added, on m-c.
[Risks and why]: Low risk; should not change behavior for images that aren't transformed SVGs.
[String/UUID change made/needed]: None.
Attachment #8534763 - Flags: approval-mozilla-beta?
Attachment #8534763 - Flags: approval-mozilla-aurora?
Comment on attachment 8535162 [details] [diff] [review]
(Part 2) - Add tests for nonuniform scaling of SVG-as-image

(Also requesting uplift for the new tests.)
Attachment #8535162 - Flags: approval-mozilla-beta?
Attachment #8535162 - Flags: approval-mozilla-aurora?
Attachment #8535162 - Flags: approval-mozilla-beta?
Attachment #8535162 - Flags: approval-mozilla-beta+
Attachment #8535162 - Flags: approval-mozilla-aurora?
Attachment #8535162 - Flags: approval-mozilla-aurora+
Attachment #8534763 - Flags: approval-mozilla-beta?
Attachment #8534763 - Flags: approval-mozilla-beta+
Attachment #8534763 - Flags: approval-mozilla-aurora?
Attachment #8534763 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.