Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request (with working POC): display animated gifs in iTerm2 #54

Closed
rmartine-ias opened this issue Nov 21, 2023 · 5 comments · Fixed by #56
Closed

Feature request (with working POC): display animated gifs in iTerm2 #54

rmartine-ias opened this issue Nov 21, 2023 · 5 comments · Fixed by #56

Comments

@rmartine-ias
Copy link
Contributor

I was trying to display a gif with presenterm in iTerm2, and found it rendered the first frame, but didn't animate it. It would be very useful if I could embed gifs in presentations.

viuer is able to handle this case, but it seems to need to print from a file, as otherwise it encodes the image as a PNG. Here's how the viu tool does it.

This code has issues, but allows presenterm to display animated gifs in iTerm2. It also significantly speeds up loading slides that have large images on them, for some reason.
diff --git a/src/builder.rs b/src/builder.rs
index 5c9021d..a1f95a5 100644
--- a/src/builder.rs
+++ b/src/builder.rs
@@ -367,7 +367,7 @@ impl<'a> PresentationBuilder<'a> {
 
     fn push_image(&mut self, path: PathBuf) -> Result<(), BuildError> {
         let image = self.resources.image(&path)?;
-        self.chunk_operations.push(RenderOperation::RenderImage(image));
+        self.chunk_operations.push(RenderOperation::RenderImage {image, path});
         self.chunk_operations.push(RenderOperation::SetColors(self.theme.default_style.colors.clone()));
         Ok(())
     }
diff --git a/src/diff.rs b/src/diff.rs
index 47bd691..2aaa972 100644
--- a/src/diff.rs
+++ b/src/diff.rs
@@ -71,7 +71,7 @@ impl ContentDiff for RenderOperation {
             (RenderText { alignment: original, .. }, RenderText { alignment: updated, .. }) if original != updated => {
                 false
             }
-            (RenderImage(original), RenderImage(updated)) if original != updated => true,
+            (RenderImage { image: original, .. }, RenderImage{ image: updated, ..}) if original != updated => true,
             (RenderPreformattedLine(original), RenderPreformattedLine(updated)) if original != updated => true,
             (InitColumnLayout { columns: original }, InitColumnLayout { columns: updated }) if original != updated => {
                 true
diff --git a/src/presentation.rs b/src/presentation.rs
index 6c75430..36de14f 100644
--- a/src/presentation.rs
+++ b/src/presentation.rs
@@ -5,7 +5,7 @@ use crate::{
     theme::{Alignment, Margin, PresentationTheme},
 };
 use serde::Deserialize;
-use std::{fmt::Debug, rc::Rc};
+use std::{fmt::Debug, rc::Rc, path::PathBuf};
 
 /// A presentation.
 pub(crate) struct Presentation {
@@ -361,7 +361,7 @@ pub(crate) enum RenderOperation {
     RenderLineBreak,
 
     /// Render an image.
-    RenderImage(Image),
+    RenderImage { image: Image, path: PathBuf},
 
     /// Render a preformatted line.
     ///
diff --git a/src/render/engine.rs b/src/render/engine.rs
index 16a467c..9812e93 100644
--- a/src/render/engine.rs
+++ b/src/render/engine.rs
@@ -13,7 +13,7 @@ use crate::{
     style::Colors,
     theme::Alignment,
 };
-use std::{io, mem};
+use std::{io, mem, path::PathBuf};
 
 pub(crate) struct RenderEngine<'a, W>
 where
@@ -54,7 +54,7 @@ where
             RenderOperation::JumpToBottomRow { index } => self.jump_to_bottom(*index),
             RenderOperation::RenderText { line: texts, alignment } => self.render_text(texts, alignment),
             RenderOperation::RenderLineBreak => self.render_line_break(),
-            RenderOperation::RenderImage(image) => self.render_image(image),
+            RenderOperation::RenderImage{ image, path } => self.render_image(image, path),
             RenderOperation::RenderPreformattedLine(operation) => self.render_preformatted_line(operation),
             RenderOperation::RenderDynamic(generator) => self.render_dynamic(generator.as_ref()),
             RenderOperation::RenderOnDemand(generator) => self.render_on_demand(generator.as_ref()),
@@ -132,10 +132,10 @@ where
         Ok(())
     }
 
-    fn render_image(&mut self, image: &Image) -> RenderResult {
+    fn render_image(&mut self, image: &Image, path: &PathBuf) -> RenderResult {
         let position = CursorPosition { row: self.terminal.cursor_row, column: self.current_rect().start_column };
         MediaRender
-            .draw_image(image, position, self.current_dimensions())
+            .draw_image(image, path, position, self.current_dimensions())
             .map_err(|e| RenderError::Other(Box::new(e)))?;
         // TODO try to avoid
         self.terminal.sync_cursor_row()?;
diff --git a/src/render/media.rs b/src/render/media.rs
index 0767b7b..2f8cec3 100644
--- a/src/render/media.rs
+++ b/src/render/media.rs
@@ -1,6 +1,6 @@
 use crate::render::properties::WindowSize;
 use image::{DynamicImage, ImageError};
-use std::{fmt::Debug, io, rc::Rc};
+use std::{fmt::Debug, io, rc::Rc, path::PathBuf};
 use viuer::ViuError;
 
 use super::properties::CursorPosition;
@@ -41,6 +41,7 @@ impl MediaRender {
     pub(crate) fn draw_image(
         &self,
         image: &Image,
+        image_path: &PathBuf,
         position: CursorPosition,
         dimensions: &WindowSize,
     ) -> Result<(), RenderImageError> {
@@ -73,12 +74,13 @@ impl MediaRender {
         let start_column = dimensions.columns / 2 - (width_in_columns / 2) as u16;
         let start_column = start_column + position.column;
         let config = viuer::Config {
+
             width: Some(width_in_columns),
             x: start_column,
             y: position.row as i16,
             ..Default::default()
         };
-        viuer::print(image, &config)?;
+        viuer::print_from_file(image_path, &config)?;
         Ok(())
     }
 }
Screen.Recording.2023-11-21.at.2.20.03.PM.mov

Some things that the above code wants:

  • Testing/fallback for sixel support -- I couldn't get building presenterm to find libsixel so didn't go too far here. viu has a fallback method it uses.

  • Support for kitty protocol; I didn't test this at all

  • Correct positioning/resizing; this layout bug appears if there's not enough space:

    Screenshot 2023-11-21 at 2 32 19 PM
  • Understanding of why this speeds up loading large PNG images, to make sure those gains are realized (i.e. maybe don't switch to only printing from file for gifs, but maybe for all iterm2 images?)

  • When you navigate to a slide with a gif, it takes a second for the gif to load in. Are there easy fixes for this? (Delay showing slide, have stand-in rectangle displayed so gif popping in is less jarring)

    • This seems like it's iTerm2 being slow; I used imgcat to get all the encoding/escape sequences sorted so you could cat output and get the gif displayed, and it still took ~1s.
@mfontanini
Copy link
Owner

Thanks for the detailed ticket, this is awesome! A few thoughts:

  • At least for kitty, viuer doesn't support gifs. You can display gifs in viu but it's because it renders each frame in a loop. So this is not great because it would only work on some terminals.
    • Note that kitty does support gifs but they don't seem to be implemented here.
  • For kitty, viu's print_from_file basically reads the image and calls print with it so this is actually worse than what we have today as we read each image only once.
  • So because of ^ and ^^ we would need to know which graphics protocol (e.g. iterm2, kitty) viuer will use but we can't because the API in viuer is way too simplistic. So if we wanted to go this route we'd have to either duplicate the logic to determine this or create a ticket/PR in viuer to have a nicer API. e.g. something where you create a "printer" or something and you can tell which protocol you'll use.
  • The layout issue you show is interesting. It would seem that it's not about resizing but about the cursor not being sync'd into the right position after printing the gif (see this). Can you confirm this works for non gif images? e.g. have an image and then text after it and them not overlapping.
  • As for the delay, it's tricky. There is some flickering already when the image is large and this is caused primarily because of this same line again which I'd love to avoid but it's.. maybe hard, I'm not sure. We'd need to make sure our estimate of the height of the image (in terminal rows!) matches what the terminal uses. Like, this sync is there so that our knowledge of the cursor matches the real thing and from what I saw it creates flickering (AFAIK because under the hood it flushes stdout). It's possible there's more happening though, like viuer doing syncs or the terminal simply taking time to show the image.
    • If we know our estimate of the height was always correct tho, we could print a square, finish printing the entire slide and then go back and print the image in the right spot. I think that should be doable.

Anyhow, I think this would be a great addition but I'm mostly concerned about a) this only working in iterm2 and us having issue determining when to use each function in viuer b) the fact that the sync seems to be wrong, but we could probably solve this.

@rmartine-ias
Copy link
Contributor Author

rmartine-ias commented Nov 22, 2023

For a), does viuer::is_iterm_supported work? Playing gifs could be a progressive enhancement where supported.

I figured out the layout issue; I'm dumb -- it occurs when a block quote (or other element) wraps and this isn't taken into account for image placement (happens for non-gifs as well, even without this patch):

Screen Recording 2023-11-22 at 1 05 19 PM mov

Also, as written this code breaks presenting the demo from the git root, because the doge.png path is relative to demo.md, not the CWD. (cding into examples/ fixes this)

@mfontanini
Copy link
Owner

For a), does viuer::is_iterm_supported work?

Yep, that should work. Looks like wezterm supports the iterm2 protocol so I should be able to test this on linux too.

it occurs when a block quote (or other element) wraps

Hmm the wrap around is expected but this should update the cursor location when that happens. I'll have a look.

because the doge.png path is relative to demo.md

Yeah this is intentional. I don't really want your presentation to break/work depending on what your cwd is. Making it relative to the presentation file makes it consistently work (or not).

Thanks for uncovering all these issues!

@mfontanini
Copy link
Owner

Okay both gif support and the preformatted block wrap around bug are merged. Can you double check please? Thanks!

@rmartine-ias
Copy link
Contributor Author

rmartine-ias commented Nov 27, 2023

Both work great! The overlapping is completely fixed, but the size without wrapping seems a bit smaller than before.

Edit: Aha, this happens when a wrap has started but doesn't go to the next actual line

Screen Recording 2023-11-27 at 11 23 39 AM mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants