diff --git a/Tests/test_image_histogram.py b/Tests/test_image_histogram.py index 436eb78a26e..c3f3c391601 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,48 @@ 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)) + with pytest.raises(ValueError, match="image has wrong mode"): + hopper(mode).histogram(mask) + + +def test_histogram_mask_size_mismatch() -> None: + mask = Image.new("1", (1, 1)) + with pytest.raises(ValueError, match="images do not match"): + hopper("L").histogram(mask) + + +@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"): + hopper("L").histogram(mask) diff --git a/docs/releasenotes/13.0.0.rst b/docs/releasenotes/13.0.0.rst index 74603803e46..90a120041ee 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 histograms for masked LA, La and PA images +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +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 7af60003511..3ae43532c41 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 ImagingHistogramNew + + 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,16 +109,19 @@ 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]++; - } else { - in += 4; + 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++, in += 4) { + if (mask[x] != 0) { + histogram[*in]++; + if (im->bands == 2) { + histogram[*(in + 3) + 256]++; + } else { + histogram[*(in + 1) + 256]++; + histogram[*(in + 2) + 512]++; + histogram[*(in + 3) + 768]++; + } } } } @@ -120,9 +131,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 +142,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 +162,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 +172,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 +188,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 +198,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]++; } } }