Improve accuracy of computed metrics for FT fonts

In particular, with bitmap fonts, a floating-point error was affecting
y_bearing, yielding a value close to an integer instead of the integer
exactly. The change consists in replacing some operations of the form
A * (1/B) by A/B. Details of the issue in #503.

This new code will not always avoid rounding errors in final values
when these values could be exact, but it makes some of these errors
disappear.

The changes in the src/cairo-ft-font.c code consists of:
* Define the SCALE() macro (with some explanations in a comment).
* Remove the declarations and definitions of x_factor and y_factor.
* Update the code to use SCALE() instead of x_factor and y_factor.
  perl -pi -e 's{(DOUBLE_\w+) ?(\(.*\)) \* ([xy])_factor}
                {SCALE (\1 \2, unscaled->\3_scale)}' \
    src/cairo-ft-font.c
* Replace the remaining 0 * x_factor and 0 * y_factor by 0.

1 files changed, 53 insertions, 50 deletions

diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
index bf0872e93..7618e37dc 100644
--- a/src/cairo-ft-font.c
+++ b/src/cairo-ft-font.c
@@ -109,6 +109,32 @@
 #define DOUBLE_TO_16_16(d) ((FT_Fixed)((d) * 65536.0))
 #define DOUBLE_FROM_16_16(t) ((double)(t) / 65536.0)
 
+/* SCALE() mimics code from commit 399b00a99b2bbc1c56a05974c936aa69a08021f5
+ * concerning a potential division by 0, but instead of doing a * (1/b), it
+ * does a/b, thus improving the accuracy. With a * (1/b), for a bitmap font
+ * of size 13, the computed -y_bearing was 0x1.6000000000001p+3 instead of
+ * 0x1.6p+3 (= 11). This triggered a bug in GNU Emacs (when built against
+ * cairo), which rounded the value to an integer with ceil().
+ * Details:
+ *   https://gitlab.freedesktop.org/cairo/cairo/-/issues/503
+ *   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44284
+ *
+ * Note that rounding errors are not necessarily expected by applications
+ * in simple cases like the GNU Emacs one (an identity transformation,
+ * which should normally leave the inputs unchanged). However, with the
+ * current cairo code, due to the scaling, there is no guarantee that
+ * rounding errors will always be avoided at the end. For instance,
+ * (a/b)*b may be different from a, but this is still better than doing
+ * (a*(1/b))*b.
+ *
+ * According to the commit mentioned above, avoiding a division by zero
+ * was an attempt to fix
+ *   https://bugzilla.gnome.org/show_bug.cgi?id=311299
+ * but this did not actually solve the problem. So it might be possible
+ * to change SCALE() to just do (a) / (b).
+ */
+#define SCALE(a,b) ((b) == 0 ? 0.0 : (a) / (b))
+
 /* This is the max number of FT_face objects we keep open at once
  */
 #define MAX_OPEN_FACES 10
