bug#36315: 27.0.50; SVG transparency handling is inaccurate

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Pip Cet]

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

Evaluate the following in emacs -Q:

(require 'svg)

(defun make-image (color)
  (let ((svg (svg-create 100 100)))
    (svg-rectangle svg 0 0 100 100 :fill color)
    (svg-image svg)))

(set-frame-parameter (window-frame) 'background-color "black")

(insert (propertize " " 'display (make-image "#f00000")))

The expected result is a rectangle (on black background) of color
#f00000. The actual result is a rectangle of color #ef0000. For black
backgrounds, white is no longer representable.

This is related to bug #36304, but much easier to fix.

Patch attached.

[-- Attachment #2: 0001-SVG-scale-color-values-properly.patch --]
[-- Type: text/x-patch, Size: 1707 bytes --]

From 7abe6404d3af04db2f5a503c1c873f80ab86f69e Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Thu, 20 Jun 2019 20:13:12 +0000
Subject: [PATCH] SVG: scale color values properly

* src/image.c (svg_load_image): scale color channel values to proper
range.
---
 src/image.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/image.c b/src/image.c
index 866323ba6e..6b6235a617 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9658,17 +9658,20 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
       {
 	for (int x = 0; x < width; ++x)
 	  {
-	    int red     = *pixels++;
-	    int green   = *pixels++;
-	    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)));
+	    unsigned int red     = *pixels++;
+	    unsigned int green   = *pixels++;
+	    unsigned int blue    = *pixels++;
+	    unsigned int opacity = *pixels++;
+
+	    /* opacity and the color channel values are in the range {0..255},
+	     * but expected output values are in the range {0..65535}, so scale
+	     * by (256/255)^2. */
+#define MIX(a, b, opacity)						\
+	    (((((a) * opacity) + ((b) * (255 - opacity))) * 65535 + 32512) / 65025)
+	    red   = MIX (red, background.red, opacity);
+	    green = MIX (green, background.red, opacity);
+	    blue  = MIX (blue, background.red, opacity);
+#undef MIX
 
 	    PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
 	  }
-- 
2.20.1

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Pip Cet]

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

Oops, typo in the patch. Better patch attached.

Subject: [PATCH] SVG: scale color values properly

* src/image.c (svg_load_image): scale color channel values to proper
range.
---
 src/image.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/image.c b/src/image.c
index 866323ba6e..8e25f1f590 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9658,17 +9658,20 @@ svg_load_image (struct frame *f, struct image
*img, char *contents,
       {
     for (int x = 0; x < width; ++x)
       {
-        int red     = *pixels++;
-        int green   = *pixels++;
-        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)));
+        unsigned int red     = *pixels++;
+        unsigned int green   = *pixels++;
+        unsigned int blue    = *pixels++;
+        unsigned int opacity = *pixels++;
+
+        /* opacity and the color channel values are in the range {0..255},
+         * but expected output values are in the range {0..65535}, so scale
+         * by (256/255)^2. */
+#define MIX(a, b, opacity)                        \
+        (((((a) * opacity) + ((b) * (255 - opacity))) * 65535 + 32512) / 65025)
+        red   = MIX (red, background.red, opacity);
+        green = MIX (green, background.green, opacity);
+        blue  = MIX (blue, background.blue, opacity);
+#undef MIX

         PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
       }
-- 
2.20.1


On Thu, Jun 20, 2019 at 8:43 PM Pip Cet <pipcet@gmail.com> wrote:
>
> Evaluate the following in emacs -Q:
>
> (require 'svg)
>
> (defun make-image (color)
>   (let ((svg (svg-create 100 100)))
>     (svg-rectangle svg 0 0 100 100 :fill color)
>     (svg-image svg)))
>
> (set-frame-parameter (window-frame) 'background-color "black")
>
> (insert (propertize " " 'display (make-image "#f00000")))
>
> The expected result is a rectangle (on black background) of color
> #f00000. The actual result is a rectangle of color #ef0000. For black
> backgrounds, white is no longer representable.
>
> This is related to bug #36304, but much easier to fix.
>
> Patch attached.

[-- Attachment #2: 0001-SVG-scale-color-values-properly.patch --]
[-- Type: application/x-patch, Size: 1710 bytes --]

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Alan Third]

On Thu, Jun 20, 2019 at 08:46:20PM +0000, Pip Cet wrote:
> Oops, typo in the patch. Better patch attached.

I know nothing about librsvg, but the patch does appear to fix the
described behaviour (although I had to try #FFFFFF because #F00000,
appeared to come out as something else entirely in both cases).
-- 
Alan Third

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu]

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

On Fri, 21 Jun 2019 05:26:53 +0900,
Pip Cet wrote:
> 
> [1  <text/plain; UTF-8 (7bit)>]
> Evaluate the following in emacs -Q:
> 
> (require 'svg)
> 
> (defun make-image (color)
>   (let ((svg (svg-create 100 100)))
>     (svg-rectangle svg 0 0 100 100 :fill color)
>     (svg-image svg)))
> 
> (set-frame-parameter (window-frame) 'background-color "black")
> 
> (insert (propertize " " 'display (make-image "#f00000")))
> 
> The expected result is a rectangle (on black background) of color
> #f00000. The actual result is a rectangle of color #ef0000. For black
> backgrounds, white is no longer representable.
> 
> This is related to bug #36304, but much easier to fix.
> 
> Patch attached.

An alternative way would be to use rsvg_handle_render_cairo, which is
recommended by librsvg, and let it blend with the background color.

Patch attached.  Note that this does not require --with-cairo.
Raising the required version of librsvg to 2.14 is not a problem, as
we are already using rsvg_handle_get_dimensions that requires that
version.  Is Windows librsvg DLL compiled with libcairo?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

[-- Attachment #2: svg-cairo.diff --]
[-- Type: application/octet-stream, Size: 12879 bytes --]

diff --git a/configure.ac b/configure.ac
index 0507f58054a..4fe9700b03e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2603,7 +2603,7 @@ AC_DEFUN
 HAVE_RSVG=no
 if test "${HAVE_X11}" = "yes" || test "${HAVE_NS}" = "yes" || test "${opsys}" = "mingw32"; then
   if test "${with_rsvg}" != "no"; then
-    RSVG_REQUIRED=2.11.0
+    RSVG_REQUIRED=2.14.0
     RSVG_MODULE="librsvg-2.0 >= $RSVG_REQUIRED"
 
     EMACS_CHECK_MODULES([RSVG], [$RSVG_MODULE])
diff --git a/src/image.c b/src/image.c
index 7b648c46ae9..f93898aedd0 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9379,17 +9379,17 @@ DEF_DLL_FN (void, rsvg_handle_get_dimensions,
 DEF_DLL_FN (gboolean, rsvg_handle_write,
 	    (RsvgHandle *, const guchar *, gsize, GError **));
 DEF_DLL_FN (gboolean, rsvg_handle_close, (RsvgHandle *, GError **));
-DEF_DLL_FN (GdkPixbuf *, rsvg_handle_get_pixbuf, (RsvgHandle *));
 DEF_DLL_FN (void, rsvg_handle_set_base_uri, (RsvgHandle *, const char *));
+DEF_DLL_FN (gboolean, rsvg_handle_render_cairo, (RsvgHandle *, cairo_t *));
 
-DEF_DLL_FN (int, gdk_pixbuf_get_width, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_height, (const GdkPixbuf *));
-DEF_DLL_FN (guchar *, gdk_pixbuf_get_pixels, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_rowstride, (const GdkPixbuf *));
-DEF_DLL_FN (GdkColorspace, gdk_pixbuf_get_colorspace, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_n_channels, (const GdkPixbuf *));
-DEF_DLL_FN (gboolean, gdk_pixbuf_get_has_alpha, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_bits_per_sample, (const GdkPixbuf *));
+DEF_DLL_FN (cairo_surface_t *, cairo_image_surface_create,
+	    (cairo_format_t, int, int));
+DEF_DLL_FN (cairo_t *, cairo_create, (cairo_surface_t *));
+DEF_DLL_FN (void, cairo_set_source_rgb, (cairo_t *, double, double, double));
+DEF_DLL_FN (void, cairo_paint, (cairo_t *));
+DEF_DLL_FN (void, cairo_destroy, (cairo_t *));
+DEF_DLL_FN (unsigned char *, cairo_image_surface_get_data, (cairo_surface_t *));
+DEF_DLL_FN (int, cairo_image_surface_get_stride, (cairo_surface_t *));
 
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
 DEF_DLL_FN (void, g_type_init, (void));
@@ -9400,14 +9400,14 @@ DEF_DLL_FN (void, g_clear_error, (GError **));
 static bool
 init_svg_functions (void)
 {
-  HMODULE library, gdklib = NULL, glib = NULL, gobject = NULL;
+  HMODULE library, cairo = NULL, glib = NULL, gobject = NULL;
 
-  if (!(glib = w32_delayed_load (Qglib))
+  if (!(cairo = w32_delayed_load (Qcairo))
+      || !(glib = w32_delayed_load (Qglib))
       || !(gobject = w32_delayed_load (Qgobject))
-      || !(gdklib = w32_delayed_load (Qgdk_pixbuf))
       || !(library = w32_delayed_load (Qsvg)))
     {
-      if (gdklib)  FreeLibrary (gdklib);
+      if (cairo)   FreeLibrary (cairo);
       if (gobject) FreeLibrary (gobject);
       if (glib)    FreeLibrary (glib);
       return 0;
@@ -9417,17 +9417,16 @@ init_svg_functions (void)
   LOAD_DLL_FN (library, rsvg_handle_get_dimensions);
   LOAD_DLL_FN (library, rsvg_handle_write);
   LOAD_DLL_FN (library, rsvg_handle_close);
-  LOAD_DLL_FN (library, rsvg_handle_get_pixbuf);
   LOAD_DLL_FN (library, rsvg_handle_set_base_uri);
+  LOAD_DLL_FN (library, rsvg_handle_render_cairo);
 
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_width);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_height);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_pixels);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_rowstride);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_colorspace);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_n_channels);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_has_alpha);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_bits_per_sample);
+  LOAD_DLL_FN (cairo, cairo_image_surface_create);
+  LOAD_DLL_FN (cairo, cairo_create);
+  LOAD_DLL_FN (cairo, cairo_set_source_rgb);
+  LOAD_DLL_FN (cairo, cairo_paint);
+  LOAD_DLL_FN (cairo, cairo_destroy);
+  LOAD_DLL_FN (cairo, cairo_image_surface_get_data);
+  LOAD_DLL_FN (cairo, cairo_image_surface_get_stride);
 
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
   LOAD_DLL_FN (gobject, g_type_init);
@@ -9441,32 +9440,30 @@ init_svg_functions (void)
 /* The following aliases for library functions allow dynamic loading
    to be used on some platforms.  */
 
-#  undef gdk_pixbuf_get_bits_per_sample
-#  undef gdk_pixbuf_get_colorspace
-#  undef gdk_pixbuf_get_has_alpha
-#  undef gdk_pixbuf_get_height
-#  undef gdk_pixbuf_get_n_channels
-#  undef gdk_pixbuf_get_pixels
-#  undef gdk_pixbuf_get_rowstride
-#  undef gdk_pixbuf_get_width
+#  undef cairo_create
+#  undef cairo_destroy
+#  undef cairo_image_surface_create
+#  undef cairo_image_surface_get_data
+#  undef cairo_image_surface_get_stride
+#  undef cairo_paint
+#  undef cairo_set_source_rgb
 #  undef g_clear_error
 #  undef g_object_unref
 #  undef g_type_init
 #  undef rsvg_handle_close
 #  undef rsvg_handle_get_dimensions
-#  undef rsvg_handle_get_pixbuf
 #  undef rsvg_handle_new
+#  undef rsvg_handle_render_cairo
 #  undef rsvg_handle_set_base_uri
 #  undef rsvg_handle_write
 
-#  define gdk_pixbuf_get_bits_per_sample fn_gdk_pixbuf_get_bits_per_sample
-#  define gdk_pixbuf_get_colorspace fn_gdk_pixbuf_get_colorspace
-#  define gdk_pixbuf_get_has_alpha fn_gdk_pixbuf_get_has_alpha
-#  define gdk_pixbuf_get_height fn_gdk_pixbuf_get_height
-#  define gdk_pixbuf_get_n_channels fn_gdk_pixbuf_get_n_channels
-#  define gdk_pixbuf_get_pixels fn_gdk_pixbuf_get_pixels
-#  define gdk_pixbuf_get_rowstride fn_gdk_pixbuf_get_rowstride
-#  define gdk_pixbuf_get_width fn_gdk_pixbuf_get_width
+#  define cairo_create fn_cairo_create
+#  define cairo_destroy fn_cairo_destroy
+#  define cairo_image_surface_create fn_cairo_image_surface_create
+#  define cairo_image_surface_get_data fn_cairo_image_surface_get_data
+#  define cairo_image_surface_get_stride fn_cairo_image_surface_get_stride
+#  define cairo_paint fn_cairo_paint
+#  define cairo_set_source_rgb fn_cairo_set_source_rgb
 #  define g_clear_error fn_g_clear_error
 #  define g_object_unref fn_g_object_unref
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
@@ -9474,8 +9471,8 @@ init_svg_functions (void)
 #  endif
 #  define rsvg_handle_close fn_rsvg_handle_close
 #  define rsvg_handle_get_dimensions fn_rsvg_handle_get_dimensions
-#  define rsvg_handle_get_pixbuf fn_rsvg_handle_get_pixbuf
 #  define rsvg_handle_new fn_rsvg_handle_new
+#  define rsvg_handle_render_cairo fn_rsvg_handle_render_cairo
 #  define rsvg_handle_set_base_uri fn_rsvg_handle_set_base_uri
 #  define rsvg_handle_write fn_rsvg_handle_write
 
@@ -9550,11 +9547,6 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   RsvgHandle *rsvg_handle;
   RsvgDimensionData dimension_data;
   GError *err = NULL;
-  GdkPixbuf *pixbuf;
-  int width;
-  int height;
-  const guint8 *pixels;
-  int rowstride;
 
 #if ! GLIB_CHECK_VERSION (2, 36, 0)
   /* g_type_init is a glib function that must be called prior to
@@ -9596,104 +9588,78 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   #endif
 
   rsvg_handle_get_dimensions (rsvg_handle, &dimension_data);
-  if (! check_image_size (f, dimension_data.width, dimension_data.height))
+  int width = dimension_data.width, height = dimension_data.height;
+  if (! check_image_size (f, width, height))
     {
       image_size_error ();
       goto rsvg_error;
     }
 
-  /* We can now get a valid pixel buffer from the svg file, if all
-     went ok.  */
-  pixbuf = rsvg_handle_get_pixbuf (rsvg_handle);
-  if (!pixbuf) goto rsvg_error;
-  g_object_unref (rsvg_handle);
-
-  /* Extract some meta data from the svg handle.  */
-  width     = gdk_pixbuf_get_width (pixbuf);
-  height    = gdk_pixbuf_get_height (pixbuf);
-  pixels    = gdk_pixbuf_get_pixels (pixbuf);
-  rowstride = gdk_pixbuf_get_rowstride (pixbuf);
-
-  /* Validate the svg meta data.  */
-  eassert (gdk_pixbuf_get_colorspace (pixbuf) == GDK_COLORSPACE_RGB);
-  eassert (gdk_pixbuf_get_n_channels (pixbuf) == 4);
-  eassert (gdk_pixbuf_get_has_alpha (pixbuf));
-  eassert (gdk_pixbuf_get_bits_per_sample (pixbuf) == 8);
-
-  {
-    /* Try to create a x pixmap to hold the svg pixmap.  */
-    Emacs_Pix_Container ximg;
-    if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
-      {
-	g_object_unref (pixbuf);
-	return 0;
-      }
+  /* Try to create a x pixmap to hold the svg pixmap.  */
+  Emacs_Pix_Container ximg;
+  if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
+    {
+      g_object_unref (rsvg_handle);
+      return 0;
+    }
 
-    init_color_table ();
+  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
-       pixel, and the image background color.  */
-    for (int y = 0; y < height; ++y)
-      {
-	for (int x = 0; x < width; ++x)
-	  {
-	    int red     = *pixels++;
-	    int green   = *pixels++;
-	    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));
-	  }
+  /* 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);
+
+  cairo_surface_t *surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24,
+							 width, height);
+  cairo_t *cr = cairo_create (surface);
+  cairo_set_source_rgb (cr, background.red >> 8,
+			background.green >> 8, background.blue >> 8);
+  cairo_paint (cr);
+  rsvg_handle_render_cairo (rsvg_handle, cr);
+  cairo_destroy (cr);
+  g_object_unref (rsvg_handle);
 
-	pixels += rowstride - 4 * width;
-      }
+  unsigned char *data = cairo_image_surface_get_data (surface);
+  int stride = cairo_image_surface_get_stride (surface);
+  for (int y = 0; y < height; ++y)
+    {
+      guint32 *pixels = (guint32 *) data;
+      for (int x = 0; x < width; ++x)
+	{
+	  guint32 rgb = *pixels++;
+	  int red   = ((rgb >> 16) & 0xff) << 8;
+	  int green = ((rgb >> 8) & 0xff) << 8;
+	  int blue  = (rgb & 0xff) << 8;
+	  PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
+	}
+      data += stride;
+    }
 
 #ifdef COLOR_TABLE_SUPPORT
-    /* Remember colors allocated for this image.  */
-    img->colors = colors_in_color_table (&img->ncolors);
-    free_color_table ();
+  /* Remember colors allocated for this image.  */
+  img->colors = colors_in_color_table (&img->ncolors);
+  free_color_table ();
 #endif /* COLOR_TABLE_SUPPORT */
 
-    g_object_unref (pixbuf);
+  cairo_surface_destroy (surface);
 
-    img->width  = width;
-    img->height = height;
+  img->width  = width;
+  img->height = height;
 
-    /* Maybe fill in the background field while we have ximg handy.
-       Casting avoids a GCC warning.  */
-    IMAGE_BACKGROUND (img, f, (Emacs_Pix_Context)ximg);
+  /* Maybe fill in the background field while we have ximg handy.
+     Casting avoids a GCC warning.  */
+  IMAGE_BACKGROUND (img, f, (Emacs_Pix_Context)ximg);
 
-    /* Put ximg into the image.  */
-    image_put_x_image (f, img, ximg, 0);
-  }
+  /* Put ximg into the image.  */
+  image_put_x_image (f, img, ximg, 0);
 
   return 1;
 
@@ -10249,7 +10215,7 @@ non-numeric, there is no explicit limit on the size of images.  */);
   add_image_type (Qsvg);
 #ifdef HAVE_NTGUI
   /* Other libraries used directly by svg code.  */
-  DEFSYM (Qgdk_pixbuf, "gdk-pixbuf");
+  DEFSYM (Qcairo, "cairo");
   DEFSYM (Qglib, "glib");
   DEFSYM (Qgobject, "gobject");
 #endif /* HAVE_NTGUI  */

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu]

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

On Mon, 24 Jun 2019 16:56:45 +0900,
YAMAMOTO Mitsuharu wrote:
> 
> An alternative way would be to use rsvg_handle_render_cairo, which is
> recommended by librsvg, and let it blend with the background color.
> 
> Patch attached.  Note that this does not require --with-cairo.
> Raising the required version of librsvg to 2.14 is not a problem, as
> we are already using rsvg_handle_get_dimensions that requires that
> version.  Is Windows librsvg DLL compiled with libcairo?

Sorry, wrong patch.  Please try this instead.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

[-- Attachment #2: svg-cairo.diff --]
[-- Type: application/octet-stream, Size: 12898 bytes --]

diff --git a/configure.ac b/configure.ac
index 0507f58054a..4fe9700b03e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2603,7 +2603,7 @@ AC_DEFUN
 HAVE_RSVG=no
 if test "${HAVE_X11}" = "yes" || test "${HAVE_NS}" = "yes" || test "${opsys}" = "mingw32"; then
   if test "${with_rsvg}" != "no"; then
-    RSVG_REQUIRED=2.11.0
+    RSVG_REQUIRED=2.14.0
     RSVG_MODULE="librsvg-2.0 >= $RSVG_REQUIRED"
 
     EMACS_CHECK_MODULES([RSVG], [$RSVG_MODULE])
diff --git a/src/image.c b/src/image.c
index 7b648c46ae9..924a188b4a0 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9379,17 +9379,17 @@ DEF_DLL_FN (void, rsvg_handle_get_dimensions,
 DEF_DLL_FN (gboolean, rsvg_handle_write,
 	    (RsvgHandle *, const guchar *, gsize, GError **));
 DEF_DLL_FN (gboolean, rsvg_handle_close, (RsvgHandle *, GError **));
-DEF_DLL_FN (GdkPixbuf *, rsvg_handle_get_pixbuf, (RsvgHandle *));
 DEF_DLL_FN (void, rsvg_handle_set_base_uri, (RsvgHandle *, const char *));
+DEF_DLL_FN (gboolean, rsvg_handle_render_cairo, (RsvgHandle *, cairo_t *));
 
-DEF_DLL_FN (int, gdk_pixbuf_get_width, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_height, (const GdkPixbuf *));
-DEF_DLL_FN (guchar *, gdk_pixbuf_get_pixels, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_rowstride, (const GdkPixbuf *));
-DEF_DLL_FN (GdkColorspace, gdk_pixbuf_get_colorspace, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_n_channels, (const GdkPixbuf *));
-DEF_DLL_FN (gboolean, gdk_pixbuf_get_has_alpha, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_bits_per_sample, (const GdkPixbuf *));
+DEF_DLL_FN (cairo_surface_t *, cairo_image_surface_create,
+	    (cairo_format_t, int, int));
+DEF_DLL_FN (cairo_t *, cairo_create, (cairo_surface_t *));
+DEF_DLL_FN (void, cairo_set_source_rgb, (cairo_t *, double, double, double));
+DEF_DLL_FN (void, cairo_paint, (cairo_t *));
+DEF_DLL_FN (void, cairo_destroy, (cairo_t *));
+DEF_DLL_FN (unsigned char *, cairo_image_surface_get_data, (cairo_surface_t *));
+DEF_DLL_FN (int, cairo_image_surface_get_stride, (cairo_surface_t *));
 
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
 DEF_DLL_FN (void, g_type_init, (void));
@@ -9400,14 +9400,14 @@ DEF_DLL_FN (void, g_clear_error, (GError **));
 static bool
 init_svg_functions (void)
 {
-  HMODULE library, gdklib = NULL, glib = NULL, gobject = NULL;
+  HMODULE library, cairo = NULL, glib = NULL, gobject = NULL;
 
-  if (!(glib = w32_delayed_load (Qglib))
+  if (!(cairo = w32_delayed_load (Qcairo))
+      || !(glib = w32_delayed_load (Qglib))
       || !(gobject = w32_delayed_load (Qgobject))
-      || !(gdklib = w32_delayed_load (Qgdk_pixbuf))
       || !(library = w32_delayed_load (Qsvg)))
     {
-      if (gdklib)  FreeLibrary (gdklib);
+      if (cairo)   FreeLibrary (cairo);
       if (gobject) FreeLibrary (gobject);
       if (glib)    FreeLibrary (glib);
       return 0;
@@ -9417,17 +9417,16 @@ init_svg_functions (void)
   LOAD_DLL_FN (library, rsvg_handle_get_dimensions);
   LOAD_DLL_FN (library, rsvg_handle_write);
   LOAD_DLL_FN (library, rsvg_handle_close);
-  LOAD_DLL_FN (library, rsvg_handle_get_pixbuf);
   LOAD_DLL_FN (library, rsvg_handle_set_base_uri);
+  LOAD_DLL_FN (library, rsvg_handle_render_cairo);
 
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_width);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_height);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_pixels);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_rowstride);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_colorspace);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_n_channels);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_has_alpha);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_bits_per_sample);
+  LOAD_DLL_FN (cairo, cairo_image_surface_create);
+  LOAD_DLL_FN (cairo, cairo_create);
+  LOAD_DLL_FN (cairo, cairo_set_source_rgb);
+  LOAD_DLL_FN (cairo, cairo_paint);
+  LOAD_DLL_FN (cairo, cairo_destroy);
+  LOAD_DLL_FN (cairo, cairo_image_surface_get_data);
+  LOAD_DLL_FN (cairo, cairo_image_surface_get_stride);
 
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
   LOAD_DLL_FN (gobject, g_type_init);
@@ -9441,32 +9440,30 @@ init_svg_functions (void)
 /* The following aliases for library functions allow dynamic loading
    to be used on some platforms.  */
 
-#  undef gdk_pixbuf_get_bits_per_sample
-#  undef gdk_pixbuf_get_colorspace
-#  undef gdk_pixbuf_get_has_alpha
-#  undef gdk_pixbuf_get_height
-#  undef gdk_pixbuf_get_n_channels
-#  undef gdk_pixbuf_get_pixels
-#  undef gdk_pixbuf_get_rowstride
-#  undef gdk_pixbuf_get_width
+#  undef cairo_create
+#  undef cairo_destroy
+#  undef cairo_image_surface_create
+#  undef cairo_image_surface_get_data
+#  undef cairo_image_surface_get_stride
+#  undef cairo_paint
+#  undef cairo_set_source_rgb
 #  undef g_clear_error
 #  undef g_object_unref
 #  undef g_type_init
 #  undef rsvg_handle_close
 #  undef rsvg_handle_get_dimensions
-#  undef rsvg_handle_get_pixbuf
 #  undef rsvg_handle_new
+#  undef rsvg_handle_render_cairo
 #  undef rsvg_handle_set_base_uri
 #  undef rsvg_handle_write
 
-#  define gdk_pixbuf_get_bits_per_sample fn_gdk_pixbuf_get_bits_per_sample
-#  define gdk_pixbuf_get_colorspace fn_gdk_pixbuf_get_colorspace
-#  define gdk_pixbuf_get_has_alpha fn_gdk_pixbuf_get_has_alpha
-#  define gdk_pixbuf_get_height fn_gdk_pixbuf_get_height
-#  define gdk_pixbuf_get_n_channels fn_gdk_pixbuf_get_n_channels
-#  define gdk_pixbuf_get_pixels fn_gdk_pixbuf_get_pixels
-#  define gdk_pixbuf_get_rowstride fn_gdk_pixbuf_get_rowstride
-#  define gdk_pixbuf_get_width fn_gdk_pixbuf_get_width
+#  define cairo_create fn_cairo_create
+#  define cairo_destroy fn_cairo_destroy
+#  define cairo_image_surface_create fn_cairo_image_surface_create
+#  define cairo_image_surface_get_data fn_cairo_image_surface_get_data
+#  define cairo_image_surface_get_stride fn_cairo_image_surface_get_stride
+#  define cairo_paint fn_cairo_paint
+#  define cairo_set_source_rgb fn_cairo_set_source_rgb
 #  define g_clear_error fn_g_clear_error
 #  define g_object_unref fn_g_object_unref
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
@@ -9474,8 +9471,8 @@ init_svg_functions (void)
 #  endif
 #  define rsvg_handle_close fn_rsvg_handle_close
 #  define rsvg_handle_get_dimensions fn_rsvg_handle_get_dimensions
