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

Off-by-one in RGB truecolor escape sequence parameter indices #227

Closed
egmontkob opened this issue Dec 12, 2017 · 9 comments
Closed

Off-by-one in RGB truecolor escape sequence parameter indices #227

egmontkob opened this issue Dec 12, 2017 · 9 comments

Comments

@egmontkob
Copy link

I know you weren't happy with adding ':' as separator, so probably you won't be happy about this one... I'm not happy about this either :-D

VTE's main developer Christian discovered that pretty much everyone misread/misimplemented the ITU Recommendation T.416 at truecolor escape sequences.

Pretty much all terminal emulators that implement true colors interpret the escape sequences the xterm-compatible way:

CSI 38;2;rrr;ggg;bbb m

taking exactly 3 parameters after "38;2;" (the rest, if any, are interpreted as brand new SGR codes).

Some even allow colon as the separator, and ideally also allow (and ignore) further optional parameters:

CSI 38:2:rrr:ggg:bbb[:unused:tolerance:color_space_for_tolerance] m

However, according to T.416 a.k.a. ISO 8613-6, this syntax is wrong. The correct format is (allowing colons only as separator):

CSI 38:2:[color_space_id]:rrr:ggg:bbb[:unused:tolerance:color_space_for_tolerance] m

Most apps that use truecolor emit the xterm-compatible escape sequences, because not too many terminal emulators support ':' as the delimiter, and understandably they choose the easy path which works everywhere. So I wouldn't touch the parameters of the semicolon-based sequence.

With the colon as separator, it would be nice to fix it and adhere to the standard. I'm a bit uncertain if keeping some level of backwards compatibility here (namely: if only three parameters follow the "38:2:" prefix then treat them as R, G, B) is a good idea or not.

Please see the VTE discussion for further details/thoughts.

I'm curious to hear your take on the story, and ideally key players of the story (i.e. popular terminal emulators) would agree on a direction to go.

@kovidgoyal
Copy link
Owner

Is there someplace this standard is publicly viewable, without needing to pay? Because I have zero sympathy for standards that are not freely available. And I have to ask, why do we care about this standard document anyway? No existing terminal emulators implement it. No applications use it. So why are we refering to it as canonical?

You might also want to involve @leonerd in this discussion.

@egmontkob
Copy link
Author

Is there someplace this standard is publicly viewable, without needing to pay?

Google for it as "ISO 8613-6", they'll ask for a sh*tload of money. Google for it as "ITU Recommendation T.416" and the very same document is available for free.

And I have to ask, why do we care about this standard document anyway?

I believe the choice of "38;2;r;g;b" came from this document, I don't know about any other source mentioning anything similar. (Let me please know if you think this is not the case.) However, it was misinterpreted in two different ways, one is the usage of semicolons vs. colons, the other one is the omitted parameter.

I don't necessarily care about this particular standard, but I do care about something being interpreted and then implemented correctly, if at all.

No existing terminal emulators implement it. No applications use it. So why are we refering to it as canonical?

My earlier opinion why semicolon sucks and colon is okay from #160 still applies (and sure does yours, too).

@kovidgoyal
Copy link
Owner

kovidgoyal commented Dec 12, 2017 via email

@kovidgoyal
Copy link
Owner

Also looping @XVilka into this discussion

@leonerd
Copy link

leonerd commented Dec 12, 2017

I don't know of any terminal emulator or application that actually follows that additional parameter for the colour space. It may be that some terminal could apply some logic to count the number of arguments, indeed.

In practice for a sending application, it would be best just to stick to what is conventionally done by a number of terminals now - 38:2:r:g:b rather than including that colourspace as well.

On the subject of T.416, the fact it's not freely available does seriously limit the audience who can read it. At this point I'm trying to collect up what is commonly actually done in terminals, in order to obtain some sort of standard-by-consensus, rather than by reading some old piece of paper that nobody pays attention to these days anyway. My (very much work-in-progress) attempts can be found at

https://docs.google.com/spreadsheets/d/19W-lXWS9jYwqCK-LwgYo31GucPPxYVld_hVEcfpNpXg

@egmontkob
Copy link
Author

  1. Breaks backward compatibility [...]

Here the point is that it's practically not used by anyone (yet?). We can right now pretty safely make this change, it would hardly break anything.

[...] unless you deviate from the standard anyway. In which case, why care about matching the standard?

Luckily, if we insist on not breaking anything at all, this can still be done safely.

If we interpreted 3 additional parameters after "38:2:" as "R, G, B", and 4 or more parameters as "color-space-id, R, G, B, rest", then no valid escape sequences defined in the standard would be interpreted incorrectly. We'd just add some interpretation to something that's invalid according to the standard. And that's absolutely fine, and doesn't count as deviating from the standard any more than supporting semicolons.

  1. Increases complexity throughout the ecosystem.

A tiny little bit. Way less than a mixture of good and bad interpretations does.

  1. Means that setting colors now wastes an extra byte everywhere, all the time just to comply with a dead standard.

You don't have to store that color space anywhere. And I couldn't care less about the cost of those extra ":"s in the data stream while listening to music being streamed along with video that I'm not even looking at :D

@leonerd
Copy link

leonerd commented Dec 12, 2017

If we interpreted 3 additional parameters after "38:2:" as "R, G, B", and 4 or more parameters as "color-space-id, R, G, B, rest", then no valid escape sequences defined in the standard would be interpreted incorrectly. We'd just add some interpretation to something that's invalid according to the standard. And that's absolutely fine, and doesn't count as deviating from the standard any more than supporting semicolons.

Yes, I'd be happy with that as a compromise.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Dec 12, 2017 via email

@egmontkob
Copy link
Author

You hope.

In VTE we have stable branches twice a year, and development (unstable) versions in between. We could temporarily disallow the lack of color-space-id, see if any reports come in, and revert for the stable branch. To have some more concrete feedback (or lack thereof) than a "hope".

So, I will modify kitty to throw away the first parameter of a color sequence if there are four parameters.

Thanks! :)

loumarrero pushed a commit to loumarrero/hterm that referenced this issue Dec 21, 2017
It was pointed out recently that the ISO 8613-6 forms I recently added
support for based on xterm inherited some bugs from xterm.  Namely in
that xterm uses "38:2:R:G:B" when the standard says the form is actually
"38:2:ID:R:G:B:[more args]".  See also the linked bugs for more details.
While reading the spec myself, I opted to overhaul these code paths and
more thoroughly document things on our side, hence this commit is not a
small one.

URL: https://bugzilla.gnome.org/show_bug.cgi?id=791456
URL: kovidgoyal/kitty#227
Change-Id: I4bf6827220b3c5697cd8aa6d65812e51603d5e65
Reviewed-on: https://chromium-review.googlesource.com/828084
Reviewed-by: Brandon Gilmore <varz@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>
vapier added a commit to chromium/hterm that referenced this issue Jan 5, 2018
It was pointed out recently that the ISO 8613-6 forms I recently added
support for based on xterm inherited some bugs from xterm.  Namely in
that xterm uses "38:2:R:G:B" when the standard says the form is actually
"38:2:ID:R:G:B:[more args]".  See also the linked bugs for more details.
While reading the spec myself, I opted to overhaul these code paths and
more thoroughly document things on our side, hence this commit is not a
small one.

URL: https://bugzilla.gnome.org/show_bug.cgi?id=791456
URL: kovidgoyal/kitty#227
Change-Id: I4bf6827220b3c5697cd8aa6d65812e51603d5e65
Reviewed-on: https://chromium-review.googlesource.com/828084
Reviewed-by: Brandon Gilmore <varz@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>
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

No branches or pull requests

3 participants