Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
cbgfx: Add blend functions to calculate transparency
BUG=b:144969091,b:160839199 BRANCH=puff TEST=dut-control power_state:rec press ctrl-d Ensure background is dimmed when dialog pops up
Change-Id: I95468f27836d34ab80392727d726a69c09dc168e Signed-off-by: Shelley Chen shchen@google.com --- M 3rdparty/blobs M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 3 files changed, 84 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/43358/1
diff --git a/3rdparty/blobs b/3rdparty/blobs index bbe5d99..7ad2d22 160000 --- a/3rdparty/blobs +++ b/3rdparty/blobs @@ -1 +1 @@ -Subproject commit bbe5d99780d2d085e92d9bae2c0f7b6787419d72 +Subproject commit 7ad2d22452225a14c19b17570cb77920d8fc81a5 diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index bb8467b..3ed7390 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -61,6 +61,36 @@ .y = 0, };
+static struct blend_value bvalue = { + .alpha = 0, + .rgb.red = 0, + .rgb.green = 0, + .rgb.blue = 0, +}; + +int set_blend(const struct rgb_color *rgb, uint8_t alpha) +{ + if (rgb == NULL) + return CBGFX_ERROR_INVALID_PARAMETER; + + bvalue.rgb.red = rgb->red; + bvalue.rgb.green = rgb->green; + bvalue.rgb.blue = rgb->blue; + bvalue.alpha = alpha; + + return CBGFX_SUCCESS; +} + +int clear_blend(void) +{ + bvalue.rgb.red = 0; + bvalue.rgb.green = 0; + bvalue.rgb.blue = 0; + bvalue.alpha = 0; + + return CBGFX_SUCCESS; +} + static void add_vectors(struct vector *out, const struct vector *v1, const struct vector *v2) { @@ -139,11 +169,29 @@ uint8_t invert) { uint32_t color = 0; - color |= (rgb->red >> (8 - fbinfo->red_mask_size)) + struct rgb_color new_rgb; + + new_rgb.red = rgb->red; + new_rgb.green = rgb->green; + new_rgb.blue = rgb->blue; + if (bvalue.alpha > 0 && + (new_rgb.red != bvalue.rgb.red || + new_rgb.green != bvalue.rgb.green || + new_rgb.blue != bvalue.rgb.blue)) { + + new_rgb.red = (new_rgb.red * (UINT8_MAX - bvalue.alpha) + + bvalue.rgb.red * bvalue.alpha) >> 8; + new_rgb.green = (new_rgb.green * (UINT8_MAX - bvalue.alpha) + + bvalue.rgb.green * bvalue.alpha) >> 8; + new_rgb.blue = (new_rgb.blue * (UINT8_MAX - bvalue.alpha) + + bvalue.rgb.blue * bvalue.alpha) >> 8; + } + + color |= (new_rgb.red >> (8 - fbinfo->red_mask_size)) << fbinfo->red_mask_pos; - color |= (rgb->green >> (8 - fbinfo->green_mask_size)) + color |= (new_rgb.green >> (8 - fbinfo->green_mask_size)) << fbinfo->green_mask_pos; - color |= (rgb->blue >> (8 - fbinfo->blue_mask_size)) + color |= (new_rgb.blue >> (8 - fbinfo->blue_mask_size)) << fbinfo->blue_mask_pos; if (invert) color ^= 0xffffffff; diff --git a/payloads/libpayload/include/cbgfx.h b/payloads/libpayload/include/cbgfx.h index 869f272..313c308 100644 --- a/payloads/libpayload/include/cbgfx.h +++ b/payloads/libpayload/include/cbgfx.h @@ -89,6 +89,11 @@ uint8_t blue; };
+struct blend_value { + uint8_t alpha; + struct rgb_color rgb; +}; + /* * Resolution of scale parameters used to describe height, width, coordinate, * etc. relative to the canvas. For example, if it's 100, scales range from 0 to @@ -210,3 +215,30 @@ * in the original size are returned. */ int get_bitmap_dimension(const void *bitmap, size_t sz, struct scale *dim_rel); + +/** + * Set alpha values to setup transparency calculations + * + * @param[in] rgb Color for transparency + * @param[in] alpha Opacity of color, from 0-255 where + * 0 = completely transparent (no blending) + * 255 = completely opaque + * + * @return CBGFX_* error codes + */ +int set_blend(const struct rgb_color *rgb, uint8_t alpha); + +/** + * Clear alpha and rgb values, thus disabling dimming. + * + * @return CBGFX_* error codes + */ +int clear_blend(void); + +/** + * Constant for 60% opacity + * For reference: + * 255 = 100% opacity + * 0 = 0% opacity + */ +#define OPACITY_60 153
Shelley Chen has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
cbgfx: Add blend functions to calculate transparency
Up until now we have no way of adding transparency into our firmware screens. Adding set_blend() and clear_blend() functions to store values/flags to calculate this and applying these values into calculate_colors().
BUG=b:144969091,b:160839199 BRANCH=puff TEST=dut-control power_state:rec press ctrl-d Ensure background is dimmed when dialog pops up
Change-Id: I95468f27836d34ab80392727d726a69c09dc168e Signed-off-by: Shelley Chen shchen@google.com --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 83 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/43358/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 2:
(9 comments)
https://review.coreboot.org/c/coreboot/+/43358/1/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/c/coreboot/+/43358/1/3rdparty/blobs@1 PS1, Line 1: Subproject commit 7ad2d22452225a14c19b17570cb77920d8fc81a5 This looks like it doesn't belong here.
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... PS2, Line 64: bvalue nit: I'd maybe call this 'blend' (or 'blend_value' if you want) because 'bvalue' is a little unclear if it suddenly appears as a global half-way down the file. The 'b' could stand for anything.
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... PS2, Line 78: bvalue.rgb.blue = rgb->blue; nit: you can also use struct assignments (e.g. bvalue.rgb = *rgb) for these things if you want (don't have to though)
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... PS2, Line 84: int can be void
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... PS2, Line 182: UINT8_MAX Sorry, this was wrong from me... this should actually be 256 (or (1 << 8) or 0x100 or however you want to write it), not 255. Otherwise blending with always make it 1/256th darker than its supposed to be (because the >> 8 is an implicit divide by 256... in fact, my be better to write that as (... / 256) for clarity).
Could also maybe consider factoring this out into another function so you don't have to write it three times:
new_rgb.red = apply_blend(new_rgb.red);
(Maybe then you wouldn't even need the new_rgb at all you can just call the function when you OR in the color below directly.)
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/include... PS2, Line 225: 255 This would then be not really completely opaque, it would just be 255/256th opaque (which is fine because making it completely opaque would make no sense).
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/include... PS2, Line 241: * 255 = 100% opacity Same here
https://review.coreboot.org/c/coreboot/+/43358/1/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43358/1/payloads/libpayload/include... PS1, Line 92: struct blend_value { If this struct is internal for graphics.c it can be defined in there, doesn't need to be here. (Or just having two separate variables would be fine too, I think.)
https://review.coreboot.org/c/coreboot/+/43358/1/payloads/libpayload/include... PS1, Line 244: #define OPACITY_60 153 Kinda odd to have this here (and no other percentage)? If anything, I would define this in depthcharge. Or make this a little more flexible as
#define ALPHA(percentage) (256 * percentage / 100)
or something like that, then it would make more sense to have it here.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43358
to look at the new patch set (#3).
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
cbgfx: Add blend functions to calculate transparency
Up until now we have no way of adding transparency into our firmware screens. Adding set_blend() and clear_blend() functions to store values/flags to calculate this and applying these values into calculate_colors().
BUG=b:144969091,b:160839199 BRANCH=puff TEST=dut-control power_state:rec press ctrl-d Ensure background is dimmed when dialog pops up
Change-Id: I95468f27836d34ab80392727d726a69c09dc168e Signed-off-by: Shelley Chen shchen@google.com --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 85 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/43358/3
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/coreboot/+/43358/1/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/c/coreboot/+/43358/1/3rdparty/blobs@1 PS1, Line 1: Subproject commit 7ad2d22452225a14c19b17570cb77920d8fc81a5
This looks like it doesn't belong here.
Yeah, saw this and corrected this in patch 2.
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... PS2, Line 64: bvalue
nit: I'd maybe call this 'blend' (or 'blend_value' if you want) because 'bvalue' is a little unclear […]
Done
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... PS2, Line 84: int
can be void
Done
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/drivers... PS2, Line 182: UINT8_MAX
Sorry, this was wrong from me... […]
Done
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/include... PS2, Line 225: 255
This would then be not really completely opaque, it would just be 255/256th opaque (which is fine be […]
Done
https://review.coreboot.org/c/coreboot/+/43358/2/payloads/libpayload/include... PS2, Line 241: * 255 = 100% opacity
Same here
Done
https://review.coreboot.org/c/coreboot/+/43358/1/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43358/1/payloads/libpayload/include... PS1, Line 92: struct blend_value {
If this struct is internal for graphics.c it can be defined in there, doesn't need to be here. […]
moved to graphics.c
https://review.coreboot.org/c/coreboot/+/43358/1/payloads/libpayload/include... PS1, Line 244: #define OPACITY_60 153
Kinda odd to have this here (and no other percentage)? If anything, I would define this in depthchar […]
Done
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... PS3, Line 92: blend.alpha = 0; Move this to the first line for consistency?
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... PS3, Line 188: need_blend ? : apply_blend(rgb->red, blend.rgb.red) : rgb->red Do we need to enclose this with parentheses?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43358/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43358/3//COMMIT_MSG@10 PS3, Line 10: Adding Add
https://review.coreboot.org/c/coreboot/+/43358/3//COMMIT_MSG@11 PS3, Line 11: applying apply
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 3: Code-Review+2
Hello Ting Shen, build bot (Jenkins), Joel Kitching, Frans Hendriks, Hsuan-ting Chen, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43358
to look at the new patch set (#4).
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
cbgfx: Add blend functions to calculate transparency
Up until now we have no way of adding transparency into our firmware screens. Add set_blend() and clear_blend() functions to store values/flags to calculate this and apply these values into calculate_colors().
BUG=b:144969091,b:160839199 BRANCH=puff TEST=dut-control power_state:rec press ctrl-d Ensure background is dimmed when dialog pops up
Change-Id: I95468f27836d34ab80392727d726a69c09dc168e Signed-off-by: Shelley Chen shchen@google.com --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 85 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/43358/4
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43358/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43358/3//COMMIT_MSG@10 PS3, Line 10: Adding
Add
Done
https://review.coreboot.org/c/coreboot/+/43358/3//COMMIT_MSG@11 PS3, Line 11: applying
apply
Done
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... PS3, Line 92: blend.alpha = 0;
Move this to the first line for consistency?
Done
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... PS3, Line 188: need_blend ? : apply_blend(rgb->red, blend.rgb.red) : rgb->red
Do we need to enclose this with parentheses?
Functionality, no. But if you think that it helps with readability, then let's put it in.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43358/4/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/4/payloads/libpayload/drivers... PS4, Line 188: ? Could we move the conditional logic into apply_blend?
if (blend.alpha == 0 || color == blend_color) return color; return (...) / 256;
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... PS3, Line 188: need_blend ? : apply_blend(rgb->red, blend.rgb.red) : rgb->red
Functionality, no. But if you think that it helps with readability, then let's put it in.
Doesn't ">>" has higher precedence than "a?b:c", so that
a ? x : y >> shift
will be interpreted as
a ? x : (y >> shift)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... PS3, Line 188: need_blend ? : apply_blend(rgb->red, blend.rgb.red) : rgb->red
Doesn't ">>" has higher precedence than "a?b:c", so that […]
https://en.cppreference.com/w/c/language/operator_precedence
Sure, not needed, but since this is a complex statement and is broken down in a rather awkward way, I'd prefer the parentheses to be left in to ease human parsing.
Hello Ting Shen, build bot (Jenkins), Joel Kitching, Frans Hendriks, Hsuan-ting Chen, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43358
to look at the new patch set (#5).
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
cbgfx: Add blend functions to calculate transparency
Up until now we have no way of adding transparency into our firmware screens. Add set_blend() and clear_blend() functions to store values/flags to calculate this and apply these values into calculate_colors().
BUG=b:144969091,b:160839199 BRANCH=puff TEST=dut-control power_state:rec press ctrl-d Ensure background is dimmed when dialog pops up
Change-Id: I95468f27836d34ab80392727d726a69c09dc168e Signed-off-by: Shelley Chen shchen@google.com --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 81 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/43358/5
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/3/payloads/libpayload/drivers... PS3, Line 188: need_blend ? : apply_blend(rgb->red, blend.rgb.red) : rgb->red
https://en.cppreference.com/w/c/language/operator_precedence […]
This logic has been moved to apply_blend() so this isn't an issue anymore.
https://review.coreboot.org/c/coreboot/+/43358/4/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/4/payloads/libpayload/drivers... PS4, Line 188: ?
Could we move the conditional logic into apply_blend? […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 5: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 5:
(4 comments)
LGTM except for style and documentation nits
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/drivers... PS5, Line 189: nit: should technically indent one space less here, if you're trying to match parentheses
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... PS5, Line 215: * Set alpha values to setup transparency calculations This isn't a super good explanation of what it really does (and also no relation to the explanation for clean_blend(), which says "disabling dimming" while this says "transparency calculations"). Something here should explain the whole process clearly, that after you call this all future drawing calls (boxes and images) will be alpha blended with the supplied values, until you turn it off again.
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... PS5, Line 220: * 256 = completely opaque Same here (see below)... 255 is the largest legal value.
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... PS5, Line 236: * 256 = 100% opacity Except that that one is illegal and will be treated as 0 because the parameter to set_blend() can only fit a uint8_t. ;)
Actually, probably better to put a MAX() in the macro just in case.
Hello Ting Shen, build bot (Jenkins), Joel Kitching, Frans Hendriks, Hsuan-ting Chen, Julius Werner, Angel Pons, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43358
to look at the new patch set (#6).
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
cbgfx: Add blend functions to calculate transparency
Up until now we have no way of adding transparency into our firmware screens. Add set_blend() and clear_blend() functions to store alpha value and rgb values to calculate alpha blending in calculate_colors().
BUG=b:144969091,b:160839199 BRANCH=puff TEST=dut-control power_state:rec press ctrl-d Ensure background is dimmed when dialog pops up
Change-Id: I95468f27836d34ab80392727d726a69c09dc168e Signed-off-by: Shelley Chen shchen@google.com --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 84 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/43358/6
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/drivers... PS5, Line 189:
nit: should technically indent one space less here, if you're trying to match parentheses
Done
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... PS5, Line 215: * Set alpha values to setup transparency calculations
This isn't a super good explanation of what it really does (and also no relation to the explanation […]
Ack. Updated description.
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... PS5, Line 220: * 256 = completely opaque
Same here (see below)... 255 is the largest legal value.
Done
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... PS5, Line 236: * 256 = 100% opacity
Except that that one is illegal and will be treated as 0 because the parameter to set_blend() can on […]
Done. Added MIN so we don't go above 255.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 6: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... PS5, Line 236: * 256 = 100% opacity
Done. Added MIN so we don't go above 255.
When will I ever learn not to mix those up... ^^
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
cbgfx: Add blend functions to calculate transparency
Up until now we have no way of adding transparency into our firmware screens. Add set_blend() and clear_blend() functions to store alpha value and rgb values to calculate alpha blending in calculate_colors().
BUG=b:144969091,b:160839199 BRANCH=puff TEST=dut-control power_state:rec press ctrl-d Ensure background is dimmed when dialog pops up
Change-Id: I95468f27836d34ab80392727d726a69c09dc168e Signed-off-by: Shelley Chen shchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43358 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Yu-Ping Wu yupingso@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, 84 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Yu-Ping Wu: 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 bb8467b..54d3dfa 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -61,6 +61,37 @@ .y = 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, +}; + +int set_blend(const struct rgb_color *rgb, uint8_t alpha) +{ + if (rgb == NULL) + return CBGFX_ERROR_INVALID_PARAMETER; + + blend.alpha = alpha; + blend.rgb = *rgb; + + return CBGFX_SUCCESS; +} + +void clear_blend(void) +{ + blend.alpha = 0; + blend.rgb.red = 0; + blend.rgb.green = 0; + blend.rgb.blue = 0; +} + static void add_vectors(struct vector *out, const struct vector *v1, const struct vector *v2) { @@ -135,16 +166,33 @@ return -1; }
+/* + * Helper function that applies color and opacity from blend struct + * into the color. + */ +static inline uint8_t apply_blend(uint8_t color, uint8_t blend_color) +{ + if (blend.alpha == 0 || color == blend_color) + return color; + + return (color * (256 - blend.alpha) + + blend_color * blend.alpha) / 256; +} + static inline uint32_t calculate_color(const struct rgb_color *rgb, uint8_t invert) { uint32_t color = 0; - color |= (rgb->red >> (8 - fbinfo->red_mask_size)) - << fbinfo->red_mask_pos; - color |= (rgb->green >> (8 - fbinfo->green_mask_size)) - << fbinfo->green_mask_pos; - color |= (rgb->blue >> (8 - fbinfo->blue_mask_size)) - << fbinfo->blue_mask_pos; + + color |= (apply_blend(rgb->red, blend.rgb.red) + >> (8 - fbinfo->red_mask_size)) + << fbinfo->red_mask_pos; + color |= (apply_blend(rgb->green, blend.rgb.green) + >> (8 - fbinfo->green_mask_size)) + << fbinfo->green_mask_pos; + color |= (apply_blend(rgb->blue, blend.rgb.blue) + >> (8 - fbinfo->blue_mask_size)) + << fbinfo->blue_mask_pos; if (invert) color ^= 0xffffffff; return color; diff --git a/payloads/libpayload/include/cbgfx.h b/payloads/libpayload/include/cbgfx.h index 869f272..84e76f2 100644 --- a/payloads/libpayload/include/cbgfx.h +++ b/payloads/libpayload/include/cbgfx.h @@ -210,3 +210,33 @@ * in the original size are returned. */ int get_bitmap_dimension(const void *bitmap, size_t sz, struct scale *dim_rel); + +/** + * 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 + * clear_blend() to disable alpha blending. + * + * @param[in] rgb Color for transparency + * @param[in] alpha Opacity of color, from 0-255 where + * 0 = completely transparent (no blending) + * 255 = max alpha argument + * + * @return CBGFX_* error codes + */ +int set_blend(const struct rgb_color *rgb, uint8_t alpha); + +/** + * Clear alpha and rgb values, thus disabling any alpha blending. + * + * @return CBGFX_* error codes + */ +void clear_blend(void); + +/** + * For calculating Alpha value from % opacity + * For reference: + * 255 = max alpha argument + * 0 = min alpha argument, 0% opacity + */ +#define ALPHA(percentage) MIN(255, (256 * percentage / 100))
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43358 )
Change subject: cbgfx: Add blend functions to calculate transparency ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43358/5/payloads/libpayload/include... PS5, Line 236: * 256 = 100% opacity
When will I ever learn not to mix those up... […]
I understand your pain 😄
-
Angel Pons (Code Review) -
Frans Hendriks (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Shelley Chen (Code Review)
-
Yu-Ping Wu (Code Review)