Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
libpayload: cbgfx: Add color mapping functionality
Similar to set_blend(), add set_color_map() for mapping background and foreground colors of a bitmap. Also add clear_color_map() for clearing the saved color mappings.
Note that when drawing a bitmap, the color mapping will be applied before blending.
BRANCH=puff BUG=b:146399181, b:162357639 TEST=emerge-puff libpayload
Change-Id: I640ff3e8455cd4aaa5a41d03a0183dff282648a5 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 64 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44375/1
diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index b52bd99..49a05fc 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -61,6 +61,32 @@ .y = 0, };
+struct color_mapping { + struct rgb_color bg; + struct rgb_color fg; + int enabled; +}; + +static struct color_mapping color_map; + +int set_color_map(const struct rgb_color *background, + const struct rgb_color *foreground) +{ + if (background == NULL || foreground == NULL) + return CBGFX_ERROR_INVALID_PARAMETER; + + color_map.bg = *background; + color_map.fg = *foreground; + color_map.enabled = 1; + + return CBGFX_SUCCESS; +} + +void clear_color_map(void) +{ + color_map.enabled = 0; +} + struct blend_value { uint8_t alpha; struct rgb_color rgb; @@ -185,6 +211,17 @@ return -1; }
+/* Helper function that applies color_map to the color. */ +static inline uint8_t apply_map(uint8_t color, + uint8_t bg_color, uint8_t fg_color) +{ + if (!color_map.enabled) + return color; + + return ((int16_t)bg_color * (UINT8_MAX - color) + + (int16_t)fg_color * color) / UINT8_MAX; +} + /* * Helper function that applies color and opacity from blend struct * into the color. @@ -203,13 +240,19 @@ { uint32_t color = 0;
- color |= (apply_blend(rgb->red, blend.rgb.red) + color |= (apply_blend(apply_map(rgb->red, + color_map.bg.red, color_map.fg.red), + blend.rgb.red) >> (8 - fbinfo->red_mask_size)) << fbinfo->red_mask_pos; - color |= (apply_blend(rgb->green, blend.rgb.green) + color |= (apply_blend(apply_map(rgb->green, + color_map.bg.green, color_map.fg.green), + blend.rgb.green) >> (8 - fbinfo->green_mask_size)) << fbinfo->green_mask_pos; - color |= (apply_blend(rgb->blue, blend.rgb.blue) + color |= (apply_blend(apply_map(rgb->blue, + color_map.bg.blue, color_map.fg.blue), + blend.rgb.blue) >> (8 - fbinfo->blue_mask_size)) << fbinfo->blue_mask_pos; if (invert) diff --git a/payloads/libpayload/include/cbgfx.h b/payloads/libpayload/include/cbgfx.h index f2883b0..85b61a7 100644 --- a/payloads/libpayload/include/cbgfx.h +++ b/payloads/libpayload/include/cbgfx.h @@ -228,6 +228,24 @@ int get_bitmap_dimension(const void *bitmap, size_t sz, struct scale *dim_rel);
/** + * Setup color mappings of background and foreground colors. Black and white + * pixels will be mapped to the background and foreground colors, respectively. + * Call clear_color_map() to disabled color mapping. + * + * @param[in] background Background color. + * @param[in] foreground Foreground color. + * + * @return CBGFX_* error codes + */ +int set_color_map(const struct rgb_color *background, + const struct rgb_color *foreground); + +/** + * Clear color mappings. + */ +void clear_color_map(void); + +/** * Setup alpha and rgb values for alpha blending. When alpha is != 0, * this enables a translucent layer of color (defined by rgb) to be * blended at a given translucency (alpha) to all things drawn. Call @@ -244,8 +262,6 @@
/** * Clear alpha and rgb values, thus disabling any alpha blending. - * - * @return CBGFX_* error codes */ void clear_blend(void);
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 1:
(2 comments)
Neat! Does it work (i.e. look right)?
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... PS1, Line 221: (UINT8_MAX - color) It took me embarrasingly long to figure out what you're doing here and why what I wrote in the bug was wrong. ^^ (I think the other option would have been `bg_color + ((fg_color - bg_color) * color / UINT8_MAX)`, but the two should be equivalent. Maybe one operation less, though?)
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... PS1, Line 222: int16_t This should be unnecessary (automatic integer promotion to `int`). If you want it for clarity, the correct type should be uint16_t (255 * 255 might exceed int16_t).
Hello Shelley Chen, build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44375
to look at the new patch set (#2).
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
libpayload: cbgfx: Add color mapping functionality
Similar to set_blend(), add set_color_map() for mapping background and foreground colors of a bitmap. Also add clear_color_map() for clearing the saved color mappings.
Note that when drawing a bitmap, the color mapping will be applied before blending.
BRANCH=puff BUG=b:146399181, b:162357639 TEST=emerge-puff libpayload
Change-Id: I640ff3e8455cd4aaa5a41d03a0183dff282648a5 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 62 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44375/2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 2:
(2 comments)
Patch Set 1:
(2 comments)
Neat! Does it work (i.e. look right)?
Yup. The buttons look exactly the same as before.
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... PS1, Line 221: (UINT8_MAX - color)
It took me embarrasingly long to figure out what you're doing here and why what I wrote in the bug w […]
Let's use
bg_color + (fg_color - bg_color) * color / UINT8_MAX
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... PS1, Line 222: int16_t
This should be unnecessary (automatic integer promotion to `int`). […]
Done. Learned something about integer promotion today.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
LGTM with one nit.
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... PS1, Line 95: = { : .alpha = 0, : .rgb.red = 0, : .rgb.green = 0, : .rgb.blue = 0, : }; Should we remove the initialization here to bring it in line with color_map?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44375/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/2/payloads/libpayload/drivers... PS2, Line 220: fg_color - bg_color nit: Actually, one more optimization might be to do this calculation in set_color_map() already, so it doesn't have to be redone for every pixel. Maybe rename the globals color_map.base and color_map.scale or something like that then since they wouldn't be storing the plain foreground color anymore (just the difference to the background).
Hello Shelley Chen, build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44375
to look at the new patch set (#3).
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
libpayload: cbgfx: Add color mapping functionality
Similar to set_blend(), add set_color_map() for mapping background and foreground colors of a bitmap. Also add clear_color_map() for clearing the saved color mappings.
Note that when drawing a bitmap, the color mapping will be applied before blending.
Also remove unnecessary initialization for static variable 'blend'.
BRANCH=puff BUG=b:146399181, b:162357639 TEST=emerge-puff libpayload
Change-Id: I640ff3e8455cd4aaa5a41d03a0183dff282648a5 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 75 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44375/3
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/1/payloads/libpayload/drivers... PS1, Line 95: = { : .alpha = 0, : .rgb.red = 0, : .rgb.green = 0, : .rgb.blue = 0, : };
Should we remove the initialization here to bring it in line with color_map?
Done
https://review.coreboot.org/c/coreboot/+/44375/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/2/payloads/libpayload/drivers... PS2, Line 220: fg_color - bg_color
nit: Actually, one more optimization might be to do this calculation in set_color_map() already, so […]
Done
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Let's let Julius do a final review.
https://review.coreboot.org/c/coreboot/+/44375/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/3/payloads/libpayload/drivers... PS3, Line 59: = { : .x = 0, : .y = 0, : }; Here's another one that we can remove while we're at it?
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44375/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/3/payloads/libpayload/drivers... PS3, Line 59: = { : .x = 0, : .y = 0, : };
Here's another one that we can remove while we're at it?
Since it's the zero vector, I think it's fine to explicitly initialize it?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44375/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/3/payloads/libpayload/drivers... PS3, Line 59: = { : .x = 0, : .y = 0, : };
Since it's the zero vector, I think it's fine to explicitly initialize it?
This is just a matter of taste, there is no hard rule... personally, I think it's perfectly fine to explicitly zero-initialize globals for clarity (it can help make the difference between "the initial value of this global doesn't matter" and "the initial value of this global must be 0 for good reason" a bit clearer).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44375/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/44375/3/payloads/libpayload/drivers... PS3, Line 59: = { : .x = 0, : .y = 0, : };
This is just a matter of taste, there is no hard rule... […]
Ack
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44375 )
Change subject: libpayload: cbgfx: Add color mapping functionality ......................................................................
libpayload: cbgfx: Add color mapping functionality
Similar to set_blend(), add set_color_map() for mapping background and foreground colors of a bitmap. Also add clear_color_map() for clearing the saved color mappings.
Note that when drawing a bitmap, the color mapping will be applied before blending.
Also remove unnecessary initialization for static variable 'blend'.
BRANCH=puff BUG=b:146399181, b:162357639 TEST=emerge-puff libpayload
Change-Id: I640ff3e8455cd4aaa5a41d03a0183dff282648a5 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/44375 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Joel Kitching kitching@google.com Reviewed-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 75 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Joel Kitching: Looks good to me, but someone else must approve
diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index b52bd99..21f520c 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -61,17 +61,53 @@ .y = 0, };
+struct color_transformation { + uint8_t base; + int16_t scale; +}; + +struct color_mapping { + struct color_transformation red; + struct color_transformation green; + struct color_transformation blue; + int enabled; +}; + +static struct color_mapping color_map; + +static inline void set_color_trans(struct color_transformation *trans, + uint8_t bg_color, uint8_t fg_color) +{ + trans->base = bg_color; + trans->scale = fg_color - bg_color; +} + +int set_color_map(const struct rgb_color *background, + const struct rgb_color *foreground) +{ + if (background == NULL || foreground == NULL) + return CBGFX_ERROR_INVALID_PARAMETER; + + set_color_trans(&color_map.red, background->red, foreground->red); + set_color_trans(&color_map.green, background->green, + foreground->green); + set_color_trans(&color_map.blue, background->blue, foreground->blue); + color_map.enabled = 1; + + return CBGFX_SUCCESS; +} + +void clear_color_map(void) +{ + color_map.enabled = 0; +} + struct blend_value { uint8_t alpha; struct rgb_color rgb; };
-static struct blend_value blend = { - .alpha = 0, - .rgb.red = 0, - .rgb.green = 0, - .rgb.blue = 0, -}; +static struct blend_value blend;
int set_blend(const struct rgb_color *rgb, uint8_t alpha) { @@ -185,6 +221,15 @@ return -1; }
+/* Helper function that applies color_map to the color. */ +static inline uint8_t apply_map(uint8_t color, + const struct color_transformation *trans) +{ + if (!color_map.enabled) + return color; + return trans->base + trans->scale * color / UINT8_MAX; +} + /* * Helper function that applies color and opacity from blend struct * into the color. @@ -203,13 +248,16 @@ { uint32_t color = 0;
- color |= (apply_blend(rgb->red, blend.rgb.red) + color |= (apply_blend(apply_map(rgb->red, &color_map.red), + blend.rgb.red) >> (8 - fbinfo->red_mask_size)) << fbinfo->red_mask_pos; - color |= (apply_blend(rgb->green, blend.rgb.green) + color |= (apply_blend(apply_map(rgb->green, &color_map.green), + blend.rgb.green) >> (8 - fbinfo->green_mask_size)) << fbinfo->green_mask_pos; - color |= (apply_blend(rgb->blue, blend.rgb.blue) + color |= (apply_blend(apply_map(rgb->blue, &color_map.blue), + blend.rgb.blue) >> (8 - fbinfo->blue_mask_size)) << fbinfo->blue_mask_pos; if (invert) diff --git a/payloads/libpayload/include/cbgfx.h b/payloads/libpayload/include/cbgfx.h index f2883b0..85b61a7 100644 --- a/payloads/libpayload/include/cbgfx.h +++ b/payloads/libpayload/include/cbgfx.h @@ -228,6 +228,24 @@ int get_bitmap_dimension(const void *bitmap, size_t sz, struct scale *dim_rel);
/** + * Setup color mappings of background and foreground colors. Black and white + * pixels will be mapped to the background and foreground colors, respectively. + * Call clear_color_map() to disabled color mapping. + * + * @param[in] background Background color. + * @param[in] foreground Foreground color. + * + * @return CBGFX_* error codes + */ +int set_color_map(const struct rgb_color *background, + const struct rgb_color *foreground); + +/** + * Clear color mappings. + */ +void clear_color_map(void); + +/** * Setup alpha and rgb values for alpha blending. When alpha is != 0, * this enables a translucent layer of color (defined by rgb) to be * blended at a given translucency (alpha) to all things drawn. Call @@ -244,8 +262,6 @@
/** * Clear alpha and rgb values, thus disabling any alpha blending. - * - * @return CBGFX_* error codes */ void clear_blend(void);
-
Joel Kitching (Code Review) -
Julius Werner (Code Review)
-
Yu-Ping Wu (Code Review)