-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Ensure DPI key exists in image info before accessing. #123
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
Conversation
|
It actually may make more sense to just assume inches and return 'dots_per_unit' when no unit is given... |
|
Hmm, interesting, thanks for reporting this Colin. The Resolution Unit field is a required field according to the TIFF 6 spec. But it appears iOS has its own ideas :) It might make sense to report that to them as a bug. Either way you're right to think we should code defensively here. What was the value your images had for the ImageLength and ImageWidth tags? Did they have values that seemed to imply inches was the intended value for the ResolutionUnit field (like 72 or 300 for example)? I'm inclined to make the "fix" as narrow as possible. Making the software "helpful" has a nasty way of biting back. |
|
That is what I was seeing, and the docx library was throwing KeyError exceptions when trying to use these images. However, let me do a bit of digging and ensure it is Apple's fault...it could be that a service I have been using to transmit the images has been messing with the image's metadata (which seems just wrong!). Sorry I probably should have thought of this before making a pull request, I will follow up. Cheers. |
|
Interesting thing I found in the spec. It was gently nagging me with uncertainty, I must have caught the contradiction subconsciously :) While it's true the TIFF6 spec defines 'ResolutionUnit' as a required field, in the details of the field itself it states that the default value for that field is 2 (inches). Now how a required field can have a default I'm unable to discern, but it seems clear that the right solution is to not barf on ResolutionUnit being missing, rather to default to inches. So I take back any unkind words I may have had for the iOS folks :) Thanks again for catching this Colin. I'll get it into the upcoming release, should be a couple weeks at most. |
|
Say, by the way, if you change your mind on a pull request (PR), you don't have to create a new one. Just add the fix on the same branch and rebase to groom your previous change out of the branch. The PR points to your branch rather than a particular commit. Actually it points from your tip of that branch to the first shared commit. That's why only your new ones show up. It's also best practice to create a new, dedicated branch for your PR rather than doing it on master or even develop. Maybe a name like fix/tiff-res. This SO page has some interesting details: Avoids unnecessary clutter on the issues list :) |
|
Thanks Steve, interesting stuff! I am going to do the git courses at codeschool.org and hopefully that will help get by git skills up to snuff. Thanks for the additional material. Hope I can contribute some more to the project in the future...with proper repo management ;) |
|
Hi Colin, you might want to check out this Git resource: https://jwiegley.github.io/git-from-the-bottom-up/ I found it removed the mystery from Git for me once I understood what was going on under the covers. Especially about rebasing, which was a completely black art for me before reading this :) There's a PDF here that's easier for offfline reading: |


I was having trouble producing word documents that included images taken with an iPad running iOS 8.1.2. I realized it's because this release of iOS does not include the 'Resolution Unit' key in the TIFF info of it's photos. Adding this simple check fixes it by assuming a safe value for the DPI when it not given in the image info...