-#  define rsvg_handle_get_pixbuf fn_rsvg_handle_get_pixbuf
 #  define rsvg_handle_new fn_rsvg_handle_new
+#  define rsvg_handle_render_cairo fn_rsvg_handle_render_cairo
 #  define rsvg_handle_set_base_uri fn_rsvg_handle_set_base_uri
 #  define rsvg_handle_write fn_rsvg_handle_write
 
@@ -9550,11 +9547,6 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   RsvgHandle *rsvg_handle;
   RsvgDimensionData dimension_data;
   GError *err = NULL;
-  GdkPixbuf *pixbuf;
-  int width;
-  int height;
-  const guint8 *pixels;
-  int rowstride;
 
 #if ! GLIB_CHECK_VERSION (2, 36, 0)
   /* g_type_init is a glib function that must be called prior to
@@ -9596,104 +9588,79 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   #endif
 
   rsvg_handle_get_dimensions (rsvg_handle, &dimension_data);
-  if (! check_image_size (f, dimension_data.width, dimension_data.height))
+  int width = dimension_data.width, height = dimension_data.height;
+  if (! check_image_size (f, width, height))
     {
       image_size_error ();
       goto rsvg_error;
     }
 
-  /* We can now get a valid pixel buffer from the svg file, if all
-     went ok.  */
-  pixbuf = rsvg_handle_get_pixbuf (rsvg_handle);
-  if (!pixbuf) goto rsvg_error;
-  g_object_unref (rsvg_handle);
-
-  /* Extract some meta data from the svg handle.  */
-  width     = gdk_pixbuf_get_width (pixbuf);
-  height    = gdk_pixbuf_get_height (pixbuf);
-  pixels    = gdk_pixbuf_get_pixels (pixbuf);
-  rowstride = gdk_pixbuf_get_rowstride (pixbuf);
-
-  /* Validate the svg meta data.  */
-  eassert (gdk_pixbuf_get_colorspace (pixbuf) == GDK_COLORSPACE_RGB);
-  eassert (gdk_pixbuf_get_n_channels (pixbuf) == 4);
-  eassert (gdk_pixbuf_get_has_alpha (pixbuf));
-  eassert (gdk_pixbuf_get_bits_per_sample (pixbuf) == 8);
-
-  {
-    /* Try to create a x pixmap to hold the svg pixmap.  */
-    Emacs_Pix_Container ximg;
-    if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
-      {
-	g_object_unref (pixbuf);
-	return 0;
-      }
+  /* Try to create a x pixmap to hold the svg pixmap.  */
+  Emacs_Pix_Container ximg;
+  if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
+    {
+      g_object_unref (rsvg_handle);
+      return 0;
+    }
 
-    init_color_table ();
+  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
-       pixel, and the image background color.  */
-    for (int y = 0; y < height; ++y)
-      {
-	for (int x = 0; x < width; ++x)
-	  {
-	    int red     = *pixels++;
-	    int green   = *pixels++;
-	    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));
-	  }
+  /* 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);
+
+  cairo_surface_t *surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24,
+							 width, height);
+  cairo_t *cr = cairo_create (surface);
+  cairo_set_source_rgb (cr, background.red / 65535.0,
+			background.green / 65535.0,
+			background.blue / 65535.0);
+  cairo_paint (cr);
+  rsvg_handle_render_cairo (rsvg_handle, cr);
+  cairo_destroy (cr);
+  g_object_unref (rsvg_handle);
 
-	pixels += rowstride - 4 * width;
-      }
+  unsigned char *data = cairo_image_surface_get_data (surface);
+  int stride = cairo_image_surface_get_stride (surface);
+  for (int y = 0; y < height; ++y)
+    {
+      guint32 *pixels = (guint32 *) data;
+      for (int x = 0; x < width; ++x)
+	{
+	  guint32 rgb = *pixels++;
+	  int red   = ((rgb >> 16) & 0xff) << 8;
+	  int green = ((rgb >> 8) & 0xff) << 8;
+	  int blue  = (rgb & 0xff) << 8;
+	  PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
+	}
+      data += stride;
+    }
 
 #ifdef COLOR_TABLE_SUPPORT
-    /* Remember colors allocated for this image.  */
-    img->colors = colors_in_color_table (&img->ncolors);
-    free_color_table ();
+  /* Remember colors allocated for this image.  */
+  img->colors = colors_in_color_table (&img->ncolors);
+  free_color_table ();
 #endif /* COLOR_TABLE_SUPPORT */
 
-    g_object_unref (pixbuf);
+  cairo_surface_destroy (surface);
 
-    img->width  = width;
-    img->height = height;
+  img->width  = width;
+  img->height = height;
 
-    /* Maybe fill in the background field while we have ximg handy.
-       Casting avoids a GCC warning.  */
-    IMAGE_BACKGROUND (img, f, (Emacs_Pix_Context)ximg);
+  /* Maybe fill in the background field while we have ximg handy.
+     Casting avoids a GCC warning.  */
+  IMAGE_BACKGROUND (img, f, (Emacs_Pix_Context)ximg);
 
-    /* Put ximg into the image.  */
-    image_put_x_image (f, img, ximg, 0);
-  }
+  /* Put ximg into the image.  */
+  image_put_x_image (f, img, ximg, 0);
 
   return 1;
 
@@ -10249,7 +10216,7 @@ non-numeric, there is no explicit limit on the size of images.  */);
   add_image_type (Qsvg);
 #ifdef HAVE_NTGUI
   /* Other libraries used directly by svg code.  */
-  DEFSYM (Qgdk_pixbuf, "gdk-pixbuf");
+  DEFSYM (Qcairo, "cairo");
   DEFSYM (Qglib, "glib");
   DEFSYM (Qgobject, "gobject");
 #endif /* HAVE_NTGUI  */

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Pip Cet]

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

On Mon, Jun 24, 2019 at 8:17 AM YAMAMOTO Mitsuharu
<mituharu@math.s.chiba-u.ac.jp> wrote:
> On Mon, 24 Jun 2019 16:56:45 +0900,
> YAMAMOTO Mitsuharu wrote:
> >
> > An alternative way would be to use rsvg_handle_render_cairo, which is
> > recommended by librsvg, and let it blend with the background color.
> >
> > Patch attached.  Note that this does not require --with-cairo.
> > Raising the required version of librsvg to 2.14 is not a problem, as
> > we are already using rsvg_handle_get_dimensions that requires that
> > version.  Is Windows librsvg DLL compiled with libcairo?
>
> Sorry, wrong patch.  Please try this instead.

Thank you very much, that fixes the problem. Unfortunately, I do not
know about the situation on Windows.

I'm not sure about the additional changes in the attached relative
patch, so feel free to take or leave them as you see fit. The second
call to cairo_set_source_rgb is currently unnecessary, because rsvg
forces the foreground color to black anyway, but that might change one
day (ideally, we'd use the frame foreground color, or even the current
face's foreground color; I have patches here that do this, though they
don't fix rsvg).

@@ -9619,15 +9619,19 @@ svg_load_image (struct frame *f, struct image
*img, char *contents,

   cairo_surface_t *surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24,
                              width, height);
+  if (cairo_surface_status (surface) != CAIRO_STATUS_SUCCESS)
+    goto rsvg_error;
   cairo_t *cr = cairo_create (surface);
   cairo_set_source_rgb (cr, background.red / 65535.0,
             background.green / 65535.0,
             background.blue / 65535.0);
   cairo_paint (cr);
+  cairo_set_source_rgb (cr, 0.0, 0.0, 0.0);
   rsvg_handle_render_cairo (rsvg_handle, cr);
   cairo_destroy (cr);
   g_object_unref (rsvg_handle);

+  cairo_surface_flush (surface);
   unsigned char *data = cairo_image_surface_get_data (surface);
   int stride = cairo_image_surface_get_stride (surface);
   for (int y = 0; y < height; ++y)
@@ -9636,9 +9640,9 @@ svg_load_image (struct frame *f, struct image
*img, char *contents,
       for (int x = 0; x < width; ++x)
     {
       guint32 rgb = *pixels++;
-      int red   = ((rgb >> 16) & 0xff) << 8;
-      int green = ((rgb >> 8) & 0xff) << 8;
-      int blue  = (rgb & 0xff) << 8;
+      int red   = ((rgb >> 16) & 0xff) * 0x101;
+      int green = ((rgb >> 8) & 0xff) * 0x101;
+      int blue  = (rgb & 0xff) * 0x101;
       PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
     }
       data += stride;

[-- Attachment #2: 0001-minor-things.patch --]
[-- Type: text/x-patch, Size: 1691 bytes --]

From 402dbd1e1ff5c33fe9061dd362d3b47a10ac5658 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Mon, 24 Jun 2019 16:20:19 +0000
Subject: [PATCH] minor things

---
 src/image.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/image.c b/src/image.c
index 924a188b4a..ce5534b07a 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9619,15 +9619,19 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 
   cairo_surface_t *surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24,
 							 width, height);
+  if (cairo_surface_status (surface) != CAIRO_STATUS_SUCCESS)
+    goto rsvg_error;
   cairo_t *cr = cairo_create (surface);
   cairo_set_source_rgb (cr, background.red / 65535.0,
 			background.green / 65535.0,
 			background.blue / 65535.0);
   cairo_paint (cr);
+  cairo_set_source_rgb (cr, 0.0, 0.0, 0.0);
   rsvg_handle_render_cairo (rsvg_handle, cr);
   cairo_destroy (cr);
   g_object_unref (rsvg_handle);
 
+  cairo_surface_flush (surface);
   unsigned char *data = cairo_image_surface_get_data (surface);
   int stride = cairo_image_surface_get_stride (surface);
   for (int y = 0; y < height; ++y)
@@ -9636,9 +9640,9 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
       for (int x = 0; x < width; ++x)
 	{
 	  guint32 rgb = *pixels++;
-	  int red   = ((rgb >> 16) & 0xff) << 8;
-	  int green = ((rgb >> 8) & 0xff) << 8;
-	  int blue  = (rgb & 0xff) << 8;
+	  int red   = ((rgb >> 16) & 0xff) * 0x101;
+	  int green = ((rgb >> 8) & 0xff) * 0x101;
+	  int blue  = (rgb & 0xff) * 0x101;
 	  PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
 	}
       data += stride;
-- 
2.20.1

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: 36315@debbugs.gnu.org
> 
> An alternative way would be to use rsvg_handle_render_cairo, which is
> recommended by librsvg, and let it blend with the background color.
> 
> Patch attached.  Note that this does not require --with-cairo.
> Raising the required version of librsvg to 2.14 is not a problem, as
> we are already using rsvg_handle_get_dimensions that requires that
> version.  Is Windows librsvg DLL compiled with libcairo?

librsvg on Windows is indeed build with libcairo, but if we don't get
the --with-cairo option, we don't probe for the necessary functions,
so at least theoretically we could have librsvg without Cairo.

The patch looks quite large.  Do we gain anything significant, apart
of the appraisal of librsvg developers?

I've built the patch on Windows (you forgot cairo_surface_destroy, so
I needed to add it), but the result is strange, or maybe I don't
understand what is expected.  I don't see any rectangle of color
#f00000, I see the entire frame with black background, and a few
characters in other colors.

Thanks.

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu]

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

