379616 - Switch SVG's frame mRect to app units

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Attachment: mRect in app units

As requested in bug 265084.

Comment 1

[Jonathan Watt [:jwatt]]

Comment on attachment 263616 [details] [diff] [review]
mRect in app units

Preliminary comments: user unit/px values in SVG are CSS pixels, not device pixels. For most devices they'll be the same so we won't see a difference, but maybe our use of AppUnitsPerDevPixel() instead of AppUnitsPerCSSPixel() has been part of the reason SVG isn't being scaled properly on high resolution devices. I think we want the latter in general and this case in particular. If so, then...

>   /*
>    * Inflate a floating-point rect to a nsRect

can you change this comment to "Convert a rect from app units to CSS pixel units"

>    */
>   static nsRect
>-  ToBoundingPixelRect(double xmin, double ymin, double xmax, double ymax);
>+  ToBoundingPixelRect(PRInt32 aAppUnitsPerDevPixel,
>+                      double xmin, double ymin, double xmax, double ymax);

Given the ambiguity over CSS pixels versus device pixels I think this function name could be better. Also, the function is independent of "Bounding". Perhaps ToCSSPixelRect?

Also perhaps just pass in the PresContext as an argument and let this function do the AppUnitsPerCSSPixel call.

Comment 2

[tor]

