Page MenuHomeFreeBSD
Differential D13645 Diff 37172 sys/dev/vt/colors/vt_termcolors.c

Changeset View

Standalone View

View Options

sys/dev/vt/colors/vt_termcolors.c

Show All 27 Lines
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE. * SUCH DAMAGE.
*/ */
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/kernel.h>
#include <sys/libkern.h>
#include <dev/vt/colors/vt_termcolors.h> #include <dev/vt/colors/vt_termcolors.h>
static const struct { #define NCOLORS 16
static struct {
unsigned char r; /* Red percentage value. */ unsigned char r; /* Red percentage value. */
unsigned char g; /* Green percentage value. */ unsigned char g; /* Green percentage value. */
unsigned char b; /* Blue percentage value. */ unsigned char b; /* Blue percentage value. */
} color_def[16] = { } color_def[NCOLORS] = {
{0, 0, 0}, /* black */ {0, 0, 0}, /* black */
{50, 0, 0}, /* dark red */ {50, 0, 0}, /* dark red */
{0, 50, 0}, /* dark green */ {0, 50, 0}, /* dark green */
{77, 63, 0}, /* dark yellow */ {77, 63, 0}, /* dark yellow */
{20, 40, 64}, /* dark blue */ {20, 40, 64}, /* dark blue */
{50, 0, 50}, /* dark magenta */ {50, 0, 50}, /* dark magenta */
{0, 50, 50}, /* dark cyan */ {0, 50, 50}, /* dark cyan */
{75, 75, 75}, /* light gray */ {75, 75, 75}, /* light gray */
{18, 20, 21}, /* dark gray */ {18, 20, 21}, /* dark gray */
{100, 0, 0}, /* light red */ {100, 0, 0}, /* light red */
{0, 100, 0}, /* light green */ {0, 100, 0}, /* light green */
{100, 100, 0}, /* light yellow */ {100, 100, 0}, /* light yellow */
{45, 62, 81}, /* light blue */ {45, 62, 81}, /* light blue */
{100, 0, 100}, /* light magenta */ {100, 0, 100}, /* light magenta */
{0, 100, 100}, /* light cyan */ {0, 100, 100}, /* light cyan */
{100, 100, 100}, /* white */ {100, 100, 100}, /* white */
}; };
/* /*
* Between console's palette and VGA's one: * Between console's palette and VGA's one:
* - blue and red are swapped (1 <-> 4) * - blue and red are swapped (1 <-> 4)
* - yellow ad cyan are swapped (3 <-> 6) * - yellow ad cyan are swapped (3 <-> 6)
*/ */
static const int cons_to_vga_colors[16] = { static const int cons_to_vga_colors[NCOLORS] = {
0, 4, 2, 6, 1, 5, 3, 7, 0, 4, 2, 6, 1, 5, 3, 7,
0, 4, 2, 6, 1, 5, 3, 7 0, 4, 2, 6, 1, 5, 3, 7
}; };
static int
vt_parse_rgb_triplet(const char *rgb, size_t len,
unsigned char *r, unsigned char *g, unsigned char *b)
{
unsigned long v;
const char *ptr;
char *endptr;
ptr = rgb;
/* Handle #rrggbb case */
if (*ptr == '#') {
if (strlen(ptr) != 7)
return (-1);
v = strtoul(ptr + 1, &endptr, 16);
if (*endptr != '\0')
return (-1);
*r = (v >> 16) & 0xff;
*g = (v >> 8) & 0xff;
*b = v & 0xff;
return (0);
}
/* "r, g, b" case */
jillesUnsubmitted
Not Done
Inline Actions

Hmm, perhaps supporting only #rrggbb is sufficient; this reduces verbose and error-prone code. If so, ignore my comments about the r,g,b parser below :)

jilles: Hmm, perhaps supporting only `#rrggbb` is sufficient; this reduces verbose and error-prone code.
gonzoAuthorUnsubmitted
Not Done
Inline Actions

I think having comma-separated triplets is useful. Normally people want to migrate their color scheme from whatever format they have and it's either hex triplets or rgb triplets (in my experience). Supporting both formats may save some users' time.

gonzo: I think having comma-separated triplets is useful. Normally people want to migrate their color…
v = strtoul(ptr, &endptr, 10);
jillesUnsubmitted
Done
Inline Actions

Given that strtoul() depends on the '\0', passing along the length may not be very useful.

jilles: Given that `strtoul()` depends on the `'\0'`, passing along the length may not be very useful.
if (ptr == endptr)
jillesUnsubmitted
Done
Inline Actions

This while loop could be omitted, since strtoul() already skips leading whitespace.

jilles: This while loop could be omitted, since `strtoul()` already skips leading whitespace.
return (-1);
if (v > 255)
return (-1);
*r = v & 0xff;
ptr = endptr;
/* skip separator */
while (((*ptr == ',') || (*ptr == ' ')) && (ptr < (rgb + len)))
jillesUnsubmitted
Done
Inline Actions

Given that '\0' is not a space and not a comma, the condition on the length is redundant.

Also, it seems unusual to redundantly parenthesize arithmetic within a relational operator or relational operators within logical operators.

jilles: Given that `'\0'` is not a space and not a comma, the condition on the length is redundant.
ptr++;
if (ptr == (rgb + len))
return (-1);
v = strtoul(ptr, &endptr, 10);
if (ptr == endptr)
return (-1);
if (v > 255)
return (-1);
*g = v & 0xff;
ptr = endptr;
/* skip separator */
while (((*ptr == ',') || (*ptr == ' ')) && (ptr < (rgb + len)))
ptr++;
if (ptr == (rgb + len))
return (-1);
v = strtoul(ptr, &endptr, 10);
if (ptr == endptr)
return (-1);
if (v > 255)
return (-1);
*b = v & 0xff;
jillesUnsubmitted
Not Done
Inline Actions

Note that any characters after the blue value are ignored. Is this deliberate?

jilles: Note that any characters after the blue value are ignored. Is this deliberate?
gonzoAuthorUnsubmitted
Not Done
Inline Actions

Nope, it's oversight from my side. I added this check and some fixes to the parser. I didn't want to make "r,g.b" parser too strict and enforce comma-only format, hence the whitespaces check. But there is no reason to ignore garbage at the end of the string

gonzo: Nope, it's oversight from my side. I added this check and some fixes to the parser. I didn't…
return (0);
}
static void
vt_palette_init()
{
int i;
char rgb[32];
char tunable[32];
unsigned char r, g, b;
for (i = 0; i < NCOLORS; i++) {
snprintf(tunable, sizeof(tunable),
"kern.vt.color.%d.rgb", i);
if (TUNABLE_STR_FETCH(tunable, rgb, sizeof(rgb))) {
if (vt_parse_rgb_triplet(rgb, strlen(rgb), &r, &g, &b) == 0) {
/* convert to percentages */
color_def[i].r = r*100/255;
color_def[i].g = g*100/255;
color_def[i].b = b*100/255;
jillesUnsubmitted
Not Done
Inline Actions

Perhaps change color_def from 0..100 to 0..255, since it is an implementation detail of vt_termcolors.c. This makes the code slightly easier to understand and improves accuracy.

jilles: Perhaps change `color_def` from 0..100 to 0..255, since it is an implementation detail of…
gonzoAuthorUnsubmitted
Not Done
Inline Actions

It makes sense to have color_def in percentages because it may be applied to different bits-per-pixel values. On the other hand most common format from user perspective is 8-bit color component specification. IMO it's better to convert from user visible data to internal representation than make palette application logic less readable.

gonzo: It makes sense to have color_def in percentages because it may be applied to different bits-per…
jillesUnsubmitted
Not Done
Inline Actions

Provided there is no overflow, the maximum of 100 or 255 simply appears in #define CF below. This is just a bit of churn of the color_def table.

jilles: Provided there is no overflow, the maximum of 100 or 255 simply appears in `#define CF` below.
gonzoAuthorUnsubmitted
Not Done
Inline Actions

I think switching from 100 to 255 is a marginal improvement and not strictly required for the configurable palette feature. What IMO would be better from accuracy point of view is switching to 0..255 range in a palette and using number of bits per color component and logical shift instead of Nmax and dividing by non-power of two numbers. I believe this should be done as a separate patch

gonzo: I think switching from 100 to 255 is a marginal improvement and not strictly required for the…
gonzoAuthorUnsubmitted
Not Done
Inline Actions

Just realized "division to shift" part of the statement above is not entirely correct, it will have to be shift + some rounding logic.

gonzo: Just realized "division to shift" part of the statement above is not entirely correct, it will…
jillesUnsubmitted
Not Done
Inline Actions

I agree that this need not be fixed for this commit.

jilles: I agree that this need not be fixed for this commit.
}
}
}
}
int int
vt_generate_cons_palette(uint32_t *palette, int format, uint32_t rmax, vt_generate_cons_palette(uint32_t *palette, int format, uint32_t rmax,
int roffset, uint32_t gmax, int goffset, uint32_t bmax, int boffset) int roffset, uint32_t gmax, int goffset, uint32_t bmax, int boffset)
{ {
int i; int i;
vt_palette_init();
#define CF(_f, _i) ((_f ## max * color_def[(_i)]._f / 100) << _f ## offset) #define CF(_f, _i) ((_f ## max * color_def[(_i)]._f / 100) << _f ## offset)
for (i = 0; i < 16; i++) { for (i = 0; i < NCOLORS; i++) {
switch (format) { switch (format) {
case COLOR_FORMAT_VGA: case COLOR_FORMAT_VGA:
palette[i] = cons_to_vga_colors[i]; palette[i] = cons_to_vga_colors[i];
break; break;
case COLOR_FORMAT_RGB: case COLOR_FORMAT_RGB:
palette[i] = CF(r, i) | CF(g, i) | CF(b, i); palette[i] = CF(r, i) | CF(g, i) | CF(b, i);
break; break;
default: default:
return (ENODEV); return (ENODEV);
} }
} }
#undef CF #undef CF
return (0); return (0);
} }