bug#40845: SVG rendering issues

[Alan Third <alan@idiocy.org>]

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

On Sat, May 09, 2020 at 03:27:27PM +0100, Alan Third wrote:
> On Sun, May 03, 2020 at 05:24:17PM +0100, Alan Third wrote:
> > 
> > I’ve attached a first pass. It solves 1 and 4 from the original bug
> > report: scaling and setting the foreground colour.
> 
> Can someone please try the patch from the previous email on a non‐NS
> platform and confirm whether or not the background colour in the image
> matches the background colour of the text?
> 
> They don’t match here, but I strongly suspect that’s because the NS
> port uses different colour spaces for images and everything else.

Apologies, please can someone try the attached patch.
-- 
Alan Third

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

From 6c1620ebd80bcfa1065b02350dfa875bec04fc0c 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  |  17 -----
 src/dispextern.h |   2 +-
 src/gtkutil.c    |   2 +-
 src/image.c      | 179 ++++++++++++++++++++++++++++++++++++-----------
 src/nsmenu.m     |   2 +-
 src/xdisp.c      |   6 +-
 6 files changed, 145 insertions(+), 63 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/dispextern.h b/src/dispextern.h
index 0b1f3d14ae..cd09e0846c 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3475,7 +3475,7 @@ #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
 void mark_image_cache (struct image_cache *);
 bool valid_image_p (Lisp_Object);
 void prepare_image_for_display (struct frame *, struct image *);
-ptrdiff_t lookup_image (struct frame *, Lisp_Object);
+ptrdiff_t lookup_image (struct frame *, Lisp_Object, int face_id);
 
 #if defined HAVE_X_WINDOWS || defined USE_CAIRO || defined HAVE_NS
 #define RGB_PIXEL_COLOR unsigned long
diff --git a/src/gtkutil.c b/src/gtkutil.c
index 681f86f51b..2f6b664d2d 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -5101,7 +5101,7 @@ update_frame_tool_bar (struct frame *f)
           else
             idx = -1;
 
-          img_id = lookup_image (f, image);
+          img_id = lookup_image (f, image, -1);
           img = IMAGE_FROM_ID (f, img_id);
           prepare_image_for_display (f, img);
 
diff --git a/src/image.c b/src/image.c
index c8a192aaaf..5333d66c7d 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1066,7 +1066,7 @@ DEFUN ("image-size", Fimage_size, Simage_size, 1, 3, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       int width = img->width + 2 * img->hmargin;
       int height = img->height + 2 * img->vmargin;
@@ -1096,7 +1096,7 @@ DEFUN ("image-mask-p", Fimage_mask_p, Simage_mask_p, 1, 2, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       if (img->mask)
 	mask = Qt;
@@ -1119,7 +1119,7 @@ DEFUN ("image-metadata", Fimage_metadata, Simage_metadata, 1, 2, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       ext = img->lisp_data;
     }
@@ -1586,7 +1586,8 @@ make_image_cache (void)
 /* Find an image matching SPEC in the cache, and return it.  If no
    image is found, return NULL.  */
 static struct image *
-search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
+search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
+                    unsigned long foreground, unsigned long background)
 {
   struct image *img;
   struct image_cache *c = FRAME_IMAGE_CACHE (f);
@@ -1609,8 +1610,8 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
   for (img = c->buckets[i]; img; img = img->next)
     if (img->hash == hash
 	&& !NILP (Fequal (img->spec, spec))
-	&& img->frame_foreground == FRAME_FOREGROUND_PIXEL (f)
-	&& img->frame_background == FRAME_BACKGROUND_PIXEL (f))
+	&& img->frame_foreground == foreground
+	&& img->frame_background == background)
       break;
   return img;
 }
