The default bug view has changed. See this FAQ.

SVG doesn't get scaled up on high-resolution displays

RESOLVED FIXED

Status

()

Core
SVG
P3
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Eli Friedman, Assigned: tor)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

10 years ago
To test:
1. Run Fx on high-res screen, or set layout.css.dpi to 200
2. Go to http://www.croczilla.com/svg/samples/butterfly/butterfly.svg

Expected: Butterly is scaled up.
Actual: Butterfly is too small.

I don't know the SVG code well enough to know how to fix this.

Note that this doesn't apply to all SVG; the SVG in http://www.croczilla.com/svg/samples/events1/events1.xml gets scaled up correctly.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 375119
(Assignee)

Comment 2

10 years ago
Created attachment 260366 [details] [diff] [review]
adjust for css/device pixel difference

Maybe not the cleanest approach, but preserves the common case unitless number fastpath.

Comment 3

10 years ago
Comment on attachment 260366 [details] [diff] [review]
adjust for css/device pixel difference

>+          mCoordCtxMmPerPx *=
>+            float(nsIDeviceContext::AppUnitsPerCSSPixel()) /
>+            float(context->AppUnitsPerDevPixel());

/= presContext->AppUnitsToFloatCSSPixels(PresContext()->AppUnitsPerDevPixel()) perhaps?

>+    vb2vp->Scale(float(nsIDeviceContext::AppUnitsPerCSSPixel()) /
>+                 float(PresContext()->AppUnitsPerDevPixel()),
>+                 getter_AddRefs(mCanvasTM));

1 / presContext->AppUnitsToFloatCSSPixels(PresContext()->AppUnitsPerDevPixel())?

or some other prescontext function maybe. There are a number to choose from.
(Assignee)

Comment 4

10 years ago
Created attachment 260834 [details] [diff] [review]
adjust per suggestion
Attachment #260366 - Attachment is obsolete: true
Attachment #260834 - Flags: review?(elifree)
(Assignee)

Updated

10 years ago
Attachment #260834 - Flags: review?(elifree) → review?(sharparrow1)
(Reporter)

Comment 5

10 years ago
It seems like it would be much more straightforward to use something like:
25.4f * float(nsPresContext::AppUnitsPerCSSPixel()) / float(presContext->AppUnitsPerInch());
instead of calling GetScreenPixelToMillimeterX and correcting the result.

I actually preferred the way you wrote the scale factor in the original version: (float(nsIDeviceContext::AppUnitsPerCSSPixel()) /
float(PresContext()->AppUnitsPerDevPixel()));
the corrected version requires an extra division for no good reason.  I'll leave that up to you, though; it's not very important.

I'm assuming that the results of GetCanvasTM aren't visible to script; that seems to be true, but I'm not completely sure.  We don't want to be changing the way any script calculations happen that don't explicitly request the size a screen pixel.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 379369
Changing mCoordCtxMmPerPx to device pixels instead of CSS pixels will break things like nsSVGLength2::ConvertToUserUnits which expect GetMMPerPixel to be returning a mm per _CSS_ pixel (user unit) value. We'd then have the potential for scripts to act very strangly on high resolution screens.
(Reporter)

Comment 8