@@ -2104,27 +2130,15 @@ _cairo_ft_font_face_scaled_font_create (void		    *abstract_font_face,
      */
     if (scaled_font->base.options.hint_metrics != CAIRO_HINT_METRICS_OFF ||
 	face->units_per_EM == 0) {
-	double x_factor, y_factor;
-
-	if (unscaled->x_scale == 0)
-	    x_factor = 0;
-	else
-	    x_factor = 1 / unscaled->x_scale;
-
-	if (unscaled->y_scale == 0)
-	    y_factor = 0;
-	else
-	    y_factor = 1 / unscaled->y_scale;
-
-	fs_metrics.ascent =        DOUBLE_FROM_26_6(metrics->ascender) * y_factor;
-	fs_metrics.descent =       DOUBLE_FROM_26_6(- metrics->descender) * y_factor;
-	fs_metrics.height =        DOUBLE_FROM_26_6(metrics->height) * y_factor;
+	fs_metrics.ascent =        SCALE (DOUBLE_FROM_26_6 (metrics->ascender), unscaled->y_scale);
+	fs_metrics.descent =       SCALE (DOUBLE_FROM_26_6 (- metrics->descender), unscaled->y_scale);
+	fs_metrics.height =        SCALE (DOUBLE_FROM_26_6 (metrics->height), unscaled->y_scale);
 	if (!_cairo_ft_scaled_font_is_vertical (&scaled_font->base)) {
-	    fs_metrics.max_x_advance = DOUBLE_FROM_26_6(metrics->max_advance) * x_factor;
+	    fs_metrics.max_x_advance = SCALE (DOUBLE_FROM_26_6 (metrics->max_advance), unscaled->x_scale);
 	    fs_metrics.max_y_advance = 0;
 	} else {
 	    fs_metrics.max_x_advance = 0;
-	    fs_metrics.max_y_advance = DOUBLE_FROM_26_6(metrics->max_advance) * y_factor;
+	    fs_metrics.max_y_advance = SCALE (DOUBLE_FROM_26_6 (metrics->max_advance), unscaled->y_scale);
 	}
     } else {
 	double scale = face->units_per_EM;
@@ -3137,7 +3151,6 @@ _cairo_ft_scaled_glyph_get_metrics (cairo_ft_scaled_font_t     *scaled_font,
 				    cairo_text_extents_t *fs_metrics)
 {
     FT_Glyph_Metrics *metrics;
-    double x_factor, y_factor;
     cairo_ft_unscaled_font_t *unscaled = scaled_font->unscaled;
     cairo_bool_t hint_metrics = scaled_font->base.options.hint_metrics != CAIRO_HINT_METRICS_OFF;
     FT_GlyphSlot glyph = face->glyph;
@@ -3147,16 +3160,6 @@ _cairo_ft_scaled_glyph_get_metrics (cairo_ft_scaled_font_t     *scaled_font,
      */
     metrics = &glyph->metrics;
 
-    if (unscaled->x_scale == 0)
-	x_factor = 0;
-    else
-	x_factor = 1 / unscaled->x_scale;
-
-    if (unscaled->y_scale == 0)
-	y_factor = 0;
-    else
-	y_factor = 1 / unscaled->y_scale;
-
     /*
      * Note: Y coordinates of the horizontal bearing need to be negated.
      *
@@ -3181,13 +3184,13 @@ _cairo_ft_scaled_glyph_get_metrics (cairo_ft_scaled_font_t     *scaled_font,
 
 	    advance = ((metrics->horiAdvance + 32) & -64);
 
-	    fs_metrics->x_bearing = DOUBLE_FROM_26_6 (x1) * x_factor;
-	    fs_metrics->y_bearing = DOUBLE_FROM_26_6 (y1) * y_factor;
+	    fs_metrics->x_bearing = SCALE (DOUBLE_FROM_26_6 (x1), unscaled->x_scale);
+	    fs_metrics->y_bearing = SCALE (DOUBLE_FROM_26_6 (y1), unscaled->y_scale);
 
-	    fs_metrics->width  = DOUBLE_FROM_26_6 (x2 - x1) * x_factor;
-	    fs_metrics->height  = DOUBLE_FROM_26_6 (y2 - y1) * y_factor;
+	    fs_metrics->width  = SCALE (DOUBLE_FROM_26_6 (x2 - x1), unscaled->x_scale);
+	    fs_metrics->height  = SCALE (DOUBLE_FROM_26_6 (y2 - y1), unscaled->y_scale);
 
-	    fs_metrics->x_advance = DOUBLE_FROM_26_6 (advance) * x_factor;
+	    fs_metrics->x_advance = SCALE (DOUBLE_FROM_26_6 (advance), unscaled->x_scale);
 	    fs_metrics->y_advance = 0;
 	} else {
 	    x1 = (metrics->vertBearingX) & -64;
@@ -3197,37 +3200,37 @@ _cairo_ft_scaled_glyph_get_metrics (cairo_ft_scaled_font_t     *scaled_font,
 
 	    advance = ((metrics->vertAdvance + 32) & -64);
 
-	    fs_metrics->x_bearing = DOUBLE_FROM_26_6 (x1) * x_factor;
-	    fs_metrics->y_bearing = DOUBLE_FROM_26_6 (y1) * y_factor;
+	    fs_metrics->x_bearing = SCALE (DOUBLE_FROM_26_6 (x1), unscaled->x_scale);
+	    fs_metrics->y_bearing = SCALE (DOUBLE_FROM_26_6 (y1), unscaled->y_scale);
 
-	    fs_metrics->width  = DOUBLE_FROM_26_6 (x2 - x1) * x_factor;
-	    fs_metrics->height  = DOUBLE_FROM_26_6 (y2 - y1) * y_factor;
+	    fs_metrics->width  = SCALE (DOUBLE_FROM_26_6 (x2 - x1), unscaled->x_scale);
+	    fs_metrics->height  = SCALE (DOUBLE_FROM_26_6 (y2 - y1), unscaled->y_scale);
 
 	    fs_metrics->x_advance = 0;
-	    fs_metrics->y_advance = DOUBLE_FROM_26_6 (advance) * y_factor;
+	    fs_metrics->y_advance = SCALE (DOUBLE_FROM_26_6 (advance), unscaled->y_scale);
 	}
     } else {
-	fs_metrics->width  = DOUBLE_FROM_26_6 (metrics->width) * x_factor;
-	fs_metrics->height = DOUBLE_FROM_26_6 (metrics->height) * y_factor;
+	fs_metrics->width  = SCALE (DOUBLE_FROM_26_6 (metrics->width), unscaled->x_scale);
+	fs_metrics->height = SCALE (DOUBLE_FROM_26_6 (metrics->height), unscaled->y_scale);
 
 	if (!vertical_layout) {
-	    fs_metrics->x_bearing = DOUBLE_FROM_26_6 (metrics->horiBearingX) * x_factor;
-	    fs_metrics->y_bearing = DOUBLE_FROM_26_6 (-metrics->horiBearingY) * y_factor;
+	    fs_metrics->x_bearing = SCALE (DOUBLE_FROM_26_6 (metrics->horiBearingX), unscaled->x_scale);
+	    fs_metrics->y_bearing = SCALE (DOUBLE_FROM_26_6 (-metrics->horiBearingY), unscaled->y_scale);
 
 	    if (hint_metrics || glyph->format != FT_GLYPH_FORMAT_OUTLINE)
-		fs_metrics->x_advance = DOUBLE_FROM_26_6 (metrics->horiAdvance) * x_factor;
+		fs_metrics->x_advance = SCALE (DOUBLE_FROM_26_6 (metrics->horiAdvance), unscaled->x_scale);
 	    else
-		fs_metrics->x_advance = DOUBLE_FROM_16_16 (glyph->linearHoriAdvance) * x_factor;
-	    fs_metrics->y_advance = 0 * y_factor;
+		fs_metrics->x_advance = SCALE (DOUBLE_FROM_16_16 (glyph->linearHoriAdvance), unscaled->x_scale);
+	    fs_metrics->y_advance = 0;
 	} else {
-	    fs_metrics->x_bearing = DOUBLE_FROM_26_6 (metrics->vertBearingX) * x_factor;
-	    fs_metrics->y_bearing = DOUBLE_FROM_26_6 (metrics->vertBearingY) * y_factor;
+	    fs_metrics->x_bearing = SCALE (DOUBLE_FROM_26_6 (metrics->vertBearingX), unscaled->x_scale);
+	    fs_metrics->y_bearing = SCALE (DOUBLE_FROM_26_6 (metrics->vertBearingY), unscaled->y_scale);
 
-	    fs_metrics->x_advance = 0 * x_factor;
+	    fs_metrics->x_advance = 0;
 	    if (hint_metrics || glyph->format != FT_GLYPH_FORMAT_OUTLINE)
-		fs_metrics->y_advance = DOUBLE_FROM_26_6 (metrics->vertAdvance) * y_factor;
+		fs_metrics->y_advance = SCALE (DOUBLE_FROM_26_6 (metrics->vertAdvance), unscaled->y_scale);
 	    else
-		fs_metrics->y_advance = DOUBLE_FROM_16_16 (glyph->linearVertAdvance) * y_factor;
+		fs_metrics->y_advance = SCALE (DOUBLE_FROM_16_16 (glyph->linearVertAdvance), unscaled->y_scale);
 	}
     }
 }