Use imageio v3 API #6764
Use imageio v3 API #6764
Conversation
|
I had a quick look at CI and it looks like the tests that break call Instead of |
|
Paletted read is still broken: |
|
Matplotlib reads this image correctly; PIL does too, but you need to handle https://github.com/scikit-image/scikit-image/raw/main/skimage/data/palette_color.png @FirefoxMetzger would you consider this to be a bug? |
|
Interesting image @stefanv . Would we be allowed to adopt it in our own test suite? Currently, we have a warning/note for this behavior in the pillow plugin's read function: and you can have ImageIO reconstruct it for you by asking for a result in RGB: img = iio.imread("palette_color.png", mode="RGB")
img.shape # (10, 10, 3)That said, we have changed the behavior for GIF a while ago, i.e., you get a reconstructed image (RGB/A depending on the palette) instead of palette indices. This came after listening to a frequent stream of feedback asking if GIF was broken/buggy. I think we could extend this behavior to other paletted formats including PNG since you, too, are asking if the behavior for palette PNG is a bug 😄. |
@stefanv This will be fixed by imageio/imageio#955 . That PR is ready except for a regression test; would I be allowed to add https://github.com/scikit-image/scikit-image/raw/main/skimage/data/palette_color.png to our test suite (and distribute it alongside the suite), or should I look into generating my own palette PNG image? |
|
The other failure is that ImageIO sometimes returns a read-only ndarray that is a view into a PIL buffer instead of making a copy of the data. This is a performance optimization done internally by PIL; however, it seems to break some of the filters here which assume that the image is writable. Does |
|
@FirefoxMetzger Yes, indeed, the idea is that you get a standard ndarray back that you can manipulate however you wish; so that Note that we can also very easily add this to our wrapper, which may be easier? |
You can go ahead and use that image. |
|
The changes I pushed should work around the read-only behavior. I should probably test for that explicitly (I don't know which images show this behavior), but I know it's being tested for implicitly in the test suite. |
|
This failure I don't understand. On my local machine, |
|
@FirefoxMetzger This seems like a genuine bug in imageio: |
|
Sorry, @stefanv; I didn't get around to checking this until now. The image is again a paletted PNG and its palette is RGB. I.e., the palette sets all color channels to the same level to give the user the impression of a gray image but internally stores RGB colors. Storing data as RGB currently means that get data back as RGB since that's what most users expect. In the case of Our old pillow plugin had an extra step of custom logic that would scan palettes and run a check for grayscale. If the palette only contains gray values, it would then reconstruct the RGB image as usual, but then add an additional post-processing step to convert to grayscale before returning the result. The new plugin does not do this additional step and, at first glance, this seems intuitive. For 8-bit depth, there is no benefit in palettizing a gray image. It won't compress the image (we still need 8-bit per pixel) and now we also have to store the palette, which takes up extra space. If, despite this, a user still stores gray images as a palette then their reason for doing so likely revolves around the fact that palettes are RGB internally, which is the key difference to storing grayscale directly. To me, having ImageIO undo this internally seems counterproductive. That said, there is (as always) utility in continuity and backward compatibility. We could reintroduce the RGB->gray palette check and add a flag to turn it on/off. I'm curious what you think about this @stefanv . Based on the unit test you clearly expected a grayscale image, but I'm curious if this is mainly an appeal to tradition ("it has always been this way") or if there is deeper reasoning for this check that I am not aware of. |
It seems like the recent release of imageio v2.28 breaks some of our tests [1]. It's being worked on [2],[3] but in the meantime let's pin the dependency below 2.28 so that our CI is useful again. [1] scikit-image#6908 [2] scikit-image#6764 [3] scikit-image#6907 (comment)
It seems like the recent release of imageio v2.28 breaks some of our tests [1]. It's being worked on [2],[3] but in the meantime let's pin the dependency below 2.28 so that our CI is useful again. [1] scikit-image#6908 [2] scikit-image#6764 [3] scikit-image#6907 (comment)
It seems like the recent release of imageio v2.28 breaks some of our tests [1]. It's being worked on [2],[3] but in the meantime let's pin the dependency below 2.28 so that our CI is useful again. [1] scikit-image#6908 [2] scikit-image#6764 [3] scikit-image#6907 (comment)
@lagru Yes, because v2.28 only affects the There are still some discussion points in this PR though that need resolution before it can be merged. In particular, we need to decide how to deal with RGB paletted images that store "grey as RGB", i.e., the entire palette consists of gray values encoded as RGB. The old ImageIO had an explicit (and slow) check for this and did RGB->gray conversion if needed; however, we currently don't do this, because the image was explicitly stored as RGB which is probably intentional so you get things back as stored, which means as RGB. |
It seems like the recent release of imageio v2.28 breaks some of our tests [1]. It's being worked on [2],[3] but in the meantime let's pin the dependency below 2.28 so that our CI is useful again. [1] #6908 [2] #6764 [3] #6907 (comment)
|
I suspect the new imageio behavior is more correct, and we should fix our eagle data file instead. |
|
With how you summarize the issue in #6908 (comment) and here, I would also consider the new v3 behavior to be more "correct". If the image is stored as RGB, returning RGB seems like an expected thing to do. And at least this part should be an easy enough update on the side of scikit-image. We could just drop the 2 channels after loading it, or do the proper thing and update the file itself. :) |
|
See https://gitlab.com/scikit-image/data/-/merge_requests/22. Once that's merged we can update the registry entry for |
|
Awesome @lagru once that is merged all the CI failures here should resolve themself (at least as far as I can see). |
|
@lagru Merged on gitlab; do you mind updating the data repo? You are welcome to push to this PR what you need to. |
Updates the registry entry for eagle.png to the updated version from [1]. The original file contained a RGB palette whose contents were identical. The image is therefore effectively grayscale. Previous to imageio v2.28, their pillow plugin had custom logic to check for this case in which case it would remove the duplicate channels. However, this behavior was removed in v2.28, which means that reading the image will return an array with those RGB channels as the last dimension which breaks scikit-image's existing API and tests. [2] [1] https://gitlab.com/scikit-image/data/-/merge_requests/22 [2] scikit-image#6764 (comment)
|
Thanks! Should be good to go on green. |
|
@FirefoxMetzger We're almost good to go, but with latest imageio we have Imagemagick tells me: Any idea what the correct output here should be? |
|
Edit: With @stefanv's command I actually get Click to expandThe shape of 0 and 13 seems off. |
|
I've re-introduced the <2.28 pin, since we do not have time to figure out this latest change in behavior before the release. |
|
Yes, I noticed this as well last week. Unfortunately, I didn't yet find the time to think through the important scenarios and decide the correct behavior. I started a new job last week and getting set up didn’t leave much free time. The short version: You already identified that the first plane of this GIF is RGB and all others are RGBA. This is likely due to the writer taking advantage of one of GIF's more obscure compression features, which is commonly "misused" to create transparent GIFs. In a nutshell returning RGB is the technically correct thing to do (read: it strictly follows the GIF standard), but it may be more user friendly to return RGBA instead (read: strictly following GIF might be surprising here). (sorry that the below response turned out so long. It's again one of those edge cases that doesn't have a clear default and depends on "what the average user expects". My hope with this lengthy explanation is that it gives you enough context to give me feedback on what skimage's "average user" would expect.) The longer version: A GIF palette can’t store RGBA; it's always RGB. Instead, one may set a byte (a The obligatory piece of trivia: The original intent of this transparency flag was to allow better compression (smaller per-frame palettes) and to circumvent the 256-color limitation. If we can reuse 2+ colors from the previous frame, we can choose 1+ new colors for the current frame and create a 257+ color image (255 new colors, and 2+ reused/old). It's an ingenious idea, but not many writers ever took advantage of it because it means a lot of GIF-specific custom logic when creating a frame-level palette. Instead people found a loophole in the definition of this feature (the implicit background color), which eventually became RGBA support in GIF. This brings us back to the ultimate question of: What is the expected behavior of a reader for this particular image? Since, for this image, the first frame is RGB (not RGB + transparency flag) we can always trace the final value to a valid RGB color and avoid the “undefined behavior/transparent pixel hack” territory. Also, for each index, users expect a fully decoded frame. This means that - by default - for |
|
@FirefoxMetzger, no worries and thank you so much for taking the time to give a detailed answer anyway! It's very much appreciated. Hope can settle nicely into the new job. :) Regarding scikit-image/skimage/io/tests/test_pil.py Lines 265 to 267 in e8a92e3 I don't think our tests actually care for the properties of this GIF that we are currently discussing as long as the shape can be asserted. In this context I think we are free to just update the unit test and / or the file. Do you know a quick way to generate a similar file without the ambiguous first channel? I don't think the content actually matters to our tests... I'm not sure which behavior imageio should aim for but I find it noteworthy that PIL has been returning only 3 channels for this GIF for some time. |
RIP ⚰️ 🤣
If you want the same pixels, loading the image in the mode you want (RGB/RGBA) and saving it again should do the trick. # if you want to have RGB consistently
img = iio.imread("no_time_for_that_tiny.gif", mode="RBG")
# if you want to have RGBA consistently
img = iio.imread("no_time_for_that_tiny.gif", mode="RBG")
iio.imwrite("new_test_file.gif", img)If you don't care about the actual pixel values, you can also opt to generate a GIF in memory: desired_shape = (24, 25, 14, 3)
img = np.random.randint(0, 255, desired_shape, dtype=np.uint8)
# img = np.full(desired_shape, 42, dtype=np.uint8)
buffer = iio.imwrite("<bytes>", img, extension=".gif")
result = iio.imread(buffer)
assert result.shape == desired_shape
Hm. That combined with the fact that RGB is the standard conform thing to do makes two arguments towards the current behavior being what we should do. I'll think about it some more. |
in favor of a fixture. Bypasses problems due to the file returning a different number of channels depending on imageio's version [1]. Continuation of scikit-image#6764 [2]. Also remove its mention in ImageCollection's docstring and simplify / update the example section. [1] scikit-image#6764 (comment) [2] scikit-image#6764
In response to
https://discuss.scientific-python.org/t/full-support-for-4-channel-images-in-imsave/651/9
Thanks for the prod, @FirefoxMetzger