From 465ea33a66e08b008a10104bf4b9cc895ec2a43d Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Wed, 16 Sep 2026 08:55:00 +0300 Subject: [PATCH 1/3] Apply hoist/restrict optimizations to ImagingGetHistogram --- src/libImaging/Histo.c | 84 +++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 37 deletions(-) diff --git a/src/libImaging/Histo.c b/src/libImaging/Histo.c index 7af60003511..7c8741e6c7c 100644 --- a/src/libImaging/Histo.c +++ b/src/libImaging/Histo.c @@ -59,7 +59,6 @@ ImagingHistogramNew(Imaging im) { ImagingHistogram ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { ImagingSectionCookie cookie; - int x, y, i; ImagingHistogram h; INT32 imin, imax; FLOAT32 fmin, fmax, scale; @@ -68,9 +67,10 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { return ImagingError_ModeError(); } + int xsize = im->xsize, ysize = im->ysize; if (imMask) { /* Validate mask */ - if (im->xsize != imMask->xsize || im->ysize != imMask->ysize) { + if (xsize != imMask->xsize || ysize != imMask->ysize) { return ImagingError_Mismatch(); } if (imMask->mode != IMAGING_MODE_1 && imMask->mode != IMAGING_MODE_L) { @@ -83,14 +83,22 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { return NULL; } + // restrict safe: im and imMask are both read-only here + // (they may even be the same image). + // histogram is a fresh allocation from just above. + + long *restrict histogram = h->histogram; + if (imMask) { /* mask */ if (im->image8) { ImagingSectionEnter(&cookie); - for (y = 0; y < im->ysize; y++) { - for (x = 0; x < im->xsize; x++) { - if (imMask->image8[y][x] != 0) { - h->histogram[im->image8[y][x]]++; + for (int y = 0; y < ysize; y++) { + UINT8 *restrict in = im->image8[y]; + UINT8 *restrict mask = imMask->image8[y]; + for (int x = 0; x < xsize; x++) { + if (mask[x] != 0) { + histogram[in[x]]++; } } } @@ -101,14 +109,15 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { return ImagingError_ModeError(); } ImagingSectionEnter(&cookie); - for (y = 0; y < im->ysize; y++) { - UINT8 *in = (UINT8 *)im->image32[y]; - for (x = 0; x < im->xsize; x++) { - if (imMask->image8[y][x] != 0) { - h->histogram[(*in++)]++; - h->histogram[(*in++) + 256]++; - h->histogram[(*in++) + 512]++; - h->histogram[(*in++) + 768]++; + for (int y = 0; y < ysize; y++) { + UINT8 *restrict in = (UINT8 *)im->image32[y]; + UINT8 *restrict mask = imMask->image8[y]; + for (int x = 0; x < xsize; x++) { + if (mask[x] != 0) { + histogram[(*in++)]++; + histogram[(*in++) + 256]++; + histogram[(*in++) + 512]++; + histogram[(*in++) + 768]++; } else { in += 4; } @@ -120,9 +129,10 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { /* mask not given; process pixels in image */ if (im->image8) { ImagingSectionEnter(&cookie); - for (y = 0; y < im->ysize; y++) { - for (x = 0; x < im->xsize; x++) { - h->histogram[im->image8[y][x]]++; + for (int y = 0; y < ysize; y++) { + UINT8 *restrict in = im->image8[y]; + for (int x = 0; x < xsize; x++) { + histogram[in[x]]++; } } ImagingSectionLeave(&cookie); @@ -130,16 +140,16 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { switch (im->type) { case IMAGING_TYPE_UINT8: ImagingSectionEnter(&cookie); - for (y = 0; y < im->ysize; y++) { - UINT8 *in = (UINT8 *)im->image[y]; - for (x = 0; x < im->xsize; x++, in += 4) { - h->histogram[*in]++; + for (int y = 0; y < ysize; y++) { + UINT8 *restrict in = (UINT8 *)im->image[y]; + for (int x = 0; x < xsize; x++, in += 4) { + histogram[*in]++; if (im->bands == 2) { - h->histogram[*(in + 3) + 256]++; + histogram[*(in + 3) + 256]++; } else { - h->histogram[*(in + 1) + 256]++; - h->histogram[*(in + 2) + 512]++; - h->histogram[*(in + 3) + 768]++; + histogram[*(in + 1) + 256]++; + histogram[*(in + 2) + 512]++; + histogram[*(in + 3) + 768]++; } } } @@ -150,7 +160,7 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { ImagingHistogramDelete(h); return ImagingError_ValueError("min/max not given"); } - if (!im->xsize || !im->ysize) { + if (!xsize || !ysize) { break; } memcpy(&imin, minmax, sizeof(imin)); @@ -160,12 +170,12 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { } ImagingSectionEnter(&cookie); scale = 255.0F / (imax - imin); - for (y = 0; y < im->ysize; y++) { - INT32 *in = im->image32[y]; - for (x = 0; x < im->xsize; x++) { - i = (int)(((*in++) - imin) * scale); + for (int y = 0; y < ysize; y++) { + INT32 *restrict in = im->image32[y]; + for (int x = 0; x < xsize; x++) { + int i = (int)(((*in++) - imin) * scale); if (i >= 0 && i < 256) { - h->histogram[i]++; + histogram[i]++; } } } @@ -176,7 +186,7 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { ImagingHistogramDelete(h); return ImagingError_ValueError("min/max not given"); } - if (!im->xsize || !im->ysize) { + if (!xsize || !ysize) { break; } memcpy(&fmin, minmax, sizeof(fmin)); @@ -186,12 +196,12 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { } ImagingSectionEnter(&cookie); scale = 255.0F / (fmax - fmin); - for (y = 0; y < im->ysize; y++) { - FLOAT32 *in = (FLOAT32 *)im->image32[y]; - for (x = 0; x < im->xsize; x++) { - i = (int)(((*in++) - fmin) * scale); + for (int y = 0; y < ysize; y++) { + FLOAT32 *restrict in = (FLOAT32 *)im->image32[y]; + for (int x = 0; x < xsize; x++) { + int i = (int)(((*in++) - fmin) * scale); if (i >= 0 && i < 256) { - h->histogram[i]++; + histogram[i]++; } } } From 6b67f5528245f337cd300b696d106cb9258e2afc Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Wed, 16 Sep 2026 09:08:42 +0300 Subject: [PATCH 2/3] Fix masked histograms for 2-band images + add test coverage Same fix as dc7d646db03bb34abd493a79ec2ceb78ec778265 but applied in the other code path too. --- Tests/test_image_histogram.py | 47 +++++++++++++++++++++++++++++++++++ docs/releasenotes/13.0.0.rst | 7 ++++++ src/libImaging/Histo.c | 16 ++++++------ 3 files changed, 63 insertions(+), 7 deletions(-) diff --git a/Tests/test_image_histogram.py b/Tests/test_image_histogram.py index 436eb78a26e..7a7fc64733c 100644 --- a/Tests/test_image_histogram.py +++ b/Tests/test_image_histogram.py @@ -1,5 +1,9 @@ from __future__ import annotations +import pytest + +from PIL import Image + from .helper import hopper @@ -20,3 +24,46 @@ def histogram(mode: str) -> tuple[int, int, int]: assert histogram("RGBA") == (1024, 0, 16384) assert histogram("CMYK") == (1024, 0, 16384) assert histogram("YCbCr") == (768, 0, 1908) + + +def test_histogram_masked() -> None: + mask = Image.new("1", (128, 128), 0) + crop_box = (0, 0, 128, 64) + mask.paste(1, crop_box) + + def histogram(mode: str) -> tuple[int, int, int]: + im = hopper(mode) + h = im.histogram(mask) + assert h == im.crop(crop_box).histogram() + return len(h), min(h), max(h) + + assert histogram("1") == (256, 0, 5024) + assert histogram("L") == (256, 0, 178) + assert histogram("LA") == (512, 0, 8192) + assert histogram("La") == (512, 0, 8192) + assert histogram("P") == (256, 0, 797) + assert histogram("PA") == (512, 0, 8192) + assert histogram("RGB") == (768, 1, 174) + assert histogram("RGBA") == (1024, 0, 8192) + assert histogram("CMYK") == (1024, 0, 8192) + assert histogram("YCbCr") == (768, 0, 1009) + + +@pytest.mark.parametrize("mode", ("I", "F")) +def test_histogram_masked_unsupported_mode(mode: str) -> None: + mask = Image.new("1", (128, 128), 1) + with pytest.raises(ValueError, match="image has wrong mode"): + hopper(mode).histogram(mask) + + +def test_histogram_mask_size_mismatch() -> None: + mask = Image.new("1", (64, 64), 1) + with pytest.raises(ValueError, match="images do not match"): + hopper("L").histogram(mask) + + +@pytest.mark.parametrize("mode", ("P", "I", "F", "RGB", "RGBA")) +def test_histogram_bad_mask_mode(mode: str) -> None: + mask = Image.new(mode, (128, 128)) + with pytest.raises(ValueError, match="bad transparency mask"): + hopper("L").histogram(mask) diff --git a/docs/releasenotes/13.0.0.rst b/docs/releasenotes/13.0.0.rst index 74603803e46..0b02cf4b77e 100644 --- a/docs/releasenotes/13.0.0.rst +++ b/docs/releasenotes/13.0.0.rst @@ -159,3 +159,10 @@ Fixed ImageStat variance rounding Fixed a floating-point rounding error that may have produced a negative variance in :py:attr:`~PIL.ImageStat.Stat.var`. + +Fixed masked histograms for LA, La and PA images +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +When :py:meth:`~PIL.Image.Image.histogram` was given a mask, the second band of a +two-band image was read from the wrong byte, so LA, La and PA images reported a +copy of the first band rather than the alpha channel. diff --git a/src/libImaging/Histo.c b/src/libImaging/Histo.c index 7c8741e6c7c..c5ec7edd62d 100644 --- a/src/libImaging/Histo.c +++ b/src/libImaging/Histo.c @@ -112,14 +112,16 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { for (int y = 0; y < ysize; y++) { UINT8 *restrict in = (UINT8 *)im->image32[y]; UINT8 *restrict mask = imMask->image8[y]; - for (int x = 0; x < xsize; x++) { + for (int x = 0; x < xsize; x++, in += 4) { if (mask[x] != 0) { - histogram[(*in++)]++; - histogram[(*in++) + 256]++; - histogram[(*in++) + 512]++; - histogram[(*in++) + 768]++; - } else { - in += 4; + histogram[*in]++; + if (im->bands == 2) { + histogram[*(in + 3) + 256]++; + } else { + histogram[*(in + 1) + 256]++; + histogram[*(in + 2) + 512]++; + histogram[*(in + 3) + 768]++; + } } } } From 349c6caac871c7c6a55d2d3bca1120d975ac1566 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Thu, 17 Sep 2026 08:29:53 +0300 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com> --- Tests/test_image_histogram.py | 8 +++++--- docs/releasenotes/13.0.0.rst | 8 ++++---- src/libImaging/Histo.c | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Tests/test_image_histogram.py b/Tests/test_image_histogram.py index 7a7fc64733c..c3f3c391601 100644 --- a/Tests/test_image_histogram.py +++ b/Tests/test_image_histogram.py @@ -51,18 +51,20 @@ def histogram(mode: str) -> tuple[int, int, int]: @pytest.mark.parametrize("mode", ("I", "F")) def test_histogram_masked_unsupported_mode(mode: str) -> None: - mask = Image.new("1", (128, 128), 1) + mask = Image.new("1", (128, 128)) with pytest.raises(ValueError, match="image has wrong mode"): hopper(mode).histogram(mask) def test_histogram_mask_size_mismatch() -> None: - mask = Image.new("1", (64, 64), 1) + mask = Image.new("1", (1, 1)) with pytest.raises(ValueError, match="images do not match"): hopper("L").histogram(mask) -@pytest.mark.parametrize("mode", ("P", "I", "F", "RGB", "RGBA")) +@pytest.mark.parametrize( + "mode", ("LA", "La", "I", "F", "P", "PA", "RGB", "RGBA", "CMYK", "YCbCr") +) def test_histogram_bad_mask_mode(mode: str) -> None: mask = Image.new(mode, (128, 128)) with pytest.raises(ValueError, match="bad transparency mask"): diff --git a/docs/releasenotes/13.0.0.rst b/docs/releasenotes/13.0.0.rst index 0b02cf4b77e..90a120041ee 100644 --- a/docs/releasenotes/13.0.0.rst +++ b/docs/releasenotes/13.0.0.rst @@ -160,9 +160,9 @@ Fixed ImageStat variance rounding Fixed a floating-point rounding error that may have produced a negative variance in :py:attr:`~PIL.ImageStat.Stat.var`. -Fixed masked histograms for LA, La and PA images +Fixed histograms for masked LA, La and PA images ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -When :py:meth:`~PIL.Image.Image.histogram` was given a mask, the second band of a -two-band image was read from the wrong byte, so LA, La and PA images reported a -copy of the first band rather than the alpha channel. +When :py:meth:`~PIL.Image.Image.histogram` was given a mask for a two-band image (LA, +La or PA), the second band was incorrectly derived from the first band of the input +image, rather than the alpha band. diff --git a/src/libImaging/Histo.c b/src/libImaging/Histo.c index c5ec7edd62d..3ae43532c41 100644 --- a/src/libImaging/Histo.c +++ b/src/libImaging/Histo.c @@ -85,7 +85,7 @@ ImagingGetHistogram(Imaging im, Imaging imMask, void *minmax) { // restrict safe: im and imMask are both read-only here // (they may even be the same image). - // histogram is a fresh allocation from just above. + // histogram is a fresh allocation from ImagingHistogramNew long *restrict histogram = h->histogram;