bug#40845: SVG rendering issues

bug#40845: SVG rendering issues

[Clément Pit-Claudel]

Hi all,

As discussed on the mailing list, a number of issues currently exist with our SVG rendering implementation.  I have tried to summarize the ones I'm aware of in the following example.

(with-current-buffer (get-buffer-create "*svg bugs*")
  (erase-buffer)
  (require 'face-remap)
  (setq text-scale-mode-amount 10)
  (text-scale-mode)
  (let ((svg (svg-create 16 16)))
    (svg-ellipse svg 8 8 4 4)
    (insert "Text: ")
    (print (svg-image svg :ascent 100))
    (insert-image (svg-image svg :ascent 100))
    (insert-image (svg-image svg :scale 5.0 :ascent 'center :foreground "red" :background "darkgreen"))
    (add-text-properties
     (point-min) (point-max)
     '(face (:foreground "orange" :background "purple")
            mouse-face '(:foreground "purple" :background "orange"))))
  (pop-to-buffer (current-buffer)))

The issues:

1. Manually scaling an image, as is done for the second image, doesn't re-render the svg: is scales the bitmap-rendered version of it, causing blurriness.
2. The SVG images don't inherit the background of the current face; instead, they inherit the background of the default face.
3. The SVG images don't inherit the foreground of the current face; instead, they use a black foreground.
4. The :foreground keyword has no effect on svg images.
5. The images are not scaled with the text: changing text-scale-mode-amount doesn't change the size of the images.

For 1, 2, 3, and 4, the expected behavior is easy to define:
- For 1, the image should be scaled before being rasterized. 
- For 2 and 3, the image should inherit the characteristics of the current face, and be re-rendered if that face changes (e.g. when mouse-face applies, which is important for buttons)
- For 4, the :foreground property should be respected

For 5, it's a bit trickier.  One option would be to accept a float-valued :height specification and treat that as a multiple of the current font size.

A partial workaround for 2 is to add an explicit :background, but it doesn't help with face changes, such as applying a mouse-face.  For 1 and 5 it can be enough to set the size in the SVG and add hooks, but that only works easily for SVGs created within emacs.  For 3 and 4, I don't know of a workaround except modifying the SVG.

Cheers,
Clément.

bug#40845: SVG rendering issues

[Pip Cet]

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

On Sat, Apr 25, 2020 at 12:20 PM Clément Pit-Claudel
<cpitclaudel@gmail.com> wrote:
>
> Hi all,
>
> As discussed on the mailing list, a number of issues currently exist with our SVG rendering implementation.  I have tried to summarize the ones I'm aware of in the following example.
>
> (with-current-buffer (get-buffer-create "*svg bugs*")
>   (erase-buffer)
>   (require 'face-remap)
>   (setq text-scale-mode-amount 10)
>   (text-scale-mode)
>   (let ((svg (svg-create 16 16)))
>     (svg-ellipse svg 8 8 4 4)
>     (insert "Text: ")
>     (print (svg-image svg :ascent 100))
>     (insert-image (svg-image svg :ascent 100))
>     (insert-image (svg-image svg :scale 5.0 :ascent 'center :foreground "red" :background "darkgreen"))
>     (add-text-properties
>      (point-min) (point-max)
>      '(face (:foreground "orange" :background "purple")
>             mouse-face '(:foreground "purple" :background "orange"))))
>   (pop-to-buffer (current-buffer)))
>
> The issues:
>
> 1. Manually scaling an image, as is done for the second image, doesn't re-render the svg: is scales the bitmap-rendered version of it, causing blurriness.
> 2. The SVG images don't inherit the background of the current face; instead, they inherit the background of the default face.
> 3. The SVG images don't inherit the foreground of the current face; instead, they use a black foreground.
> 4. The :foreground keyword has no effect on svg images.
> 5. The images are not scaled with the text: changing text-scale-mode-amount doesn't change the size of the images.

I would like to add

6. When the cursor is over an SVG image, it is displayed with a box
around it rather than with inverted video as characters are.

I've played around a while ago in an attempt to fix these issues, and
be able to define "character-like" SVG glyphs. My approach was to add
a display spec of the form (gen FN), which calls FN with the current
face parameters as arguments whenever redisplaying the spec, then uses
the return value (an image spec) as the actual spec to be displayed.

It's probably quite slow, but (as of June last) it works, even when
displaying the same buffer twice. Calling elisp from the redisplay
engine is, of course, something we don't really want to have more of,
but we'd do so anyway for a `when' spec (in fact, an alternative
approach to get this working with old Emacsen was to abuse a `when'
spec to set the image spec correctly).

The attached patch, IIRC, is the main part. As you can see, it leaves
most of the work to the Lisp side of things. It can fix all issues
mentioned above by modifying the SVG as required.

Also, IIRC, 3 is an rsvg issue.

[-- Attachment #2: gen.diff --]
[-- Type: text/x-patch, Size: 3137 bytes --]

commit 0af1850485f3ad1519e52de2b1694efa922917f4
Author: Pip Cet <pipcet@gmail.com>
Date:   Sat Jun 22 07:53:28 2019 +0000

    snapshot

diff --git a/src/callint.c b/src/callint.c
index 88a3c348d0..6f2e5d5066 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -824,6 +824,7 @@ syms_of_callint (void)
   DEFSYM (Qlet, "let");
   DEFSYM (Qif, "if");
   DEFSYM (Qwhen, "when");
+  DEFSYM (Qgen, "gen");
   DEFSYM (Qletx, "let*");
   DEFSYM (Qsave_excursion, "save-excursion");
   DEFSYM (Qprogn, "progn");
diff --git a/src/xdisp.c b/src/xdisp.c
index 5699aac61b..717fb30ec3 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2728,7 +2728,6 @@ safe_call2 (Lisp_Object fn, Lisp_Object arg1, Lisp_Object arg2)
   return safe_call (3, fn, arg1, arg2);
 }
 
-
 \f
 /***********************************************************************
 			      Debugging
@@ -4893,6 +4892,7 @@ handle_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
 #endif
       && !EQ (XCAR (spec), Qspace)
       && !EQ (XCAR (spec), Qwhen)
+      && !EQ (XCAR (spec), Qgen)
       && !EQ (XCAR (spec), Qslice)
       && !EQ (XCAR (spec), Qspace_width)
       && !EQ (XCAR (spec), Qheight)
@@ -4991,6 +4991,8 @@ display_prop_end (struct it *it, Lisp_Object object, struct text_pos start_pos)
    Value is non-zero if something was found which replaces the display
    of buffer or string text.  */
 
+extern Lisp_Object *lface_id_to_name;
+
 static int
 handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
 			    Lisp_Object overlay, struct text_pos *position,
@@ -5002,6 +5004,46 @@ handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
   struct text_pos start_pos = *position;
   void *itdata = NULL;
 
+  if (it != NULL &&
+      CONSP (spec) &&
+      EQ (XCAR (spec), Qgen))
+    {
+      spec = XCDR (spec);
+      if (!CONSP (spec))
+	return 0;
+      Lisp_Object gen = XCAR (spec);
+      struct face *face = FACE_FROM_ID (it->f, it->face_id);
+      Lisp_Object lface = Qnil;
+      Lisp_Object props[] = {
+	QCtype,
+	QCfamily,
+	QCfoundry,
+	QCwidth,
+	QCheight,
+	QCweight,
+	QCslant,
+	QCunderline,
+	QCinverse_video,
+	QCforeground,
+	QCbackground,
+	QCstipple,
+	QCoverline,
+	QCstrike_through,
+	QCbox,
+	QCfont,
+	QCinherit,
+	QCfontset,
+	QCdistant_foreground,
+      };
+      for (int i = 0; i < LFACE_VECTOR_SIZE; i++)
+	lface = Fcons (Fcons (props[i], face->lface[i]),
+		       lface);
+      Lisp_Object font = Qnil;
+      XSETFONT (font, face->font);
+      lface = Fcons (Fcons (QCfont, font), lface);
+      spec = safe_call1 (gen, lface);
+    }
+
   /* If SPEC is a list of the form `(when FORM . VALUE)', evaluate FORM.
      If the result is non-nil, use VALUE instead of SPEC.  */
   form = Qt;
diff --git a/src/xfaces.c b/src/xfaces.c
index 012cc96470..47453c2b31 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -294,7 +294,7 @@ #define FACE_CACHE_BUCKETS_SIZE 1001
 
 /* A vector mapping Lisp face Id's to face names.  */
 
-static Lisp_Object *lface_id_to_name;
+Lisp_Object *lface_id_to_name;
 static ptrdiff_t lface_id_to_name_size;
 
 #ifdef HAVE_WINDOW_SYSTEM

bug#40845: SVG rendering issues

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 25 Apr 2020 14:34:11 +0000
> Cc: 40845@debbugs.gnu.org
> 
> 6. When the cursor is over an SVG image, it is displayed with a box
> around it rather than with inverted video as characters are.
> 
> I've played around a while ago in an attempt to fix these issues, and
> be able to define "character-like" SVG glyphs. My approach was to add
> a display spec of the form (gen FN), which calls FN with the current
> face parameters as arguments whenever redisplaying the spec, then uses
> the return value (an image spec) as the actual spec to be displayed.

I don't think I understand why we need to call a function for this.
This is about displaying an image with a specific background color,
isn't it?

bug#40845: SVG rendering issues

[Pip Cet]

On Sat, Apr 25, 2020 at 3:30 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > I've played around a while ago in an attempt to fix these issues, and
> > be able to define "character-like" SVG glyphs. My approach was to add
> > a display spec of the form (gen FN), which calls FN with the current
> > face parameters as arguments whenever redisplaying the spec, then uses
> > the return value (an image spec) as the actual spec to be displayed.
>
> I don't think I understand why we need to call a function for this.
> This is about displaying an image with a specific background color,
> isn't it?

Even if the problem were just the choice of background color, that
would require significant and non-trivial changes for some cases: for
example, an emoji might have to choose foreground colors that provide
sufficient contrast to the background color, and antialiasing and
sub-pixel rendering would also need to be taken into account.

But, at least for me, it's not: I want to be able to define something
that behaves like a character, including displaying differently in
different frames and depending on different face parameters such as
weight, slant, and RTL-ness. I don't really see a way of doing that
satisfactorily without invoking user code outside of the display
engine/image backend code.

bug#40845: SVG rendering issues

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 25 Apr 2020 15:48:39 +0000
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org
> 
> On Sat, Apr 25, 2020 at 3:30 PM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > I don't think I understand why we need to call a function for this.
> > This is about displaying an image with a specific background color,
> > isn't it?
> 
> Even if the problem were just the choice of background color, that
> would require significant and non-trivial changes for some cases: for
> example, an emoji might have to choose foreground colors that provide
> sufficient contrast to the background color, and antialiasing and
> sub-pixel rendering would also need to be taken into account.

We are talking about displaying images, not characters, right?  So why
are emoji relevant to this?

> I want to be able to define something that behaves like a character,
> including displaying differently in different frames and depending
> on different face parameters such as weight, slant, and RTL-ness.

All of that is possible with images, so I don't think I understand why
we need to handle this as a character.  This very bug report was filed
because I said we shouldn't use characters and fonts for these
purposes, so let's not discuss that alternative, please, at least not
here.

bug#40845: SVG rendering issues

[Pip Cet]

On Sat, Apr 25, 2020 at 4:10 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sat, 25 Apr 2020 15:48:39 +0000
> > Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org
> >
> > On Sat, Apr 25, 2020 at 3:30 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > >
> > > I don't think I understand why we need to call a function for this.
> > > This is about displaying an image with a specific background color,
> > > isn't it?
> >
> > Even if the problem were just the choice of background color, that
> > would require significant and non-trivial changes for some cases: for
> > example, an emoji might have to choose foreground colors that provide
> > sufficient contrast to the background color, and antialiasing and
> > sub-pixel rendering would also need to be taken into account.
>
> We are talking about displaying images, not characters, right?

Correct: images which behave like character glyphs, but don't actually
have character codepoints, and aren't implemented by fonts.

> So why are emoji relevant to this?

An emoji is an example of an image which needs to know face properties
to blend in, or stick out, or whatever it is people who use them want
to happen. The intention was not that you'd use an emoji codepoint and
a font providing a character for that codepoint, which is a silly way
of implementing emoji.

> > I want to be able to define something that behaves like a character,
> > including displaying differently in different frames and depending
> > on different face parameters such as weight, slant, and RTL-ness.
>
> All of that is possible with images, so I don't think I understand why
> we need to handle this as a character.

Yes, my approach uses images, and no, it doesn't "handle this as a
character". It makes the glyph's apparent behavior match that of
character glyphs, while actually displaying an image spec which
changes with the properties of surrounding text, etc.

> This very bug report was filed
> because I said we shouldn't use characters and fonts for these
> purposes, so let's not discuss that alternative, please, at least not
> here.

No one was.

bug#40845: SVG rendering issues

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 25 Apr 2020 17:38:31 +0000
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org
> 
> Correct: images which behave like character glyphs, but don't actually
> have character codepoints, and aren't implemented by fonts.
> 
> > So why are emoji relevant to this?
> 
> An emoji is an example of an image which needs to know face properties
> to blend in, or stick out, or whatever it is people who use them want
> to happen. The intention was not that you'd use an emoji codepoint and
> a font providing a character for that codepoint, which is a silly way
> of implementing emoji.
> 
> > > I want to be able to define something that behaves like a character,
> > > including displaying differently in different frames and depending
> > > on different face parameters such as weight, slant, and RTL-ness.
> >
> > All of that is possible with images, so I don't think I understand why
> > we need to handle this as a character.
> 
> Yes, my approach uses images, and no, it doesn't "handle this as a
> character". It makes the glyph's apparent behavior match that of
> character glyphs, while actually displaying an image spec which
> changes with the properties of surrounding text, etc.
> 
> > This very bug report was filed
> > because I said we shouldn't use characters and fonts for these
> > purposes, so let's not discuss that alternative, please, at least not
> > here.
> 
> No one was.

Then I guess I don't understand your implementation at all.

bug#40845: SVG rendering issues

[Pip Cet]

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

On Sat, Apr 25, 2020 at 6:07 PM Eli Zaretskii <eliz@gnu.org> wrote:
or these
> Then I guess I don't understand your implementation at all.

My use case is to include a glyph which is supposed to look like a
character, but doesn't actually have a unicode codepoint. (I'm sorry
if this differs from the use cases you're exclusively concerned with,
but it appeared to be relevant enough to Clément's problem that I
assumed he was having a similar issue).

That means that we want to use an image spec, not a character in a
font; but that image spec depends on face/font properties, because we
want to blend in with surrounding text. The most obvious ones are
foreground and background color and size, but slant and weight would
also affect properly-rendered character-like images.

It seems fairly obvious to me that it's a bad idea to do all the work
in the display engine or in C code: sub-pixel rendering and
anti-aliasing are hard to get right. A character-like glyph might need
a third color which provides sufficient contrast to the foreground and
background colors, and that color space calculation is complicated
enough to be moved to Lisp.

So I moved it to Lisp: there's Lisp code which is passed the font/face
properties, and returns an image spec that's appropriate for that.

The attached patch actually applies to and works with master, and
includes an example. It can no longer easily be demonstrated to do the
right thing when the same buffer is displayed in different frames,
because Emacs currently applies text scaling per buffer rather than
per frame (IMHO, that's a bug). It also doesn't properly display the
cursor as a block cursor when it's over the glyph, because I can no
longer find the code which allowed me to tell, from Lisp, whether that
was the case.

And of course it doesn't use the right font metrics, because these
currently aren't exposed to Lisp.

But all these limitations are fixable.

[-- Attachment #2: 0001-support-generated-display-specs.patch --]
[-- Type: text/x-patch, Size: 4818 bytes --]

From 8e7aa38e098bd044a71fa20df1593d2b37347f1c Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sat, 25 Apr 2020 18:41:02 +0000
Subject: [PATCH] support generated display specs

---
 generated-image-specs.el | 31 ++++++++++++++++++++++++++++
 src/callint.c            |  1 +
 src/xdisp.c              | 44 ++++++++++++++++++++++++++++++++++++++++
 src/xfaces.c             |  2 +-
 4 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 generated-image-specs.el

diff --git a/generated-image-specs.el b/generated-image-specs.el
new file mode 100644
index 0000000000..b764d1999d
--- /dev/null
+++ b/generated-image-specs.el
@@ -0,0 +1,31 @@
+(require 'svg)
+
+(defun generate-circle-image (alist)
+  (let* ((vsize (font-get (alist-get :font alist) :size))
+         (foreground (apply #'color-rgb-to-hex (nconc (color-name-to-rgb (alist-get :foreground alist)) (list 2))))
+         (background (apply #'color-rgb-to-hex (nconc (color-name-to-rgb (alist-get :background alist)) (list 2))))
+         (hsize (* vsize .8))
+         (svg (svg-create hsize vsize))
+         (sw (* .1 hsize)))
+    (svg-rectangle svg 0 0 hsize vsize :fill background)
+    (svg-circle svg (* hsize .5)
+                (- (* .8 vsize) (* .5 hsize))
+                (- (* hsize .5) sw)
+                :fill "none"
+                :stroke foreground
+                :stroke-width sw)
+    (svg-line svg
+              (* hsize .5) (- (* .8 vsize) (* .5 hsize))
+              (* hsize .5) (- (* .8 vsize) sw)
+              :stroke foreground
+              :stroke-width sw)
+    (svg-circle svg (* hsize .5) (- (* .8 vsize) (* (/ 2.0 3.0) hsize))
+                sw
+                :fill foreground
+                :stroke "none")
+    (let ((ret (svg-image svg)))
+      (setcdr ret (plist-put (cdr ret) :scale 1.0))
+      (setcdr ret (plist-put (cdr ret) :ascent 80))
+      ret)))
+
+(insert (propertize " " 'display `(generated-spec ,#'generate-circle-image)))
diff --git a/src/callint.c b/src/callint.c
index eb916353a0..2c34dddbe4 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -826,6 +826,7 @@ syms_of_callint (void)
   DEFSYM (Qlet, "let");
   DEFSYM (Qif, "if");
   DEFSYM (Qwhen, "when");
+  DEFSYM (Qgenerated_spec, "generated-spec");
   DEFSYM (Qletx, "let*");
   DEFSYM (Qsave_excursion, "save-excursion");
   DEFSYM (Qprogn, "progn");
diff --git a/src/xdisp.c b/src/xdisp.c
index 3258893956..b341cbb735 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -5050,6 +5050,7 @@ handle_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
 #endif
       && !EQ (XCAR (spec), Qspace)
       && !EQ (XCAR (spec), Qwhen)
+      && !EQ (XCAR (spec), Qgenerated_spec)
       && !EQ (XCAR (spec), Qslice)
       && !EQ (XCAR (spec), Qspace_width)
       && !EQ (XCAR (spec), Qheight)
@@ -5148,6 +5149,8 @@ display_prop_end (struct it *it, Lisp_Object object, struct text_pos start_pos)
    Value is non-zero if something was found which replaces the display
    of buffer or string text.  */
 
+extern Lisp_Object *lface_id_to_name;
+
 static int
 handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
 			    Lisp_Object overlay, struct text_pos *position,
@@ -5159,6 +5162,47 @@ handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
   struct text_pos start_pos = *position;
   void *itdata = NULL;
 
+  if (it != NULL &&
+      CONSP (spec) &&
+      EQ (XCAR (spec), Qgenerated_spec))
+    {
+      spec = XCDR (spec);
+      if (!CONSP (spec))
+	return 0;
+      Lisp_Object gen = XCAR (spec);
+      struct face *face = FACE_FROM_ID (it->f, it->face_id);
+      Lisp_Object lface = Qnil;
+      Lisp_Object props[] = {
+	QCtype,
+	QCfamily,
+	QCfoundry,
+	QCwidth,
+	QCheight,
+	QCweight,
+	QCslant,
+	QCunderline,
+	QCinverse_video,
+	QCforeground,
+	QCbackground,
+	QCstipple,
+	QCoverline,
+	QCstrike_through,
+	QCbox,
+	QCfont,
+	QCinherit,
+	QCfontset,
+	QCdistant_foreground,
+	QCextend,
+      };
+      for (int i = 0; i < LFACE_VECTOR_SIZE; i++)
+	lface = Fcons (Fcons (props[i], face->lface[i]),
+		       lface);
+      Lisp_Object font = Qnil;
+      XSETFONT (font, face->font);
+      lface = Fcons (Fcons (QCfont, font), lface);
+      spec = safe_call1 (gen, lface);
+    }
+
   /* If SPEC is a list of the form `(when FORM . VALUE)', evaluate FORM.
      If the result is non-nil, use VALUE instead of SPEC.  */
   form = Qt;
diff --git a/src/xfaces.c b/src/xfaces.c
index bab142ade0..01429ac86e 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -310,7 +310,7 @@ #define FACE_CACHE_BUCKETS_SIZE 1001
 
 /* A vector mapping Lisp face Id's to face names.  */
 
-static Lisp_Object *lface_id_to_name;
+Lisp_Object *lface_id_to_name;
 static ptrdiff_t lface_id_to_name_size;
 
 #ifdef HAVE_WINDOW_SYSTEM
-- 
2.26.2

bug#40845: SVG rendering issues

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 25 Apr 2020 19:41:31 +0000
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org
> 
> My use case is to include a glyph which is supposed to look like a
> character, but doesn't actually have a unicode codepoint. (I'm sorry
> if this differs from the use cases you're exclusively concerned with,
> but it appeared to be relevant enough to Clément's problem that I
> assumed he was having a similar issue).
> 
> That means that we want to use an image spec, not a character in a
> font; but that image spec depends on face/font properties, because we
> want to blend in with surrounding text. The most obvious ones are
> foreground and background color and size, but slant and weight would
> also affect properly-rendered character-like images.

If we want to display an image, the logical way would be to use an
image spec, which is already supported, enhancing it as needed.  I
don't see anything in the above attributes that would be hard to have
within the existing framework of how we display images.  I don't
really understand how you can make images "slant" or "bold" (unless
they were like that to begin with), or why would we want to, but maybe
there's some way of interpreting that as well.

None of this requires a new spec or calling Lisp.  All of the metrics
of the current face's font are known by the display code, and there
are many that cannot be obtained from Lisp, like the current line
height.  Doing these calculations in Lisp is IMO the worst possible
way of using Lisp callouts from the display engine: this has all the
disadvantages of calling Lisp, and none of the advantages.

> It seems fairly obvious to me that it's a bad idea to do all the work
> in the display engine or in C code: sub-pixel rendering and
> anti-aliasing are hard to get right.

Aren't we talking about displaying existing images, which someone
already prepared while using all those techniques?  Why would the
display engine want or need to modify the image using them?

> A character-like glyph might need a third color which provides
> sufficient contrast to the foreground and background colors, and
> that color space calculation is complicated enough to be moved to
> Lisp.

Within the context of what these icons are supposed to be used for, I
envision the images coming with bright, catchy colors that should be
left alone, not enhanced by color and contrast tweaking of whatever
happens to be the face colors the user wants.  Those who design such
icons do a much better job than any Lisp program.

> But all these limitations are fixable.

They wouldn't have existed if you used the code which displays images.

So I still don't understand the motivation for this approach.  (Read:
I think it's wrong.)

bug#40845: SVG rendering issues

[Pip Cet]

On Sat, Apr 25, 2020 at 8:11 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sat, 25 Apr 2020 19:41:31 +0000
> > Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org
> >
> > My use case is to include a glyph which is supposed to look like a
> > character, but doesn't actually have a unicode codepoint.
> >
> > That means that we want to use an image spec, not a character in a
> > font; but that image spec depends on face/font properties, because we
> > want to blend in with surrounding text. The most obvious ones are
> > foreground and background color and size, but slant and weight would
> > also affect properly-rendered character-like images.
>
> If we want to display an image, the logical way would be to use an
> image spec, which is already supported, enhancing it as needed.

I want to display a glyph that looks different depending on its
textual context. That means it's not "an image", in the traditional
sense. and enhancing image specs to handle such context-dependent
glyphs is wrong, because it moves code into the image backend that
applies to all image backends.

To be clear, to me "an image" is a compressed way of storing RGBA
information for each pixel in a rectangle. It has no concept of
foreground color or background color or slant or weight.

The enhancement I propose is a layer of indirection, allowing the
image spec itself to be adjusted to face/font properties. Whether or
not it's best to use a Lisp function for that, or a hash table, is
something I'm not clear about yet.

> I don't see anything in the above attributes that would be hard to have
> within the existing framework of how we display images.

I don't see anything in the above attributes that would be possible to
have without major changes to image backends, for features that do not
depend on the image backend.

> I don't
> really understand how you can make images "slant" or "bold" (unless
> they were like that to begin with), or why would we want to, but maybe
> there's some way of interpreting that as well.

Because we want the glyph to blend in with surrounding text. Like a
character. Thus "character-like glyph". It's not an image. My use case
is displaying character-like glyphs, implemented as
dynamically-generated images, not to display final images.

And, no, there's no way for an image backend to interpret that. We
have to use different image specs.

> None of this requires a new spec or calling Lisp.

I don't see how to do it without a new spec. I do see an obvious way
of doing it without calling Lisp, by using, say, a hash table. That's
an implementation detail, and one I'm willing to argue about, but "you
want images, not character-like glyphs" isn't an argument, it's simply
a statement that you don't want my use case to be supported.

> All of the metrics
> of the current face's font are known by the display code, and there
> are many that cannot be obtained from Lisp, like the current line
> height.

Indeed, so the display code has to expose those to Lisp.

> Doing these calculations in Lisp is IMO the worst possible
> way of using Lisp callouts from the display engine: this has all the
> disadvantages of calling Lisp, and none of the advantages.

The advantage is my use case is supported, and with your proposal
(which I don't understand completely, but it appears to be a
perfunctory implementation of "foreground"/"background" colors for
images) it isn't.

Whether we should call Lisp (which would, in practice, return a cached
image spec most of the time) or use a hash table (and we'd have to
somehow trigger Lisp updating that hash table) is something I'm not
clear about. Since we're already calling Lisp from code in the
vicinity, it seems relatively safe to do so.

> > It seems fairly obvious to me that it's a bad idea to do all the work
> > in the display engine or in C code: sub-pixel rendering and
> > anti-aliasing are hard to get right.
>
> Aren't we talking about displaying existing images, which someone
> already prepared while using all those techniques?

I'm talking about displaying character-like glyphs.

> Why would the
> display engine want or need to modify the image using them?

To make the glyph character-like.

> > A character-like glyph might need a third color which provides
> > sufficient contrast to the foreground and background colors, and
> > that color space calculation is complicated enough to be moved to
> > Lisp.
>
> Within the context of what these icons are supposed to be used for, I
> envision the images coming with bright, catchy colors that should be
> left alone, not enhanced by color and contrast tweaking of whatever
> happens to be the face colors the user wants.

I'm not sure which icons you're thinking about, but I certainly don't
want my symbols to have "bright, catchy colors that should be left
alone". That's one valid thing for the Lisp code to do, to leave the
image alone and not modify it, but I want the option to have
character-like glyphs, which look like characters but aren't, and that
means they don't have bright, catchy colors that should be left alone.

> Those who design such
> icons do a much better job than any Lisp program.

That's an end user decision, not something we should assume a priori.

> > But all these limitations are fixable.
> They wouldn't have existed if you used the code which displays images.

I want to display character-like glyphs. I don't see a way doing that
with existing image specs, because the image specs do not depend on
face/font properties. I'm still not sure what you're proposing, but
making images depend in a brutish way on "foreground" and "background"
color is a highly specific and limited solution that doesn't solve my
use case at all.

> So I still don't understand the motivation for this approach.  (Read:
> I think it's wrong.)

I'm still not sure whether you're suggesting anything that would
handle my use case even remotely, or simply saying it's not a use case
that Emacs should be extensible to. I've certainly not seen any
suggestion that would do the former.

But, just to be clear, I posted this code because I'd already made the
minor effort to get a new (and, IMHO, important) Emacs feature to
work. I deliberately decided not to submit it, because while it is a
new, generally useful feature Emacs sorely lacks, there are both
productive discussions to have about it (such as whether it uses a
hash table or a Lisp function calls) and unproductive ones, and the
latter in particular translate to a major effort that I did not at the
time feel was worth it.

bug#40845: SVG rendering issues

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 26 Apr 2020 10:15:39 +0000
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org
> 
> > If we want to display an image, the logical way would be to use an
> > image spec, which is already supported, enhancing it as needed.
> 
> I want to display a glyph that looks different depending on its
> textual context. That means it's not "an image", in the traditional
> sense.

Then I guess we are talking about two different use cases.  This bug
report was filed in the context of this discussion:

  https://lists.gnu.org/archive/html/emacs-devel/2020-04/msg01386.html

where I said that I think such UI effects should be implemented by
using images, not special characters and special fonts.

> and enhancing image specs to handle such context-dependent
> glyphs is wrong, because it moves code into the image backend that
> applies to all image backends.

"Image backend" in this context is the image libraries, like librsvg
and libpng, is that right?  If so, which one of them can do the stuff
Clément reported missing?

> The enhancement I propose is a layer of indirection, allowing the
> image spec itself to be adjusted to face/font properties.

The kind of adjustments that we need for the use case which prompted
this discussion can be done inside the display engine.  Or at least I
don't see a reason why it couldn't be.

> > I don't
> > really understand how you can make images "slant" or "bold" (unless
> > they were like that to begin with), or why would we want to, but maybe
> > there's some way of interpreting that as well.
> 
> Because we want the glyph to blend in with surrounding text. Like a
> character.

Not really, not in the use case discussed here.

> And, no, there's no way for an image backend to interpret that. We
> have to use different image specs.

A Lisp program that prepares such display can prepare the spec
accordingly, if needed.  Any effects that cannot be explicitly
produced by Lisp and we'd need the display code to produce can be
communicated using the image spec keywords, like we always do.  For
example, if we want the image to scale with the surrounding face, we
can have a special value of the :scale attribute.  And similarly with
other attributes; we can also invent new keywords for attributes
currently not supported.

> "you want images, not character-like glyphs" isn't an argument, it's
> simply a statement that you don't want my use case to be supported.

It just means your use case is different from what is being discussed
here, and serves purposes other than those which prompted this bug
report.  I don't think we should mix them.

> I'm talking about displaying character-like glyphs.

And I'm not.  This bug report is about a different use case.

> > Within the context of what these icons are supposed to be used for, I
> > envision the images coming with bright, catchy colors that should be
> > left alone, not enhanced by color and contrast tweaking of whatever
> > happens to be the face colors the user wants.
> 
> I'm not sure which icons you're thinking about, but I certainly don't
> want my symbols to have "bright, catchy colors that should be left
> alone".

I suggest to read the above discussion, and also look at the UI
look-and-feel that is being sought.  You will see many examples there
of the kind of images that people would like to see.  I think their
vivid colors is one of the main aspects that attract users to such UI.

> I'm still not sure whether you're suggesting anything that would
> handle my use case even remotely, or simply saying it's not a use case
> that Emacs should be extensible to.

I guess I'm saying the latter.  Maybe if you explained when such use
cases arise, I will agree with you.  But in any case, it's a different
use case.

bug#40845: SVG rendering issues

[Pip Cet]

On Sun, Apr 26, 2020 at 2:38 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 26 Apr 2020 10:15:39 +0000
> > Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org
> >
> > > If we want to display an image, the logical way would be to use an
> > > image spec, which is already supported, enhancing it as needed.
> >
> > I want to display a glyph that looks different depending on its
> > textual context. That means it's not "an image", in the traditional
> > sense.
>
> Then I guess we are talking about two different use cases.

A more general one, and a more limited and specific one. As it
happens, the fix for the more general one is simpler.

> This bug
> report was filed in the context of this discussion:
>
>   https://lists.gnu.org/archive/html/emacs-devel/2020-04/msg01386.html
>
> where I said that I think such UI effects should be implemented by
> using images, not special characters and special fonts.

Indeed. You're right about that. We should use dynamically-generated
image specs for them. That's what my patch allows for.

> > and enhancing image specs to handle such context-dependent
> > glyphs is wrong, because it moves code into the image backend that
> > applies to all image backends.
>
> "Image backend" in this context is the image libraries, like librsvg
> and libpng, is that right?

Yes.

> If so, which one of them can do the stuff
> Clément reported missing?

Err, who said they could? I'm saying they _shouldn't_. It's not the
image backend's job to alter and adapt an image, just to read a
compressed version of a bitmap and uncompress it, interpreting
whatever method of compression (say, vectorization) was chosen.

> > The enhancement I propose is a layer of indirection, allowing the
> > image spec itself to be adjusted to face/font properties.
>
> The kind of adjustments that we need for the use case which prompted
> this discussion can be done inside the display engine.

As opposed to doing them when the spec is evaluated?

> > And, no, there's no way for an image backend to interpret that. We
> > have to use different image specs.
>
> A Lisp program that prepares such display can prepare the spec
> accordingly, if needed.

That's what I'm proposing: call a Lisp program to prepare a spec
whenever we display the glyph. Not more often, which is wasteful.

Except you're currently wrong: a Lisp program cannot prepare "the
spec" accordingly, because there might be several required specs in
effect at the same time, for example.

> Any effects that cannot be explicitly
> produced by Lisp and we'd need the display code to produce can be
> communicated using the image spec keywords, like we always do.  For
> example, if we want the image to scale with the surrounding face, we
> can have a special value of the :scale attribute.  And similarly with
> other attributes; we can also invent new keywords for attributes
> currently not supported.

Yes, that describes what I'm doing, except for the important point
that the spec is generated per occurence of the glyph rather than once
globally.

> > "you want images, not character-like glyphs" isn't an argument, it's
> > simply a statement that you don't want my use case to be supported.
>
> It just means your use case is different from what is being discussed
> here, and serves purposes other than those which prompted this bug
> report.  I don't think we should mix them.

It's strict containment: my use case is more general.

> > > Within the context of what these icons are supposed to be used for, I
> > > envision the images coming with bright, catchy colors that should be
> > > left alone, not enhanced by color and contrast tweaking of whatever
> > > happens to be the face colors the user wants.
> >
> > I'm not sure which icons you're thinking about, but I certainly don't
> > want my symbols to have "bright, catchy colors that should be left
> > alone".
>
> I suggest to read the above discussion, and also look at the UI
> look-and-feel that is being sought.  You will see many examples there
> of the kind of images that people would like to see.  I think their
> vivid colors is one of the main aspects that attract users to such UI.

I did read the discussion, and the main thing mentioned about their
colors is that they shouldn't be fixed, i.e. not "left alone".

> > I'm still not sure whether you're suggesting anything that would
> > handle my use case even remotely, or simply saying it's not a use case
> > that Emacs should be extensible to.
>
> I guess I'm saying the latter.

My impression is you're treating extensibility itself as something to
be avoided, not as something on the benefit side of a cost-benefit
analysis (cost: 5 lines of actual code. benefit: solves both the more
specific use case sought here and the more general use case I need).

> Maybe if you explained when such use
> cases arise, I will agree with you.  But in any case, it's a different
> use case.

A more general one.

As to how they arise: mathematicians, for example, have a
long-standing tradition of making up symbols. Those new symbols aren't
(and shouldn't be) in Unicode, and there are many other symbols that,
for example, cannot be in Unicode for legal or ethical reasons. Users
should be able to display such symbols in an Emacs buffer and have
them be as close to character glyphs (or different from them) as they
are willing to make them.

I remain convinced that I'm not smart enough to figure out in advance
the precise set of use cases I want a feature for. You shouldn't limit
generality to handle only those cases you're convinced are useful.

bug#40845: SVG rendering issues

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 26 Apr 2020 19:00:36 +0000
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org
> 
> > Then I guess we are talking about two different use cases.
> 
> A more general one, and a more limited and specific one. As it
> happens, the fix for the more general one is simpler.

"More general" in what sense?  Using Lisp does mean you could write
more general programs, but it doesn't necessarily mean you will be
able to solve more general problems.  The display code maintains a lot
of state that describes each position on the screen (see 'struct it'
in dispextern.h), and your proposal seems to expose only the font and
the face attributes to the Lisp function.  What about the other
information? are you suggesting to expose that as well?  For example,
some use cases may wish to know the pixel coordinates of the image
being rendered, or whether it's on a continued or an R2L line, or the
ascent or descent of the screen line, or the pixel distance from the
window edge, etc. etc.  The display code has all of that at its
fingertips.

> > > and enhancing image specs to handle such context-dependent
> > > glyphs is wrong, because it moves code into the image backend that
> > > applies to all image backends.
> >
> > "Image backend" in this context is the image libraries, like librsvg
> > and libpng, is that right?
> 
> Yes.

That's what I thought.  Then I don't think I understand your comment
about "moving code into the image backend", since I never proposed to
move anything into those libraries.  We need to force those of the
libraries that are capable to produce the images we want.  Like force
a certain background of an SVG image.  That is all done outside of the
backends.

> > A Lisp program that prepares such display can prepare the spec
> > accordingly, if needed.
> 
> That's what I'm proposing: call a Lisp program to prepare a spec
> whenever we display the glyph. Not more often, which is wasteful.

I meant a Lisp program that prepared the spec before redisplay was
entered.  There's no need to re-evaluate it each time the
corresponding part of the screen is updated or Emacs needs to
calculate its layout for other purposes, such as moving the cursor N
lines down on the screen.  Please consider how many times this code
will be executed, sometimes in contexts that are completely unrelated
to showing the images.

> Except you're currently wrong: a Lisp program cannot prepare "the
> spec" accordingly, because there might be several required specs in
> effect at the same time, for example.

We should identify the aspects of the images we'd like to control, and
enhance the image display to obey those aspects when needed.  This is
not complicated enough to call for a Lisp implementation, and doesn't
justify the downsides of calling Lisp from the display code.

> My impression is you're treating extensibility itself as something to
> be avoided, not as something on the benefit side of a cost-benefit
> analysis (cost: 5 lines of actual code. benefit: solves both the more
> specific use case sought here and the more general use case I need).

I indeed prefer to avoid extending the display engine in Lisp, as
explained elsewhere.  I hope I convinced you, or that at least you see
my point.  Other extensions of the display code are much more welcome,
although the resulting complexity should be carefully considered and
weighed against the advantages.  The display code is extremely complex
and handles an almost infinite number of use cases, so much so that it
sometimes takes years to find bugs in some subtle use case.  We should
try not to over-complicate the code without a good reason.  And a
theoretical "generalization" is not a good reason in my book.

> As to how they arise: mathematicians, for example, have a
> long-standing tradition of making up symbols. Those new symbols aren't
> (and shouldn't be) in Unicode, and there are many other symbols that,
> for example, cannot be in Unicode for legal or ethical reasons. Users
> should be able to display such symbols in an Emacs buffer and have
> them be as close to character glyphs (or different from them) as they
> are willing to make them.

Could you please show examples of such symbols, and tell how they are
displayed by other editors or word processors?

> I remain convinced that I'm not smart enough to figure out in advance
> the precise set of use cases I want a feature for. You shouldn't limit
> generality to handle only those cases you're convinced are useful.

I don't want to limit generality, I want to limit the price we pay for
a use case that has yet to materialize.  I think this keeps the Emacs
display code from becoming completely unmanageable.

bug#40845: SVG rendering issues

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 25 Apr 2020 14:34:11 +0000
> Cc: 40845@debbugs.gnu.org
> 
> > 3. The SVG images don't inherit the foreground of the current face; instead, they use a black foreground.
> 
> Also, IIRC, 3 is an rsvg issue.

I don't think I understand what item 3 is complaining about.  If I
visit etc/images/splash.svg, it gets the background color of the
default face.  So is the complaint that it takes the background from
the default face instead of the current face?  That should be easy to
fix, I think, as this code in svg_load_image shows:

    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);

bug#40845: SVG rendering issues

[Clément Pit-Claudel]

On 25/04/2020 11.46, Eli Zaretskii wrote:
>> From: Pip Cet <pipcet@gmail.com>
>> Date: Sat, 25 Apr 2020 14:34:11 +0000
>> Cc: 40845@debbugs.gnu.org
>>
>>> 3. The SVG images don't inherit the foreground of the current face; instead, they use a black foreground.
>>
>> Also, IIRC, 3 is an rsvg issue.
> 
> I don't think I understand what item 3 is complaining about.  If I
> visit etc/images/splash.svg, it gets the background color of the
> default face.  So is the complaint that it takes the background from
> the default face instead of the current face?  That should be easy to
> fix, I think, as this code in svg_load_image shows:

That's item 2:

  2. The SVG images don't inherit the background of the current face; instead, they inherit the background of the default face.

and you summarized it exactly right.  Item 3 is about the color of the circle that the repro draws. . It should be purple, but it's black.

Clément.

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Cc: 40845@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Sat, 25 Apr 2020 12:42:07 -0400
> 
> >>> 3. The SVG images don't inherit the foreground of the current face; instead, they use a black foreground.
> >>
> >> Also, IIRC, 3 is an rsvg issue.
> > 
> > I don't think I understand what item 3 is complaining about.  If I
> > visit etc/images/splash.svg, it gets the background color of the
> > default face.  So is the complaint that it takes the background from
> > the default face instead of the current face?  That should be easy to
> > fix, I think, as this code in svg_load_image shows:
> 
> That's item 2:
> 
>   2. The SVG images don't inherit the background of the current face; instead, they inherit the background of the default face.
> 
> and you summarized it exactly right.  Item 3 is about the color of the circle that the repro draws. . It should be purple, but it's black.

Ah, sorry.

But IMO the foreground of an image shouldn't be affected by the
current face's foreground.  Images should come with their own colors,
and be displayed like that.  I think in the context of the discussion
that led to this bug report it's actually almost a must: we want
images with attractive colors to serve as the icons, we don't want
them to be affected by whatever face happens to be nearby.

bug#40845: SVG rendering issues

[Clément Pit-Claudel]

On 25/04/2020 13.02, Eli Zaretskii wrote:
> IMO the foreground of an image shouldn't be affected by the
> current face's foreground.  Images should come with their own colors,
> and be displayed like that.  I think in the context of the discussion
> that led to this bug report it's actually almost a must: we want
> images with attractive colors to serve as the icons, we don't want
> them to be affected by whatever face happens to be nearby.

Yes, of course: if an image has a foreground color, it should be respected.
But it's not uncommon for SVG images to not include a foreground color, as shown in the repro.  In that case, the image should use the current foreground color, I think.
(of course, a :foreground keyword, if any, should take precedence; that is issue 4).

bug#40845: SVG rendering issues

[Alan Third]

On Sat, Apr 25, 2020 at 01:24:13PM -0400, Clément Pit-Claudel wrote:
> On 25/04/2020 13.02, Eli Zaretskii wrote:
> > IMO the foreground of an image shouldn't be affected by the
> > current face's foreground.  Images should come with their own colors,
> > and be displayed like that.  I think in the context of the discussion
> > that led to this bug report it's actually almost a must: we want
> > images with attractive colors to serve as the icons, we don't want
> > them to be affected by whatever face happens to be nearby.
> 
> Yes, of course: if an image has a foreground color, it should be
> respected. But it's not uncommon for SVG images to not include a
> foreground color, as shown in the repro. In that case, the image
> should use the current foreground color, I think. (of course, a
> :foreground keyword, if any, should take precedence; that is issue
> 4).

Lars fixed the foreground issue for eww by wrapping svg files in
another svg. See svg--wrap-svg in shr.el (bug#37159).

I think this is the only practical way to handle svg files with no
foreground colour set. To do this when loading _any_ svg we’d probably
have to move it into create-image or image.c.
-- 
Alan Third

bug#40845: SVG rendering issues

[Pip Cet]

On Sat, Apr 25, 2020 at 5:46 PM Alan Third <alan@idiocy.org> wrote:
> On Sat, Apr 25, 2020 at 01:24:13PM -0400, Clément Pit-Claudel wrote:
> > Yes, of course: if an image has a foreground color, it should be
> > respected. But it's not uncommon for SVG images to not include a
> > foreground color, as shown in the repro. In that case, the image
> > should use the current foreground color, I think. (of course, a
> > :foreground keyword, if any, should take precedence; that is issue
> > 4).

For reference, the relevant chunk of librsvg source code is this:

// https://www.w3.org/TR/SVG/color.html#ColorProperty
make_property!(
    ComputedValues,
    Color,
    // The SVG spec allows the user agent to choose its own default
for the "color" property.
    // We don't allow passing in an initial CSS in the public API, so
we'll start with black.
    //
    // See https://bugzilla.gnome.org/show_bug.cgi?id=764808 for a
case where this would
    // be useful - rendering equations with currentColor, so they take
on the color of the
    // surrounding text.
    default: cssparser::RGBA::new(0, 0, 0, 0xff),
    inherits_automatically: true,
    newtype_parse: cssparser::RGBA,
);

> Lars fixed the foreground issue for eww by wrapping svg files in
> another svg. See svg--wrap-svg in shr.el (bug#37159).
>
> I think this is the only practical way to handle svg files with no
> foreground colour set. To do this when loading _any_ svg we’d probably
> have to move it into create-image or image.c.

I think it's a neat hack, but it's just one of the more obvious ways
of working around an annoying limitation of librsvg (Ideally, we'd use
a library without such a limitation by default and fall back to
modifying/wrapping SVGs when using a limited library.)

For applications which change glyphs' foreground colors a lot, it
might be preferrable to coax rsvg into providing an alpha-only map for
the foreground plus an RGBA map for the background and blend them
together, and that approach would work for other image types.

bug#40845: SVG rendering issues

[Alan Third]

On Sat, Apr 25, 2020 at 06:46:51PM +0100, Alan Third wrote:
> On Sat, Apr 25, 2020 at 01:24:13PM -0400, Clément Pit-Claudel wrote:
> > On 25/04/2020 13.02, Eli Zaretskii wrote:
> > > IMO the foreground of an image shouldn't be affected by the
> > > current face's foreground.  Images should come with their own colors,
> > > and be displayed like that.  I think in the context of the discussion
> > > that led to this bug report it's actually almost a must: we want
> > > images with attractive colors to serve as the icons, we don't want
> > > them to be affected by whatever face happens to be nearby.
> > 
> > Yes, of course: if an image has a foreground color, it should be
> > respected. But it's not uncommon for SVG images to not include a
> > foreground color, as shown in the repro. In that case, the image
> > should use the current foreground color, I think. (of course, a
> > :foreground keyword, if any, should take precedence; that is issue
> > 4).
> 
> Lars fixed the foreground issue for eww by wrapping svg files in
> another svg. See svg--wrap-svg in shr.el (bug#37159).
> 
> I think this is the only practical way to handle svg files with no
> foreground colour set. To do this when loading _any_ svg we’d probably
> have to move it into create-image or image.c.

FWIW, with some experimentation I’ve found that this wrapping method
can easily sort the scaling problems too.

The process would have to go, roughly:

 * load the svg code
 * parse it with librsvg
 * extract the size
 * calculate the new size (compute_image_size in image.c should work
   fine)
 * base64 encode the original svg
 * wrap it in the new svg code
 * parse
 * produce the bitmap

Then we’re probably as well skipping all the matrix calculation stuff,
the same as we do for ImageMagick, as the bitmap should be the exact
size we want.

I’m happy to give this a go, but I’m not sure about base64 encoding.
Do we already have a way of doing that to a char buffer?
-- 
Alan Third

bug#40845: SVG rendering issues

[Clément Pit-Claudel]

On 26/04/2020 17.17, Alan Third wrote:
> I’m happy to give this a go, but I’m not sure about base64 encoding.
> Do we already have a way of doing that to a char buffer?

We have base64-encode-region and base64-encode-string, which are both C functions — but why do we need to base64 encode?

bug#40845: SVG rendering issues

[Alan Third]

On Sun, Apr 26, 2020 at 06:48:09PM -0400, Clément Pit-Claudel wrote:
> On 26/04/2020 17.17, Alan Third wrote:
> > I’m happy to give this a go, but I’m not sure about base64 encoding.
> > Do we already have a way of doing that to a char buffer?
> 
> We have base64-encode-region and base64-encode-string, which are
> both C functions — but why do we need to base64 encode?

I could be wrong, but in order to wrap the SVG file in another SVG, we
need to do something like:

    <svg xmlns:xlink="http://www.w3.org/1999/xlink"
         xmlns:xi="http://www.w3.org/2001/XInclude"
         width="100" height="100"
         preserveAspectRatio="none"
         viewBox="0 0 283.465 283.465">
      <xi:include href="data:image/svg+xml;base64,<base64 encoded data>">
      </xi:include>
    </svg>

We can’t just put the contents in directly, like:

    <svg xmlns:xlink="http://www.w3.org/1999/xlink"
         width="100" height="100"
         preserveAspectRatio="none"
         viewBox="0 0 283.465 283.465">
      <!-- file contents -->   
      <svg>
         stuff
      </svg>
      <!-- end file contents -->
    </svg>

as the file may (and probably should) have XML declarations at the
start which break the SVG file. We could strip them out but I’m not
sure that that’s better than base64 encoding, as it would mean
modifying the contents and I’m not sure that’s the only thing that
might cause problems.

We could also potentially just insert the file as‐is like so:

    <svg xmlns:xlink="http://www.w3.org/1999/xlink"
         xmlns:xi="http://www.w3.org/2001/XInclude"
         width="100" height="100"
         preserveAspectRatio="none"
         viewBox="0 0 283.465 283.465">
      <xi:include href="data:image/svg+xml;utf8,<svg>stuff</svg>">
      </xi:include>
    </svg>

but I can’t make that work. I think we’d have to URL encode the angle
brackets and any quotes.

If there’s anything obvious I’ve missed, please let me know.
-- 
Alan Third

bug#40845: SVG rendering issues

[Clément Pit-Claudel]

On 27/04/2020 11.22, Alan Third wrote:
> On Sun, Apr 26, 2020 at 06:48:09PM -0400, Clément Pit-Claudel wrote:
>> On 26/04/2020 17.17, Alan Third wrote:
>>> I’m happy to give this a go, but I’m not sure about base64 encoding.
>>> Do we already have a way of doing that to a char buffer?
>>
>> We have base64-encode-region and base64-encode-string, which are
>> both C functions — but why do we need to base64 encode?
> 
> We can’t just put the contents in directly […]
> as the file may (and probably should) have XML declarations at the
> start which break the SVG file. We could strip them out but I’m not
> sure that that’s better than base64 encoding, as it would mean
> modifying the contents and I’m not sure that’s the only thing that
> might cause problems.

Excellent point, I hadn't considered the xml declaration at the top.

bug#40845: SVG rendering issues

[Alan Third]

On Sun, Apr 26, 2020 at 06:48:09PM -0400, Clément Pit-Claudel wrote:
> On 26/04/2020 17.17, Alan Third wrote:
> > I’m happy to give this a go, but I’m not sure about base64 encoding.
> > Do we already have a way of doing that to a char buffer?
> 
> We have base64-encode-region and base64-encode-string, which are both C functions — but why do we need to base64 encode?

I’ve run into a problem with the base64 encoding.

/usr/bin/base64 encodes this file differently from Emacs:

    https://upload.wikimedia.org/wikipedia/commons/9/92/MixedUnitAddition.svg

librsvg doesn’t like the way Emacs encodes the file, but is fine with
the system base64.

I think the problem is the GBP £ symbol on line 39.

Do I need to do something special, like set up character encodings?
-- 
Alan Third

bug#40845: SVG rendering issues

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> I’ve run into a problem with the base64 encoding.
>
> /usr/bin/base64 encodes this file differently from Emacs:
>
>     https://upload.wikimedia.org/wikipedia/commons/9/92/MixedUnitAddition.svg
>
> librsvg doesn’t like the way Emacs encodes the file, but is fine with
> the system base64.
>
> I think the problem is the GBP £ symbol on line 39.
>
> Do I need to do something special, like set up character encodings?

Before base64-encoding, you should encode the data to whatever coding
system is expected, which would probably be utf-8 here.

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

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sun, 3 May 2020 15:13:48 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 40845@debbugs.gnu.org, pipcet@gmail.com
> 
> I’ve run into a problem with the base64 encoding.
> 
> /usr/bin/base64 encodes this file differently from Emacs:
> 
>     https://upload.wikimedia.org/wikipedia/commons/9/92/MixedUnitAddition.svg
> 
> librsvg doesn’t like the way Emacs encodes the file, but is fine with
> the system base64.
> 
> I think the problem is the GBP £ symbol on line 39.
> 
> Do I need to do something special, like set up character encodings?

You need to make sure the text passed to base64-encode-region is
unibyte.  I suggest to look at how other packages call that function.

Let me know if this general advice doesn't help.

Thanks.

bug#40845: SVG rendering issues

[Alan Third]

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

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

Yes, it’s sorted the problem. Thanks!

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

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

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

-- 
Alan Third

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

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

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

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

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sun, 3 May 2020 17:24:17 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> 
> I’ve attached a first pass. It solves 1 and 4 from the original bug
> report: scaling and setting the foreground colour.

Thanks.

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

Can you describe the difficulty in using the current face?

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

It's okay to call malloc and free in image.c.

bug#40845: SVG rendering issues

[Alan Third]

On Sun, May 03, 2020 at 07:49:45PM +0300, Eli Zaretskii wrote:
> > Date: Sun, 3 May 2020 17:24:17 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> > 
> > One thing is that it gets the foreground colour from the default face
> > instead of the current face. I’m not sure how to solve that.
> 
> Can you describe the difficulty in using the current face?

I’m mostly unsure how I access it. I don’t think images or image.c
have any real concept of where they are in the buffer.

I think we’d also have to recreate the image any time the face
changed. For example in Clement’s test code from the original bug
report, he has the face changing when the mouse hovers over the text.
In order to handle that case we would need to create a new version of
the image with the updated foreground and background when the mouse
moves over it.

We can work around having to update the background by using proper
transparency, but I don’t know what other problems that might
introduce. It’s harder for the foreground. Pip Cet suggested creating
masks for foreground and background and using them to draw the image
in sections, but I feel that’s perhaps getting a little complex,
especially when I think most images won’t be changing their foreground
and background colours.

I feel it would be easier just to have two or more cached images with
different foreground and backgrounds as required.

> > I’m unsure if my use of malloc is OK, as I know Emacs code uses some
> > different methods in different places.
> 
> It's okay to call malloc and free in image.c.

Excellent, thanks.
-- 
Alan Third

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sun, 3 May 2020 19:38:39 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> 
> > > One thing is that it gets the foreground colour from the default face
> > > instead of the current face. I’m not sure how to solve that.
> > 
> > Can you describe the difficulty in using the current face?
> 
> I’m mostly unsure how I access it. I don’t think images or image.c
> have any real concept of where they are in the buffer.

When the 'load' method (in your case, svg_load) is called from
prepare_image_for_display, we have the face information handy.
Usually, at that point we don't call the 'load' method, because the
image is already loaded and cached.  But we could change the logic if
we know that the image must be modified to have a different face.
Then we can pass the color information to the 'load' method.

> I think we’d also have to recreate the image any time the face
> changed.

Yes, because changing the color(s) makes a different image.  But that
is not a problem, I think.

> I feel it would be easier just to have two or more cached images with
> different foreground and backgrounds as required.

Agreed.  At least as the first approximation; we could always change
that later if we find this not to be good enough.

bug#40845: SVG rendering issues

[Alan Third]

On Sun, May 03, 2020 at 05:24:17PM +0100, Alan Third wrote:
> 
> I’ve attached a first pass. It solves 1 and 4 from the original bug
> report: scaling and setting the foreground colour.

Can someone please try the patch from the previous email on a non‐NS
platform and confirm whether or not the background colour in the image
matches the background colour of the text?

They don’t match here, but I strongly suspect that’s because the NS
port uses different colour spaces for images and everything else.
-- 
Alan Third

bug#40845: SVG rendering issues

[Alan Third]

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

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

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

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

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

---
 lisp/net/shr.el  |  17 -----
 src/dispextern.h |   2 +-
 src/gtkutil.c    |   2 +-
 src/image.c      | 179 ++++++++++++++++++++++++++++++++++++-----------
 src/nsmenu.m     |   2 +-
 src/xdisp.c      |   6 +-
 6 files changed, 145 insertions(+), 63 deletions(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 1f80ab74db..c862cf04e3 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -1195,25 +1195,8 @@ shr-parse-image-data
             ;; that are non-ASCII.
 	    (shr-dom-to-xml
 	     (libxml-parse-xml-region (point) (point-max)) 'utf-8)))
-    ;; SVG images often do not have a specified foreground/background
-    ;; color, so wrap them in styles.
-    (when (and (display-images-p)
-               (eq content-type 'image/svg+xml))
-      (setq data (svg--wrap-svg data)))
     (list data content-type)))
 
-(defun svg--wrap-svg (data)
-  "Add a default foreground colour to SVG images."
-  (let ((size (image-size (create-image data nil t :scaling 1) t)))
-    (with-temp-buffer
-      (insert
-       (format
-        "<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\" xmlns:xi=\"http://www.w3.org/2001/XInclude\" style=\"color: %s;\" viewBox=\"0 0 %d %d\"> <xi:include href=\"data:image/svg+xml;base64,%s\"></xi:include></svg>"
-        (face-foreground 'default)
-        (car size) (cdr size)
-        (base64-encode-string data t)))
-      (buffer-string))))
-
 (defun shr-image-displayer (content-function)
   "Return a function to display an image.
 CONTENT-FUNCTION is a function to retrieve an image for a cid url that
diff --git a/src/dispextern.h b/src/dispextern.h
index 0b1f3d14ae..cd09e0846c 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3475,7 +3475,7 @@ #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
 void mark_image_cache (struct image_cache *);
 bool valid_image_p (Lisp_Object);
 void prepare_image_for_display (struct frame *, struct image *);
-ptrdiff_t lookup_image (struct frame *, Lisp_Object);
+ptrdiff_t lookup_image (struct frame *, Lisp_Object, int face_id);
 
 #if defined HAVE_X_WINDOWS || defined USE_CAIRO || defined HAVE_NS
 #define RGB_PIXEL_COLOR unsigned long
diff --git a/src/gtkutil.c b/src/gtkutil.c
index 681f86f51b..2f6b664d2d 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -5101,7 +5101,7 @@ update_frame_tool_bar (struct frame *f)
           else
             idx = -1;
 
-          img_id = lookup_image (f, image);
+          img_id = lookup_image (f, image, -1);
           img = IMAGE_FROM_ID (f, img_id);
           prepare_image_for_display (f, img);
 
diff --git a/src/image.c b/src/image.c
index c8a192aaaf..5333d66c7d 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1066,7 +1066,7 @@ DEFUN ("image-size", Fimage_size, Simage_size, 1, 3, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       int width = img->width + 2 * img->hmargin;
       int height = img->height + 2 * img->vmargin;
@@ -1096,7 +1096,7 @@ DEFUN ("image-mask-p", Fimage_mask_p, Simage_mask_p, 1, 2, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       if (img->mask)
 	mask = Qt;
@@ -1119,7 +1119,7 @@ DEFUN ("image-metadata", Fimage_metadata, Simage_metadata, 1, 2, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       ext = img->lisp_data;
     }
@@ -1586,7 +1586,8 @@ make_image_cache (void)
 /* Find an image matching SPEC in the cache, and return it.  If no
    image is found, return NULL.  */
 static struct image *
-search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
+search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
+                    unsigned long foreground, unsigned long background)
 {
   struct image *img;
   struct image_cache *c = FRAME_IMAGE_CACHE (f);
@@ -1609,8 +1610,8 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
   for (img = c->buckets[i]; img; img = img->next)
     if (img->hash == hash
 	&& !NILP (Fequal (img->spec, spec))
-	&& img->frame_foreground == FRAME_FOREGROUND_PIXEL (f)
-	&& img->frame_background == FRAME_BACKGROUND_PIXEL (f))
+	&& img->frame_foreground == foreground
+	&& img->frame_background == background)
       break;
   return img;
 }
@@ -1621,7 +1622,7 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
 static void
 uncache_image (struct frame *f, Lisp_Object spec)
 {
-  struct image *img = search_image_cache (f, spec, sxhash (spec));
+  struct image *img = search_image_cache (f, spec, sxhash (spec), FRAME_FOREGROUND_PIXEL (f), FRAME_BACKGROUND_PIXEL (f));
   if (img)
     {
       free_image (f, img);
@@ -2108,7 +2109,17 @@ image_set_transform (struct frame *f, struct image *img)
 
   /* Determine size.  */
   int width, height;
-  compute_image_size (img->width, img->height, img->spec, &width, &height);
+
+#ifdef HAVE_RSVG
+  /* SVGs are pre-scaled to the correct size.  */
+  if (EQ (image_spec_value (img->spec, QCtype, NULL), Qsvg))
+    {
+      width = img->width;
+      height = img->height;
+    }
+  else
+#endif
+    compute_image_size (img->width, img->height, img->spec, &width, &height);
 
   /* Determine rotation.  */
   double rotation = 0.0;
@@ -2275,11 +2286,15 @@ image_set_transform (struct frame *f, struct image *img)
    SPEC must be a valid Lisp image specification (see valid_image_p).  */
 
 ptrdiff_t
-lookup_image (struct frame *f, Lisp_Object spec)
+lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 {
   struct image *img;
   EMACS_UINT hash;
 
+  struct face *face = (face_id >= 0) ? FACE_FROM_ID (f, face_id) : FRAME_DEFAULT_FACE (f);
+  unsigned long foreground = FACE_COLOR_TO_PIXEL (face->foreground, f);
+  unsigned long background = FACE_COLOR_TO_PIXEL (face->background, f);
+
   /* F must be a window-system frame, and SPEC must be a valid image
      specification.  */
   eassert (FRAME_WINDOW_P (f));
@@ -2287,7 +2302,7 @@ lookup_image (struct frame *f, Lisp_Object spec)
 
   /* Look up SPEC in the hash table of the image cache.  */
   hash = sxhash (spec);
-  img = search_image_cache (f, spec, hash);
+  img = search_image_cache (f, spec, hash, foreground, background);
   if (img && img->load_failed_p)
     {
       free_image (f, img);
@@ -2300,9 +2315,9 @@ lookup_image (struct frame *f, Lisp_Object spec)
       block_input ();
       img = make_image (spec, hash);
       cache_image (f, img);
+      img->frame_foreground = foreground;
+      img->frame_background = background;
       img->load_failed_p = ! img->type->load (f, img);
-      img->frame_foreground = FRAME_FOREGROUND_PIXEL (f);
-      img->frame_background = FRAME_BACKGROUND_PIXEL (f);
 
       /* If we can't load the image, and we don't have a width and
 	 height, use some arbitrary width and height so that we can
@@ -9393,6 +9408,7 @@ DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
   SVG_ALGORITHM,
   SVG_HEURISTIC_MASK,
   SVG_MASK,
+  SVG_FOREGROUND,
   SVG_BACKGROUND,
   SVG_LAST
 };
@@ -9411,6 +9427,7 @@ DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
   {":conversion",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
   {":heuristic-mask",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
   {":mask",		IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":foreground",	IMAGE_STRING_OR_NIL_VALUE,		0},
   {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
 };
 
@@ -9675,6 +9692,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   int height;
   const guint8 *pixels;
   int rowstride;
+  char *wrapped_contents;
+  ptrdiff_t wrapped_size;
 
 #if ! GLIB_CHECK_VERSION (2, 36, 0)
   /* g_type_init is a glib function that must be called prior to
@@ -9682,6 +9701,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   g_type_init ();
 #endif
 
+  /* Parse the unmodified SVG data so we can get it's initial size.  */
+
 #if LIBRSVG_CHECK_VERSION (2, 32, 0)
   GInputStream *input_stream
     = g_memory_input_stream_new_from_data (contents, size, NULL);
@@ -9710,6 +9731,107 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   rsvg_handle_write (rsvg_handle, (unsigned char *) contents, size, &err);
   if (err) goto rsvg_error;
 
+  /* The parsing is complete, rsvg_handle is ready to used, close it
+     for further writes.  */
+  rsvg_handle_close (rsvg_handle, &err);
+  if (err) goto rsvg_error;
+#endif
+
+  /* Get the image dimensions.  */
+  rsvg_handle_get_dimensions (rsvg_handle, &dimension_data);
+
+  /* We are now done with the unmodified data.  */
+  g_object_unref (rsvg_handle);
+
+  /* Calculate the final image size.  */
+  compute_image_size (dimension_data.width, dimension_data.height,
+                      img->spec, &width, &height);
+
+  /* Wrap the SVG data in another SVG.  This allows us to set the
+     width and height, as well as modify the foreground and background
+     colors.  */
+  {
+    Lisp_Object value;
+    unsigned long foreground = img->frame_foreground;
+    unsigned long background = img->frame_background;
+
+    Lisp_Object encoded_contents = Fbase64_encode_string
+      (make_unibyte_string (contents, size), Qt);
+
+    /* The wrapper sets the foreground color, width and height, and
+       viewBox must contain the dimensions of the original image.  It
+       also draws a rectangle over the whole space, set to the
+       background color, before including the original image. This
+       acts to set the background color, instead of leaving it
+       transparent.  */
+    const char *wrapper =
+      "<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\" "
+      "xmlns:xi=\"http://www.w3.org/2001/XInclude\" "
+      "style=\"color: #%06X; fill: currentColor;\" "
+      "width=\"%d\" height=\"%d\" preserveAspectRatio=\"none\" "
+      "viewBox=\"0 0 %d %d\">"
+      "<rect width=\"100%%\" height=\"100%%\" fill=\"#%06X\"/>"
+      "<xi:include href=\"data:image/svg+xml;base64,%s\"></xi:include>"
+      "</svg>";
+
+    /* FIXME: I've added 64 in the hope it will cover the size of the
+       width and height strings and things.  */
+    int buffer_size = SBYTES (encoded_contents) + strlen (wrapper) + 64;
+
+    value = image_spec_value (img->spec, QCforeground, NULL);
+    if (!NILP (value))
+      {
+        foreground = image_alloc_image_color (f, img, value, img->frame_foreground);
+      }
+    value = image_spec_value (img->spec, QCbackground, NULL);
+    if (!NILP (value))
+      {
+        background = image_alloc_image_color (f, img, value, img->frame_background);
+        img->background = background;
+        img->background_valid = 1;
+      }
+
+    wrapped_contents = malloc (buffer_size);
+
+    if (!wrapped_contents
+        || buffer_size <= snprintf (wrapped_contents, buffer_size, wrapper,
+                                    foreground & 0xFFFFFF, width, height,
+                                    dimension_data.width, dimension_data.height,
+                                    background & 0xFFFFFF, SSDATA (encoded_contents)))
+      goto rsvg_error;
+
+    wrapped_size = strlen (wrapped_contents);
+  }
+
+  /* Now we parse the wrapped version.  */
+
+#if LIBRSVG_CHECK_VERSION (2, 32, 0)
+  input_stream = g_memory_input_stream_new_from_data (wrapped_contents, wrapped_size, NULL);
+  base_file = filename ? g_file_new_for_path (filename) : NULL;
+  rsvg_handle = rsvg_handle_new_from_stream_sync (input_stream, base_file,
+						  RSVG_HANDLE_FLAGS_NONE,
+						  NULL, &err);
+  if (base_file)
+    g_object_unref (base_file);
+  g_object_unref (input_stream);
+
+  /* Check rsvg_handle too, to avoid librsvg 2.40.13 bug (Bug#36773#26).  */
+  if (!rsvg_handle || err) goto rsvg_error;
+#else
+  /* Make a handle to a new rsvg object.  */
+  rsvg_handle = rsvg_handle_new ();
+  eassume (rsvg_handle);
+
+  /* Set base_uri for properly handling referenced images (via 'href').
+     See rsvg bug 596114 - "image refs are relative to curdir, not .svg file"
+     <https://gitlab.gnome.org/GNOME/librsvg/issues/33>. */
+  if (filename)
+    rsvg_handle_set_base_uri (rsvg_handle, filename);
+
+  /* Parse the contents argument and fill in the rsvg_handle.  */
+  rsvg_handle_write (rsvg_handle, (unsigned char *) wrapped_contents, wrapped_size, &err);
+  if (err) goto rsvg_error;
+
   /* The parsing is complete, rsvg_handle is ready to used, close it
      for further writes.  */
   rsvg_handle_close (rsvg_handle, &err);
@@ -9728,6 +9850,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   pixbuf = rsvg_handle_get_pixbuf (rsvg_handle);
   if (!pixbuf) goto rsvg_error;
   g_object_unref (rsvg_handle);
+  free (wrapped_contents);
 
   /* Extract some meta data from the svg handle.  */
   width     = gdk_pixbuf_get_width (pixbuf);
@@ -9752,25 +9875,6 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 
     init_color_table ();
 
-    /* Handle alpha channel by combining the image with a background
-       color.  */
-    Emacs_Color background;
-    Lisp_Object specified_bg = image_spec_value (img->spec, QCbackground, NULL);
-    if (!STRINGP (specified_bg)
-	|| !FRAME_TERMINAL (f)->defined_color_hook (f,
-                                                    SSDATA (specified_bg),
-                                                    &background,
-                                                    false,
-                                                    false))
-      FRAME_TERMINAL (f)->query_frame_background_color (f, &background);
-
-    /* SVG pixmaps specify transparency in the last byte, so right
-       shift 8 bits to get rid of it, since emacs doesn't support
-       transparency.  */
-    background.red   >>= 8;
-    background.green >>= 8;
-    background.blue  >>= 8;
-
     /* This loop handles opacity values, since Emacs assumes
        non-transparent images.  Each pixel must be "flattened" by
        calculating the resulting color, given the transparency of the
@@ -9784,14 +9888,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 	    int blue    = *pixels++;
 	    int opacity = *pixels++;
 
-	    red   = ((red * opacity)
-		     + (background.red * ((1 << 8) - opacity)));
-	    green = ((green * opacity)
-		     + (background.green * ((1 << 8) - opacity)));
-	    blue  = ((blue * opacity)
-		     + (background.blue * ((1 << 8) - opacity)));
-
-	    PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
+	    PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red << 8, green << 8, blue << 8));
 	  }
 
 	pixels += rowstride - 4 * width;
@@ -9821,6 +9918,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
  rsvg_error:
   if (rsvg_handle)
     g_object_unref (rsvg_handle);
+  if (wrapped_contents)
+    free (wrapped_contents);
   /* FIXME: Use error->message so the user knows what is the actual
      problem with the image.  */
   image_error ("Error parsing SVG image `%s'", img->spec);
@@ -10119,7 +10218,7 @@ DEFUN ("lookup-image", Flookup_image, Slookup_image, 1, 1, 0,
   ptrdiff_t id = -1;
 
   if (valid_image_p (spec))
-    id = lookup_image (SELECTED_FRAME (), spec);
+    id = lookup_image (SELECTED_FRAME (), spec, -1);
 
   debug_print (spec);
   return make_fixnum (id);
diff --git a/src/nsmenu.m b/src/nsmenu.m
index b7e4cbd565..e313fc03f4 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -1092,7 +1092,7 @@ - (Lisp_Object)runMenuAt: (NSPoint)p forFrame: (struct frame *)f
           continue;
         }
 
-      img_id = lookup_image (f, image);
+      img_id = lookup_image (f, image, -1);
       img = IMAGE_FROM_ID (f, img_id);
       prepare_image_for_display (f, img);
 
diff --git a/src/xdisp.c b/src/xdisp.c
index 140d134572..f05fc4cb37 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -5690,7 +5690,7 @@ handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
       else
 	{
 	  it->what = IT_IMAGE;
-	  it->image_id = lookup_image (it->f, value);
+	  it->image_id = lookup_image (it->f, value, it->face_id);
 	  it->position = start_pos;
 	  it->object = NILP (object) ? it->w->contents : object;
 	  it->method = GET_FROM_IMAGE;
@@ -22379,7 +22379,7 @@ push_prefix_prop (struct it *it, Lisp_Object prop)
   else if (IMAGEP (prop))
     {
       it->what = IT_IMAGE;
-      it->image_id = lookup_image (it->f, prop);
+      it->image_id = lookup_image (it->f, prop, it->face_id);
       it->method = GET_FROM_IMAGE;
     }
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -27262,7 +27262,7 @@ calc_pixel_width_or_height (double *res, struct it *it, Lisp_Object prop,
 	  if (FRAME_WINDOW_P (it->f)
 	      && valid_image_p (prop))
 	    {
-	      ptrdiff_t id = lookup_image (it->f, prop);
+	      ptrdiff_t id = lookup_image (it->f, prop, it->face_id);
 	      struct image *img = IMAGE_FROM_ID (it->f, id);
 
 	      return OK_PIXELS (width_p ? img->width : img->height);
-- 
2.26.1

bug#40845: SVG rendering issues

[Eli Zaretskii]

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

I didn't yet have time to try that, but I haven't forgot.  Hope the
current tsunami on emacs-devel will soon calm down, and I will have
more time.

bug#40845: SVG rendering issues

[Alan Third]

On Fri, May 15, 2020 at 02:09:56PM +0300, Eli Zaretskii wrote:
> > Date: Sat, 9 May 2020 20:54:15 +0100
> > From: Alan Third <alan@idiocy.org>
> > 
> > On Sat, May 09, 2020 at 03:27:27PM +0100, Alan Third wrote:
> > > On Sun, May 03, 2020 at 05:24:17PM +0100, Alan Third wrote:
> > > > 
> > > > I’ve attached a first pass. It solves 1 and 4 from the original bug
> > > > report: scaling and setting the foreground colour.
> > > 
> > > Can someone please try the patch from the previous email on a non‐NS
> > > platform and confirm whether or not the background colour in the image
> > > matches the background colour of the text?
> > > 
> > > They don’t match here, but I strongly suspect that’s because the NS
> > > port uses different colour spaces for images and everything else.
> > 
> > Apologies, please can someone try the attached patch.
> 
> I didn't yet have time to try that, but I haven't forgot.  Hope the
> current tsunami on emacs-devel will soon calm down, and I will have
> more time.

Take as long as you need, I'm in no great hurry. I still need to work
out how to calculate whether I should be using the mouse face and
decide exactly how to handle flushing an image from the cache as
it's somewhat broken in that patch.
-- 
Alan Third

bug#40845: SVG rendering issues

[Alan Third]

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

On Fri, May 15, 2020 at 11:40:53PM +0200, Alan Third wrote:
> 
> Take as long as you need, I'm in no great hurry. I still need to work
> out how to calculate whether I should be using the mouse face and
> decide exactly how to handle flushing an image from the cache as
> it's somewhat broken in that patch.

I still don't know how to use the mouse face. I couldn't see any way
to detect if it's in use when we first load the image in xdisp.c.

Most likely I've missed something, but if not then I worry that the
easiest fix may be to actually support transparency when we draw to
the screen instead of generating background colours when we load the
images. I suspect we do it the way we do because it would have been
slow on older machines, but modern machines should be able to handle
it quickly via XRender and friends.

As for the cache I decided to just delete all images that match the
image spec, no matter what colours they use. It seems to me to be the
safest option, as we don't want users flushing an image from the
cache to display an updated version, only for the old one to reappear
when the face changes.
-- 
Alan Third

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

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

* test/manual/image-transforms-tests.el: Replace hard-coded colors
with defaults.
* src/dispextern.h (struct image):
* src/image.c (search_image_cache):
(xbm_load_image):
(xbm_load):
(pbm_load): Rename from frame to face where relevant.
(svg_load_image): Parse the image to find out the size, then wrap it
in another SVG to set a new size and colors, etc.
(lookup_image): Use the face colors instead of the frame colors.
(search_image_cache): Add ability to ignore the face colors.
(uncache_image): Uncache all copies of the image that share the spec,
even if the face colors don't match.
---
 lisp/net/shr.el                       |  17 ---
 src/dispextern.h                      |   6 +-
 src/gtkutil.c                         |   2 +-
 src/image.c                           | 205 ++++++++++++++++++++------
 src/nsmenu.m                          |   2 +-
 src/xdisp.c                           |   6 +-
 test/manual/image-transforms-tests.el |  50 +++----
 7 files changed, 189 insertions(+), 99 deletions(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index ddd8112721..24595301b7 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -1209,25 +1209,8 @@ shr-parse-image-data
             ;; that are non-ASCII.
 	    (shr-dom-to-xml
 	     (libxml-parse-xml-region (point) (point-max)) 'utf-8)))
-    ;; SVG images often do not have a specified foreground/background
-    ;; color, so wrap them in styles.
-    (when (and (display-images-p)
-               (eq content-type 'image/svg+xml))
-      (setq data (svg--wrap-svg data)))
     (list data content-type)))
 
-(defun svg--wrap-svg (data)
-  "Add a default foreground colour to SVG images."
-  (let ((size (image-size (create-image data nil t :scaling 1) t)))
-    (with-temp-buffer
-      (insert
-       (format
-        "<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\" xmlns:xi=\"http://www.w3.org/2001/XInclude\" style=\"color: %s;\" viewBox=\"0 0 %d %d\"> <xi:include href=\"data:image/svg+xml;base64,%s\"></xi:include></svg>"
-        (face-foreground 'default)
-        (car size) (cdr size)
-        (base64-encode-string data t)))
-      (buffer-string))))
-
 (defun shr-image-displayer (content-function)
   "Return a function to display an image.
 CONTENT-FUNCTION is a function to retrieve an image for a cid url that
diff --git a/src/dispextern.h b/src/dispextern.h
index 311867a0c8..88bbb71fb9 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3056,9 +3056,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
      if necessary.  */
   unsigned long background;
 
-  /* Foreground and background colors of the frame on which the image
+  /* Foreground and background colors of the face on which the image
      is created.  */
-  unsigned long frame_foreground, frame_background;
+  unsigned long face_foreground, face_background;
 
   /* True if this image has a `transparent' background -- that is, is
      uses an image mask.  The accessor macro for this is
@@ -3475,7 +3475,7 @@ #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
 void mark_image_cache (struct image_cache *);
 bool valid_image_p (Lisp_Object);
 void prepare_image_for_display (struct frame *, struct image *);
-ptrdiff_t lookup_image (struct frame *, Lisp_Object);
+ptrdiff_t lookup_image (struct frame *, Lisp_Object, int face_id);
 
 #if defined HAVE_X_WINDOWS || defined USE_CAIRO || defined HAVE_NS
 #define RGB_PIXEL_COLOR unsigned long
diff --git a/src/gtkutil.c b/src/gtkutil.c
index 1fe160acca..fafd94c0f7 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -5113,7 +5113,7 @@ update_frame_tool_bar (struct frame *f)
           else
             idx = -1;
 
-          img_id = lookup_image (f, image);
+          img_id = lookup_image (f, image, -1);
           img = IMAGE_FROM_ID (f, img_id);
           prepare_image_for_display (f, img);
 
diff --git a/src/image.c b/src/image.c
index 123de54ba2..297278ec9a 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1081,7 +1081,7 @@ DEFUN ("image-size", Fimage_size, Simage_size, 1, 3, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       int width = img->width + 2 * img->hmargin;
       int height = img->height + 2 * img->vmargin;
@@ -1111,7 +1111,7 @@ DEFUN ("image-mask-p", Fimage_mask_p, Simage_mask_p, 1, 2, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       if (img->mask)
 	mask = Qt;
@@ -1134,7 +1134,7 @@ DEFUN ("image-metadata", Fimage_metadata, Simage_metadata, 1, 2, 0,
   if (valid_image_p (spec))
     {
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
+      ptrdiff_t id = lookup_image (f, spec, -1);
       struct image *img = IMAGE_FROM_ID (f, id);
       ext = img->lisp_data;
     }
@@ -1611,7 +1611,9 @@ equal_lists (Lisp_Object a, Lisp_Object b)
 /* Find an image matching SPEC in the cache, and return it.  If no
    image is found, return NULL.  */
 static struct image *
-search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
+search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
+                    unsigned long foreground, unsigned long background,
+                    bool ignore_colors)
 {
   struct image *img;
   struct image_cache *c = FRAME_IMAGE_CACHE (f);
@@ -1634,8 +1636,8 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
   for (img = c->buckets[i]; img; img = img->next)
     if (img->hash == hash
 	&& equal_lists (img->spec, spec)
-	&& img->frame_foreground == FRAME_FOREGROUND_PIXEL (f)
-	&& img->frame_background == FRAME_BACKGROUND_PIXEL (f))
+	&& (ignore_colors || (img->face_foreground == foreground
+                              && img->face_background == background)))
       break;
   return img;
 }
@@ -1646,8 +1648,13 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
 static void
 uncache_image (struct frame *f, Lisp_Object spec)
 {
-  struct image *img = search_image_cache (f, spec, sxhash (spec));
-  if (img)
+  struct image *img;
+
+  /* Because the background colors are based on the current face, we
+     can have multiple copies of an image with the same spec. We want
+     to remove them all to ensure the user doesn't see an old version
+     of the image when the face changes.  */
+  while ((img = search_image_cache (f, spec, sxhash (spec), 0, 0, true)))
     {
       free_image (f, img);
       /* As display glyphs may still be referring to the image ID, we
@@ -2133,7 +2140,17 @@ image_set_transform (struct frame *f, struct image *img)
 
   /* Determine size.  */
   int width, height;
-  compute_image_size (img->width, img->height, img->spec, &width, &height);
+
+#ifdef HAVE_RSVG
+  /* SVGs are pre-scaled to the correct size.  */
+  if (EQ (image_spec_value (img->spec, QCtype, NULL), Qsvg))
+    {
+      width = img->width;
+      height = img->height;
+    }
+  else
+#endif
+    compute_image_size (img->width, img->height, img->spec, &width, &height);
 
   /* Determine rotation.  */
   double rotation = 0.0;
@@ -2312,11 +2329,16 @@ image_set_transform (struct frame *f, struct image *img)
    SPEC must be a valid Lisp image specification (see valid_image_p).  */
 
 ptrdiff_t
-lookup_image (struct frame *f, Lisp_Object spec)
+lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 {
   struct image *img;
   EMACS_UINT hash;
 
+  struct face *face = (face_id >= 0) ? FACE_FROM_ID (f, face_id)
+    : FACE_FROM_ID_OR_NULL (f, DEFAULT_FACE_ID);
+  unsigned long foreground = FACE_COLOR_TO_PIXEL (face->foreground, f);
+  unsigned long background = FACE_COLOR_TO_PIXEL (face->background, f);
+
   /* F must be a window-system frame, and SPEC must be a valid image
      specification.  */
   eassert (FRAME_WINDOW_P (f));
@@ -2324,7 +2346,7 @@ lookup_image (struct frame *f, Lisp_Object spec)
 
   /* Look up SPEC in the hash table of the image cache.  */
   hash = sxhash (spec);
-  img = search_image_cache (f, spec, hash);
+  img = search_image_cache (f, spec, hash, foreground, background, true);
   if (img && img->load_failed_p)
     {
       free_image (f, img);
@@ -2337,9 +2359,9 @@ lookup_image (struct frame *f, Lisp_Object spec)
       block_input ();
       img = make_image (spec, hash);
       cache_image (f, img);
+      img->face_foreground = foreground;
+      img->face_background = background;
       img->load_failed_p = ! img->type->load (f, img);
-      img->frame_foreground = FRAME_FOREGROUND_PIXEL (f);
-      img->frame_background = FRAME_BACKGROUND_PIXEL (f);
 
       /* If we can't load the image, and we don't have a width and
 	 height, use some arbitrary width and height so that we can
@@ -2393,8 +2415,7 @@ lookup_image (struct frame *f, Lisp_Object spec)
 	      if (!NILP (bg))
 		{
 		  img->background
-		    = image_alloc_image_color (f, img, bg,
-                                               FRAME_BACKGROUND_PIXEL (f));
+		    = image_alloc_image_color (f, img, bg, background);
 		  img->background_valid = 1;
 		}
 	    }
@@ -3667,8 +3688,8 @@ xbm_load_image (struct frame *f, struct image *img, char *contents, char *end)
 			     &data, 0);
   if (rc)
     {
-      unsigned long foreground = FRAME_FOREGROUND_PIXEL (f);
-      unsigned long background = FRAME_BACKGROUND_PIXEL (f);
+      unsigned long foreground = img->face_foreground;
+      unsigned long background = img->face_background;
       bool non_default_colors = 0;
       Lisp_Object value;
 
@@ -3764,8 +3785,8 @@ xbm_load (struct frame *f, struct image *img)
     {
       struct image_keyword fmt[XBM_LAST];
       Lisp_Object data;
-      unsigned long foreground = FRAME_FOREGROUND_PIXEL (f);
-      unsigned long background = FRAME_BACKGROUND_PIXEL (f);
+      unsigned long foreground = img->face_foreground;
+      unsigned long background = img->face_background;
       bool non_default_colors = 0;
       char *bits;
       bool parsed_p;
@@ -6125,8 +6146,8 @@ pbm_load (struct frame *f, struct image *img)
       unsigned char c = 0;
       int g;
       struct image_keyword fmt[PBM_LAST];
-      unsigned long fg = FRAME_FOREGROUND_PIXEL (f);
-      unsigned long bg = FRAME_BACKGROUND_PIXEL (f);
+      unsigned long fg = img->face_foreground;
+      unsigned long bg = img->face_background;
       /* Parse the image specification.  */
       memcpy (fmt, pbm_format, sizeof fmt);
       parse_image_spec (img->spec, fmt, PBM_LAST, Qpbm);
@@ -9433,6 +9454,7 @@ DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
   SVG_ALGORITHM,
   SVG_HEURISTIC_MASK,
   SVG_MASK,
+  SVG_FOREGROUND,
   SVG_BACKGROUND,
   SVG_LAST
 };
@@ -9451,6 +9473,7 @@ DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
   {":conversion",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
   {":heuristic-mask",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
   {":mask",		IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":foreground",	IMAGE_STRING_OR_NIL_VALUE,		0},
   {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
 };
 
@@ -9715,6 +9738,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   int height;
   const guint8 *pixels;
   int rowstride;
+  char *wrapped_contents = NULL;
+  ptrdiff_t wrapped_size;
 
 #if ! GLIB_CHECK_VERSION (2, 36, 0)
   /* g_type_init is a glib function that must be called prior to
@@ -9722,6 +9747,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   g_type_init ();
 #endif
 
+  /* Parse the unmodified SVG data so we can get it's initial size.  */
+
 #if LIBRSVG_CHECK_VERSION (2, 32, 0)
   GInputStream *input_stream
     = g_memory_input_stream_new_from_data (contents, size, NULL);
@@ -9750,6 +9777,107 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   rsvg_handle_write (rsvg_handle, (unsigned char *) contents, size, &err);
   if (err) goto rsvg_error;
 
+  /* The parsing is complete, rsvg_handle is ready to used, close it
+     for further writes.  */
+  rsvg_handle_close (rsvg_handle, &err);
+  if (err) goto rsvg_error;
+#endif
+
+  /* Get the image dimensions.  */
+  rsvg_handle_get_dimensions (rsvg_handle, &dimension_data);
+
+  /* We are now done with the unmodified data.  */
+  g_object_unref (rsvg_handle);
+
+  /* Calculate the final image size.  */
+  compute_image_size (dimension_data.width, dimension_data.height,
+                      img->spec, &width, &height);
+
+  /* Wrap the SVG data in another SVG.  This allows us to set the
+     width and height, as well as modify the foreground and background
+     colors.  */
+  {
+    Lisp_Object value;
+    unsigned long foreground = img->face_foreground;
+    unsigned long background = img->face_background;
+
+    Lisp_Object encoded_contents = Fbase64_encode_string
+      (make_unibyte_string (contents, size), Qt);
+
+    /* The wrapper sets the foreground color, width and height, and
+       viewBox must contain the dimensions of the original image.  It
+       also draws a rectangle over the whole space, set to the
+       background color, before including the original image. This
+       acts to set the background color, instead of leaving it
+       transparent.  */
+    const char *wrapper =
+      "<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\" "
+      "xmlns:xi=\"http://www.w3.org/2001/XInclude\" "
+      "style=\"color: #%06X; fill: currentColor;\" "
+      "width=\"%d\" height=\"%d\" preserveAspectRatio=\"none\" "
+      "viewBox=\"0 0 %d %d\">"
+      "<rect width=\"100%%\" height=\"100%%\" fill=\"#%06X\"/>"
+      "<xi:include href=\"data:image/svg+xml;base64,%s\"></xi:include>"
+      "</svg>";
+
+    /* FIXME: I've added 64 in the hope it will cover the size of the
+       width and height strings and things.  */
+    int buffer_size = SBYTES (encoded_contents) + strlen (wrapper) + 64;
+
+    value = image_spec_value (img->spec, QCforeground, NULL);
+    if (!NILP (value))
+      {
+        foreground = image_alloc_image_color (f, img, value, img->face_foreground);
+      }
+    value = image_spec_value (img->spec, QCbackground, NULL);
+    if (!NILP (value))
+      {
+        background = image_alloc_image_color (f, img, value, img->face_background);
+        img->background = background;
+        img->background_valid = 1;
+      }
+
+    wrapped_contents = malloc (buffer_size);
+
+    if (!wrapped_contents
+        || buffer_size <= snprintf (wrapped_contents, buffer_size, wrapper,
+                                    foreground & 0xFFFFFF, width, height,
+                                    dimension_data.width, dimension_data.height,
+                                    background & 0xFFFFFF, SSDATA (encoded_contents)))
+      goto rsvg_error;
+
+    wrapped_size = strlen (wrapped_contents);
+  }
+
+  /* Now we parse the wrapped version.  */
+
+#if LIBRSVG_CHECK_VERSION (2, 32, 0)
+  input_stream = g_memory_input_stream_new_from_data (wrapped_contents, wrapped_size, NULL);
+  base_file = filename ? g_file_new_for_path (filename) : NULL;
+  rsvg_handle = rsvg_handle_new_from_stream_sync (input_stream, base_file,
+						  RSVG_HANDLE_FLAGS_NONE,
+						  NULL, &err);
+  if (base_file)
+    g_object_unref (base_file);
+  g_object_unref (input_stream);
+
+  /* Check rsvg_handle too, to avoid librsvg 2.40.13 bug (Bug#36773#26).  */
+  if (!rsvg_handle || err) goto rsvg_error;
+#else
+  /* Make a handle to a new rsvg object.  */
+  rsvg_handle = rsvg_handle_new ();
+  eassume (rsvg_handle);
+
+  /* Set base_uri for properly handling referenced images (via 'href').
+     See rsvg bug 596114 - "image refs are relative to curdir, not .svg file"
+     <https://gitlab.gnome.org/GNOME/librsvg/issues/33>. */
+  if (filename)
+    rsvg_handle_set_base_uri (rsvg_handle, filename);
+
+  /* Parse the contents argument and fill in the rsvg_handle.  */
+  rsvg_handle_write (rsvg_handle, (unsigned char *) wrapped_contents, wrapped_size, &err);
+  if (err) goto rsvg_error;
+
   /* The parsing is complete, rsvg_handle is ready to used, close it
      for further writes.  */
   rsvg_handle_close (rsvg_handle, &err);
@@ -9768,6 +9896,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   pixbuf = rsvg_handle_get_pixbuf (rsvg_handle);
   if (!pixbuf) goto rsvg_error;
   g_object_unref (rsvg_handle);
+  free (wrapped_contents);
 
   /* Extract some meta data from the svg handle.  */
   width     = gdk_pixbuf_get_width (pixbuf);
@@ -9792,25 +9921,6 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 
     init_color_table ();
 
-    /* Handle alpha channel by combining the image with a background
-       color.  */
-    Emacs_Color background;
-    Lisp_Object specified_bg = image_spec_value (img->spec, QCbackground, NULL);
-    if (!STRINGP (specified_bg)
-	|| !FRAME_TERMINAL (f)->defined_color_hook (f,
-                                                    SSDATA (specified_bg),
-                                                    &background,
-                                                    false,
-                                                    false))
-      FRAME_TERMINAL (f)->query_frame_background_color (f, &background);
-
-    /* SVG pixmaps specify transparency in the last byte, so right
-       shift 8 bits to get rid of it, since emacs doesn't support
-       transparency.  */
-    background.red   >>= 8;
-    background.green >>= 8;
-    background.blue  >>= 8;
-
     /* This loop handles opacity values, since Emacs assumes
        non-transparent images.  Each pixel must be "flattened" by
        calculating the resulting color, given the transparency of the
@@ -9822,16 +9932,11 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 	    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)));
+            /* Skip opacity.  */
+	    pixels++;
 
-	    PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red, green, blue));
+	    PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, red << 8, green << 8, blue << 8));
 	  }
 
 	pixels += rowstride - 4 * width;
@@ -9861,6 +9966,8 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
  rsvg_error:
   if (rsvg_handle)
     g_object_unref (rsvg_handle);
+  if (wrapped_contents)
+    free (wrapped_contents);
   /* FIXME: Use error->message so the user knows what is the actual
      problem with the image.  */
   image_error ("Error parsing SVG image `%s'", img->spec);
@@ -10159,7 +10266,7 @@ DEFUN ("lookup-image", Flookup_image, Slookup_image, 1, 1, 0,
   ptrdiff_t id = -1;
 
   if (valid_image_p (spec))
-    id = lookup_image (SELECTED_FRAME (), spec);
+    id = lookup_image (SELECTED_FRAME (), spec, -1);
 
   debug_print (spec);
   return make_fixnum (id);
diff --git a/src/nsmenu.m b/src/nsmenu.m
index b7e4cbd565..e313fc03f4 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -1092,7 +1092,7 @@ - (Lisp_Object)runMenuAt: (NSPoint)p forFrame: (struct frame *)f
           continue;
         }
 
-      img_id = lookup_image (f, image);
+      img_id = lookup_image (f, image, -1);
       img = IMAGE_FROM_ID (f, img_id);
       prepare_image_for_display (f, img);
 
diff --git a/src/xdisp.c b/src/xdisp.c
index ad03ac4605..764d5345f2 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -5696,7 +5696,7 @@ handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
       else
 	{
 	  it->what = IT_IMAGE;
-	  it->image_id = lookup_image (it->f, value);
+	  it->image_id = lookup_image (it->f, value, it->face_id);
 	  it->position = start_pos;
 	  it->object = NILP (object) ? it->w->contents : object;
 	  it->method = GET_FROM_IMAGE;
@@ -22434,7 +22434,7 @@ push_prefix_prop (struct it *it, Lisp_Object prop)
   else if (IMAGEP (prop))
     {
       it->what = IT_IMAGE;
-      it->image_id = lookup_image (it->f, prop);
+      it->image_id = lookup_image (it->f, prop, it->face_id);
       it->method = GET_FROM_IMAGE;
     }
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -27335,7 +27335,7 @@ calc_pixel_width_or_height (double *res, struct it *it, Lisp_Object prop,
 	  if (FRAME_WINDOW_P (it->f)
 	      && valid_image_p (prop))
 	    {
-	      ptrdiff_t id = lookup_image (it->f, prop);
+	      ptrdiff_t id = lookup_image (it->f, prop, it->face_id);
 	      struct image *img = IMAGE_FROM_ID (it->f, id);
 
 	      return OK_PIXELS (width_p ? img->width : img->height);
diff --git a/test/manual/image-transforms-tests.el b/test/manual/image-transforms-tests.el
index 0ebd5c7a19..02607e6367 100644
--- a/test/manual/image-transforms-tests.el
+++ b/test/manual/image-transforms-tests.el
@@ -48,24 +48,24 @@ test-cropping
   (let ((image "<svg height='30' width='30'>
                   <rect x='0' y='0' width='10' height='10'/>
                   <rect x='10' y='10' width='10' height='10'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                  <line x1='10' y1='10' x2='20' y2='20' style='stroke:#000'/>
-                  <line x1='20' y1='10' x2='10' y2='20' style='stroke:#000'/>
+                        style='fill:none;stroke-width:1;stroke:currentColor'/>
+                  <line x1='10' y1='10' x2='20' y2='20' style='stroke:currentColor'/>
+                  <line x1='20' y1='10' x2='10' y2='20' style='stroke:currentColor'/>
                   <rect x='20' y='20' width='10' height='10'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
+                        style='fill:none;stroke-width:1;stroke:currentColor'/>
                 </svg>")
         (top-left "<svg height='10' width='10'>
                      <rect x='0' y='0' width='10' height='10'/>
                    </svg>")
         (middle "<svg height='10' width='10'>
                    <rect x='0' y='0' width='10' height='10'
-                         style='fill:none;stroke-width:1;stroke:#000'/>
-                   <line x1='0' y1='0' x2='10' y2='10' style='stroke:#000'/>
-                   <line x1='10' y1='0' x2='0' y2='10' style='stroke:#000'/>
+                         style='fill:none;stroke-width:1;stroke:currentColor'/>
+                   <line x1='0' y1='0' x2='10' y2='10' style='stroke:currentColor'/>
+                   <line x1='10' y1='0' x2='0' y2='10' style='stroke:currentColor'/>
                  </svg>")
         (bottom-right "<svg height='10' width='10'>
                          <rect x='0' y='0' width='10' height='10'
-                               style='fill:none;stroke-width:1;stroke:#000'/>
+                               style='fill:none;stroke-width:1;stroke:currentColor'/>
                        </svg>"))
     (insert-header "Test Crop: cropping an image (only works with ImageMagick)")
     (insert-test "all params" top-left image '(:crop (10 10 0 0)))
@@ -77,23 +77,23 @@ test-cropping
 (defun test-scaling ()
   (let ((image "<svg height='10' width='10'>
                   <rect x='0' y='0' width='10' height='10'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                  <line x1='0' y1='0' x2='10' y2='10' style='stroke:#000'/>
-                  <line x1='10' y1='0' x2='0' y2='10' style='stroke:#000'/>
+                        style='fill:none;stroke-width:1;stroke:currentColor'/>
+                  <line x1='0' y1='0' x2='10' y2='10' style='stroke:currentColor'/>
+                  <line x1='10' y1='0' x2='0' y2='10' style='stroke:currentColor'/>
                 </svg>")
         (large "<svg height='20' width='20'>
                   <rect x='0' y='0' width='20' height='20'
-                        style='fill:none;stroke-width:2;stroke:#000'/>
+                        style='fill:none;stroke-width:2;stroke:currentColor'/>
                   <line x1='0' y1='0' x2='20' y2='20'
-                        style='stroke-width:2;stroke:#000'/>
+                        style='stroke-width:2;stroke:currentColor'/>
                   <line x1='20' y1='0' x2='0' y2='20'
-                        style='stroke-width:2;stroke:#000'/>
+                        style='stroke-width:2;stroke:currentColor'/>
                 </svg>")
         (small "<svg height='5' width='5'>
                   <rect x='0' y='0' width='4' height='4'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                  <line x1='0' y1='0' x2='4' y2='4' style='stroke:#000'/>
-                  <line x1='4' y1='0' x2='0' y2='4' style='stroke:#000'/>
+                        style='fill:none;stroke-width:1;stroke:currentColor'/>
+                  <line x1='0' y1='0' x2='4' y2='4' style='stroke:currentColor'/>
+                  <line x1='4' y1='0' x2='0' y2='4' style='stroke:currentColor'/>
                 </svg>"))
     (insert-header "Test Scaling: resize an image (pixelization may occur)")
     (insert-test "1x" image image '(:scale 1))
@@ -107,27 +107,27 @@ test-scaling
 (defun test-scaling-rotation ()
   (let ((image "<svg height='20' width='20'>
                   <rect x='0' y='0' width='20' height='20'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
+                        style='fill:none;stroke-width:1;stroke:currentColor'/>
                   <rect x='0' y='0' width='10' height='10'
-                        style='fill:#000'/>
+                        style='fill:currentColor'/>
                 </svg>")
         (x2-90 "<svg height='40' width='40'>
                   <rect x='0' y='0' width='40' height='40'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
+                        style='fill:none;stroke-width:1;stroke:currentColor'/>
                   <rect x='20' y='0' width='20' height='20'
-                        style='fill:#000'/>
+                        style='fill:currentColor'/>
                 </svg>")
         (x2--90 "<svg height='40' width='40'>
                    <rect x='0' y='0' width='40' height='40'
-                         style='fill:none;stroke-width:1;stroke:#000'/>
+                         style='fill:none;stroke-width:1;stroke:currentColor'/>
                    <rect x='0' y='20' width='20' height='20'
-                         style='fill:#000'/>
+                         style='fill:currentColor'/>
                  </svg>")
         (x0.5-180 "<svg height='10' width='10'>
                      <rect x='0' y='0' width='10' height='10'
-                           style='fill:none;stroke-width:1;stroke:#000'/>
+                           style='fill:none;stroke-width:1;stroke:currentColor'/>
                      <rect x='5' y='5' width='5' height='5'
-                           style='fill:#000'/>
+                           style='fill:currentColor'/>
                    </svg>"))
     (insert-header "Test Scaling and Rotation: resize and rotate an image (pixelization may occur)")
     (insert-test "1x, 0 degrees" image image '(:scale 1 :rotation 0))
-- 
2.26.1

bug#40845: SVG rendering issues

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> As for the cache I decided to just delete all images that match the
> image spec, no matter what colours they use. It seems to me to be the
> safest option, as we don't want users flushing an image from the
> cache to display an updated version, only for the old one to reappear
> when the face changes.

Makes sense.

I've tried your patch set, and it fixes the test case in this bug report
for me (under Debian).

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

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sat, 22 Aug 2020 18:15:15 +0200 (CEST)
> From: Alan Third <alan@idiocy.org>
> 
> I still don't know how to use the mouse face. I couldn't see any way
> to detect if it's in use when we first load the image in xdisp.c.

Can you please remind me what was the problem?  The bug discussion is
very long, and I didn't have time/patience to find the mouse face
bits.

> -ptrdiff_t lookup_image (struct frame *, Lisp_Object);
> +ptrdiff_t lookup_image (struct frame *, Lisp_Object, int face_id);
                                                        ^^^^^^^^^^^
Please don't use names in prototypes, only types.

> +  /* Parse the unmodified SVG data so we can get it's initial size.  */
                                                    ^^^^
"its"

> +  /* The parsing is complete, rsvg_handle is ready to used, close it
                                             ^^^^^^^^^^^^^^^^
"is ready to be used"

> +       background color, before including the original image. This
                                                               ^^
Two spaces between sentences, please.

> +    Lisp_Object encoded_contents = Fbase64_encode_string
> +      (make_unibyte_string (contents, size), Qt);

Our style of breaking long lines like this one is different:

  Lisp_Object encoded_contents
    = Fbase64_encode_string (make_unibyte_string (contents, size), Qt);

> +    if (!NILP (value))
> +      {
> +        foreground = image_alloc_image_color (f, img, value, img->face_foreground);
> +      }

No need for braces when the block has only one line.

Thanks.

bug#40845: SVG rendering issues

[Alan Third]

On Sat, Aug 22, 2020 at 07:54:48PM +0300, Eli Zaretskii wrote:
> > Date: Sat, 22 Aug 2020 18:15:15 +0200 (CEST)
> > From: Alan Third <alan@idiocy.org>
> > 
> > I still don't know how to use the mouse face. I couldn't see any way
> > to detect if it's in use when we first load the image in xdisp.c.
> 
> Can you please remind me what was the problem?  The bug discussion is
> very long, and I didn't have time/patience to find the mouse face
> bits.

No problem.

Basically, if you try Clement's original example, when you move the
mouse over the image the background colour doesn't change to match the
rest of the line.

I'm picking up the background colour from the face stored in "it" in
push_prefix_prop or handle_single_display_spec in xdisp.c. As far as I
can tell one or both of these are the only places we can define the
image with the face colours as this is where we actually load the
image. The problem is that I don't see any way to work out if the
current image should use the background colour from the mouse face.

I know that later on we can do (s->hl == DRAW_MOUSE_FACE), but I don't
think those structures are built yet?

I've fixed the mistakes you pointed out. Thanks.
-- 
Alan Third

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sat, 22 Aug 2020 20:57:59 +0200 (CEST)
> From: Alan Third <alan@idiocy.org>
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> 
> Basically, if you try Clement's original example, when you move the
> mouse over the image the background colour doesn't change to match the
> rest of the line.
> 
> I'm picking up the background colour from the face stored in "it" in
> push_prefix_prop or handle_single_display_spec in xdisp.c. As far as I
> can tell one or both of these are the only places we can define the
> image with the face colours as this is where we actually load the
> image. The problem is that I don't see any way to work out if the
> current image should use the background colour from the mouse face.
> 
> I know that later on we can do (s->hl == DRAW_MOUSE_FACE), but I don't
> think those structures are built yet?

I think you want to modify note_mouse_highlight so that it sets up the
frame's mouse-highlight info for image glyphs, like it currently does
for character glyphs.  It's possible this is all that needs to be
done; if not, we should see how to change show_mouse_face and its
subroutines to display images with mouse-highlight.

Let me know if you need more guidance.

bug#40845: SVG rendering issues

[Alan Third]

On Sat, Aug 22, 2020 at 10:17:53PM +0300, Eli Zaretskii wrote:
> > Date: Sat, 22 Aug 2020 20:57:59 +0200 (CEST)
> > From: Alan Third <alan@idiocy.org>
> > Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> > 
> > Basically, if you try Clement's original example, when you move the
> > mouse over the image the background colour doesn't change to match the
> > rest of the line.
> > 
> > I'm picking up the background colour from the face stored in "it" in
> > push_prefix_prop or handle_single_display_spec in xdisp.c. As far as I
> > can tell one or both of these are the only places we can define the
> > image with the face colours as this is where we actually load the
> > image. The problem is that I don't see any way to work out if the
> > current image should use the background colour from the mouse face.
> > 
> > I know that later on we can do (s->hl == DRAW_MOUSE_FACE), but I don't
> > think those structures are built yet?
> 
> I think you want to modify note_mouse_highlight so that it sets up the
> frame's mouse-highlight info for image glyphs, like it currently does
> for character glyphs.  It's possible this is all that needs to be
> done; if not, we should see how to change show_mouse_face and its
> subroutines to display images with mouse-highlight.
> 
> Let me know if you need more guidance.

Thanks for your help, and that certainly helped me understand a bit
more about how mouse highlighting works and I don't think setting the
background colour from a face is a practical way of doing this.

But I've just realised we don't need to do it. Masks provide this
functionality. If you want the background of your SVG to match the
background of the buffer, use a mask, which can be keyed off a
specific colour or whatever.

Unfortunately it won't work well for semi-transparent images, but I
think the only really practical solution for that is to introduce
proper transparency handling.

Therefore I think this patch is ready to go.
-- 
Alan Third

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sat, 22 Aug 2020 23:35:46 +0200 (CEST)
> From: Alan Third <alan@idiocy.org>
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> 
> Therefore I think this patch is ready to go.

Thanks.  Before we install this, one question: is there a possibility
someone would want the previous behavior?

bug#40845: SVG rendering issues

[Alan Third]

On Sun, Aug 23, 2020 at 08:47:27AM +0300, Eli Zaretskii wrote:
> > Date: Sat, 22 Aug 2020 23:35:46 +0200 (CEST)
> > From: Alan Third <alan@idiocy.org>
> > Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> > 
> > Therefore I think this patch is ready to go.
> 
> Thanks.  Before we install this, one question: is there a possibility
> someone would want the previous behavior?

I don't think so. The resizing is plainly better, and if someone wants
to enforce a foreground and background colour they can still do that
with the :foreground and :background image properties.
-- 
Alan Third

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sun, 23 Aug 2020 11:09:01 +0200 (CEST)
> From: Alan Third <alan@idiocy.org>
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> 
> > Thanks.  Before we install this, one question: is there a possibility
> > someone would want the previous behavior?
> 
> I don't think so. The resizing is plainly better, and if someone wants
> to enforce a foreground and background colour they can still do that
> with the :foreground and :background image properties.

OK, then let's say that last bit in the NEWS entry announcing this new
behavior.

bug#40845: SVG rendering issues

[Alan Third]

On Sun, Aug 23, 2020 at 12:11:55PM +0300, Eli Zaretskii wrote:
> > Date: Sun, 23 Aug 2020 11:09:01 +0200 (CEST)
> > From: Alan Third <alan@idiocy.org>
> > Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> > 
> > > Thanks.  Before we install this, one question: is there a possibility
> > > someone would want the previous behavior?
> > 
> > I don't think so. The resizing is plainly better, and if someone wants
> > to enforce a foreground and background colour they can still do that
> > with the :foreground and :background image properties.
> 
> OK, then let's say that last bit in the NEWS entry announcing this new
> behavior.

Something like this?

*** The background and foreground of images now default to face colors.
To load images with the default frame colors use the ':foreground'
and ':background' image attributes, for example:

    (create-image "filename" nil nil
                  :foreground (face-attribute 'default :foreground)
                  :background (face-attribute 'default :background))

-- 
Alan Third

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sun, 23 Aug 2020 13:48:45 +0200 (CEST)
> From: Alan Third <alan@idiocy.org>
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> 
> *** The background and foreground of images now default to face colors.
> To load images with the default frame colors use the ':foreground'
> and ':background' image attributes, for example:
> 
>     (create-image "filename" nil nil
>                   :foreground (face-attribute 'default :foreground)
>                   :background (face-attribute 'default :background))

Yes, thanks.  But maybe expand a bit about "the face colors" part (not
in the heading line, but in the body): what face is that, and when
will the colors be used (I understand they will be used only if the
image doesn't specify its own colors).

bug#40845: SVG rendering issues

[Alan Third]

On Sun, Aug 23, 2020 at 03:05:24PM +0300, Eli Zaretskii wrote:
> 
> Yes, thanks.  But maybe expand a bit about "the face colors" part (not
> in the heading line, but in the body): what face is that, and when
> will the colors be used (I understand they will be used only if the
> image doesn't specify its own colors).

How's this?

---
*** The background and foreground of images now default to face
colors.  When an image doesn't specify a foreground or background
color, Emacs now uses colors from the face used to draw the
surrounding text instead of the frame's default colors.

To load images with the default frame colors use the ':foreground'
and ':background' image attributes, for example:

    (create-image "filename" nil nil
                  :foreground (face-attribute 'default :foreground)
                  :background (face-attribute 'default :background))

This change doesn't affect image types that do not support foreground
and background colors, such as png or jpeg.

-- 
Alan Third

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sun, 23 Aug 2020 14:19:28 +0200 (CEST)
> From: Alan Third <alan@idiocy.org>
> Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> 
> *** The background and foreground of images now default to face
> colors.  When an image doesn't specify a foreground or background
> color, Emacs now uses colors from the face used to draw the
> surrounding text instead of the frame's default colors.
> 
> To load images with the default frame colors use the ':foreground'
> and ':background' image attributes, for example:
> 
>     (create-image "filename" nil nil
>                   :foreground (face-attribute 'default :foreground)
>                   :background (face-attribute 'default :background))
> 
> This change doesn't affect image types that do not support foreground
> and background colors, such as png or jpeg.

Almost perfect: I would suggest to modify the last sentence so that it
tells which types of images do support this feature.

Thanks.

bug#40845: SVG rendering issues

[Alan Third]

On Sun, Aug 23, 2020 at 03:23:33PM +0300, Eli Zaretskii wrote:
> > Date: Sun, 23 Aug 2020 14:19:28 +0200 (CEST)
> > From: Alan Third <alan@idiocy.org>
> > Cc: cpitclaudel@gmail.com, 40845@debbugs.gnu.org, pipcet@gmail.com
> > 
> > *** The background and foreground of images now default to face
> > colors.  When an image doesn't specify a foreground or background
> > color, Emacs now uses colors from the face used to draw the
> > surrounding text instead of the frame's default colors.
> > 
> > To load images with the default frame colors use the ':foreground'
> > and ':background' image attributes, for example:
> > 
> >     (create-image "filename" nil nil
> >                   :foreground (face-attribute 'default :foreground)
> >                   :background (face-attribute 'default :background))
> > 
> > This change doesn't affect image types that do not support foreground
> > and background colors, such as png or jpeg.
> 
> Almost perfect: I would suggest to modify the last sentence so that it
> tells which types of images do support this feature.

Thanks. I've pushed it to master.
-- 
Alan Third

bug#40845: SVG rendering issues

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> Thanks. I've pushed it to master.

Great!

I'm getting this warning, though:

image.c: In function ‘lookup_image’:
image.c:2340:17: warning: potential null pointer dereference [-Wnull-dereference]
 2340 |   unsigned long background = FACE_COLOR_TO_PIXEL (face->background, f);
      |                 ^~~~~~~~~~
image.c:2339:17: warning: potential null pointer dereference [-Wnull-dereference]
 2339 |   unsigned long foreground = FACE_COLOR_TO_PIXEL (face->foreground, f);
      |                 ^~~~~~~~~~

This is on Debian/bullseye, with gcc (Debian 9.3.0-15) 9.3.0.

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

bug#40845: SVG rendering issues

[Alan Third]

On Sun, Aug 23, 2020 at 05:43:24PM +0200, Lars Ingebrigtsen wrote:
> Alan Third <alan@idiocy.org> writes:
> 
> > Thanks. I've pushed it to master.
> 
> Great!
> 
> I'm getting this warning, though:
> 
> image.c: In function ‘lookup_image’:
> image.c:2340:17: warning: potential null pointer dereference [-Wnull-dereference]
>  2340 |   unsigned long background = FACE_COLOR_TO_PIXEL (face->background, f);
>       |                 ^~~~~~~~~~
> image.c:2339:17: warning: potential null pointer dereference [-Wnull-dereference]
>  2339 |   unsigned long foreground = FACE_COLOR_TO_PIXEL (face->foreground, f);
>       |                 ^~~~~~~~~~
> 
> This is on Debian/bullseye, with gcc (Debian 9.3.0-15) 9.3.0.

I don't think I should have used FACE_FROM_ID_OR_NULL. I would hope
we're not going to be trying to load images without a default face
already defined.

I've pushed a fix.
-- 
Alan Third

bug#40845: SVG rendering issues

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> I don't think I should have used FACE_FROM_ID_OR_NULL. I would hope
> we're not going to be trying to load images without a default face
> already defined.
>
> I've pushed a fix.

Thanks; that fixes the warnings.

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

bug#40845: SVG rendering issues

[Eli Zaretskii]

> Date: Sun, 26 Apr 2020 22:17:41 +0100
> From: Alan Third <alan@idiocy.org>
> 
> I’m happy to give this a go, but I’m not sure about base64 encoding.
> Do we already have a way of doing that to a char buffer?

I don't think I understand the question.  Can you elaborate?