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

Fix Sixel rendering of GIF images with transparency #148

Merged
merged 1 commit into from Jun 4, 2023

Conversation

Delgan
Copy link
Contributor

@Delgan Delgan commented Jun 4, 2023

Fix #147 (kind of).

As suggested, I just replaced P2=1 with P2=0 while generating Sixel images.

However, this is rather disappointing. Not all terminals seem to support this parameter very well.

Screenshots are more descriptive than words. I configured the terminal with a background when I could.

Layout is:

wezterm | konsole | mlterm
--------------------------
 xterm  |  zellij |  foot

With P2=1 (current)

chafa -f sixels -d 1 poke.gif
p2eq1_chafa

cat poke.sixel
p2eq1_cat


With P2=0 (new)

chafa -f sixels -d 1 poke.gif
p2eq0_chafa

cat poke.sixel
p2eq0_cat


That looks like an improvement for foot but a regression for wezterm and zellij.
I think wezterm supports Kitty protocol, so I assume it will be preferred over Sixel most of the time?
Regarding zellij, rendering of GIF is already quite poor because it causes a lot of flickering (regardless of P2 value).
Finally, for xterm, it fixes the layered frames but at the cost of a black background even for static images.

What are your thoughts on these results?

@hpjansson hpjansson merged commit d638470 into hpjansson:master Jun 4, 2023
1 check passed
@hpjansson
Copy link
Owner

Ok, those are great screenshots. It's worse than I thought; in fact only foot seems to get both cases right (P2=1 -> OVER and P2=0 -> SRC). The others are always SRC and/or replace alpha with black.

I took a quick look at the xterm source, and it reflects the general confusion (the spec is unclear):

	if (Pbgmode == 1) {
	    context.background = COLOR_HOLE;
	} else {
	    /* FIXME: is the default background register always zero?  what about in light background mode? */
	    context.background = 0;
	}

Regarding wezterm: We're currently defaulting to sixels there. It was the only option when I tested it -- if it has good kitty support, we should switch (one-liner in chafa-term-db.c, or a few more lines if we need to/can make a distinction for different versions). Feel like submitting a PR?

It's close, but I think losing transparency (or getting the wrong background color, depending on how you look at it) in some situations is preferable. We can try to convince terminal maintainers to fix this/change their approach to match foot's. It seems like the most useful spec interpretation.

I think the most futureproof™ way of dealing with any remaining inconsistencies would be to implement ChafaImage/ChafaAnim/ChafaPlacement classes, so the library backend has a concept of animation, and then advertising terminal tweaks in ChafaTermInfo so it can do the right thing depending on terminal. That's a fair amount of work, though, and requires careful planning (for instance, the app should be able to feed animations incrementally via a callback or stream interface to avoid loading it all into memory at once).

In the meantime, let's merge this. Thanks a lot!

@Delgan
Copy link
Contributor Author

Delgan commented Jun 10, 2023

in fact only foot seems to get both cases right (P2=1 -> OVER and P2=0 -> SRC)

Yes, that is one of the reasons why I like foot so much. He pays great attention to detail and correctness.

I took a quick look at the xterm source, and it reflects the general confusion (the spec is unclear):

	if (Pbgmode == 1) {
	    context.background = COLOR_HOLE;
	} else {
	    /* FIXME: is the default background register always zero?  what about in light background mode? */
	    context.background = 0;
	}

Nice finding. It explains the observed behavior.

Regarding wezterm: We're currently defaulting to sixels there. It was the only option when I tested it -- if it has good kitty support, we should switch (one-liner in chafa-term-db.c, or a few more lines if we need to/can make a distinction for different versions). Feel like submitting a PR?

Thanks for the hint. I opened #149. 👍

We can try to convince terminal maintainers to fix this/change their approach to match foot's. It seems like the most useful spec interpretation.

I agree, this is the kind of thing where we can consider that "it must be fixed upstream". Of course, in practice, I don't know if it's possible and if all terminals are willing to make this change.

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 this pull request may close these issues.

Rendering issue for GIF with transparency on some terminals (using Sixel encoding)
2 participants