On Tue, 25 Jun 2019 02:41:03 +0900,
Eli Zaretskii wrote:
> 
> librsvg on Windows is indeed build with libcairo, but if we don't get
> the --with-cairo option, we don't probe for the necessary functions,
> so at least theoretically we could have librsvg without Cairo.

I found librsvg actually requires libcairo, not optional.  Sorry for
my bogus question and thanks for testing the patch.

> The patch looks quite large.  Do we gain anything significant, apart
> of the appraisal of librsvg developers?

1. The current librsvg generates gdk-pixbuf via cairo image surface.
   So we can avoid unnecessarily intermediate data structure and
   roundtrip of alpha-component processing using cairo directly.
2. If configured --with-cairo, we can do further shortcut.  This is
   included in the patch attached to this mail.  Pip's patch is also
   reflected.
3. Image transformations can be applied when rendering to the cairo
   surface, not after generating bitmaps.  So we can take advantage of
   outline format and get better results of scaling.  This is not in
   the patch.  Probably it should be done by a separate commit after
   general image transformation code has been stabilized.

> I've built the patch on Windows (you forgot cairo_surface_destroy, so
> I needed to add it), but the result is strange, or maybe I don't
> understand what is expected.  I don't see any rectangle of color
> #f00000, I see the entire frame with black background, and a few
> characters in other colors.

When I tested Pip's test case, I started with emacs -Q -rv to avoid
text becomes invisible.  I could see a red rectangle on X11.  Do you
see such a rectangle without my patch?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

[-- Attachment #2: svg-cairo.diff --]
[-- Type: application/octet-stream, Size: 13462 bytes --]

diff --git a/src/image.c b/src/image.c
index 7b648c46ae9..956043a2219 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9379,17 +9379,20 @@ DEF_DLL_FN (void, rsvg_handle_get_dimensions,
 DEF_DLL_FN (gboolean, rsvg_handle_write,
 	    (RsvgHandle *, const guchar *, gsize, GError **));
 DEF_DLL_FN (gboolean, rsvg_handle_close, (RsvgHandle *, GError **));
-DEF_DLL_FN (GdkPixbuf *, rsvg_handle_get_pixbuf, (RsvgHandle *));
 DEF_DLL_FN (void, rsvg_handle_set_base_uri, (RsvgHandle *, const char *));
-
-DEF_DLL_FN (int, gdk_pixbuf_get_width, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_height, (const GdkPixbuf *));
-DEF_DLL_FN (guchar *, gdk_pixbuf_get_pixels, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_rowstride, (const GdkPixbuf *));
-DEF_DLL_FN (GdkColorspace, gdk_pixbuf_get_colorspace, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_n_channels, (const GdkPixbuf *));
-DEF_DLL_FN (gboolean, gdk_pixbuf_get_has_alpha, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_bits_per_sample, (const GdkPixbuf *));
+DEF_DLL_FN (gboolean, rsvg_handle_render_cairo, (RsvgHandle *, cairo_t *));
+
+DEF_DLL_FN (cairo_surface_t *, cairo_image_surface_create,
+	    (cairo_format_t, int, int));
+DEF_DLL_FN (cairo_status_t, cairo_surface_status, (cairo_surface_t *));
+DEF_DLL_FN (cairo_t *, cairo_create, (cairo_surface_t *));
+DEF_DLL_FN (void, cairo_set_source_rgb, (cairo_t *, double, double, double));
+DEF_DLL_FN (void, cairo_paint, (cairo_t *));
+DEF_DLL_FN (void, cairo_destroy, (cairo_t *));
+DEF_DLL_FN (void, cairo_surface_flush, (cairo_surface_t *));
+DEF_DLL_FN (unsigned char *, cairo_image_surface_get_data, (cairo_surface_t *));
+DEF_DLL_FN (int, cairo_image_surface_get_stride, (cairo_surface_t *));
+DEF_DLL_FN (void, cairo_surface_destroy, (cairo_surface_t *));
 
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
 DEF_DLL_FN (void, g_type_init, (void));
@@ -9400,14 +9403,14 @@ DEF_DLL_FN (void, g_clear_error, (GError **));
 static bool
 init_svg_functions (void)
 {
-  HMODULE library, gdklib = NULL, glib = NULL, gobject = NULL;
+  HMODULE library, cairo = NULL, glib = NULL, gobject = NULL;
 
-  if (!(glib = w32_delayed_load (Qglib))
+  if (!(cairo = w32_delayed_load (Qcairo))
+      || !(glib = w32_delayed_load (Qglib))
       || !(gobject = w32_delayed_load (Qgobject))
-      || !(gdklib = w32_delayed_load (Qgdk_pixbuf))
       || !(library = w32_delayed_load (Qsvg)))
     {
-      if (gdklib)  FreeLibrary (gdklib);
+      if (cairo)   FreeLibrary (cairo);
       if (gobject) FreeLibrary (gobject);
       if (glib)    FreeLibrary (glib);
       return 0;
@@ -9417,17 +9420,19 @@ init_svg_functions (void)
   LOAD_DLL_FN (library, rsvg_handle_get_dimensions);
   LOAD_DLL_FN (library, rsvg_handle_write);
   LOAD_DLL_FN (library, rsvg_handle_close);
-  LOAD_DLL_FN (library, rsvg_handle_get_pixbuf);
   LOAD_DLL_FN (library, rsvg_handle_set_base_uri);
-
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_width);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_height);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_pixels);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_rowstride);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_colorspace);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_n_channels);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_has_alpha);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_bits_per_sample);
+  LOAD_DLL_FN (library, rsvg_handle_render_cairo);
+
+  LOAD_DLL_FN (cairo, cairo_image_surface_create);
+  LOAD_DLL_FN (cairo, cairo_surface_status);
+  LOAD_DLL_FN (cairo, cairo_create);
+  LOAD_DLL_FN (cairo, cairo_set_source_rgb);
+  LOAD_DLL_FN (cairo, cairo_paint);
+  LOAD_DLL_FN (cairo, cairo_destroy);
+  LOAD_DLL_FN (cairo, cairo_surface_flush);
+  LOAD_DLL_FN (cairo, cairo_image_surface_get_data);
+  LOAD_DLL_FN (cairo, cairo_image_surface_get_stride);
+  LOAD_DLL_FN (cairo, cairo_surface_destroy);
 
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
   LOAD_DLL_FN (gobject, g_type_init);
@@ -9441,32 +9446,36 @@ init_svg_functions (void)
 /* The following aliases for library functions allow dynamic loading
    to be used on some platforms.  */
 
-#  undef gdk_pixbuf_get_bits_per_sample
-#  undef gdk_pixbuf_get_colorspace
-#  undef gdk_pixbuf_get_has_alpha
-#  undef gdk_pixbuf_get_height
-#  undef gdk_pixbuf_get_n_channels
-#  undef gdk_pixbuf_get_pixels
-#  undef gdk_pixbuf_get_rowstride
-#  undef gdk_pixbuf_get_width
+#  undef cairo_create
+#  undef cairo_destroy
+#  undef cairo_image_surface_create
+#  undef cairo_image_surface_get_data
+#  undef cairo_image_surface_get_stride
+#  undef cairo_paint
+#  undef cairo_set_source_rgb
+#  undef cairo_surface_destroy
+#  undef cairo_surface_flush
+#  undef cairo_surface_status
 #  undef g_clear_error
 #  undef g_object_unref
 #  undef g_type_init
 #  undef rsvg_handle_close
 #  undef rsvg_handle_get_dimensions
-#  undef rsvg_handle_get_pixbuf
 #  undef rsvg_handle_new
+#  undef rsvg_handle_render_cairo
 #  undef rsvg_handle_set_base_uri
 #  undef rsvg_handle_write
 
-#  define gdk_pixbuf_get_bits_per_sample fn_gdk_pixbuf_get_bits_per_sample
-#  define gdk_pixbuf_get_colorspace fn_gdk_pixbuf_get_colorspace
-#  define gdk_pixbuf_get_has_alpha fn_gdk_pixbuf_get_has_alpha
-#  define gdk_pixbuf_get_height fn_gdk_pixbuf_get_height
-#  define gdk_pixbuf_get_n_channels fn_gdk_pixbuf_get_n_channels
-#  define gdk_pixbuf_get_pixels fn_gdk_pixbuf_get_pixels
-#  define gdk_pixbuf_get_rowstride fn_gdk_pixbuf_get_rowstride
-#  define gdk_pixbuf_get_width fn_gdk_pixbuf_get_width
+#  define cairo_create fn_cairo_create
+#  define cairo_destroy fn_cairo_destroy
+#  define cairo_image_surface_create fn_cairo_image_surface_create
+#  define cairo_image_surface_get_data fn_cairo_image_surface_get_data
+#  define cairo_image_surface_get_stride fn_cairo_image_surface_get_stride
+#  define cairo_paint fn_cairo_paint
+#  define cairo_set_source_rgb fn_cairo_set_source_rgb
+#  define cairo_surface_destroy fn_cairo_surface_destroy
+#  define cairo_surface_flush fn_cairo_surface_flush
+#  define cairo_surface_status fn_cairo_surface_status
 #  define g_clear_error fn_g_clear_error
 #  define g_object_unref fn_g_object_unref
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
@@ -9474,8 +9483,8 @@ init_svg_functions (void)
 #  endif
 #  define rsvg_handle_close fn_rsvg_handle_close
 #  define rsvg_handle_get_dimensions fn_rsvg_handle_get_dimensions
-#  define rsvg_handle_get_pixbuf fn_rsvg_handle_get_pixbuf
 #  define rsvg_handle_new fn_rsvg_handle_new
+#  define rsvg_handle_render_cairo fn_rsvg_handle_render_cairo
 #  define rsvg_handle_set_base_uri fn_rsvg_handle_set_base_uri
 #  define rsvg_handle_write fn_rsvg_handle_write
 
@@ -9550,11 +9559,6 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   RsvgHandle *rsvg_handle;
   RsvgDimensionData dimension_data;
   GError *err = NULL;
-  GdkPixbuf *pixbuf;
-  int width;
-  int height;
-  const guint8 *pixels;
-  int rowstride;
 
 #if ! GLIB_CHECK_VERSION (2, 36, 0)
   /* g_type_init is a glib function that must be called prior to
@@ -9596,104 +9600,92 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   #endif
 
   rsvg_handle_get_dimensions (rsvg_handle, &dimension_data);
-  if (! check_image_size (f, dimension_data.width, dimension_data.height))
+  int width = dimension_data.width, height = dimension_data.height;
+  if (! check_image_size (f, width, height))
     {
       image_size_error ();
       goto rsvg_error;
     }
 
-  /* We can now get a valid pixel buffer from the svg file, if all
-     went ok.  */
-  pixbuf = rsvg_handle_get_pixbuf (rsvg_handle);
-  if (!pixbuf) goto rsvg_error;
-  g_object_unref (rsvg_handle);
-
-  /* Extract some meta data from the svg handle.  */
-  width     = gdk_pixbuf_get_width (pixbuf);
-  height    = gdk_pixbuf_get_height (pixbuf);
-  pixels    = gdk_pixbuf_get_pixels (pixbuf);
-  rowstride = gdk_pixbuf_get_rowstride (pixbuf);
-
-  /* Validate the svg meta data.  */
-  eassert (gdk_pixbuf_get_colorspace (pixbuf) == GDK_COLORSPACE_RGB);
-  eassert (gdk_pixbuf_get_n_channels (pixbuf) == 4);
-  eassert (gdk_pixbuf_get_has_alpha (pixbuf));
-  eassert (gdk_pixbuf_get_bits_per_sample (pixbuf) == 8);
+  /* Try to create a x pixmap to hold the svg pixmap.  */
+  Emacs_Pix_Container ximg;
+  if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
+    {
+      g_object_unref (rsvg_handle);
+      return 0;
+    }
 
-  {
-    /* Try to create a x pixmap to hold the svg pixmap.  */
-    Emacs_Pix_Container ximg;
-    if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
-      {
-	g_object_unref (pixbuf);
-	return 0;
-      }
+  init_color_table ();
 
-    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);
 
