镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Keep negative values when filtering I mode images - #10466

Merged
radarhere merged 5 commits into
python-pillow:mainfrom
tsayin4:fix-filter-i-negative
Nov 9, 2026
Merged

radarhere merged 5 commits into
python-pillow:mainfrom
tsayin4:fix-filter-i-negative

Conversation

@tsayin4

@tsayin4 tsayin4 commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

clip32() in src/libImaging/Filter.c clamps its lower bound to zero, but mode I is a
32-bit signed integer mode, so every negative pixel that goes through the convolution
becomes 0:

>>> from PIL import Image, ImageFilter
>>> im = Image.new("I", (8, 8), -1000)
>>> im.filter(ImageFilter.SMOOTH).getpixel((4, 4))
0

BLUR, SHARPEN, DETAIL, EMBOSS and any ImageFilter.Kernel are affected the same
way; all of these have unity gain, so a flat image should come back unchanged (it does for
1000). The border pixels are copied with memcpy(), so only the interior is corrupted,
which leaves the filtered image self-inconsistent:

>>> im = Image.new("I", (4, 4))
>>> im.putdata([-8, -7, -6, -5, -4, -3, -2, -1, 0, 1, 2, 3, 4, 5, 6, 7])
>>> kernel = ImageFilter.Kernel((3, 3), [0, 0, 0, 0, 1, 0, 0, 0, 0], scale=1)
>>> list(im.filter(kernel).get_flattened_data())
[-8, -7, -6, -5, -4, 0, 0, -1, 0, 1, 2, 3, 4, 5, 6, 7]

The // Clamp the low bound branchlessly (maxss) comment is correct for clip8() and
clip16(), whose modes are unsigned, but clip32() only has ImagingFilter3x3() and
ImagingFilter5x5() as callers, both for IMAGING_TYPE_INT32.

Two things were needed. The lower bound now clamps to INT32_MIN instead of 0, and the
cast rounds towards negative infinity: offset already carries a +0.5 bias, and a plain
cast truncates towards zero, which would still turn -1 into 0. Positive values take the
same path as before, since (INT32)in is never greater than in for them, so this is
bit-for-bit unchanged above zero. I checked that over 0 ... 3e6 with a standalone program
comparing the old and new function: no differences.

I could not build Pillow locally, so CI is the check for the compiled result. The bug itself
was reproduced against current main (the 13.1.0.dev0 nightly, which already contains
c4888f336), and the arithmetic of the replacement was verified separately in C for the
range bounds, the 2**24 neighbourhood and negative values.

One related issue is left alone here: the accumulator is a float, so mode I values above
2**24 lose precision (1000000004 comes back as 1000000000). That is a separate change
and I did not want to mix a precision and performance decision into this fix.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@radarhere radarhere added the 🤖-assisted AI-assisted label Nov 8, 2026
@radarhere

Copy link
Copy Markdown
Member

Hi. You can see that our tests are failing with your suggested change.

Filtering a mode "I" image now keeps the negative values that the
unsigned I;16 modes clamp to zero, so test_consistency_i16 can no
longer compare the two results directly. Image.convert() applies the
same clamp, so convert the reference into the mode under test instead
of converting the result to "I".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tsayin4

tsayin4 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — that was test_consistency_i16, and it is a real consequence of the change rather than a flake.

That test compared the mode "I" result with the I;16 result directly. Now that the 32-bit path keeps negative values while the unsigned 16-bit path still clamps them to zero, the two can no longer be equal everywhere: the emboss kernel with scale=0.3 drives some pixels below zero.

I have pushed a fix that converts the reference into the mode under test instead of converting the result to "I". Image.convert() applies the same clamp as clip16 (test_16bit in Tests/test_image_convert.py already covers that), and both filter branches accumulate ss in the same order, so the comparison stays exact and still verifies that every I;16 variant agrees with "I" on every value I;16 can represent.

clip32() saturates at both ends of the range, but only the lower
bound was tested. Add 2**31 - 1 to the identity-kernel loop, so that
both explicit return statements are exercised.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tsayin4

tsayin4 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Tests are green now. The only remaining red is codecov/patch: clip32() saturates at both ends of the INT32 range, but the test only drove the lower one. I have pushed a commit that adds 2**31 - 1 to the identity-kernel loop, so both bounds are exercised.

@tsayin4

tsayin4 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

The remaining failure, Tests/test_image.py::TestImage::test_effect_noise on macOS / pypy3.11, is unrelated to this change. effect_noise is generated in Effects.c and returns an L image, while clip32() is only reachable from the mode-I branch of ImagingFilter3x3/5x5.

It is a pre-existing flake. The test asserts that the first five pixels of column 0 are not all equal, and with sigma=128 those pixels saturate at 255 often enough to collide. Over 20,000 runs of Image.effect_noise((100, 100), 128) on current main I measured 4 collisions — a rate of 0.02%, which across the job matrix works out to roughly 1 in 250 full CI runs.

I would rather not touch it from this PR. Happy to open a separate one making that test deterministic if you would like it.

@radarhere

radarhere commented Nov 9, 2026 •

Copy link
Copy Markdown
Member

The incorrect clipping to zero was added in #7108

You've undone some of the performance improvements from python-pillow/Pillow#9736, so I've put those back in.

@radarhere
radarhere force-pushed the fix-filter-i-negative branch from 125b895 to d37f0af Compare November 9, 2026 07:34
@radarhere
radarhere force-pushed the fix-filter-i-negative branch from d37f0af to b5817f7 Compare November 9, 2026 07:53
@radarhere
radarhere merged commit 9c3da10 into python-pillow:main Nov 9, 2026
63 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-assisted AI-assisted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants