bug#40845: SVG rendering issues

[Alan Third <alan@idiocy.org>]

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

On Sun, May 03, 2020 at 07:07:56PM +0300, Eli Zaretskii wrote:
> > Date: Sun, 3 May 2020 15:13:48 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: Eli Zaretskii <eliz@gnu.org>, 40845@debbugs.gnu.org, pipcet@gmail.com
> > 
> > I’ve run into a problem with the base64 encoding.
> > 
> > /usr/bin/base64 encodes this file differently from Emacs:
> > 
> >     https://upload.wikimedia.org/wikipedia/commons/9/92/MixedUnitAddition.svg
> > 
> > librsvg doesn’t like the way Emacs encodes the file, but is fine with
> > the system base64.
> > 
> > I think the problem is the GBP £ symbol on line 39.
> > 
> > Do I need to do something special, like set up character encodings?
> 
> You need to make sure the text passed to base64-encode-region is
> unibyte.  I suggest to look at how other packages call that function.
> 
> Let me know if this general advice doesn't help.

Yes, it’s sorted the problem. Thanks!

I’ve attached a first pass. It solves 1 and 4 from the original bug
report: scaling and setting the foreground colour.

One thing is that it gets the foreground colour from the default face
instead of the current face. I’m not sure how to solve that.

I’m unsure if my use of malloc is OK, as I know Emacs code uses some
different methods in different places.

-- 
Alan Third

[-- Attachment #2: 0001-Set-basic-SVG-attributes-bug-40845.patch --]
[-- Type: text/plain, Size: 10906 bytes --]

From 78026c9697edda23073409a59176367fc1ad58ba Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Sun, 3 May 2020 17:09:31 +0100
Subject: [PATCH] Set basic SVG attributes (bug#40845)

* lisp/net/shr.el (shr-parse-image-data): Remove call to
svg--wrap-svg.
(svg--wrap-svg): Remove function.
* src/image.c (image_set_transform): SVGs should not be scaled by
native transforms as they should be the correct size already.
(enum svg_keyword_index):
(svg_format): Add the :foreground keyword.
(svg_load_image): Calculate the desired size of the SVG and wrap it
in another SVG that allows us to resize it.  Also set the background
and foreground colors.
---
 lisp/net/shr.el |  17 ------
 src/image.c     | 150 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 122 insertions(+), 45 deletions(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 1f80ab74db..c862cf04e3 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -1195,25 +1195,8 @@ shr-parse-image-data
             ;; that are non-ASCII.
 	    (shr-dom-to-xml
 	     (libxml-parse-xml-region (point) (point-max)) 'utf-8)))
-    ;; SVG images often do not have a specified foreground/background
-    ;; color, so wrap them in styles.
-    (when (and (display-images-p)
-               (eq content-type 'image/svg+xml))
-      (setq data (svg--wrap-svg data)))
     (list data content-type)))
 
-(defun svg--wrap-svg (data)
-  "Add a default foreground colour to SVG images."
-  (let ((size (image-size (create-image data nil t :scaling 1) t)))
-    (with-temp-buffer
-      (insert
-       (format
-        "<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\" xmlns:xi=\"http://www.w3.org/2001/XInclude\" style=\"color: %s;\" viewBox=\"0 0 %d %d\"> <xi:include href=\"data:image/svg+xml;base64,%s\"></xi:include></svg>"
-        (face-foreground 'default)
-        (car size) (cdr size)
-        (base64-encode-string data t)))
-      (buffer-string))))
-
 (defun shr-image-displayer (content-function)
   "Return a function to display an image.
 CONTENT-FUNCTION is a function to retrieve an image for a cid url that
diff --git a/src/image.c b/src/image.c
index c8a192aaaf..06760a62c2 100644
--- a/src/image.c
+++ b/src/image.c
@@ -2108,7 +2108,17 @@ image_set_transform (struct frame *f, struct image *img)
 
   /* Determine size.  */
   int width, height;
-  compute_image_size (img->width, img->height, img->spec, &width, &height);
+
+#ifdef HAVE_RSVG
+  /* SVGs are pre-scaled to the correct size.  */
+  if (EQ (image_spec_value (img->spec, QCtype, NULL), Qsvg))
+    {
+      width = img->width;
+      height = img->height;
+    }
+  else
+#endif
+    compute_image_size (img->width, img->height, img->spec, &width, &height);
 
   /* Determine rotation.  */
   double rotation = 0.0;
@@ -9393,6 +9403,7 @@ DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
   SVG_ALGORITHM,
   SVG_HEURISTIC_MASK,
   SVG_MASK,
+  SVG_FOREGROUND,
   SVG_BACKGROUND,
   SVG_LAST
 };
@@ -9411,6 +9422,7 @@ DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
   {":conversion",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
   {":heuristic-mask",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
   {":mask",		IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":foreground",	IMAGE_STRING_OR_NIL_VALUE,		0},
   {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
 };
 
@@ -9675,6 +9687,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   int height;
   const guint8 *pixels;
   int rowstride;
+  char *wrapped_contents;
+  ptrdiff_t wrapped_size;
 
 #if ! GLIB_CHECK_VERSION (2, 36, 0)
   /* g_type_init is a glib function that must be called prior to
@@ -9682,6 +9696,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   g_type_init ();
 #endif
 
+  /* Parse the unmodified SVG data so we can get it's initial size.  */
+
 #if LIBRSVG_CHECK_VERSION (2, 32, 0)
   GInputStream *input_stream
     = g_memory_input_stream_new_from_data (contents, size, NULL);
@@ -9710,6 +9726,107 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   rsvg_handle_write (rsvg_handle, (unsigned char *) contents, size, &err);
   if (err) goto rsvg_error;
 
+  /* The parsing is complete, rsvg_handle is ready to used, close it
+     for further writes.  */
+  rsvg_handle_close (rsvg_handle, &err);
+  if (err) goto rsvg_error;
+#endif
+
+  /* Get the image dimensions.  */
+  rsvg_handle_get_dimensions (rsvg_handle, &dimension_data);
+
+  /* We are now done with the unmodified data.  */
+  g_object_unref (rsvg_handle);
+
+  /* Calculate the final image size.  */
+  compute_image_size (dimension_data.width, dimension_data.height,
+                      img->spec, &width, &height);
+
+  /* Wrap the SVG data in another SVG.  This allows us to set the
+     width and height, as well as modify the foreground and background
+     colors.  */
+  {
+    Lisp_Object value;
+    unsigned long foreground = FRAME_FOREGROUND_PIXEL (f);
+    unsigned long background = FRAME_BACKGROUND_PIXEL (f);
+
+    Lisp_Object encoded_contents = Fbase64_encode_string
+      (make_unibyte_string (contents, size), Qt);
+
+    /* The wrapper sets the foreground color, width and height, and
+       viewBox must contain the dimensions of the original image.  It
+       also draws a rectangle over the whole space, set to the
+       background color, before including the original image. This
+       acts to set the background color, instead of leaving it
+       transparent.  */
+    const char *wrapper =
+      "<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\" "
+      "xmlns:xi=\"http://www.w3.org/2001/XInclude\" "
+      "style=\"color: #%06X; fill: currentColor;\" "
+      "width=\"%d\" height=\"%d\" preserveAspectRatio=\"none\" "
+      "viewBox=\"0 0 %d %d\">"
+      "<rect width=\"100%%\" height=\"100%%\" fill=\"#%06X\"/>"
+      "<xi:include href=\"data:image/svg+xml;base64,%s\"></xi:include>"
+      "</svg>";
+
+    /* FIXME: I've added 64 in the hope it will cover the size of the
+       width and height strings and things.  */
+    int buffer_size = SBYTES (encoded_contents) + strlen (wrapper) + 64;
+
+    value = image_spec_value (img->spec, QCforeground, NULL);
+    if (!NILP (value))
+      {
+        foreground = image_alloc_image_color (f, img, value, foreground);
+      }
+    value = image_spec_value (img->spec, QCbackground, NULL);
+    if (!NILP (value))
+      {
+        background = image_alloc_image_color (f, img, value, background);
+        img->background = background;
+        img->background_valid = 1;
+      }
+
+    wrapped_contents = malloc (buffer_size);
+
+    if (!wrapped_contents
+        || buffer_size <= snprintf (wrapped_contents, buffer_size, wrapper,
+                                    foreground & 0xFFFFFF, width, height,
+                                    dimension_data.width, dimension_data.height,
+                                    background & 0xFFFFFF, SSDATA (encoded_contents)))
+      goto rsvg_error;
+
+    wrapped_size = strlen (wrapped_contents);
+  }
+
+  /* Now we parse the wrapped version.  */
+
+#if LIBRSVG_CHECK_VERSION (2, 32, 0)
+  input_stream = g_memory_input_stream_new_from_data (wrapped_contents, wrapped_size, NULL);
+  base_file = filename ? g_file_new_for_path (filename) : NULL;
+  rsvg_handle = rsvg_handle_new_from_stream_sync (input_stream, base_file,
+						  RSVG_HANDLE_FLAGS_NONE,
+						  NULL, &err);
+  if (base_file)
+    g_object_unref (base_file);
+  g_object_unref (input_stream);
+
+  /* Check rsvg_handle too, to avoid librsvg 2.40.13 bug (Bug#36773#26).  */
+  if (!rsvg_handle || err) goto rsvg_error;
+#else
+  /* Make a handle to a new rsvg object.  */
+  rsvg_handle = rsvg_handle_new ();
+  eassume (rsvg_handle);
+
+  /* Set base_uri for properly handling referenced images (via 'href').
+     See rsvg bug 596114 - "image refs are relative to curdir, not .svg file"
+     <https://gitlab.gnome.org/GNOME/librsvg/issues/33>. */
+  if (filename)
+    rsvg_handle_set_base_uri (rsvg_handle, filename);
+
+  /* Parse the contents argument and fill in the rsvg_handle.  */
+  rsvg_handle_write (rsvg_handle, (unsigned char *) wrapped_contents, wrapped_size, &err);
+  if (err) goto rsvg_error;
+
   /* The parsing is complete, rsvg_handle is ready to used, close it
      for further writes.  */
   rsvg_handle_close (rsvg_handle, &err);
@@ -9728,6 +9845,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   pixbuf = rsvg_handle_get_pixbuf (rsvg_handle);
   if (!pixbuf) goto rsvg_error;
   g_object_unref (rsvg_handle);
+  free (wrapped_contents);
 
   /* Extract some meta data from the svg handle.  */
   width     = gdk_pixbuf_get_width (pixbuf);
@@ -9752,25 +9870,6 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 
     init_color_table ();
 
-    /* Handle alpha channel by combining the image with a background
-       color.  */
-    Emacs_Color background;
-    Lisp_Object specified_bg = image_spec_value (img->spec, QCbackground, NULL);
-    if (!STRINGP (specified_bg)
-	|| !FRAME_TERMINAL (f)->defined_color_hook (f,
-                                                    SSDATA (specified_bg),
-                                                    &background,
-                                                    false,
-                                                    false))
-      FRAME_TERMINAL (f)->query_frame_background_color (f, &background);
-
-    /* SVG pixmaps specify transparency in the last byte, so right
-       shift 8 bits to get rid of it, since emacs doesn't support
-       transparency.  */
-    background.red   >>= 8;
-    background.green >>= 8;
-    background.blue  >>= 8;
-
     /* This loop handles opacity values, since Emacs assumes
        non-transparent images.  Each pixel must be "flattened" by
        calculating the resulting color, given the transparency of the
@@ -9784,14 +9883,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 	    int blue    = *pixels++;
 	    int opacity = *pixels++;
 
-	    red   = ((red * opacity)
-		     + (background.red * ((1 << 8) - opacity)));
-	    green = ((green * opacity)
-		     + (background.green * ((1 << 8) - opacity)));
-	    blue  = ((blue * opacity)
-		     + (background.blue * ((1 << 8) - opacity)));
-
-	    PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
+	    PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red << 8, green << 8, blue << 8));
 	  }
 
 	pixels += rowstride - 4 * width;
@@ -9821,6 +9913,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
  rsvg_error:
   if (rsvg_handle)
     g_object_unref (rsvg_handle);
+  if (wrapped_contents)
+    free (wrapped_contents);
   /* FIXME: Use error->message so the user knows what is the actual
      problem with the image.  */
   image_error ("Error parsing SVG image `%s'", img->spec);
-- 
2.26.1