(In reply to comment #1)
> (From update of attachment 263616 [details] [diff] [review])
> Preliminary comments: user unit/px values in SVG are CSS pixels, not device
> pixels. For most devices they'll be the same so we won't see a difference, but
> maybe our use of AppUnitsPerDevPixel() instead of AppUnitsPerCSSPixel() has
> been part of the reason SVG isn't being scaled properly on high resolution
> devices. I think we want the latter in general and this case in particular. If
> so, then...

If you apply this patch and the patch from bug 370006, SVG behaves properly with dpi set to 200.

Comment 3

[tor]

Attachment: some updates per comments

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 264270 [details] [diff] [review]
some updates per comments

>+nsSVGForeignObjectFrame::GetFrameForPoint(const nsPoint &aPoint)
> {
>   nsIFrame* kid = GetFirstChild(nsnull);
>   if (!kid) {
>-    *hit = nsnull;
>-    return NS_OK;
>+    return nsnull;
>   }
>   nsPoint pt;
>-  nsresult rv = TransformPointFromOuterPx(x, y, &pt);
>+  nsresult rv =
>+    TransformPointFromOuterPx(PresContext()->AppUnitsToDevPixels(aPoint.x),
>+                              PresContext()->AppUnitsToDevPixels(aPoint.y),
>+                              &pt);

TransformPointFromOuterPx just converts the pixels back to app units. I think rather than adding a second conversion here you could just remove the conversion from TransformPointFromOuterPx so that the unit is app units throughout this code path.


> nsSVGOuterSVGFrame::GetFrameForPoint(const nsPoint& aPoint)
> {
>   // XXX This algorithm is really bad. Because we only have a
>   // singly-linked list we have to test each and every SVG element for
>   // a hit. What we really want is a double-linked list.
> 
>-  float x = PresContext()->AppUnitsToDevPixels(aPoint.x);
>-  float y = PresContext()->AppUnitsToDevPixels(aPoint.y);
>+//  float x = PresContext()->AppUnitsToDevPixels(aPoint.x);
>+//  float y = PresContext()->AppUnitsToDevPixels(aPoint.y);

Any reason not to just delete these lines? Also the comment above them could die since nsSVGUtils::HitTestChildren now traverses frames backwards.


>+  gfxPoint devicePoint =
>+    context.DeviceToUser(gfxPoint(aPoint.x,
>+                                  aPoint.y) / PresContext()->AppUnitsPerDevPixel());

Can you change the incorrectly named "devicePoint" to "userSpacePoint" while you're here?

Comment 5

[tor]

Attachment: more updates

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 264418 [details] [diff] [review]
more updates

(In reply to comment #2)
> If you apply this patch and the patch from bug 370006, SVG behaves properly
> with dpi set to 200.

As I've noted in that bug, I think the patch there is wrong. These patches may fix the symptoms for the testcases you've looked at, but I'm fairly convinced some of the places you're using device pixels are wrong. Masking the symptoms with incorrect code could be worse than not fixing them since it will make it more difficult to figure out and fix things correctly later for other cases that will inevitably crop up. I'd rather see the code incrementally moved forward to the correct fix with correct patches.

>+  float x = PresContext()->AppUnitsToDevPixels(aIn.x);
>+  float y = PresContext()->AppUnitsToDevPixels(aIn.y);
>+  nsSVGUtils::TransformPoint(inverse, &x, &y);
>+  *aOut = nsPoint(PresContext()->DevPixelsToAppUnits(nscoord(x)),
>+                  PresContext()->DevPixelsToAppUnits(nscoord(y)));

SVG transforms create a user space (i.e. CSS pixel) coordinate space. Coordinate values x and y need to be converted to these units not device pixels.

>@@ -1312,17 +1306,18 @@ nsSVGGlyphFrame::ContainsPoint(float x, 
>     gfx->SetMatrix(matrix);
> 
>     if (!cp) {
>       pt.x += metrics.mAdvanceWidth;
>     }
>   }
> 
>   gfx->IdentityMatrix();
>-  return gfx->PointInFill(gfxPoint(x, y));
>+  return gfx->PointInFill(gfxPoint(aPoint.x,
>+                                   aPoint.y) / PresContext()->AppUnitsPerDevPixel());

The glyphs are drawn into a user units coordinate context, not device pixels.

>+nsSVGImageFrame::GetFrameForPoint(const nsPoint &aPoint)
> {
>   if (GetStyleDisplay()->IsScrollableOverflow() && mImageContainer) {
>     PRInt32 nativeWidth, nativeHeight;
>     mImageContainer->GetWidth(&nativeWidth);
>     mImageContainer->GetHeight(&nativeHeight);
> 
>     nsCOMPtr<nsIDOMSVGMatrix> fini = GetImageTransform();
> 
>     if (!nsSVGUtils::HitTestRect(fini,
>                                  0, 0, nativeWidth, nativeHeight,
>-                                 x, y)) {
>-      *hit = nsnull;
>-      return NS_OK;
>+                                 PresContext()->AppUnitsToDevPixels(aPoint.x),
>+                                 PresContext()->AppUnitsToDevPixels(aPoint.y))) {

GetImageTransform get's the transform to user space.

>+nsSVGInnerSVGFrame::GetFrameForPoint(const nsPoint &aPoint)
> {
>   if (GetStyleDisplay()->IsScrollableOverflow()) {
>     float clipX, clipY, clipWidth, clipHeight;
>     nsCOMPtr<nsIDOMSVGMatrix> clipTransform;
> 
>     nsSVGElement *svg = NS_STATIC_CAST(nsSVGElement*, mContent);
>     svg->GetAnimatedLengthValues(&clipX, &clipY, &clipWidth, &clipHeight, nsnull);
> 
>     nsSVGContainerFrame *parent = NS_STATIC_CAST(nsSVGContainerFrame*,
>                                                  mParent);
>     clipTransform = parent->GetCanvasTM();
> 
>     if (!nsSVGUtils::HitTestRect(clipTransform,
>                                  clipX, clipY, clipWidth, clipHeight,
>-                                 x, y)) {
>-      *hit = nsnull;
>-      return NS_OK;
>+                                 PresContext()->AppUnitsToDevPixels(aPoint.x),
>+                                 PresContext()->AppUnitsToDevPixels(aPoint.y))) {

parent->GetCanvasTM() returns the transform to user space.

>+nsSVGUtils::ToAppPixelRect(nsPresContext *aPresContext, const gfxRect& rect)
> {
>   return nsRect(nscoord(floor(rect.X())),
>                 nscoord(floor(rect.Y())),
>                 nscoord(ceil(rect.XMost()) - floor(rect.X())),
>-                nscoord(ceil(rect.YMost()) - floor(rect.Y())));
>+                nscoord(ceil(rect.YMost()) - floor(rect.Y()))).ScaleRoundOut(aPresContext->AppUnitsPerDevPixel());

It seems to me that every single rect that's passed in is in user units, which would make pretending they're in device pixel wrong.

Comment 7

[tor]

Robert, Eli - as the resident experts in gecko units, can you offer some advice to stop us from going backwards trying to fix this problem and the related high-resolution svg problem (bug 370006)?

Comment 8

[Eli Friedman]

Okay, I'll take a shot:

We have many coordinate systems here: one is the layout coordinate system, which is in app units.  There is also the SVG "device" coordinate system, which should be in CSS pixels.  Then there is the SVG user coordinate system, which is the transformed version of the "device" coordinate system transformed by a user-defined transform.  The other coordinate system is the Thebes coordinate system, which is in device pixels.

Because of all of this, the transform matrix needed for painting and invalidation is different from the transform matrix needed for anything visible to scripts.

I'll take a look at the SVG code and try to give some more specific advice later.

Comment 9

[tor]

(In reply to comment #8)
> We have many coordinate systems here: one is the layout coordinate system,
> which is in app units.  There is also the SVG "device" coordinate system, which
> should be in CSS pixels.

I don't know of any SVG device coordinate system - there's the coordinates resulting from applying the current user transform to the user coordinate system.

> Then there is the SVG user coordinate system, which
> is the transformed version of the "device" coordinate system transformed by a
> user-defined transform.  The other coordinate system is the Thebes coordinate
> system, which is in device pixels.

Maybe our problem is terminology?  My graphics experience inverts these descriptions - the SVG user coordinate system is what's there before the application of the user transfrom.

> Because of all of this, the transform matrix needed for painting and
> invalidation is different from the transform matrix needed for anything visible
> to scripts.

Except in specific cases in the DOM API (screenCTM and the like), all the DOM deals with is the user coordinate system.

Comment 10

[Jonathan Watt [:jwatt]]

My comments below are meant to help us align our worlds and move toward a common understanding of terminology. I'm not trying to be unnecessarily picky.

(In reply to comment #8)
> We have many coordinate systems here: one is the layout coordinate system,
> which is in app units.  There is also the SVG "device" coordinate system, which
> should be in CSS pixels.

In SVG 1 user unit == 1 CSS px by definition, regardless of the transform in use. I'd rather avoid the term "SVG device coordinate system" since that's not a term I've seen anyone else use. I guess you mean the "initial/global coordinate system" (the coordinate system in place for a root <svg> element before the implicit transform any viewBox attribute on it may have is applied.) Having said that, yes, user units being equal to CSS px, the initial coordinate system for SVG is in CSS pixels (as are all other transformed coordinate systems in the SVG).

> Then there is the SVG user coordinate system, which
> is the transformed version of the "device" coordinate system transformed by a
> user-defined transform. 

Well "user coordinate system" just means the coordinate system in which unitless/CSS px lengths are interpreted. So for some lengths the initial ("device") coordinate system is also the user coordinate system. ;-)

> The other coordinate system is the Thebes coordinate
> system, which is in device pixels.

Hmm. So if I tell thebes to draw a line from x1,y1 to x2,y2 then I must convert these coordinates from CSS px/user units to device units first? I.e. use the nsPresContext methods to convert.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #10)
> Hmm. So if I tell thebes to draw a line from x1,y1 to x2,y2 then I must convert
> these coordinates from CSS px/user units to device units first? I.e. use the
> nsPresContext methods to convert.

Yes. It's a bit unfortunate. Wherever layout uses a gfxContext, we have to convert from appunits to device units.

In the past I've argued for having the gfxContext coordinate system be in appunits, which would simplify layout code and mitigate some of the confusion here...

Comment 12

[tor]

(In reply to comment #10)
> In SVG 1 user unit == 1 CSS px by definition, regardless of the transform in
> use. I'd rather avoid the term "SVG device coordinate system" since that's not
> a term I've seen anyone else use. I guess you mean the "initial/global
> coordinate system" (the coordinate system in place for a root <svg> element
> before the implicit transform any viewBox attribute on it may have is applied.)
> Having said that, yes, user units being equal to CSS px, the initial coordinate
> system for SVG is in CSS pixels (as are all other transformed coordinate
> systems in the SVG).

To quote the language from the specification:

One px unit is defined to be equal to one user unit. Thus, a length of "5px" is the same as a length of "5".

Note that at initialization, a user unit in the the initial coordinate system is equivalenced to the parent environment's notion of a px unit. Thus, in the the initial coordinate system, because the user coordinate system aligns exactly with the parent's coordinate system, and because often the parent's coordinate system aligns with the device pixel grid, "5px" might actually map to 5 devices pixels. However, if there are any coordinate system transformation due to the use of transform or viewBox attributes, because "5px" maps to 5 user units and because the coordinate system transformations have resulted in a revised user coordinate system, "5px" likely will not map to 5 device pixels. As a result, in most circumstances, "px" units will not map to the device pixel grid.

http://www.w3.org/TR/SVG11/coords.html#Units

Comment 13

[Eli Friedman]

(In reply to comment #10)
> I'd rather avoid the term "SVG device coordinate system" since that's not
> a term I've seen anyone else use. I guess you mean the "initial/global
> coordinate system" (the coordinate system in place for a root <svg> element
> before the implicit transform any viewBox attribute on it may have is applied.)

Yeah, that's what I meant.

> > Then there is the SVG user coordinate system, which
> > is the transformed version of the "device" coordinate system transformed by a
> > user-defined transform. 

> Well "user coordinate system" just means the coordinate system in which
> unitless/CSS px lengths are interpreted. So for some lengths the initial
> ("device") coordinate system is also the user coordinate system. ;-)

Okay, I guess I didn't explain very well.  I'm not too well-versed in SVG terminology.  Thanks for clarifying.

> > The other coordinate system is the Thebes coordinate
> > system, which is in device pixels.
> 
> Hmm. So if I tell thebes to draw a line from x1,y1 to x2,y2 then I must convert
> these coordinates from CSS px/user units to device units first? I.e. use the
> nsPresContext methods to convert.

Yes; gfxContext expects anything it sees to be in device pixels.

The issue in bug 370006 is how to modify the SVG code so that it always passes coordinates in the Thebes coordinate system.

One possibility is just to do a conversion before passing the coordinates to Thebes; that would work, but it would require adding conversions all over the place, and I'd be kinda worried about missing some spots.

Another possibility is to make the root SVG scale the gfxContext for painting so that it takes coordinates in the initial coordinate system.

tor's approach in bug 370006 is to make GetCanvasTM return the transform to the Thebes coordinate system.

The patch in this bug appears to be correct assuming the patch in bug 370006.

Comment 14

[Jeremy Lea]

Hi,
(In reply to comment #12)
> Note that at initialization, a user unit in the the initial coordinate system
> is equivalenced to the parent environment's notion of a px unit. Thus, in the
> the initial coordinate system, because the user coordinate system aligns
> exactly with the parent's coordinate system, and because often the parent's
> coordinate system aligns with the device pixel grid, "5px" might actually map
> to 5 devices pixels. However, if there are any coordinate system transformation
> due to the use of transform or viewBox attributes, because "5px" maps to 5 user
> units and because the coordinate system transformations have resulted in a
> revised user coordinate system, "5px" likely will not map to 5 device pixels.
> As a result, in most circumstances, "px" units will not map to the device pixel
> grid.

I might be out of line here, but from my reading of the 'fix the use of units bug', the decision taken on units in Gecko is that 'device pixels' in most standards means 'CSS pixels'.  So on a 600dpi device there are 6 'real device pixels' per 'standard device pixel = CSS pixels', and CSS pixel dpi is 100dpi which is the closest you can get to the 96dpi standard with an integer number of real device pixels.  Thus for the purposes of reading the SVG standard '5' = '5px' = 5 CSS pixels = '5px@72dpi = 5px@96dpi = 30px@600dpi' (assuming no user transform).

Thanks for looking into this bug Tim.  I am still maintaining the massive patch that's in bug 265084.  I'm busy with other things at the moment, but I'm also hoping to add some more compile time checks to root out places in the code which assume CSS pixels = device pixels.

Regards,
  -Jeremy

Comment 15

[Jonathan Watt [:jwatt]]

Robert has a clear explanation on his blog:
http://weblogs.mozillazine.org/roc/archives/2007/02/units_patch_lan.html

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> I might be out of line here, but from my reading of the 'fix the use of units
> bug', the decision taken on units in Gecko is that 'device pixels' in most
> standards means 'CSS pixels'.

Depends on the context of the standard, but in this context, yes.

Comment 17

[tor]

Comment on attachment 264418 [details] [diff] [review]
more updates

Probably post-1.9 work at this point, removing review request on now likely bitrotted patch.

Comment 18

[Jeremy Lea]

Attachment: Refresh patch to mozilla-central

This is an updated patch to mozilla-central.  I have lightly tested it, so there could still be problems.  Still trying to get reftests working...  I'm working off mozilla-central now, and have my repo up at http://www.openpave.org/cgi-bin/hgweb.cgi.

The patch also fixes up a lot of nsRect misuse, which makes it easier to see what is in pixels and what is in app units.

I hope you don't mind me obsoleting the old patch.

Comment 19

[tor]

Comment on attachment 331688 [details] [diff] [review]
Refresh patch to mozilla-central

Noticed a couple of small things reading through the patch.  First, you need to change the IID of nsISVGChildFrame.  Second, was the removal of the IsDisabled() check in nsSVGForeignObjectFrame::GetFrameForPointSVG intentional?

Comment 20

[Jeremy Lea]

Attachment: Address initial comments

Oops, merging patches from too many sources...  I have used the new IID from the old patch.

Comment 21

[Jeremy Lea]

Attachment: Fix bit rot...

Fix post churn of nsSVGUtils.cpp

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

tor, do you want me to review this? I think Jeremy needs a quick review here to avoid rot.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(And we need to land this before we can land 448830, which rots even faster)

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

tor, can you sr+ that too please? :-)

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, I really should check this in so we can get on with 448830

Comment 26

[Benjamin Smedberg]

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/e92e18d52d71

Comment 27

[Benjamin Smedberg]

*** 31282 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | svgbase bounding rect width - got 45, expected 0
*** 31283 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | svgbase bounding rect height - got 20, expected 0
*** 31284 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | svgbase bounding rect right - got 45, expected 0
*** 31285 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | svgbase bounding rect bottom - got 20, expected 0
*** 31298 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | svgrect bounding rect width - got 45, expected 0
*** 31299 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | svgrect bounding rect height - got 20, expected 0
*** 31300 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | svgrect bounding rect right - got 45, expected 0
*** 31301 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | svgrect bounding rect bottom - got 20, expected 0

backed out

Comment 28

[Jeremy Lea]

The failing tests are from bug 111034, which landed after I did my tests.  I think the problem is with that bug, and is exposed because that bug thinks SVG's mRect is in app units, not pixels, so scales it down and gets zero, so the tests pass.  Although 45/60 should round to 1.  But, this shouldn't happen because the code for bug 111034 explicitly tests for SVG elements and returns an empty rectangle.  So that means that those tests are not sufficient to actually test for an SVG element.  I think this is because the tests are calling nsGenericElement::IsNodeOfType, not nsINode::IsNodeOfType, but I could be wrong on that.

Comment 29

[Robert Longson [:longsonr]]

It's not failing in the part that tests for SVG elements. Its failing in getBoundingClientRect not matching getComputedStyle any more.

getComputedStyle for the width and height on the svg elements returns auto which is converted in the test harness to 0.

diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -1247,9 +1247,6 @@
     nsRect r;
     nsIFrame* outer = nsSVGUtils::GetOuterSVGFrameAndCoveredRegion(aFrame, &r);

returned 45, 20 without (and presumably also with) the patch

     if (outer) {
-      // r is in pixels relative to 'outer', get it into appunits
-      // relative to aRelativeTo
-      r.ScaleRoundOut(1.0/aFrame->PresContext()->AppUnitsPerDevPixel());

Was divided by 60 and then later rounded to 0 by caller. This no longer happens.

The issue seems to me to be that with this patch nsSVGUtils::GetOuterSVGFrameAndCoveredRegion is still returning pixels rather than app units.

Comment 30

[Robert Longson [:longsonr]]

Attachment: fix up test

I've compiled the patch now. The issue is that width and height in SVG are attributes rather than properties so the test that failed is wrong.

nsSVGUtils::GetOuterSVGFrameAndCoveredRegion returns 45 x 20 without the patch and 45 * 60 x 20 * 60 with the patch which is right I think. The new code returns this directly in layout/base/nsLayoutUtils.cpp whereas the old code divided the 45 x 20 by 60 to get really small numbers that were then rounded to 0.

So I think the old code was wrong in layout/base/nsLayoutUtils.cpp...

-      // r is in pixels relative to 'outer', get it into appunits
-      // relative to aRelativeTo
-      r.ScaleRoundOut(1.0/aFrame->PresContext()->AppUnitsPerDevPixel());

Which is the inverse of what it should have been, isn't it?

This means that you can't land this patch without the other one as either on their own will make the test fail.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't getComputedStyle on an SVG element with "width" or "height" attributes actually return that width or height?

Comment 32

[Robert Longson [:longsonr]]

I don't think so.

Width and height are not listed in http://www.w3.org/TR/SVG/styling.html#SVGStylingProperties as styling properties.

And they're always referred to as attribute definitions rather than CSS properties.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 334678 [details] [diff] [review]
fix up test

okay, this makes sense.

Comment 34

[Jonathan Watt [:jwatt]]

(In reply to comment #31)
> Shouldn't getComputedStyle on an SVG element with "width" or "height"
> attributes actually return that width or height?

Yes. Let me get some links and explain.

Comment 35

[Jonathan Watt [:jwatt]]

So http://www.w3.org/TR/SVGMobile12/coords.html#InitialViewport essentially says outer <svg> is a CSS replaced element. It's all quite messy, but that's what bug 294086 was all about. There are comments in nsSVGOuterSVGFrame::Reflow and the other reflow related methods on that class about this.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Perhaps so, but as the test is also testing width and height of a <rect>, this fix is still needed, so we should go ahead with it.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed attachment 334768 [details], changeset d48d40cf07e5.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed main patch, changeset 3933040bac46.