bug#67533: SVG images confound position pixel measurements

bug#67533: SVG images confound position pixel measurements

[JD Smith]

(As posted on emacs-devel)
++++++++++++++++++++++

In developing a new pixel-precise smooth scrolling mode, I’ve noticed that inline SVG images confuse Emacs regarding pixel positions of surrounding elements.  You sometimes also experience this while visiting SVG image-rich files (think org-latex-preview) while in visual line mode.  In this case, previous/next-line sometimes jump from one side of the window to the other.  

But it’s easiest to reproduce with line-truncation in effect.  Run the snippet below with your frame either expanded wide enough to accommodate the full width of the 1st line of text, or too narrow (eliciting truncation).  While truly truncated and with point on the SVG, pixel text measurements above are erroneous (reporting zero pixel height above), as if it thinks it’s on the prior line.  At other points in line 2, the pixel-size values are correct.  

This erroneous pixel size/position result occurs whether the image appears via an overlay or a text-property.

Tested in Emacs 29.1 NS and Mac ports.

Small update:  this only occurs when the image is at the start of the line.  And PNG images exhibit the same issue.

+++++++

;;; test-svg-pixel-position --- test pixel position for SVG images

;;; This small code creates a buffer with two lines, the first of
;;; which is long, and the second of which has an SVG image at start.
;;; Line truncation is turned on.  `window-text-pixel-size` returns
;;; differing results depending on whether truncation is actually in
;;; effect (alter the frame width to see this).

