bug#44655: 28.0.50; Oversized SVG margin

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

On 11/6/21 05:55, Alan Third wrote:

> There seems to be a lot of churn in the librsvg API at the moment, and
> their documentation isn't keeping up as it still doesn't mark
> rsvg_handle_get_dimensions as deprecated.
> 
> It appears they've introduced rsvg_handle_get_intrinsic_size_in_pixels
> which does the same thing (but better) than my sizing code, but anyone
> using librsvg 2.46-2.52, which is probably most people for now, can't
> use it and probably don't want to use the old scheme, so I expect
> there's no point replacing my code at the moment.

If it improves on your code then let's try using it for bleeding-edge 
librsvg (2.52.0+).


> Perhaps what we should do is move the final "else" section of the
> previous code block (where rsvg_handle_get_geometry_for_layer is
> called) into its own block which is executed
> 
>    if (viewbox_width == 0 || viewbox_height == 0)
> 
> instead of only in the case where rsvg_handle_get_intrinsic_dimensions
> fails to return any dimensions. That way we should have *some*
> dimensions without having to call rsvg_handle_get_dimensions.

I installed a patch into master to do that, along with other patches to 
try using svg_handle_get_intrinsic_size_in_pixels with bleeding-edge 
librsvg, and to catch some potential integer overflow problems I noticed 
while doing all this (see attached).

[-- Attachment #2: 0001-More-robust-svg_load_image-fallback.patch --]
[-- Type: text/x-patch, Size: 1092 bytes --]

From 9a4a14d1e838e85da2a480340e0a9c8109faee45 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 3 Dec 2021 09:47:22 -0800
Subject: [PATCH 1/4] More-robust svg_load_image fallback

Suggested by Alan Third (Bug#44655#56).
* src/image.c (svg_load_image): Fall back on
rsvg_handle_get_geometry_for_layer if the
rsvg_handle_get_intrinsic_dimensions computations yielded unusable
viewbox width and height, instead of falling back only if
rsvg_handle_get_intrinsic_dimensions did not report image width
and height, or did not report a viewbox.
---
 src/image.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/image.c b/src/image.c
index f2597f529d..f13304912c 100644
--- a/src/image.c
+++ b/src/image.c
@@ -10484,6 +10484,9 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
       viewbox_height = viewbox.height;
     }
   else
+    viewbox_width = viewbox_height = 0;
+
+  if (viewbox_width == 0 || viewbox_height == 0)
     {
       /* We haven't found a usable set of sizes, so try working out
          the visible area.  */
-- 
2.32.0


[-- Attachment #3: 0002-Simplify-svg_load_image.patch --]
[-- Type: text/x-patch, Size: 1244 bytes --]

From 24303f19f3fdd9971014b26b2b8ee7043a055fdc Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 3 Dec 2021 09:47:22 -0800
Subject: [PATCH 2/4] Simplify svg_load_image

* src/image.c (svg_load_image): Simplify slightly.
---
 src/image.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/image.c b/src/image.c
index f13304912c..1db2b78ad5 100644
--- a/src/image.c
+++ b/src/image.c
@@ -10469,14 +10469,12 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   else if (has_width && has_viewbox)
     {
       viewbox_width = svg_css_length_to_pixels (iwidth, dpi, img->face_font_size);
-      viewbox_height = svg_css_length_to_pixels (iwidth, dpi, img->face_font_size)
-        * viewbox.height / viewbox.width;
+      viewbox_height = viewbox_width * viewbox.height / viewbox.width;
     }
   else if (has_height && has_viewbox)
     {
       viewbox_height = svg_css_length_to_pixels (iheight, dpi, img->face_font_size);
-      viewbox_width = svg_css_length_to_pixels (iheight, dpi, img->face_font_size)
-        * viewbox.width / viewbox.height;
+      viewbox_width = viewbox_height * viewbox.width / viewbox.height;
     }
   else if (has_viewbox)
     {
-- 
2.32.0


[-- Attachment #4: 0003-Improve-overflow-checking-in-svg_load_image.patch --]
[-- Type: text/x-patch, Size: 6774 bytes --]

From 57eaf033d71d755009ffc1e9b552201143129218 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 3 Dec 2021 09:47:22 -0800
Subject: [PATCH 3/4] Improve overflow checking in svg_load_image
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/image.c: Include math.h, for lrint.
(scale_image_size, compute_image_size): Use ‘double’, not ‘int’
for image size args, since librsvg uses ‘double’ for pixel counts.
(scale_image_size): Use ceil instead of rounding, to avoid
discarding fractional SVG pixels.  Divisor and multiplier are now
double instead of int, for better portability to librsvg
functions with fractional pixel sizes.
(image_get_dimension, compute_image_size, svg_load_image):
Be more careful about ignoring, rejecting or clipping scale
factors or sizes that are out of integer range.
(compute_image_size): Don’t bother to calculate :max-width if
:width is specified, and likewise for :max-height and :height.
---
 src/image.c | 76 +++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 40 deletions(-)

diff --git a/src/image.c b/src/image.c
index 1db2b78ad5..14e9944db2 100644
--- a/src/image.c
+++ b/src/image.c
@@ -31,6 +31,7 @@ Copyright (C) 1989, 1992-2021 Free Software Foundation, Inc.
 
 #include <setjmp.h>
 
+#include <math.h>
 #include <stdint.h>
 #include <c-ctype.h>
 #include <flexmember.h>
@@ -2016,14 +2017,16 @@ postprocess_image (struct frame *f, struct image *img)
    safely rounded and clipped to int range.  */
 
 static int
-scale_image_size (int size, size_t divisor, size_t multiplier)
+scale_image_size (int size, double divisor, double multiplier)
 {
   if (divisor != 0)
     {
-      double s = size;
-      double scaled = s * multiplier / divisor + 0.5;
+      double scaled = size * multiplier / divisor;
       if (scaled < INT_MAX)
-	return scaled;
+	{
+	  /* Use ceil, as rounding can discard fractional SVG pixels.  */
+	  return ceil (scaled);
+	}
     }
   return INT_MAX;
 }
@@ -2044,84 +2047,77 @@ image_get_dimension (struct image *img, Lisp_Object symbol)
   if (FIXNATP (value))
     return min (XFIXNAT (value), INT_MAX);
   if (CONSP (value) && NUMBERP (CAR (value)) && EQ (Qem, CDR (value)))
-    return min (img->face_font_size * XFLOATINT (CAR (value)), INT_MAX);
+    return scale_image_size (img->face_font_size, 1, XFLOATINT (CAR (value)));
 
   return -1;
 }
 
 /* Compute the desired size of an image with native size WIDTH x HEIGHT.
-   Use SPEC to deduce the size.  Store the desired size into
+   Use IMG to deduce the size.  Store the desired size into
    *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
 static void
-compute_image_size (size_t width, size_t height,
+compute_image_size (double width, double height,
 		    struct image *img,
 		    int *d_width, int *d_height)
 {
-  Lisp_Object value;
-  int int_value;
-  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
   double scale = 1;
-
-  value = image_spec_value (img->spec, QCscale, NULL);
+  Lisp_Object value = image_spec_value (img->spec, QCscale, NULL);
   if (NUMBERP (value))
-    scale = XFLOATINT (value);
-
-  int_value = image_get_dimension (img, QCmax_width);
-  if (int_value >= 0)
-    max_width = int_value;
-
-  int_value = image_get_dimension (img, QCmax_height);
-  if (int_value >= 0)
-    max_height = int_value;
+    {
+      double dval = XFLOATINT (value);
+      if (0 <= dval)
+	scale = dval;
+    }
 
   /* If width and/or height is set in the display spec assume we want
      to scale to those values.  If either h or w is unspecified, the
      unspecified should be calculated from the specified to preserve
      aspect ratio.  */
-  int_value = image_get_dimension (img, QCwidth);
-  if (int_value >= 0)
+  int desired_width = image_get_dimension (img, QCwidth), max_width;
+  if (desired_width < 0)
+    max_width = image_get_dimension (img, QCmax_width);
+  else
     {
-      desired_width = int_value * scale;
+      desired_width = scale_image_size (desired_width, 1, scale);
       /* :width overrides :max-width. */
       max_width = -1;
     }
 
-  int_value = image_get_dimension (img, QCheight);
-  if (int_value >= 0)
+  int desired_height = image_get_dimension (img, QCheight), max_height;
+  if (desired_height < 0)
+    max_height = image_get_dimension (img, QCmax_height);
+  else
     {
-      desired_height = int_value * scale;
+      desired_height = scale_image_size (desired_height, 1, scale);
       /* :height overrides :max-height. */
       max_height = -1;
     }
 
   /* If we have both width/height set explicitly, we skip past all the
      aspect ratio-preserving computations below. */
-  if (desired_width != -1 && desired_height != -1)
+  if (0 <= desired_width && 0 <= desired_height)
     goto out;
 
-  width = width * scale;
-  height = height * scale;
-
-  if (desired_width != -1)
+  if (0 <= desired_width)
     /* Width known, calculate height. */
     desired_height = scale_image_size (desired_width, width, height);
-  else if (desired_height != -1)
+  else if (0 <= desired_height)
     /* Height known, calculate width. */
     desired_width = scale_image_size (desired_height, height, width);
   else
     {
-      desired_width = width;
-      desired_height = height;
+      desired_width = scale_image_size (width, 1, scale);
+      desired_height = scale_image_size (height, 1, scale);
     }
 
-  if (max_width != -1 && desired_width > max_width)
+  if (0 <= max_width && max_width < desired_width)
     {
       /* The image is wider than :max-width. */
       desired_width = max_width;
       desired_height = scale_image_size (desired_width, width, height);
     }
 
-  if (max_height != -1 && desired_height > max_height)
+  if (0 <= max_height && max_height < desired_height)
     {
       /* The image is higher than :max-height. */
       desired_height = max_height;
@@ -10484,7 +10480,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   else
     viewbox_width = viewbox_height = 0;
 
-  if (viewbox_width == 0 || viewbox_height == 0)
+  if (! (0 < viewbox_width && 0 < viewbox_height))
     {
       /* We haven't found a usable set of sizes, so try working out
          the visible area.  */
@@ -10505,8 +10501,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   compute_image_size (viewbox_width, viewbox_height, img,
                       &width, &height);
 
-  width *= FRAME_SCALE_FACTOR (f);
-  height *= FRAME_SCALE_FACTOR (f);
+  width = scale_image_size (width, 1, FRAME_SCALE_FACTOR (f));
+  height = scale_image_size (height, 1, FRAME_SCALE_FACTOR (f));
 
   if (! check_image_size (f, width, height))
     {
-- 
2.32.0


[-- Attachment #5: 0004-Prefer-rsvg_handle_get_intrinsic_size_in_pixels.patch --]
[-- Type: text/x-patch, Size: 6597 bytes --]

From 89d494e7a26e6031f1ae35e8590793e413b22641 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 3 Dec 2021 09:47:22 -0800
Subject: [PATCH 4/4] Prefer rsvg_handle_get_intrinsic_size_in_pixels

Use rsvg_handle_get_intrinsic_size_in_pixels if available,
as this is simpler and better than what we were doing.
From a comment by by Alan Third (Bug#44655#56).
* src/image.c (init_svg_functions): Arrange for the new function.
(svg_load_image): Prefer the results of
rsvg_handle_get_intrinsic_size_in_pixels if available.
---
 src/image.c | 114 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 74 insertions(+), 40 deletions(-)

diff --git a/src/image.c b/src/image.c
index 14e9944db2..c08ee92287 100644
--- a/src/image.c
+++ b/src/image.c
@@ -10066,6 +10066,10 @@ DEF_DLL_FN (gboolean, rsvg_handle_close, (RsvgHandle *, GError **));
 DEF_DLL_FN (void, rsvg_handle_set_dpi_x_y,
 	    (RsvgHandle * handle, double dpi_x, double dpi_y));
 
+#  if LIBRSVG_CHECK_VERSION (2, 52, 1)
+DEF_DLL_FN (void, rsvg_handle_get_intrinsic_size_in_pixels,
+            (RsvgHandle *, gdouble *, gdouble *));
+#  endif
 #  if LIBRSVG_CHECK_VERSION (2, 46, 0)
 DEF_DLL_FN (void, rsvg_handle_get_intrinsic_dimensions,
             (RsvgHandle *, gboolean *, RsvgLength *, gboolean *,
@@ -10129,6 +10133,9 @@ init_svg_functions (void)
   LOAD_DLL_FN (library, rsvg_handle_close);
 #endif
   LOAD_DLL_FN (library, rsvg_handle_set_dpi_x_y);
+#if LIBRSVG_CHECK_VERSION (2, 52, 1)
+  LOAD_DLL_FN (library, rsvg_handle_get_intrinsic_size_in_pixels);
+#endif
 #if LIBRSVG_CHECK_VERSION (2, 46, 0)
   LOAD_DLL_FN (library, rsvg_handle_get_intrinsic_dimensions);
   LOAD_DLL_FN (library, rsvg_handle_get_geometry_for_layer);
@@ -10172,6 +10179,9 @@ init_svg_functions (void)
 #  undef g_clear_error
 #  undef g_object_unref
 #  undef g_type_init
+#  if LIBRSVG_CHECK_VERSION (2, 52, 1)
+#   undef rsvg_handle_get_intrinsic_size_in_pixels
+#  endif
 #  if LIBRSVG_CHECK_VERSION (2, 46, 0)
 #   undef rsvg_handle_get_intrinsic_dimensions
 #   undef rsvg_handle_get_geometry_for_layer
@@ -10207,6 +10217,10 @@ init_svg_functions (void)
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
 #   define g_type_init fn_g_type_init
 #  endif
+#  if LIBRSVG_CHECK_VERSION (2, 52, 1)
+#   define rsvg_handle_get_intrinsic_size_in_pixels \
+	fn_rsvg_handle_get_intrinsic_size_in_pixels
+#  endif
 #  if LIBRSVG_CHECK_VERSION (2, 46, 0)
 #   define rsvg_handle_get_intrinsic_dimensions \
 	fn_rsvg_handle_get_intrinsic_dimensions
@@ -10444,51 +10458,71 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 
   /* Get the image dimensions.  */
 #if LIBRSVG_CHECK_VERSION (2, 46, 0)
-  RsvgRectangle zero_rect, viewbox, out_logical_rect;
-
-  /* Try the intrinsic dimensions first.  */
-  gboolean has_width, has_height, has_viewbox;
-  RsvgLength iwidth, iheight;
-  double dpi = FRAME_DISPLAY_INFO (f)->resx;
-
-  rsvg_handle_get_intrinsic_dimensions (rsvg_handle,
-                                        &has_width, &iwidth,
-                                        &has_height, &iheight,
-                                        &has_viewbox, &viewbox);
+  gdouble gviewbox_width, gviewbox_height;
+  gboolean has_viewbox = FALSE;
+# if LIBRSVG_CHECK_VERSION (2, 52, 1)
+  has_viewbox = rsvg_handle_get_intrinsic_size_in_pixels (rsvg_handle,
+							  &gviewbox_width,
+							  &gviewbox_height);
+# endif
 
-  if (has_width && has_height)
+  if (has_viewbox)
     {
-      /* Success!  We can use these values directly.  */
-      viewbox_width = svg_css_length_to_pixels (iwidth, dpi, img->face_font_size);
-      viewbox_height = svg_css_length_to_pixels (iheight, dpi, img->face_font_size);
-    }
-  else if (has_width && has_viewbox)
-    {
-      viewbox_width = svg_css_length_to_pixels (iwidth, dpi, img->face_font_size);
-      viewbox_height = viewbox_width * viewbox.height / viewbox.width;
-    }
-  else if (has_height && has_viewbox)
-    {
-      viewbox_height = svg_css_length_to_pixels (iheight, dpi, img->face_font_size);
-      viewbox_width = viewbox_height * viewbox.width / viewbox.height;
-    }
-  else if (has_viewbox)
-    {
-      viewbox_width = viewbox.width;
-      viewbox_height = viewbox.height;
+      viewbox_width = gviewbox_width;
+      viewbox_height = gviewbox_height;
     }
   else
-    viewbox_width = viewbox_height = 0;
-
-  if (! (0 < viewbox_width && 0 < viewbox_height))
     {
-      /* We haven't found a usable set of sizes, so try working out
-         the visible area.  */
-      rsvg_handle_get_geometry_for_layer (rsvg_handle, NULL,
-                                          &zero_rect, &viewbox,
-                                          &out_logical_rect, NULL);
-      viewbox_width = viewbox.x + viewbox.width;
-      viewbox_height = viewbox.y + viewbox.height;
+      RsvgRectangle zero_rect, viewbox, out_logical_rect;
+
+      /* Try the intrinsic dimensions first.  */
+      gboolean has_width, has_height;
+      RsvgLength iwidth, iheight;
+      double dpi = FRAME_DISPLAY_INFO (f)->resx;
+
+      rsvg_handle_get_intrinsic_dimensions (rsvg_handle,
+					    &has_width, &iwidth,
+					    &has_height, &iheight,
+					    &has_viewbox, &viewbox);
+
+      if (has_width && has_height)
+	{
+	  /* Success!  We can use these values directly.  */
+	  viewbox_width = svg_css_length_to_pixels (iwidth, dpi,
+						    img->face_font_size);
+	  viewbox_height = svg_css_length_to_pixels (iheight, dpi,
+						     img->face_font_size);
+	}
+      else if (has_width && has_viewbox)
+	{
+	  viewbox_width = svg_css_length_to_pixels (iwidth, dpi,
+						    img->face_font_size);
+	  viewbox_height = viewbox_width * viewbox.height / viewbox.width;
+	}
+      else if (has_height && has_viewbox)
+	{
+	  viewbox_height = svg_css_length_to_pixels (iheight, dpi,
+						     img->face_font_size);
+	  viewbox_width = viewbox_height * viewbox.width / viewbox.height;
+	}
+      else if (has_viewbox)
+	{
+	  viewbox_width = viewbox.width;
+	  viewbox_height = viewbox.height;
+	}
+      else
+	viewbox_width = viewbox_height = 0;
+
+      if (! (0 < viewbox_width && 0 < viewbox_height))
+	{
+	  /* We haven't found a usable set of sizes, so try working out
+	     the visible area.  */
+	  rsvg_handle_get_geometry_for_layer (rsvg_handle, NULL,
+					      &zero_rect, &viewbox,
+					      &out_logical_rect, NULL);
+	  viewbox_width = viewbox.x + viewbox.width;
+	  viewbox_height = viewbox.y + viewbox.height;
+	}
     }
 #else
   /* In librsvg before 2.46.0, guess the viewbox from the image dimensions.  */
-- 
2.32.0