Refactor popup image feature - #21234

I plan on this to be fully backwards compatible, except now instead of automatically detecting the image protocol to use (for terminal), to use imageprotocol option instead (yet to be implemented), similar to keyprotocol.

A suggestion for improvement:

The SIXEL protocol is the most inferior protocol supported by Vim right now since it's limited to 255 colors. For good results any non trivial image must be properly quantized to 255 colors. While it would be nice to have quantization in Vim, I understand that quantization is a non-trivial problem and not in the scope of the project. After all Vim isn't an image viewer.

I've experimented with quantization through pngquant (properly the best quantization implementation out there) and was able to yield good results.

This image is just 'resize by ImageMagick -> convert to RGBA -> display by Vim'. You can see the artifacts caused by missing quantization:

This one was rendered by 'resize by ImageMagick -> quantize with pngquant -o $OUTPUT --posterize 2 255 -> convert to RGBA -> display by Vim'. The quality is drastically better:

Source code of my highly experimental plugin implementing this can be found here: https://codeberg.org/yamagi/vim-imagepopup

Long story short: It would be very appreciated if Vim provided a way to detect the color depth supported by the active image backend. Or at least what image backend is active. That way a plugin could determine automatically how the image should be processed before it's displayed by Vim.

I plan on this to be fully backwards compatible, except now instead of automatically detecting the image protocol to use (for terminal), to use imageprotocol option instead (yet to be implemented), similar to keyprotocol.

A suggestion for improvement:

The SIXEL protocol is the most inferior protocol supported by Vim right now since it's limited to 255 colors. For good results any non trivial image must be properly quantized to 255 colors. While it would be nice to have quantization in Vim, I understand that quantization is a non-trivial problem and not in the scope of the project. After all Vim isn't an image viewer.

I've experimented with quantization through pngquant (properly the best quantization implementation out there) and was able to yield good results.

This image is just 'resize by ImageMagick -> convert to RGBA -> display by Vim'. You can see the artifacts caused by missing quantization:

This one was rendered by 'resize by ImageMagick -> quantize with `pngquant -o $OUTPUT --posterize 2 255` -> convert to RGBA -> display by Vim'. The quality is drastically better: Source code of my highly experimental plugin implementing this can be found here: https://codeberg.org/yamagi/vim-imagepopup

Long story short: It would be very appreciated if Vim provided a way to detect the color depth supported by the active image backend. Or at least what image backend is active. That way a plugin could determine automatically how the image should be processed before it's displayed by Vim.

I suppose there can be a v: variable for this. However I'm not sure how you would detect the color depth from the image backend? Shouldn't this be possible by just knowing what image backend is being used?

I suppose there can be a v: variable for this. However I'm not sure how you would detect the color depth from the image backend? Shouldn't this be possible by just knowing what image backend is being used?

Good point. Thinking about it the only real advantage of a colordepth variable would be the addition of new backends existing plugins don't know about. But I would suspect that any new backend, be it in GUI or some new terminal protocol, would likely support 24 bit colors. So knowing what backend is used would be enough for plugin authors to special case SIXELs.

I like both of your ideas of a more generic image_add and embedding images inside documents.

I'd suggest keep the idea/impl open to the idea of animated images and fixing to lines with the tallest image setting the lineheight.

Animated images/shaders inside a markdown document templeOS style;

I have conceal on all my markdown formatting and animated images unconcealing to file-refs on cursor over would look so cool, especially if blog writing from within vim.

I've decided to switch to using the libsixel library for the sixel support. Sixel is honestly really complex and has a bunch of edge cases that does not seem right for a text editor to have to implement. Additionally libsixel seems to be very well known and well distributed, and supported on all relevant platforms. This makes maintainance in the future easier

I'm going to put off creating the tests in this PR. I've thought of several solutions, but I think the most suitable one would to literally implement sixel in the builtin terminal (libvterm). Then there could be a term_dumpwrite() like function that can dump a pixel buffer view of the terminal, which can be compared. This should be maybe feasible? (with the help of the Cairo library and maybe Pango to render text? (probably unecessary for testing)). This is a nice feature in its own right as well.

The initial sixel support can be very rudimentary, possibly without scrollback support or sixel scrolling (DECSDM).

This all sounds a bit overkill for just a test suite, but it looks like a fun challenge for me 😄

