Fix incorrect color blending for overlapping glyphs

[ZachNagengast]

Fixes the previous code that just took the color with the highest alpha value.

Changes proposed in this pull request:

• With this change, colors are now properly blended based on their respective alpha values.

This occurs only for overlapping glyphs in emoji zwj sequences as 🫱🏼‍🫲🏾 and 👩🏼‍❤️‍👨🏽

Example before:

Notice the artifacts on the left side of the heart that overlaps, and the background color coming through between the shaking hands.

Here is the corrected output:

The heart now retains its alpha entirely, and the hands fit together much closer.

Here is some code you can use to test it out:

from PIL import Image, ImageDraw, ImageFont

font = ImageFont.truetype(
    "/System/Library/Fonts/Apple Color Emoji.ttc", size=160
)

im = Image.new("RGBA", (160*3, 160), "blue")
d = ImageDraw.Draw(im)

d.text((0, 0), "👩🏼‍❤️‍👨🏽🫱🏼‍🫲🏾🏴‍☠️", fill="white", embedded_color=True, font=font)

im.show()

The issue requires a glyph that is originally separate and put together on the fly. This probably saves space because of all the possible combinations.

"u1F468.3.u1F48B.L"

"u1F468.3.u1F48B.R"

and

"u1FAF1.2.L"

"u1FAF2.2.R"

[homm]

You shouldn’t use float in pixels computations. You can learn from the other code or wait for help with this.

[ZachNagengast]

Fair enough, just sharing the issue and what worked for me. I'll take a second look but open to edits or suggestions.

[homm]

If I understand this correctly (currently from the phone), this is alpha blending. We already have implemented alpha blending as a separate function. It would be great to reuse it, which cold be not so easy. Or at least you can copy parts of code

[radarhere]

Interestingly, running your code on my machine, I don't get the disfigured heart with main.

You can see that our test suite fails with these changes. That is because

Pillow/Tests/test_imagefont.py

Lines 966 to 975 in d3fd173

	font = ImageFont.truetype(
	"Tests/fonts/BungeeColor-Regular_colr_Windows.ttf",
	size=64,
	layout_engine=layout_engine,
	)
	im = Image.new("RGB", (300, 75), "white")
	d = ImageDraw.Draw(im)
	d.text((15, 5), "Bungee", font=font, embedded_color=True)

no longer gives https://github.com/python-pillow/Pillow/blob/main/Tests/images/colr_bungee.png

but instead

with a darker outline. Did you have any thoughts on this difference?

[nulano]

The BGRA and grayscale bitmaps should be rendered the same way. If this is to be merged, the gray glyphs should be alpha blended as well.

[ZachNagengast]

@radarhere I believe the blond haired version doesn't show as much of the alpha issues because the majority of the hair portion has higher alpha, I can see it's still there by the jagged edge at the border, and also the heart is supposed to overlap the left side when rendered. You might see it with this version of the emoji 👩🏻‍❤️‍👨🏼. I can also see a lot of blue coming through the hands so I believe it's there in your picture. I'm not sure about the dark outline but @nulano's explanation makes sense, will adjust that.

[ZachNagengast]

Ok I was able to resolve the dark outline by bringing back in the unpremultiply step, and also updated the blending logic to use one of the macros that is already in the codebase.

I also started down the path of adding a test, but the system apple color emoji font is 189mb which seems a bit large to add. I may be able to pare it down by creating a much smaller set that only includes the emojis needed to test, or investigate some other font that has similar overlapping glyphs.

[ZachNagengast]

@nulano Regarding the grey scale alpha blending, I've implemented a method do it in a similar way as the RGBA, but it's resulting in an image that is ~25% bigger although it looks visually the same. This is because the new code results in an image that has an alpha channel, whereas the test image doesn't.

