The approach implemented is to track how many lines have been scrolled up

This sounds like it might have a significant performance impact. Have you actually tested the performance of this PR?

The grid region under a new graphic is filled with empty cells, and a non-breaking space (U+00A0) is added to the bottom-left of the graphic. This is just a mark to indicate that the grid has some content up to the bottom of the graphic. This is necessary in situations where there is no text after the graphic. For example, if we execute this script:

That sounds extremely hacky, which I am not a fan of. This will likely just cause an endless heap of issues with things like selection, so it's not something we can just throw in and forget about.

This patch also includes support for the iTerm2 inline images protocol, but only for drawing images.

This seems to just pile on a heap of code to support a bunch of image protocols. I don't like the idea of adding thousands of lines to Alacritty for something with so little use. If we support any protocol it should be the simplest one without any performance impact. I see no benefit in supporting multiple formats.

The OpenGL extensions GL_ARB_clear_texture and GL_ARB_copy_image can be used to resize a texure. If the hardware does not have these extensions, the implementation uses a fallback fully compatible with OpenGL 3.3.

What about support for devices below OpenGL 3.3? This is something we would like to look into for the future so adding more code that requires at least OpenGL 3.3+ does not seem ideal.

The following crates have been added as dependencies

I see little reason for adding memoffset/lazy_static usually, but I haven't looked at the code yet.

I've got

