Skip to content
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

Jump to bottom

Improved I mode conversion #3838

Merged
merged 5 commits into from
Jun 5, 2019
Merged

Improved I mode conversion #3838

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a035b8
Improved I mode conversion
radarhere May 8, 2019
ddc83fd
Updated ImageMath tests for scaled I mode conversion
radarhere May 7, 2019
2787b9b
Added tests
radarhere May 8, 2019
64eff66
Updated pixel range documentation [ci skip]
radarhere May 9, 2019
a258b8d
Apply suggestions from code review [ci skip]
radarhere May 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Binary file added Tests/images/hopper_I.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion Tests/test_file_png.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ def test_save_greyscale_transparency(self):
for mode, num_transparent in {
"1": 1994,
"L": 559,
"I": 559,
"I": 4096,
}.items():
in_file = "Tests/images/"+mode.lower()+"_trns.png"
im = Image.open(in_file)
Expand Down
10 changes: 10 additions & 0 deletions Tests/test_image_convert.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def test_default(self):

def _test_float_conversion(self, im):
orig = im.getpixel((5, 5))
if im.mode[0] == 'I':
orig //= 256
converted = im.convert('F').getpixel((5, 5))
self.assertEqual(orig, converted)

Expand Down Expand Up @@ -231,3 +233,11 @@ def test_matrix_identity(self):
# Assert
# No change
self.assert_image_equal(converted_im, im)

def test_i_to_l(self):
im = Image.open("Tests/images/hopper_I.png").convert("L")
self.assert_image_similar(hopper("L"), im, 0.02)

def test_i_to_rgb(self):
im = Image.open("Tests/images/hopper_I.png").convert("RGB")
self.assert_image_similar(hopper("L").convert("RGB"), im, 0.05)
2 changes: 1 addition & 1 deletion Tests/test_image_getcolors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ def getcolors(mode, limit=None):

self.assertEqual(getcolors("1"), 2)
self.assertEqual(getcolors("L"), 255)
self.assertEqual(getcolors("I"), 255)
self.assertEqual(getcolors("F"), 255)
self.assertEqual(getcolors("P"), 90) # fixed palette
self.assertIsNone(getcolors("RGB"))
self.assertIsNone(getcolors("RGBA"))
self.assertIsNone(getcolors("CMYK"))
self.assertIsNone(getcolors("YCbCr"))
self.assertIsNone(getcolors("I"))

self.assertIsNone(getcolors("L", 128))
self.assertEqual(getcolors("L", 1024), 255)
Expand Down
2 changes: 1 addition & 1 deletion Tests/test_image_getdata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def getdata(mode):

self.assertEqual(getdata("1"), (0, 960, 960))
self.assertEqual(getdata("L"), (16, 960, 960))
self.assertEqual(getdata("I"), (16, 960, 960))
self.assertEqual(getdata("I"), (4313, 960, 960))
self.assertEqual(getdata("F"), (16.0, 960, 960))
self.assertEqual(getdata("RGB"), ((11, 13, 52), 960, 960))
self.assertEqual(getdata("RGBA"), ((11, 13, 52, 255), 960, 960))
Expand Down
4 changes: 2 additions & 2 deletions Tests/test_image_getextrema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def extrema(mode):

self.assertEqual(extrema("1"), (0, 255))
self.assertEqual(extrema("L"), (0, 255))
self.assertEqual(extrema("I"), (0, 255))
self.assertEqual(extrema("I"), (150, 65280))
self.assertEqual(extrema("F"), (0, 255))
self.assertEqual(extrema("P"), (0, 225)) # fixed palette
self.assertEqual(
Expand All @@ -20,7 +20,7 @@ def extrema(mode):
extrema("RGBA"), ((0, 255), (0, 255), (0, 255), (255, 255)))
self.assertEqual(
extrema("CMYK"), ((0, 255), (0, 255), (0, 255), (0, 0)))
self.assertEqual(extrema("I;16"), (0, 255))
self.assertEqual(extrema("I;16"), (150, 65280))

def test_true_16(self):
im = Image.open("Tests/images/16_bit_noise.tif")
Expand Down
2 changes: 1 addition & 1 deletion Tests/test_image_histogram.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def histogram(mode):

self.assertEqual(histogram("1"), (256, 0, 10994))
self.assertEqual(histogram("L"), (256, 0, 638))
self.assertEqual(histogram("I"), (256, 0, 638))
self.assertEqual(histogram("I"), (256, 1, 662))
self.assertEqual(histogram("F"), (256, 0, 638))
self.assertEqual(histogram("P"), (256, 0, 1871))
self.assertEqual(histogram("RGB"), (768, 4, 675))
Expand Down
56 changes: 28 additions & 28 deletions Tests/test_imagemath.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,22 +31,22 @@ class TestImageMath(PillowTestCase):
def test_sanity(self):
self.assertEqual(ImageMath.eval("1"), 1)
self.assertEqual(ImageMath.eval("1+A", A=2), 3)
self.assertEqual(pixel(ImageMath.eval("A+B", A=A, B=B)), "I 3")
self.assertEqual(pixel(ImageMath.eval("A+B", images)), "I 3")
self.assertEqual(pixel(ImageMath.eval("A+B", A=A, B=B)), "I 768")
self.assertEqual(pixel(ImageMath.eval("A+B", images)), "I 768")
self.assertEqual(pixel(ImageMath.eval("float(A)+B", images)), "F 3.0")
self.assertEqual(pixel(
ImageMath.eval("int(float(A)+B)", images)), "I 3")
ImageMath.eval("int(float(A)+B)", images)), "I 768")

def test_ops(self):

self.assertEqual(pixel(ImageMath.eval("-A", images)), "I -1")
self.assertEqual(pixel(ImageMath.eval("-A", images)), "I -256")
self.assertEqual(pixel(ImageMath.eval("+B", images)), "L 2")

self.assertEqual(pixel(ImageMath.eval("A+B", images)), "I 3")
self.assertEqual(pixel(ImageMath.eval("A-B", images)), "I -1")
self.assertEqual(pixel(ImageMath.eval("A*B", images)), "I 2")
self.assertEqual(pixel(ImageMath.eval("A+B", images)), "I 768")
self.assertEqual(pixel(ImageMath.eval("A-B", images)), "I -256")
self.assertEqual(pixel(ImageMath.eval("A*B", images)), "I 131072")
self.assertEqual(pixel(ImageMath.eval("A/B", images)), "I 0")
self.assertEqual(pixel(ImageMath.eval("B**2", images)), "I 4")
self.assertEqual(pixel(ImageMath.eval("B**2", images)), "I 262144")
self.assertEqual(pixel(
ImageMath.eval("B**33", images)), "I 2147483647")

Expand All @@ -72,61 +72,61 @@ def test_convert(self):
ImageMath.eval("convert(A+B, 'RGB')", images)), "RGB (3, 3, 3)")

def test_compare(self):
self.assertEqual(pixel(ImageMath.eval("min(A, B)", images)), "I 1")
self.assertEqual(pixel(ImageMath.eval("max(A, B)", images)), "I 2")
self.assertEqual(pixel(ImageMath.eval("A == 1", images)), "I 1")
self.assertEqual(pixel(ImageMath.eval("min(A, B)", images)), "I 256")
self.assertEqual(pixel(ImageMath.eval("max(A, B)", images)), "I 512")
self.assertEqual(pixel(ImageMath.eval("A == 256", images)), "I 1")
self.assertEqual(pixel(ImageMath.eval("A == 2", images)), "I 0")

def test_one_image_larger(self):
self.assertEqual(pixel(ImageMath.eval("A+B", A=A2, B=B)), "I 3")
self.assertEqual(pixel(ImageMath.eval("A+B", A=A, B=B2)), "I 3")
self.assertEqual(pixel(ImageMath.eval("A+B", A=A2, B=B)), "I 768")
self.assertEqual(pixel(ImageMath.eval("A+B", A=A, B=B2)), "I 768")

def test_abs(self):
self.assertEqual(pixel(ImageMath.eval("abs(A)", A=A)), "I 1")
self.assertEqual(pixel(ImageMath.eval("abs(B)", B=B)), "I 2")
self.assertEqual(pixel(ImageMath.eval("abs(A)", A=A)), "I 256")
self.assertEqual(pixel(ImageMath.eval("abs(B)", B=B)), "I 512")

def test_binary_mod(self):
self.assertEqual(pixel(ImageMath.eval("A%A", A=A)), "I 0")
self.assertEqual(pixel(ImageMath.eval("B%B", B=B)), "I 0")
self.assertEqual(pixel(ImageMath.eval("A%B", A=A, B=B)), "I 1")
self.assertEqual(pixel(ImageMath.eval("A%B", A=A, B=B)), "I 256")
self.assertEqual(pixel(ImageMath.eval("B%A", A=A, B=B)), "I 0")
self.assertEqual(pixel(ImageMath.eval("Z%A", A=A, Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("Z%B", B=B, Z=Z)), "I 0")

def test_bitwise_invert(self):
self.assertEqual(pixel(ImageMath.eval("~Z", Z=Z)), "I -1")
self.assertEqual(pixel(ImageMath.eval("~A", A=A)), "I -2")
self.assertEqual(pixel(ImageMath.eval("~B", B=B)), "I -3")
self.assertEqual(pixel(ImageMath.eval("~A", A=A)), "I -257")
self.assertEqual(pixel(ImageMath.eval("~B", B=B)), "I -513")

def test_bitwise_and(self):
self.assertEqual(pixel(ImageMath.eval("Z&Z", A=A, Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("Z&A", A=A, Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("A&Z", A=A, Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("A&A", A=A, Z=Z)), "I 1")
self.assertEqual(pixel(ImageMath.eval("A&A", A=A, Z=Z)), "I 256")

def test_bitwise_or(self):
self.assertEqual(pixel(ImageMath.eval("Z|Z", A=A, Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("Z|A", A=A, Z=Z)), "I 1")
self.assertEqual(pixel(ImageMath.eval("A|Z", A=A, Z=Z)), "I 1")
self.assertEqual(pixel(ImageMath.eval("A|A", A=A, Z=Z)), "I 1")
self.assertEqual(pixel(ImageMath.eval("Z|A", A=A, Z=Z)), "I 256")
self.assertEqual(pixel(ImageMath.eval("A|Z", A=A, Z=Z)), "I 256")
self.assertEqual(pixel(ImageMath.eval("A|A", A=A, Z=Z)), "I 256")

def test_bitwise_xor(self):
self.assertEqual(pixel(ImageMath.eval("Z^Z", A=A, Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("Z^A", A=A, Z=Z)), "I 1")
self.assertEqual(pixel(ImageMath.eval("A^Z", A=A, Z=Z)), "I 1")
self.assertEqual(pixel(ImageMath.eval("Z^A", A=A, Z=Z)), "I 256")
self.assertEqual(pixel(ImageMath.eval("A^Z", A=A, Z=Z)), "I 256")
self.assertEqual(pixel(ImageMath.eval("A^A", A=A, Z=Z)), "I 0")

def test_bitwise_leftshift(self):
self.assertEqual(pixel(ImageMath.eval("Z<<0", Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("Z<<1", Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("A<<0", A=A)), "I 1")
self.assertEqual(pixel(ImageMath.eval("A<<1", A=A)), "I 2")
self.assertEqual(pixel(ImageMath.eval("A<<0", A=A)), "I 256")
self.assertEqual(pixel(ImageMath.eval("A<<1", A=A)), "I 512")

def test_bitwise_rightshift(self):
self.assertEqual(pixel(ImageMath.eval("Z>>0", Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("Z>>1", Z=Z)), "I 0")
self.assertEqual(pixel(ImageMath.eval("A>>0", A=A)), "I 1")
self.assertEqual(pixel(ImageMath.eval("A>>1", A=A)), "I 0")
self.assertEqual(pixel(ImageMath.eval("A>>0", A=A)), "I 256")
self.assertEqual(pixel(ImageMath.eval("A>>1", A=A)), "I 128")

def test_logical_eq(self):
self.assertEqual(pixel(ImageMath.eval("A==A", A=A)), "I 1")
Expand Down
2 changes: 2 additions & 0 deletions Tests/test_mode_i16.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ def verify(self, im1):
xy = x, y
p1 = pix1[xy]
p2 = pix2[xy]
if im1.mode[0] != "I" and im2.mode[0] == "I":
p2 //= 256
self.assertEqual(
p1, p2,
("got %r from mode %s at %s, expected %r" %
Expand Down
6 changes: 3 additions & 3 deletions docs/handbook/concepts.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ Modes
-----

The ``mode`` of an image defines the type and depth of a pixel in the image.
Each pixel uses the full range of the bit depth. So a 1-bit pixel has a range
of 0-1, an 8-bit pixel has a range of 0-255 and so on. The current release
supports the following standard modes:
A 1-bit pixel has a range of 0-1, an 8-bit pixel or a 32-bit floating point
pixel has a range of 0-255, and a 32-bit signed integer has a range of 0-65535.
The current release supports the following standard modes:
radarhere marked this conversation as resolved.
Show resolved Hide resolved

* ``1`` (1-bit pixels, black and white, stored with one pixel per byte)
* ``L`` (8-bit pixels, black and white)
Expand Down
60 changes: 21 additions & 39 deletions src/libImaging/Convert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ rgb2i(UINT8* out_, const UINT8* in, int xsize)
int x;
INT32* out = (INT32*) out_;
for (x = 0; x < xsize; x++, in += 4)
*out++ = L24(in) >> 16;
*out++ = L24(in) >> 8;
}

static void
Expand Down Expand Up @@ -575,8 +575,8 @@ l2i(UINT8* out_, const UINT8* in, int xsize)
{
int x;
INT32* out = (INT32*) out_;
for (x = 0; x < xsize; x++)
*out++ = (INT32) *in++;
for (x = 0; x < xsize; x++, in++)
*out++ = (INT32) (*in << 8);
}

static void
Expand All @@ -585,12 +585,7 @@ i2l(UINT8* out, const UINT8* in_, int xsize)
int x;
INT32* in = (INT32*) in_;
for (x = 0; x < xsize; x++, in++, out++) {
if (*in <= 0)
*out = 0;
else if (*in >= 255)
*out = 255;
else
*out = (UINT8) *in;
*out = (UINT8) (*in >> 8);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think that you’ve got the I;16 and I modes mixed up. I suspect that part of the problem with the referenced issues was likely I;16 image data being loaded in an I(;32) image because the i16 modes are complicated and only partially supported, but apart from the conversion to 8 bit, it’s safe (but inefficient) to operate on 16bit data in 32 bit storage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for checking, and what you're saying makes sense. So in order to fix this properly, am I going to have to switch Pillow to reading I;16 actually as I;16 not I then?

Copy link
Member

@wiredfool wiredfool Jun 10, 2019 •
edited by radarhere

Choose a reason for hiding this comment

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

I think it's more difficult than that.

I;16 and I;32 are not used quite like the 8 bit versions -- often times the actual image is only 15 or 10 bit depth, but there's no way to specify a 10 bit gray image, so it gets rounded up to the next byte. There are definitely files that I've seen with I;16 that are in the range of 0-8000 or so. So there's really no way to do a downconversion without extra info to tell you what the intent of the conversion is. Do you want to preserve the LSB, MSB, or autoscale?

The various I;16 modes (signed or unsigned) are not well supported, and really are only useful for bridging to numpy or file format conversions. I think it's basic things like resampling and filtering that don't work with I;16. But operating on a I;16 in 32 bit mode is fine, because none of the math is broken, so we went with that.

The reason that I never fixed this longstanding issue is that I never had a good handle on what I wanted the API to look like, and changing things as it is will break some users while fixing others.

I think that something like a convert mode for downsampling would make sense:

img = i32.convert('L', overflow=Image.CLIP)

could work, where we had an enum of Clip, Shift or Autoscale. Autoscale is definitely a new case and more work. Clip is what we have, Shift is what you wrote (mostly). (Though, this can work both ways, expanding is also a thing on upconversion). (naming is hard)

I think that fundamentally, you should be able to losslessly convert an I;16 to an I;32 and back by default, and similarly, I think that L->I;32->L should also work. I'd be ok with changing the defaults here on a major version, but probably not for a .x release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've created PR #3898 to revert this.

}

Expand All @@ -601,7 +596,7 @@ i2f(UINT8* out_, const UINT8* in_, int xsize)
INT32* in = (INT32*) in_;
FLOAT32* out = (FLOAT32*) out_;
for (x = 0; x < xsize; x++)
*out++ = (FLOAT32) *in++;
*out++ = (FLOAT32) (*in++ >> 8);
}

static void
Expand All @@ -610,12 +605,7 @@ i2rgb(UINT8* out, const UINT8* in_, int xsize)
int x;
INT32* in = (INT32*) in_;
for (x = 0; x < xsize; x++, in++, out+=4) {
if (*in <= 0)
out[0] = out[1] = out[2] = 0;
else if (*in >= 255)
out[0] = out[1] = out[2] = 255;
else
out[0] = out[1] = out[2] = (UINT8) *in;
out[0] = out[1] = out[2] = (UINT8) (*in >> 8);
out[3] = 255;
}
}
Expand Down Expand Up @@ -664,7 +654,7 @@ f2i(UINT8* out_, const UINT8* in_, int xsize)
FLOAT32* in = (FLOAT32*) in_;
INT32* out = (INT32*) out_;
for (x = 0; x < xsize; x++)
*out++ = (INT32) *in++;
*out++ = (INT32) *in++ << 8;
}

/* ----------------- */
Expand Down Expand Up @@ -722,24 +712,22 @@ ycbcr2la(UINT8* out, const UINT8* in, int xsize)
static void
I_I16L(UINT8* out, const UINT8* in_, int xsize)
{
int x, v;
int x;
INT32* in = (INT32*) in_;
for (x = 0; x < xsize; x++, in++) {
v = CLIP16(*in);
*out++ = (UINT8) v;
*out++ = (UINT8) (v >> 8);
*out++ = (UINT8) *in;
*out++ = (UINT8) (*in >> 8);
}
}

static void
I_I16B(UINT8* out, const UINT8* in_, int xsize)
{
int x, v;
int x;
INT32* in = (INT32*) in_;
for (x = 0; x < xsize; x++, in++) {
v = CLIP16(*in);
*out++ = (UINT8) (v >> 8);
*out++ = (UINT8) v;
*out++ = (UINT8) (*in >> 8);
*out++ = (UINT8) *in;
}
}

Expand Down Expand Up @@ -769,7 +757,7 @@ I16L_F(UINT8* out_, const UINT8* in, int xsize)
int x;
FLOAT32* out = (FLOAT32*) out_;
for (x = 0; x < xsize; x++, in += 2)
*out++ = (FLOAT32) (in[0] + ((int) in[1] << 8));
*out++ = (FLOAT32) in[1];
}


Expand All @@ -779,16 +767,16 @@ I16B_F(UINT8* out_, const UINT8* in, int xsize)
int x;
FLOAT32* out = (FLOAT32*) out_;
for (x = 0; x < xsize; x++, in += 2)
*out++ = (FLOAT32) (in[1] + ((int) in[0] << 8));
*out++ = (FLOAT32) in[0];
}

static void
L_I16L(UINT8* out, const UINT8* in, int xsize)
{
int x;
for (x = 0; x < xsize; x++, in++) {
*out++ = *in << 8;
*out++ = *in;
*out++ = 0;
}
}

Expand All @@ -797,8 +785,8 @@ L_I16B(UINT8* out, const UINT8* in, int xsize)
{
int x;
for (x = 0; x < xsize; x++, in++) {
*out++ = 0;
*out++ = *in;
*out++ = *in << 8;
}
}

Expand All @@ -807,21 +795,15 @@ I16L_L(UINT8* out, const UINT8* in, int xsize)
{
int x;
for (x = 0; x < xsize; x++, in += 2)
if (in[1] != 0)
*out++ = 255;
else
*out++ = in[0];
*out++ = in[1] + (in[0] << 8);
}

static void
I16B_L(UINT8* out, const UINT8* in, int xsize)
{
int x;
for (x = 0; x < xsize; x++, in += 2)
if (in[0] != 0)
*out++ = 255;
else
*out++ = in[1];
*out++ = in[0] + (in[1] << 8);
}

static struct {
Expand Down Expand Up @@ -1020,7 +1002,7 @@ p2i(UINT8* out_, const UINT8* in, int xsize, const UINT8* palette)
int x;
INT32* out = (INT32*) out_;
for (x = 0; x < xsize; x++)
*out++ = L(&palette[in[x]*4]) / 1000;
*out++ = (L(&palette[in[x]*4]) / 1000) << 8;
}

static void
Expand All @@ -1029,7 +1011,7 @@ pa2i(UINT8* out_, const UINT8* in, int xsize, const UINT8* palette)
int x;
INT32* out = (INT32*) out_;
for (x = 0; x < xsize; x++, in += 4)
*out++ = L(&palette[in[0]*4]) / 1000;
*out++ = (L(&palette[in[0]*4]) / 1000) << 8;
}

static void
Expand Down