I have done a big refactor, now pixman is a dependency for the image feature. This is because I need to handle having some parts of the image being under some text (e.g. border). For sixel this is simply a matter of ordering how you draw everything to the screen. However for the kitty graphics protocol and GTK4, images are actual "objects", not just pixels blit to the screen, meaning there has to be a way to figure out what rectangles in an image are visible. Pixman does that, its very fast (SIMD optimized) and well tested, and has no dependencies. Additionally pixman has some image manipulation features that can we possibly expose to the user (e.g. cropping, scaling).

Before:

After:

The kitty and sixel tests are still gone

The branch still deletes Test_popup_image_kitty_leave_tabpage,
Test_popup_image_kitty_covered and
Test_popup_image_sixel_leave_tabpage from test_popupwin.vim, together
with the s:PtyOutput(), s:StartVimWithImageOnPty() and
s:WaitForPtyOutput() helpers, and nothing in test_popupwin.vim
replaces them.

test_image.vim is not the same thing. It checks the arguments of
image_add(), image_discard() and image_info() and that
'imageprotocol' switches v:imagebackend, which is all at the Vim
script level. The tests that went away run Vim on a pty and look at the
bytes it writes: that the placement is deleted when you leave the tab
page and placed again without transmitting the pixels when you come
back, and that a popup with a higher zindex leaves four placements
around it. Those are what 9.2.1074 and 9.2.1109 fixed. With this rewrite
of the drawing code they are exactly the kind of thing that breaks
quietly, and CI stays green.

I reminded you about this, didn't I? And you reacted with a thumbs-up emoji. #21234 (comment)
Please bring them back.

Documentation

Mistakes I noticed while reading:

• builtin.txt: "a blob containg RGB or RGBA data" (containing); "it
  simply loads the image data so that can be reference elsewhere" (so
  that it can be referenced); "the popup window still display the image"
  (displays); the method example of image_discard() is
  id->image_discard without the parentheses.
• builtin.txt: the list says image_info({id}) and Dict, the entry
  says image_info([{id}]) and Return type: list<dict<any>>. The
  argument is optional and a List comes back, so the list is wrong on
  both counts.
• popup.txt: "based on the image dimensions nad terminal/GUI cell size"
  (and); "The dimensions of a popup containing an image is automatically
  calculated" (dimensions are).
• usr_41.txt: "Get information about an image" starts with a capital
  while the two lines above it do not.
• Two spaces after a period, one is used in popup.txt "supported on all
  platforms. The protocol" and in options.txt "the image backend
  changes. This includes".
• E1584, E1585 and E1586 are not in any help file. E1587 is.

Comments in the code

image.h has two empty comment blocks:

/*
 *
 */
typedef struct image_S image_T;

The one above image_placement_S is empty as well. Either say what the
struct is for or drop them.

This one says the opposite of what the flag does:

#define IMAGEF_FORCE 4      // If image backend should use a cache

image_sixel.c uses it the other way around, if (!(place->flags & IMAGEF_FORCE) && ...) takes the cached sequences, so the flag forces
the cache to be skipped.

image_ref(), image_unref() and image_free() have no comment. Who
takes a reference and who drops it is the thing a reader needs here,
more than for the functions around them that do have one.

ga_concat_int() in alloc.c has none either, while every other function
in that file does. And since ga_concat_len() is right there, a word on
why the new one exists would save the next reader the trip: it reports
OOM and it does not stop at a NUL byte.

The GTK4 image removal is replaced by work in screen_char()

9.2.1079 dropped the images that a fill or a scroll writes over, inside
the draw area:

    vim_draw_area_remove_images_in(self, row1, col1, row2, col2);

This branch deletes DrawImage, vim_draw_area_remove_image(),
vim_draw_area_remove_images_in() and gui_gtk4_remove_image(), and
screen.c tells image.c about the cell instead:

    mark_dirty_region_for_images(row, col, 1, 1);

The placement is marked dirty and drawn again rather than dropped, which
should come out the same on the screen. What I am unsure about is the
cost: that call walks all placements and runs
pixman_region32_contains_rectangle() on each, for every cell that is
written. With no image on the screen it returns at once, with one image
a full redraw does it for every cell. Did you measure it? screen_char()
does not have much room to spare.

It also calls redraw_all_later(UPD_VALID) from there while
"updating_screen" is not set. I could not tell by reading whether that
can bounce.

I have no test to point at for this one, 9.2.1079 came without one.

I do not think the performance hit of screen_char is an issue. It only queues a screen redraw if the drawn cell intersects an image. Though I may be wrong, I'll perform a benchmark when I get back to my laptop to verify. Also can you clarify what you mean by "bounce", thanks

