-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
Co-Authored-By: Hugo <hugovk@users.noreply.github.com>
This one is going to need release notes as a potentially breaking change |
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. | ||
These modes are supported: |
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.
*out = 255; | ||
else | ||
*out = (UINT8) *in; | ||
*out = (UINT8) (*in >> 8); | ||
} |
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.
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 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?
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've created PR #3898 to revert this.
Noted in #3835. Or should we play it safe and hold off on this until major release 7.0.0 on 2020-01-01? |
As I noted on some of the other comments, I’m not sure that this pr is correct, because it appears that the i16 and i32 modes got confused. |
Was this change released in version 6.1.0? I can see that it is referenced but I couldn't find it in the release notes. |
This change was reverted - it turns out it did not perfectly solve the problem. |
I see. Thank you for clarifying @radarhere |
is the fix available in release 7.0.0? |
@veonua the fix is not part of any release. It was reverted, as it did not perfectly solve the problem |
@wiredfool told that it can't be a part of a minor release,
at this moment I'm facing an unexpected problem - around 5% of my pipeline output is blank images. please let me know if there is an alternative library that can simply convert image to grayscale (8bit) png |
when fix? |
I don't know when this issue will be fixed. If you would like to, you could investigate and create a PR with a solution. |
It would be far more productive if someone who already knew the code handled this, obviously. |
so... is there a workaround to this problem in 2024?? |
There is not a general workaround. If you have a specific situation, you can open a new issue and we can offer a specific solution, something along the lines of #5991 |
Improves I mode conversion, scaling values, rather than clipping.
hopper("I")
hopper("I;16")
hopper("F").convert("I")
Also resolves the first part of #3159.
This change the results for a number of tests though, most significantly for ImageMath. The value of 'I' is changed now, being a scaled version of other modes instead of a clipped version, so when ImageMath returns a result with 'I' in it, that number is also scaled.