Skip to content

Use more color than 16 in sixel graphics #271

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

Merged
merged 5 commits into from
Oct 23, 2020
Merged

Conversation

yoichi
Copy link
Contributor

@yoichi yoichi commented Oct 17, 2020

Problem

In sixel backend, the colors are limited to a very small number 16.
If the image specified by the image option uses more colors, the vividness will be lost.

16

Fix

Increase color number from 16 to 128.

128

The color of the beak has been reproduced.

Side Effect

Image rendering time become longer.
On my environment, it takes ~0.24sec with 16 colors -> ~0.4sec with 128 colors

Enironment

  • onefetch runs on Ubuntu20.04
  • mlterm runs on Windows 10

@o2sh o2sh requested a review from shuni64 October 17, 2020 17:05
Copy link
Contributor

@shuni64 shuni64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is that at least xterm isn't required to support more than 16 colors. This isn't because of a flaw with sixel graphics itself, it's because the VT340 doesn't support more than 16 color map entries, so the right behavior from xterm would be to fail above that because it emulates the VT340.
It's possible to increase the amount of colors for emulators that support them, but 16 colors is a safe default that should work on everything emulating the VT340.

The best way to do this would be to query the terminal emulator for the amount of color map entries it supports, but I'm not sure if that's possible.
Alternatively there could be a command line argument to specify that value, that way there shouldn't be any case where sixel images can't be displayed because of a hardcoded value.

@yoichi
Copy link
Contributor Author

yoichi commented Oct 17, 2020

Thanks @CephalonRho for clarifying the reason for the 16 colors.
I understand the existence of terminals that support sixel but not 256 colors and the need for support for them (although I haven't used them myself).

Thanks @spenserblack for suggestion to use environment variable. However, there are situations where that method cannot be used (ssh to target host from the terminal, or docker run from the terminal). We should query the terminal for those cases, but I don't know how to determine color support.

I'll consider a command line argument as @CephalonRho suggested:

Alternatively there could be a command line argument to specify that value, that way there shouldn't be any case where sixel images can't be displayed because of a hardcoded value.

@yoichi yoichi changed the title Use more color (16 -> 128) in sixel graphics Use more color than 16 in sixel graphics Oct 17, 2020
@yoichi
Copy link
Contributor Author

yoichi commented Oct 17, 2020

In 83459a8 I've added a new option --image-colors to specifiy number of colors to be used.
If the option is not given, use 16 colors as before.

image

@yoichi yoichi requested a review from shuni64 October 17, 2020 23:30
@yoichi yoichi requested a review from spenserblack October 21, 2020 15:00
Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as CLI setup goes, looks good to me! 👍
Just one request to change the grammar in the help setting.

I'm not too familiar with sixel, so I'll be asking @CephalonRho to look at the possible_values setting.

.value_name("NUM")
.takes_value(true)
.max_values(1)
.possible_values(&["16", "32", "64", "128", "256"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CephalonRho Can you confirm that these are the possible values that should be used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any number above 0 should work. These seem to be chosen arbitrarily, but they are values that should provide a reasonable looking output.
A solution using a custom validator would be perfect, but it's probably not worth the effort.

@o2sh o2sh merged commit 69306bd into o2sh:master Oct 23, 2020
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.

4 participants