Skip to content

Support ImageFilter.BuiltinFilter for I;16* images #8438

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.

Sign up for GitHub

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

Merged
radarhere merged 2 commits into from
Oct 10, 2024
Merged

Support ImageFilter.BuiltinFilter for I;16* images #8438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07be6fc
Corrected check for BuiltinFilter
radarhere Oct 4, 2024
418ae7c
Support BuiltinFilter for I;16* images
radarhere Oct 4, 2024
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
25 changes: 19 additions & 6 deletions Tests/test_image_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,25 @@
ImageFilter.UnsharpMask(10),
),
)
@pytest.mark.parametrize("mode", ("L", "I", "RGB", "CMYK"))
def test_sanity(filter_to_apply: ImageFilter.Filter, mode: str) -> None:
@pytest.mark.parametrize(
"mode", ("L", "I", "I;16", "I;16L", "I;16B", "I;16N", "RGB", "CMYK")
)
def test_sanity(
filter_to_apply: ImageFilter.Filter | type[ImageFilter.Filter], mode: str
) -> None:
im = hopper(mode)
if mode != "I" or isinstance(filter_to_apply, ImageFilter.BuiltinFilter):
if mode[0] != "I" or (
callable(filter_to_apply)
and issubclass(filter_to_apply, ImageFilter.BuiltinFilter)
):
Copy link
Member Author

@radarhere radarhere Oct 4, 2024

Choose a reason for hiding this comment

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

The isinstance condition, added in #7108, was never true - each BuiltinFilter run of this test uses a BuiltinFilter class, not an instance. So this corrects it.

out = im.filter(filter_to_apply)
assert out.mode == im.mode
assert out.size == im.size


@pytest.mark.parametrize("mode", ("L", "I", "RGB", "CMYK"))
@pytest.mark.parametrize(
"mode", ("L", "I", "I;16", "I;16L", "I;16B", "I;16N", "RGB", "CMYK")
)
def test_sanity_error(mode: str) -> None:
im = hopper(mode)
with pytest.raises(TypeError):
Expand Down Expand Up @@ -145,7 +154,9 @@ def test_kernel_not_enough_coefficients() -> None:
ImageFilter.Kernel((3, 3), (0, 0))


@pytest.mark.parametrize("mode", ("L", "LA", "I", "RGB", "CMYK"))
@pytest.mark.parametrize(
"mode", ("L", "LA", "I", "I;16", "I;16L", "I;16B", "I;16N", "RGB", "CMYK")
)
def test_consistency_3x3(mode: str) -> None:
with Image.open("Tests/images/hopper.bmp") as source:
with Image.open("Tests/images/hopper_emboss.bmp") as reference:
Expand All @@ -161,7 +172,9 @@ def test_consistency_3x3(mode: str) -> None:
assert_image_equal(source.filter(kernel), reference)


@pytest.mark.parametrize("mode", ("L", "LA", "I", "RGB", "CMYK"))
@pytest.mark.parametrize(
"mode", ("L", "LA", "I", "I;16", "I;16L", "I;16B", "I;16N", "RGB", "CMYK")
)
def test_consistency_5x5(mode: str) -> None:
with Image.open("Tests/images/hopper.bmp") as source:
with Image.open("Tests/images/hopper_emboss_more.bmp") as reference:
Expand Down
106 changes: 92 additions & 14 deletions src/libImaging/Filter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

#include "Imaging.h"

#define ROUND_UP(f) ((int)((f) >= 0.0 ? (f) + 0.5F : (f) - 0.5F))
Copy link
Member

@hugovk hugovk Oct 6, 2024

Choose a reason for hiding this comment

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

Can we make this a function instead?

Copy link
Member Author

@radarhere radarhere Oct 6, 2024

Choose a reason for hiding this comment

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

Any particular reason?

This was copying

Pillow/src/libImaging/Reduce.c

Line 5 in 27c1bb2

#define ROUND_UP(f) ((int)((f) >= 0.0 ? (f) + 0.5F : (f) - 0.5F))
and

Pillow/src/libImaging/Resample.c

Line 5 in 27c1bb2

#define ROUND_UP(f) ((int)((f) >= 0.0 ? (f) + 0.5F : (f) - 0.5F))

If there's a reason to prefer functions, then maybe they should all be changed.

I'd suggest a common function for all three, except that this uses a float and those other two use double.

Copy link
Member

@hugovk hugovk Oct 8, 2024

Choose a reason for hiding this comment

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

Functions are generally safer than macros, which need extra care around parentheses and don't get extra checking. But if this is a copy/paste of existing ones then we should be fine.