-    /* 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
-       pixel, and the image background color.  */
-    for (int y = 0; y < height; ++y)
-      {
-	for (int x = 0; x < width; ++x)
-	  {
-	    int red     = *pixels++;
-	    int green   = *pixels++;
-	    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));
-	  }
+  cairo_surface_t *surface;
+#ifdef USE_CAIRO
+  surface = cairo_image_surface_create_for_data ((unsigned char *) ximg->data,
+						 CAIRO_FORMAT_RGB24,
+						 width, height,
+						 ximg->bytes_per_line);
+#else
+  surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24, width, height);
+#endif
+  if (cairo_surface_status (surface) != CAIRO_STATUS_SUCCESS)
+    goto rsvg_error;
+  cairo_t *cr = cairo_create (surface);
+  cairo_set_source_rgb (cr, background.red / 65535.0,
+			background.green / 65535.0,
+			background.blue / 65535.0);
+  cairo_paint (cr);
+  cairo_set_source_rgb (cr, 0.0, 0.0, 0.0);
+  rsvg_handle_render_cairo (rsvg_handle, cr);
+  cairo_destroy (cr);
+  g_object_unref (rsvg_handle);
 
-	pixels += rowstride - 4 * width;
-      }
+  cairo_surface_flush (surface);
+#ifndef USE_CAIRO
+  unsigned char *data = cairo_image_surface_get_data (surface);
+  int stride = cairo_image_surface_get_stride (surface);
+  for (int y = 0; y < height; ++y)
+    {
+      guint32 *pixels = (guint32 *) data;
+      for (int x = 0; x < width; ++x)
+	{
+	  guint32 rgb = *pixels++;
+	  int red   = ((rgb >> 16) & 0xff) * 0x101;
+	  int green = ((rgb >> 8) & 0xff) * 0x101;
+	  int blue  = (rgb & 0xff) * 0x101;
+	  PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
+	}
+      data += stride;
+    }
+#endif	/* !USE_CAIRO */
 
 #ifdef COLOR_TABLE_SUPPORT
-    /* Remember colors allocated for this image.  */
-    img->colors = colors_in_color_table (&img->ncolors);
-    free_color_table ();
+  /* Remember colors allocated for this image.  */
+  img->colors = colors_in_color_table (&img->ncolors);
+  free_color_table ();
 #endif /* COLOR_TABLE_SUPPORT */
 
-    g_object_unref (pixbuf);
+  cairo_surface_destroy (surface);
 
-    img->width  = width;
-    img->height = height;
+  img->width  = width;
+  img->height = height;
 
-    /* Maybe fill in the background field while we have ximg handy.
-       Casting avoids a GCC warning.  */
-    IMAGE_BACKGROUND (img, f, (Emacs_Pix_Context)ximg);
+  /* Maybe fill in the background field while we have ximg handy.
+     Casting avoids a GCC warning.  */
+  IMAGE_BACKGROUND (img, f, (Emacs_Pix_Context)ximg);
 
-    /* Put ximg into the image.  */
-    image_put_x_image (f, img, ximg, 0);
-  }
+  /* Put ximg into the image.  */
+  image_put_x_image (f, img, ximg, 0);
 
   return 1;
 
@@ -10249,7 +10241,7 @@ non-numeric, there is no explicit limit on the size of images.  */);
   add_image_type (Qsvg);
 #ifdef HAVE_NTGUI
   /* Other libraries used directly by svg code.  */
-  DEFSYM (Qgdk_pixbuf, "gdk-pixbuf");
+  DEFSYM (Qcairo, "cairo");
   DEFSYM (Qglib, "glib");
   DEFSYM (Qgobject, "gobject");
 #endif /* HAVE_NTGUI  */

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> Date: Tue, 25 Jun 2019 08:06:23 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: pipcet@gmail.com,
> 	36315@debbugs.gnu.org
> 
> > The patch looks quite large.  Do we gain anything significant, apart
> > of the appraisal of librsvg developers?
> 
> 1. The current librsvg generates gdk-pixbuf via cairo image surface.
>    So we can avoid unnecessarily intermediate data structure and
>    roundtrip of alpha-component processing using cairo directly.
> 2. If configured --with-cairo, we can do further shortcut.  This is
>    included in the patch attached to this mail.  Pip's patch is also
>    reflected.
> 3. Image transformations can be applied when rendering to the cairo
>    surface, not after generating bitmaps.  So we can take advantage of
>    outline format and get better results of scaling.  This is not in
>    the patch.  Probably it should be done by a separate commit after
>    general image transformation code has been stabilized.

Maybe it's just me, but I'm uneasy to bypass librsvg and call Cairo
directly for manipulating SVG images.  Why doesn't librsvg provide a
way to do this via its own APIs?

Does anyone else think it's unusual to make such direct calls to what
is essentially a lower-level library?

> > I've built the patch on Windows (you forgot cairo_surface_destroy, so
> > I needed to add it), but the result is strange, or maybe I don't
> > understand what is expected.  I don't see any rectangle of color
> > #f00000, I see the entire frame with black background, and a few
> > characters in other colors.
> 
> When I tested Pip's test case, I started with emacs -Q -rv to avoid
> text becomes invisible.  I could see a red rectangle on X11.  Do you
> see such a rectangle without my patch?

Yes, I see an orange rectangle (a square, actually, I think).

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Alan Third]

On Tue, Jun 25, 2019 at 07:54:52PM +0300, Eli Zaretskii wrote:
> > 3. Image transformations can be applied when rendering to the cairo
> >    surface, not after generating bitmaps.  So we can take advantage of
> >    outline format and get better results of scaling.  This is not in
> >    the patch.  Probably it should be done by a separate commit after
> >    general image transformation code has been stabilized.
> 
> Maybe it's just me, but I'm uneasy to bypass librsvg and call Cairo
> directly for manipulating SVG images.  Why doesn't librsvg provide a
> way to do this via its own APIs?
> 
> Does anyone else think it's unusual to make such direct calls to what
> is essentially a lower-level library?

Librsvg doesn’t provide any way to modify the images. It loads them
into either ‘GIO’ format (whatever that is), Cairo, or a pixel buffer.
There are functions for use with a GDKPixBuf that allow scaling, but
they’re now deprecated in favour of using Cairo.
-- 
Alan Third

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> Date: Tue, 25 Jun 2019 19:44:28 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>,
> 	36315@debbugs.gnu.org, pipcet@gmail.com
> 
> > Maybe it's just me, but I'm uneasy to bypass librsvg and call Cairo
> > directly for manipulating SVG images.  Why doesn't librsvg provide a
> > way to do this via its own APIs?
> > 
> > Does anyone else think it's unusual to make such direct calls to what
> > is essentially a lower-level library?
> 
> Librsvg doesn’t provide any way to modify the images.

Yes, but neither do any of the other image libraries, right?

The issue at hand, I believe, had to do with colors that weren't
right.  Image transformations are a separate issue, and we already
know how to do that for SVG and for other image types.

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu]

On Wed, 26 Jun 2019 01:54:52 +0900,
Eli Zaretskii wrote:
> 
> > Date: Tue, 25 Jun 2019 08:06:23 +0900
> > From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > Cc: pipcet@gmail.com,
> > 	36315@debbugs.gnu.org
> > 
> > > The patch looks quite large.  Do we gain anything significant, apart
> > > of the appraisal of librsvg developers?
> > 
> > 1. The current librsvg generates gdk-pixbuf via cairo image surface.
> >    So we can avoid unnecessarily intermediate data structure and
> >    roundtrip of alpha-component processing using cairo directly.
> > 2. If configured --with-cairo, we can do further shortcut.  This is
> >    included in the patch attached to this mail.  Pip's patch is also
> >    reflected.
> > 3. Image transformations can be applied when rendering to the cairo
> >    surface, not after generating bitmaps.  So we can take advantage of
> >    outline format and get better results of scaling.  This is not in
> >    the patch.  Probably it should be done by a separate commit after
> >    general image transformation code has been stabilized.
> 
> Maybe it's just me, but I'm uneasy to bypass librsvg and call Cairo
> directly for manipulating SVG images.  Why doesn't librsvg provide a
> way to do this via its own APIs?
>
> Does anyone else think it's unusual to make such direct calls to what
> is essentially a lower-level library?

What kind of operations do you think librsvg should provide us with,
instead of letting us use cairo?

BTW, GTK 4 is deemphasizing GdkPixbuf:

  https://developer.gnome.org/gtk4/stable/ch29s02.html#id-1.6.4.4.33

I don't think GdkPixbuf is dropped from GTK/GDK soon, but we don't
have any particular reasons to stick to it for rendering SVG images.

> > > I've built the patch on Windows (you forgot cairo_surface_destroy, so
> > > I needed to add it), but the result is strange, or maybe I don't
> > > understand what is expected.  I don't see any rectangle of color
> > > #f00000, I see the entire frame with black background, and a few
> > > characters in other colors.
> > 
> > When I tested Pip's test case, I started with emacs -Q -rv to avoid
> > text becomes invisible.  I could see a red rectangle on X11.  Do you
> > see such a rectangle without my patch?
> 
> Yes, I see an orange rectangle (a square, actually, I think).

If the square is not displayed with my patch, then there is a bug in
it.  I've sent 3 versions and the first one was wrong.  Please try
again with the latest one in my previous mail:

  https://debbugs.gnu.org/cgi/bugreport.cgi?att=1;msg=26;bug=36315;filename=svg-cairo.diff

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Lars Ingebrigtsen]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> What kind of operations do you think librsvg should provide us with,
> instead of letting us use cairo?

I know nothing about cairo, but last time I was playing with font stuff
in SVGs, it seemed like librsvg had very good support for advanced
ligature stuff, while other SVG libraries didn't...  how is cairo on
that front?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> Date: Wed, 26 Jun 2019 08:48:25 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: 	pipcet@gmail.com,
> 	36315@debbugs.gnu.org
> 
> > Maybe it's just me, but I'm uneasy to bypass librsvg and call Cairo
> > directly for manipulating SVG images.  Why doesn't librsvg provide a
> > way to do this via its own APIs?
> >
> > Does anyone else think it's unusual to make such direct calls to what
> > is essentially a lower-level library?
> 
> What kind of operations do you think librsvg should provide us with,
> instead of letting us use cairo?

Those for which you called the Cairo functions directly.

> > > > I've built the patch on Windows (you forgot cairo_surface_destroy, so
> > > > I needed to add it), but the result is strange, or maybe I don't
> > > > understand what is expected.  I don't see any rectangle of color
> > > > #f00000, I see the entire frame with black background, and a few
> > > > characters in other colors.
> > > 
> > > When I tested Pip's test case, I started with emacs -Q -rv to avoid
> > > text becomes invisible.  I could see a red rectangle on X11.  Do you
> > > see such a rectangle without my patch?
> > 
> > Yes, I see an orange rectangle (a square, actually, I think).
> 
> If the square is not displayed with my patch, then there is a bug in
> it.  I've sent 3 versions and the first one was wrong.  Please try
> again with the latest one in my previous mail:
> 
>   https://debbugs.gnu.org/cgi/bugreport.cgi?att=1;msg=26;bug=36315;filename=svg-cairo.diff

I tried with the second patch.  I tried now again with the above one:
still no rectangle.

Thanks.

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu]

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

On Thu, 27 Jun 2019 00:57:48 +0900,
Eli Zaretskii wrote:
> 
> > > Maybe it's just me, but I'm uneasy to bypass librsvg and call Cairo
> > > directly for manipulating SVG images.  Why doesn't librsvg provide a
> > > way to do this via its own APIs?
> > >
> > > Does anyone else think it's unusual to make such direct calls to what
> > > is essentially a lower-level library?
> > 
> > What kind of operations do you think librsvg should provide us with,
> > instead of letting us use cairo?
> 
> Those for which you called the Cairo functions directly.

Which one, concretely?  Or you mean something in other parts?

+  cairo_surface_t *surface;
+#ifdef USE_CAIRO
+  surface = cairo_image_surface_create_for_data ((unsigned char *) ximg->data,
+						 CAIRO_FORMAT_RGB24,
+						 width, height,
+						 ximg->bytes_per_line);
+#else
+  surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24, width, height);
+#endif
+  if (cairo_surface_status (surface) != CAIRO_STATUS_SUCCESS)
+    goto rsvg_error;
+  cairo_t *cr = cairo_create (surface);
+  cairo_set_source_rgb (cr, background.red / 65535.0,
+			background.green / 65535.0,
+			background.blue / 65535.0);
+  cairo_paint (cr);
+  cairo_set_source_rgb (cr, 0.0, 0.0, 0.0);
+  rsvg_handle_render_cairo (rsvg_handle, cr);
+  cairo_destroy (cr);
+  g_object_unref (rsvg_handle);

> I tried with the second patch.  I tried now again with the above one:
> still no rectangle.

Seems like a problem in DLL loading.  Please try the attached one.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

