* Removing assumption of unsigned long pixel values for colours
@ 2019-05-04 18:08 Alex Gramiak
2019-05-04 18:39 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Alex Gramiak @ 2019-05-04 18:08 UTC (permalink / raw)
To: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
I've attached a WIP patch (for illustrative purposes) creating a new
union, emacs_color, that removes the single type limitation for the
internal representation of colours. This approach is intended to be used
with a new backend that I'm working on which uses a quadruple of
normalized double values for each colour rather than a single pixel
value. Using a union would avoid having to frequently convert between
the two representations.
I'd like to know whether this approach would be accepted (provided that
it's completed and submitted with a backend that uses it) before working
out the kinks and using it for the new backend.
Some issues that still have yet to be addressed in the patch:
* prepare_face_for_display takes in an XGCValues, which assumes unsigned
long colour values. On non-X systems that create their own XGCValues,
they can just be redefined, but on X that won't work. That means a new
struct has to also be defined on X and then copied into an XGCValues
in x_create_gc.
* The colors member of struct image was changed to use the new union,
but XFreeColors expects an array of unsigned long pixels. It looks
like the colors array is only used when COLOR_TABLE_SUPPORT is
defined, so perhaps it could just used unsigned long unconditionally.
Question: In realize_gui_face, why is a defaulted underline colour set
to 0 when defaulted overline/strike-through colours are set to the
foreground colour? The comment above the underline case mentions that
it's the same as the foreground colour, so should it be explicitly set
there as well?
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: begone unsigned long --]
[-- Type: text/x-patch, Size: 50384 bytes --]
From 4651524207ee361ab143d280a75da9991d9ece4e Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Sat, 4 May 2019 11:45:30 -0600
Subject: [PATCH] Introduce emacs_color union
* src/dispextern.h (emacs_color): Define.
(emacs_RGB): Define as an internal representation used for
color_distance and face_color_gray_p.
(face, image): Change unsigned long colors to emacs_color unions.
* src/frame.h (frame): Change background and foreground to emacs_color
unions, and rename to background_color and foreground_color
respectively.
(FRAME_FOREGROUND_PIXEL, FRAME_BACKGROUND_PIXEL): Redefined to access
the foreground/background as pixel values.
(FRAME_FOREGROUND_COLOR, FRAME_BACKGROUND_COLOR): New macros to access
the foreground/background as union values.
* src/termhooks.h (color_equals, emacs_color_to_rgb)
(print_emacs_color): New terminal hooks.
* src/xdisp.c (extend_face_to_end_of_line): Use color_equals hook.
* src/xfaces.c (load_color2): Remove helper procedure.
(load_color): Take over job of load_color2. Take the address of the
color to be loaded as an argument to set rather than returning a pixel
value.
---
src/dispextern.h | 43 +++++---
src/dispnew.c | 4 +-
src/frame.h | 12 ++-
src/image.c | 62 ++++++-----
src/term.c | 12 +--
src/termhooks.h | 16 ++-
src/xdisp.c | 9 +-
src/xfaces.c | 273 +++++++++++++++++++++++++++++------------------
src/xfns.c | 25 ++++-
src/xterm.c | 74 +++++++------
src/xterm.h | 3 +-
11 files changed, 326 insertions(+), 207 deletions(-)
diff --git a/src/dispextern.h b/src/dispextern.h
index bb981f83fc..07b87aeecf 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1574,6 +1574,21 @@ enum face_underline_type
FACE_UNDER_WAVE
};
+union emacs_color
+{
+ unsigned long pixel;
+#ifdef USE_PURE_GTK
+ GdkRGBA gdk_rgba;
+#endif
+};
+
+struct emacs_RGB
+{
+ unsigned short red;
+ unsigned short green;
+ unsigned short blue;
+};
+
/* Structure describing a realized face.
For each Lisp face, 0..N realized faces can exist for different
@@ -1605,19 +1620,19 @@ struct face
/* Pixel value of foreground color for X frames. Color index
for tty frames. */
- unsigned long foreground;
+ union emacs_color foreground;
/* Pixel value or color index of background color. */
- unsigned long background;
+ union emacs_color background;
/* Pixel value or color index of underline color. */
- unsigned long underline_color;
+ union emacs_color underline_color;
/* Pixel value or color index of overlined, strike-through, or box
color. */
- unsigned long overline_color;
- unsigned long strike_through_color;
- unsigned long box_color;
+ union emacs_color overline_color;
+ union emacs_color strike_through_color;
+ union emacs_color box_color;
struct font *font;
@@ -2977,7 +2992,7 @@ struct image
#endif
/* Colors allocated for this image, if any. Allocated via xmalloc. */
- unsigned long *colors;
+ union emacs_color *colors;
int ncolors;
/* A single `background color' for this image, for the use of anyone that
@@ -2985,11 +3000,11 @@ struct image
is true. This should generally be accessed by calling the accessor
macro `IMAGE_BACKGROUND', which will heuristically calculate a value
if necessary. */
- unsigned long background;
+ union emacs_color background;
/* Foreground and background colors of the frame on which the image
is created. */
- unsigned long frame_foreground, frame_background;
+ union emacs_color frame_foreground, frame_background;
/* True if this image has a `transparent' background -- that is, is
uses an image mask. The accessor macro for this is
@@ -3379,7 +3394,7 @@ void prepare_image_for_display (struct frame *, struct image *);
ptrdiff_t lookup_image (struct frame *, Lisp_Object);
#if defined (HAVE_X_WINDOWS) || defined (HAVE_NS)
-#define RGB_PIXEL_COLOR unsigned long
+#define RGB_PIXEL_COLOR union emacs_color
#endif
#ifdef HAVE_NTGUI
@@ -3413,13 +3428,13 @@ void x_free_colors (struct frame *, unsigned long *, int);
void update_face_from_frame_parameter (struct frame *, Lisp_Object,
Lisp_Object);
-extern bool tty_defined_color (struct frame *f, const char *, XColor *, bool,
- bool);
+extern bool tty_defined_color (struct frame *, const char *,
+ union emacs_color *, bool, bool);
Lisp_Object tty_color_name (struct frame *, int);
void clear_face_cache (bool);
-unsigned long load_color (struct frame *, struct face *, Lisp_Object,
- enum lface_attribute_index);
+void load_color (struct frame *, struct face *, Lisp_Object,
+ enum lface_attribute_index, union emacs_color *);
char *choose_face_font (struct frame *, Lisp_Object *, Lisp_Object,
int *);
#ifdef HAVE_WINDOW_SYSTEM
diff --git a/src/dispnew.c b/src/dispnew.c
index 52a7b6d6ee..430aad97da 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -4879,9 +4879,11 @@ update_frame_line (struct frame *f, int vpos, bool updating_menu_p)
struct glyph_row *desired_row = MATRIX_ROW (desired_matrix, vpos);
bool must_write_whole_line_p;
bool write_spaces_p = FRAME_MUST_WRITE_SPACES (f);
- bool colored_spaces_p = (FACE_FROM_ID (f, DEFAULT_FACE_ID)->background
+ bool colored_spaces_p = (FACE_FROM_ID (f, DEFAULT_FACE_ID)->background.pixel
!= FACE_TTY_DEFAULT_BG_COLOR);
+ eassert (FRAME_TERMCAP_P (f) || FRAME_MSDOS_P (f));
+
if (colored_spaces_p)
write_spaces_p = 1;
diff --git a/src/frame.h b/src/frame.h
index b8aed823af..17388494fb 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -598,9 +598,9 @@ struct frame
/* Additional space to put between text lines on this frame. */
int extra_line_spacing;
- /* All display backends seem to need these two pixel values. */
- unsigned long background_pixel;
- unsigned long foreground_pixel;
+ /* All display backends need default background and foreground values. */
+ union emacs_color background_color;
+ union emacs_color foreground_color;
#ifdef NS_IMPL_COCOA
/* NSAppearance theme used on this frame. */
@@ -1168,8 +1168,10 @@ default_pixels_per_inch_y (void)
#define FRAME_CURSOR_WIDTH(f) ((f)->cursor_width)
#define FRAME_BLINK_OFF_CURSOR_WIDTH(f) ((f)->blink_off_cursor_width)
-#define FRAME_FOREGROUND_PIXEL(f) ((f)->foreground_pixel)
-#define FRAME_BACKGROUND_PIXEL(f) ((f)->background_pixel)
+#define FRAME_FOREGROUND_COLOR(f) ((f)->foreground_color)
+#define FRAME_BACKGROUND_COLOR(f) ((f)->background_color)
+#define FRAME_FOREGROUND_PIXEL(f) ((f)->foreground_color.pixel)
+#define FRAME_BACKGROUND_PIXEL(f) ((f)->background_color.pixel)
/* Return a pointer to the face cache of frame F. */
diff --git a/src/image.c b/src/image.c
index 3d724a773b..0dd4df6dd7 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1395,12 +1395,11 @@ image_clear_image (struct frame *f, struct image *img)
IMG->colors, so that it can be freed again. Value is the pixel
color. */
-static unsigned long
+static union emacs_color
image_alloc_image_color (struct frame *f, struct image *img,
- Lisp_Object color_name, unsigned long dflt)
+ Lisp_Object color_name, union emacs_color dflt)
{
- XColor color;
- unsigned long result;
+ union emacs_color color;
eassert (STRINGP (color_name));
@@ -1416,14 +1415,13 @@ image_alloc_image_color (struct frame *f, struct image *img,
reallocating the color vector to the needed size, here. */
ptrdiff_t ncolors = img->ncolors + 1;
img->colors = xrealloc (img->colors, ncolors * sizeof *img->colors);
- img->colors[ncolors - 1] = color.pixel;
+ img->colors[ncolors - 1] = color;
img->ncolors = ncolors;
- result = color.pixel;
}
else
- result = dflt;
+ color = dflt;
- return result;
+ return color;
}
@@ -1982,8 +1980,8 @@ lookup_image (struct frame *f, Lisp_Object spec)
{
img->background
= image_alloc_image_color (f, img, bg,
- FRAME_BACKGROUND_PIXEL (f));
- img->background_valid = 1;
+ FRAME_BACKGROUND_COLOR (f));
+ img->background_valid = true;
}
}
@@ -3161,9 +3159,9 @@ xbm_load_image (struct frame *f, struct image *img, char *contents, char *end)
&data, 0);
if (rc)
{
- unsigned long foreground = FRAME_FOREGROUND_PIXEL (f);
- unsigned long background = FRAME_BACKGROUND_PIXEL (f);
- bool non_default_colors = 0;
+ union emacs_color foreground = FRAME_FOREGROUND_COLOR (f);
+ union emacs_color background = FRAME_BACKGROUND_COLOR (f);
+ bool non_default_colors = false;
Lisp_Object value;
eassert (img->width > 0 && img->height > 0);
@@ -3173,15 +3171,15 @@ xbm_load_image (struct frame *f, struct image *img, char *contents, char *end)
if (!NILP (value))
{
foreground = image_alloc_image_color (f, img, value, foreground);
- non_default_colors = 1;
+ non_default_colors = true;
}
value = image_spec_value (img->spec, QCbackground, NULL);
if (!NILP (value))
{
background = image_alloc_image_color (f, img, value, background);
img->background = background;
- img->background_valid = 1;
- non_default_colors = 1;
+ img->background_valid = true;
+ non_default_colors = true;
}
Create_Pixmap_From_Bitmap_Data (f, img, data,
@@ -3195,7 +3193,7 @@ xbm_load_image (struct frame *f, struct image *img, char *contents, char *end)
image_error ("Unable to create X pixmap for `%s'", img->spec);
}
else
- success_p = 1;
+ success_p = true;
}
else
image_error ("Error loading XBM image `%s'", img->spec);
@@ -3255,12 +3253,12 @@ xbm_load (struct frame *f, struct image *img)
{
struct image_keyword fmt[XBM_LAST];
Lisp_Object data;
- unsigned long foreground = FRAME_FOREGROUND_PIXEL (f);
- unsigned long background = FRAME_BACKGROUND_PIXEL (f);
- bool non_default_colors = 0;
+ union emacs_color foreground = FRAME_FOREGROUND_COLOR (f);
+ union emacs_color background = FRAME_BACKGROUND_COLOR (f);
+ bool non_default_colors = false;
char *bits;
bool parsed_p;
- bool in_memory_file_p = 0;
+ bool in_memory_file_p = false;
/* See if data looks like an in-memory XBM file. */
data = image_spec_value (img->spec, QCdata, NULL);
@@ -3280,7 +3278,7 @@ xbm_load (struct frame *f, struct image *img)
if (!check_image_size (f, img->width, img->height))
{
image_size_error ();
- return 0;
+ return false;
}
}
@@ -3292,7 +3290,7 @@ xbm_load (struct frame *f, struct image *img)
img,
fmt[XBM_FOREGROUND].value,
foreground);
- non_default_colors = 1;
+ non_default_colors = true;
}
if (fmt[XBM_BACKGROUND].count
@@ -3302,7 +3300,7 @@ xbm_load (struct frame *f, struct image *img)
img,
fmt[XBM_BACKGROUND].value,
background);
- non_default_colors = 1;
+ non_default_colors = true;
}
if (in_memory_file_p)
@@ -3356,7 +3354,7 @@ xbm_load (struct frame *f, struct image *img)
img->pixmap = NO_PIXMAP;
if (img->pixmap)
- success_p = 1;
+ success_p = true;
else
{
image_error ("Unable to create pixmap for XBM image `%s'",
@@ -5652,8 +5650,8 @@ pbm_load (struct frame *f, struct image *img)
unsigned char c = 0;
int g;
struct image_keyword fmt[PBM_LAST];
- unsigned long fg = FRAME_FOREGROUND_PIXEL (f);
- unsigned long bg = FRAME_BACKGROUND_PIXEL (f);
+ union emacs_color fg = FRAME_FOREGROUND_COLOR (f);
+ union emacs_color bg = FRAME_BACKGROUND_COLOR (f);
#ifdef USE_CAIRO
XColor xfg, xbg;
int fga32, bga32;
@@ -5672,7 +5670,7 @@ pbm_load (struct frame *f, struct image *img)
false,
false))
{
- xfg.pixel = fg;
+ xfg.pixel = fg.pixel;
x_query_colors (f, &xfg, 1);
}
fga32 = xcolor_to_argb32 (xfg);
@@ -5685,7 +5683,7 @@ pbm_load (struct frame *f, struct image *img)
false,
false))
{
- xbg.pixel = bg;
+ xbg.pixel = bg.pixel;
x_query_colors (f, &xbg, 1);
}
bga32 = xcolor_to_argb32 (xbg);
@@ -5698,7 +5696,7 @@ pbm_load (struct frame *f, struct image *img)
{
bg = image_alloc_image_color (f, img, fmt[PBM_BACKGROUND].value, bg);
img->background = bg;
- img->background_valid = 1;
+ img->background_valid = true;
}
#endif
@@ -8087,10 +8085,10 @@ gif_load (struct frame *f, struct image *img)
init_color_table ();
#ifndef USE_CAIRO
- unsigned long bgcolor UNINIT;
+ union emacs_color bgcolor = { .pixel = 0 };
if (STRINGP (specified_bg))
bgcolor = image_alloc_image_color (f, img, specified_bg,
- FRAME_BACKGROUND_PIXEL (f));
+ FRAME_BACKGROUND_COLOR (f));
#endif
for (j = 0; j <= idx; ++j)
diff --git a/src/term.c b/src/term.c
index 6a8fc2ee93..cf7279cfff 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1906,8 +1906,8 @@ static void
turn_on_face (struct frame *f, int face_id)
{
struct face *face = FACE_FROM_ID (f, face_id);
- unsigned long fg = face->foreground;
- unsigned long bg = face->background;
+ unsigned long fg = face->foreground.pixel;
+ unsigned long bg = face->background.pixel;
struct tty_display_info *tty = FRAME_TTY (f);
/* Use reverse video if the face specifies that.
@@ -1993,10 +1993,10 @@ turn_off_face (struct frame *f, int face_id)
/* Switch back to default colors. */
if (tty->TN_max_colors > 0
- && ((face->foreground != FACE_TTY_DEFAULT_COLOR
- && face->foreground != FACE_TTY_DEFAULT_FG_COLOR)
- || (face->background != FACE_TTY_DEFAULT_COLOR
- && face->background != FACE_TTY_DEFAULT_BG_COLOR)))
+ && ((face->foreground.pixel != FACE_TTY_DEFAULT_COLOR
+ && face->foreground.pixel != FACE_TTY_DEFAULT_FG_COLOR)
+ || (face->background.pixel != FACE_TTY_DEFAULT_COLOR
+ && face->background.pixel != FACE_TTY_DEFAULT_BG_COLOR)))
OUTPUT1_IF (tty, tty->TS_orig_pair);
}
diff --git a/src/termhooks.h b/src/termhooks.h
index 54f09e0303..7431883e7a 100644
--- a/src/termhooks.h
+++ b/src/termhooks.h
@@ -489,6 +489,14 @@ struct terminal
void (*update_end_hook) (struct frame *);
void (*set_terminal_window_hook) (struct frame *, int);
+ /* Decide if COLOR1 and COLOR2 are equal. */
+ bool (*color_equals) (const union emacs_color *color1,
+ const union emacs_color *color2);
+
+ /* Translate emacs_color COLOR to emacs_RGB RGB. */
+ void (*emacs_color_to_rgb) (const union emacs_color *color,
+ struct emacs_RGB *rgb);
+
/* Decide if color named COLOR_NAME is valid for the display
associated with the frame F; if so, return the RGB values in
COLOR_DEF. If ALLOC (and MAKEINDEX for NS), allocate a new
@@ -496,10 +504,16 @@ struct terminal
If MAKEINDEX (on NS), set COLOR_DEF pixel to ARGB. */
bool (*defined_color_hook) (struct frame *f, const char *color_name,
- XColor *color_def,
+ union emacs_color *color_def,
bool alloc,
bool makeIndex);
+#ifdef GLYPH_DEBUG
+ void (*print_emacs_color) (const char *attribute,
+ const union emacs_color *color,
+ const char *name);
+#endif
+
/* Multi-frame and mouse support hooks. */
/* Graphical window systems are expected to define all of the
diff --git a/src/xdisp.c b/src/xdisp.c
index 3bdb8ea1b0..33685de109 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -20318,7 +20318,8 @@ extend_face_to_end_of_line (struct it *it)
if (FRAME_WINDOW_P (f)
&& MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
&& face->box == FACE_NO_BOX
- && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
+ && (FRAME_TERMINAL (f)->color_equals
+ (&face->background, &FRAME_BACKGROUND_COLOR (f)))
#ifdef HAVE_WINDOW_SYSTEM
&& !face->stipple
#endif
@@ -20463,7 +20464,8 @@ extend_face_to_end_of_line (struct it *it)
&& (it->glyph_row->used[LEFT_MARGIN_AREA]
< WINDOW_LEFT_MARGIN_WIDTH (it->w))
&& !it->glyph_row->mode_line_p
- && FACE_COLOR_TO_PIXEL (face->background, f) != FRAME_BACKGROUND_PIXEL (f))
+ && (!FRAME_TERMINAL (f)->color_equals
+ (&face->background, &FRAME_BACKGROUND_COLOR (f))))
{
struct glyph *g = it->glyph_row->glyphs[LEFT_MARGIN_AREA];
struct glyph *e = g + it->glyph_row->used[LEFT_MARGIN_AREA];
@@ -20504,7 +20506,8 @@ extend_face_to_end_of_line (struct it *it)
&& (it->glyph_row->used[RIGHT_MARGIN_AREA]
< WINDOW_RIGHT_MARGIN_WIDTH (it->w))
&& !it->glyph_row->mode_line_p
- && FACE_COLOR_TO_PIXEL (face->background, f) != FRAME_BACKGROUND_PIXEL (f))
+ && (!FRAME_TERMINAL (f)->color_equals
+ (&face->background, &FRAME_BACKGROUND_COLOR (f))))
{
struct glyph *g = it->glyph_row->glyphs[RIGHT_MARGIN_AREA];
struct glyph *e = g + it->glyph_row->used[RIGHT_MARGIN_AREA];
diff --git a/src/xfaces.c b/src/xfaces.c
index 5c2414b7b0..d124df0b80 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -897,30 +897,33 @@ tty_lookup_color (struct frame *f, Lisp_Object color, XColor *tty_color,
bool
tty_defined_color (struct frame *f, const char *color_name,
- XColor *color_def, bool alloc, bool _makeIndex)
+ union emacs_color *color_def, bool alloc, bool _makeIndex)
{
bool status = true;
+ XColor xcol;
/* Defaults. */
- color_def->pixel = FACE_TTY_DEFAULT_COLOR;
- color_def->red = 0;
- color_def->blue = 0;
- color_def->green = 0;
+ xcol.pixel = FACE_TTY_DEFAULT_COLOR;
+ xcol.red = 0;
+ xcol.blue = 0;
+ xcol.green = 0;
if (*color_name)
- status = tty_lookup_color (f, build_string (color_name), color_def, NULL);
+ status = tty_lookup_color (f, build_string (color_name), &xcol, NULL);
- if (color_def->pixel == FACE_TTY_DEFAULT_COLOR && *color_name)
+ if (xcol.pixel == FACE_TTY_DEFAULT_COLOR && *color_name)
{
if (strcmp (color_name, "unspecified-fg") == 0)
- color_def->pixel = FACE_TTY_DEFAULT_FG_COLOR;
+ xcol.pixel = FACE_TTY_DEFAULT_FG_COLOR;
else if (strcmp (color_name, "unspecified-bg") == 0)
- color_def->pixel = FACE_TTY_DEFAULT_BG_COLOR;
+ xcol.pixel = FACE_TTY_DEFAULT_BG_COLOR;
}
- if (color_def->pixel != FACE_TTY_DEFAULT_COLOR)
+ if (xcol.pixel != FACE_TTY_DEFAULT_COLOR)
status = true;
+ color_def->pixel = xcol.pixel;
+
return status;
}
@@ -965,20 +968,24 @@ tty_color_name (struct frame *f, int idx)
static bool
face_color_gray_p (struct frame *f, const char *color_name)
{
- XColor color;
+ union emacs_color color;
bool gray_p;
if (FRAME_TERMINAL (f)->defined_color_hook
(f, color_name, &color, false, true))
- gray_p = (/* Any color sufficiently close to black counts as gray. */
- (color.red < 5000 && color.green < 5000 && color.blue < 5000)
- ||
- ((eabs (color.red - color.green)
- < max (color.red, color.green) / 20)
- && (eabs (color.green - color.blue)
- < max (color.green, color.blue) / 20)
- && (eabs (color.blue - color.red)
- < max (color.blue, color.red) / 20)));
+ {
+ struct emacs_RGB rgb;
+ FRAME_TERMINAL (f)->emacs_color_to_rgb (&color, &rgb);
+
+ gray_p = (/* Any color sufficiently close to black counts as gray. */
+ (rgb.red < 5000 && rgb.green < 5000 && rgb.blue < 5000)
+ || ((eabs (rgb.red - rgb.green)
+ < max (rgb.red, rgb.green) / 20)
+ && (eabs (rgb.green - rgb.blue)
+ < max (rgb.green, rgb.blue) / 20)
+ && (eabs (rgb.blue - rgb.red)
+ < max (rgb.blue, rgb.red) / 20)));
+ }
else
gray_p = false;
@@ -994,7 +1001,7 @@ face_color_supported_p (struct frame *f, const char *color_name,
bool background_p)
{
Lisp_Object frame;
- XColor not_used;
+ union emacs_color not_used;
XSETFRAME (frame, f);
return
@@ -1040,10 +1047,18 @@ COLOR must be a valid color name. */)
? Qt : Qnil);
}
+/* Load color with name NAME for use by face FACE on frame F.
+ TARGET_INDEX must be one of LFACE_FOREGROUND_INDEX,
+ LFACE_BACKGROUND_INDEX, LFACE_UNDERLINE_INDEX, LFACE_OVERLINE_INDEX,
+ LFACE_STRIKE_THROUGH_INDEX, or LFACE_BOX_INDEX. Value is the
+ pixel color. If color cannot be loaded, display a message, and
+ return the foreground, background or underline color of F in COLOR,
+ but record that fact in flags of the face so that we don't try to
+ free these colors. */
-static unsigned long
-load_color2 (struct frame *f, struct face *face, Lisp_Object name,
- enum lface_attribute_index target_index, XColor *color)
+void
+load_color (struct frame *f, struct face *face, Lisp_Object name,
+ enum lface_attribute_index target_index, union emacs_color *color)
{
eassert (STRINGP (name));
eassert (target_index == LFACE_FOREGROUND_INDEX
@@ -1053,8 +1068,8 @@ load_color2 (struct frame *f, struct face *face, Lisp_Object name,
|| target_index == LFACE_STRIKE_THROUGH_INDEX
|| target_index == LFACE_BOX_INDEX);
- /* if the color map is full, defined_color_hook will return a best match
- to the values in an existing cell. */
+ /* On X, if the color map is full, defined_color_hook will return a
+ best match to the values in an existing cell. */
if (!FRAME_TERMINAL (f)->defined_color_hook
(f, SSDATA (name), color, true, true))
{
@@ -1064,32 +1079,32 @@ load_color2 (struct frame *f, struct face *face, Lisp_Object name,
{
case LFACE_FOREGROUND_INDEX:
face->foreground_defaulted_p = true;
- color->pixel = FRAME_FOREGROUND_PIXEL (f);
+ *color = FRAME_FOREGROUND_COLOR (f);
break;
case LFACE_BACKGROUND_INDEX:
face->background_defaulted_p = true;
- color->pixel = FRAME_BACKGROUND_PIXEL (f);
+ *color = FRAME_BACKGROUND_COLOR (f);
break;
case LFACE_UNDERLINE_INDEX:
face->underline_defaulted_p = true;
- color->pixel = FRAME_FOREGROUND_PIXEL (f);
+ *color = FRAME_FOREGROUND_COLOR (f);
break;
case LFACE_OVERLINE_INDEX:
face->overline_color_defaulted_p = true;
- color->pixel = FRAME_FOREGROUND_PIXEL (f);
+ *color = FRAME_FOREGROUND_COLOR (f);
break;
case LFACE_STRIKE_THROUGH_INDEX:
face->strike_through_color_defaulted_p = true;
- color->pixel = FRAME_FOREGROUND_PIXEL (f);
+ *color = FRAME_FOREGROUND_COLOR (f);
break;
case LFACE_BOX_INDEX:
face->box_color_defaulted_p = true;
- color->pixel = FRAME_FOREGROUND_PIXEL (f);
+ *color = FRAME_FOREGROUND_COLOR (f);
break;
default:
@@ -1100,28 +1115,8 @@ load_color2 (struct frame *f, struct face *face, Lisp_Object name,
else
++ncolors_allocated;
#endif
-
- return color->pixel;
}
-/* Load color with name NAME for use by face FACE on frame F.
- TARGET_INDEX must be one of LFACE_FOREGROUND_INDEX,
- LFACE_BACKGROUND_INDEX, LFACE_UNDERLINE_INDEX, LFACE_OVERLINE_INDEX,
- LFACE_STRIKE_THROUGH_INDEX, or LFACE_BOX_INDEX. Value is the
- pixel color. If color cannot be loaded, display a message, and
- return the foreground, background or underline color of F, but
- record that fact in flags of the face so that we don't try to free
- these colors. */
-
-unsigned long
-load_color (struct frame *f, struct face *face, Lisp_Object name,
- enum lface_attribute_index target_index)
-{
- XColor color;
- return load_color2 (f, face, name, target_index, &color);
-}
-
-
#ifdef HAVE_WINDOW_SYSTEM
/* Load colors for face FACE which is used on frame F. Colors are
@@ -1134,7 +1129,6 @@ load_face_colors (struct frame *f, struct face *face,
Lisp_Object attrs[LFACE_VECTOR_SIZE])
{
Lisp_Object fg, bg, dfg;
- XColor xfg, xbg;
bg = attrs[LFACE_BACKGROUND_INDEX];
fg = attrs[LFACE_FOREGROUND_INDEX];
@@ -1159,17 +1153,36 @@ load_face_colors (struct frame *f, struct face *face,
face->stipple = load_pixmap (f, Vface_default_stipple);
}
- face->background = load_color2 (f, face, bg, LFACE_BACKGROUND_INDEX, &xbg);
- face->foreground = load_color2 (f, face, fg, LFACE_FOREGROUND_INDEX, &xfg);
+ load_color (f, face, bg, LFACE_BACKGROUND_INDEX, &face->background);
+ load_color (f, face, fg, LFACE_FOREGROUND_INDEX, &face->foreground);
dfg = attrs[LFACE_DISTANT_FOREGROUND_INDEX];
- if (!NILP (dfg) && !UNSPECIFIEDP (dfg)
- && color_distance (&xbg, &xfg) < face_near_same_color_threshold)
+ if (!NILP (dfg) && !UNSPECIFIEDP (dfg))
+
{
- if (EQ (attrs[LFACE_INVERSE_INDEX], Qt))
- face->background = load_color (f, face, dfg, LFACE_BACKGROUND_INDEX);
- else
- face->foreground = load_color (f, face, dfg, LFACE_FOREGROUND_INDEX);
+ struct emacs_RGB rgb_fore;
+ struct emacs_RGB rgb_back;
+ XColor xcol_fore;
+ XColor xcol_back;
+ FRAME_TERMINAL (f)->emacs_color_to_rgb (&face->foreground, &rgb_fore);
+ FRAME_TERMINAL (f)->emacs_color_to_rgb (&face->background, &rgb_back);
+
+ xcol_fore.red = rgb_fore.red;
+ xcol_fore.green = rgb_fore.green;
+ xcol_fore.blue = rgb_fore.blue;
+
+ xcol_back.red = rgb_back.red;
+ xcol_back.green = rgb_back.green;
+ xcol_back.blue = rgb_back.blue;
+
+ if (color_distance (&xcol_fore, &xcol_back)
+ < face_near_same_color_threshold)
+ {
+ if (EQ (attrs[LFACE_INVERSE_INDEX], Qt))
+ load_color (f, face, dfg, LFACE_BACKGROUND_INDEX, &face->background);
+ else
+ load_color (f, face, dfg, LFACE_FOREGROUND_INDEX, &face->foreground);
+ }
}
}
@@ -1202,41 +1215,41 @@ free_face_colors (struct frame *f, struct face *face)
if (!face->foreground_defaulted_p)
{
- x_free_colors (f, &face->foreground, 1);
+ x_free_colors (f, &face->foreground.pixel, 1);
IF_DEBUG (--ncolors_allocated);
}
if (!face->background_defaulted_p)
{
- x_free_colors (f, &face->background, 1);
+ x_free_colors (f, &face->background.pixel, 1);
IF_DEBUG (--ncolors_allocated);
}
if (face->underline_p
&& !face->underline_defaulted_p)
{
- x_free_colors (f, &face->underline_color, 1);
+ x_free_colors (f, &face->underline_color.pixel, 1);
IF_DEBUG (--ncolors_allocated);
}
if (face->overline_p
&& !face->overline_color_defaulted_p)
{
- x_free_colors (f, &face->overline_color, 1);
+ x_free_colors (f, &face->overline_color.pixel, 1);
IF_DEBUG (--ncolors_allocated);
}
if (face->strike_through_p
&& !face->strike_through_color_defaulted_p)
{
- x_free_colors (f, &face->strike_through_color, 1);
+ x_free_colors (f, &face->strike_through_color.pixel, 1);
IF_DEBUG (--ncolors_allocated);
}
if (face->box != FACE_NO_BOX
&& !face->box_color_defaulted_p)
{
- x_free_colors (f, &face->box_color, 1);
+ x_free_colors (f, &face->box_color.pixel, 1);
IF_DEBUG (--ncolors_allocated);
}
@@ -4206,12 +4219,13 @@ two lists of the form (RED GREEN BLUE) aforementioned. */)
{
struct frame *f = decode_live_frame (frame);
XColor cdef1, cdef2;
+ union emacs_color ecol1, ecol2;
if (!(CONSP (color1) && parse_rgb_list (color1, &cdef1))
&& !(STRINGP (color1)
&& FRAME_TERMINAL (f)->defined_color_hook (f,
SSDATA (color1),
- &cdef1,
+ &ecol1,
false,
true)))
signal_error ("Invalid color", color1);
@@ -4219,11 +4233,29 @@ two lists of the form (RED GREEN BLUE) aforementioned. */)
&& !(STRINGP (color2)
&& FRAME_TERMINAL (f)->defined_color_hook (f,
SSDATA (color2),
- &cdef2,
+ &ecol2,
false,
true)))
signal_error ("Invalid color", color2);
+ if (STRINGP (color1))
+ {
+ struct emacs_RGB rgb;
+ FRAME_TERMINAL (f)->emacs_color_to_rgb (&ecol1, &rgb);
+ cdef1.red = rgb.red;
+ cdef1.green = rgb.green;
+ cdef1.blue = rgb.blue;
+ }
+
+ if (STRINGP (color2))
+ {
+ struct emacs_RGB rgb;
+ FRAME_TERMINAL (f)->emacs_color_to_rgb (&ecol2, &rgb);
+ cdef2.red = rgb.red;
+ cdef2.green = rgb.green;
+ cdef2.blue = rgb.blue;
+ }
+
if (NILP (metric))
return make_fixnum (color_distance (&cdef1, &cdef2));
else
@@ -5633,8 +5665,12 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
{
/* A simple box of line width 1 drawn in color given by
the string. */
- face->box_color = load_color (f, face, attrs[LFACE_BOX_INDEX],
- LFACE_BOX_INDEX);
+ load_color (f,
+ face,
+ attrs[LFACE_BOX_INDEX],
+ LFACE_BOX_INDEX,
+ &face->box_color);
+
face->box = FACE_SIMPLE_BOX;
face->box_line_width = 1;
}
@@ -5678,8 +5714,7 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
{
if (STRINGP (value))
{
- face->box_color = load_color (f, face, value,
- LFACE_BOX_INDEX);
+ load_color (f, face, value, LFACE_BOX_INDEX, &face->box_color);
face->use_box_color_for_shadows_p = true;
}
}
@@ -5702,7 +5737,7 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
face->underline_p = true;
face->underline_type = FACE_UNDER_LINE;
face->underline_defaulted_p = true;
- face->underline_color = 0;
+ face->underline_color.pixel = 0;
}
else if (STRINGP (underline))
{
@@ -5710,22 +5745,24 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
face->underline_p = true;
face->underline_type = FACE_UNDER_LINE;
face->underline_defaulted_p = false;
- face->underline_color
- = load_color (f, face, underline,
- LFACE_UNDERLINE_INDEX);
+ load_color (f,
+ face,
+ underline,
+ LFACE_UNDERLINE_INDEX,
+ &face->underline_color);
}
else if (NILP (underline))
{
face->underline_p = false;
face->underline_defaulted_p = false;
- face->underline_color = 0;
+ face->underline_color.pixel = 0;
}
else if (CONSP (underline))
{
/* `(:color COLOR :style STYLE)'.
STYLE being one of `line' or `wave'. */
face->underline_p = true;
- face->underline_color = 0;
+ face->underline_color.pixel = 0;
face->underline_defaulted_p = true;
face->underline_type = FACE_UNDER_LINE;
@@ -5748,13 +5785,16 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
if (EQ (value, Qforeground_color))
{
face->underline_defaulted_p = true;
- face->underline_color = 0;
+ face->underline_color.pixel = 0;
}
else if (STRINGP (value))
{
face->underline_defaulted_p = false;
- face->underline_color = load_color (f, face, value,
- LFACE_UNDERLINE_INDEX);
+ load_color (f,
+ face,
+ value,
+ LFACE_UNDERLINE_INDEX,
+ &face->underline_color);
}
}
else if (EQ (keyword, QCstyle))
@@ -5770,9 +5810,11 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
overline = attrs[LFACE_OVERLINE_INDEX];
if (STRINGP (overline))
{
- face->overline_color
- = load_color (f, face, attrs[LFACE_OVERLINE_INDEX],
- LFACE_OVERLINE_INDEX);
+ load_color (f,
+ face,
+ attrs[LFACE_OVERLINE_INDEX],
+ LFACE_OVERLINE_INDEX,
+ &face->overline_color);
face->overline_p = true;
}
else if (EQ (overline, Qt))
@@ -5785,9 +5827,11 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
strike_through = attrs[LFACE_STRIKE_THROUGH_INDEX];
if (STRINGP (strike_through))
{
- face->strike_through_color
- = load_color (f, face, attrs[LFACE_STRIKE_THROUGH_INDEX],
- LFACE_STRIKE_THROUGH_INDEX);
+ load_color (f,
+ face,
+ attrs[LFACE_STRIKE_THROUGH_INDEX],
+ LFACE_STRIKE_THROUGH_INDEX,
+ &face->strike_through_color);
face->strike_through_p = true;
}
else if (EQ (strike_through, Qt))
@@ -5843,7 +5887,13 @@ map_tty_color (struct frame *f, struct face *face,
if (pixel == default_pixel && STRINGP (color))
{
- pixel = load_color (f, face, color, idx);
+ union emacs_color col;
+
+ /* Frame must be a termcap frame. */
+ eassert (FRAME_TERMCAP_P (f) || FRAME_MSDOS_P (f));
+
+ load_color (f, face, color, idx, &col);
+ pixel = col.pixel;
#ifdef MSDOS
/* If the foreground of the default face is the default color,
@@ -5874,9 +5924,9 @@ map_tty_color (struct frame *f, struct face *face,
}
if (foreground_p)
- face->foreground = pixel;
+ face->foreground.pixel = pixel;
else
- face->background = pixel;
+ face->background.pixel = pixel;
}
@@ -5923,15 +5973,15 @@ realize_tty_face (struct face_cache *cache,
frame-creation function calls x-handle-reverse-video. */
if (face->tty_reverse_p && !face_colors_defaulted)
{
- unsigned long tem = face->foreground;
- face->foreground = face->background;
- face->background = tem;
+ unsigned long tem = face->foreground.pixel;
+ face->foreground.pixel = face->background.pixel;
+ face->background.pixel = tem;
}
if (tty_suppress_bold_inverse_default_colors_p
&& face->tty_bold_p
- && face->background == FACE_TTY_DEFAULT_FG_COLOR
- && face->foreground == FACE_TTY_DEFAULT_BG_COLOR)
+ && face->background.pixel == FACE_TTY_DEFAULT_FG_COLOR
+ && face->foreground.pixel == FACE_TTY_DEFAULT_BG_COLOR)
face->tty_bold_p = false;
return face;
@@ -6397,18 +6447,33 @@ where R,G,B are numbers between 0 and 255 and name is an arbitrary string. */)
/* Print the contents of the realized face FACE to stderr. */
static void
-dump_realized_face (struct face *face)
+dump_realized_face (struct face *face, struct frame *f)
{
fprintf (stderr, "ID: %d\n", face->id);
#ifdef HAVE_X_WINDOWS
fprintf (stderr, "gc: %p\n", face->gc);
#endif
- fprintf (stderr, "foreground: 0x%lx (%s)\n",
- face->foreground,
- SDATA (face->lface[LFACE_FOREGROUND_INDEX]));
- fprintf (stderr, "background: 0x%lx (%s)\n",
- face->background,
- SDATA (face->lface[LFACE_BACKGROUND_INDEX]));
+ if (FRAME_TERMINAL (f)->print_emacs_color)
+ {
+ FRAME_TERMINAL (f)->
+ print_emacs_color ("foreground",
+ &face->foreground,
+ SDATA (face->lface[LFACE_FOREGROUND_INDEX]));
+ FRAME_TERMINAL (f)->
+ print_emacs_color ("background",
+ &face->background,
+ SDATA (face->lface[LFACE_BACKGROUND_INDEX]));
+ }
+ else
+ {
+ fprintf (stderr, "foreground: 0x%lx (%s)\n",
+ face->foreground.pixel,
+ SDATA (face->lface[LFACE_FOREGROUND_INDEX]));
+ fprintf (stderr, "background: 0x%lx (%s)\n",
+ face->background.pixel,
+ SDATA (face->lface[LFACE_BACKGROUND_INDEX]));
+ }
+
if (face->font)
fprintf (stderr, "font_name: %s (%s)\n",
SDATA (face->font->props[FONT_NAME_INDEX]),
@@ -6450,7 +6515,7 @@ DEFUN ("dump-face", Fdump_face, Sdump_face, 0, 1, 0, doc: /* */)
face = FACE_FROM_ID_OR_NULL (SELECTED_FRAME (), XFIXNUM (n));
if (face == NULL)
error ("Not a valid face");
- dump_realized_face (face);
+ dump_realized_face (face, SELECTED_FRAME ());
}
return Qnil;
diff --git a/src/xfns.c b/src/xfns.c
index 2ceb55a30a..a500004541 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -651,9 +651,9 @@ gamma_correct (struct frame *f, XColor *color)
allocate the color. Value is false if COLOR_NAME is invalid, or
no color could be allocated. */
-bool
+static bool
x_defined_color (struct frame *f, const char *color_name,
- XColor *color, bool alloc_p, bool _makeIndex)
+ XColor *color, bool alloc_p)
{
bool success_p = false;
Colormap cmap = FRAME_X_COLORMAP (f);
@@ -671,6 +671,21 @@ x_defined_color (struct frame *f, const char *color_name,
return success_p;
}
+/* Like x_defined_color, but return the pixel value through the
+ emacs_color *COLOR. This is used as the defined_color terminal
+ hook. */
+bool
+x_defined_color_emacs_color (struct frame *f, const char *color_name,
+ union emacs_color *color, bool alloc_p,
+ bool _makeIndex)
+{
+ XColor col;
+ const bool res = x_defined_color (f, color_name, &col, alloc_p);
+
+ color->pixel = col.pixel;
+
+ return res;
+}
/* Return the pixel color value for color COLOR_NAME on frame F. If F
is a monochrome frame, return MONO_COLOR regardless of what ARG says.
@@ -698,7 +713,7 @@ x_decode_color (struct frame *f, Lisp_Object color_name, int mono_color)
/* x_defined_color is responsible for coping with failures
by looking for a near-miss. */
- if (x_defined_color (f, SSDATA (color_name), &cdef, true, false))
+ if (x_defined_color (f, SSDATA (color_name), &cdef, true))
return cdef.pixel;
signal_error ("Undefined color", color_name);
@@ -4110,7 +4125,7 @@ DEFUN ("xw-color-defined-p", Fxw_color_defined_p, Sxw_color_defined_p, 1, 2, 0,
CHECK_STRING (color);
- if (x_defined_color (f, SSDATA (color), &foo, false, false))
+ if (x_defined_color (f, SSDATA (color), &foo, false))
return Qt;
else
return Qnil;
@@ -4126,7 +4141,7 @@ DEFUN ("xw-color-values", Fxw_color_values, Sxw_color_values, 1, 2, 0,
CHECK_STRING (color);
- if (x_defined_color (f, SSDATA (color), &foo, false, false))
+ if (x_defined_color (f, SSDATA (color), &foo, false))
return list3i (foo.red, foo.green, foo.blue);
else
return Qnil;
diff --git a/src/xterm.c b/src/xterm.c
index 26f74cde91..cd92105599 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -1044,7 +1044,7 @@ x_draw_vertical_window_border (struct window *w, int x, int y0, int y1)
face = FACE_FROM_ID_OR_NULL (f, VERTICAL_BORDER_FACE_ID);
if (face)
XSetForeground (FRAME_X_DISPLAY (f), f->output_data.x->normal_gc,
- face->foreground);
+ face->foreground.pixel);
#ifdef USE_CAIRO
x_fill_rectangle (f, f->output_data.x->normal_gc, x, y0, 1, y1 - y0);
@@ -1065,12 +1065,14 @@ x_draw_window_divider (struct window *w, int x0, int x1, int y0, int y1)
= FACE_FROM_ID_OR_NULL (f, WINDOW_DIVIDER_FIRST_PIXEL_FACE_ID);
struct face *face_last
= FACE_FROM_ID_OR_NULL (f, WINDOW_DIVIDER_LAST_PIXEL_FACE_ID);
- unsigned long color = face ? face->foreground : FRAME_FOREGROUND_PIXEL (f);
+ unsigned long color = (face
+ ? face->foreground.pixel
+ : FRAME_FOREGROUND_PIXEL (f));
unsigned long color_first = (face_first
- ? face_first->foreground
+ ? face_first->foreground.pixel
: FRAME_FOREGROUND_PIXEL (f));
unsigned long color_last = (face_last
- ? face_last->foreground
+ ? face_last->foreground.pixel
: FRAME_FOREGROUND_PIXEL (f));
Display *display = FRAME_X_DISPLAY (f);
@@ -1243,7 +1245,7 @@ x_clear_under_internal_border (struct frame *f)
if (face)
{
- unsigned long color = face->background;
+ unsigned long color = face->background.pixel;
Display *display = FRAME_X_DISPLAY (f);
GC gc = f->output_data.x->normal_gc;
@@ -1309,7 +1311,7 @@ x_after_update_window_line (struct window *w, struct glyph_row *desired_row)
block_input ();
if (face)
{
- unsigned long color = face->background;
+ unsigned long color = face->background.pixel;
Display *display = FRAME_X_DISPLAY (f);
GC gc = f->output_data.x->normal_gc;
@@ -1350,12 +1352,12 @@ x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fring
if (face->stipple)
XSetFillStyle (display, face->gc, FillOpaqueStippled);
else
- XSetForeground (display, face->gc, face->background);
+ XSetForeground (display, face->gc, face->background.pixel);
x_fill_rectangle (f, face->gc, p->bx, p->by, p->nx, p->ny);
if (!face->stipple)
- XSetForeground (display, face->gc, face->foreground);
+ XSetForeground (display, face->gc, face->foreground.pixel);
}
#ifdef USE_CAIRO
@@ -1365,10 +1367,10 @@ x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fring
XGetGCValues (display, gc, GCForeground | GCBackground, &gcv);
XSetForeground (display, gc, (p->cursor_p
- ? (p->overlay_p ? face->background
+ ? (p->overlay_p ? face->background.pixel
: f->output_data.x->cursor_pixel)
- : face->foreground));
- XSetBackground (display, gc, face->background);
+ : face->foreground.pixel));
+ XSetBackground (display, gc, face->background.pixel);
x_cr_draw_image (f, gc, fringe_bmp[p->which], 0, 0, 0, p->dh,
p->wd, p->h, p->x, p->y, p->overlay_p);
XSetForeground (display, gc, gcv.foreground);
@@ -1392,10 +1394,12 @@ x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fring
by the server. */
pixmap = XCreatePixmapFromBitmapData (display, drawable, bits, p->wd, p->h,
(p->cursor_p
- ? (p->overlay_p ? face->background
+ ? (p->overlay_p
+ ? face->background.pixel
: f->output_data.x->cursor_pixel)
- : face->foreground),
- face->background, depth);
+ : face->foreground.pixel),
+ face->background.pixel,
+ depth);
if (p->overlay_p)
{
@@ -1445,8 +1449,8 @@ static void
x_set_cursor_gc (struct glyph_string *s)
{
if (s->font == FRAME_FONT (s->f)
- && s->face->background == FRAME_BACKGROUND_PIXEL (s->f)
- && s->face->foreground == FRAME_FOREGROUND_PIXEL (s->f)
+ && s->face->background.pixel == FRAME_BACKGROUND_PIXEL (s->f)
+ && s->face->foreground.pixel == FRAME_FOREGROUND_PIXEL (s->f)
&& !s->cmp)
s->gc = s->f->output_data.x->cursor_gc;
else
@@ -1456,22 +1460,22 @@ x_set_cursor_gc (struct glyph_string *s)
unsigned long mask;
xgcv.background = s->f->output_data.x->cursor_pixel;
- xgcv.foreground = s->face->background;
+ xgcv.foreground = s->face->background.pixel;
/* If the glyph would be invisible, try a different foreground. */
if (xgcv.foreground == xgcv.background)
- xgcv.foreground = s->face->foreground;
+ xgcv.foreground = s->face->foreground.pixel;
if (xgcv.foreground == xgcv.background)
xgcv.foreground = s->f->output_data.x->cursor_foreground_pixel;
if (xgcv.foreground == xgcv.background)
- xgcv.foreground = s->face->foreground;
+ xgcv.foreground = s->face->foreground.pixel;
/* Make sure the cursor is distinct from text in this face. */
- if (xgcv.background == s->face->background
- && xgcv.foreground == s->face->foreground)
+ if (xgcv.background == s->face->background.pixel
+ && xgcv.foreground == s->face->foreground.pixel)
{
- xgcv.background = s->face->foreground;
- xgcv.foreground = s->face->background;
+ xgcv.background = s->face->foreground.pixel;
+ xgcv.foreground = s->face->background.pixel;
}
IF_DEBUG (x_check_font (s->f, s->font));
@@ -1520,8 +1524,8 @@ x_set_mouse_face_gc (struct glyph_string *s)
XGCValues xgcv;
unsigned long mask;
- xgcv.background = s->face->background;
- xgcv.foreground = s->face->foreground;
+ xgcv.background = s->face->background.pixel;
+ xgcv.foreground = s->face->foreground.pixel;
xgcv.graphics_exposures = False;
mask = GCForeground | GCBackground | GCGraphicsExposures;
@@ -2581,11 +2585,11 @@ x_setup_relief_colors (struct glyph_string *s)
unsigned long color;
if (s->face->use_box_color_for_shadows_p)
- color = s->face->box_color;
+ color = s->face->box_color.pixel;
else if (s->first_glyph->type == IMAGE_GLYPH
&& s->img->pixmap
&& !IMAGE_BACKGROUND_TRANSPARENT (s->img, s->f, 0))
- color = IMAGE_BACKGROUND (s->img, s->f, 0);
+ color = IMAGE_BACKGROUND (s->img, s->f, 0).pixel;
else
{
XGCValues xgcv;
@@ -2804,7 +2808,7 @@ x_draw_box_rect (struct glyph_string *s,
XGCValues xgcv;
XGetGCValues (s->display, s->gc, GCForeground, &xgcv);
- XSetForeground (s->display, s->gc, s->face->box_color);
+ XSetForeground (s->display, s->gc, s->face->box_color.pixel);
x_set_clip_rectangles (s->f, s->gc, clip_rect, 1);
/* Top. */
@@ -3650,7 +3654,7 @@ x_draw_glyph_string (struct glyph_string *s)
{
XGCValues xgcv;
XGetGCValues (s->display, s->gc, GCForeground, &xgcv);
- XSetForeground (s->display, s->gc, s->face->underline_color);
+ XSetForeground (s->display, s->gc, s->face->underline_color.pixel);
x_draw_underwave (s);
XSetForeground (s->display, s->gc, xgcv.foreground);
}
@@ -3734,7 +3738,7 @@ x_draw_glyph_string (struct glyph_string *s)
{
XGCValues xgcv;
XGetGCValues (s->display, s->gc, GCForeground, &xgcv);
- XSetForeground (s->display, s->gc, s->face->underline_color);
+ XSetForeground (s->display, s->gc, s->face->underline_color.pixel);
x_fill_rectangle (s->f, s->gc,
s->x, y, s->width, thickness);
XSetForeground (s->display, s->gc, xgcv.foreground);
@@ -3753,7 +3757,7 @@ x_draw_glyph_string (struct glyph_string *s)
{
XGCValues xgcv;
XGetGCValues (s->display, s->gc, GCForeground, &xgcv);
- XSetForeground (s->display, s->gc, s->face->overline_color);
+ XSetForeground (s->display, s->gc, s->face->overline_color.pixel);
x_fill_rectangle (s->f, s->gc, s->x, s->y + dy,
s->width, h);
XSetForeground (s->display, s->gc, xgcv.foreground);
@@ -3782,7 +3786,7 @@ x_draw_glyph_string (struct glyph_string *s)
{
XGCValues xgcv;
XGetGCValues (s->display, s->gc, GCForeground, &xgcv);
- XSetForeground (s->display, s->gc, s->face->strike_through_color);
+ XSetForeground (s->display, s->gc, s->face->strike_through_color.pixel);
x_fill_rectangle (s->f, s->gc, s->x, glyph_y + dy,
s->width, h);
XSetForeground (s->display, s->gc, xgcv.foreground);
@@ -9299,8 +9303,8 @@ x_draw_bar_cursor (struct window *w, struct glyph_row *row, int width, enum text
invisible. Use the glyph's foreground color instead in this
case, on the assumption that the glyph's colors are chosen so
that the glyph is legible. */
- if (face->background == f->output_data.x->cursor_pixel)
- xgcv.background = xgcv.foreground = face->foreground;
+ if (face->background.pixel == f->output_data.x->cursor_pixel)
+ xgcv.background = xgcv.foreground = face->foreground.pixel;
else
xgcv.background = xgcv.foreground = f->output_data.x->cursor_pixel;
xgcv.graphics_exposures = False;
@@ -13203,7 +13207,7 @@ x_create_terminal (struct x_display_info *dpyinfo)
terminal->read_socket_hook = XTread_socket;
terminal->frame_up_to_date_hook = XTframe_up_to_date;
terminal->buffer_flipping_unblocked_hook = XTbuffer_flipping_unblocked_hook;
- terminal->defined_color_hook = x_defined_color;
+ terminal->defined_color_hook = x_defined_color_emacs_color;
terminal->query_frame_background_color = x_query_frame_background_color;
terminal->query_colors = x_query_colors;
terminal->mouse_position_hook = XTmouse_position;
diff --git a/src/xterm.h b/src/xterm.h
index 266a42afa0..de7bbdb37a 100644
--- a/src/xterm.h
+++ b/src/xterm.h
@@ -1222,7 +1222,8 @@ extern void destroy_frame_xic (struct frame *);
extern void xic_set_preeditarea (struct window *, int, int);
extern void xic_set_statusarea (struct frame *);
extern void xic_set_xfontset (struct frame *, const char *);
-extern bool x_defined_color (struct frame *, const char *, XColor *, bool, bool);
+extern bool x_defined_color_emacs_color (struct frame *, const char *,
+ union emacs_color *, bool, bool);
#ifdef HAVE_X_I18N
extern void free_frame_xic (struct frame *);
# if defined HAVE_X_WINDOWS && defined USE_X_TOOLKIT
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-04 18:08 Removing assumption of unsigned long pixel values for colours Alex Gramiak
@ 2019-05-04 18:39 ` Eli Zaretskii
2019-05-04 23:04 ` Alex Gramiak
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-04 18:39 UTC (permalink / raw)
To: Alex Gramiak; +Cc: emacs-devel
> From: Alex Gramiak <agrambot@gmail.com>
> Date: Sat, 04 May 2019 12:08:34 -0600
>
> I've attached a WIP patch (for illustrative purposes) creating a new
> union, emacs_color, that removes the single type limitation for the
> internal representation of colours. This approach is intended to be used
> with a new backend that I'm working on which uses a quadruple of
> normalized double values for each colour rather than a single pixel
> value. Using a union would avoid having to frequently convert between
> the two representations.
Can you describe the goal, in terms of the advantages of such a
representation, and the limitations of the existing representation
that it attempts to resolve? I think whether we want to make this
change depends on what will we gain from the changes.
Thanks.
> * prepare_face_for_display takes in an XGCValues, which assumes unsigned
> long colour values. On non-X systems that create their own XGCValues,
> they can just be redefined, but on X that won't work. That means a new
> struct has to also be defined on X and then copied into an XGCValues
> in x_create_gc.
>
> * The colors member of struct image was changed to use the new union,
> but XFreeColors expects an array of unsigned long pixels. It looks
> like the colors array is only used when COLOR_TABLE_SUPPORT is
> defined, so perhaps it could just used unsigned long unconditionally.
The main reason that we use unsigned integers for colors is that this
is how various color systems handle colors. It isn't an arbitrary
design decision. By using a different representation we introduce an
extra level of complexity, so it's important to know what will that
gain us.
> Question: In realize_gui_face, why is a defaulted underline colour set
> to 0 when defaulted overline/strike-through colours are set to the
> foreground colour? The comment above the underline case mentions that
> it's the same as the foreground colour, so should it be explicitly set
> there as well?
realize_gui_face just sets a flag, the implementation should be in the
*term.c back-ends. When that flag is set, the color of the underline
should be disregarded and taken from the current foreground color. On
MS-Windows, the underline changes its color according to the
foreground color of the text it underlines. Don't you see the same on X?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-04 18:39 ` Eli Zaretskii
@ 2019-05-04 23:04 ` Alex Gramiak
2019-05-05 16:14 ` Eli Zaretskii
2019-05-06 8:12 ` Alan Third
0 siblings, 2 replies; 22+ messages in thread
From: Alex Gramiak @ 2019-05-04 23:04 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Alan Third, emacs-devel
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Alex Gramiak <agrambot@gmail.com>
>> Date: Sat, 04 May 2019 12:08:34 -0600
>>
>> I've attached a WIP patch (for illustrative purposes) creating a new
>> union, emacs_color, that removes the single type limitation for the
>> internal representation of colours. This approach is intended to be used
>> with a new backend that I'm working on which uses a quadruple of
>> normalized double values for each colour rather than a single pixel
>> value. Using a union would avoid having to frequently convert between
>> the two representations.
>
> Can you describe the goal, in terms of the advantages of such a
> representation, and the limitations of the existing representation
> that it attempts to resolve? I think whether we want to make this
> change depends on what will we gain from the changes.
The goal is to avoid frequent conversions between the colour type used
in Emacs and the colour type used in drawing backends. In my case, the
drawing backend is Cairo (GTK), which uses 4 double values in the range
[0, 1] for ARGB. One can see the conversion from XColor to Cairo's
format in x_set_cr_source_with_gc_foreground and
x_set_cr_source_with_gc_background.
It's not as much of an issue with the X+Cairo backend since the colour
is calculated from XQueryColors which returns an XColor, but the backend
I'm working on uses gdk_rgba_parse to query colors, which returns a
GdkRGBA (which is compatible with Cairo's). This means that a conversion
from the quadruple into unsigned long has to be done for every colour
queried.
Using a union for stored colours would allow for direct access/storage
of the colours rather than conversions for every query and drawing
operation.
Also, AFAIU a GdkRGBA wouldn't necessarily convert into an unsigned long
without losing precision.
It might also help simplify the NS side to use NSColor objects directly
rather than using the unsigned long values as indices to an
ns_color_table (CC'd Alan to confirm/deny).
>> Question: In realize_gui_face, why is a defaulted underline colour set
>> to 0 when defaulted overline/strike-through colours are set to the
>> foreground colour? The comment above the underline case mentions that
>> it's the same as the foreground colour, so should it be explicitly set
>> there as well?
>
> realize_gui_face just sets a flag, the implementation should be in the
> *term.c back-ends. When that flag is set, the color of the underline
> should be disregarded and taken from the current foreground color. On
> MS-Windows, the underline changes its color according to the
> foreground color of the text it underlines. Don't you see the same on X?
Yes, I didn't mean that the underline colour wasn't inherited; I was
just referring to the value that underline_color is set to when
defaulted:
face->underline_defaulted_p = true;
face->underline_color = 0;
where the other defaulted cases set the colour explicitly (perhaps just
being overly cautious?):
face->overline_color = face->foreground;
face->overline_color_defaulted_p = true;
face->strike_through_color = face->foreground;
face->strike_through_color_defaulted_p = true;
AFAICS the value set doesn't matter (*_draw_glyph_string just checks the
flags), but I wanted to check that there wasn't a reason that
underline_color was different that I was missing.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-04 23:04 ` Alex Gramiak
@ 2019-05-05 16:14 ` Eli Zaretskii
2019-05-05 19:35 ` Alex Gramiak
2019-05-06 8:12 ` Alan Third
1 sibling, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-05 16:14 UTC (permalink / raw)
To: Alex Gramiak; +Cc: alan, emacs-devel
> From: Alex Gramiak <agrambot@gmail.com>
> Cc: emacs-devel@gnu.org, Alan Third <alan@idiocy.org>
> Date: Sat, 04 May 2019 17:04:43 -0600
>
> > Can you describe the goal, in terms of the advantages of such a
> > representation, and the limitations of the existing representation
> > that it attempts to resolve? I think whether we want to make this
> > change depends on what will we gain from the changes.
>
> The goal is to avoid frequent conversions between the colour type used
> in Emacs and the colour type used in drawing backends. In my case, the
> drawing backend is Cairo (GTK), which uses 4 double values in the range
> [0, 1] for ARGB. One can see the conversion from XColor to Cairo's
> format in x_set_cr_source_with_gc_foreground and
> x_set_cr_source_with_gc_background.
>
> It's not as much of an issue with the X+Cairo backend since the colour
> is calculated from XQueryColors which returns an XColor, but the backend
> I'm working on uses gdk_rgba_parse to query colors, which returns a
> GdkRGBA (which is compatible with Cairo's). This means that a conversion
> from the quadruple into unsigned long has to be done for every colour
> queried.
Such conversion needs about 2 to 3 machine instructions on modern
hardware, so why is it a problem? If you are annoyed by seeing it in
the sources all the time, then a macro or an inline function should
take care of that.
Are there other issues that caused you to propose this change?
I see a couple of disadvantages with your proposal:
. it leaks to platform-independent parts of the code representation
that is specific to one platform, something that goes against
everything you've been doing lately in your other work
. the union you propose has no indication which of its
interpretations is being used, which will lead to bugs
. the structures which use colors will now be significantly larger,
because instead of a single 32-bit or 64-bit number the union will
need to hold 4 64-bit double-precision numbers (larger structures
slow down Emacs)
Against these disadvantages, what are the advantages? just the fact
that you can carry the native color representation of some platform?
IMO, that's not enough to justify such changes.
> Also, AFAIU a GdkRGBA wouldn't necessarily convert into an unsigned long
> without losing precision.
If we want to increase the number of bits per pixel we support in
Emacs, we need to do that for all the platforms. Currently, they all
are aligned on 32 bpp, AFAIK. If indeed there will be loss of
information (and I'd like to see some evidence to that first), then we
need to move all the platforms to higher precision, not just one.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-05 16:14 ` Eli Zaretskii
@ 2019-05-05 19:35 ` Alex Gramiak
2019-05-06 2:45 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Alex Gramiak @ 2019-05-05 19:35 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: emacs-devel
Eli Zaretskii <eliz@gnu.org> writes:
>> It's not as much of an issue with the X+Cairo backend since the colour
>> is calculated from XQueryColors which returns an XColor, but the backend
>> I'm working on uses gdk_rgba_parse to query colors, which returns a
>> GdkRGBA (which is compatible with Cairo's). This means that a conversion
>> from the quadruple into unsigned long has to be done for every colour
>> queried.
>
> Such conversion needs about 2 to 3 machine instructions on modern
> hardware, so why is it a problem? If you are annoyed by seeing it in
> the sources all the time, then a macro or an inline function should
> take care of that.
It's certainly possible that I'm overestimating the cost of the
conversions. It just doesn't seem good to have to do this conversion for
every draw operation.
Perhaps I should follow the old adage and just use unsigned long with
conversions first and benchmark after.
> Are there other issues that caused you to propose this change?
Not directly, but to avoid the conversions I did add additional
complexity by using a local tagged union that I feel is too much
trouble. I figured that I'd float this option by before changing
strategies.
> I see a couple of disadvantages with your proposal:
>
> . it leaks to platform-independent parts of the code representation
> that is specific to one platform, something that goes against
> everything you've been doing lately in your other work
I'm not sure what leaks you're referring to here. The cases that use
.pixel directly should only be in the TTY-specific or X-specific code.
> . the union you propose has no indication which of its
> interpretations is being used, which will lead to bugs
I considered that, but as it looks like faces are tied to frames (which
are tied to terminals), then the type shouldn't need to be checked as
long as the terminal-independent code doesn't alter it.
> (larger structures slow down Emacs)
More so than other programs?
>> Also, AFAIU a GdkRGBA wouldn't necessarily convert into an unsigned long
>> without losing precision.
>
> If we want to increase the number of bits per pixel we support in
> Emacs, we need to do that for all the platforms. Currently, they all
> are aligned on 32 bpp, AFAIK. If indeed there will be loss of
> information (and I'd like to see some evidence to that first), then we
> need to move all the platforms to higher precision, not just one.
Wouldn't it only need to be done for platforms that support a higher
pixel depth?
gdk_rgba_parse uses pango_color_parse, which returns a PangoColor (48
bpp RGB), and normalizes each component over 2^16. If using either
CAIRO_FORMAT_RGB30 (30 bpp RGB) with image surfaces or using the OpenGL
backend (which I believe the GTK Wayland backend uses), then storing
ARGB values into an unsigned long would mean lost precision that could
have been used.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-05 19:35 ` Alex Gramiak
@ 2019-05-06 2:45 ` Eli Zaretskii
2019-05-06 14:13 ` Daniel Pittman
2019-05-06 15:11 ` Alex Gramiak
0 siblings, 2 replies; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-06 2:45 UTC (permalink / raw)
To: Alex Gramiak; +Cc: emacs-devel
> From: Alex Gramiak <agrambot@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 05 May 2019 13:35:01 -0600
>
> It's certainly possible that I'm overestimating the cost of the
> conversions. It just doesn't seem good to have to do this conversion for
> every draw operation.
>
> Perhaps I should follow the old adage and just use unsigned long with
> conversions first and benchmark after.
Yes, please.
> > . it leaks to platform-independent parts of the code representation
> > that is specific to one platform, something that goes against
> > everything you've been doing lately in your other work
>
> I'm not sure what leaks you're referring to here. The cases that use
> .pixel directly should only be in the TTY-specific or X-specific code.
No, I meant the RGBA representation. The integer color values are our
current abstraction.
> > . the union you propose has no indication which of its
> > interpretations is being used, which will lead to bugs
>
> I considered that, but as it looks like faces are tied to frames (which
> are tied to terminals), then the type shouldn't need to be checked as
> long as the terminal-independent code doesn't alter it.
That "as long as" part is already a potential problem. And I see
other opportunities for exciting bugs.
> > (larger structures slow down Emacs)
>
> More so than other programs?
Why should other programs matter here? I don't want to have slow-down
in the inner-most loops of Emacs, because that causes legitimate user
complaints. Faces are used in redisplay.
> > If we want to increase the number of bits per pixel we support in
> > Emacs, we need to do that for all the platforms. Currently, they all
> > are aligned on 32 bpp, AFAIK. If indeed there will be loss of
> > information (and I'd like to see some evidence to that first), then we
> > need to move all the platforms to higher precision, not just one.
>
> Wouldn't it only need to be done for platforms that support a higher
> pixel depth?
No, we want all platforms to support the same color representation,
for various good reasons. For example, platform-independent
representation of standard colors.
> gdk_rgba_parse uses pango_color_parse, which returns a PangoColor (48
> bpp RGB), and normalizes each component over 2^16. If using either
> CAIRO_FORMAT_RGB30 (30 bpp RGB) with image surfaces or using the OpenGL
> backend (which I believe the GTK Wayland backend uses), then storing
> ARGB values into an unsigned long would mean lost precision that could
> have been used.
Once again, I'm okay with discussing a change for all the platforms,
but it needs to show benefits for all of them, or at least a
majority. What you describe could mean a use of a single 'double' for
a color; we definitely don't need 4 'double's yet.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-04 23:04 ` Alex Gramiak
2019-05-05 16:14 ` Eli Zaretskii
@ 2019-05-06 8:12 ` Alan Third
2019-05-06 9:18 ` mituharu
2019-05-06 15:06 ` Eli Zaretskii
1 sibling, 2 replies; 22+ messages in thread
From: Alan Third @ 2019-05-06 8:12 UTC (permalink / raw)
To: Alex Gramiak; +Cc: Eli Zaretskii, emacs-devel
On Sat, May 04, 2019 at 05:04:43PM -0600, Alex Gramiak wrote:
>
> It might also help simplify the NS side to use NSColor objects directly
> rather than using the unsigned long values as indices to an
> ns_color_table (CC'd Alan to confirm/deny).
Unfortunately I don’t know too much about this area, but it certainly
looks like it might simplify things. It seems like there’s a lot of
conversion from NSColor, to unsigned long, which must then be
converted back to NSColor for drawing.
--
Alan Third
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 8:12 ` Alan Third
@ 2019-05-06 9:18 ` mituharu
2019-05-06 15:06 ` Eli Zaretskii
1 sibling, 0 replies; 22+ messages in thread
From: mituharu @ 2019-05-06 9:18 UTC (permalink / raw)
To: Alan Third; +Cc: Eli Zaretskii, Alex Gramiak, emacs-devel
> On Sat, May 04, 2019 at 05:04:43PM -0600, Alex Gramiak wrote:
>>
>> It might also help simplify the NS side to use NSColor objects directly
>> rather than using the unsigned long values as indices to an
>> ns_color_table (CC'd Alan to confirm/deny).
>
> Unfortunately I don’t know too much about this area, but it certainly
> looks like it might simplify things. It seems like there’s a lot of
> conversion from NSColor, to unsigned long, which must then be
> converted back to NSColor for drawing.
In the Mac port, CGColor objects are encapsulated into GC, and
conversion from unsigned long only happens when setting GC
foreground/background values.
YAMAMOTO Mitsuharu
mituharu@math.s.chiba-u.ac.jp
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 2:45 ` Eli Zaretskii
@ 2019-05-06 14:13 ` Daniel Pittman
2019-05-06 16:11 ` Alex Gramiak
2019-05-06 15:11 ` Alex Gramiak
1 sibling, 1 reply; 22+ messages in thread
From: Daniel Pittman @ 2019-05-06 14:13 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Alex Gramiak, emacs-devel
[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]
>
>
> > It's certainly possible that I'm overestimating the cost of the
> > conversions. It just doesn't seem good to have to do this conversion for
> > every draw operation.
> >
> > Perhaps I should follow the old adage and just use unsigned long with
> > conversions first and benchmark after.
>
> Yes, please.
>
If you wanted to explore the optimization (and because I was curious,
personally):
https://godbolt.org/z/vC_yOg
You can change the width of the representation, and possibly even use
signed int color values (untested), to see the effect it has. spoiler
alert, pretty much none, even with a 64-bit integer for each color value.
Even using a relatively old compilers, gcc 4.1.2 from February 13, 2007,
the compiled results are quite efficient.
See the LLVM-MCA analysis, but your costs seem well within reasonable
limits to me.
I don't think I'd cache it either: at around 14 cycles per conversion, it
is around 5.3ns to run, and fetching from in the L1 cache is around 9ns, so
your best case scenario is that you took 60 percent longer to fetch it than
to convert it, excluding CPU stalls from other activity.
So, benchmark if you want, but I'm pretty sure you can't win much back by
doing the conversion only once, rather than inline at execution time.
[-- Attachment #2: Type: text/html, Size: 1838 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 8:12 ` Alan Third
2019-05-06 9:18 ` mituharu
@ 2019-05-06 15:06 ` Eli Zaretskii
1 sibling, 0 replies; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-06 15:06 UTC (permalink / raw)
To: Alan Third; +Cc: agrambot, emacs-devel
> Date: Mon, 6 May 2019 09:12:55 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
>
> On Sat, May 04, 2019 at 05:04:43PM -0600, Alex Gramiak wrote:
> >
> > It might also help simplify the NS side to use NSColor objects directly
> > rather than using the unsigned long values as indices to an
> > ns_color_table (CC'd Alan to confirm/deny).
>
> Unfortunately I don’t know too much about this area, but it certainly
> looks like it might simplify things. It seems like there’s a lot of
> conversion from NSColor, to unsigned long, which must then be
> converted back to NSColor for drawing.
AFAICS, it just indexes into a table of color descriptors. Which is
something similar to what we do on TTY frames as well, only more
efficient.
Once again, I'm okay, in principle, with changing our
platform-independent abstraction of a color to a different
representation, if that would benefit some of the platforms, but such
a change will have to be across the board, and I imagine it would be
tricky, since the current abstraction is exposed to users (e.g., see
how many of the themes customize their colors).
It's the idea of having 2 representations at once on the
platform-independent level that I don't like.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 2:45 ` Eli Zaretskii
2019-05-06 14:13 ` Daniel Pittman
@ 2019-05-06 15:11 ` Alex Gramiak
2019-05-06 15:45 ` Eli Zaretskii
1 sibling, 1 reply; 22+ messages in thread
From: Alex Gramiak @ 2019-05-06 15:11 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: emacs-devel
Eli Zaretskii <eliz@gnu.org> writes:
>> I'm not sure what leaks you're referring to here. The cases that use
>> .pixel directly should only be in the TTY-specific or X-specific code.
>
> No, I meant the RGBA representation. The integer color values are our
> current abstraction.
Do you mean the conditional GdkRGBA in emacs_color? I don't think that's
really any different than the platform-specific output_data types. What
would the alternatives be in the case that a single representation
couldn't handle all supported platforms?
>> > (larger structures slow down Emacs)
>>
>> More so than other programs?
>
> Why should other programs matter here? I don't want to have slow-down
> in the inner-most loops of Emacs, because that causes legitimate user
> complaints. Faces are used in redisplay.
I meant if there was any specific reason to Emacs that slightly larger
structures would cause a non-negligible slowdown. In any case, I would
have thought that the conversions would cause more of a slow-down, but
we can perhaps find that out later.
>> > If we want to increase the number of bits per pixel we support in
>> > Emacs, we need to do that for all the platforms. Currently, they all
>> > are aligned on 32 bpp, AFAIK. If indeed there will be loss of
>> > information (and I'd like to see some evidence to that first), then we
>> > need to move all the platforms to higher precision, not just one.
>>
>> Wouldn't it only need to be done for platforms that support a higher
>> pixel depth?
>
> No, we want all platforms to support the same color representation,
> for various good reasons. For example, platform-independent
> representation of standard colors.
Ideally, but if there is no way to represent a certain precision on a
particular platform, and if the size of structures is of concern to you,
then would it not make sense to only support the maximum precision
possible?
I meant something along the lines of:
#ifdef <Using a platform needing 64-bits>
typedef unsigned long long emacs_pixel;
#else
typedef unsigned long emacs_pixel;
#endif
>> gdk_rgba_parse uses pango_color_parse, which returns a PangoColor (48
>> bpp RGB), and normalizes each component over 2^16. If using either
>> CAIRO_FORMAT_RGB30 (30 bpp RGB) with image surfaces or using the OpenGL
>> backend (which I believe the GTK Wayland backend uses), then storing
>> ARGB values into an unsigned long would mean lost precision that could
>> have been used.
>
> Once again, I'm okay with discussing a change for all the platforms,
> but it needs to show benefits for all of them, or at least a
> majority.
I'm not sure about the situation on other platforms, but IMO it would be
worthwhile to discuss a change even if it benefits only a single
supported platform as long as it doesn't introduce non-trivial
complexity to the other platforms (such as the above #ifdef).
> What you describe could mean a use of a single 'double' for
> a color; we definitely don't need 4 'double's yet.
I'm not sure that a single double would suffice here.
P.S. You mention "platform-independent representation of standard
colors", but isn't the unsigned long used differently on different
platforms already? NS and X seem to use it as indices to color tables
(AFAIU X uses the pixel value to lookup a 48-bpp RGB triplet and store
it in an XColor), and w32 uses it to embed a COLORREF.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 15:11 ` Alex Gramiak
@ 2019-05-06 15:45 ` Eli Zaretskii
2019-05-06 16:29 ` Alex Gramiak
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-06 15:45 UTC (permalink / raw)
To: Alex Gramiak; +Cc: emacs-devel
> From: Alex Gramiak <agrambot@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Mon, 06 May 2019 09:11:18 -0600
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> I'm not sure what leaks you're referring to here. The cases that use
> >> .pixel directly should only be in the TTY-specific or X-specific code.
> >
> > No, I meant the RGBA representation. The integer color values are our
> > current abstraction.
>
> Do you mean the conditional GdkRGBA in emacs_color?
Yes.
> I don't think that's really any different than the platform-specific
> output_data types. What would the alternatives be in the case that a
> single representation couldn't handle all supported platforms?
I don't know, we will need to discuss this when we have an actual
problem. For now this is a hypothetical problem.
> I meant if there was any specific reason to Emacs that slightly larger
> structures would cause a non-negligible slowdown.
It could happen if enlarging a structure makes it large enough to not
fit into a cache line. So we should do this only if really necessary,
and then very cautiously. The display engine already juggles some
very large structures.
> In any case, I would have thought that the conversions would cause
> more of a slow-down, but we can perhaps find that out later.
I think you are mistaken, doing simple arithmetics, even FP
arithmetics, is lightning fast on modern machines, specially since
they have several execution units that work in parallel.
> > No, we want all platforms to support the same color representation,
> > for various good reasons. For example, platform-independent
> > representation of standard colors.
>
> Ideally, but if there is no way to represent a certain precision on a
> particular platform, and if the size of structures is of concern to you,
> then would it not make sense to only support the maximum precision
> possible?
>
> I meant something along the lines of:
>
> #ifdef <Using a platform needing 64-bits>
> typedef unsigned long long emacs_pixel;
> #else
> typedef unsigned long emacs_pixel;
> #endif
This will bite us at some point because we currently more or less
expose the X color values to users.
> P.S. You mention "platform-independent representation of standard
> colors", but isn't the unsigned long used differently on different
> platforms already? NS and X seem to use it as indices to color tables
> (AFAIU X uses the pixel value to lookup a 48-bpp RGB triplet and store
> it in an XColor), and w32 uses it to embed a COLORREF.
COLORREF is just the RGB representation of a color, see
https://docs.microsoft.com/en-us/windows/desktop/gdi/colorref
Our color abstraction is also RGB representation, which AFAIR comes
from X. Maybe X uses something different nowadays, but since the RGB
color descriptions are exposed to Lisp, changing that would be tricky.
So conversion where needed is TRT, I think. And IME it rarely is
expensive enough to matter, so I don't think the need to convert per
se justifies significant changes in interfaces and infrastructure.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 14:13 ` Daniel Pittman
@ 2019-05-06 16:11 ` Alex Gramiak
2019-05-06 16:51 ` Stefan Monnier
0 siblings, 1 reply; 22+ messages in thread
From: Alex Gramiak @ 2019-05-06 16:11 UTC (permalink / raw)
To: Daniel Pittman; +Cc: Eli Zaretskii, emacs-devel
Daniel Pittman <slippycheeze@google.com> writes:
> If you wanted to explore the optimization (and because I was curious, personally):
> https://godbolt.org/z/vC_yOg
>
> You can change the width of the representation, and possibly even use signed int color values (untested), to see the effect it has. spoiler alert, pretty much none, even with a 64-bit integer for each color value.
>
> Even using a relatively old compilers, gcc 4.1.2 from February 13, 2007, the compiled results are quite efficient.
> See the LLVM-MCA analysis, but your costs seem well within reasonable limits to me.
>
> I don't think I'd cache it either: at around 14 cycles per conversion, it is around 5.3ns to run, and fetching from in the L1 cache is around 9ns, so your best case scenario is that you took 60 percent longer to fetch it than to convert it, excluding
> CPU stalls from other activity.
Thanks for exploring this! It's good to see that the results are
efficient.
> So, benchmark if you want, but I'm pretty sure you can't win much back by doing the conversion only once, rather than inline at execution time.
The original plan was to avoid any conversion at all (no conversion
necessary on the Emacs side to create the struct of doubles); would you
still consider the difference between no conversion and 4 conversions to
be negligible?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 15:45 ` Eli Zaretskii
@ 2019-05-06 16:29 ` Alex Gramiak
2019-05-06 16:54 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Alex Gramiak @ 2019-05-06 16:29 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: emacs-devel
Eli Zaretskii <eliz@gnu.org> writes:
>> Ideally, but if there is no way to represent a certain precision on a
>> particular platform, and if the size of structures is of concern to you,
>> then would it not make sense to only support the maximum precision
>> possible?
>>
>> I meant something along the lines of:
>>
>> #ifdef <Using a platform needing 64-bits>
>> typedef unsigned long long emacs_pixel;
>> #else
>> typedef unsigned long emacs_pixel;
>> #endif
>
> This will bite us at some point because we currently more or less
> expose the X color values to users.
What do you mean here? Where is the pixel value exposed? I see that
color-values exposes a 16 bpc RGB representation, which the unsigned
long long still supports (as long as it uses 16 bpc with an alpha
channel, which is what I want to use it for).
>> P.S. You mention "platform-independent representation of standard
>> colors", but isn't the unsigned long used differently on different
>> platforms already? NS and X seem to use it as indices to color tables
>> (AFAIU X uses the pixel value to lookup a 48-bpp RGB triplet and store
>> it in an XColor), and w32 uses it to embed a COLORREF.
>
> COLORREF is just the RGB representation of a color, see
>
> https://docs.microsoft.com/en-us/windows/desktop/gdi/colorref
Right, but my point is that AFAIU the unsigned long value is used in a
different manor (embedded rather than used to lookup a value) on the w32
side, so it could be viewed that the representation of colors is already
platform-dependent.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 16:11 ` Alex Gramiak
@ 2019-05-06 16:51 ` Stefan Monnier
2019-05-06 20:03 ` Alex Gramiak
0 siblings, 1 reply; 22+ messages in thread
From: Stefan Monnier @ 2019-05-06 16:51 UTC (permalink / raw)
To: emacs-devel
> The original plan was to avoid any conversion at all (no conversion
> necessary on the Emacs side to create the struct of doubles); would you
> still consider the difference between no conversion and 4 conversions to
> be negligible?
That which costs is when we can't use a pre-existing struct and need to
allocate a new struct and copy the values to it (with or without
conversion).
If we need to do the copy anyway, the added cost of a conversion along
the way is very likely to be negligible. The rule of thumb is that
arithmetic operations are free (contrary to data movement).
[ Which also means that "the conversion" can be costly in the case where
the need for the conversion in turn requires allocation of
a new struct. ]
Stefan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 16:29 ` Alex Gramiak
@ 2019-05-06 16:54 ` Eli Zaretskii
2019-05-06 17:14 ` Alex Gramiak
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-06 16:54 UTC (permalink / raw)
To: Alex Gramiak; +Cc: emacs-devel
> From: Alex Gramiak <agrambot@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Mon, 06 May 2019 10:29:38 -0600
>
> > COLORREF is just the RGB representation of a color, see
> >
> > https://docs.microsoft.com/en-us/windows/desktop/gdi/colorref
>
> Right, but my point is that AFAIU the unsigned long value is used in a
> different manor (embedded rather than used to lookup a value) on the w32
> side, so it could be viewed that the representation of colors is already
> platform-dependent.
OK, but that just says that each platform already performs the mapping
you wanted to avoid. What makes your back-end special that it needs
to avoid this?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 16:54 ` Eli Zaretskii
@ 2019-05-06 17:14 ` Alex Gramiak
2019-05-06 17:39 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Alex Gramiak @ 2019-05-06 17:14 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: emacs-devel
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Alex Gramiak <agrambot@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Mon, 06 May 2019 10:29:38 -0600
>>
>> > COLORREF is just the RGB representation of a color, see
>> >
>> > https://docs.microsoft.com/en-us/windows/desktop/gdi/colorref
>>
>> Right, but my point is that AFAIU the unsigned long value is used in a
>> different manor (embedded rather than used to lookup a value) on the w32
>> side, so it could be viewed that the representation of colors is already
>> platform-dependent.
>
> OK, but that just says that each platform already performs the mapping
> you wanted to avoid. What makes your back-end special that it needs
> to avoid this?
My initial reasoning was the time argument, which turned out to be
wrong.
Now I want to avoid the lookup table approach and try to just embed the
GdkRGBA into a pixel value, but unsigned long is not necessarily wide
enough to support this.
Should I take the lack of response to the other point as an OK for using
the emacs_pixel typedef approach? An unsigned long long would be enough
to fit the 16bpc ARGB value in.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 17:14 ` Alex Gramiak
@ 2019-05-06 17:39 ` Eli Zaretskii
2019-05-06 18:00 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-06 17:39 UTC (permalink / raw)
To: Alex Gramiak; +Cc: emacs-devel
> From: Alex Gramiak <agrambot@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Mon, 06 May 2019 11:14:41 -0600
>
> > OK, but that just says that each platform already performs the mapping
> > you wanted to avoid. What makes your back-end special that it needs
> > to avoid this?
>
> My initial reasoning was the time argument, which turned out to be
> wrong.
>
> Now I want to avoid the lookup table approach
Once again: why?
And what table look up do you need, when you originally said that a
conversion would do? What am I missing?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 17:39 ` Eli Zaretskii
@ 2019-05-06 18:00 ` Eli Zaretskii
2019-05-06 19:49 ` Alex Gramiak
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-06 18:00 UTC (permalink / raw)
To: agrambot; +Cc: emacs-devel
> Date: Mon, 06 May 2019 20:39:13 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
>
> > From: Alex Gramiak <agrambot@gmail.com>
> > Cc: emacs-devel@gnu.org
> > Date: Mon, 06 May 2019 11:14:41 -0600
> >
> > > OK, but that just says that each platform already performs the mapping
> > > you wanted to avoid. What makes your back-end special that it needs
> > > to avoid this?
> >
> > My initial reasoning was the time argument, which turned out to be
> > wrong.
> >
> > Now I want to avoid the lookup table approach
>
> Once again: why?
Btw, I think this discussion basically puts the cart before the horse,
so to speak. You say you are developing a new back-end, which might
need these changes, but we never saw that back-end and didn't yet
decide it's a useful addition to Emacs. IMO, this is the wrong order
of discussing this stuff. let's first see the feature itself, discuss
its implementation and integration with Emacs, and only after that
talk about any necessary changes in the infrastructure. That way, all
the sides of the discussion will know what's at stake, and the
discussion becomes more concrete.
OK?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 18:00 ` Eli Zaretskii
@ 2019-05-06 19:49 ` Alex Gramiak
2019-05-07 2:29 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Alex Gramiak @ 2019-05-06 19:49 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: emacs-devel
Eli Zaretskii <eliz@gnu.org> writes:
> Once again: why?
A table lookup would require memory allocation that could be easily
avoided if the integral representation that Emacs uses for colours was
at least 64-bit instead of 32-bit.
> And what table look up do you need, when you originally said that a
> conversion would do? What am I missing?
I brought up the table lookup as an alternative to the converted
embedding scheme if you weren't okay with widening the integral
representation. Otherwise the embedding could lose precision as
mentioned previously.
> Btw, I think this discussion basically puts the cart before the horse,
> so to speak. You say you are developing a new back-end, which might
> need these changes, but we never saw that back-end and didn't yet
> decide it's a useful addition to Emacs. IMO, this is the wrong order
> of discussing this stuff. let's first see the feature itself, discuss
> its implementation and integration with Emacs, and only after that
> talk about any necessary changes in the infrastructure. That way, all
> the sides of the discussion will know what's at stake, and the
> discussion becomes more concrete.
>
> OK?
Sure, though I don't know what relevant information is missing; the goal
of this thread was to determine if a certain change is suitable
_assuming_ that such a backend is submitted with the change, and that
the backend needs it (or rather finds it sufficiently useful). I just
wanted to avoid having to implement the union scheme fully and having to
revert it upon review. It turns out that it was good to post this, since
I was assured that using the integral representation is good enough.
I can just restrict the colours to 8bpc in the meantime.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 16:51 ` Stefan Monnier
@ 2019-05-06 20:03 ` Alex Gramiak
0 siblings, 0 replies; 22+ messages in thread
From: Alex Gramiak @ 2019-05-06 20:03 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> The original plan was to avoid any conversion at all (no conversion
>> necessary on the Emacs side to create the struct of doubles); would you
>> still consider the difference between no conversion and 4 conversions to
>> be negligible?
>
> That which costs is when we can't use a pre-existing struct and need to
> allocate a new struct and copy the values to it (with or without
> conversion).
The no-conversion method I was proposing wouldn't involve heap
allocation, but it would involve copying the doubles around from, e.g.,
a frame struct to GC struct.
I might have been able to avoid some of those copies by using GdkRGBA
pointers in the GC structs.
> If we need to do the copy anyway, the added cost of a conversion along
> the way is very likely to be negligible. The rule of thumb is that
> arithmetic operations are free (contrary to data movement).
I knew that they're efficient, but I thought given the high frequency of
draw operations that it would do well to shave them off. I guess my
intuition isn't great on this topic.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Removing assumption of unsigned long pixel values for colours
2019-05-06 19:49 ` Alex Gramiak
@ 2019-05-07 2:29 ` Eli Zaretskii
0 siblings, 0 replies; 22+ messages in thread
From: Eli Zaretskii @ 2019-05-07 2:29 UTC (permalink / raw)
To: Alex Gramiak; +Cc: emacs-devel
> From: Alex Gramiak <agrambot@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Mon, 06 May 2019 13:49:30 -0600
>
> Sure, though I don't know what relevant information is missing; the goal
> of this thread was to determine if a certain change is suitable
> _assuming_ that such a backend is submitted with the change, and that
> the backend needs it (or rather finds it sufficiently useful). I just
> wanted to avoid having to implement the union scheme fully and having to
> revert it upon review. It turns out that it was good to post this, since
> I was assured that using the integral representation is good enough.
I wasn't saying that the question shouldn't have been asked. I was
saying that, once the union idea was off the table, discussing
alternatives would be better delayed until the need for that is clear,
and that needs to see the clients of such alternatives.
> I can just restrict the colours to 8bpc in the meantime.
Thanks. I think you could also assume 16 bpp on 64-bit hosts.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-05-07 2:29 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-04 18:08 Removing assumption of unsigned long pixel values for colours Alex Gramiak
2019-05-04 18:39 ` Eli Zaretskii
2019-05-04 23:04 ` Alex Gramiak
2019-05-05 16:14 ` Eli Zaretskii
2019-05-05 19:35 ` Alex Gramiak
2019-05-06 2:45 ` Eli Zaretskii
2019-05-06 14:13 ` Daniel Pittman
2019-05-06 16:11 ` Alex Gramiak
2019-05-06 16:51 ` Stefan Monnier
2019-05-06 20:03 ` Alex Gramiak
2019-05-06 15:11 ` Alex Gramiak
2019-05-06 15:45 ` Eli Zaretskii
2019-05-06 16:29 ` Alex Gramiak
2019-05-06 16:54 ` Eli Zaretskii
2019-05-06 17:14 ` Alex Gramiak
2019-05-06 17:39 ` Eli Zaretskii
2019-05-06 18:00 ` Eli Zaretskii
2019-05-06 19:49 ` Alex Gramiak
2019-05-07 2:29 ` Eli Zaretskii
2019-05-06 8:12 ` Alan Third
2019-05-06 9:18 ` mituharu
2019-05-06 15:06 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).