As for the removed tests, sorry 😅 . I'm guessing I just removed them because they were failing then just forgot about it.

Thank you for pointing out the many small issues though, I will fix them.

Edit:

and that a popup with a higher zindex leaves four placements
around it

Can you elaborate on this? A popup can not have four image placements.

I meant the check itself, not the redraw. For every cell that is
written, mark_dirty_region_for_images() goes through all placements
and calls pixman_region32_contains_rectangle() on each one. With no
image n_placements is 0 and it returns, so this only shows up when an
image is on the screen. Thanks for running a benchmark. One image up and
a full redraw of a big file, with and without it, should tell.

Sorry, "bounce" was a poor word. I meant it might loop.
mark_dirty_region_for_images() calls redraw_all_later(UPD_VALID)
when "updating_screen" is not set. That redraw writes the cells again,
each write calls it again, and that can ask for another redraw, and so
on. Maybe "updating_screen" is always set there and it stops after one
round. I could not tell from reading.

The pty tests are the ones I want back. They cover 9.2.1074 and 9.2.1109
and do not care how the drawing works. 👍

@h-east I have performed some benchmarks (with some unpushed optimizations). I ran with a 100 image placements of a 512x512 rgba image, which increased the redraw time by ~6 ms. However if i put an early return in mark_dirty_region_for_images(), it only increases by ~5 ms compared to no images. This means the main bottleneck is the popup rendering code itself. Though maybe 1 ms is a big deal? Not sure.

Tested with -O2 (default makefile), foot terminal with sixel backend, and just held Ctrl-L to test in a syntax color heavy file. profile_start() was called before the main drawing logic in update_screen(), and profile_end() was called just before draw_image_placements().

Tried adding the pty tests back but they are failing (despite manual testing being fine), so I'll work on that later.

However I think these pty tests seem awfully hacky and fragile. Once we get testing infrastructure for the image feature then I think we can remove those (and reimplement them of course).

I have only added back one pty test Test_popup_image_kitty_leave_tabpage() because still works with this refactor. Test_popup_image_kitty_covered() I could not get it work, and what it covers was thoroughly manually tested by me. Test_popup_image_sixel_leave_tabpage() does not seem to be needed anymore because the screen is not cleared now when leaving tabpage.

I have only added back one pty test Test_popup_image_kitty_leave_tabpage() because still works with this refactor. Test_popup_image_kitty_covered() I could not get it work, and what it covers was thoroughly manually tested by me. Test_popup_image_sixel_leave_tabpage() does not seem to be needed anymore because the screen is not cleared now when leaving tabpage.

Hmm,
Test_popup_image_kitty_covered() covers what 9.2.1109 fixed, an image
drawn over a popup with a higher zindex. That it no longer works is
itself something this refactor changed, which is a reason to keep the
test, not to drop it.

Testing by hand does not carry over to the next person who touches this
code.

Test_popup_image_sixel_leave_tabpage() checks that leaving the tab
page clears the screen, which is what 9.2.1074 fixed. If the screen is
no longer cleared, the image still has to go somehow, and that is what
the test should check instead. With nothing in its place, the original
bug coming back would not show up.

I put the two tests back, adapted to how this PR works. Both pass
here, and so does the rest of test_image.vim.

Test_popup_image_kitty_covered() is the old one with the z=50 part
of each placement dropped. The split into p=1..4 is what keeps the
image out of the covered cells, and kitty treats any z of 0 or more as
"above the text", so the value itself does not carry the popup's
zindex. In kitty two overlapping image popups still come out in
zindex order.

Test_popup_image_sixel_leave_tabpage() no longer waits for the screen
to be cleared. It checks that the image is not drawn again while the
other tab page is up, and is drawn again on coming back.

diff --git a/src/testdir/test_image.vim b/src/testdir/test_image.vim
index 62b455403..40c5bb855 100644
--- a/src/testdir/test_image.vim
+++ b/src/testdir/test_image.vim
@@ -198,6 +198,11 @@ let s:kitty_place = "\<Esc>_Ga=p,"
 " a=d,d=i: delete the placement of an image, the terminal keeps the pixels.
 let s:kitty_delete = "\<Esc>_Ga=d,d=i,"
 
+" The start of a sixel image, a DCS with the "q" command.
+let s:sixel_start_pat = "\<Esc>P[0-9;]*q"
+" The tab line of the second tab page, drawn after ":tabedit".
+let s:tabline = "[No Name] "
+
 " The script for the Vim on the pty: create a popup with an image.
 let s:image_popup_script =<< trim END
   set imageprotocol=.*:kitty
