After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 704449 - Support for 16 million colors
Support for 16 million colors
Status: RESOLVED FIXED
Product: vte
Classification: Core
Component: general
unspecified
Other All
: Low enhancement
: vte-0-36
Assigned To: VTE Maintainers
VTE Maintainers
[fixed-0-36][needed-next][commit:c5a3...
: 700341 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-07-18 09:31 UTC by Anton Kochkov
Modified: 2017-12-14 14:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
image viewer for fun (741 bytes, text/plain)
2014-01-07 03:01 UTC, Egmont Koblinger
  Details
Add true color support (21.03 KB, patch)
2014-01-07 03:08 UTC, Egmont Koblinger
none Details | Review
Add true color support, v2 (23.09 KB, patch)
2014-01-07 04:00 UTC, Egmont Koblinger
none Details | Review
Add true color support, v3 (24.72 KB, patch)
2014-01-08 03:08 UTC, Egmont Koblinger
none Details | Review
Add true color support, v4 (25.61 KB, patch)
2014-01-08 16:14 UTC, Egmont Koblinger
none Details | Review
Add true color support, v5 (28.08 KB, patch)
2014-01-08 18:22 UTC, Egmont Koblinger
committed Details | Review

Description Anton Kochkov 2013-07-18 09:31:25 UTC
ere's a test case

printf "\x1b[38;2;255;100;0mTRUECOLOR\x1b[0m\n"

According to Wikipedia[1], this is only supported by xterm and konsole.

It's a common confusion about terminal colors... Actually we have this:

* plain ascii
* ansi escape codes (16 color codes with bold/italic and background)
* 256 color palette (216 colors+16gray + ansi) (colors are 24bit)
* 24bit true color (8*8*8 colors (aka 16 milion)

The 256 color palete is configured at start, and it's a 6*6*6 cube of
colors, each of them defined as a 24bit (8*8*8 rgb) color.

This means that current support can only display 256 *different* colors
in the terminal, while truecolor means that you can display 16 milion
different colors at the same time.

Truecolor escape codes doesnt uses a color palete. It just specifies the
color itself.

[1] https://en.wikipedia.org/wiki/ANSI_color

Here is another terminals discussions:

st (from suckless) - http://lists.suckless.org/dev/1307/16688.html
urxvt - http://lists.schmorp.de/pipermail/rxvt-unicode/2013q3/001826.html
konsole (already fixed) https://bugs.kde.org/show_bug.cgi?id=138740
sakura https://bugs.launchpad.net/sakura/+bug/1202564
Comment 1 Christian Persch 2013-07-18 09:39:53 UTC
*** Bug 700341 has been marked as a duplicate of this bug. ***
Comment 2 Christian Persch 2013-07-18 09:42:08 UTC
I don't think vte will implement this ever.
Comment 3 Egmont Koblinger 2014-01-05 23:07:31 UTC
Doesn't sound hard to implement at all.

The relevant part of the scrollback is stored in the attr_stream file with records of 16 bytes: 8 bytes offset, 4 bytes attr and 4 bytes unused padding (assuming 64 bit architecture). This means that its size would not grow at all. Memory would grow by 4 bytes for each cell, that is, around 8kB (default size) - ~50kB (my full screen) per terminal, I think it's negligible.

What we really need is a set of applications supporting true color that make spending time on this feature worthwhile. Anton: could you please help here, collecting as many apps as possible that support it (either out of the box, or via a 3rd party patch)?

I remember when I added 256 color support to midnight commander and created the first skin supporting it, I had a hard time living off of 256 colors only, I could have created something way better if I had access to the full palette. Unfortunately mc (as most other apps) uses ncurses or slang, and these (as far as I can tell) don't support true color, it would require them to bypass these libraries when it comes to color. (I might be wrong, though.) I'll take a look how easy it would be to add true colors to mc.

I think it's false that XTerm supports true color. Konsole supports it, as well as st (suckless). I couldn't find any other.

I'd be happy to add this feature, but first I'd like to see reasonably good applications that would benefit from this.

This could be a catch 22: hardly any apps support it due to lack in terminal support, and hardly any terminals support it due to the lack of killer apps...
Comment 4 Egmont Koblinger 2014-01-06 00:19:41 UTC
xterm >= 282 recognizes true color escape sequences, but converts them to the nearest entry in the palette.

It also starts accepting ':' instead of ';' as the delimiter after CSI 38/48, something we should do too. The problem with semicolon is that it's supposed to delimit properties that are independent from each other and take no additional parameters. E.g. vte currently doesn't recognize ^[[38;2;1;3;4 as a true color escape sequence (rgb:1/3/4), but instead it switches to bold(1), italic(3) and underline(4) since it has no clue how many parameters 38;2 is supposed to take.

iTerm also has support: https://code.google.com/p/iterm2/issues/detail?id=218
Comment 5 Egmont Koblinger 2014-01-06 02:15:31 UTC
elinks (if configured with --enable-true-color) supports true colors.

This is a one-liner image viewer, great fun: http://www.reddit.com/r/commandline/comments/1e6oev/24bit_aka_true_color_image_viewer_inside_your/

3rd party patch for vim, I haven't tried: https://bitbucket.org/ZyX_I/vim/commits/5e3877f

That's all I've found so far. Not too much. Adding support to ncurses seems hopeless, it uses "short" data type for colors everywhere. Adding support to slang seems to be hard too, at least it relies on terminfo entries (tput setaf/tput setab) which are shipped by ncurses and only contain support for 256 colors.
Comment 6 Anton Kochkov 2014-01-06 11:51:17 UTC
Also radare2 tool (http://rada.re/) have support for true colors.
Comment 7 Egmont Koblinger 2014-01-06 12:14:09 UTC
Patch for emacs, and link to an ncurses discussion: http://emacs.1067599.n5.nabble.com/RFC-Add-tty-True-Color-support-td299962.html
Comment 8 Egmont Koblinger 2014-01-06 18:07:17 UTC
The work should probably start with the refactoring mentioned in bug 616436 comment 19.
Comment 9 Egmont Koblinger 2014-01-07 03:01:04 UTC
Created attachment 265489 [details]
image viewer for fun

Inspired by the image viewer linked above, here's one that doesn't stretch the image vertically, but rather puts two pixels in each character cell by using U+2580 (upper half block: ▀) characters and setting both the foreground and the background color. Good for fun, good for testing.
Comment 10 Egmont Koblinger 2014-01-07 03:08:53 UTC
Created attachment 265490 [details] [review]
Add true color support

And here's a patch adding true color support. I think it works correctly, but please test thoroughly.

Todo:

- Use more named constants rather than some built-in 256, 8, 16 or alike numbers. The overload of VTE_PALETTE_SIZE having the meaning of offset for old-fashioned code is also ugly.

- Figure out why that attribute packed is required. Input from someone understanding gcc would be appreciated. Apparently a struct consisting of unit64 and uint32 (in this order) is 12 bytes, whereas a struct consisting of a struct of 8 bytes and a uint32 (in this order) is 16 bytes.

- Verify that compiles/works on 32 bit architecture too. (I have 64 bit.)

- Verify that attr_stream's records indeed did not grow.

ChPe, once cleaned up, would you accept this patch/feature for mainstream?
Comment 11 Egmont Koblinger 2014-01-07 03:19:08 UTC
Further todos:

- accept ':' in addition to ';' in the escape sequence (can be a separate bug, not blocking this one, since konsole and st don't support this either)

- parsing params of 38 and 48 duplicates tons of code, factor them out to a helper function

- get rid of the now-obsolete _vte_terminal_map_pango_color() (first understand when/how/why it is called at all)
Comment 12 Egmont Koblinger 2014-01-07 04:00:02 UTC
Created attachment 265504 [details] [review]
Add true color support, v2

Previous version segfaulted when cursor was over a truecolor cell.
Comment 13 Behdad Esfahbod 2014-01-07 08:17:24 UTC
Thanks Egmont!

Patch looks good to me.

Really, please request a git access account and list me and ChPe as references.
Comment 14 Christian Persch 2014-01-07 12:18:25 UTC
(In reply to comment #10)
> - Use more named constants rather than some built-in 256, 8, 16 or alike
> numbers. The overload of VTE_PALETTE_SIZE having the meaning of offset for
> old-fashioned code is also ugly.

Yes please .

> - Figure out why that attribute packed is required. Input from someone
> understanding gcc would be appreciated. Apparently a struct consisting of
> unit64 and uint32 (in this order) is 12 bytes, whereas a struct consisting of a
> struct of 8 bytes and a uint32 (in this order) is 16 bytes.

(BTW, bug 512860 would have added a G_GNUC_PACKED macro, but it was rejected.)
 
> - Verify that compiles/works on 32 bit architecture too. (I have 64 bit.)

I can do that.
 
> ChPe, once cleaned up, would you accept this patch/feature for mainstream?

Sure! :-)

(In reply to comment #11)
> Further todos:
> 
> - accept ':' in addition to ';' in the escape sequence (can be a separate bug,
> not blocking this one, since konsole and st don't support this either)

Yes, let's do this. I filed that already as bug , but it's fine to deal with this here.

> - parsing params of 38 and 48 duplicates tons of code, factor them out to a
> helper function

Yes, let's do that. And have some more input validation (e.g. in  VTE_PALETTE_SIZE + param - 90 + VTE_COLOR_BRIGHT_OFFSET check that @param is in the right range / clamp to the right range).

> - get rid of the now-obsolete _vte_terminal_map_pango_color() (first understand
> when/how/why it is called at all)

It's only called inside the preedit string drawing afaict, and now that with this patch we can draw any colour, not just palette colours, it should not be needed anymore.
Comment 15 Christian Persch 2014-01-07 12:19:48 UTC
(The colour sequences bug is bug 685759.)
Comment 16 Egmont Koblinger 2014-01-07 12:29:36 UTC
(In reply to comment #14)

> Yes, let's do that. And have some more input validation (e.g. in 
> VTE_PALETTE_SIZE + param - 90 + VTE_COLOR_BRIGHT_OFFSET check that @param is in
> the right range / clamp to the right range).

This is okay, @param is the one on you switch()ed.

> > - get rid of the now-obsolete _vte_terminal_map_pango_color() (first understand
> > when/how/why it is called at all)
> 
> It's only called inside the preedit string drawing afaict, and now that with
> this patch we can draw any colour, not just palette colours, it should not be
> needed anymore.

That was my guess too, but in order to verify I'd like to understand how to trigger the execution of this code from the UI - I have no clue what that "preedit string" means. If you happen to know off the top of your head, please explain to me so you save some time for me :)
Comment 17 Christian Persch 2014-01-07 12:37:21 UTC
That's input method support, used e.g. by CJK input. You can try it out by adding a CJK input source in gnome control centre keyboard prefs.
Comment 18 Egmont Koblinger 2014-01-08 02:08:48 UTC
attribute(packed) findings on x86_64:

Bitpacking seems to place everything not crossing an alignment boundary of its own data type. E.g. with fields like "uint32 fore:25" (as in my first patch) the field would not cross a 4 byte boundary by default, unless the structure is attribute(packed). It was only by pure accident that fore:25 and back:25 did not cross the 4 byte boundary, hence the size of VteCellAttr was the desired 8 bytes rather than 12.

Using uint32 member fields for some reason has the effect that later on this structure, when it becomes a member of another structure, behaves differently than an uint64, despite its own size being exactly 64 bits too. The right thing to do is to change all its members to uint64, then this problem disappears. VteCell and VteIntCell both become 16 bytes.

If both VteCell and the inlined structured within VteIntCell get an attribute(packed) then they both become 12 bytes and everything remains okay. I have no preference either way (tiny bit more complicated and slower binary vs wasting 4 bytes per cell), if anyone has, please speak up.
Comment 19 Egmont Koblinger 2014-01-08 03:08:24 UTC
Created attachment 265632 [details] [review]
Add true color support, v3

Some progress made
Comment 20 Egmont Koblinger 2014-01-08 16:14:15 UTC
Created attachment 265722 [details] [review]
Add true color support, v4

Please take a look, I'm planning to submit this.

':' as separator is still a pending issue, it's a nontrivial code change so I'll do it separately.
Comment 21 Christian Persch 2014-01-08 16:37:10 UTC
Review of attachment 265722 [details] [review]:

Thanks, looks good! Just a few minor points below.

::: src/vte-private.h
@@ +83,3 @@
 #define VTE_COLOR_BRIGHT_OFFSET		8
 #define VTE_COLOR_DIM_OFFSET		16
+#define VTE_RGB_COLOR			(1 << 24)

I was going to comment on v3 to do this, and here it is in v4 before I said anything. Perfect! :-)

::: src/vterowdata.h
@@ +47,2 @@
 typedef struct _VteCellAttr {
+	guint64 fragment: 1;	/* A continuation cell. */

I presume using guint64 vs guint32 doesn't have any perf implications on 32-bit, right? If so, this looks fine; but I would be ok with using guint32 with packed attrs, too.

@@ +55,3 @@
+	guint64 italic: 1;
+	guint64 fore: 25;	/* Index into color palette, or direct RGB, */
+	guint64 back: 25;	/* see vte-private.h */

Let's put a comment like /* 4-byte boundary */ between them.

@@ +79,3 @@
  */
 
+typedef struct __attribute__((__packed__)) _VteCell {

Maybe #define VTE_GNUC_PACKED for this?

From bug 512860:
+#if     __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 6)
+#define G_GNUC_PACKED \
+  __attribute__((__packed__))
+#else
+#define G_GNUC_PACKED
+#endif  /* !__GNUC__ */
+

::: src/vteseq.c
@@ +2437,3 @@
+					param4 = g_value_get_long(value4);
+					if (G_LIKELY (param2 >= 0 && param2 < 256 && param3 >= 0 && param3 < 256 && param4 >= 0 && param4 < 256)) {
+						long value = (1 << 24) | (param2 << 16) | (param3 << 8) | param4;

gulong, or guint32.
Comment 22 Egmont Koblinger 2014-01-08 16:55:47 UTC
(In reply to comment #21)

> I was going to comment on v3 to do this, and here it is in v4 before I said
> anything. Perfect! :-)

I said as a todo entry that I'd do it :)

> ::: src/vterowdata.h
> @@ +47,2 @@
>  typedef struct _VteCellAttr {
> +    guint64 fragment: 1;    /* A continuation cell. */
> 
> I presume using guint64 vs guint32 doesn't have any perf implications on
> 32-bit, right? If so, this looks fine; but I would be ok with using guint32
> with packed attrs, too.

I don't have access to 32 bit so I can't tell. As long as we have bug 572210 comment 3, it's sure not a big deal. The other approach led to a bigger maze where I couldn't figure out gcc's behavior, so I wouldn't be confident with that.

> @@ +55,3 @@
> +    guint64 italic: 1;
> +    guint64 fore: 25;    /* Index into color palette, or direct RGB, */
> +    guint64 back: 25;    /* see vte-private.h */
> 
> Let's put a comment like /* 4-byte boundary */ between them.

It's not a functional requirement, but probably does good to performance. Will do.

> Maybe #define VTE_GNUC_PACKED for this?
> +#if     __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 6)

2.7 is 6+ years old, do we really care? Or do we support non-glibc systems w/o attrib packed?

The problem is that by omitting the attribute packed, the subsequent static assertion on the structure size would fail. Branching that assertion on glibc version sounds a bit overengineering (but doable of course). And I'd prefer to keep that assertion so things don't accidentally get out of hands.

Or, I can just drop the packed stuff and live with larger structs.

> +                        long value = (1 << 24) | (param2 << 16) | (param3 <<
> 8) | param4;
> 
> gulong, or guint32.

Okay.
Comment 23 Christian Persch 2014-01-08 16:56:29 UTC
Comment on attachment 265489 [details]
image viewer for fun

Would be great if you could put a copyright & licence header comment in and commit this script, too. Not sure where in the tree... a few scripts are under perf/ so let's put it there until we can reorganise things.
Comment 24 Christian Persch 2014-01-08 16:58:34 UTC
(In reply to comment #22)
> > Maybe #define VTE_GNUC_PACKED for this?
> > +#if     __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 6)
> 
> 2.7 is 6+ years old, do we really care? Or do we support non-glibc systems w/o
> attrib packed?

We don't care, no :-)  So it's fine to just #if __GNUC__ this one; everything else will either work or fail the static assert.
Comment 25 Egmont Koblinger 2014-01-08 18:22:57 UTC
Created attachment 265733 [details] [review]
Add true color support, v5
Comment 26 Egmont Koblinger 2014-01-08 19:00:01 UTC
(Why did I think __GNUC__ stood for glibc version rather than gcc? It didn't make any sense :) and of course gcc 2.7 is waaaaaaay older.)
Comment 27 Christian Persch 2014-01-08 23:09:51 UTC
Comment on attachment 265733 [details] [review]
Add true color support, v5

Looks good to go, thanks! :-)
Comment 28 Egmont Koblinger 2014-01-08 23:16:37 UTC
Fixed in vte-0-36. Keeping open for vte-next.

Followup work (colon as separator) in bug 685759...
Comment 29 Christian Persch 2014-01-09 10:11:25 UTC
Looks great!

The only drawback is that it's rather slow. (Not the fault of the script; if I redirect the output and them just cat it, same happens.) It's using 2 escape sequences per cell, but anything using this for displaying images will likely do it the same way...
Comment 30 Egmont Koblinger 2014-01-09 10:20:37 UTC
Indeed. Unfortunately it's also slow without the patch (when it uses weird colors). Vte in general is slow as hell :(

Also, the runlength encoding of identical attributes for the scrollback is really uneffective here. But cat'ing images won't be the typical use case for this feature.
Comment 32 Egmont Koblinger 2017-12-14 14:43:01 UTC
FYI: This is now being continued/revised in bug 791456.