-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix incorrect color blending for overlapping glyphs #7497
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
Changes from 16 commits
f97570f
49fd211
76f758e
bb0eff4
a7f805d
e1aaec3
b15b2d4
fdecfca
8ecf2e9
11bea8f
d127600
0a33b30
29ca3fc
f3b3442
0cef9f2
38992f6
9c60e85
78f78d2
e800026
bd2977c
a6a612c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import math | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import numbers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import struct | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from . import Image, ImageColor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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):
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:
Lines 805 to 806 in b51dcc0
| PY_LONG_LONG foreground_ink_long = 0; | |
| unsigned int foreground_ink; |
Line 842 in b51dcc0
| foreground_ink = foreground_ink_long; |
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.
There was a problem hiding this comment.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1049,6 +1049,7 @@ font_render(FontObject *self, PyObject *args) { | |
| int k; | ||
| unsigned char v; | ||
ZachNagengast marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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 */ | ||
|
|
@@ -1059,34 +1060,58 @@ 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]; | ||
| int src_alpha = source[k * 4 + 3]; | ||
ZachNagengast marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| /* paste only if source has data */ | ||
| if (src_alpha > 0) { | ||
| /* unpremultiply RGBA */ | ||
| int src_red = CLIP8((255 * (int)source[k * 4 + 0]) / src_alpha); | ||
| int src_grn = CLIP8((255 * (int)source[k * 4 + 1]) / src_alpha); | ||
| int src_blu = CLIP8((255 * (int)source[k * 4 + 2]) / src_alpha); | ||
ZachNagengast marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| /* blend required if target has data */ | ||
| if (target[k * 4 + 3] > 0) { | ||
| /* 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); | ||
|
||
|
|
||
| /* blend alpha */ | ||
| int out_alpha = CLIP8(src_alpha + MULDIV255(target[k * 4 + 3], (255 - src_alpha), tmp)); | ||
| target[k * 4 + 3] = out_alpha; | ||
ZachNagengast marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } else { | ||
| /* paste unpremultiplied RGBA values */ | ||
| target[k * 4 + 0] = src_blu; | ||
| target[k * 4 + 1] = src_grn; | ||
| target[k * 4 + 2] = src_red; | ||
| target[k * 4 + 3] = src_alpha; | ||
ZachNagengast marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| } | ||
| } 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; | ||
| int src_alpha = source[k] * convert_scale; | ||
ZachNagengast marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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; | ||
| int src_alpha = source[k] * convert_scale; | ||
ZachNagengast marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (src_alpha > 0) { | ||
| target[k] = target[k] > 0 ? CLIP8(src_alpha + MULDIV255(target[k], (255 - src_alpha), tmp)) : src_alpha; | ||
| } | ||
| } | ||
| } | ||
|
|
||



Uh oh!
There was an error while loading. Please reload this page.