10 years ago
(In reply to comment #7)
> Changing mCoordCtxMmPerPx to device pixels instead of CSS pixels will break
> things like nsSVGLength2::ConvertToUserUnits which expect GetMMPerPixel to be
> returning a mm per _CSS_ pixel (user unit) value. We'd then have the potential
> for scripts to act very strangly on high resolution screens.

No, this patch puts mCoordCtxMmPerPx into CSS pixels, although in a very indirect way.  Although, like I said earlier, "It seems like it would be much more straightforward to use something like:
25.4f * float(nsPresContext::AppUnitsPerCSSPixel()) /
float(presContext->AppUnitsPerInch());
instead of calling GetScreenPixelToMillimeterX and correcting the result."

The bigger issue here is whether the GetCanvasTM change is correct.  (See 
bug 379616 comment 13 for background.)  I'm not sure if it's the best approach, but it seems like it'll work (I suspect it breaks foreignobject, though.)
So let me think this through out loud.

  context->AppUnitsToFloatCSSPixels(context->AppUnitsPerDevPixel())

That evaluates to the number of CSS pixels per device pixel. It seems to me that if you _divide_ by that number, as the patch does, then you're converting from CSS pixels to device pixels. GetScreenPixelToMillimeterX returns the number of mm per device pixel, and if you multiply that by device pixels per CSS pixel, you get, uh, hmm, err, wow, mm per CSS pixel! Hmph. So that means our code is currently wrong and the patch is right. I have to say though, I've seen conversions that were easier to follow. :-P

You know what, I think the correct way to fix this is to change GetScreenPixelToMillimeterX. The space says:

  readonly float screenPixelToMillimeterX
    User interface (UI) events in DOM Level 2 indicate the screen positions
    at which the given UI event occurred. When the user agent actually knows
    the physical size of a "screen unit", this attribute will express that
    information; otherwise, user agents will provide a suitable default
    value such as .28mm.

DOM event coordinates are in CSS pixels, not device pixels, right? It seems clear then that we're doing the wrong thing by returning a mm per device pixel value here ("screen" unit seems to be a misleading term). We should be returning a mm per CSS pixel value for this to be of any use to anyone using the DOM. (Of course CSS and device pixels are typically the same, so nobody has noticed this bug yet.) Guess what, if we do that then GetScreenPixelToMillimeterX will give us the mm per CSS pixel value we needed all along. Thoughts?
I agree.
(In reply to comment #8)
> The bigger issue here is whether the GetCanvasTM change is correct.  (See 
> bug 379616 comment 13 for background.)  I'm not sure if it's the best approach,
> but it seems like it'll work (I suspect it breaks foreignobject, though.)

Yeah, if we were to apply the CSS pixel to device pixel conversion for gfxContext drawing by using the GetCanvasTM change in the patch then we'd need to reverse this scale when passing through foreignObject boundaries. I'm also not yet clear whether that's how we should do it though.
(Reporter)

Updated

10 years ago
Blocks: 389769
(Reporter)

Comment 12

10 years ago
IMO, we definitely want this for 1.9; nominating to get this on the radar.
Flags: blocking1.9?
(Reporter)

Updated

10 years ago
Depends on: 390161
(Reporter)

Comment 13

10 years ago
Comment on attachment 260834 [details] [diff] [review]
adjust per suggestion

With the fix to bug 390161, the first part of this patch is not needed.  The second part is very SVG-specific, so I'm not really sure if it's the right thing to do; redirecting the review request so that an SVG peer can decide whether the method of scaling used is appropriate.
Attachment #260834 - Flags: review?(sharparrow1) → review?(jwatt)

Comment 14

10 years ago
Applying the second part of the patch fixes print preview on Windows although actual printing of SVG still doesn't work for me.
(Assignee)

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 15

10 years ago
Created attachment 281330 [details] [diff] [review]
second chunk only, updated to tip
Attachment #260834 - Attachment is obsolete: true
Attachment #281330 - Flags: review?(jwatt)
Attachment #260834 - Flags: review?(jwatt)
I'm going to try to look at this properly tomorrow. Sorry I still haven't got to it.
(Assignee)

Updated

10 years ago
Assignee: general → tor
(Assignee)

Updated

10 years ago
OS: Windows XP → All
Hardware: PC → All
Note that contrary to comment 0, http://www.croczilla.com/svg/samples/events1/events1.xml does not get scaled up correctly. That file consists of SVG embedded inline in XHTML, and while the text in the XHTML gets scaled, the embedded SVG circles do not.
Looks like you need to trace through the call paths and fix things as necessary. For example, nsSVGForeignObjectFrame::TransformPointFromOuterPx and nsSVGForeignObjectFrame::TransformPointFromOuter look like they would need to be changed to use nsPresContext::DevPixelsToAppUnits and nsPresContext::AppUnitsToDevPixels respectively (or whatever the names are).

Also note that this patch in its current form would break getClientRects and getBoundingClientRect on high resolution devices since nsNSElementTearoff::TryGetSVGBoundingRect calls nsSVGUtils::GetOuterSVGFrameAndCoveredRegion. If GetCanvasTM is changed to include a device pixel to CSS pixel transform then nsSVGPathGeometryFrame::UpdateCoveredRegion (which calls nsSVGPathGeometryFrame::GeneratePath) and nsSVGForeignObjectFrame::UpdateCoveredRegion will both return device pixel rects instead of CSS pixel rects.
(Assignee)

Comment 19

10 years ago
I think we've always assumed that CoveredRegion was device pixels - see nsSVGOuterSVGFrame::InvalidateRect.
(Assignee)

Comment 20

10 years ago
Created attachment 286200 [details] [diff] [review]
address review comments
Attachment #281330 - Attachment is obsolete: true
Attachment #281330 - Flags: review?(jwatt)

Comment 21

10 years ago
WSere you going to ask for a review for this?
(Assignee)

Updated

10 years ago
Attachment #286200 - Flags: review?(jwatt)
(Assignee)

Updated

10 years ago
Priority: -- → P3
Comment on attachment 286200 [details] [diff] [review]
address review comments

The AppUnitsPerCSSPixel in nsSVGForeignObjectFrame::FlushDirtyRegion needs to be changed to device pixels.

With this patch nsSVGPathGeometryFrame::GetBBox would be wrong since GeneratePath would then create a path in device pixels instead of CSS px. Similarly nsSVGGlyphFrame::GetBBox would be wrong since nsSVGGlyphFrame::GetGlobalTransform would then contain the CSS px to device pixel scaling. I'm not sure quite how to fix that. If you can't come up with a fix, then I think it's still worth taking this patch. At least then most SVG will be rendered correctly, and only SVG using getBBox in script will be slightly broken.

>+    svgElement->GetViewboxToViewportTransform(getter_AddRefs(vb2vp));
>+
>+    vb2vp->Scale(1 / PresContext()->AppUnitsToFloatCSSPixels(
>+                                        PresContext()->AppUnitsPerDevPixel()),
>+                 getter_AddRefs(mCanvasTM));

To make this easier to follow can you either split the unit conversion stuff out into a variable called devPxPerCSSPx, or else add a comment along the lines of "Scale the transform from CSS pixel space to device pixel space"?
Comment on attachment 286200 [details] [diff] [review]
address review comments

It's not entirely clear to me whether nsSVGPatternFrame::PaintPattern will do the right thing if nsSVGPatternFrame::GetCallerGeometry changes to output device pixels. If you can figure that out, please add a comment above the declaration of GetCallerGeometry saying what units it's outparams are in, and perhaps rename the variables where it's called so they don't look like they're in CSS px units. ("callerBBox" and "callerCTM" both say CSS px units to me.)
(In reply to comment #19)
> I think we've always assumed that CoveredRegion was device pixels

I've always assumed that everything was in CSS pixels, and so has a fair bit of the code. Since CSS pixels and device pixels have been equivalent on all the machines that I've developed on, I haven't noticed any problems. Clearly we need to do a far better job of documenting the units of function arguments and return values. In that vein...

Please add this comment above GetCanvasTM in nsSVGContainerFrame.h:

  // Returns the transform to our gfxContext (i.e. to device pixels, not CSS px)

Please add this comment above GetCoveredRegion in nsISVGChildFrame.h:

  // Get bounds in our gfxContext's coordinate space (i.e. in device pixels)

Please add a comment above GetFrameForPointSVG saying what units x and y are in, and what exactly the point is relative to.

Please add a comment above nsSVGUtils::HitTestClip saying what units x and y are in.

Please add a comment above GetInvalidationRegion saying what unit the returned nsRect is in.

For anyone who isn't familiar with the SVG code these comments could be very helpful.

Comment 25

10 years ago
I'm commenting here because this bug seems relevant and is 'live'


zoom is currently working for many examples
BUT the polarity is wrong ie the opposite of expectations
http://peepo.com/svg/games.svg
http://www.peepo.co.uk/index.svg

on my system os x ppc nightlies 1440/900

however  #1 re http://www.croczilla.com/svg/samples/events1/events1.xml
the text scales, but not the SVG which is contrary to the comment.

fwiw, I came to this from the 'fixed' XUL bug 275254 where again the text scales but not the svg in my dupe attachment.

let me know if this should be in a separate bug, nb there are already a few  svg zoom bugs

Updated

10 years ago
Blocks: 366681
(Assignee)

Comment 26

10 years ago
Created attachment 290612 [details] [diff] [review]
more functional patch

Doesn't address all review comments yet, but getting closer to desired behavior.
Attachment #286200 - Attachment is obsolete: true
Attachment #286200 - Flags: review?(jwatt)
(Assignee)

Comment 27

10 years ago
Created attachment 290613 [details] [diff] [review]
leave old textZoom behavior, in case it get revived
Attachment #290612 - Attachment is obsolete: true
(Assignee)

Comment 28

10 years ago
(In reply to comment #22)
> (From update of attachment 286200 [details] [diff] [review])
> With this patch nsSVGPathGeometryFrame::GetBBox would be wrong since
> GeneratePath would then create a path in device pixels instead of CSS px.
> Similarly nsSVGGlyphFrame::GetBBox would be wrong since

GetBBox actually works in most cases because we turn off matrix propagation before calling it.  The couple cases where we don't for object space coordinates need further checking.

Comment 29

10 years ago
Comment on attachment 290613 [details] [diff] [review]
leave old textZoom behavior, in case it get revived

>Index: layout/svg/base/src/nsSVGGlyphFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp,v
>retrieving revision 1.115
>diff -u -8 -p -r1.115 nsSVGGlyphFrame.cpp
>--- layout/svg/base/src/nsSVGGlyphFrame.cpp	21 Oct 2007 09:05:41 -0000	1.115
>+++ layout/svg/base/src/nsSVGGlyphFrame.cpp	29 Nov 2007 00:04:59 -0000
>@@ -117,17 +117,18 @@ nsSVGGlyphFrame::DidSetStyleContext()
>   const nsStyleFont* fontData = GetStyleFont();
>   nsFont font = fontData->mFont;
> 
>   // Since SVG has its own scaling, we really don't want
>   // fonts in SVG to respond to the browser's "TextZoom"
>   // (Ctrl++,Ctrl+-)
>   nsPresContext *presContext = PresContext();
>   float textZoom = presContext->TextZoom();
>-  double size = presContext->AppUnitsToDevPixels(fontData->mSize) / textZoom;
>+  double size =
>+    presContext->AppUnitsToFloatCSSPixels(fontData->mSize) / textZoom;

Doesn't SeaMonkey still have text zoom? Will this change be OK there?

>Index: layout/svg/base/src/nsSVGUtils.h

>   /* Hit testing - check if point hits the clipPath of indicated
>-   * frame.  Returns true of no clipPath set. */
>-
>+   * frame.  (x,y) are specified in device pixels relateive to the

s/relateive/relative/

>+   * origin of the outer svg frame.  Returns true of no clipPath

s/of no/if no/

>+   * set. */

Comment 30

10 years ago
> Doesn't SeaMonkey still have text zoom? Will this change be OK there?

Scratch that first comment I clearly can't read.
(Assignee)

Comment 31

10 years ago
Created attachment 290905 [details] [diff] [review]
fix clip/mask object bounding space, other review comments
Attachment #290613 - Attachment is obsolete: true
Attachment #290905 - Flags: review?(jwatt)

Updated

9 years ago
Attachment #290905 - Flags: review?(jwatt) → review+
(Assignee)

Updated

9 years ago
Attachment #290905 - Flags: superreview?(roc)
+  *aOut = nsPoint(PRInt32(aX * PresContext()->AppUnitsPerDevPixel()),
+                  PRInt32(aY * PresContext()->AppUnitsPerDevPixel()));

Use NSToCoordRound here instead of the PRInt32 cast.

It's worth loading AppUnitsPerDevPixel into a local variable because PresContext() isn't quite trivial.

+  TransformPointFromOuterPx(aPt.x / PresContext()->AppUnitsPerDevPixel(),
+                            aPt.y / PresContext()->AppUnitsPerDevPixel(),

Ditto.

Looks like code in nsSVGClipPathFrame::GetCanvasTM and nsSVGMaskFrame::GetCanvasTM should be shared?
(Assignee)

Comment 33

9 years ago
Created attachment 291362 [details] [diff] [review]
adjust per comments
Attachment #290905 - Attachment is obsolete: true
Attachment #291362 - Flags: superreview?(roc)
Attachment #290905 - Flags: superreview?(roc)
Attachment #291362 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 34

9 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.