;;; Code:
(require 'svg)
(let ((buf "svg-pixel-demo")
     (svg (svg-create 50 25)))
 (svg-circle svg 25 25 25 :stroke-color "green")
 (with-current-buffer (get-buffer-create buf)
   (erase-buffer)
   (insert "Pellentesque condimentum, magna ut suscipit hendrerit, ipsum augue ornare nulla, non luctus diam neque sit amet urna.\n")
   (insert (propertize "THISISACIRCLE" 'display (svg-image svg)))
   (insert " Aliquam posuere.\n")
   (pop-to-buffer buf)
   (goto-char (point-max))
   (forward-line -1)
   (toggle-truncate-lines 1)
   (message "PIXEL SIZE OF LINE ABOVE IMAGE: %S"
	     (window-text-pixel-size nil (cons (point) -1) (point) nil nil nil t))))

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Wed, 29 Nov 2023 15:31:43 -0500
> 
> ;;; test-svg-pixel-position --- test pixel position for SVG images
> 
> ;;; This small code creates a buffer with two lines, the first of
> ;;; which is long, and the second of which has an SVG image at start.
> ;;; Line truncation is turned on.  `window-text-pixel-size` returns
> ;;; differing results depending on whether truncation is actually in
> ;;; effect (alter the frame width to see this).
> 
> ;;; Code:
> (require 'svg)
> (let ((buf "svg-pixel-demo")
>      (svg (svg-create 50 25)))
>  (svg-circle svg 25 25 25 :stroke-color "green")
>  (with-current-buffer (get-buffer-create buf)
>    (erase-buffer)
>    (insert "Pellentesque condimentum, magna ut suscipit hendrerit, ipsum augue ornare nulla, non luctus diam neque sit amet urna.\n")
>    (insert (propertize "THISISACIRCLE" 'display (svg-image svg)))
>    (insert " Aliquam posuere.\n")
>    (pop-to-buffer buf)
>    (goto-char (point-max))
>    (forward-line -1)
>    (toggle-truncate-lines 1)
>    (message "PIXEL SIZE OF LINE ABOVE IMAGE: %S"
> 	     (window-text-pixel-size nil (cons (point) -1) (point) nil nil nil t))))

Does the patch below fix the issue?  (It should fix the recipe you
posted, but you hinted that this is just the simplest way of seeing a
more general problem, so I wonder whether that more general problem is
also fixed.)

diff --git a/src/xdisp.c b/src/xdisp.c
index eb38ebb..7cf3902 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -11487,6 +11487,16 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
 	      it.max_descent = max (it.max_descent, it.descent);
 	    }
 	}
+      else if (IT_CHARPOS (it) > end
+	       && it.line_wrap == TRUNCATE
+	       && it.current_x - it.first_visible_x >= it.last_visible_x)
+	{
+	  /* If the display property at END is at the beginning of the
+	     line, and the previous line was truncated, we are at END,
+	     but it.current_y is not yet updated to reflect that.  */
+	  it.current_y += max (it.max_ascent, it.ascent)
+			  + max (it.max_descent, it.descent);
+	}
     }
   else
     bidi_unshelve_cache (it2data, true);

bug#67533: SVG images confound position pixel measurements

[JD Smith]

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


> On Nov 30, 2023, at 12:32 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> Does the patch below fix the issue?  (It should fix the recipe you
> posted, but you hinted that this is just the simplest way of seeing a
> more general problem, so I wonder whether that more general problem is
> also fixed.)

Thanks for looking into it.  As you surmised, this patch fixes only the demo case with truncate-lines.  For the more general case, originally discovered with visual-line-mode on an org-mode file with many SVG preview overlays, the issue remains.  In that more general case, window-text-pixel-size sometimes gives 0 or (when the image is not at the start of the visual line) a value which is too large.  

The file I can pretty readily reproduce it on here is at https://gist.github.com/jdtsmith/de34aac901fc9f96f1a88a1b1b67d46e (I use dvigms for latex preview).  Only some of the preview overlay images (and other locations) exhibit the misbehavior, but more so for narrower frames with more wrapping.  I’m happy to test on this end, since my pixel scrolling code finds these types of incorrect results quite instantly.  As I change the window width and the text reflows, different positions exhibit the problems. 

The misbehavior extends to other regular text on the line, too, not just images.  There are various possibilities for what (window-text-pixel-size nil (cons (point) -1) (point) nil nil nil t) returns:

The correct value for position (start of line above) and (line-pixel-height) of that line.  Most text does this, but not when the visual line starts with a “bad” image.
Images at line start can (but don’t always) return their own position and zero height.
On an image further right of a line-starting “bad” image, it returns the bad image position, but the full height of the line. 
Some images on lines which do not start with an image return the correct position (on the prior line), but a height which is too large, as if they included some of their own height in the total.

Here’s a shot of a larger version of the file, with point on a position which gives misbehavior #2.




[-- Attachment #2.1: Type: text/html, Size: 6206 bytes --]

[-- Attachment #2.2: PastedGraphic-1.png --]
[-- Type: image/png, Size: 106328 bytes --]

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Thu, 30 Nov 2023 16:00:09 -0500
> Cc: 67533@debbugs.gnu.org
> 
> > On Nov 30, 2023, at 12:32 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> > Does the patch below fix the issue?  (It should fix the recipe you
> > posted, but you hinted that this is just the simplest way of seeing a
> > more general problem, so I wonder whether that more general problem is
> > also fixed.)
> 
> Thanks for looking into it.  As you surmised, this patch fixes only the demo case with truncate-lines.  For the more general case, originally discovered with visual-line-mode on an org-mode file with many SVG preview overlays, the issue remains.  In that more general case, window-text-pixel-size sometimes gives 0 or (when the image is not at the start of the visual line) a value which is too large.  
> 
> The file I can pretty readily reproduce it on here is at https://gist.github.com/jdtsmith/de34aac901fc9f96f1a88a1b1b67d46e (I use dvigms for latex preview).  Only some of the preview overlay images (and other locations) exhibit the misbehavior, but more so for narrower frames with more wrapping.  I’m happy to test on this end, since my pixel scrolling code finds these types of incorrect results quite instantly.  As I change the window width and the text reflows, different positions exhibit the problems. 
> 
> The misbehavior extends to other regular text on the line, too, not just images.  There are various possibilities for what (window-text-pixel-size nil (cons (point) -1) (point) nil nil nil t) returns:
> 
> The correct value for position (start of line above) and (line-pixel-height) of that line.  Most text does this, but not when the visual line starts with a “bad” image.
> Images at line start can (but don’t always) return their own position and zero height.
> On an image further right of a line-starting “bad” image, it returns the bad image position, but the full height of the line. 
> Some images on lines which do not start with an image return the correct position (on the prior line), but a height which is too large, as if they included some of their own height in the total.

Thanks, but I need recipes I could reproduce on my system, and
reproduce relatively easily.  Debugging this usually takes numerous
runs under GDB, so the recipe should be reasonably easy to run, so it
doesn't take too long to set up a debugging session, and preferably it
should not require installation of optional packages.  It also cannot
depend on something that doesn't work on MS-Windows, because this is
the only system where I can debug a GUI Emacs.  So, for example, using
LaTeX Preview and dvigsm is out of the question; I need ready image
files prepared by LaTeX Preview and Lisp code to display them in a way
that causes problems.

So please try to provide recipes for the problems you still see, after
applying the patch I sent, as simple and preferably stand-alone Lisp
programs that don't require to install and run all of your code.  If
necessary, please emulate what your package does with simplified Lisp
code that just inserts text with display properties and overlays, to
produce the same result as the package does.

I'm sorry to insist on this and asking you to do extra work, but IME
this is the only way I can debug these issues efficiently enough to
solve them.  I appreciate the offer to test on your end, but such
testing is only useful when I have a tentative solution in hand for
you to test.  Such a tentative solution must be based on some problem
I found and fixed here, and for that I must be able to step through
the offending code in GDB.

Thanks.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

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


> On Dec 1, 2023, at 2:08 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> Thanks, but I need recipes I could reproduce on my system, and
> reproduce relatively easily.  

Please find a stand-alone reproduction code which can separately test both text-property and overlay inline SVGs at this gist <https://gist.github.com/jdtsmith/ad765047a6afe20f353de573d8c07da9>.  See the header for instructions.  

You should get many reported errors in which the size above is larger than expected, but as you narrow your frame and re-run the check, you’ll eventually find some images reporting 0 height above.  Interestingly, for the same frame width, overlays produce some of the same but overall fewer errors than text-properties, using precisely the same images for ‘display.

The report also reveals that the posn-x-y of the mis-reporting images is usually wrong, which is at least partially responsible for incorrect pixel position measurements.  

The one bug in my original org file I haven’t been able to reproduce is random text characters (usually on a line with a bad image) misreporting pixel measurements.  But I could certainly imagine how in more complex files this could get out of sync. 

BTW, I haven’t tested it, but I do not think this misbehavior is specific to SVG images.

[-- Attachment #2: Type: text/html, Size: 3111 bytes --]

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Fri, 1 Dec 2023 17:04:59 -0500
> 
>  On Dec 1, 2023, at 2:08 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  Thanks, but I need recipes I could reproduce on my system, and
>  reproduce relatively easily.  
> 
> Please find a stand-alone reproduction code which can separately test both text-property and overlay
> inline SVGs at this gist.  See the header for instructions.  

Thanks, but I'd appreciate more focused pointers to specific problems,
see below.  I also seem to be unable to reproduce the problems(?).

> You should get many reported errors in which the size above is larger than expected, but as you
> narrow your frame and re-run the check, you’ll eventually find some images reporting 0 height above.
>  Interestingly, for the same frame width, overlays produce some of the same but overall fewer errors
> than text-properties, using precisely the same images for ‘display.

What is/are the specific problem(s) I should be looking into?  Is the
problem only the zero size when it is reported?  Or are there also
other problems, and if so, how do I recognize where and when they
happen?

> The report also reveals that the posn-x-y of the mis-reporting images is usually wrong, which is at
> least partially responsible for incorrect pixel position measurements.  

Again, can you point me to the situations when this happens and tell
how to realize it did happen?  I'd need to reproduce those specific
situations under a debugger.

When I run "M-x my/check-buffer-pixel-values" in the buffer created by
"M-x my/test-svg-positions", all I see is a single message "End of
buffer", nothing else.  I've resized the frame horizontally many
times, and I still see only this single message.  So either the code
somehow doesn't work on my system, or I am missing some important part
of the reproduction recipe.

Also, your test file doesn't use lexical-binding -- should it?  Or
does it not matter for the purposes of reproducing these issues?

> The one bug in my original org file I haven’t been able to reproduce is random text characters (usually
> on a line with a bad image) misreporting pixel measurements.

Misreporting pixel measurements of what?

> BTW, I haven’t tested it, but I do not think this misbehavior is specific to SVG images.

I don't think it is.  The display engine has no idea about the source
of the image data, all it cares about is that it is an image of a
given size and given attributes.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

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



> On Dec 2, 2023, at 2:30 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Fri, 1 Dec 2023 17:04:59 -0500
>> 
>> On Dec 1, 2023, at 2:08 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>> Thanks, but I need recipes I could reproduce on my system, and
>> reproduce relatively easily.  
>> 
>> Please find a stand-alone reproduction code which can separately test both text-property and overlay
>> inline SVGs at this gist.  See the header for instructions.  
> 
> Thanks, but I'd appreciate more focused pointers to specific problems,
> see below.  I also seem to be unable to reproduce the problems(?).

Strange.  Both of my ports here (NS and Mac) show the same issue.  Did you check the svg-pixel-demo-report buffer the header mentions, where the mismatch measurement information gets reported?  The specific problems encountered are listed in the report buffer, updated with my/check-buffer-pixel-values.  For example, at a standard width of 80 characters, I get this report:

SVG Position analysis for svg-pixel-demo (width 80, text-properties)

Incorrect at point= 34: line  2 at   (224 . 14) (image): expected 14 got 28
Incorrect at point= 99: line  3 at   (244 . 43) (image): expected 29 got 46
Incorrect at point=126: line  4 at    (42 . 75) (image): expected 32 got 46
Incorrect at point=162: line  4 at   (345 . 75) (image): expected 32 got 52
Incorrect at point=210: line  5 at  (133 . 110) (image): expected 35 got 49

So in this case, 5 of the image locations (of 7) are showing sizes of entities one pixel above them as too large (with sizes sometimes not matching any line height).  At small window widths (like 25-30 characters) many zero heights start showing up.   Running the check in a normal text window produces an empty report (no errors). 

> When I run "M-x my/check-buffer-pixel-values" in the buffer created by
> "M-x my/test-svg-positions", all I see is a single message "End of
> buffer", nothing else.  

Perhaps you did not check the report buffer? I usually am opening that in another frame. 

> Also, your test file doesn't use lexical-binding -- should it?  Or
> does it not matter for the purposes of reproducing these issues?

I neglected that.  I don’t think it makes a difference, but have now included it, and fixed a couple of resulting unused variable complaints.  Please re-download the gist <https://gist.github.com/jdtsmith/ad765047a6afe20f353de573d8c07da9> and try it again to be sure.

>> The one bug in my original org file I haven’t been able to reproduce is random text characters (usually
>> on a line with a bad image) misreporting pixel measurements.
> 
> Misreporting pixel measurements of what?

Same thing as for the images: the pixel heights of the entity at an offset of -1 pixels.  In the demo code I’ve written, this seems to occur only on characters with image 'display (overlay/text-property).  In my (much larger) org mode with latex preview file, it sometimes happens on regular text characters.  I’m hoping/expecting whatever fix we find for this problem corrects that too. 

>> BTW, I haven’t tested it, but I do not think this misbehavior is specific to SVG images.
> 
> I don't think it is.  The display engine has no idea about the source
> of the image data, all it cares about is that it is an image of a
> given size and given attributes.

Makes sense.  That comports with the fact that I’ve seen similar issues with PNG overlays.


[-- Attachment #2: Type: text/html, Size: 5167 bytes --]

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sat, 2 Dec 2023 08:36:07 -0500
> Cc: 67533@debbugs.gnu.org
> 
> Strange.  Both of my ports here (NS and Mac) show the same issue.  Did you check the
> svg-pixel-demo-report buffer the header mentions, where the mismatch measurement information
> gets reported?  The specific problems encountered are listed in the report buffer, updated with
> my/check-buffer-pixel-values.  For example, at a standard width of 80 characters, I get this report:
> 
>  SVG Position analysis for svg-pixel-demo (width 80, text-properties)
> 
>  Incorrect at point= 34: line  2 at   (224 . 14) (image): expected 14 got 28
>  Incorrect at point= 99: line  3 at   (244 . 43) (image): expected 29 got 46
>  Incorrect at point=126: line  4 at    (42 . 75) (image): expected 32 got 46
>  Incorrect at point=162: line  4 at   (345 . 75) (image): expected 32 got 52
>  Incorrect at point=210: line  5 at  (133 . 110) (image): expected 35 got 49

I never understood I need to look in another buffer.  I will look into
these.

> So in this case, 5 of the image locations (of 7) are showing sizes of entities one pixel above them as
> too large (with sizes sometimes not matching any line height).  At small window widths (like 25-30
> characters) many zero heights start showing up.   Running the check in a normal text window
> produces an empty report (no errors). 

So any messages in that buffer are problems in your opinion?  Or just
those which state zero height?

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> Cc: 67533@debbugs.gnu.org
> Date: Sat, 02 Dec 2023 16:18:28 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: JD Smith <jdtsmith@gmail.com>
> > Date: Sat, 2 Dec 2023 08:36:07 -0500
> > Cc: 67533@debbugs.gnu.org
> > 
> > Strange.  Both of my ports here (NS and Mac) show the same issue.  Did you check the
> > svg-pixel-demo-report buffer the header mentions, where the mismatch measurement information
> > gets reported?  The specific problems encountered are listed in the report buffer, updated with
> > my/check-buffer-pixel-values.  For example, at a standard width of 80 characters, I get this report:
> > 
> >  SVG Position analysis for svg-pixel-demo (width 80, text-properties)
> > 
> >  Incorrect at point= 34: line  2 at   (224 . 14) (image): expected 14 got 28
> >  Incorrect at point= 99: line  3 at   (244 . 43) (image): expected 29 got 46
> >  Incorrect at point=126: line  4 at    (42 . 75) (image): expected 32 got 46
> >  Incorrect at point=162: line  4 at   (345 . 75) (image): expected 32 got 52
> >  Incorrect at point=210: line  5 at  (133 . 110) (image): expected 35 got 49
> 
> I never understood I need to look in another buffer.  I will look into
> these.

Please try the patch below and see if any problems remain.

diff --git a/src/xdisp.c b/src/xdisp.c
index 0b2508c..cc95ab3 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -11436,7 +11436,7 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
       /* Start at the beginning of the line containing FROM.  Otherwise
 	 IT.current_x will be incorrectly set to zero at some arbitrary
 	 non-zero X coordinate.  */
-      reseat_at_previous_visible_line_start (&it);
+      move_it_by_lines (&it, 0);
       it.current_x = it.hpos = 0;
       if (IT_CHARPOS (it) != start)
 	{
@@ -11513,6 +11513,8 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
      the width of the last buffer position manually.  */
   if (IT_CHARPOS (it) > end)
     {
+      int end_y = it.current_y;
+
       end--;
       RESTORE_IT (&it, &it2, it2data);
       x = move_it_to (&it, end, to_x, max_y, -1, move_op);
@@ -11525,8 +11527,13 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
 
 	  /* DTRT if ignore_line_at_end is t.  */
 	  if (!NILP (ignore_line_at_end))
-	    doff = (max (it.max_ascent, it.ascent)
-		    + max (it.max_descent, it.descent));
+	    {
+	      /* If END-1 is on the previous screen line, we need to
+                 account for the vertical dimensions of previous line.  */
+	      if (it.current_y < end_y)
+		doff = (max (it.max_ascent, it.ascent)
+			+ max (it.max_descent, it.descent));
+	    }
 	  else
 	    {
 	      it.max_ascent = max (it.max_ascent, it.ascent);

bug#67533: SVG images confound position pixel measurements

[JD Smith]

Thanks for your work.   The port I use has fallen a few weeks behind master and the patches to xdisp.c unfortunately don’t apply cleanly.  I’ll see about building an NS port and report back. 

> On Dec 2, 2023, at 2:39 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Cc: 67533@debbugs.gnu.org
>> Date: Sat, 02 Dec 2023 16:18:28 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>>> From: JD Smith <jdtsmith@gmail.com>
>>> Date: Sat, 2 Dec 2023 08:36:07 -0500
>>> Cc: 67533@debbugs.gnu.org
>>> 
>>> Strange.  Both of my ports here (NS and Mac) show the same issue.  Did you check the
>>> svg-pixel-demo-report buffer the header mentions, where the mismatch measurement information
>>> gets reported?  The specific problems encountered are listed in the report buffer, updated with
>>> my/check-buffer-pixel-values.  For example, at a standard width of 80 characters, I get this report:
>>> 
>>> SVG Position analysis for svg-pixel-demo (width 80, text-properties)
>>> 
>>> Incorrect at point= 34: line  2 at   (224 . 14) (image): expected 14 got 28
>>> Incorrect at point= 99: line  3 at   (244 . 43) (image): expected 29 got 46
>>> Incorrect at point=126: line  4 at    (42 . 75) (image): expected 32 got 46
>>> Incorrect at point=162: line  4 at   (345 . 75) (image): expected 32 got 52
>>> Incorrect at point=210: line  5 at  (133 . 110) (image): expected 35 got 49
>> 
>> I never understood I need to look in another buffer.  I will look into
>> these.
> 
> Please try the patch below and see if any problems remain.
> 
> diff --git a/src/xdisp.c b/src/xdisp.c
> index 0b2508c..cc95ab3 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -11436,7 +11436,7 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
>       /* Start at the beginning of the line containing FROM.  Otherwise
> 	 IT.current_x will be incorrectly set to zero at some arbitrary
> 	 non-zero X coordinate.  */
> -      reseat_at_previous_visible_line_start (&it);
> +      move_it_by_lines (&it, 0);
>       it.current_x = it.hpos = 0;
>       if (IT_CHARPOS (it) != start)
> 	{
> @@ -11513,6 +11513,8 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
>      the width of the last buffer position manually.  */
>   if (IT_CHARPOS (it) > end)
>     {
> +      int end_y = it.current_y;
> +
>       end--;
>       RESTORE_IT (&it, &it2, it2data);
>       x = move_it_to (&it, end, to_x, max_y, -1, move_op);
> @@ -11525,8 +11527,13 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
> 
> 	  /* DTRT if ignore_line_at_end is t.  */
> 	  if (!NILP (ignore_line_at_end))
> -	    doff = (max (it.max_ascent, it.ascent)
> -		    + max (it.max_descent, it.descent));
> +	    {
> +	      /* If END-1 is on the previous screen line, we need to
> +                 account for the vertical dimensions of previous line.  */
> +	      if (it.current_y < end_y)
> +		doff = (max (it.max_ascent, it.ascent)
> +			+ max (it.max_descent, it.descent));
> +	    }
> 	  else
> 	    {
> 	      it.max_ascent = max (it.max_ascent, it.ascent);

bug#67533: SVG images confound position pixel measurements

[JD Smith]

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

Progress.  The latest patch had two small hunks fail (see below), but they were simple enough that I was able to insert them by hand and recompile.

The good news is that this patch does on my end fix all the problems with the generated SVGs buffer from my recent test, at all window widths.  Unfortunately, checking my original file where this issue was revealed, there are still quite a few “false zero height above” reports.  Happily in that file there are no longer any of the “overly large height” variety, so this particular issue has apparently has been fixed by your patch.

I’ve placed a new test that generates a minimal buffer with a single :file-based SVG overlay at the same gist, and copied/attached them below, for posterity.  I’ve also fixed the bug in my/check-buffer-pixel-values which caused the report buffer not to appear (due to end-of-buffer being signaled), so please update that.  And please note the new svg_test.svg file, to be placed in the same directory.

I don’t think there should be anything special about this particular SVG file; it was automatically generated from the underlying latex fragment.  But it seems to be confusing the display engine somehow (as do many others).

One thing to note about these SVG files is that they have transparent backgrounds, but I don’t think that’s anything unusual.

++++++++
New test:
++++++++

(let ((buf "svg-file-pixel-zero-demo"))
  (with-current-buffer (get-buffer-create buf)
    (erase-buffer)
    (visual-line-mode 1)
    (insert "\nProin quam nisl, tincidunt et, mattis eget, convallis nec, purus.  $\\chi(y) = \\exp\\left(\\sqrt{\\tan(y)}\\right)$  Nullam tempus.")
    (goto-char (point-min))
    (when  (re-search-forward (rx ?$ (* (not ?$)) ?$))
      (let ((ov (make-overlay (match-beginning 0) (match-end 0))))
(overlay-put ov 'display
    `(image :type svg
    :file ,(expand-file-name "svg_test.svg")
    :ascent center)))))
  (pop-to-buffer buf)
  (my/check-buffer-pixel-values))

++++++++++++++
Buffer check code
++++++++++++++

(eval-when-compile (require 'cl-lib))

(defun my/check-buffer-pixel-values ()
  (interactive)
  (goto-char (point-min))
  (let ((line-heights (vconcat (save-excursion
(cl-loop while (< (point) (point-max))
 collect (line-pixel-height)
 do (vertical-motion 1)))))
(line 0)
vmax)
    (with-output-to-temp-buffer "svg-pixel-demo-report"
      (princ (format "SVG Position analysis for %s (width %d, %s)\n\n"
    (current-buffer) (window-width)
    (if (next-single-property-change (point-min) 'display)
"text-properties" "overlays")))
      (while (not (eobp))
(beginning-of-visual-line)
(vertical-motion 1)
(setq vmax (save-excursion (end-of-visual-line) (point)))
(save-excursion
 (while (and (<= (point) vmax) (not (eobp)))
   (let* ((ps (window-text-pixel-size nil (cons (point) -1)
      (point) nil nil nil t))
  (h (nth 1 ps)))
     (unless (= h (aref line-heights line))
(princ
(format "Incorrect at point=%3d: line %2d at %12S (%5s): expected %2d got %2d\n"
(point) (+ line 2) (posn-x-y (posn-at-point))
(if (or (overlays-at (point))
(get-text-property (point) 'display))
    "image"
  (char-to-string (char-after (point))))
(aref line-heights line) h))))
   (forward-char)))
(cl-incf line)))))

++++++++++
Failed hunks:
++++++++++

@ -11436,7 +11436,7 @@
      /* Start at the beginning of the line containing FROM.  Otherwise
    IT.current_x will be incorrectly set to zero at some arbitrary
    non-zero X coordinate.  */
-      reseat_at_previous_visible_line_start (&it);
+      move_it_by_lines (&it, 0);
      it.current_x = it.hpos = 0;
      if (IT_CHARPOS (it) != start)
   {
@@ -11513,6 +11513,8 @@
     the width of the last buffer position manually.  */
  if (IT_CHARPOS (it) > end)
    {
+      int end_y = it.current_y;
+
      end--;
      RESTORE_IT (&it, &it2, it2data);
      x = move_it_to (&it, end, to_x, max_y, -1, move_op);


[-- Attachment #2: svg_test.svg --]
[-- Type: image/svg+xml, Size: 12859 bytes --]

[-- Attachment #3: Type: text/plain, Size: 3272 bytes --]


> On Dec 2, 2023, at 4:44 PM, JD Smith <jdtsmith@gmail.com> wrote:
> 
> Thanks for your work.   The port I use has fallen a few weeks behind master and the patches to xdisp.c unfortunately don’t apply cleanly.  I’ll see about building an NS port and report back. 
> 
>> On Dec 2, 2023, at 2:39 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> Cc: 67533@debbugs.gnu.org
>>> Date: Sat, 02 Dec 2023 16:18:28 +0200
>>> From: Eli Zaretskii <eliz@gnu.org>
>>> 
>>>> From: JD Smith <jdtsmith@gmail.com>
>>>> Date: Sat, 2 Dec 2023 08:36:07 -0500
>>>> Cc: 67533@debbugs.gnu.org
>>>> 
>>>> Strange.  Both of my ports here (NS and Mac) show the same issue.  Did you check the
>>>> svg-pixel-demo-report buffer the header mentions, where the mismatch measurement information
>>>> gets reported?  The specific problems encountered are listed in the report buffer, updated with
>>>> my/check-buffer-pixel-values.  For example, at a standard width of 80 characters, I get this report:
>>>> 
>>>> SVG Position analysis for svg-pixel-demo (width 80, text-properties)
>>>> 
>>>> Incorrect at point= 34: line  2 at   (224 . 14) (image): expected 14 got 28
>>>> Incorrect at point= 99: line  3 at   (244 . 43) (image): expected 29 got 46
>>>> Incorrect at point=126: line  4 at    (42 . 75) (image): expected 32 got 46
>>>> Incorrect at point=162: line  4 at   (345 . 75) (image): expected 32 got 52
>>>> Incorrect at point=210: line  5 at  (133 . 110) (image): expected 35 got 49
>>> 
>>> I never understood I need to look in another buffer.  I will look into
>>> these.
>> 
>> Please try the patch below and see if any problems remain.
>> 
>> diff --git a/src/xdisp.c b/src/xdisp.c
>> index 0b2508c..cc95ab3 100644
>> --- a/src/xdisp.c
>> +++ b/src/xdisp.c
>> @@ -11436,7 +11436,7 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
>>      /* Start at the beginning of the line containing FROM.  Otherwise
>> IT.current_x will be incorrectly set to zero at some arbitrary
>> non-zero X coordinate.  */
>> -      reseat_at_previous_visible_line_start (&it);
>> +      move_it_by_lines (&it, 0);
>>      it.current_x = it.hpos = 0;
>>      if (IT_CHARPOS (it) != start)
>> {
>> @@ -11513,6 +11513,8 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
>>     the width of the last buffer position manually.  */
>>  if (IT_CHARPOS (it) > end)
>>    {
>> +      int end_y = it.current_y;
>> +
>>      end--;
>>      RESTORE_IT (&it, &it2, it2data);
>>      x = move_it_to (&it, end, to_x, max_y, -1, move_op);
>> @@ -11525,8 +11527,13 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
>> 
>>  /* DTRT if ignore_line_at_end is t.  */
>>  if (!NILP (ignore_line_at_end))
>> -    doff = (max (it.max_ascent, it.ascent)
>> -    + max (it.max_descent, it.descent));
>> +    {
>> +      /* If END-1 is on the previous screen line, we need to
>> +                 account for the vertical dimensions of previous line.  */
>> +      if (it.current_y < end_y)
>> + doff = (max (it.max_ascent, it.ascent)
>> + + max (it.max_descent, it.descent));
>> +    }
>>  else
>>    {
>>      it.max_ascent = max (it.max_ascent, it.ascent);
> 

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sat, 2 Dec 2023 22:04:25 -0500
> Cc: 67533@debbugs.gnu.org
> 
> Progress.  The latest patch had two small hunks fail (see below), but they were simple enough that I was able to insert them by hand and recompile.
> 
> The good news is that this patch does on my end fix all the problems with the generated SVGs buffer from my recent test, at all window widths.  Unfortunately, checking my original file where this issue was revealed, there are still quite a few “false zero height above” reports.  Happily in that file there are no longer any of the “overly large height” variety, so this particular issue has apparently has been fixed by your patch.
> 
> I’ve placed a new test that generates a minimal buffer with a single :file-based SVG overlay at the same gist, and copied/attached them below, for posterity.  I’ve also fixed the bug in my/check-buffer-pixel-values which caused the report buffer not to appear (due to end-of-buffer being signaled), so please update that.  And please note the new svg_test.svg file, to be placed in the same directory.
> 
> I don’t think there should be anything special about this particular SVG file; it was automatically generated from the underlying latex fragment.  But it seems to be confusing the display engine somehow (as do many others).

There _is_ something special about that SVG: it is wider that 1/4th of
the default frame width.  If you widen the frame to be wider than
4*209=836 pixels, the problem disappears.  Such wide images are
handles specially by the display engine.

The cumulative patch below should fix all the problems you threw on me
till now.

diff --git a/src/xdisp.c b/src/xdisp.c
index 0b2508c..ca85838 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -11436,7 +11436,7 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
       /* Start at the beginning of the line containing FROM.  Otherwise
 	 IT.current_x will be incorrectly set to zero at some arbitrary
 	 non-zero X coordinate.  */
-      reseat_at_previous_visible_line_start (&it);
+      move_it_by_lines (&it, 0);
       it.current_x = it.hpos = 0;
       if (IT_CHARPOS (it) != start)
 	{
@@ -11513,6 +11513,8 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
      the width of the last buffer position manually.  */
   if (IT_CHARPOS (it) > end)
     {
+      int end_y = it.current_y;
+
       end--;
       RESTORE_IT (&it, &it2, it2data);
       x = move_it_to (&it, end, to_x, max_y, -1, move_op);
@@ -11525,14 +11527,29 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
 
 	  /* DTRT if ignore_line_at_end is t.  */
 	  if (!NILP (ignore_line_at_end))
-	    doff = (max (it.max_ascent, it.ascent)
-		    + max (it.max_descent, it.descent));
+	    {
+	      /* If END-1 is on the previous screen line, we need to
+                 account for the vertical dimensions of previous line.  */
+	      if (it.current_y < end_y)
+		doff = (max (it.max_ascent, it.ascent)
+			+ max (it.max_descent, it.descent));
+	    }
 	  else
 	    {
 	      it.max_ascent = max (it.max_ascent, it.ascent);
 	      it.max_descent = max (it.max_descent, it.descent);
 	    }
 	}
+      else if (IT_CHARPOS (it) > end
+	       && it.line_wrap == TRUNCATE
+	       && it.current_x - it.first_visible_x >= it.last_visible_x)
+	{
+          /* If the display property at END is at the beginning of the
+             line, and the previous line was truncated, we are at END,
+             but it.current_y is not yet updated to reflect that.  */
+          it.current_y += max (it.max_ascent, it.ascent)
+                          + max (it.max_descent, it.descent);
+	}
     }
   else
     bidi_unshelve_cache (it2data, true);
@@ -31343,9 +31360,13 @@ produce_image_glyph (struct it *it)
 
   take_vertical_position_into_account (it);
 
-  /* Automatically crop wide image glyphs at right edge so we can
-     draw the cursor on same display row.  */
-  if ((crop = it->pixel_width - (it->last_visible_x - it->current_x), crop > 0)
+  /* Automatically crop wide image glyphs at right edge so we can draw
+     the cursor on same display row.  But don't do that under
+     word-wrap, unless the image starts at column zero, because
+     wrapping correctly needs the real pixel width of the image.  */
+  if ((it->line_wrap != WORD_WRAP || it->hpos == 0)
+      && (crop = it->pixel_width - (it->last_visible_x - it->current_x),
+	  crop > 0)
       && (it->hpos == 0 || it->pixel_width > it->last_visible_x / 4))
     {
       it->pixel_width -= crop;

bug#67533: SVG images confound position pixel measurements

[JD Smith]

> On Dec 3, 2023, at 8:02 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>> 
>> 
>> I don’t think there should be anything special about this particular SVG file; it was automatically generated from the underlying latex fragment.  But it seems to be confusing the display engine somehow (as do many others).
> 
> There _is_ something special about that SVG: it is wider that 1/4th of
> the default frame width.  If you widen the frame to be wider than
> 4*209=836 pixels, the problem disappears.  Such wide images are
> handles specially by the display engine.
> 
> The cumulative patch below should fix all the problems you threw on me
> till now.

Most excellent, thank you for the sleuthing Eli!  Your roll-up patch applies cleanly and fixes all the pixel size related issues in my large complex org-with-latex-preview file.  I can induce the same behavior in my original svg-generating code by bumping the default width up to:

      (w (+ 142 (* 2 (round (expt (1+ r) 1.25)))))

and it solves it there too. (I’ve updated the gist to do this, and included the final function below, for posterity).

Now, because every good novel has a denouement, there’s... one more thing.    When I was running my/check-buffer-pixel-values in my large latex-preview-laden org file with your new patch, everything was going swimmingly.  No reported problems at all at a variety of frame widths.  But, then, at a single magic frame width (81 chars, but I think this is arbitrary), a bunch of `expected 28 got 14’ errors showed up on one particular line.

A new flavor of under-reported pixel size?  No!  In fact, all the characters on the reported line were yielding the correct size above themselves.  Instead, around this line, (vertical-motion) as well as previous/next line is *skipping a screen line*, confusing my test!  I have sometimes seen this while using up/down arrow to navigate such image-rich files, when an image is wrapped to column zero.  E.g. instead of moving directly up, point jumps to the end of the line above.

Given that the size problems are fixed, I think I should try to isolate this motion problem and submit it as a separate bug.  So far it has eluded a simple reproduction.  I’ve included a short movie of the effect in a gist comment[1] to spurs some thoughts.

[1] https://gist.github.com/jdtsmith/ad765047a6afe20f353de573d8c07da9?permalink_comment_id=4780726#gistcomment-4780726

+++

(defun my/test-svg-positions (arg)
  (interactive "P")
  (let ((buf "svg-pixel-demo")
(default-height (frame-char-height)))
    (with-current-buffer (get-buffer-create buf)
      (erase-buffer)
      (insert "\nPellentesque condimentum, magna ut suscipit hendrerit, ipsum augue ornare nulla, non luctus diam neque sit amet urna.\nEtiam vel tortor sodales tellus ultricies commodo. Curabitur vulputate vestibulum lorem.  Nam euismod tellus id erat.\n\nNullam tristique diam non turpis.\n")
      (goto-char (point-min))
      (cl-loop for i from 1
      for p = (point) then (progn (forward-word) (point))
      while (< p (point-max))
      if (zerop (% i 5)) do
      (let* ((word-start (save-excursion (backward-word) (point)))
     (r0 (/ (float i) 11))
     (r (round (* 10 (- r0 (floor r0))))) ; some psuedo-randoms
     (r2 (round (* 10 (- (* r0 10) (floor (* r0 10))))))
     (h (+ default-height (* 3 r2)))
     (w (+ 142 (* 2 (round (expt (1+ r) 1.25)))))
     (m (/ w 2))
     (svg (svg-create w h)))
(svg-circle svg m m m
    :fill-color (face-foreground 'default)
    :stroke-width 3
    :stroke-color (if (zerop (% i 2)) "green" "red"))
(if arg
    (let ((ov (make-overlay word-start p)))
      (overlay-put ov 'evaporate t)
      (overlay-put ov 'display
   (svg-image svg :ascent 'center)))
  (put-text-property word-start p 'display
     (svg-image svg :ascent 'center)))))
      (pop-to-buffer buf)
      (visual-line-mode 1)
      (my/check-buffer-pixel-values))))

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sun, 3 Dec 2023 10:48:20 -0500
> Cc: 67533@debbugs.gnu.org
> 
> > The cumulative patch below should fix all the problems you threw on me
> > till now.
> 
> Most excellent, thank you for the sleuthing Eli!  Your roll-up patch applies cleanly and fixes all the pixel size related issues in my large complex org-with-latex-preview file.  I can induce the same behavior in my original svg-generating code by bumping the default width up to:
> 
>       (w (+ 142 (* 2 (round (expt (1+ r) 1.25)))))
> 
> and it solves it there too. (I’ve updated the gist to do this, and included the final function below, for posterity).

Thanks, I will install the changes (on master) soon.

> Now, because every good novel has a denouement, there’s... one more thing.    When I was running my/check-buffer-pixel-values in my large latex-preview-laden org file with your new patch, everything was going swimmingly.  No reported problems at all at a variety of frame widths.  But, then, at a single magic frame width (81 chars, but I think this is arbitrary), a bunch of `expected 28 got 14’ errors showed up on one particular line.
> 
> A new flavor of under-reported pixel size?  No!  In fact, all the characters on the reported line were yielding the correct size above themselves.  Instead, around this line, (vertical-motion) as well as previous/next line is *skipping a screen line*, confusing my test!  I have sometimes seen this while using up/down arrow to navigate such image-rich files, when an image is wrapped to column zero.  E.g. instead of moving directly up, point jumps to the end of the line above.
> 
> Given that the size problems are fixed, I think I should try to isolate this motion problem and submit it as a separate bug.  So far it has eluded a simple reproduction.  I’ve included a short movie of the effect in a gist comment[1] to spurs some thoughts.

Yes, a separate bug would be good.

In general, vertical-motion can go awry when there are too many
images, so I'll withdraw judgment until I see the issue.

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> Cc: 67533@debbugs.gnu.org
> Date: Sun, 03 Dec 2023 17:52:12 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: JD Smith <jdtsmith@gmail.com>
> > Date: Sun, 3 Dec 2023 10:48:20 -0500
> > Cc: 67533@debbugs.gnu.org
> > 
> > > The cumulative patch below should fix all the problems you threw on me
> > > till now.
> > 
> > Most excellent, thank you for the sleuthing Eli!  Your roll-up patch applies cleanly and fixes all the pixel size related issues in my large complex org-with-latex-preview file.  I can induce the same behavior in my original svg-generating code by bumping the default width up to:
> > 
> >       (w (+ 142 (* 2 (round (expt (1+ r) 1.25)))))
> > 
> > and it solves it there too. (I’ve updated the gist to do this, and included the final function below, for posterity).
> 
> Thanks, I will install the changes (on master) soon.

Now done.

Is there anything else to do here, or can we close this bug?

Thanks.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

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

I’m sorry to report that this fix seems to have introduced a hard lockup in org-display-inline-images, when displaying images in an org file with larger images.  

I just confirmed by reverting to the state just before your patch was applied to master.  It may have something to do with the final large image fix.

> On Dec 3, 2023, at 11:31 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Cc: 67533@debbugs.gnu.org
>> Date: Sun, 03 Dec 2023 17:52:12 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>>> From: JD Smith <jdtsmith@gmail.com>
>>> Date: Sun, 3 Dec 2023 10:48:20 -0500
>>> Cc: 67533@debbugs.gnu.org
>>> 
>>>> The cumulative patch below should fix all the problems you threw on me
>>>> till now.
>>> 
>>> Most excellent, thank you for the sleuthing Eli!  Your roll-up patch applies cleanly and fixes all the pixel size related issues in my large complex org-with-latex-preview file.  I can induce the same behavior in my original svg-generating code by bumping the default width up to:
>>> 
>>>      (w (+ 142 (* 2 (round (expt (1+ r) 1.25)))))
>>> 
>>> and it solves it there too. (I’ve updated the gist to do this, and included the final function below, for posterity).
>> 
>> Thanks, I will install the changes (on master) soon.
> 
> Now done.
> 
> Is there anything else to do here, or can we close this bug?
> 
> Thanks.


[-- Attachment #2: Type: text/html, Size: 4928 bytes --]

bug#67533: SVG images confound position pixel measurements

[JD Smith]

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

<conjecture>
This has something to do with org-indent or the org-element-cache.  So this may be a “revealed bug” in that code, which was perhaps relying on some flaw in movement around larger images.  
</conjecture>

I don’t think there’s an urgent need to revert; I’ll bring this to the org folk’s attention.

> On Dec 3, 2023, at 4:25 PM, JD Smith <jdtsmith@gmail.com> wrote:
> 
> I’m sorry to report that this fix seems to have introduced a hard lockup in org-display-inline-images, when displaying images in an org file with larger images.  
> 
> I just confirmed by reverting to the state just before your patch was applied to master.  It may have something to do with the final large image fix.
> 
>> On Dec 3, 2023, at 11:31 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> Cc: 67533@debbugs.gnu.org
>>> Date: Sun, 03 Dec 2023 17:52:12 +0200
>>> From: Eli Zaretskii <eliz@gnu.org>
>>> 
>>>> From: JD Smith <jdtsmith@gmail.com>
>>>> Date: Sun, 3 Dec 2023 10:48:20 -0500
>>>> Cc: 67533@debbugs.gnu.org
>>>> 
>>>>> The cumulative patch below should fix all the problems you threw on me
>>>>> till now.
>>>> 
>>>> Most excellent, thank you for the sleuthing Eli!  Your roll-up patch applies cleanly and fixes all the pixel size related issues in my large complex org-with-latex-preview file.  I can induce the same behavior in my original svg-generating code by bumping the default width up to:
>>>> 
>>>>      (w (+ 142 (* 2 (round (expt (1+ r) 1.25)))))
>>>> 
>>>> and it solves it there too. (I’ve updated the gist to do this, and included the final function below, for posterity).
>>> 
>>> Thanks, I will install the changes (on master) soon.
>> 
>> Now done.
>> 
>> Is there anything else to do here, or can we close this bug?
>> 
>> Thanks.
> 


[-- Attachment #2: Type: text/html, Size: 5684 bytes --]

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sun, 3 Dec 2023 16:25:01 -0500
> Cc: 67533@debbugs.gnu.org
> 
> I’m sorry to report that this fix seems to have introduced a hard lockup in org-display-inline-images,
> when displaying images in an org file with larger images.  
> 
> I just confirmed by reverting to the state just before your patch was applied to master.  It may have
> something to do with the final large image fix.

Please provide a recipe to reproduce these lockups.

Thanks.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

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

I am so far unable to find a recipe without including org-mode in the mix.  An org-mode recipe is quite simple:

Create a new org-mode file.
M-x org-indent-mode
M-x visual-line-mode
Insert text below, replacing the image name with a large image of your choosing (wider than the standard frame width), placed in the same directory as your org file
M-x org-display-inline-images
Move point down towards the image
Emacs hangs

+++
* Image
[[./image.png]]


> On Dec 3, 2023, at 10:27 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Sun, 3 Dec 2023 16:25:01 -0500
>> Cc: 67533@debbugs.gnu.org
>> 
>> I’m sorry to report that this fix seems to have introduced a hard lockup in org-display-inline-images,
>> when displaying images in an org file with larger images.  
>> 
>> I just confirmed by reverting to the state just before your patch was applied to master.  It may have
>> something to do with the final large image fix.
> 
> Please provide a recipe to reproduce these lockups.
> 
> Thanks.


[-- Attachment #2: Type: text/html, Size: 1592 bytes --]

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sun, 3 Dec 2023 23:32:44 -0500
> Cc: 67533@debbugs.gnu.org
> 
> 1 Create a new org-mode file.
> 2 M-x org-indent-mode
> 3 M-x visual-line-mode
> 4 Insert text below, replacing the image name with a large image of your choosing (wider than the
>  standard frame width), placed in the same directory as your org file
> 5 M-x org-display-inline-images
> 6 Move point down towards the image
> 7 Emacs hangs

Thanks, I hope I fixed this now.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

> On Dec 4, 2023, at 8:11 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Sun, 3 Dec 2023 23:32:44 -0500
>> Cc: 67533@debbugs.gnu.org
>> 
>> 1 Create a new org-mode file.
>> 2 M-x org-indent-mode
>> 3 M-x visual-line-mode
>> 4 Insert text below, replacing the image name with a large image of your choosing (wider than the
>> standard frame width), placed in the same directory as your org file
>> 5 M-x org-display-inline-images
>> 6 Move point down towards the image
>> 7 Emacs hangs
> 
> Thanks, I hope I fixed this now.

Great, I just recompiled and it’s fixed here too. Will keep an eye out for any other image-display issues.  Thanks for your work on this.

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Mon, 4 Dec 2023 09:14:32 -0500
> Cc: 67533@debbugs.gnu.org
> 
> 
> 
> > On Dec 4, 2023, at 8:11 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> >> From: JD Smith <jdtsmith@gmail.com>
> >> Date: Sun, 3 Dec 2023 23:32:44 -0500
> >> Cc: 67533@debbugs.gnu.org
> >> 
> >> 1 Create a new org-mode file.
> >> 2 M-x org-indent-mode
> >> 3 M-x visual-line-mode
> >> 4 Insert text below, replacing the image name with a large image of your choosing (wider than the
> >> standard frame width), placed in the same directory as your org file
> >> 5 M-x org-display-inline-images
> >> 6 Move point down towards the image
> >> 7 Emacs hangs
> > 
> > Thanks, I hope I fixed this now.
> 
> Great, I just recompiled and it’s fixed here too. Will keep an eye out for any other image-display issues.  Thanks for your work on this.

Any further issues, or can this be closed now?

bug#67533: SVG images confound position pixel measurements

[JD Smith]

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


> On Dec 16, 2023, at 4:32 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>> Great, I just recompiled and it’s fixed here too. Will keep an eye out for any other image-display issues.  Thanks for your work on this.
> 
> Any further issues, or can this be closed now?

I haven’t encountered any, other than the possibly related motion issue with inline images in bug#67604.  I believe it can be closed.  

Given the unexpected issue that cropped up with org-indent and displayed wide images, it may be worth stress-testing this fix in buffers with other unusual/wide images (copying Karthik since I know he often works on latex/org image preview).

Thanks for your work.


[-- Attachment #2: Type: text/html, Size: 1907 bytes --]

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sat, 16 Dec 2023 10:07:33 -0500
> Cc: 67533@debbugs.gnu.org,
>  contact@karthinks.com
> 
>  On Dec 16, 2023, at 4:32 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  Great, I just recompiled and it’s fixed here too. Will keep an eye out for any other
>  image-display issues.  Thanks for your work on this.
> 
>  Any further issues, or can this be closed now?
> 
> I haven’t encountered any, other than the possibly related motion issue with inline images in
> bug#67604.  I believe it can be closed.  

Done.

> Given the unexpected issue that cropped up with org-indent and displayed wide images, it may be
> worth stress-testing this fix in buffers with other unusual/wide images (copying Karthik since I know he
> often works on latex/org image preview).

Any issues with stuff other than window-text-pixel-size should be
submitted as separate bugs.  If there are still issues with
window-text-pixel-size, we can reopen this bug, or start a new one.

Thanks.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

BTW, in case anyone is wondering where window-text-pixel-size with a negative pixel FROM offset is needed, Po Lu's pixel-scroll-precision-up-page is using precisely this approach to measure the content above window-start (so as to smoothly scroll up with high performance):

        (let* ((start (window-start))
               (dims (window-text-pixel-size nil (cons start (- delta))
                                             start nil nil nil t))

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sun, 3 Dec 2023 10:49:37 -0500
> 
> BTW, in case anyone is wondering where window-text-pixel-size with a negative pixel FROM offset is needed, Po Lu's pixel-scroll-precision-up-page is using precisely this approach to measure the content above window-start (so as to smoothly scroll up with high performance):
> 
>         (let* ((start (window-start))
>                (dims (window-text-pixel-size nil (cons start (- delta))
>                                              start nil nil nil t))
> 

I know very well why that feature was added.  My questions was not
about negative offsets in general, but about negative offsets when
FROM is in the first line of the buffer, i.e. when there's no text at
all above that line.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

> On Dec 3, 2023, at 11:33 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Sun, 3 Dec 2023 10:49:37 -0500
>> 
>> BTW, in case anyone is wondering where window-text-pixel-size with a negative pixel FROM offset is needed, Po Lu's pixel-scroll-precision-up-page is using precisely this approach to measure the content above window-start (so as to smoothly scroll up with high performance):
>> 
>>        (let* ((start (window-start))
>>               (dims (window-text-pixel-size nil (cons start (- delta))
>>                                             start nil nil nil t))
>> 
> 
> I know very well why that feature was added.  My questions was not
> about negative offsets in general, but about negative offsets when
> FROM is in the first line of the buffer, i.e. when there's no text at
> all above that line.

Thanks, all set on this end.  Just mentioning the use-case for the casual reader who may wonder what real world impact it has.

bug#67533: SVG images confound position pixel measurements

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Wed, 29 Nov 2023 15:31:43 -0500
>> 
>> ;;; test-svg-pixel-position --- test pixel position for SVG images
>> 
>> ;;; This small code creates a buffer with two lines, the first of
>> ;;; which is long, and the second of which has an SVG image at start.
>> ;;; Line truncation is turned on.  `window-text-pixel-size` returns
>> ;;; differing results depending on whether truncation is actually in
>> ;;; effect (alter the frame width to see this).
>> 
>> ;;; Code:
>> (require 'svg)
>> (let ((buf "svg-pixel-demo")
>>      (svg (svg-create 50 25)))
>>  (svg-circle svg 25 25 25 :stroke-color "green")
>>  (with-current-buffer (get-buffer-create buf)
>>    (erase-buffer)
>>    (insert "Pellentesque condimentum, magna ut suscipit hendrerit, ipsum augue ornare nulla, non luctus diam neque sit amet urna.\n")
>>    (insert (propertize "THISISACIRCLE" 'display (svg-image svg)))
>>    (insert " Aliquam posuere.\n")
>>    (pop-to-buffer buf)
>>    (goto-char (point-max))
>>    (forward-line -1)
>>    (toggle-truncate-lines 1)
>>    (message "PIXEL SIZE OF LINE ABOVE IMAGE: %S"
>> 	     (window-text-pixel-size nil (cons (point) -1) (point) nil nil nil t))))
>
> Does the patch below fix the issue?  (It should fix the recipe you
> posted, but you hinted that this is just the simplest way of seeing a
> more general problem, so I wonder whether that more general problem is
> also fixed.)

Hi,

I have applied your patch to master and here are the results I get with
the recipe at the end of this message:

With a not large enough window:
  FIRST LINE: (925 24 1); ABOVE IMAGE: (925 24 119)

With a large enough window (i.e., that can display the whole first
line):
  FIRST LINE: (1053 24 1); ABOVE IMAGE: (1062 24 119)

--8<---------------cut here---------------start------------->8---
;;; test-svg-pixel-position --- test pixel position for SVG images

;;; This small code creates a buffer with two lines, the first of
;;; which is long, and the second of which has an SVG image at start.
;;; Line truncation is turned on.  `window-text-pixel-size` returns
;;; differing results depending on whether truncation is actually in
;;; effect (alter the frame width to see this).

;;; Code:
(require 'svg)
(let ((buf "svg-pixel-demo")
      (svg (svg-create 50 25)))
  (svg-circle svg 25 25 25 :stroke-color "green")
  (with-current-buffer (get-buffer-create buf)
    (erase-buffer)
    (insert "Pellentesque condimentum, magna ut suscipit hendrerit, ipsum augue ornare nulla, non luctus diam neque sit amet urna.\n")
    (insert "Pellentesque condimentum, magna ut suscipit hendrerit, ipsum augue ornare nulla, non luctus diam neque sit amet urna.\n")
    (insert (propertize "THISISACIRCLE" 'display (svg-image svg)))
    (insert " Aliquam posuere.\n")
    (pop-to-buffer buf)
    (goto-char (point-max))
    (forward-line -1)
    (toggle-truncate-lines 1)
    (let ((above-image (window-text-pixel-size nil (cons (point) -1) (point) nil nil nil t)))
      (forward-line -1)
      (message "FIRST LINE: %S; ABOVE IMAGE: %S"
	       (window-text-pixel-size nil (cons (point) -1) (point) nil nil nil t)
	       above-image))))
--8<---------------cut here---------------end--------------->8---
-- 
Manuel Giraud

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: JD Smith <jdtsmith@gmail.com>,  67533@debbugs.gnu.org
> Date: Fri, 01 Dec 2023 15:40:59 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Does the patch below fix the issue?  (It should fix the recipe you
> > posted, but you hinted that this is just the simplest way of seeing a
> > more general problem, so I wonder whether that more general problem is
> > also fixed.)
> 
> Hi,
> 
> I have applied your patch to master and here are the results I get with
> the recipe at the end of this message:
> 
> With a not large enough window:
>   FIRST LINE: (925 24 1); ABOVE IMAGE: (925 24 119)
> 
> With a large enough window (i.e., that can display the whole first
> line):
>   FIRST LINE: (1053 24 1); ABOVE IMAGE: (1062 24 119)

If you think these results still show a problem, please elaborate.
Since you did this on your system, with your fonts and frame
dimensions, I cannot know whether the numbers are correct or not.

The patch I posted is supposed to fix only one issue: the fact that
the Y-dimension of the line above the image was shown as zero.  AFAIU,
it is indeed not zero in the output you show above.

Btw, I wonder what you and JD expect from the (cons (point) -1)
argument.  The doc string says:

  If FROM is a cons, its car specifies a buffer position, and its cdr
  specifies the vertical offset in pixels from that position to the
  first screen line to be measured.

What is the meaning of negative offset from the first line of the
buffer? there's no screen line at that offset, so what do you expect
that to do?  Or what am I missing?

bug#67533: SVG images confound position pixel measurements

[JD Smith]

> On Dec 1, 2023, at 9:55 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: JD Smith <jdtsmith@gmail.com>,  67533@debbugs.gnu.org
>> Date: Fri, 01 Dec 2023 15:40:59 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>>> Does the patch below fix the issue?  (It should fix the recipe you
>>> posted, but you hinted that this is just the simplest way of seeing a
>>> more general problem, so I wonder whether that more general problem is
>>> also fixed.)
>> 
>> Hi,
>> 
>> I have applied your patch to master and here are the results I get with
>> the recipe at the end of this message:
>> 
>> With a not large enough window:
>>  FIRST LINE: (925 24 1); ABOVE IMAGE: (925 24 119)
>> 
>> With a large enough window (i.e., that can display the whole first
>> line):
>>  FIRST LINE: (1053 24 1); ABOVE IMAGE: (1062 24 119)
> 
> Btw, I wonder what you and JD expect from the (cons (point) -1)
> argument.  The doc string says:
> 
>  If FROM is a cons, its car specifies a buffer position, and its cdr
>  specifies the vertical offset in pixels from that position to the
>  first screen line to be measured.
> 
> What is the meaning of negative offset from the first line of the
> buffer? there's no screen line at that offset, so what do you expect
> that to do?  Or what am I missing?

In that case I would expect zero pixel height is returned.  As you saw, the problem occurs at lines beyond the first visual line.  

My guiding assumption is that the display pixel dimensions returned from (cons (point) -1) to (point) (when omitting the height of the line including (point)) should be equal to the height of the prior visual line (line-pixel-height), unless it has zero height.

I’ll work up a simpler reproducer in non-truncated and/or visual-line-mode.

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Fri, 1 Dec 2023 10:21:19 -0500
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,
>  67533@debbugs.gnu.org
> 
> 
> > Btw, I wonder what you and JD expect from the (cons (point) -1)
> > argument.  The doc string says:
> > 
> >  If FROM is a cons, its car specifies a buffer position, and its cdr
> >  specifies the vertical offset in pixels from that position to the
> >  first screen line to be measured.
> > 
> > What is the meaning of negative offset from the first line of the
> > buffer? there's no screen line at that offset, so what do you expect
> > that to do?  Or what am I missing?
> 
> In that case I would expect zero pixel height is returned.

Why zero?  Why not consider that undefined behavior?

> I’ll work up a simpler reproducer in non-truncated and/or visual-line-mode.

Thanks.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

> On Dec 1, 2023, at 10:36 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Fri, 1 Dec 2023 10:21:19 -0500
>> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,
>> 67533@debbugs.gnu.org
>> 
>> 
>>> Btw, I wonder what you and JD expect from the (cons (point) -1)
>>> argument.  The doc string says:
>>> 
>>> If FROM is a cons, its car specifies a buffer position, and its cdr
>>> specifies the vertical offset in pixels from that position to the
>>> first screen line to be measured.
>>> 
>>> What is the meaning of negative offset from the first line of the
>>> buffer? there's no screen line at that offset, so what do you expect
>>> that to do?  Or what am I missing?
>> 
>> In that case I would expect zero pixel height is returned.
> 
> Why zero?  Why not consider that undefined behavior?

Depends on what the natural height on a non-existent line is.  Zero makes sense to me.  But I suppose returning height=nil or something else to indicate “I gave up” would also be reasonable. 

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Fri, 1 Dec 2023 10:45:00 -0500
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,
>  67533@debbugs.gnu.org
> 
> 
> 
> > On Dec 1, 2023, at 10:36 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> >> From: JD Smith <jdtsmith@gmail.com>
> >> Date: Fri, 1 Dec 2023 10:21:19 -0500
> >> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,
> >> 67533@debbugs.gnu.org
> >> 
> >> 
> >>> Btw, I wonder what you and JD expect from the (cons (point) -1)
> >>> argument.  The doc string says:
> >>> 
> >>> If FROM is a cons, its car specifies a buffer position, and its cdr
> >>> specifies the vertical offset in pixels from that position to the
> >>> first screen line to be measured.
> >>> 
> >>> What is the meaning of negative offset from the first line of the
> >>> buffer? there's no screen line at that offset, so what do you expect
> >>> that to do?  Or what am I missing?
> >> 
> >> In that case I would expect zero pixel height is returned.
> > 
> > Why zero?  Why not consider that undefined behavior?
> 
> Depends on what the natural height on a non-existent line is.  Zero makes sense to me.  But I suppose returning height=nil or something else to indicate “I gave up” would also be reasonable. 

A non-existent line can have any height, including an infinite one.
Since that line doesn't exist, any assertion about it cannot be
disproved.

bug#67533: SVG images confound position pixel measurements

[JD Smith]

> On Dec 1, 2023, at 10:59 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>>>>> What is the meaning of negative offset from the first line of the
>>>>> buffer? there's no screen line at that offset, so what do you expect
>>>>> that to do?  Or what am I missing?
>>>> 
>>>> In that case I would expect zero pixel height is returned.
>>> 
>>> Why zero?  Why not consider that undefined behavior?
>> 
>> Depends on what the natural height on a non-existent line is.  Zero makes sense to me.  But I suppose returning height=nil or something else to indicate “I gave up” would also be reasonable.
> 
> A non-existent line can have any height, including an infinite one.
> Since that line doesn't exist, any assertion about it cannot be
> disproved.

As a general statement, of course.  But since the docs say:

> The optional argument FROM, if non-nil, specifies the first text
> position to consider, and defaults to the minimum accessible position
> of the buffer.


would it not be reasonable that (FROM . (- too-many-pixels)) would also “default to the minimum accessible position”, should the offset prove to be too-many-pixels?  Then, by symmetry, asking for pixel measurements beyond the final accessible line should "default
to the maximum accessible position of the buffer”.  Basically clip the request at the window’s boundaries.  But in any case, the answer to this specific question is I believe a side issue for this bug.

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Fri, 1 Dec 2023 11:17:49 -0500
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,
>  67533@debbugs.gnu.org
> 
> >>> Why zero?  Why not consider that undefined behavior?
> >> 
> >> Depends on what the natural height on a non-existent line is.  Zero makes sense to me.  But I suppose returning height=nil or something else to indicate “I gave up” would also be reasonable.
> > 
> > A non-existent line can have any height, including an infinite one.
> > Since that line doesn't exist, any assertion about it cannot be
> > disproved.
> 
> As a general statement, of course.  But since the docs say:
> 
> > The optional argument FROM, if non-nil, specifies the first text
> > position to consider, and defaults to the minimum accessible position
> > of the buffer.
> 
> 
> would it not be reasonable that (FROM . (- too-many-pixels)) would also “default to the minimum accessible position”, should the offset prove to be too-many-pixels?

No, "defaults to" according to our conventions means "if it's nil or
omitted".  Nonsense values don't activate the default and are not
replaced by the default.  Unless we say something like "any other
value is taken as...".

bug#67533: SVG images confound position pixel measurements

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> The patch I posted is supposed to fix only one issue: the fact that
> the Y-dimension of the line above the image was shown as zero.  AFAIU,
> it is indeed not zero in the output you show above.

Yes, so your patch is the correct fix.  BTW, I was just trying to help
with some more data point.

> Btw, I wonder what you and JD expect from the (cons (point) -1)
> argument.  The doc string says:

And I was not expecting anything here.  I've just used the JD's example.
-- 
Manuel Giraud

bug#67533: SVG images confound position pixel measurements

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: jdtsmith@gmail.com,  67533@debbugs.gnu.org
> Date: Fri, 01 Dec 2023 17:27:25 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The patch I posted is supposed to fix only one issue: the fact that
> > the Y-dimension of the line above the image was shown as zero.  AFAIU,
> > it is indeed not zero in the output you show above.
> 
> Yes, so your patch is the correct fix.  BTW, I was just trying to help
> with some more data point.

Thanks.

> > Btw, I wonder what you and JD expect from the (cons (point) -1)
> > argument.  The doc string says:
> 
> And I was not expecting anything here.  I've just used the JD's example.

Understood.