[-- Attachment #2: svg-cairo.diff --]
[-- Type: application/octet-stream, Size: 14916 bytes --]

diff --git a/configure.ac b/configure.ac
index 8ff0e21fbf6..bcd70d34cbb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2603,7 +2603,7 @@ AC_DEFUN
 HAVE_RSVG=no
 if test "${HAVE_X11}" = "yes" || test "${HAVE_NS}" = "yes" || test "${opsys}" = "mingw32"; then
   if test "${with_rsvg}" != "no"; then
-    RSVG_REQUIRED=2.11.0
+    RSVG_REQUIRED=2.14.0
     RSVG_MODULE="librsvg-2.0 >= $RSVG_REQUIRED"
 
     EMACS_CHECK_MODULES([RSVG], [$RSVG_MODULE])
diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index fef954eb7a3..285ca5d6967 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -3082,7 +3082,7 @@ Dynamic Libraries
         (tiff "libtiff3.dll" "libtiff.dll")
         (gif "giflib4.dll" "libungif4.dll" "libungif.dll")
         (svg "librsvg-2-2.dll")
-        (gdk-pixbuf "libgdk_pixbuf-2.0-0.dll")
+        (cairo "libcairo-2.dll")
         (glib "libglib-2.0-0.dll")
         (gobject "libgobject-2.0-0.dll")))
 @end example
diff --git a/lisp/term/w32-win.el b/lisp/term/w32-win.el
index 044b82ed1e0..270b062e75a 100644
--- a/lisp/term/w32-win.el
+++ b/lisp/term/w32-win.el
@@ -277,7 +277,7 @@ libgnutls-version
 	     '(gif "libgif-6.dll" "giflib5.dll" "gif.dll")
 	 '(gif "libgif-5.dll" "giflib4.dll" "libungif4.dll" "libungif.dll")))
        '(svg "librsvg-2-2.dll")
-       '(gdk-pixbuf "libgdk_pixbuf-2.0-0.dll")
+       '(cairo "libcairo-2.dll")
        '(glib "libglib-2.0-0.dll")
        '(gobject "libgobject-2.0-0.dll")
        (if (>= libgnutls-version 30400)
diff --git a/src/image.c b/src/image.c
index 7b648c46ae9..d56dd998908 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9379,17 +9379,20 @@ DEF_DLL_FN (void, rsvg_handle_get_dimensions,
 DEF_DLL_FN (gboolean, rsvg_handle_write,
 	    (RsvgHandle *, const guchar *, gsize, GError **));
 DEF_DLL_FN (gboolean, rsvg_handle_close, (RsvgHandle *, GError **));
-DEF_DLL_FN (GdkPixbuf *, rsvg_handle_get_pixbuf, (RsvgHandle *));
 DEF_DLL_FN (void, rsvg_handle_set_base_uri, (RsvgHandle *, const char *));
-
-DEF_DLL_FN (int, gdk_pixbuf_get_width, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_height, (const GdkPixbuf *));
-DEF_DLL_FN (guchar *, gdk_pixbuf_get_pixels, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_rowstride, (const GdkPixbuf *));
-DEF_DLL_FN (GdkColorspace, gdk_pixbuf_get_colorspace, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_n_channels, (const GdkPixbuf *));
-DEF_DLL_FN (gboolean, gdk_pixbuf_get_has_alpha, (const GdkPixbuf *));
-DEF_DLL_FN (int, gdk_pixbuf_get_bits_per_sample, (const GdkPixbuf *));
+DEF_DLL_FN (gboolean, rsvg_handle_render_cairo, (RsvgHandle *, cairo_t *));
+
+DEF_DLL_FN (cairo_surface_t *, cairo_image_surface_create,
+	    (cairo_format_t, int, int));
+DEF_DLL_FN (cairo_status_t, cairo_surface_status, (cairo_surface_t *));
+DEF_DLL_FN (cairo_t *, cairo_create, (cairo_surface_t *));
+DEF_DLL_FN (void, cairo_set_source_rgb, (cairo_t *, double, double, double));
+DEF_DLL_FN (void, cairo_paint, (cairo_t *));
+DEF_DLL_FN (void, cairo_destroy, (cairo_t *));
+DEF_DLL_FN (void, cairo_surface_flush, (cairo_surface_t *));
+DEF_DLL_FN (unsigned char *, cairo_image_surface_get_data, (cairo_surface_t *));
+DEF_DLL_FN (int, cairo_image_surface_get_stride, (cairo_surface_t *));
+DEF_DLL_FN (void, cairo_surface_destroy, (cairo_surface_t *));
 
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
 DEF_DLL_FN (void, g_type_init, (void));
@@ -9400,14 +9403,14 @@ DEF_DLL_FN (void, g_clear_error, (GError **));
 static bool
 init_svg_functions (void)
 {
-  HMODULE library, gdklib = NULL, glib = NULL, gobject = NULL;
+  HMODULE library, cairo = NULL, glib = NULL, gobject = NULL;
 
   if (!(glib = w32_delayed_load (Qglib))
       || !(gobject = w32_delayed_load (Qgobject))
-      || !(gdklib = w32_delayed_load (Qgdk_pixbuf))
+      || !(cairo = w32_delayed_load (Qcairo))
       || !(library = w32_delayed_load (Qsvg)))
     {
-      if (gdklib)  FreeLibrary (gdklib);
+      if (cairo)   FreeLibrary (cairo);
       if (gobject) FreeLibrary (gobject);
       if (glib)    FreeLibrary (glib);
       return 0;
@@ -9417,17 +9420,19 @@ init_svg_functions (void)
   LOAD_DLL_FN (library, rsvg_handle_get_dimensions);
   LOAD_DLL_FN (library, rsvg_handle_write);
   LOAD_DLL_FN (library, rsvg_handle_close);
-  LOAD_DLL_FN (library, rsvg_handle_get_pixbuf);
   LOAD_DLL_FN (library, rsvg_handle_set_base_uri);
-
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_width);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_height);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_pixels);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_rowstride);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_colorspace);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_n_channels);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_has_alpha);
-  LOAD_DLL_FN (gdklib, gdk_pixbuf_get_bits_per_sample);
+  LOAD_DLL_FN (library, rsvg_handle_render_cairo);
+
+  LOAD_DLL_FN (cairo, cairo_image_surface_create);
+  LOAD_DLL_FN (cairo, cairo_surface_status);
+  LOAD_DLL_FN (cairo, cairo_create);
+  LOAD_DLL_FN (cairo, cairo_set_source_rgb);
+  LOAD_DLL_FN (cairo, cairo_paint);
+  LOAD_DLL_FN (cairo, cairo_destroy);
+  LOAD_DLL_FN (cairo, cairo_surface_flush);
+  LOAD_DLL_FN (cairo, cairo_image_surface_get_data);
+  LOAD_DLL_FN (cairo, cairo_image_surface_get_stride);
+  LOAD_DLL_FN (cairo, cairo_surface_destroy);
 
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
   LOAD_DLL_FN (gobject, g_type_init);
@@ -9441,32 +9446,36 @@ init_svg_functions (void)
 /* The following aliases for library functions allow dynamic loading
    to be used on some platforms.  */
 
-#  undef gdk_pixbuf_get_bits_per_sample
-#  undef gdk_pixbuf_get_colorspace
-#  undef gdk_pixbuf_get_has_alpha
-#  undef gdk_pixbuf_get_height
-#  undef gdk_pixbuf_get_n_channels
-#  undef gdk_pixbuf_get_pixels
-#  undef gdk_pixbuf_get_rowstride
-#  undef gdk_pixbuf_get_width
+#  undef cairo_create
+#  undef cairo_destroy
+#  undef cairo_image_surface_create
+#  undef cairo_image_surface_get_data
+#  undef cairo_image_surface_get_stride
+#  undef cairo_paint
+#  undef cairo_set_source_rgb
+#  undef cairo_surface_destroy
+#  undef cairo_surface_flush
+#  undef cairo_surface_status
 #  undef g_clear_error
 #  undef g_object_unref
 #  undef g_type_init
 #  undef rsvg_handle_close
 #  undef rsvg_handle_get_dimensions
-#  undef rsvg_handle_get_pixbuf
 #  undef rsvg_handle_new
+#  undef rsvg_handle_render_cairo
 #  undef rsvg_handle_set_base_uri
 #  undef rsvg_handle_write
 
-#  define gdk_pixbuf_get_bits_per_sample fn_gdk_pixbuf_get_bits_per_sample
-#  define gdk_pixbuf_get_colorspace fn_gdk_pixbuf_get_colorspace
-#  define gdk_pixbuf_get_has_alpha fn_gdk_pixbuf_get_has_alpha
-#  define gdk_pixbuf_get_height fn_gdk_pixbuf_get_height
-#  define gdk_pixbuf_get_n_channels fn_gdk_pixbuf_get_n_channels
-#  define gdk_pixbuf_get_pixels fn_gdk_pixbuf_get_pixels
-#  define gdk_pixbuf_get_rowstride fn_gdk_pixbuf_get_rowstride
-#  define gdk_pixbuf_get_width fn_gdk_pixbuf_get_width
+#  define cairo_create fn_cairo_create
+#  define cairo_destroy fn_cairo_destroy
+#  define cairo_image_surface_create fn_cairo_image_surface_create
+#  define cairo_image_surface_get_data fn_cairo_image_surface_get_data
+#  define cairo_image_surface_get_stride fn_cairo_image_surface_get_stride
+#  define cairo_paint fn_cairo_paint
+#  define cairo_set_source_rgb fn_cairo_set_source_rgb
+#  define cairo_surface_destroy fn_cairo_surface_destroy
+#  define cairo_surface_flush fn_cairo_surface_flush
+#  define cairo_surface_status fn_cairo_surface_status
 #  define g_clear_error fn_g_clear_error
 #  define g_object_unref fn_g_object_unref
 #  if ! GLIB_CHECK_VERSION (2, 36, 0)
@@ -9474,8 +9483,8 @@ init_svg_functions (void)
 #  endif
 #  define rsvg_handle_close fn_rsvg_handle_close
 #  define rsvg_handle_get_dimensions fn_rsvg_handle_get_dimensions
-#  define rsvg_handle_get_pixbuf fn_rsvg_handle_get_pixbuf
 #  define rsvg_handle_new fn_rsvg_handle_new
+#  define rsvg_handle_render_cairo fn_rsvg_handle_render_cairo
 #  define rsvg_handle_set_base_uri fn_rsvg_handle_set_base_uri
 #  define rsvg_handle_write fn_rsvg_handle_write
 
@@ -9550,11 +9559,6 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   RsvgHandle *rsvg_handle;
   RsvgDimensionData dimension_data;
   GError *err = NULL;
-  GdkPixbuf *pixbuf;
-  int width;
-  int height;
-  const guint8 *pixels;
-  int rowstride;
 
 #if ! GLIB_CHECK_VERSION (2, 36, 0)
   /* g_type_init is a glib function that must be called prior to
@@ -9596,104 +9600,92 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   #endif
 
   rsvg_handle_get_dimensions (rsvg_handle, &dimension_data);
-  if (! check_image_size (f, dimension_data.width, dimension_data.height))
+  int width = dimension_data.width, height = dimension_data.height;
+  if (! check_image_size (f, width, height))
     {
       image_size_error ();
       goto rsvg_error;
     }
 
-  /* We can now get a valid pixel buffer from the svg file, if all
-     went ok.  */
-  pixbuf = rsvg_handle_get_pixbuf (rsvg_handle);
-  if (!pixbuf) goto rsvg_error;
-  g_object_unref (rsvg_handle);
-
-  /* Extract some meta data from the svg handle.  */
-  width     = gdk_pixbuf_get_width (pixbuf);
-  height    = gdk_pixbuf_get_height (pixbuf);
-  pixels    = gdk_pixbuf_get_pixels (pixbuf);
-  rowstride = gdk_pixbuf_get_rowstride (pixbuf);
-
-  /* Validate the svg meta data.  */
-  eassert (gdk_pixbuf_get_colorspace (pixbuf) == GDK_COLORSPACE_RGB);
-  eassert (gdk_pixbuf_get_n_channels (pixbuf) == 4);
-  eassert (gdk_pixbuf_get_has_alpha (pixbuf));
-  eassert (gdk_pixbuf_get_bits_per_sample (pixbuf) == 8);
+  /* Try to create a x pixmap to hold the svg pixmap.  */
+  Emacs_Pix_Container ximg;
+  if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
+    {
+      g_object_unref (rsvg_handle);
+      return 0;
+    }
 
-  {
-    /* Try to create a x pixmap to hold the svg pixmap.  */
-    Emacs_Pix_Container ximg;
-    if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
-      {
-	g_object_unref (pixbuf);
-	return 0;
-      }
+  init_color_table ();
 
-    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);
 
-    /* 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
-       pixel, and the image background color.  */
-    for (int y = 0; y < height; ++y)
-      {
-	for (int x = 0; x < width; ++x)
-	  {
-	    int red     = *pixels++;
-	    int green   = *pixels++;
-	    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));
-	  }
+  cairo_surface_t *surface;
+#ifdef USE_CAIRO
+  surface = cairo_image_surface_create_for_data ((unsigned char *) ximg->data,
+						 CAIRO_FORMAT_RGB24,
+						 width, height,
+						 ximg->bytes_per_line);
+#else
+  surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24, width, height);
+#endif
+  if (cairo_surface_status (surface) != CAIRO_STATUS_SUCCESS)
+    goto rsvg_error;
+  cairo_t *cr = cairo_create (surface);
+  cairo_set_source_rgb (cr, background.red / 65535.0,
+			background.green / 65535.0,
+			background.blue / 65535.0);
+  cairo_paint (cr);
+  cairo_set_source_rgb (cr, 0.0, 0.0, 0.0);
+  rsvg_handle_render_cairo (rsvg_handle, cr);
+  cairo_destroy (cr);
+  g_object_unref (rsvg_handle);
 
-	pixels += rowstride - 4 * width;
-      }
+  cairo_surface_flush (surface);
+#ifndef USE_CAIRO
+  unsigned char *data = cairo_image_surface_get_data (surface);
+  int stride = cairo_image_surface_get_stride (surface);
+  for (int y = 0; y < height; ++y)
+    {
+      guint32 *pixels = (guint32 *) data;
+      for (int x = 0; x < width; ++x)
+	{
+	  guint32 rgb = *pixels++;
+	  int red   = ((rgb >> 16) & 0xff) * 0x101;
+	  int green = ((rgb >> 8) & 0xff) * 0x101;
+	  int blue  = (rgb & 0xff) * 0x101;
+	  PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
+	}
+      data += stride;
+    }
+#endif	/* !USE_CAIRO */
 
 #ifdef COLOR_TABLE_SUPPORT
-    /* Remember colors allocated for this image.  */
-    img->colors = colors_in_color_table (&img->ncolors);
-    free_color_table ();
+  /* Remember colors allocated for this image.  */
+  img->colors = colors_in_color_table (&img->ncolors);
+  free_color_table ();
 #endif /* COLOR_TABLE_SUPPORT */
 
-    g_object_unref (pixbuf);
+  cairo_surface_destroy (surface);
 
-    img->width  = width;
-    img->height = height;
+  img->width  = width;
+  img->height = height;
 
-    /* Maybe fill in the background field while we have ximg handy.
-       Casting avoids a GCC warning.  */
-    IMAGE_BACKGROUND (img, f, (Emacs_Pix_Context)ximg);
+  /* Maybe fill in the background field while we have ximg handy.
+     Casting avoids a GCC warning.  */
+  IMAGE_BACKGROUND (img, f, (Emacs_Pix_Context)ximg);
 
-    /* Put ximg into the image.  */
-    image_put_x_image (f, img, ximg, 0);
-  }
+  /* Put ximg into the image.  */
+  image_put_x_image (f, img, ximg, 0);
 
   return 1;
 
@@ -10249,7 +10241,7 @@ non-numeric, there is no explicit limit on the size of images.  */);
   add_image_type (Qsvg);
 #ifdef HAVE_NTGUI
   /* Other libraries used directly by svg code.  */
-  DEFSYM (Qgdk_pixbuf, "gdk-pixbuf");
+  DEFSYM (Qcairo, "cairo");
   DEFSYM (Qglib, "glib");
   DEFSYM (Qgobject, "gobject");
 #endif /* HAVE_NTGUI  */

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> Date: Thu, 27 Jun 2019 12:33:46 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: pipcet@gmail.com,
> 	36315@debbugs.gnu.org
> 
> > > What kind of operations do you think librsvg should provide us with,
> > > instead of letting us use cairo?
> > 
> > Those for which you called the Cairo functions directly.
> 
> Which one, concretely?  Or you mean something in other parts?

All of the Cairo functions you called:

   cairo_create
   cairo_destroy
   cairo_image_surface_create
   cairo_image_surface_get_data
   cairo_image_surface_get_stride
   cairo_paint
   cairo_set_source_rgb
   cairo_surface_destroy
   cairo_surface_flush
   cairo_surface_status

> > I tried with the second patch.  I tried now again with the above one:
> > still no rectangle.
> 
> Seems like a problem in DLL loading.  Please try the attached one.

This works, but I don't think I see any difference in the color of the
rectangle wrt what I see in Emacs 26, i.e. without the patch.

Thanks.

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu]

On Thu, 27 Jun 2019 23:16:07 +0900,
Eli Zaretskii wrote:
> 
> > Date: Thu, 27 Jun 2019 12:33:46 +0900
> > From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > Cc: pipcet@gmail.com,
> > 	36315@debbugs.gnu.org
> > 
> > > > What kind of operations do you think librsvg should provide us with,
> > > > instead of letting us use cairo?
> > > 
> > > Those for which you called the Cairo functions directly.
> > 
> > Which one, concretely?  Or you mean something in other parts?
> 
> All of the Cairo functions you called:
> 
>    cairo_create
>    cairo_destroy
>    cairo_image_surface_create
>    cairo_image_surface_get_data
>    cairo_image_surface_get_stride
>    cairo_paint
>    cairo_set_source_rgb
>    cairo_surface_destroy
>    cairo_surface_flush
>    cairo_surface_status

Why do you think so?  Librsvg does not provide us with any further
abstractions over the cairo data structures.  Also, the first
paragraph of README.md in the source code says:

  This is librsvg - A small library to render Scalable Vector Graphics
  (SVG), associated with the GNOME Project.  It renders SVG files to
  Cairo surfaces.  Cairo is the 2D, antialiased drawing library that
  GNOME uses to draw things to the screen or to generate output for
  printing.

In librsvg, only 2 functions deal with the cairo data structures.
Librsvg is, at least except its very early versions, clearly designed
so it is used together with cairo.

> > > I tried with the second patch.  I tried now again with the above one:
> > > still no rectangle.
> > 
> > Seems like a problem in DLL loading.  Please try the attached one.
> 
> This works, but I don't think I see any difference in the color of the
> rectangle wrt what I see in Emacs 26, i.e. without the patch.

Usually the sense of sight by human cannot see the difference between
the colors #ef0000 and #f00000.  You would need some color picker to
tell the difference.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> Date: Sun, 30 Jun 2019 15:12:18 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: pipcet@gmail.com,
> 	36315@debbugs.gnu.org
> 
> > > > > What kind of operations do you think librsvg should provide us with,
> > > > > instead of letting us use cairo?
> > > > 
> > > > Those for which you called the Cairo functions directly.
> > > 
> > > Which one, concretely?  Or you mean something in other parts?
> > 
> > All of the Cairo functions you called:
> > 
> >    cairo_create
> >    cairo_destroy
> >    cairo_image_surface_create
> >    cairo_image_surface_get_data
> >    cairo_image_surface_get_stride
> >    cairo_paint
> >    cairo_set_source_rgb
> >    cairo_surface_destroy
> >    cairo_surface_flush
> >    cairo_surface_status
> 
> Why do you think so?  Librsvg does not provide us with any further
> abstractions over the cairo data structures.

It just looks like we are using libcairo and not librsvg.

Again, it isn't something entirely rational, it just sounds weird to
me.  Imagine that users libxml2 would need to call libiconv to decode
UTF-8 encoded text in an XML file, for example.  Doesn't look right.

> > This works, but I don't think I see any difference in the color of the
> > rectangle wrt what I see in Emacs 26, i.e. without the patch.
> 
> Usually the sense of sight by human cannot see the difference between
> the colors #ef0000 and #f00000.  You would need some color picker to
> tell the difference.

OK, then please ignore that remark.

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu]

On Sun, 30 Jun 2019 23:26:51 +0900,
Eli Zaretskii wrote:
> 
> > Date: Sun, 30 Jun 2019 15:12:18 +0900
> > From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > Cc: pipcet@gmail.com,
> > 	36315@debbugs.gnu.org
> > 
> > > > > > What kind of operations do you think librsvg should provide us with,
> > > > > > instead of letting us use cairo?
> > > > > 
> > > > > Those for which you called the Cairo functions directly.
> > > > 
> > > > Which one, concretely?  Or you mean something in other parts?
> > > 
> > > All of the Cairo functions you called:
> > > 
> > >    cairo_create
> > >    cairo_destroy
> > >    cairo_image_surface_create
> > >    cairo_image_surface_get_data
> > >    cairo_image_surface_get_stride
> > >    cairo_paint
> > >    cairo_set_source_rgb
> > >    cairo_surface_destroy
> > >    cairo_surface_flush
> > >    cairo_surface_status
> > 
> > Why do you think so?  Librsvg does not provide us with any further
> > abstractions over the cairo data structures.
> 
> It just looks like we are using libcairo and not librsvg.
>
> Again, it isn't something entirely rational, it just sounds weird to
> me.  Imagine that users libxml2 would need to call libiconv to decode
> UTF-8 encoded text in an XML file, for example.  Doesn't look right.

The situation for libcairo and librsvg should be familiar to us: we
are directly using Emacs core functionality even when working with
several major or minor modes.

Anyway, this is the librsvg design we cannot change here.  The
situation for another SVG rendering library resvg
(https://github.com/RazrFalcon/resvg), which supports multiple
backends, looks similar in the above respect at first glance.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> Date: Mon, 01 Jul 2019 12:46:55 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: pipcet@gmail.com,
> 	36315@debbugs.gnu.org
> 
> > It just looks like we are using libcairo and not librsvg.
> >
> > Again, it isn't something entirely rational, it just sounds weird to
> > me.  Imagine that users libxml2 would need to call libiconv to decode
> > UTF-8 encoded text in an XML file, for example.  Doesn't look right.
> 
> The situation for libcairo and librsvg should be familiar to us: we
> are directly using Emacs core functionality even when working with
> several major or minor modes.

Not sure what this alludes to.

One thing that bothers me with using sub-libraries is that we now need
another entry in dynamic-library-alist, which means complications if
Cairo ever changes its ABI and we will need to use libcairo-N.dll
where N > 2.

So on balance, I prefer the original fix, which was much simpler.
Assuming it fixed the problem, of course (I didn't try).

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Pip Cet]

On Mon, Jul 1, 2019 at 2:12 PM Eli Zaretskii <eliz@gnu.org> wrote:
> So on balance, I prefer the original fix, which was much simpler.
> Assuming it fixed the problem, of course (I didn't try).

I prefer the other fix, for what it's worth, probably for the same
reasons you don't: it makes it easier to add more fancy features
editors don't need, such as true image transparency and SVG
animations. librsvg has painful limitations such as not allowing us to
pass in a foreground color (it simply uses black), but maybe that'll
be better one day, and then this patch would make it easier for us to
use the new features.

The fix looks more complicated than it is because it changes
indentation (for the better) and removes a misleading comment.

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Mon, 1 Jul 2019 16:25:49 +0000
> Cc: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>, 36315@debbugs.gnu.org
> 
> On Mon, Jul 1, 2019 at 2:12 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > So on balance, I prefer the original fix, which was much simpler.
> > Assuming it fixed the problem, of course (I didn't try).
> 
> I prefer the other fix, for what it's worth, probably for the same
> reasons you don't: it makes it easier to add more fancy features
> editors don't need, such as true image transparency and SVG
> animations. librsvg has painful limitations such as not allowing us to
> pass in a foreground color (it simply uses black), but maybe that'll
> be better one day, and then this patch would make it easier for us to
> use the new features.
> 
> The fix looks more complicated than it is because it changes
> indentation (for the better) and removes a misleading comment.

Are we talking about the same "other fix"?  I was talking about the
one that uses Cairo calls directly.  Changes in indentation is not
what makes it more complex than the patch you suggested.

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu]

On Mon, 01 Jul 2019 23:11:31 +0900,
Eli Zaretskii wrote:
> 
> > Date: Mon, 01 Jul 2019 12:46:55 +0900
> > From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > Cc: pipcet@gmail.com,
> > 	36315@debbugs.gnu.org
> > 
> > > It just looks like we are using libcairo and not librsvg.
> > >
> > > Again, it isn't something entirely rational, it just sounds weird to
> > > me.  Imagine that users libxml2 would need to call libiconv to decode
> > > UTF-8 encoded text in an XML file, for example.  Doesn't look right.
> > 
> > The situation for libcairo and librsvg should be familiar to us: we
> > are directly using Emacs core functionality even when working with
> > several major or minor modes.
> 
> Not sure what this alludes to.

The correspondence is:

  libcairo - Emacs core functionality
  librsvg  - Major/minor mode (e.g., Org mode)

Major/minor modes are not designed to be used in their own right, but
with (the direct use of) Emacs core functionality.  Would Org mode
users complain that it looks like they are using Emacs and not Org
mode?

Even the current SVG support code does something like this:

  /* Create a new RsvgHandle object.  */
  rsvg_handle = rsvg_handle_new ();

  /* Do some tasks with rsvg_handle.  */

  /* Free the RsvgHandle object.  */
  g_object_unref (rsvg_handle);

Do you reject this code because it looks like we are using gobject and
not librsvg?

> One thing that bothers me with using sub-libraries is that we now need
> another entry in dynamic-library-alist, which means complications if
> Cairo ever changes its ABI and we will need to use libcairo-N.dll
> where N > 2.

The patch also removes the entry for gdk-pixbuf, so the situation is
not getting worse.

FWIW, if we want to fix bug#35548 (use of rsvg_handle_write and
rsvg_handle_close that are being deprecated), then we will need to add
an entry for gio.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Eli Zaretskii]

