-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 resolution option for PNG format #1179
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat 👍
if (buf[i] === 0x70 && | ||
buf[i + 1] === 0x48 && | ||
buf[i + 2] === 0x59 && | ||
buf[i + 3] === 0x73) { // pHYs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
@zbjornson I do not see this working for me? Don't you need to update the |
Hrm it looks like I lost the changes to canvas.cc that were supposed to be in this PR. The test passes because pHYs is omitted if resolution isn't specified. Will fix tonight. Thanks for pointing that out. |
@zbjornson Awesome thanks! Much appreciated. |
Did this make it onto another PR? The resolution option for png appears to do nothing... |
@mikehalmamoj yes, #1376. As an ancillary chunk, readers are allowed to ignore pHYs. That might be why you don't see it doing anything. Above shows that the chunk is encoded in the file correctly though. |
Thanks for the quick reply @zbjornson. That's ever so strange because I don't see the resolution in the image info when generating from this lib but if I add a resolution with the pngcrush CLI then I see the that resolution. 🤷♂️ I'll have to do some more digging. Thanks again! |
You're absolutely correct and it works fine - it seems the issue is the haphazard support for the |
Edge, Firefox and Chrome don't specify a
pHYs
chunk, so I left default behavior unchanged to stay consistent with them. (The internets say that the resolution doesn't mean much for on-screen display anyway.)Fixes #766
Fixes #716
Thanks for contributing!