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

DPI is a tuple #2472

Merged
merged 11 commits into from Apr 3, 2017
Merged

DPI is a tuple #2472

merged 11 commits into from Apr 3, 2017

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Apr 1, 2017

Fix merged PR #2449, re: 977f319#commitcomment-21546844

DPI should be an (xres, yres) tuple.

Confirmed in code and docs:

The open() method may set the following info properties if available:
...
dpi
A tuple representing the reported pixel density in pixels per inch, if the file is a jfif file and the units are in inches.
...
The save() method supports the following options:
...
dpi
A tuple of integers representing the pixel density, (x,y).

Note: one test image (Tests/images/exif_gps.jpg) only has an X value, and so raised exceptions, so if there's no Y value, use the X twice.

hugovk referenced this pull request Apr 1, 2017
If DPI isn't in JPEG header, fetch from EXIF
x_resolution = self._getexif()[0x011A]
self.info["dpi"] = x_resolution[0] / x_resolution[1]
exif = self._getexif()
x_resolution = exif[0x011A]
Copy link
Member

Choose a reason for hiding this comment

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

This could fail with a keyerror?

Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically, yes. I'll wrap in another try/except later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, but see below.

@hugovk
Copy link
Member Author

hugovk commented Apr 1, 2017

It's better to check the spec before implementing things...

http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/EXIF.html says:

  • Exif.Image.XResolution: "The number of pixels per <ResolutionUnit> in the <ImageWidth> direction. When the image resolution is unknown, 72 [dpi] is designated."
  • Exif.Image.YResolution: "The number of pixels per <ResolutionUnit> in the <ImageLength> direction. The same value as <XResolution> is designated."
  • Exif.Image.ResolutionUnit: "The unit for measuring <XResolution> and <YResolution>. The same unit is used for both <XResolution> and <YResolution>. If the image resolution is unknown, 2 (inches) is designated."

And http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/EXIF.html gives these for resolution unit:

(the value 1 is not standard EXIF)
1 = None
2 = inches
3 = cm

So I'll update this again:

  • we don't need to check for YResolution, it's the same as XResolution - is that correct?
  • if no value, default to 72,72 dpi
  • should also check the unit: if inches, do nothing; if cm, convert to inches; else default to inches

@wiredfool
Copy link
Member

@hugovk , you happy with this?

@hugovk
Copy link
Member Author

hugovk commented Apr 3, 2017

@wiredfool Yes.

Feel free to squash and merge if you'd like to tidy up the commits.

image

@wiredfool
Copy link
Member

someone is screen shotting on a high dpi monitor...

@wiredfool wiredfool merged commit 53df626 into python-pillow:master Apr 3, 2017
@hugovk hugovk deleted the dpi-is-a-tuple branch April 4, 2017 03:15
@hugovk
Copy link
Member Author

hugovk commented Apr 4, 2017

Yes, that was a surprisingly large image!

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.

None yet

2 participants