- if (color) {
-     unsigned char *ink = (unsigned char *)&foreground_ink;
-     for (k = x0; k < x1; k++) {
-         v = source[k] * convert_scale;
-         if (target[k * 4 + 3] < v) {
-             target[k * 4 + 0] = ink[0];
-             target[k * 4 + 1] = ink[1];
-             target[k * 4 + 2] = ink[2];
-             target[k * 4 + 3] = v;
-         }
-     }
- } else {
-     for (k = x0; k < x1; k++) {
-         v = source[k] * convert_scale;
-         if (target[k] < v) {
-             target[k] = v;
-         }
-     }
- }
+ if (color) {
+     unsigned char *ink = (unsigned char *)&foreground_ink;
+     for (k = x0; k < x1; k++) {
+         int src_alpha = source[k] * convert_scale;
+         if (src_alpha > 0) {
+             if (target[k * 4 + 3] > 0) {
+                 target[k * 4 + 0] = BLEND(src_alpha, target[k * 4 + 0], ink[0], tmp);
+                 target[k * 4 + 1] = BLEND(src_alpha, target[k * 4 + 1], ink[1], tmp);
+                 target[k * 4 + 2] = BLEND(src_alpha, target[k * 4 + 2], ink[2], tmp);
+                 target[k * 4 + 3] = CLIP8(src_alpha + MULDIV255(target[k * 4 + 3], (255 - src_alpha), tmp));
+             } else {
+                 target[k * 4 + 0] = ink[0];
+                 target[k * 4 + 1] = ink[1];
+                 target[k * 4 + 2] = ink[2];
+                 target[k * 4 + 3] = src_alpha;
+             }
+         }
+     }
+ } else {
+     for (k = x0; k < x1; k++) {
+         int src_alpha = source[k] * convert_scale;
+         if (src_alpha > 0) {
+             target[k] = target[k] > 0 ? CLIP8(src_alpha + MULDIV255(target[k], (255 - src_alpha), tmp)) : src_alpha;
+         }
+     }
+ }

outputs look the same

I'll look further into where the alpha channel is coming from but lmk if you see anything obvious here.

[ZachNagengast]

I also started down the path of adding a test, but the system apple color emoji font is 189mb which seems a bit large to add. I may be able to pare it down by creating a much smaller set that only includes the emojis needed to test, or investigate some other font that has similar overlapping glyphs.

@radarhere do you have any opinion on this? Possible options would be 1. Include the font with git lfs 2. Create a new font specifically for testing 3. Some other method to test the code (i.e. with a different font, etc)

All tests are now passing though.

[nulano]

the heart is supposed to overlap the left side when rendered

It's not clear to me how the rendering code is supposed to know which glyph should be on top when there are overlapping glyphs. AFAIK they are usually just ordered left-to-right before rendering (to account for mixed LTR/RTL text), but perhaps it is possible that intentionally overlapping glyphs are reordered by the font. This is why my original implementation just used the higher value instead of doing proper blending. However, I can see how that is undesirable in this case. I've also run your test with the Windows font Segoe UI Emoji and it has a similar issue for the hands emoji.

---

I also started down the path of adding a test, but the system apple color emoji font is 189mb which seems a bit large to add. [...] Possible options would be

1. Include the font [Apple Color Emoji] with git lfs

No, don't include that font. It's way too large and I doubt its license allows for that.

There is also https://github.com/python-pillow/test-images which is used for extra test files; I'm not sure if it could go there instead.

(Although Segoe UI Emoji is only 11MB, the Windows Fonts FAQ explicitly states that it may not be redistributed and is not even available for sale.)

2. Create a new font specifically for testing

Removing unused glyphs from a test font to minimize its size would also be fine; I'm not sure which is easier.

3. Some other method to test the code (i.e. with a different font, etc)

A few ideas:

