Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect color blending for overlapping glyphs #7497

Merged

Conversation

ZachNagengast
Copy link
Contributor

@ZachNagengast ZachNagengast commented Oct 27, 2023

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:
sequence-wrong-

sequence-wrong

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:

sequence-corrected
sequence-corrected-red

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"
glyph-u1F468 3 u1F48B L
"u1F468.3.u1F48B.R"
glyph-u1F468 3 u1F48B R

and

"u1FAF1.2.L"
glyph-u1FAF1 2 L
"u1FAF2.2.R"
glyph-u1FAF2 2 R

ZachNagengast and others added 2 commits October 26, 2023 21:40
- Previously took the highest alpha as threshold
@homm
Copy link
Member

homm commented Oct 27, 2023

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

@ZachNagengast
Copy link
Contributor Author

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

@homm
Copy link
Member

homm commented Oct 27, 2023

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
Copy link
Member

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

out

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

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?

Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/_imagingft.c Outdated Show resolved Hide resolved
@ZachNagengast
Copy link
Contributor Author

@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.

- Unpremultiply properly
- No longer uses floats
…achNagengast/Pillow into fix-alpha-for-overlapping-glyphs
@ZachNagengast
Copy link
Contributor Author

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.
image

image
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 ZachNagengast requested a review from nulano November 3, 2023 23:12
@ZachNagengast
Copy link
Contributor Author

@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
tmpsbepe46j
bitmap_font_stroke_raqm

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

@ZachNagengast
Copy link
Contributor Author

ZachNagengast commented Nov 7, 2023

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.

Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

  1. 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.

  1. 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.

src/_imagingft.c Outdated
/* blend colors to BGRa */
target[k * 4 + 0] = BLEND(src_alpha, target[k * 4 + 0], src_blu, tmp);
target[k * 4 + 1] = BLEND(src_alpha, target[k * 4 + 1], src_grn, tmp);
target[k * 4 + 2] = BLEND(src_alpha, target[k * 4 + 2], src_red, tmp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried PREBLEND with CLIP8
image

and without CLIP8
image

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

Here is just BLEND for reference:
image

src/_imagingft.c Outdated Show resolved Hide resolved
@ZachNagengast
Copy link
Contributor Author

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:
image
Test image:
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.

Co-authored-by: Ondrej Baranovič <nulano@nulano.eu>
@nulano
Copy link
Contributor

nulano commented Nov 7, 2023

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
Copy link
Contributor Author

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

@hugovk hugovk changed the title Fixes Incorrect Color Blending for Overlapping Glyphs in BGRA Mode Fix incorrect color blending for overlapping glyphs in BGRA mode Nov 7, 2023
@ZachNagengast
Copy link
Contributor Author

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

@hugovk
Copy link
Member

hugovk commented Nov 13, 2023

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
Copy link
Contributor

nulano commented Nov 27, 2023

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 bitmap_font_blend_before.png bitmap_font_blend_after.png
test_cbdt cbdt_before.png cbdt.png
test_cbdt_mask cbdt_mask_before.png cbdt_mask.png

@ZachNagengast
Copy link
Contributor Author

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
Copy link
Contributor

nulano commented Nov 27, 2023

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

src/_imagingft.c Outdated Show resolved Hide resolved
src/_imagingft.c Outdated Show resolved Hide resolved
src/_imagingft.c Outdated Show resolved Hide resolved
src/_imagingft.c Outdated Show resolved Hide resolved
src/_imagingft.c Outdated Show resolved Hide resolved
src/_imagingft.c Outdated Show resolved Hide resolved
src/_imagingft.c Outdated Show resolved Hide resolved
Co-authored-by: Ondrej Baranovič <nulano@nulano.eu>
src/_imagingft.c Outdated Show resolved Hide resolved
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@ZachNagengast
Copy link
Contributor Author

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

Tests/test_imagefont.py Outdated Show resolved Hide resolved
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@@ -542,7 +543,8 @@ def draw_text(ink, stroke_width=0, stroke_offset=None):
# font.getmask2(mode="RGBA") returns color in RGB bands and mask in A
# extract mask and set text alpha
color, mask = mask, mask.getband(3)
color.fillband(3, (ink >> 24) & 0xFF)
ink_alpha = struct.pack("=i", ink)[3]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
foreground_ink = foreground_ink_long;
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nulano, just marked this thread as unresolved.

ZachNagengast and others added 2 commits December 1, 2023 22:56
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@ZachNagengast
Copy link
Contributor Author

Any tasks still pending for this?

@radarhere radarhere changed the title Fix incorrect color blending for overlapping glyphs in BGRA mode Fix incorrect color blending for overlapping glyphs Dec 24, 2023
@radarhere
Copy link
Member

radarhere commented Dec 24, 2023

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

@radarhere radarhere merged commit 6768d3a into python-pillow:main Dec 24, 2023
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants