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

Add support for writing 16-bit greyscale images #1819

Closed
wants to merge 1 commit into from

Conversation

mairsbw
Copy link
Contributor

@mairsbw mairsbw commented Apr 9, 2016

Addresses #1514.

This has been tested and found working for writing back uint16 greyscale TIFFs little-endian TIFFs. A corresponding test has been added to the Tests directory with include an original test image collected by myself and its license is CC0 (it should be noted that some of the other Image tests failed when I ran them, but it may be a path issue since I'm running them on Windows versus a library problem).

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 624d9b8 on mairsbw:master into * on python-pillow:master*.

@mairsbw
Copy link
Contributor Author

mairsbw commented Apr 9, 2016

Looks like Travis has a configuration issue and missing a library (sphinx_rtd_theme).

@wiredfool
Copy link
Member

These are actually failing in the tests as well: https://travis-ci.org/python-pillow/Pillow/jobs/121929495#L2747

======================================================================
FAIL: TestNumpy.test_numpy_to_image
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/python-pillow/Pillow/Tests/test_numpy.py", line 51, in test_numpy_to_image
    self.assertRaises(TypeError, lambda: to_image(numpy.uint16))
AssertionError: TypeError not raised by <lambda>
----------------------------------------------------------------------
Ran 674 tests in 22.647s
FAILED (SKIP=24, failures=1)

The docs, well for some reason breaking there isn't breaking the build, and a recent change broke them.

@mairsbw
Copy link
Contributor Author

mairsbw commented Apr 9, 2016

Looks like my test successfully passed in Travis anyways.

Is there anything else I need to do to get an r+ on this?

@@ -2206,6 +2206,8 @@ def fromqpixmap(im):
# first two members of shape are set to one
# ((1, 1), "|b1"): ("1", "1"), # broken
((1, 1), "|u1"): ("L", "L"),
((1, 1), "<u2"): ("L", "I;16"),
((1, 1), ">u2"): ("L", "I;16B"),
((1, 1), "|i1"): ("I", "I;8"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two unpackers are only taking the upper byte and throwing away the lower byte, and packing this into a single channel 8 bit image. In the process, they're interpreting the upper byte as a uint8, which is unlikely to be correct if it's a signed int type.

I think that the correct mode, raw_mod tuples to use would be ('I', 'I;16S') and ('I', 'I;16BS'). These two promote the image to a 32 bit image, which is enough range to losslessly contain the I16S datatype.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual datatype is a uint16, so there's no dataloss with the uint8 issue you talk about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, I believe the "I" means that it's a color image, but in this case it's not. I don't quite understand all of these special strings that Pillow uses, so maybe I'm wrong here. I do believe this code is working as shown by my testcase. You can fully roundtrip a uint16 image with no dataloss.

@mairsbw
Copy link
Contributor Author

mairsbw commented Apr 10, 2016

I also don't know how to get unittest to stop complaining about unclosed files. I've added code to close the images in my test case but it still complains. Does anyone know the right way to do that?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 492bd0e on mairsbw:master into * on python-pillow:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 492bd0e on mairsbw:master into * on python-pillow:master*.

@wiredfool
Copy link
Member

  1. The code section you changed is the fromarray definitions, which are the interface to numpy.
  2. PIL.Image.fromarray fails with uint16 arrays #1514 is a bug/feature request for interoperation with numpy.
  3. I is a 32 bit single channel image, L is a single channel 8 bit image, color images are RGB, CMYK, or similar.
  4. The code for the fromarray changes that you added do allow for a uint16 image to be passed from numpy using the array interface to pillow, but it ends up as an 8bit image using the most significant byte, throwing out the lower byte. This is not something that I'd want to merge.
  5. The test roundtrips a 16bit image, which we do generally support, as indicated by the test passing on master without the changes. There are some specific 16 bit formats (like signed 16bit integers) that aren't supported.

I think fundamentally, you have a different problem than you're trying to solve here.

@radarhere
Copy link
Member

With regards to unclosed files, from my experience, this occurs for many other parts of Pillow, so don't consider it a blocker for this. If you like, you can create a new issue to discuss the problem.

@mairsbw
Copy link
Contributor Author

mairsbw commented Apr 11, 2016

It sounds to me like there should be the addition of a 16-bit single-channel image type, but I still don't quite understand how numpy and Pillow communicate here. If I read in an image I'd expect the array to maintain the same datatype as the file.

@wiredfool Could you take a look at the two tests I now have in this branch in d6c9ac1? One tests just the round-tripping of a TIF (which fails in Pillow 3.1, which I had installed). Additionally it adds another tests that checks creating a 16-bit TIFF from a uint16 numpy array and making sure it is read back as the same data. I don't know if that second tests should be there or in test_numpy.py or one of the test_image_*.py files.

@radarhere
Copy link
Member

Hi. I'll note here that #1984 has been merged, which replaced #1824, relating to this.

@wiredfool
Copy link
Member

I'm not sure this really applies anymore -- but if it does, feel free to rebase and do a new pr.

@wiredfool wiredfool closed this Jun 29, 2016
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 this pull request may close these issues.

4 participants