839957 - glyphs in SVG text occasionally are rendered one pixel from their expected position

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: reftest log

With svg.text.css-frames.enabled true on my HiDPI Mac, running reftests results in a few tests where a glyph will one pixel from its expected position, usually horizontally and sometimes vertically.  It is consistent, however.  See the attached reftest log.

Comment 1

[Cameron McCormack (:heycam)]

Consider simple-anchor-end.svg, where these two files should render the same:

https://hg.mozilla.org/mozilla-central/file/a7ec1554d481/layout/reftests/svg/text/simple-anchor-end.svg

https://hg.mozilla.org/mozilla-central/file/a7ec1554d481/layout/reftests/svg/text/simple-anchor-end-ref.html

The text "hello" should be right-aligned at x = 100.  The reftest failure has the "h" glyph one device pixel to the left of where it is in the HTML reference.

The anchoring code works by finding the extreme left/right edges of the text, and then, for text-anchor:end;direction:ltr, looking at the difference between the rightmost glyph and the alignment point, and adding that value on to each of the glyph positions.

Before anchoring, my glyph x positions in gfxFloats (doubles) are:

  (gdb) p mPositions[0].mPosition.x
  $23 = 100
  (gdb) p mPositions[1].mPosition.x
  $24 = 108.88333333333333
  (gdb) p mPositions[2].mPosition.x
  $25 = 117.76666666666665
  (gdb) p mPositions[3].mPosition.x
  $26 = 121.31666666666665
  (gdb) p mPositions[4].mPosition.x
  $27 = 124.86666666666665

which is just the result of doing normal layout on the anonymous block frame, inspecting the app unit positions of each glyph, and converting them into user unit positions.

The two edges are:

  (gdb) p left
  $28 = 100
  (gdb) p right
  $29 = 133.75833333333333

which gives me a shift amount of:

  (gdb) p shift
  $30 = -33.758333333333326

Once I've added on the shift amount to all of the glyph positions, I get:

  (gdb) p mPositions[0].mPosition.x
  $39 = 66.241666666666674
  (gdb) p mPositions[1].mPosition.x
  $40 = 75.125
  (gdb) p mPositions[2].mPosition.x
  $41 = 84.008333333333326
  (gdb) p mPositions[3].mPosition.x
  $42 = 87.558333333333323
  (gdb) p mPositions[4].mPosition.x
  $43 = 91.10833333333332

and here you guess what the problem is: the first glyph's position will round down instead of up to a device pixel when it eventually gets rendered.

When we go to paint, we generate a single TextRenderedRun for the <text>, and the matrix that we compute to get from the current user space into a space appropriate for calling PaintText() on the nsTextFrame is:

  (gdb) p runTransform
  $47 = {
    xx = 0.5, 
    yx = 0, 
    xy = 0, 
    yy = 0.5, 
    x0 = 132.48333333333335, 
    y0 = -25
  }

The 0.5 scale is because we generated text runs at font sizes that match the final device pixel size on screen of the text (because we've got devPixelsPerPx = 2, and no other scale in the content).  The -25 is to shift the frame's position up so that the text baseline will fall at y = 0.  The 132.48333333333335 is the x translation amount in device pixels to get the left edge of the text frame at the right position.

132.48333333333335 is mPositions[0].mPosition.x * 2.  And this is where the problem is: this value rounds down to 132.

I'm wondering if the runTransform we apply before calling nsTextFrame::PaintText() should cause the frame position to be snapped to a CSS pixel, if our text is axis aligned...

Comment 2

[Cameron McCormack (:heycam)]

Current working theory is that nsTextFrame will snap the text that it draws, and this will come up in cases like the text-align:right text in simple-anchor-end-ref.html, since the width of the text is not an even multiple of device pixels.  So we should do the same snapping whenever we are right-anchoring text, so that it appears at exactly the same position.  (Similarly for text-anchor:middle needing to match up with text-align:right.)

Probably the best way to handle this is within DoAnchoring, where if we need to shift a bunch of glyphs over, we should do so such that the leftmost glyph (in each anchored chunk? or frame?) is device pixel aligned.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch

So I was a bit off track.  The problem wasn't with needing to snap the position of the text frame before painting it, rather it was that I was computing inaccurate glyph positions and advances.  nsSVGTextFrame2::DetermineCharPositions shouldn't multiply by cssPxPerDevPx and store this value as an nscoord, as that loses accuracy.  Instead we should do it after we've converted the nscoords into gfxFloats, as part of turning them into SVG user space values.

Also, the patch from bug 839956 is required, as it was causing slightly different metrics due to a different underlying font size compared to the HTML reference.

Comment 4

[Robert Longson [:longsonr]]

Comment on attachment 735688 [details] [diff] [review]
patch

>   for (uint32_t i = 1; i < mPositions.Length(); i++) {
>     // Fill in unspecified x position.
>     if (!mPositions[i].IsXSpecified()) {
>       nscoord d = charPositions[i].x - charPositions[i - 1].x;
>       mPositions[i].mPosition.x =
>         mPositions[i - 1].mPosition.x +
>-        presContext->AppUnitsToGfxUnits(d) / mFontSizeScaleFactor;
>+        presContext->AppUnitsToGfxUnits(d) * cssPxPerDevPx / mFontSizeScaleFactor;

Calculate cssPxPerDevPx / mFontSizeScaleFactor outside the loop and use that here.

>     }
>     // Fill in unspecified y position.
>     if (!mPositions[i].IsYSpecified()) {
>       nscoord d = charPositions[i].y - charPositions[i - 1].y;
>       mPositions[i].mPosition.y =
>         mPositions[i - 1].mPosition.y +
>-        presContext->AppUnitsToGfxUnits(d) / mFontSizeScaleFactor;
>+        presContext->AppUnitsToGfxUnits(d) * cssPxPerDevPx / mFontSizeScaleFactor;

Same.

r=longsonr with that.

Comment 5

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dffb1a1c7873
https://hg.mozilla.org/integration/mozilla-inbound/rev/645b63111263

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dffb1a1c7873
https://hg.mozilla.org/mozilla-central/rev/645b63111263