@@ -206,6 +211,14 @@ let s:image_popup_script =<< trim END
   redraw
 END
 
+" The same with the sixel backend.
+let s:image_popup_script_sixel =<< trim END
+  set imageprotocol=.*:sixel
+  let img = repeat([0xff, 0, 0], 16 * 32)->list2blob()
+  call popup_create('', #{image: #{data: img, width: 16, height: 32}})
+  redraw
+END
+
 " Collect what the Vim on the pty writes in s:pty_out.
 func s:PtyOutput(job, msg)
   let s:pty_out ..= a:msg
@@ -255,4 +268,74 @@ func Test_popup_image_kitty_leave_tabpage()
   endtry
 endfunc
 
+func Test_popup_image_kitty_covered()
+  CheckUnix
+  CheckFeature job
+  CheckFeature image_popup
+
+  let lines =<< trim END
+    set imageprotocol=.*:kitty
+    let img = repeat([0xff, 0, 0], 64 * 64)->list2blob()
+    call popup_create('', #{image: #{data: img, width: 64, height: 64},
+          \ line: 2, col: 2, zindex: 50})
+    let g:cover = popup_create(['xx', 'xx'], #{line: 3, col: 6, zindex: 100})
+    redraw
+  END
+  call writefile(lines, 'XpopupImageTab', 'D')
+  let job = s:StartVimWithImageOnPty()
+  try
+    call s:WaitForPtyOutput(',p=4,', 0)
+    " The row above the cover, the parts left and right of it, the row below.
+    for seq in [',p=1,x=0,y=0,w=64,h=16,', ',p=2,x=0,y=16,w=32,h=32,',
+          \ ',p=3,x=48,y=16,w=16,h=32,', ',p=4,x=0,y=48,w=64,h=16,']
+      call assert_notequal(-1, stridx(s:pty_out, seq), seq)
+    endfor
+
+    " Without the cover one placement is enough, the others are deleted.  The
+    " screen may be drawn once more before that, with the four placements
+    " again: wait for the deletion of the last one.
+    let start = len(s:pty_out)
+    call ch_sendraw(job, ":call popup_close(g:cover)\<CR>")
+    call WaitForAssert({-> assert_match(s:kitty_delete .. 'i=\d\+,p=4,',
+          \ s:pty_out[start :])})
+    call assert_notequal(-1, stridx(s:pty_out, ',p=1,x=0,y=0,w=64,h=64,',
+          \ start))
+    for p in [2, 3, 4]
+      call assert_match(s:kitty_delete .. 'i=\d\+,p=' .. p .. ',',
+            \ s:pty_out[start :])
+    endfor
+  finally
+    call job_stop(job, 'kill')
+    call WaitForAssert({-> assert_equal('dead', job_status(job))})
+  endtry
+endfunc
+
+func Test_popup_image_sixel_leave_tabpage()
+  CheckUnix
+  CheckFeature job
+  CheckFeature image_popup
+
+  call writefile(s:image_popup_script_sixel, 'XpopupImageTab', 'D')
+  let job = s:StartVimWithImageOnPty()
+  try
+    call WaitForAssert({-> assert_match(s:sixel_start_pat, s:pty_out)})
+
+    " Sixel pixels cannot be deleted, the cells they cover are drawn over
+    " instead.  Leaving the tab page does not draw the image again.
+    let start = len(s:pty_out)
+    call ch_sendraw(job, ":tabedit\<CR>")
+    call s:WaitForPtyOutput(s:tabline, start)
+    let mid = len(s:pty_out)
+    call assert_equal(-1, match(s:pty_out[start : mid], s:sixel_start_pat))
+
+    " Entering the tab page draws it again.
+    call ch_sendraw(job, ":tabnext\<CR>")
+    call WaitForAssert({-> assert_notequal(-1,
+          \ match(s:pty_out[mid :], s:sixel_start_pat))})
+  finally
+    call job_stop(job, 'kill')
+    call WaitForAssert({-> assert_equal('dead', job_status(job))})
+  endtry
+endfunc
+
 " vim: shiftwidth=2 sts=2 expandtab

out_str((char_u *)"\033[?80l") at term.c:5450 goes out whenever Vim
is built with FEAT_IMAGE_SIXEL, before a backend has been chosen. A
terminal without sixel answers it with an error: kitty logs
Unsupported screen mode: 80 (private) for every Vim it runs. Nothing
like it comes from master.

Thank you I have applied your patches. I've also made it so that DECSDM is only sent when sixel backend is being used.

Check the following CI errors.

Linux / linux (normal, gcc, vimtags, proto, preproc_indent, encoding, codestyle)
https://github.com/vim/vim/actions/runs/35548797649/job/106179560917?pr=21234

When displaying a popup image in GUI Vim (GTK3) and changing the window size, a SEGV or ASAN error occurs.
(I avoided adding the check to screen_draw_rectangle() because it is a core function that assumes other callers (such as redraw_block()) always pass valid values.)

(ADD:)
When displaying a popup image in GUI Vim (GTK3) and changing the window
size, a SEGV or ASAN error occurs. Any backend that blits pixels is
affected: the GTK3 and MS-Windows GUI, and sixel in a terminal. GTK4
is not, its ".blit" is false.

diff --git a/src/image.c b/src/image.c
index 2e70c5b14..2247a455f 100644
--- a/src/image.c
+++ b/src/image.c
@@ -648,10 +648,16 @@ redraw_region(pixman_region32_t *region, bool now, bool restore)
 
 	if (now)
 	{
+	    // The region may be from before a resize, when the screen was
+	    // bigger.
+	    int height = MIN(rect.y2, screen_Rows) - rect.y1;
+	    int width = MIN(rect.x2, screen_Columns) - rect.x1;
+
 	    // Tip: if you want to debug stale pixels appearing, set "inverse"
 	    // of screen_draw_rectangle() to TRUE.
-	    screen_draw_rectangle(rect.y1, rect.x1,
-		    rect.y2 - rect.y1, rect.x2 - rect.x1, FALSE, TRUE);
+	    if (height > 0 && width > 0)
+		screen_draw_rectangle(rect.y1, rect.x1, height, width,
+			FALSE, TRUE);
 	    continue;
 	}
 
diff --git a/src/testdir/test_image.vim b/src/testdir/test_image.vim
index 40c5bb855..1feaaec88 100644
--- a/src/testdir/test_image.vim
+++ b/src/testdir/test_image.vim
@@ -338,4 +338,34 @@ func Test_popup_image_sixel_leave_tabpage()
   endtry
 endfunc
 
+func Test_popup_image_sixel_screen_shrinks()
+  CheckUnix
+  CheckFeature job
+  CheckFeature image_popup
+
+  " The image covers cells that are outside a smaller screen.
+  let lines =<< trim END
+    set imageprotocol=.*:sixel
+    let img = repeat([0xff, 0, 0], 400 * 400)->list2blob()
+    call popup_create('', #{image: #{data: img, width: 400, height: 400},
+          \ line: 3, col: 20})
+    redraw
+  END
+  call writefile(lines, 'XpopupImageTab', 'D')
+  let job = s:StartVimWithImageOnPty()
+  try
+    call WaitForAssert({-> assert_match(s:sixel_start_pat, s:pty_out)})
+
+    " Clearing the smaller screen must not look at the cells the image had
+    " on the bigger one.
+    let start = len(s:pty_out)
+    call ch_sendraw(job, ":set columns=40 lines=10\<CR>")
+    call WaitForAssert({-> assert_match(s:sixel_start_pat, s:pty_out[start :])})
+    call assert_equal('run', job_status(job))
+  finally
+    call job_stop(job, 'kill')
+    call WaitForAssert({-> assert_equal('dead', job_status(job))})
+  endtry
+endfunc
+
 " vim: shiftwidth=2 sts=2 expandtab

I have applied your patch, thanks

thanks. Can you squash and fix the wrong preproc indentation?

thanks. Can you squash and fix the wrong preproc indentation?

Done

Okay, this is a bit massiv to review. But one thing I noticed. The new dependency for pixman does not appear inside ./configure --help. Can you please add it there along with an optional switch to enable/disable it?

I have included it now with a few more changes: I updated the configure script, I documented the new error codes, I updated optwin.vim, quickref.txt and version9.txt and the vim syntax script, I added those changes from 89c0f36 and re-generated the pot file.

Thanks for working on that and image feature.

BTW: I tried to reach you, but you did not reply, can you please contact me privately?

I have included it now with a few more changes: I updated the configure script, I documented the new error codes, I updated optwin.vim, quickref.txt and version9.txt and the vim syntax script, I added those changes from 89c0f36 and re-generated the pot file.

Thank you

BTW: I tried to reach you, but you did not reply, can you please contact me privately?

Sorry, I guess I missed your email (I am busy with university right now). I have responded back to your latest however