-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Improved I mode conversion #3838
Changes from all commits
8a035b8
ddc83fd
2787b9b
64eff66
a258b8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I've created PR #3898 to revert this. |
||
} | ||
|
||
|
@@ -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 | ||
|
@@ -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; | ||
} | ||
} | ||
|
@@ -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; | ||
} | ||
|
||
/* ----------------- */ | ||
|
@@ -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; | ||
} | ||
} | ||
|
||
|
@@ -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]; | ||
} | ||
|
||
|
||
|
@@ -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; | ||
} | ||
} | ||
|
||
|
@@ -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; | ||
} | ||
} | ||
|
||
|
@@ -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 { | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that range actually correct? 32 bit signed is +-2 billion, that sounds more like 16bit unsigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to numpy docs 16-bit is 0 - 65535. uint32 is 0 to 4294967295. Ps thanks guys! In desperate need of 16-bit functionality so will be watching this closely.