[OpenJDK 2D-Dev] [9] Review request for 8158370 Text drawn with floating pointing scale can be shifted to one pixel

Philip Race philip.race at oracle.com
Thu Jul 7 23:10:21 UTC 2016


This is about how to "snap" the drawing origin of text to a device pixel
when the origin has been supplied as a floating point value but behind
it we have some accumulated FP error.

The scaling is one way to demonstrate this but it may also be demonstrable
with an identity transform and the overload of drawString that accepts

consider :
(Graphics2D(g)).drawString("a", 0.5f, 10f);
(Graphics2D(g)).drawString("a", 0.49999f, 10f);

I think the thing in your test that triggers it is that the FP error in
the string bounds (99.99999....) that I see - at least for the font
I am using on Linux. Mileage may vary depending on font.
This is arguably where the problem originates and your
proposed change is an (almost) final stage compensation for that.

For logic that decides precisely where to place the glyph see the
setup[LCD]BlitVector function and then the FLOOR_ASSIGN macro in
which is replicated in OGL/D3DTextRenderer.c/cpp
...  the Java loops should be doing something equivalent.

Basically we round where we start to the nearest device pixel
and so being at 0.49999 vs 0.5000 is just enough to tickle a one device
pixel difference.

Also I note that you changed only *one* out of several code paths here.
It isn't applied to drawChars and drawGlyphVector which each have the
same two branches.
Also since I believe this in principle applies to translate only too,
the "else" branches need the same treatment, although that exposes
the "base" case to this expensive calculation.
I am not asking you to go change those .. just noting it ..

Leaving aside the missing parts I am not sure about
1) whether the calculations can be made less costly,
2) whether we should try to fix this "upstream" - ie when
we report bounds can we notice we are about to report N.9999 or N.0001, or
3) further downstream, in the final rendering code
4) not attempt to fix it at all and pass this on to the application.

I think either of (2) or (3) might require changes in more than one code path.

(3) would at least be where we are doing the "+0.5" and I think it better
to keep the logic together.

(3) would look like this

diff --git a/src/java.desktop/share/native/libfontmanager/DrawGlyphList.c b/src/java.desktop/share/native/libfontmanager/DrawGlyphList.c
--- a/src/java.desktop/share/native/libfontmanager/DrawGlyphList.c
+++ b/src/java.desktop/share/native/libfontmanager/DrawGlyphList.c
@@ -87,7 +87,7 @@
      /* Add 0.5 to x and y and then use floor (or an equivalent operation)
       * to round down the glyph positions to integral pixel positions.
-    x += 0.5f;
+    x += 0.5001f;
      y += 0.5f;
      if (glyphPositions) {
          int n = -1;
@@ -541,7 +541,7 @@
          x += 0.1666667f;
          y += 0.1666667f;
      } else {
-        x += 0.5f;
+        x += 0.5001f;
          y += 0.5f;

.. maybe y needs the same ?

I am not sure yet that OGL/D3D/Java loops are all consistent here in having
that rounding but if they aren't that seems to be a bug and I'd have to wonder if
you are going to see the behaviour you are fixing there anyway ?
Have you tested to see if this is consistent (as a problem) if going through software
as well as D3DTR_DrawGlyphList(..) and OGLTR_DrawGlyphList(..) ?
If you weren't seeing the problem there it may be because they aren't
doing the rounding they should be. This needs an audit before making
any changes.

Maybe the test can try to cover these scenarios ?

Regarding the test, it can report "fail" even when it actually passes.
I've seen this because you are not accounting for the significant
of the LCD blending touching the pixels between the glyphs.
I recommend you use DEFAULT instead.

Also a test that is automated *and* draws to the screen (even if
only optionally) would make it easier for a human to evaluate.

BTW the test uses the FP version of drawString when it has floats .. it
would be important for application code to do the same rather than
cast to int. I mention this just "for the record", not because I see an
improper use anywhere.


On 6/23/16, 1:04 PM, Alexandr Scherbatiy wrote:
> Hello,
> Could you review the fix:
>   bug: https://bugs.openjdk.java.net/browse/JDK-8158370
>   webrev: http://cr.openjdk.java.net/~alexsch/8158370/webrev.00
>   Char advance in user space is calculated with float precision. That 
> leads that summed up char advances in user space can have small 
> rounding error.
>   To draw a substring of a text as selected it needs to draw the 
> selected text from the string location plus the preceding text width.
>   The selected text location is close to an integer value for an 
> integer scale:
>     (textX + sum(charAdvanceInDevSpave/scale) + transformTranslate) * 
> scale //  both textX and transformTranslate have integer value
>   The situation is different for floating point scales. Now (textX + 
> transformTranslate) * scale can be a floating point value like N.5 
> where N is some integer value. The sum(charAdvanceInDevSpave/scale) * 
> scale is close to an integer.
>   Sum of these values can be M.499.. which is rounded to M or M.500.. 
> which is rounded to M+1.
>   Because of this a selected text can be shifted to one pixel left 
> from the required position.
>   The proposed fix rounds the text coordinates to the closest half 
> integer value if it is necessary in dev space.
>  Thanks,
>  Alexandr.

More information about the 2d-dev mailing list