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.

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

Endianness is inconsistent #6769

Closed
Yay295 opened this issue Nov 30, 2022 · 14 comments · Fixed by #6834
Closed

Endianness is inconsistent #6769

Yay295 opened this issue Nov 30, 2022 · 14 comments · Fixed by #6834

Comments

@Yay295
Copy link
Contributor

Yay295 commented Nov 30, 2022

Image class methods:

Method Parameter Endianness
blend alpha native
new color native
frombytes data mode
frombuffer data mode
getcolors native
getdata all mode
getdata band native
getextrema native
getpalette mode
getpixel native
point unsupported
putdata data native
putpalette data mode
putpixel value native
remap_palette source_palette mode
tobytes mode

Methods that use one of the above methods:
eval calls point
fromarray calls frombuffer
rotate calls transform
transform calls new

Related: #2228

@radarhere
Copy link
Member

Could you provide some more detail about how you worked out this list of endianness behaviour, perhaps some Python code to demonstrate the inconsistency?

I understand that 'native' means that the endianness of the environment is used, but what do you mean by 'mode'?

@Yay295
Copy link
Contributor Author

Yay295 commented Nov 30, 2022

I worked these out by looking at the code, though it seems I got a few wrong, and some of them have mode checks that I missed. By 'mode' I mean it accepts/returns data in the endianness of the image's mode.

from PIL import Image

def t(method, *args, **kwargs):
    print(method.__name__ + ' ', end='')
    try:
        print(method(*args, **kwargs))
    except Exception as e:
        print(e)

def callMethods(image):
    t(x.getcolors)
    t(lambda:list(x.getdata()))
    t(lambda:list(x.getdata(0)))
    t(x.getextrema)
    t(x.getpixel,(0,0))
    t(x.tobytes)

for mode in ('I;16L','I;16B'):
    print(f'\n{mode} New 123')
    with Image.new(mode, (1,1), color=123) as x:
        callMethods(x)
    
    print(f'\n{mode} From Bytes \\x00{{')
    with Image.frombytes(mode, (1,1), data=b'\x00{') as x:
        callMethods(x)
    
    print(f'\n{mode} From Bytes {{\\x00')
    with Image.frombytes(mode, (1,1), data=b'{\x00') as x:
        callMethods(x)
I;16L New 123
getcolors image has wrong mode
<lambda> [123]
<lambda> image has wrong mode
getextrema image has wrong mode
getpixel 123
tobytes b'{\x00'

