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

Option to turn off automatic scaling of PPM images with arbitrary maxval #6202

Closed
SimonSegerblomRex opened this issue Apr 12, 2022 · 7 comments

Comments

@SimonSegerblomRex
Copy link

SimonSegerblomRex commented Apr 12, 2022

What did you do?

We are using imageio for reading PGM files with 12 bit values (maxval 4095 in the header) and now our tests started failing with the release of Pillow 9.1.0.

What did you expect to happen?

No change of the bahaviour of how PGM files are interpreted.

What actually happened?

Now the 12 bit values are scaled to 16 bits. This change was introduced in #6119. From the discussion in the related issue threads I understand that this was an intentional change, so you might consider this a case of https://xkcd.com/1172/.

(I haven't looked into the details of #5242, but it might be good to have this issue in mind when reviewing that.)

What are your OS, Python and Pillow versions?

  • OS: Debian 9
  • Python: 3.7, 3.8, 3.9
  • Pillow: 9.1.0

Dirty hack to "fix" this issue:

diff --git a/Tests/test_file_ppm.py b/Tests/test_file_ppm.py
index 2c965318..ea7c6ef4 100644
--- a/Tests/test_file_ppm.py
+++ b/Tests/test_file_ppm.py
@@ -23,7 +23,7 @@ def test_sanity():
     "data, mode, pixels",
     (
         (b"P5 3 1 4 \x00\x02\x04", "L", (0, 128, 255)),
-        (b"P5 3 1 257 \x00\x00\x00\x80\x01\x01", "I", (0, 32640, 65535)),
+        (b"P5 3 1 257 \x00\x00\x00\x80\x01\x01", "I", (0, 128, 257)),
         # P6 with maxval < 255
         (
             b"P6 3 1 17 \x00\x01\x02\x08\x09\x0A\x0F\x10\x11",
diff --git a/src/PIL/PpmImagePlugin.py b/src/PIL/PpmImagePlugin.py
index b760e228..39a6c1bf 100644
--- a/src/PIL/PpmImagePlugin.py
+++ b/src/PIL/PpmImagePlugin.py
@@ -138,7 +138,7 @@ class PpmDecoder(ImageFile.PyDecoder):
         maxval = min(self.args[-1], 65535)
         in_byte_count = 1 if maxval < 256 else 2
         out_byte_count = 4 if self.mode == "I" else 1
-        out_max = 65535 if self.mode == "I" else 255
+        out_max = maxval if self.mode == "I" else 255
         bands = Image.getmodebands(self.mode)
         while len(data) < self.state.xsize * self.state.ysize * bands * out_byte_count:
             pixels = self.fd.read(in_byte_count * bands)
@radarhere
Copy link
Member

So, if I'm understanding correctly, you are exactly opposed to #5403, yes? That issue was about a 10-bit PGM file not being scaled automatically, and you would like the image data not to be scaled?

It sounds like you appreciate that your scenario is not aligned with common image manipulation operations. Can I interest you in the following code instead?

from io import BytesIO
from PIL import Image
fp = BytesIO(b"P5 3 1 257 \x00\x00\x00\x80\x01\x01")
with Image.open(fp) as im:
    assert im.size == (3, 1)
    assert im.mode == "I"

    # Multiply the image data to mimic the earlier behaviour
    args = im.tile[-1]
    if args[0] == "ppm":
        maxval = args[3][1]
        im = im.point(lambda x: x * (maxval / 65535))

    px = im.load()
    assert tuple(px[x, 0] for x in range(3)) == (0, 128, 257)

@SimonSegerblomRex
Copy link
Author

SimonSegerblomRex commented Apr 12, 2022

So, if I'm understanding correctly, you are exactly opposed to #5403, yes? That issue was about a 10-bit PGM file not being scaled automatically, and you would like the image data not to be scaled?

Yes, that's correct.

It sounds like you appreciate that your scenario is not aligned with common image manipulation operations.

I agree that the new behaviour might be beneficial as long as you don't have any other information about maxval after decoding the image, but I don't find it obvious that the image always should be scaled to 16 (or 8) bits. OpenCV has a number of flags for the imread+imdecode functions, none of them scaling the values to 16 bits:

import cv2
import numpy as np

INPUT_IMAGE = np.asarray(
    bytearray(b"P5 3 1 257 \x00\x00\x00\x80\x01\x01"),
    dtype=np.uint8,
)

# Flags documented here:
# https://docs.opencv.org/4.x/d8/d6a/group__imgcodecs__flags.html#gga61d9b0126a3e57d9277ac48327799c80af660544735200cbe942eea09232eb822
FLAG_NAMES = [
    "IMREAD_UNCHANGED",
    "IMREAD_GRAYSCALE",
    "IMREAD_ANYDEPTH",
    "IMREAD_ANYCOLOR",
    "IMREAD_LOAD_GDAL",
]

for flag_name in FLAG_NAMES:
    flag = getattr(cv2, flag_name)
    try:
        image = cv2.imdecode(INPUT_IMAGE, flag)
        print(f"Max value with flag {flag_name:16}: {np.max(image):3}")
    except Exception:
        print(f"(Failed imdecode with flag {flag_name}")
Max value with flag IMREAD_UNCHANGED: 257
Max value with flag IMREAD_GRAYSCALE:   1
Max value with flag IMREAD_ANYDEPTH : 257
Max value with flag IMREAD_ANYCOLOR :   1
Max value with flag IMREAD_LOAD_GDAL: 257

(I didn't try all the combinations of the flags, but this also returns 257:

np.max(cv2.imdecode(INPUT_IMAGE, cv2.IMREAD_GRAYSCALE | cv2.IMREAD_ANYDEPTH))

)

@radarhere
Copy link
Member

If I take the image from #5403 and resave it as a PNG using Pillow, I see a image that looks good to me as a human.

from PIL import Image
im = Image.open("cat.pgm")
im.save("pillow.png")

pillow

If I try to resave it using OpenCV, I get a black image.

import cv2
im = cv2.imread("cat.pgm")
cv2.imwrite("cv2.png", im)

cv2

@SimonSegerblomRex
Copy link
Author

SimonSegerblomRex commented Apr 14, 2022

If I try to resave it using OpenCV, I get a black image.

It's actually not completely black, just very dark since you get an 8 bit image when using cv2.imread without any flags.

Using a proper combination of flags (e.g., IMREAD_ANYDEPTH) you'll get the raw values. Since PNG only supports certain bit depths you need to scale the 10 bit values to 16 bits before saving the image:

cat_10bit = cv2.imread("cat.pgm", cv2.IMREAD_ANYDEPTH)
cat_16bit = (cat_10bit / 1023 * 65535 + 0.5).astype(np.uint16)
cv2.imwrite("cat.png", cat_16bit)

(A proper PNG encoder should also set sBIT to 10 in this case.)

I'm not really trying to argue that the OpenCV functions are any better than your implementation (on the contrary they are quite useless if you don't have any prior knowledge of maxval). The new automatic scaling when decoding the image is a nice feature, but since it's a breaking change it would have been nice with a flag to get the option of keeping the old behaviour.

@SimonSegerblomRex SimonSegerblomRex changed the title Support for reading raw values of PPM with arbitrary maxval Option to turn off automatic scaling of PPM images with arbitrary maxval Apr 14, 2022
@radarhere
Copy link
Member

Oh, I thought you were holding OpenCV up as how Pillow should work, and I was trying to show that it doesn't work for #5403.

Pillow's general approach is to adjust the raw data of images to suit subsequent image operations requested by the user. There is the concept of the "rawmode" of the image, which is then read into a more common "mode".

I doubt the functionality you were using was ever intentional. I think it was an oversight, and when making the change, I perceived this as fixing a bug.

I think it would be an outlier to enable this option to read in pixel values without transformation just for PPM. To enable it for all formats would be a significant change to the way that Pillow operates.

If you want, I can put together another example of how to use Pillow to get the values as you did previously.
If you want to pursue this further, you are welcome to create a PR and explain its merits.
I just don't think it's in line with how Pillow interacts with images.

@SimonSegerblomRex
Copy link
Author

I doubt the functionality you were using was ever intentional. I think it was an oversight, and when making the change, I perceived this as fixing a bug.

I think it would be an outlier to enable this option to read in pixel values without transformation just for PPM. To enable it for all formats would be a significant change to the way that Pillow operates.

OK! I appreciate that and as long as it's not going to change again we can adapt our regression tests to the new behaviour.

If you want, I can put together another example of how to use Pillow to get the values as you did previously. If you want to pursue this further, you are welcome to create a PR and explain its merits. I just don't think it's in line with how Pillow interacts with images.

In our use case, we only have 10 bit or 12 bit RAW images stored in PGM files with a maxval of 1023 or 4095, so restoring the raw values is simply a matter of right shifting:

import cv2
import imageio.v3 as iio
from numpy.testing import assert_array_equal


# Pillow 9.1.0 (through imageio)
bit_depth = 10  # (We have access to this value)
image = iio.imread("cat.pgm")
recovered_raw_image = image >> (16 - bit_depth)

# OpenCV
image = cv2.imread("cat.pgm", cv2.IMREAD_ANYDEPTH)

assert_array_equal(image, recovered_raw_image)

Thank you for taking the time to discuss this! I was just surprised when our regression tests started failing, but it's not a big deal for us to adapt to this change. Hopefully not too many other users have been relying on the old behaviour 🙂

@SimonSegerblomRex
Copy link
Author

SimonSegerblomRex commented Apr 14, 2022

Just for reference:

I tried the other plugins for ImageIO:

import cv2
import imageio
 
from numpy.testing import assert_array_equal
import PIL
 
print(PIL.__version__)
 
imageio.plugins.freeimage.download()
 
FILENAME = "cat.pgm"
BIT_DEPTH = 10
 
image_cv2 = cv2.imread(FILENAME, cv2.IMREAD_UNCHANGED)
for plugin in imageio.config.plugins.known_plugins:
    try:
        image_imageio = imageio.v3.imread(FILENAME, plugin=plugin)
    except Exception as e:
        if ("PGM" in plugin.upper()) or ("PPM" in plugin.upper()):
            print(e)
        continue
    recovered_raw_image = image_imageio >> (16 - BIT_DEPTH)
 
    try:
        assert_array_equal(image_cv2, recovered_raw_image)
        print(f"Plugin '{plugin}' is using the Pillow 9.1.0 behaviour!")
    except Exception:
        try:
            assert_array_equal(image_cv2, image_imageio)
            print(f"Plugin '{plugin}' is relying on the old behaviour!")
        except Exception:
            print(f"Plugin '{plugin} is doing something unexpected...")

Output:

9.2.0.dev0
Plugin 'pillow' is using the Pillow 9.1.0 behaviour!
Plugin 'PPM-PIL' is using the Pillow 9.1.0 behaviour!
Plugin 'PBM-FI' is using the Pillow 9.1.0 behaviour!
`PGM-FI` can not handle the given uri.
`PGMRAW-FI` can not handle the given uri.
Plugin 'PPM-FI' is using the Pillow 9.1.0 behaviour!
`PPMRAW-FI` can not handle the given uri.

So FreeImage seems to be doing the same thing as Pillow 9.1.0!

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

No branches or pull requests

2 participants