• Look for another font with this issue. I was not able to find a suitable font, but didn't look for too long.

  An old version (see the discussion starting at Add support for CBDT and COLR fonts #4955 (comment)) of Noto Color Emoji is already included in the test files. It does not contain the glyphs from your test, but a more recent version might. However, the latest version seems to be in SVG format which is not yet supported.

  Even better might be a COLR/CPAL font as they tend to be smaller than CBDT and SBIX fonts.

• Raqm has somewhat recently added support for letter and word spacing: Letter and word spacing HOST-Oman/libraqm#171
  I'm not sure how useful it is in practice and I have not yet seen any requests for adding support for letter spacing in Pillow, but it could make it easier to test this (by adding negative spacing to force glyphs to overlap).

---

Regarding the grey scale alpha blending, I've implemented a method do it in a similar way as the RGBA, but it's resulting in an image that is ~25% bigger although it looks visually the same. This is because the new code results in an image that has an alpha channel, whereas the test image doesn't.

I don't see how that diff could possibly affect the output image format. What error do you get when you run the tests? There are separate checks for the image mode and similarity.

The diff does look correct to me, although I haven't tested it thoroughly.

[nulano]

I think these could be PREBLEND if you use the premultiplied source values. I'm not entirely certain whether that would require CLIP8 as well.

[ZachNagengast]

I tried PREBLEND with CLIP8

and without CLIP8

It seems like they're getting some artifacts in the areas with alpha, what do you think?

Here is just BLEND for reference:

[ZachNagengast]

Thanks for the detailed response! Here's the tests that are failing with the greyscale changes:

FAILED Tests/test_imagefont.py::test_bitmap_font_stroke[Layout.BASIC] - AssertionError:  average pixel value difference 0.3207 > epsilon 0.0300
FAILED Tests/test_imagefont.py::test_bitmap_font_stroke[Layout.RAQM] - AssertionError:  average pixel value difference 0.4011 > epsilon 0.0300
FAILED Tests/test_imagefont.py::test_default_font - AssertionError: got different content

The info for each image in the test test_bitmap_font_stroke
My changes:

Test image:

Similar slight difference for test_default_font, except that one is similar enough to pass the similarity check, then when checking byte values it fails. The metadata seems to suggest it has an alpha channel somehow, so I'll try to get to the bottom of that.

Next steps here, I will take a look into so other fonts already included to see if there is a case that can be used for these tests. If that doesn't work, I'll attempt your suggestion by forcing the overlap with some spacing adjustments.

[nulano]

AssertionError: average pixel value difference 0.3207 > epsilon 0.0300

Yeah, that is not a mode issue, but a change in output due to different blending of the edges.
You should be able to just replace the images by adding an im.save(target) call before the check and visually comparing they are correct.

Similar slight difference for test_default_font, except that one is similar enough to pass the similarity check, then when checking byte values it fails. The metadata seems to suggest it has an alpha channel somehow, so I'll try to get to the bottom of that.

I only see the byte check, no similarity check. Since it failed on the byte check, the mode must be correct as it is tested first. Again, it should be fine to replace the test file.

If that doesn't work, I'll attempt your suggestion by forcing the overlap with some spacing adjustments.

Note that this isn't yet implemented in Pillow, so it's not trivial to add to a test. I somehow lost that part while editing my previous comment.

[ZachNagengast]

Ok great! Just replaced the images and appears to be passing again 👍

[ZachNagengast]

Took a while but looks like everything is finally passing again except for the codecov check.

[hugovk]

Is it possible to add tests to cover the missed if and two else blocks, so we know they're working correctly and to guard against regressions?

[nulano]

I've opened ZachNagengast#1 to add tests for this PR.

I created two fonts specifically to test this PR at https://github.com/nulano/font-tests:

• a CBDT color bitmap font to test blending color glyphs when rendering in color mode,
• and a grayscale bitmap font to test blending glyphs when rendering in either mode.

Although these should provide full code coverage, I was unable to fully test one case - blending a grayscale glyph on top of a color glyph when rendering in color mode.

The CBDT font can replace NotoColorEmoji as the CBDT test font to save 5MB in the sdist.

Here are the added test images:

test	before PR	after PR
test_bitmap_blend
test_cbdt
test_cbdt_mask

[ZachNagengast]

Fantastic solution @nulano. Custom fonts seem like the way to go for these specific tests! Will monitor PR for when it's ready to merge.

[nulano]

I've updated ZachNagengast#1 to fix a big-endian bug from #4955 I found with the new test test_bitmap_blend.

[ZachNagengast]

Thanks for the reviews and additional changes, looks like everything is passing 🎉

[radarhere]

Suggested change

	ink_alpha = struct.pack("=i", ink)[3]
	ink_alpha = struct.pack("i", ink)[3]

Searching through Pillow's existing code, the tradition has been to use native mode implicitly rather than explicitly.

Pillow/src/PIL/SpiderImagePlugin.py

Line 259 in 76446ee

return [struct.pack("f", v) for v in hdr]

Pillow/src/PIL/SgiImagePlugin.py

Lines 184 to 188 in 76446ee

	fp.write(struct.pack("4s", b"")) # dummy
	fp.write(struct.pack("79s", img_name)) # truncates to 79 chars
	fp.write(struct.pack("s", b"")) # force null byte after img_name
	fp.write(struct.pack(">l", colormap))
	fp.write(struct.pack("404s", b"")) # dummy

[nulano]

The default is @ mode, which is not equivalent to =.
From https://docs.python.org/3/library/struct.html#byte-order-size-and-alignment:

Note the difference between '@' and '=': both use native byte order, but the size and alignment of the latter is standardized.

The s and f formats are the same for both @ and =.
l is platform dependent with @, but is used with > in the quoted code above.

i is platform dependent, so changing = to @ can change the behavior here.
However, in the C code, I see that int is assumed to be at least 4 bytes, so it is likely the same on all currently supported platforms even if it is not the same in general.

[ZachNagengast]

@radarhere what do you think? Happy to adjust as needed

[radarhere]

I hadn't read the linked documentation in enough detailed to notice the distinction before I made my suggestion. Thanks nulano for catching that.

I don't have strong feelings, I was just trying to follow a convention. If there is a preference to future proof this code by using =, that's fine with me.

[ZachNagengast]

Makes sense, @nulano let us know if you want to be sure to include this

[nulano]

Looking more specifically at the _draw_ink function which provides the ink value, the ink is created in an INT32 type (which corresponds to a =i format), but is cast to a C int (corresponding to @i or i format) before returning it (for future reference, the cast should probably be a long instead):

Pillow/src/_imaging.c

Lines 2848 to 2861 in 80d0ed4

	static PyObject *
	_draw_ink(ImagingDrawObject *self, PyObject *args) {
	INT32 ink = 0;
	PyObject *color;
	if (!PyArg_ParseTuple(args, "O", &color)) {
	return NULL;
	}
	if (!getink(color, self->image->image, (char *)&ink)) {
	return NULL;
	}
	return PyLong_FromLong((int)ink);
	}

When it is later received in font_render, it is stored in a long long and converted to unsigned int before being read as bytes:

Pillow/src/_imagingft.c

Lines 805 to 806 in b51dcc0

	PY_LONG_LONG foreground_ink_long = 0;
	unsigned int foreground_ink;

Pillow/src/_imagingft.c

Line 842 in b51dcc0

foreground_ink = foreground_ink_long;

Pillow/src/_imagingft.c

Line 847 in b51dcc0

FT_Byte *ink = (FT_Byte *)&foreground_ink;

So if int is smaller than 4 bytes, the getink function will discard data, and if int is larger than 4 bytes, the ink will be read as black on a big-endian platform. Either way, there will be an issue before this code is reached.

Therefore, I think it's fine to leave the format specifier as i and leave this thread "unresolved" to document it.

[ZachNagengast]

Thanks @nulano, just marked this thread as unresolved.

[ZachNagengast]

Any tasks still pending for this?

[radarhere]

I just updated the title, since the FT_PIXEL_MODE_GRAY path has been changed as well as the FT_PIXEL_MODE_BGRA path.