error[E0599]: no function or associated item named with_size found for struct alacritty_terminal::graphics::GraphicData in the current scope
--> alacritty/src/renderer/graphics/prepare.rs:319:44
|
319 | pending_graphics.push(GraphicData::with_size(
| ^^^^^^^^^ function or associated item not found in alacritty_terminal::graphics::GraphicData

when try to compile from this branch.

Same

The approach implemented is to track how many lines have been scrolled up

This sounds like it might have a significant performance impact. Have you actually tested the performance of this PR?

I launched vtebench three times, in the current master branch and in this patch.

Running cat with a 635K-lines file the performance counters also looks almost identical:

$ base64 ./target/release/alacritty > /tmp/data

$ wc -l /tmp/data
635257 /tmp/data


# graphics branch
$ perf stat -e cycles,instructions,branches,branch-misses ./target/release/alacritty -e cat /tmp/data

 Performance counter stats for './target/release/alacritty -e cat /tmp/data':

     4,590,935,968      cycles
    10,125,002,121      instructions              #    2.21  insn per cycle
     2,038,270,623      branches
         4,651,069      branch-misses             #    0.23% of all branches

       0.854657772 seconds time elapsed

       0.774462000 seconds user
       0.466697000 seconds sys


# master branch
$ perf stat -e cycles,instructions,branches,branch-misses ./target/release/alacritty  -e cat /tmp/data

 Performance counter stats for './target/release/alacritty -e cat /tmp/data':

     4,692,460,970      cycles
    10,053,624,431      instructions              #    2.14  insn per cycle
     2,035,973,894      branches
         4,795,961      branch-misses             #    0.24% of all branches

       0.855561156 seconds time elapsed

       0.762745000 seconds user
       0.493408000 seconds sys

The grid region under a new graphic is filled with empty cells, and a non-breaking space (U+00A0) is added to the bottom-left of the graphic. This is just a mark to indicate that the grid has some content up to the bottom of the graphic. This is necessary in situations where there is no text after the graphic. For example, if we execute this script:

That sounds extremely hacky, which I am not a fan of. This will likely just cause an endless heap of issues with things like selection, so it's not something we can just throw in and forget about.

I changed the NBSP character with a GRAPHICS flag in Cell.

This patch also includes support for the iTerm2 inline images protocol, but only for drawing images.

This seems to just pile on a heap of code to support a bunch of image protocols. I don't like the idea of adding thousands of lines to Alacritty for something with so little use.

Without comments and tests, the support for the iTerm2 protocol is less than 100 lines of code.

If we support any protocol it should be the simplest one without any performance impact. I see no benefit in supporting multiple formats.

The only performance impact of this protocol is an extra branch in the osc_dispatch function.

The OpenGL extensions GL_ARB_clear_texture and GL_ARB_copy_image can be used to resize a texure. If the hardware does not have these extensions, the implementation uses a fallback fully compatible with OpenGL 3.3.

What about support for devices below OpenGL 3.3? This is something we would like to look into for the future so adding more code that requires at least OpenGL 3.3+ does not seem ideal.

The fallback uses glTexImage2D, glTexSubImage2D, and glGetTexImage. All those functions exist since OpenGL 1.0.

I've got

error[E0599]: no function or associated item named with_size found for struct alacritty_terminal::graphics::GraphicData in the current scope
--> alacritty/src/renderer/graphics/prepare.rs:319:44

This issue is now fixed.

Without comments and tests, the support for the iTerm2 protocol is less than 100 lines of code.

That doesn't justify adding it. If Alacritty supports any graphics protocol, there should be one and it should be the simplest one available.

I'm also getting the following error:

There was an error initializing the shaders: Failed linking shader: error: Too many fragment shader texture samplers

There was an error initializing the shaders: Failed linking shader: error: Too many fragment shader texture samplers

What is the output of glxinfo -l | grep GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS?

96 or something like that iirc.

I uploaded a fix in 387a224 to check if this fixes the problem. If this works, we may need to generate the code of the fragment shader dynamically, in order to use the 32 units if they are available in the hardware.

That works and shows a consistent performance decrease:

@ayosec did you test the iTerm2 protocol with ranger? previewed images are kept on screen and not cleared, so new images are overlayed on top of the old ones, and images are kept in the background when text is rendered in the preview pane.

Another issue is that the iterm2 graphics is working with tmux as long as the image file is small, but with large files, it looks like I'm hitting this, which is not an Alacritty issue but Alaciritty is spamming with warning messages like this:

[2021-02-08 15:09:45.743684401] [WARN ] [graphics] Can't decode base64 data: Encoded text cannot have a 6-bit remainder.

You should be able replicate with:

• This pdf file: ftp://ftp.pwg.org/pub/pwg/candidates/cs-pwgmsn10-20020226-5101.1.pdf.
• Image preview method set as iterm2 in ranger's rc.conf.
• pdf image preview (pdftoppm) enabled (uncomment the code) in ranger's scope.sh.
• And of course running a tmux session (I actually use byobu).

Note that this issue doesn't happen when running ranger directly without going through a tmux session, and not when running in an ssh session.

@tinywrkb

did you test the iTerm2 protocol with ranger? previewed images are kept on screen and not cleared, so new images are overlayed on top of the old ones, and images are kept in the background when text is rendered in the preview pane.

Ranger relies on overwriting images with spaces. The discussion in #910 rejected my first approach to be able to have this functionality. However, if we can change the implementation in this patch to use CellExtra (I asked about that in the pull-request description), it could be possible to use Ranger without changes.

As a work-around, images can be replaced with the █ character (reversed with CSI 7 m).

Another issue is that the iterm2 graphics is working with tmux as long as the image file is small, but with large files, it looks like I'm hitting this, which is not an Alacritty issue but Alaciritty is spamming with warning messages like this:

[2021-02-08 15:09:45.743684401] [WARN ] [graphics] Can't decode base64 data: Encoded text cannot have a 6-bit remainder.

This error happens because the base64 stream is incomplete.

Did you use iTerm2 imgcat? It seems that tmux requires specific sequences to send images.

Anyways, since iTerm2 will not be accepted, maybe it's not worth it to spend time debugging the issue.

Did you use iTerm2 imgcat? It seems that tmux requires specific sequences to send images.

I can replicate the issue with the linked imgcat script and an image. I won't attach the image here because I'm not sure about its license but I can send it via email.

Anyways, since iTerm2 will not be accepted, maybe it's not worth it to spend time debugging the issue.

That's unfortunate, it would have been nice to have it.

I guess it's better to wait for sixel to support 24-bit colors?

Sixel isn't likely to change.

I have updated the patch with the following changes:

• Data needed to render graphics is now stored in a new field in CellExtra.
  • This change removes the extra code used to update the (now removed) base_position field.
  • In this version, adding new content to the grid does not have any overhead.
• Fixed the error reported in Add support for libsixel #910 (comment).
• The implementation is now much simpler.
  • Since the references to the graphics are now stored in the cells, much of the initial complexity (like OpenGL extensions, or the prepare/draw phases) was unnecessary.
  • The diff size is reduced from 3.8K to 1.9K lines.
  • Some of the initial issues (like moving lines, or text reflow) are solved.
• Removed support for iTerm2 protocol.

Can your implementation of sixel convert 24bit colors into 256 colors and display a video efficiently?
The bit conversion takes some computing power.

From your comment, @roland-5, I'm curious if the current maintainers would accept the author of this PR as another maintainer to "take care of the graphics-related code" within Alacritty, if the author of this PR has time and interest in doing so…

That just doesn't make any sense.

I feel like this is a perfect solution. As opposed to @ayosec having to maintain a separate fork, pulling from Alacritty when there is an update, just have him maintain the code you aren't willing to, as he is obviously willing to do so. (❤ you ayosec) I would love to see this added into the main Alacritty repo, even if it is just an option that you have to enable manually. I love the speed, flexibility, and customization of Alacritty, but it seems like, at this moment, a great PR is being held back by a locked door. @ayosec has already unlocked it with the key, but the maintainer keeps the door closed with a deadbolt, insisting that the door just "won't open", even if he wanted it too.

@ayosec keep up the great work, hope this request (eventually) gets merged, or I might have to move away from Alacritty in search of a project with more understanding maintainers.

Is there a way to scale all graphics by a fixed ratio?

As a quick hack, you can adjust the cell dimensions (to increase how many cells are under the image), and the size in the vertex data.

For example, to increase the size by a 50%, something like this should work:

diff --git a/alacritty/src/renderer/graphics/draw.rs b/alacritty/src/renderer/graphics/draw.rs
index eb1cbad3..433db23d 100644
--- a/alacritty/src/renderer/graphics/draw.rs
+++ b/alacritty/src/renderer/graphics/draw.rs
@@ -98,8 +98,8 @@ impl RenderList {
                 sides: TopLeft,
                 column: render_item.column.0 as GLuint,
                 line: render_item.line as GLuint,
-                height: graphic_texture.height,
-                width: graphic_texture.width,
+                height: graphic_texture.height * 3 / 2,
+                width: graphic_texture.width * 3 / 2,
                 offset_x: render_item.offset_x,
                 offset_y: render_item.offset_y,
                 base_cell_height: graphic_texture.cell_height,
@@ -112,7 +112,7 @@ impl RenderList {
             }

             if render_item.show_hint {
-                let scale = size_info.cell_height() / graphic_texture.cell_height;
+                let scale = size_info.cell_height() / graphic_texture.cell_height * 1.5;

                 let x = render_item.column.0 as f32 * size_info.cell_width()
                     - render_item.offset_x as f32 * scale;
diff --git a/alacritty_terminal/src/graphics/mod.rs b/alacritty_terminal/src/graphics/mod.rs
index 5ecdf29c..4cc63dc4 100644
--- a/alacritty_terminal/src/graphics/mod.rs
+++ b/alacritty_terminal/src/graphics/mod.rs
@@ -473,8 +473,8 @@ pub fn insert_graphic<L: EventListener>(
     graphic: GraphicData,
     palette: Option<Vec<Rgb>>,
 ) {
-    let cell_width = term.graphics.cell_width as usize;
-    let cell_height = term.graphics.cell_height as usize;
+    let cell_width = (term.graphics.cell_width / 1.5) as usize;
+    let cell_height = (term.graphics.cell_height / 1.5) as usize;

     // Store last palette if we receive a new one, and it is shared.
     if let Some(palette) = palette {

@bytesnake Can you share your use-case? If this is something that other people may use, we can add a configuration option.

@bytesnake Can you share your use-case? If this is something that other people may use, we can add a configuration option.

I have a set of notes cached to sixel graphics for fast previewing, I multiplex them with tmux and join session from computers with different resolution. Tmux keeps aspect ratios of panes fixed, leading to note preview not fitting to pane anymore. This is quite niche, I attached a patch adding a graphics.sixel_scaling key to the configuration for flexible scaling.

sixel-scaling.patch.txt

Your sample works, except for the position of the prompt okay it doesn't seem to work properly, the graphics is cut off and tmux messes up my vim.
scaling-example.mp4

i need this but for typst

I have a set of notes cached to sixel graphics for fast previewing, I multiplex them with tmux and join session from computers with different resolution. Tmux keeps aspect ratios of panes fixed, leading to note preview not fitting to pane anymore.

If Sixel images are sent through tmux I'm not sure if we can scale it after receiving it.

tmux needs to know how many cells are written before inserting the image, and it may adjust the image when needed. For example, screen_write_sixelimage checks if the position after the image exceeds the screen height; if the image is scaled up, tmux can try to write an image that will scroll the grid because it thinks that it fits in the view (which may be the cause of the behaviour in your video).

Do you know if this feature is available in other terminals? Maybe there is a way to solve the problem that I'm missing.

Any updates?

What are we missing for this PR to be merged?

What are we missing for this PR to be merged?

maintainer outlined a few things here. more or less the size of the code seems to be the biggest problem, then also qualms about the sixel API itself.

#4763 (comment)

Honestly, with the nature of their concerns, I think they're leading us and ayosec on. They should probably just close this PR. Anyone who likes this PR enough is just using ayosec's fork anyway.

Honestly, with the nature of their concerns, I think they're leading us and ayosec on.

I think I've made it pretty clear that this has little chance of ever getting upstreamed. Considering the great work ayosec has done writing and maintaining it, I just don't think it would be fair to close the PR and banish it into the void. There's nothing wrong with alternative Alacritty distributions for people with differing preferences.