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
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f97570f
Blend colors with alpha when pasting
ZachNagengast Oct 27, 2023
49fd211
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 27, 2023
76f758e
Merge branch 'main' into fix-alpha-for-overlapping-glyphs
radarhere Oct 27, 2023
bb0eff4
Update blending logic
ZachNagengast Nov 3, 2023
a7f805d
Merge branch 'fix-alpha-for-overlapping-glyphs' of ssh://github.com/Z…
ZachNagengast Nov 3, 2023
e1aaec3
Merge branch 'main' of ssh://github.com/python-pillow/Pillow into fix…
ZachNagengast Nov 3, 2023
b15b2d4
Update src/_imagingft.c
ZachNagengast Nov 7, 2023
fdecfca
Update gray glyph blending logic and tests
ZachNagengast Nov 7, 2023
8ecf2e9
Merge branch 'fix-alpha-for-overlapping-glyphs' of ssh://github.com/Z…
ZachNagengast Nov 7, 2023
11bea8f
Merge branch 'main' of ssh://github.com/python-pillow/Pillow into fix…
ZachNagengast Nov 7, 2023
d127600
Update test images for overlapping text
ZachNagengast Nov 7, 2023
0a33b30
Update caron_below_ttb test image
ZachNagengast Nov 12, 2023
29ca3fc
Update caron_below_ttb_lb test image
ZachNagengast Nov 12, 2023
f3b3442
add test for glyph alpha blending
nulano Nov 27, 2023
0cef9f2
fix drawing text alpha on RGBA image on big-endian platforms
nulano Nov 27, 2023
38992f6
Merge pull request #1 from nulano/fix-alpha-for-overlapping-glyphs
ZachNagengast Nov 27, 2023
9c60e85
Apply suggestions from code review
ZachNagengast Nov 27, 2023
78f78d2
Update src/_imagingft.c
ZachNagengast Nov 28, 2023
e800026
Update Tests/test_imagefont.py
ZachNagengast Dec 1, 2023
bd2977c
Update src/PIL/ImageDraw.py
ZachNagengast Dec 2, 2023
a6a612c
Merge branch 'main' into fix-alpha-for-overlapping-glyphs
radarhere Dec 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added Tests/fonts/CBDTTestFont.ttf
Binary file not shown.
Binary file added Tests/fonts/EBDTTestFont.ttf
Binary file not shown.
3 changes: 2 additions & 1 deletion Tests/fonts/LICENSE.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
NotoNastaliqUrdu-Regular.ttf and NotoSansSymbols-Regular.ttf, from https://github.com/googlei18n/noto-fonts
NotoSans-Regular.ttf, from https://www.google.com/get/noto/
NotoSansJP-Thin.otf, from https://www.google.com/get/noto/help/cjk/
NotoColorEmoji.ttf, from https://github.com/googlefonts/noto-emoji
AdobeVFPrototype.ttf, from https://github.com/adobe-fonts/adobe-variable-font-prototype
TINY5x3GX.ttf, from http://velvetyne.fr/fonts/tiny
ArefRuqaa-Regular.ttf, from https://github.com/google/fonts/tree/master/ofl/arefruqaa
Expand All @@ -25,3 +24,5 @@ FreeMono.ttf is licensed under GPLv3.
10x20-ISO8859-1.pcf, from https://packages.ubuntu.com/xenial/xfonts-base

"Public domain font. Share and enjoy."

CBDTTestFont.ttf and EBDTTestFont.ttf from https://github.com/nulano/font-tests are public domain.
Binary file removed Tests/fonts/NotoColorEmoji.ttf
Binary file not shown.
Binary file added Tests/images/bitmap_font_blend.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified Tests/images/bitmap_font_stroke_basic.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified Tests/images/bitmap_font_stroke_raqm.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added Tests/images/cbdt.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added Tests/images/cbdt_mask.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file removed Tests/images/cbdt_notocoloremoji.png
Binary file not shown.
Binary file removed Tests/images/cbdt_notocoloremoji_mask.png
Binary file not shown.
Binary file modified Tests/images/default_font_freetype.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified Tests/images/test_combine_caron_below_ttb.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified Tests/images/test_combine_caron_below_ttb_lb.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified Tests/images/test_combine_caron_ttb.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified Tests/images/test_combine_caron_ttb_lt.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
31 changes: 21 additions & 10 deletions Tests/test_imagefont.py
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,19 @@ def test_bitmap_font_stroke(layout_engine):
assert_image_similar_tofile(im, target, 0.03)


@pytest.mark.parametrize("embedded_color", (False, True))
def test_bitmap_blend(layout_engine, embedded_color):
font = ImageFont.truetype(
"Tests/fonts/EBDTTestFont.ttf", size=64, layout_engine=layout_engine
)

im = Image.new("RGBA", (128, 96), "white")
d = ImageDraw.Draw(im)
d.text((16, 16), "AA", font=font, embedded_color=embedded_color, fill="#8E2F52")
ZachNagengast marked this conversation as resolved.
Show resolved Hide resolved

assert_image_equal_tofile(im, "Tests/images/bitmap_font_blend.png")


def test_standard_embedded_color(layout_engine):
txt = "Hello World!"
ttf = ImageFont.truetype(FONT_PATH, 40, layout_engine=layout_engine)
Expand Down Expand Up @@ -894,15 +907,15 @@ def test_float_coord(layout_engine, fontmode):
def test_cbdt(layout_engine):
try:
font = ImageFont.truetype(
"Tests/fonts/NotoColorEmoji.ttf", size=109, layout_engine=layout_engine
"Tests/fonts/CBDTTestFont.ttf", size=64, layout_engine=layout_engine
)

im = Image.new("RGB", (150, 150), "white")
im = Image.new("RGB", (128, 96), "white")
d = ImageDraw.Draw(im)

d.text((10, 10), "\U0001f469", font=font, embedded_color=True)
d.text((16, 16), "AB", font=font, embedded_color=True)

assert_image_similar_tofile(im, "Tests/images/cbdt_notocoloremoji.png", 6.2)
assert_image_equal_tofile(im, "Tests/images/cbdt.png")
except OSError as e: # pragma: no cover
assert str(e) in ("unimplemented feature", "unknown file format")
pytest.skip("freetype compiled without libpng or CBDT support")
Expand All @@ -911,17 +924,15 @@ def test_cbdt(layout_engine):
def test_cbdt_mask(layout_engine):
try:
font = ImageFont.truetype(
"Tests/fonts/NotoColorEmoji.ttf", size=109, layout_engine=layout_engine
"Tests/fonts/CBDTTestFont.ttf", size=64, layout_engine=layout_engine
)

im = Image.new("RGB", (150, 150), "white")
im = Image.new("RGB", (128, 96), "white")
d = ImageDraw.Draw(im)

d.text((10, 10), "\U0001f469", "black", font=font)
d.text((16, 16), "AB", "green", font=font)

assert_image_similar_tofile(
im, "Tests/images/cbdt_notocoloremoji_mask.png", 6.2
)
assert_image_equal_tofile(im, "Tests/images/cbdt_mask.png")
except OSError as e: # pragma: no cover
assert str(e) in ("unimplemented feature", "unknown file format")
pytest.skip("freetype compiled without libpng or CBDT support")
Expand Down
4 changes: 3 additions & 1 deletion src/PIL/ImageDraw.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

import math
import numbers
import struct

Check warning on line 35 in src/PIL/ImageDraw.py

View check run for this annotation

Codecov / codecov/patch

src/PIL/ImageDraw.py#L35

Added line #L35 was not covered by tests

from . import Image, ImageColor

Expand Down Expand Up @@ -542,7 +543,8 @@
# 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.

color.fillband(3, ink_alpha)

Check warning on line 547 in src/PIL/ImageDraw.py

View check run for this annotation

Codecov / codecov/patch

src/PIL/ImageDraw.py#L546-L547

Added lines #L546 - L547 were not covered by tests
x, y = coord
self.im.paste(color, (x, y, x + mask.size[0], y + mask.size[1]), mask)
else:
Expand Down
59 changes: 40 additions & 19 deletions src/_imagingft.c
Original file line number Diff line number Diff line change
Expand Up @@ -1047,8 +1047,8 @@ font_render(FontObject *self, PyObject *args) {
if (yy >= 0 && yy < im->ysize) {
/* blend this glyph into the buffer */
int k;
unsigned char v;
unsigned char *target;
unsigned int tmp;
if (color) {
/* target[RGB] returns the color, target[A] returns the mask */
/* target bands get split again in ImageDraw.text */
Expand All @@ -1059,34 +1059,55 @@ font_render(FontObject *self, PyObject *args) {
if (color && bitmap.pixel_mode == FT_PIXEL_MODE_BGRA) {
/* paste color glyph */
for (k = x0; k < x1; k++) {
if (target[k * 4 + 3] < source[k * 4 + 3]) {
/* unpremultiply BGRa to RGBA */
target[k * 4 + 0] = CLIP8(
(255 * (int)source[k * 4 + 2]) / source[k * 4 + 3]);
target[k * 4 + 1] = CLIP8(
(255 * (int)source[k * 4 + 1]) / source[k * 4 + 3]);
target[k * 4 + 2] = CLIP8(
(255 * (int)source[k * 4 + 0]) / source[k * 4 + 3]);
target[k * 4 + 3] = source[k * 4 + 3];
unsigned int src_alpha = source[k * 4 + 3];

/* paste only if source has data */
if (src_alpha > 0) {
/* unpremultiply BGRa */
int src_red = CLIP8((255 * (int)source[k * 4 + 2]) / src_alpha);
int src_green = CLIP8((255 * (int)source[k * 4 + 1]) / src_alpha);
int src_blue = CLIP8((255 * (int)source[k * 4 + 0]) / src_alpha);

/* blend required if target has data */
if (target[k * 4 + 3] > 0) {
/* blend RGBA colors */
target[k * 4 + 0] = BLEND(src_alpha, target[k * 4 + 0], src_red, tmp);
target[k * 4 + 1] = BLEND(src_alpha, target[k * 4 + 1], src_green, tmp);
target[k * 4 + 2] = BLEND(src_alpha, target[k * 4 + 2], src_blue, tmp);
target[k * 4 + 3] = CLIP8(src_alpha + MULDIV255(target[k * 4 + 3], (255 - src_alpha), tmp));
} else {
/* paste unpremultiplied RGBA values */
target[k * 4 + 0] = src_red;
target[k * 4 + 1] = src_green;
target[k * 4 + 2] = src_blue;
target[k * 4 + 3] = src_alpha;
}
}
}
} else if (bitmap.pixel_mode == FT_PIXEL_MODE_GRAY) {
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;
unsigned 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++) {
v = source[k] * convert_scale;
if (target[k] < v) {
target[k] = v;
unsigned 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;
}
}
}
Expand Down
Loading