> Date: Tue, 02 Jul 2019 18:46:28 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: 	pipcet@gmail.com,
> 	36315@debbugs.gnu.org
> 
> > > > Again, it isn't something entirely rational, it just sounds weird to
> > > > me.  Imagine that users libxml2 would need to call libiconv to decode
> > > > UTF-8 encoded text in an XML file, for example.  Doesn't look right.
> > > 
> > > The situation for libcairo and librsvg should be familiar to us: we
> > > are directly using Emacs core functionality even when working with
> > > several major or minor modes.
> > 
> > Not sure what this alludes to.
> 
> The correspondence is:
> 
>   libcairo - Emacs core functionality
>   librsvg  - Major/minor mode (e.g., Org mode)
> 
> Major/minor modes are not designed to be used in their own right, but
> with (the direct use of) Emacs core functionality.  Would Org mode
> users complain that it looks like they are using Emacs and not Org
> mode?
> 
> Even the current SVG support code does something like this:
> 
>   /* Create a new RsvgHandle object.  */
>   rsvg_handle = rsvg_handle_new ();
> 
>   /* Do some tasks with rsvg_handle.  */
> 
>   /* Free the RsvgHandle object.  */
>   g_object_unref (rsvg_handle);
> 
> Do you reject this code because it looks like we are using gobject and
> not librsvg?
> 
> > One thing that bothers me with using sub-libraries is that we now need
> > another entry in dynamic-library-alist, which means complications if
> > Cairo ever changes its ABI and we will need to use libcairo-N.dll
> > where N > 2.
> 
> The patch also removes the entry for gdk-pixbuf, so the situation is
> not getting worse.
> 
> FWIW, if we want to fix bug#35548 (use of rsvg_handle_write and
> rsvg_handle_close that are being deprecated), then we will need to add
> an entry for gio.

We clearly disagree, and I already said I didn't think this is worth
an argument.  Feel free to do whatever you see fit.

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> The patch also removes the entry for gdk-pixbuf, so the situation is
>> not getting worse.
>> 
>> FWIW, if we want to fix bug#35548 (use of rsvg_handle_write and
>> rsvg_handle_close that are being deprecated), then we will need to add
>> an entry for gio.
>
> We clearly disagree, and I already said I didn't think this is worth
> an argument.  Feel free to do whatever you see fit.

As I understand this thread, moving to Cairo for SVG images fixes at
least two problems: The transparency issue, and it also allows us to
scale SVGs, both of which seem like nice additions.

There were a number of proposed patches in this thread -- which one is
the one that we should apply here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Alan Third]

On Fri, Aug 21, 2020 at 03:04:22PM +0200, Lars Ingebrigtsen wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> The patch also removes the entry for gdk-pixbuf, so the situation is
> >> not getting worse.
> >> 
> >> FWIW, if we want to fix bug#35548 (use of rsvg_handle_write and
> >> rsvg_handle_close that are being deprecated), then we will need to add
> >> an entry for gio.
> >
> > We clearly disagree, and I already said I didn't think this is worth
> > an argument.  Feel free to do whatever you see fit.
> 
> As I understand this thread, moving to Cairo for SVG images fixes at
> least two problems: The transparency issue, and it also allows us to
> scale SVGs, both of which seem like nice additions.
> 
> There were a number of proposed patches in this thread -- which one is
> the one that we should apply here?

Possibly one from bug#40845... ;)

Although the one I provided there still needs work on the image cache.
-- 
Alan Third

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

>> There were a number of proposed patches in this thread -- which one is
>> the one that we should apply here?
>
> Possibly one from bug#40845... ;)

So many patches!

> Although the one I provided there still needs work on the image cache.

Is that much work?  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36315: 27.0.50; SVG transparency handling is inaccurate

[Alan Third]

On Sat, Aug 22, 2020 at 03:29:05PM +0200, Lars Ingebrigtsen wrote:
> Alan Third <alan@idiocy.org> writes:
> 
> >> There were a number of proposed patches in this thread -- which one is
> >> the one that we should apply here?
> >
> > Possibly one from bug#40845... ;)
> 
> So many patches!
> 
> > Although the one I provided there still needs work on the image cache.
> 
> Is that much work?  :-)

Actually, I think I made a mistake, this is another issue.

On the plus side, it inspired me to finish off the patch for 40845. :)
-- 
Alan Third

bug#36315: Incorrect SVG color

[Qiantan Hong]

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

Is there any update on this bug?
The current behavior is unacceptable when the SVG color
is supposed to be the same as some color in Emacs
(in my case, I’m using SVG to build some UI elements),
The following code

(require 'svg)

(defun make-image (color)
  (let ((svg (svg-create 100 100)))
    (svg-rectangle svg 0 0 100 100 :fill color)
    (svg-image svg)))

(set-frame-parameter (window-frame) 'background-color “#f00000")

(insert (propertize " " 'display (make-image "#f00000")))

produce very notable color difference.


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

bug#36315: Incorrect SVG color

[Alan Third]

On Fri, Dec 18, 2020 at 12:49:23AM +0000, Qiantan Hong wrote:
> Is there any update on this bug?
> The current behavior is unacceptable when the SVG color
> is supposed to be the same as some color in Emacs
> (in my case, I’m using SVG to build some UI elements),
> The following code
> 
> (require 'svg)
> 
> (defun make-image (color)
>   (let ((svg (svg-create 100 100)))
>     (svg-rectangle svg 0 0 100 100 :fill color)
>     (svg-image svg)))
> 
> (set-frame-parameter (window-frame) 'background-color “#f00000")
> 
> (insert (propertize " " 'display (make-image "#f00000")))
> 
> produce very notable color difference.

I can no longer replicate this on the master branch on macOS. We've
out-sourced colour handling fully to librsvg now, so that may make the
difference?

Either that or macOS is doing something unusual with colours, which I
wouldn't rule out.
-- 
Alan Third

bug#36315: Incorrect SVG color

[Qiantan Hong]

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

I started to SEGFAULT when building from master branch
on macOS some time ago and now I’m doing
$ brew install emacs-plus —fetch-HEAD —with-xwidgets
The problem is there for me.
M-x emacs-version:
GNU Emacs 27.1 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G6032)) of 2020-12-18

> On Dec 18, 2020, at 10:00 AM, Alan Third <alan@idiocy.org> wrote:
> 
> On Fri, Dec 18, 2020 at 12:49:23AM +0000, Qiantan Hong wrote:
>> Is there any update on this bug?
>> The current behavior is unacceptable when the SVG color
>> is supposed to be the same as some color in Emacs
>> (in my case, I’m using SVG to build some UI elements),
>> The following code
>> 
>> (require 'svg)
>> 
>> (defun make-image (color)
>>  (let ((svg (svg-create 100 100)))
>>    (svg-rectangle svg 0 0 100 100 :fill color)
>>    (svg-image svg)))
>> 
>> (set-frame-parameter (window-frame) 'background-color “#f00000")
>> 
>> (insert (propertize " " 'display (make-image "#f00000")))
>> 
>> produce very notable color difference.
> 
> I can no longer replicate this on the master branch on macOS. We've
> out-sourced colour handling fully to librsvg now, so that may make the
> difference?
> 
> Either that or macOS is doing something unusual with colours, which I
> wouldn't rule out.
> --
> Alan Third


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

bug#36315: Incorrect SVG color

[Qiantan Hong]

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

I suspect it is some weird thing about macOS,
because now the SVG color is brighter instead of darker...

> On Dec 18, 2020, at 10:00 AM, Alan Third <alan@idiocy.org> wrote:
> 
> On Fri, Dec 18, 2020 at 12:49:23AM +0000, Qiantan Hong wrote:
>> Is there any update on this bug?
>> The current behavior is unacceptable when the SVG color
>> is supposed to be the same as some color in Emacs
>> (in my case, I’m using SVG to build some UI elements),
>> The following code
>> 
>> (require 'svg)
>> 
>> (defun make-image (color)
>>  (let ((svg (svg-create 100 100)))
>>    (svg-rectangle svg 0 0 100 100 :fill color)
>>    (svg-image svg)))
>> 
>> (set-frame-parameter (window-frame) 'background-color “#f00000")
>> 
>> (insert (propertize " " 'display (make-image "#f00000")))
>> 
>> produce very notable color difference.
> 
> I can no longer replicate this on the master branch on macOS. We've
> out-sourced colour handling fully to librsvg now, so that may make the
> difference?
> 
> Either that or macOS is doing something unusual with colours, which I
> wouldn't rule out.
> --
> Alan Third


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

bug#36315: Incorrect SVG color

[Qiantan Hong]

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

I fixed my build and it behaves correctly on macOS
now.

M-x emacs-version
GNU Emacs 28.0.50 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G6032)) of 2020-12-18

To make sure I understand it, did we start outsourcing color some point after 27.1?


> On Dec 18, 2020, at 10:00 AM, Alan Third <alan@idiocy.org> wrote:
> 
> On Fri, Dec 18, 2020 at 12:49:23AM +0000, Qiantan Hong wrote:
>> Is there any update on this bug?
>> The current behavior is unacceptable when the SVG color
>> is supposed to be the same as some color in Emacs
>> (in my case, I’m using SVG to build some UI elements),
>> The following code
>> 
>> (require 'svg)
>> 
>> (defun make-image (color)
>>  (let ((svg (svg-create 100 100)))
>>    (svg-rectangle svg 0 0 100 100 :fill color)
>>    (svg-image svg)))
>> 
>> (set-frame-parameter (window-frame) 'background-color “#f00000")
>> 
>> (insert (propertize " " 'display (make-image "#f00000")))
>> 
>> produce very notable color difference.
> 
> I can no longer replicate this on the master branch on macOS. We've
> out-sourced colour handling fully to librsvg now, so that may make the
> difference?
> 
> Either that or macOS is doing something unusual with colours, which I
> wouldn't rule out.
> --
> Alan Third


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

bug#36315: Incorrect SVG color

[Alan Third]

On Fri, Dec 18, 2020 at 04:42:03PM +0000, Qiantan Hong wrote:
> I fixed my build and it behaves correctly on macOS
> now.
> 
> M-x emacs-version
> GNU Emacs 28.0.50 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G6032)) of 2020-12-18

Excellent! Thanks for letting us know.

> To make sure I understand it, did we start outsourcing color some point after 27.1?

We pass the foreground and background colours into librsvg, which
creates the bitmap image that's displayed, in a commit that's just a
few months old. It will appear in Emacs 28.

I don't expect the change to be included in Emacs 27 since it's a
fairly large change introducing a number of new features and any new
Emacs 27 releases will be primarily bug fixes.
-- 
Alan Third

bug#36315: Incorrect SVG color

[Alan Third]

On Fri, Dec 18, 2020 at 06:04:45PM +0000, Alan Third wrote:
> On Fri, Dec 18, 2020 at 04:42:03PM +0000, Qiantan Hong wrote:
> > I fixed my build and it behaves correctly on macOS
> > now.
> > 
> > M-x emacs-version
> > GNU Emacs 28.0.50 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G6032)) of 2020-12-18
> 
> Excellent! Thanks for letting us know.
> 
> > To make sure I understand it, did we start outsourcing color some point after 27.1?
> 
> We pass the foreground and background colours into librsvg, which
> creates the bitmap image that's displayed, in a commit that's just a
> few months old. It will appear in Emacs 28.
> 
> I don't expect the change to be included in Emacs 27 since it's a
> fairly large change introducing a number of new features and any new
> Emacs 27 releases will be primarily bug fixes.

In a fit of over-excitement I closed this without checking whether
it's still a problem outside of the NS port. Should I reopen it?
-- 
Alan Third

bug#36315: Incorrect SVG color

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> In a fit of over-excitement I closed this without checking whether
> it's still a problem outside of the NS port. Should I reopen it?

I can't reproduce the problem with the current Emacs trunk on Debian,
either, so I think you've fixed it.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no