I;16L From Bytes \x00{
getcolors image has wrong mode
<lambda> [31488]
<lambda> image has wrong mode
getextrema image has wrong mode
getpixel 31488
tobytes b'\x00{'

I;16L From Bytes {\x00
getcolors image has wrong mode
<lambda> [123]
<lambda> image has wrong mode
getextrema image has wrong mode
getpixel 123
tobytes b'{\x00'

I;16B New 123
getcolors image has wrong mode
<lambda> [123]
<lambda> image has wrong mode
getextrema image has wrong mode
getpixel 123
tobytes b'\x00{'

I;16B From Bytes \x00{
getcolors image has wrong mode
<lambda> [123]
<lambda> image has wrong mode
getextrema image has wrong mode
getpixel 123
tobytes b'\x00{'

I;16B From Bytes {\x00
getcolors image has wrong mode
<lambda> [31488]
<lambda> image has wrong mode
getextrema image has wrong mode
getpixel 31488
tobytes b'{\x00'

@Yay295
Copy link
Contributor Author

Yay295 commented Nov 30, 2022

Here's a more interesting test:

from PIL import Image

def t(method, *args, **kwargs):
    print(method.__name__ + ' ', end='')
    try:
        print(method(*args, **kwargs))
    except Exception as e:
        print(e)

def callMethods(image):
    t(image.getpixel,(0,0))
    image.putdata(image.getdata())
    t(image.getpixel,(0,0))

for mode in ('I;16L','I;16B'):
    print(f'\n{mode} New 123')
    with Image.new(mode, (1,1), color=123) as image:
        callMethods(image)
    
    print(f'\n{mode} From Bytes \\x00{{')
    with Image.frombytes(mode, (1,1), data=b'\x00{') as image:
        callMethods(image)
    
    print(f'\n{mode} From Bytes {{\\x00')
    with Image.frombytes(mode, (1,1), data=b'{\x00') as image:
        callMethods(image)
I;16L New 123
getpixel 123
getpixel 123

I;16L From Bytes \x00{
getpixel 31488
getpixel 31743

I;16L From Bytes {\x00
getpixel 123
getpixel 123

I;16B New 123
getpixel 123
getpixel 31611

I;16B From Bytes \x00{
getpixel 123
getpixel 31611

I;16B From Bytes {\x00
getpixel 31488
getpixel 65280

@Yay295
Copy link
Contributor Author

Yay295 commented Dec 26, 2022

I created a branch with some tests: https://github.com/Yay295/Pillow/tree/bytes_tests

There are three failures:
FAILED Tests/test_image.py::TestImageBytes::test_get_pixel[I;16-2] - assert (256, 1284, 770, 1798) == (65535, 65535, 770, 1798)
FAILED Tests/test_image.py::TestImageBytes::test_get_pixel[I;16L-2] - assert (256, 1284, 770, 1798) == (65535, 65535, 770, 1798)
FAILED Tests/test_image.py::TestImageBytes::test_get_pixel[I;16B-2] - assert (1, 1029, 515, 1543) == (511, 65535, 515, 1543)

That test is just:

  1. create image from bytes
  2. store pixels in a tuple
  3. image.putdata(image.getdata())
  4. store pixels in a tuple
  5. compare tuples from steps 2 and 4

Also apparently we can't create BGR images with Image.new()? And there was an error with "I;16N" too: "unknown raw mode for given image mode".

@radarhere
Copy link
Member

radarhere commented Dec 26, 2022

I've created PR #6825. With that, tests from bytes_tests pass, as well as your other code examples earlier.

@radarhere
Copy link
Member

I attempted to test the problem with the color argument of Image.new, but found that it passed on both big and little endian.

Would you be able to post code examples of problems apart from putdata()?

@Yay295
Copy link
Contributor Author

Yay295 commented Dec 28, 2022

I added another test to my branch for creating an image with the color argument and I don't see any error either (other than some problems with "I;16N"). I think it was just putdata() causing the issues.

@radarhere
Copy link
Member

I've created PR #6834 to fix the I;16N problems.

@radarhere
Copy link
Member

radarhere commented Dec 29, 2022

PR #6825 has now been merged. If I;16N is also fixed, does that resolve this issue?

@Yay295
Copy link
Contributor Author

Yay295 commented Dec 30, 2022

I think the Image Stats might be wrong for the I;16 modes as well. And some of the extrema for those and other modes seem to be off by one. I've printed out the stats for the Hopper image for each mode: hopper_image_stats.py.txt

For the I;16 modes I believe is due to the histogram calculation not being correct for them.

@radarhere
Copy link
Member

The following code

from PIL import Image, ImageStat
for mode in ("I;16", "I;16L", "I;16B", "RGB"):
    with Image.open("hopper.png") as im:
        if mode[:4] == "I;16":
            im = im.convert("I").convert(mode)
        assert im.mode == mode
        
        print(mode, ImageStat.Stat(im).extrema)

gives me

I;16 [(0, 255)]
I;16L [(0, 255)]
I;16B [(0, 255)]
RGB [(0, 255), (0, 255), (0, 255)]

with main and with Pillow 9.3.0. So I don't know what code you used to get (0, 254) for the I;16 modes in your attachment.

@Yay295
Copy link
Contributor Author

Yay295 commented Dec 30, 2022

I was working in Tests/test_imagestat.py, so I used the hopper() function to get the image, which loads "hopper.ppm". Apparently, the images are not quite the same...

from PIL import Image, ImageStat
for ext in ("jpg","png","ppm"):
    for mode in ("I;16", "I;16L", "I;16B", "RGB"):
        with Image.open("hopper."+ext) as im:
            if mode[:4] == "I;16":
                im = im.convert("I").convert(mode)
            assert im.mode == mode
            print(mode, ImageStat.Stat(im).extrema)
I;16 [(0, 255)]
I;16L [(0, 255)]
I;16B [(0, 255)]
RGB [(0, 255), (0, 255), (0, 255)]
I;16 [(0, 255)]
I;16L [(0, 255)]
I;16B [(0, 255)]
RGB [(0, 255), (0, 255), (0, 255)]
I;16 [(0, 254)]
I;16L [(0, 254)]
I;16B [(0, 254)]
RGB [(0, 255), (0, 255), (0, 255)]

@radarhere
Copy link
Member

#1606 (comment)

ImageStat.Stat.extrema relies on the image histogram function, and simply returns the low and high bins used. This is correct for 8 bit per channel images, but fails for anything else.

So, ultimately, Image.getextrema is a much more efficient and correct function to use.

The issue was resolved by #3661, creating a note at https://pillow.readthedocs.io/en/stable/reference/ImageStat.html#PIL.ImageStat.Stat.extrema

This relies on the histogram() method, and simply returns the low and high bins used. This is correct for images with 8 bits per channel, but fails for other modes such as I or F. Instead, use getextrema() to return per-band extrema for the image. This is more correct and efficient because, for non-8-bit modes, the histogram method uses getextrema() to determine the bins used.

@radarhere
Copy link
Member

radarhere commented Mar 19, 2023

Also apparently we can't create BGR images with Image.new()?

I've created PR #7010 to add support for creating images with BGR;15, BGR;16 and BGR;24, but to drop support for BGR;32 altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants