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.