@@ -1621,7 +1622,7 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
 static void
 uncache_image (struct frame *f, Lisp_Object spec)
 {
-  struct image *img = search_image_cache (f, spec, sxhash (spec));
+  struct image *img = search_image_cache (f, spec, sxhash (spec), FRAME_FOREGROUND_PIXEL (f), FRAME_BACKGROUND_PIXEL (f));
   if (img)
     {
       free_image (f, img);
@@ -2108,7 +2109,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;
@@ -2275,11 +2286,15 @@ image_set_transform (struct frame *f, struct image *img)
    SPEC must be a valid Lisp image specification (see valid_image_p).  */
 
 ptrdiff_t
-lookup_image (struct frame *f, Lisp_Object spec)
+lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 {
   struct image *img;
   EMACS_UINT hash;
 
+  struct face *face = (face_id >= 0) ? FACE_FROM_ID (f, face_id) : FRAME_DEFAULT_FACE (f);
+  unsigned long foreground = FACE_COLOR_TO_PIXEL (face->foreground, f);
+  unsigned long background = FACE_COLOR_TO_PIXEL (face->background, f);
+
   /* F must be a window-system frame, and SPEC must be a valid image
      specification.  */
   eassert (FRAME_WINDOW_P (f));
@@ -2287,7 +2302,7 @@ lookup_image (struct frame *f, Lisp_Object spec)
 
   /* Look up SPEC in the hash table of the image cache.  */
   hash = sxhash (spec);
-  img = search_image_cache (f, spec, hash);
+  img = search_image_cache (f, spec, hash, foreground, background);
   if (img && img->load_failed_p)
     {
       free_image (f, img);
@@ -2300,9 +2315,9 @@ lookup_image (struct frame *f, Lisp_Object spec)
       block_input ();
       img = make_image (spec, hash);
       cache_image (f, img);
+      img->frame_foreground = foreground;
+      img->frame_background = background;
       img->load_failed_p = ! img->type->load (f, img);
-      img->frame_foreground = FRAME_FOREGROUND_PIXEL (f);
-      img->frame_background = FRAME_BACKGROUND_PIXEL (f);
 
       /* If we can't load the image, and we don't have a width and
 	 height, use some arbitrary width and height so that we can
@@ -9393,6 +9408,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 +9427,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 +9692,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 +9701,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 +9731,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 = img->frame_foreground;
+    unsigned long background = img->frame_background;
+
+    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, img->frame_foreground);
+      }
+    value = image_spec_value (img->spec, QCbackground, NULL);
+    if (!NILP (value))
+      {
+        background = image_alloc_image_color (f, img, value, img->frame_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 +9850,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 +9875,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 +9888,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 +9918,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);
@@ -10119,7 +10218,7 @@ DEFUN ("lookup-image", Flookup_image, Slookup_image, 1, 1, 0,
   ptrdiff_t id = -1;
 
   if (valid_image_p (spec))
-    id = lookup_image (SELECTED_FRAME (), spec);
+    id = lookup_image (SELECTED_FRAME (), spec, -1);
 
   debug_print (spec);
   return make_fixnum (id);
diff --git a/src/nsmenu.m b/src/nsmenu.m
index b7e4cbd565..e313fc03f4 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -1092,7 +1092,7 @@ - (Lisp_Object)runMenuAt: (NSPoint)p forFrame: (struct frame *)f
           continue;
         }
 
-      img_id = lookup_image (f, image);
+      img_id = lookup_image (f, image, -1);
       img = IMAGE_FROM_ID (f, img_id);
       prepare_image_for_display (f, img);
 
diff --git a/src/xdisp.c b/src/xdisp.c
index 140d134572..f05fc4cb37 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -5690,7 +5690,7 @@ handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
       else
 	{
 	  it->what = IT_IMAGE;
-	  it->image_id = lookup_image (it->f, value);
+	  it->image_id = lookup_image (it->f, value, it->face_id);
 	  it->position = start_pos;
 	  it->object = NILP (object) ? it->w->contents : object;
 	  it->method = GET_FROM_IMAGE;
@@ -22379,7 +22379,7 @@ push_prefix_prop (struct it *it, Lisp_Object prop)
   else if (IMAGEP (prop))
     {
       it->what = IT_IMAGE;
-      it->image_id = lookup_image (it->f, prop);
+      it->image_id = lookup_image (it->f, prop, it->face_id);
       it->method = GET_FROM_IMAGE;
     }
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -27262,7 +27262,7 @@ calc_pixel_width_or_height (double *res, struct it *it, Lisp_Object prop,
 	  if (FRAME_WINDOW_P (it->f)
 	      && valid_image_p (prop))
 	    {
-	      ptrdiff_t id = lookup_image (it->f, prop);
+	      ptrdiff_t id = lookup_image (it->f, prop, it->face_id);
 	      struct image *img = IMAGE_FROM_ID (it->f, id);
 
 	      return OK_PIXELS (width_p ? img->width : img->height);
-- 
2.26.1