bug#36315: 27.0.50; SVG transparency handling is inaccurate

[YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>]

[-- 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  */