static inline UINT8
clip8(float in) {
if (in <= 0.0) {
Expand Down Expand Up @@ -105,6 +107,22 @@ ImagingExpand(Imaging imIn, int xmargin, int ymargin) {
return imOut;
}

float
kernel_i16(int size, UINT8 *in0, int x, const float *kernel, int bigendian) {
int i;
float result = 0;
int half_size = (size - 1) / 2;
for (i = 0; i < size; i++) {
int x1 = x + i - half_size;
result += _i2f(
in0[x1 * 2 + (bigendian ? 1 : 0)] +
(in0[x1 * 2 + (bigendian ? 0 : 1)] >> 8)
) *
kernel[i];
}
return result;
}

void
ImagingFilter3x3(Imaging imOut, Imaging im, const float *kernel, float offset) {
#define KERNEL1x3(in0, x, kernel, d) \
Expand Down Expand Up @@ -135,21 +153,48 @@ ImagingFilter3x3(Imaging imOut, Imaging im, const float *kernel, float offset) {
out[x] = in0[x];
}
} else {
int bigendian = 0;
if (im->type == IMAGING_TYPE_SPECIAL) {
if (strcmp(im->mode, "I;16B") == 0
#ifdef WORDS_BIGENDIAN
|| strcmp(im->mode, "I;16N") == 0
#endif
) {
bigendian = 1;
}
}
for (y = 1; y < im->ysize - 1; y++) {
UINT8 *in_1 = (UINT8 *)im->image[y - 1];
UINT8 *in0 = (UINT8 *)im->image[y];
UINT8 *in1 = (UINT8 *)im->image[y + 1];
UINT8 *out = (UINT8 *)imOut->image[y];

out[0] = in0[0];
if (im->type == IMAGING_TYPE_SPECIAL) {
out[1] = in0[1];
}
for (x = 1; x < im->xsize - 1; x++) {
float ss = offset;
ss += KERNEL1x3(in1, x, &kernel[0], 1);
ss += KERNEL1x3(in0, x, &kernel[3], 1);
ss += KERNEL1x3(in_1, x, &kernel[6], 1);
out[x] = clip8(ss);
if (im->type == IMAGING_TYPE_SPECIAL) {
ss += kernel_i16(3, in1, x, &kernel[0], bigendian);
ss += kernel_i16(3, in0, x, &kernel[3], bigendian);
ss += kernel_i16(3, in_1, x, &kernel[6], bigendian);
int ss_int = ROUND_UP(ss);
out[x * 2 + (bigendian ? 1 : 0)] = clip8(ss_int % 256);
out[x * 2 + (bigendian ? 0 : 1)] = clip8(ss_int >> 8);
} else {
ss += KERNEL1x3(in1, x, &kernel[0], 1);
ss += KERNEL1x3(in0, x, &kernel[3], 1);
ss += KERNEL1x3(in_1, x, &kernel[6], 1);
out[x] = clip8(ss);
}
}
if (im->type == IMAGING_TYPE_SPECIAL) {
out[x * 2] = in0[x * 2];
out[x * 2 + 1] = in0[x * 2 + 1];
} else {
out[x] = in0[x];
}
out[x] = in0[x];
}
}
} else {
Expand Down Expand Up @@ -261,6 +306,16 @@ ImagingFilter5x5(Imaging imOut, Imaging im, const float *kernel, float offset) {
out[x + 1] = in0[x + 1];
}
} else {
int bigendian = 0;
if (im->type == IMAGING_TYPE_SPECIAL) {
if (strcmp(im->mode, "I;16B") == 0
#ifdef WORDS_BIGENDIAN
|| strcmp(im->mode, "I;16N") == 0
#endif
) {
bigendian = 1;
}
}
for (y = 2; y < im->ysize - 2; y++) {
UINT8 *in_2 = (UINT8 *)im->image[y - 2];
UINT8 *in_1 = (UINT8 *)im->image[y - 1];
Expand All @@ -271,17 +326,39 @@ ImagingFilter5x5(Imaging imOut, Imaging im, const float *kernel, float offset) {

out[0] = in0[0];
out[1] = in0[1];
if (im->type == IMAGING_TYPE_SPECIAL) {
out[2] = in0[2];
out[3] = in0[3];
}
for (x = 2; x < im->xsize - 2; x++) {
float ss = offset;
ss += KERNEL1x5(in2, x, &kernel[0], 1);
ss += KERNEL1x5(in1, x, &kernel[5], 1);
ss += KERNEL1x5(in0, x, &kernel[10], 1);
ss += KERNEL1x5(in_1, x, &kernel[15], 1);
ss += KERNEL1x5(in_2, x, &kernel[20], 1);
out[x] = clip8(ss);
if (im->type == IMAGING_TYPE_SPECIAL) {
ss += kernel_i16(5, in2, x, &kernel[0], bigendian);
ss += kernel_i16(5, in1, x, &kernel[5], bigendian);
ss += kernel_i16(5, in0, x, &kernel[10], bigendian);
ss += kernel_i16(5, in_1, x, &kernel[15], bigendian);
ss += kernel_i16(5, in_2, x, &kernel[20], bigendian);
int ss_int = ROUND_UP(ss);
out[x * 2 + (bigendian ? 1 : 0)] = clip8(ss_int % 256);
out[x * 2 + (bigendian ? 0 : 1)] = clip8(ss_int >> 8);
} else {
ss += KERNEL1x5(in2, x, &kernel[0], 1);
ss += KERNEL1x5(in1, x, &kernel[5], 1);
ss += KERNEL1x5(in0, x, &kernel[10], 1);
ss += KERNEL1x5(in_1, x, &kernel[15], 1);
ss += KERNEL1x5(in_2, x, &kernel[20], 1);
out[x] = clip8(ss);
}
}
if (im->type == IMAGING_TYPE_SPECIAL) {
out[x * 2 + 0] = in0[x * 2 + 0];
out[x * 2 + 1] = in0[x * 2 + 1];
out[x * 2 + 2] = in0[x * 2 + 2];
out[x * 2 + 3] = in0[x * 2 + 3];
} else {
out[x + 0] = in0[x + 0];
out[x + 1] = in0[x + 1];
}
out[x + 0] = in0[x + 0];
out[x + 1] = in0[x + 1];
}
}
} else {
Expand Down Expand Up @@ -383,7 +460,8 @@ ImagingFilter(Imaging im, int xsize, int ysize, const FLOAT32 *kernel, FLOAT32 o
Imaging imOut;
ImagingSectionCookie cookie;

if (im->type != IMAGING_TYPE_UINT8 && im->type != IMAGING_TYPE_INT32) {
if (im->type == IMAGING_TYPE_FLOAT32 ||
(im->type == IMAGING_TYPE_SPECIAL && im->bands != 1)) {
return (Imaging)ImagingError_ModeError();
}

Expand Down